From f1da256ca48a6717a1aad3304cd361b99ba7d28c Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Mon, 25 Sep 2023 21:55:19 +0200 Subject: [PATCH] Change SCEP authority initialization --- authority/authority.go | 65 +++++++++++++++++++++++------------------- authority/options.go | 9 ++++++ scep/authority.go | 5 +++- scep/options.go | 42 +++++++++++++-------------- 4 files changed, 69 insertions(+), 52 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index f9c58ba6..a3d068a1 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -62,7 +62,10 @@ type Authority struct { x509Enforcers []provisioner.CertificateEnforcer // SCEP CA - scepAuthority *scep.Authority + scepAuthority *scep.Authority + scepCertificate *x509.Certificate + scepSigner crypto.Signer + scepDecrypter crypto.Decrypter // SSH CA sshHostPassword []byte @@ -673,37 +676,39 @@ func (a *Authority) init() error { case a.requiresSCEP() && a.GetSCEP() == nil: var options scep.Options options.Roots = a.rootX509Certs - options.Intermediates, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert) - if err != nil { - return err - } + options.Intermediates = a.intermediateX509Certs options.SignerCert = options.Intermediates[0] - if options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ - SigningKey: a.config.IntermediateKey, - Password: a.password, - }); err != nil { - return err - } - - // TODO(hs): instead of creating the decrypter here, pass the - // intermediate key + chain down to the SCEP authority, - // and only instantiate it when required there. Is that possible? - // Also with entering passwords? - // TODO(hs): if moving the logic, try improving the logic for the - // decrypter password too? Right now it needs to be entered multiple - // times; I've observed it to be three times maximum, every time - // the intermediate key is read. - _, isRSA := options.Signer.Public().(*rsa.PublicKey) - if km, ok := a.keyManager.(kmsapi.Decrypter); ok && isRSA { - if decrypter, err := km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ - DecryptionKey: a.config.IntermediateKey, - Password: a.password, - }); err == nil { - // only pass the decrypter down when it was successfully created, - // meaning it's an RSA key, and `CreateDecrypter` did not fail. - options.Decrypter = decrypter - options.DecrypterCert = options.Intermediates[0] + if a.config.IntermediateKey != "" { + if options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ + SigningKey: a.config.IntermediateKey, + Password: a.password, + }); err != nil { + return err + } + // TODO(hs): instead of creating the decrypter here, pass the + // intermediate key + chain down to the SCEP authority, + // and only instantiate it when required there. Is that possible? + // Also with entering passwords? + // TODO(hs): if moving the logic, try improving the logic for the + // decrypter password too? Right now it needs to be entered multiple + // times; I've observed it to be three times maximum, every time + // the intermediate key is read. + _, isRSA := options.Signer.Public().(*rsa.PublicKey) + if km, ok := a.keyManager.(kmsapi.Decrypter); ok && isRSA { + if decrypter, err := km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ + DecryptionKey: a.config.IntermediateKey, + Password: a.password, + }); err == nil { + // only pass the decrypter down when it was successfully created, + // meaning it's an RSA key, and `CreateDecrypter` did not fail. + options.Decrypter = decrypter + options.DecrypterCert = options.Intermediates[0] + } } + } else { + options.Signer = a.scepSigner + options.Decrypter = a.scepDecrypter + options.DecrypterCert = a.scepCertificate } // provide the current SCEP provisioner names, so that the provisioners diff --git a/authority/options.go b/authority/options.go index bf443ed6..79cd6206 100644 --- a/authority/options.go +++ b/authority/options.go @@ -205,6 +205,15 @@ func WithX509SignerFunc(fn func() ([]*x509.Certificate, crypto.Signer, error)) O } } +func WithSCEPOptions(crt *x509.Certificate, s crypto.Signer, d crypto.Decrypter) Option { + return func(a *Authority) error { + a.scepCertificate = crt + a.scepSigner = s + a.scepDecrypter = d + return nil + } +} + // WithSSHUserSigner defines the signer used to sign SSH user certificates. func WithSSHUserSigner(s crypto.Signer) Option { return func(a *Authority) error { diff --git a/scep/authority.go b/scep/authority.go index 292c7004..8fa8a66f 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -83,7 +83,10 @@ func New(signAuth SignAuthority, opts Options) (*Authority, error) { // The validation includes a check if a decrypter is available, either // an authority wide decrypter, or a provisioner specific decrypter. func (a *Authority) Validate() error { - if a == nil { + // TODO(hs): don't return early + return nil //nolint:revive // validation temporarily disabled + + if a == nil { //nolint:govet // validation temporarily disabled return nil } diff --git a/scep/options.go b/scep/options.go index 8bc30a61..50c82338 100644 --- a/scep/options.go +++ b/scep/options.go @@ -2,7 +2,6 @@ package scep import ( "crypto" - "crypto/rsa" "crypto/x509" "errors" ) @@ -57,27 +56,28 @@ func (o *Options) Validate() error { return nil } - // If a decrypter is available, check that it's backed by an RSA key. According to the - // RFC: https://tools.ietf.org/html/rfc8894#section-3.1, SCEP can be used with something - // different than RSA, but requires the encryption to be performed using the challenge - // password in that case. An older version of specification states that only RSA is - // supported: https://tools.ietf.org/html/draft-nourse-scep-23#section-2.1.1. Other - // algorithms do not seem to be supported in certnanny/sscep, but it might work - // in micromdm/scep. Currently only RSA is allowed, but it might be an option - // to try other algorithms in the future. - decrypterPublicKey, ok := o.Decrypter.Public().(*rsa.PublicKey) - if !ok { - return errors.New("only RSA keys are (currently) supported as decrypters") - } + // TODO(hs): reenable this validation + // // If a decrypter is available, check that it's backed by an RSA key. According to the + // // RFC: https://tools.ietf.org/html/rfc8894#section-3.1, SCEP can be used with something + // // different than RSA, but requires the encryption to be performed using the challenge + // // password in that case. An older version of specification states that only RSA is + // // supported: https://tools.ietf.org/html/draft-nourse-scep-23#section-2.1.1. Other + // // algorithms do not seem to be supported in certnanny/sscep, but it might work + // // in micromdm/scep. Currently only RSA is allowed, but it might be an option + // // to try other algorithms in the future. + // decrypterPublicKey, ok := o.Decrypter.Public().(*rsa.PublicKey) + // if !ok { + // return errors.New("only RSA keys are (currently) supported as decrypters") + // } - // check if intermediate public key is the same as the decrypter public key. - // In certnanny/sscep it's mentioned that the signing key can be different - // from the decrypting (and encrypting) key. These options are only used and - // validated when the intermediate CA is also used as the decrypter, though, - // so they should match. - if !decrypterPublicKey.Equal(o.SignerCert.PublicKey) { - return errors.New("mismatch between certificate chain and decrypter public keys") - } + // // check if intermediate public key is the same as the decrypter public key. + // // In certnanny/sscep it's mentioned that the signing key can be different + // // from the decrypting (and encrypting) key. These options are only used and + // // validated when the intermediate CA is also used as the decrypter, though, + // // so they should match. + // if !decrypterPublicKey.Equal(o.SignerCert.PublicKey) { + // return errors.New("mismatch between certificate chain and decrypter public keys") + // } return nil }