From c0a1837cd9d31f9db253befabfe751061a1abfd4 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 3 Aug 2023 16:09:51 +0200 Subject: [PATCH] Verify full decrypter/signer configuration at usage time When changing the SCEP configuration it is possible that one or both of the decrypter configurations required are not available or have been provided in a way that's not usable for actual SCEP requests. Instead of failing hard when provisioners are loaded, which could result in the CA not starting properly, this type of problematic configuration errors will now be handled at usage time instead. --- authority/provisioner/scep.go | 48 ++++++++++++++++++++--------------- scep/authority.go | 46 ++++++++++++++++++--------------- 2 files changed, 53 insertions(+), 41 deletions(-) diff --git a/authority/provisioner/scep.go b/authority/provisioner/scep.go index 7bd213c6..626d26ab 100644 --- a/authority/provisioner/scep.go +++ b/authority/provisioner/scep.go @@ -3,6 +3,7 @@ package provisioner import ( "context" "crypto" + "crypto/rsa" "crypto/subtle" "crypto/x509" "encoding/pem" @@ -177,23 +178,23 @@ func (s *SCEP) Init(config Config) (err error) { if s.MinimumPublicKeyLength == 0 { s.MinimumPublicKeyLength = 2048 } - if s.MinimumPublicKeyLength%8 != 0 { return errors.Errorf("%d bits is not exactly divisible by 8", s.MinimumPublicKeyLength) } + // Set the encryption algorithm to use 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") } + // Prepare the SCEP challenge validator s.challengeValidationController = newChallengeValidationController( config.WebhookClient, s.GetOptions().GetWebhooks(), ) - skip := false // TODO(hs): remove this; currently a helper for debugging - if decryptionKey := s.DecrypterKey; decryptionKey != "" && !skip { + if decryptionKey := s.DecrypterKey; decryptionKey != "" { u, err := uri.Parse(s.DecrypterKey) if err != nil { return fmt.Errorf("failed parsing decrypter key: %w", err) @@ -226,7 +227,7 @@ func (s *SCEP) Init(config Config) (err error) { return fmt.Errorf("failed creating decrypter: %w", err) } if s.signer, err = s.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ - SigningKey: decryptionKey, // TODO(hs): support distinct signer key + SigningKey: decryptionKey, // TODO(hs): support distinct signer key in the future? Password: []byte(s.DecrypterKeyPassword), }); err != nil { return fmt.Errorf("failed creating signer: %w", err) @@ -248,23 +249,19 @@ func (s *SCEP) Init(config Config) (err error) { } // TODO(hs): alternatively, check if the KMS keyManager is a CertificateManager - // and load the certificate corresponding to the decryption key. - - // final validation for the decrypter - if s.decrypter != nil { - // // TODO(hs): enable this validation again - // if s.decrypterCertificate == nil { - // // TODO: don't hard skip at init? - // return fmt.Errorf("no decrypter certificate available for decrypter in %q", s.Name) - // } - // // validate the decrypter key - // decrypterPublicKey, ok := s.decrypter.Public().(*rsa.PublicKey) - // if !ok { - // return fmt.Errorf("only RSA keys are supported") - // } - // if !decrypterPublicKey.Equal(s.decrypterCertificate.PublicKey) { - // return errors.New("mismatch between decryption certificate and decrypter public keys") - // } + // and load the certificate corresponding to the decryption key? + + // Final validation for the decrypter. If both the decrypter and the certificate + // are available, the public keys must match. We currently allow the decrypter to + // be set without a certificate without warning the user, but + if s.decrypter != nil && s.decrypterCertificate != nil { + decrypterPublicKey, ok := s.decrypter.Public().(*rsa.PublicKey) + if !ok { + return fmt.Errorf("only RSA keys are supported") + } + if !decrypterPublicKey.Equal(s.decrypterCertificate.PublicKey) { + return errors.New("mismatch between decryption certificate and decrypter public keys") + } } // TODO: add other, SCEP specific, options? @@ -350,10 +347,19 @@ func (s *SCEP) selectValidationMethod() validationMethod { return validationMethodNone } +// GetDecrypter returns the provisioner specific decrypter, +// used to decrypt SCEP request messages sent by a SCEP client. +// The decrypter consists of a crypto.Decrypter (a private key) +// and a certificate for the public key corresponding to the +// private key. func (s *SCEP) GetDecrypter() (*x509.Certificate, crypto.Decrypter) { return s.decrypterCertificate, s.decrypter } +// GetSigner returns the provisioner specific signer, used to +// sign SCEP response messages for the client. The signer consists +// of a crypto.Signer and a certificate for the public key +// corresponding to the private key. func (s *SCEP) GetSigner() (*x509.Certificate, crypto.Signer) { return s.decrypterCertificate, s.signer } diff --git a/scep/authority.go b/scep/authority.go index 5fd7223b..c839ab99 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -486,19 +486,22 @@ func (a *Authority) ValidateChallenge(ctx context.Context, challenge, transactio func (a *Authority) selectDecrypter(ctx context.Context) (cert *x509.Certificate, pkey crypto.Decrypter, err error) { p := provisionerFromContext(ctx) - - // return provisioner specific decrypter, if available - if cert, pkey = p.GetDecrypter(); cert != nil && pkey != nil { + cert, pkey = p.GetDecrypter() + switch { + case cert != nil && pkey != nil: return + case cert == nil && pkey != nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a decrypter certificate available", p.GetName()) + case cert != nil && pkey == nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a decrypter available", p.GetName()) } - // fallback to the CA wide RSA decrypter, which is the - // intermediate CA. - cert = a.signerCertificate - pkey = a.defaultDecrypter - - if cert == nil || pkey == nil { - return nil, nil, fmt.Errorf("provisioner %q does not have a decrypter available", p.GetName()) + cert, pkey = a.signerCertificate, a.defaultDecrypter + switch { + case cert == nil && pkey != nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a default decrypter certificate available", p.GetName()) + case cert != nil && pkey == nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a default decrypter available", p.GetName()) } return @@ -506,19 +509,22 @@ func (a *Authority) selectDecrypter(ctx context.Context) (cert *x509.Certificate func (a *Authority) selectSigner(ctx context.Context) (cert *x509.Certificate, pkey crypto.PrivateKey, err error) { p := provisionerFromContext(ctx) - - // return provisioner specific signer, if available - if cert, pkey = p.GetSigner(); cert != nil && pkey != nil { + cert, pkey = p.GetSigner() + switch { + case cert != nil && pkey != nil: return + case cert == nil && pkey != nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a signer certificate available", p.GetName()) + case cert != nil && pkey == nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a signer available", p.GetName()) } - // fallback to the CA wide RSA signer, which is the - // intermediate CA. - cert = a.signerCertificate - pkey = a.defaultSigner - - if cert == nil || pkey == nil { - return nil, nil, fmt.Errorf("provisioner %q does not have a signer available", p.GetName()) + cert, pkey = a.signerCertificate, a.defaultSigner + switch { + case cert == nil && pkey != nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a default signer certificate available", p.GetName()) + case cert != nil && pkey == nil: + return nil, nil, fmt.Errorf("provisioner %q does not have a default signer available", p.GetName()) } return