Fix PR comments

authority/authority.go

@@ -595,12 +595,6 @@ func (a *Authority) IsRevoked(sn string) (bool, error) {
 	return a.db.IsRevoked(sn)
 }
 
-// GetIntermediateCertificate returns the x509 intermediate CA
-// certificate.
-func (a *Authority) GetIntermediateCertificate() (*x509.Certificate, error) {
-	return pemutil.ReadCertificate(a.config.IntermediateCert)
-}
-
 // requiresDecrypter returns whether the Authority
 // requires a KMS that provides a crypto.Decrypter
 // Currently this is only required when SCEP is

authority/provisioner/scep.go

@@ -18,8 +18,9 @@ type SCEP struct {
 	ForceCN           bool     `json:"forceCN,omitempty"`
 	ChallengePassword string   `json:"challenge,omitempty"`
 	Capabilities      []string `json:"capabilities,omitempty"`
-	// IncludeRoots makes the provisioner return the CA root(s) in the GetCACerts response
-	IncludeRoots bool `json:"includeRoots,omitempty"`
+	// IncludeRoot makes the provisioner return the CA root in addition to the
+	// intermediate in the GetCACerts response
+	IncludeRoot bool `json:"includeRoot,omitempty"`
 	// MinimumPublicKeyLength is the minimum length for public keys in CSRs
 	MinimumPublicKeyLength int `json:"minimumPublicKeyLength,omitempty"`
 	// Numerical identifier for the ContentEncryptionAlgorithm as defined in github.com/mozilla-services/pkcs7
@@ -107,7 +108,7 @@ func (s *SCEP) Init(config Config) (err error) {
 		return errors.Errorf("%d bits is not exactly divisible by 8", s.MinimumPublicKeyLength)
 	}
 
-	s.encryptionAlgorithm = s.EncryptionAlgorithmIdentifier
+	s.encryptionAlgorithm = s.EncryptionAlgorithmIdentifier // TODO(hs): we might want to upgrade the default security to AES-CBC?
 	if s.encryptionAlgorithm < 0 || s.encryptionAlgorithm > 4 {
 		return errors.New("only encryption algorithm identifiers from 0 to 4 are valid")
 	}
@@ -142,12 +143,12 @@ func (s *SCEP) GetCapabilities() []string {
 	return s.Capabilities
 }
 
-// ShouldIncludeRootsInChain indicates if the CA should
+// ShouldIncludeRootInChain indicates if the CA should
 // return its intermediate, which is currently used for
-// both signing and decryption, as well as the other certs
-// in its chain (usually a single root certificate).
-func (s *SCEP) ShouldIncludeRootsInChain() bool {
-	return s.IncludeRoots
+// both signing and decryption, as well as the root in
+// its chain.
+func (s *SCEP) ShouldIncludeRootInChain() bool {
+	return s.IncludeRoot
 }
 
 // GetContentEncryptionAlgorithm returns the numeric identifier

ca/ca.go

@@ -427,18 +427,23 @@ func (ca *CA) getTLSConfig(auth *authority.Authority) (*tls.Config, error) {
 	tlsConfig.Certificates = []tls.Certificate{}
 	tlsConfig.GetCertificate = ca.renewer.GetCertificateForCA
 
+	// initialize a certificate pool with root CA certificates to trust when doing mTLS.
 	certPool := x509.NewCertPool()
 	for _, crt := range auth.GetRootCertificates() {
 		certPool.AddCert(crt)
 	}
 
-	// adding the intermediate CA to the pool will allow clients that do not
-	// send the intermediate for chain building to connect to the CA successfully.
-	cert, err := auth.GetIntermediateCertificate()
-	if err != nil {
-		return nil, err
+	// adding the intermediate CA certificates to the pool will allow clients that
+	// do mTLS but don't send an intermediate to successfully connect. The intermediates
+	// added here are used when building a certificate chain.
+	intermediates := tlsCrt.Certificate[1:]
+	for _, certBytes := range intermediates {
+		cert, err := x509.ParseCertificate(certBytes)
+		if err != nil {
+			return nil, err
+		}
+		certPool.AddCert(cert)
 	}
-	certPool.AddCert(cert)
 
 	// Add support for mutual tls to renew certificates
 	tlsConfig.ClientAuth = tls.VerifyClientCertIfGiven

scep/authority.go

@@ -168,8 +168,8 @@ func (a *Authority) GetCACertificates(ctx context.Context) ([]*x509.Certificate,
 
 	// NOTE: we're adding the CA roots here, but they are (highly likely) different than what the RFC means.
 	// Clients are responsible to select the right cert(s) to use, though.
-	if p.ShouldIncludeRootsInChain() && len(a.caCerts) > 1 {
-		certs = append(certs, a.caCerts[1:]...)
+	if p.ShouldIncludeRootInChain() && len(a.caCerts) > 1 {
+		certs = append(certs, a.caCerts[1])
 	}
 
 	return certs, nil

scep/provisioner.go

@@ -16,6 +16,6 @@ type Provisioner interface {
 	GetOptions() *provisioner.Options
 	GetChallengePassword() string
 	GetCapabilities() []string
-	ShouldIncludeRootsInChain() bool
+	ShouldIncludeRootInChain() bool
 	GetContentEncryptionAlgorithm() int
 }