From 64680bb16dd076a33e3c1ad53b71b507825c6560 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 19 Jan 2022 11:31:33 +0100 Subject: [PATCH] Fix PR comments --- authority/authority.go | 6 ------ authority/provisioner/scep.go | 17 +++++++++-------- ca/ca.go | 17 +++++++++++------ scep/authority.go | 4 ++-- scep/provisioner.go | 2 +- 5 files changed, 23 insertions(+), 23 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index da0a6f6f..6624f11d 100644 --- a/authority/authority.go +++ b/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 diff --git a/authority/provisioner/scep.go b/authority/provisioner/scep.go index 9daf2e8b..5d67762c 100644 --- a/authority/provisioner/scep.go +++ b/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 diff --git a/ca/ca.go b/ca/ca.go index eab49b50..216acb97 100644 --- a/ca/ca.go +++ b/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 diff --git a/scep/authority.go b/scep/authority.go index 0003fd0e..f97bd55e 100644 --- a/scep/authority.go +++ b/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 diff --git a/scep/provisioner.go b/scep/provisioner.go index 75820f56..679c6353 100644 --- a/scep/provisioner.go +++ b/scep/provisioner.go @@ -16,6 +16,6 @@ type Provisioner interface { GetOptions() *provisioner.Options GetChallengePassword() string GetCapabilities() []string - ShouldIncludeRootsInChain() bool + ShouldIncludeRootInChain() bool GetContentEncryptionAlgorithm() int }