From 9c6580ccd2365c3681d35af9e30e79d077a8676c Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 14 Jan 2022 10:48:23 +0100 Subject: [PATCH] Fix macOS SCEP client issues Fixes #746 --- authority/authority.go | 14 +++++----- authority/provisioner/scep.go | 38 ++++++++++++++++++++++++--- ca/ca.go | 22 ++++++++++++---- scep/api/api.go | 19 ++++++++++---- scep/authority.go | 48 +++++++++++++++++++++++------------ scep/common.go | 4 +-- scep/provisioner.go | 2 ++ 7 files changed, 108 insertions(+), 39 deletions(-) diff --git a/authority/authority.go b/authority/authority.go index b6fcdf23..42e21149 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -437,13 +437,6 @@ func (a *Authority) init() error { } } - // Check if a KMS with decryption capability is required and available - if a.requiresDecrypter() { - if _, ok := a.keyManager.(kmsapi.Decrypter); !ok { - return errors.New("keymanager doesn't provide crypto.Decrypter") - } - } - // TODO: decide if this is a good approach for providing the SCEP functionality // It currently mirrors the logic for the x509CAService if a.requiresSCEPService() && a.scepService == nil { @@ -454,6 +447,7 @@ func (a *Authority) init() error { if err != nil { return err } + options.CertificateChain = append(options.CertificateChain, a.rootX509Certs...) options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ SigningKey: a.config.IntermediateKey, Password: []byte(a.password), @@ -601,6 +595,12 @@ func (a *Authority) IsRevoked(sn string) (bool, error) { return a.db.IsRevoked(sn) } +// GetIntermediateCertificate returns the x509 CA intermediate +// 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 145a1920..0531bd10 100644 --- a/authority/provisioner/scep.go +++ b/authority/provisioner/scep.go @@ -18,13 +18,20 @@ 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"` // MinimumPublicKeyLength is the minimum length for public keys in CSRs - MinimumPublicKeyLength int `json:"minimumPublicKeyLength,omitempty"` - Options *Options `json:"options,omitempty"` - Claims *Claims `json:"claims,omitempty"` - claimer *Claimer + MinimumPublicKeyLength int `json:"minimumPublicKeyLength,omitempty"` + // Numerical identifier for the ContentEncryptionAlgorithm as defined in github.com/mozilla-services/pkcs7 + // at https://github.com/mozilla-services/pkcs7/blob/33d05740a3526e382af6395d3513e73d4e66d1cb/encrypt.go#L63 + // Defaults to 2, being AES-256-CBC + EncryptionAlgorithmIdentifier *int `json:"encryptionAlgorithmIdentifier,omitempty"` + Options *Options `json:"options,omitempty"` + Claims *Claims `json:"claims,omitempty"` + claimer *Claimer secretChallengePassword string + encryptionAlgorithm int } // GetID returns the provisioner unique identifier. @@ -100,6 +107,15 @@ func (s *SCEP) Init(config Config) (err error) { return errors.Errorf("only minimum public keys exactly divisible by 8 are supported; %d is not exactly divisible by 8", s.MinimumPublicKeyLength) } + s.encryptionAlgorithm = 2 // default to AES-256-CBC + if s.EncryptionAlgorithmIdentifier != nil { + value := *s.EncryptionAlgorithmIdentifier + if value < 0 || value > 4 { + return errors.Errorf("only encryption algorithm identifiers from 0 to 4 are valid") + } + s.encryptionAlgorithm = value + } + // TODO: add other, SCEP specific, options? return err @@ -129,3 +145,17 @@ func (s *SCEP) GetChallengePassword() string { func (s *SCEP) GetCapabilities() []string { return s.Capabilities } + +// ShouldIncludeRootsInChain 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 +} + +// GetContentEncryptionAlgorithm returns the numeric identifier +// for the pkcs7 package encryption algorithm to use. +func (s *SCEP) GetContentEncryptionAlgorithm() int { + return s.encryptionAlgorithm +} diff --git a/ca/ca.go b/ca/ca.go index da0fb874..a07ae406 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -417,11 +417,6 @@ func (ca *CA) getTLSConfig(auth *authority.Authority) (*tls.Config, error) { } } - certPool := x509.NewCertPool() - for _, crt := range auth.GetRootCertificates() { - certPool.AddCert(crt) - } - // GetCertificate will only be called if the client supplies SNI // information or if tlsConfig.Certificates is empty. // When client requests are made using an IP address (as opposed to a domain @@ -432,6 +427,23 @@ func (ca *CA) getTLSConfig(auth *authority.Authority) (*tls.Config, error) { tlsConfig.Certificates = []tls.Certificate{} tlsConfig.GetCertificate = ca.renewer.GetCertificateForCA + certPool := x509.NewCertPool() + for _, crt := range auth.GetRootCertificates() { + certPool.AddCert(crt) + } + + // adding the intermediate CA to the pool will allow clients that + // fail to send the intermediate for chain building to connect to the CA + // successfully. + shouldAddIntermediateToClientCAPool := true // TODO(hs): make this into a configuration + if shouldAddIntermediateToClientCAPool { + cert, err := auth.GetIntermediateCertificate() + if err != nil { + return nil, err + } + certPool.AddCert(cert) + } + // Add support for mutual tls to renew certificates tlsConfig.ClientAuth = tls.VerifyClientCertIfGiven tlsConfig.ClientCAs = certPool diff --git a/scep/api/api.go b/scep/api/api.go index 0c8c469b..d1ade2aa 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -212,7 +212,7 @@ func (h *Handler) lookupProvisioner(next nextHTTP) nextHTTP { // GetCACert returns the CA certificates in a SCEP response func (h *Handler) GetCACert(ctx context.Context) (SCEPResponse, error) { - certs, err := h.Auth.GetCACertificates() + certs, err := h.Auth.GetCACertificates(ctx) if err != nil { return SCEPResponse{}, err } @@ -289,20 +289,29 @@ func (h *Handler) PKIOperation(ctx context.Context, request SCEPRequest) (SCEPRe // NOTE: at this point we have sufficient information for returning nicely signed CertReps csr := msg.CSRReqMessage.CSR - if msg.MessageType == microscep.PKCSReq { - + // NOTE: we're blocking the RenewalReq if the challenge does not match, because otherwise we don't have any authentication. + // The macOS SCEP client performs renewals using PKCSreq. The CertNanny SCEP client will use PKCSreq with challenge too, it seems, + // even if using the renewal flow as described in the README.md. MicroMDM SCEP client also only does PKCSreq by default, unless + // a certificate exists; then it will use RenewalReq. Adding the challenge check here may be a small breaking change for clients. + // We'll have to see how it works out. + if msg.MessageType == microscep.PKCSReq || msg.MessageType == microscep.RenewalReq { challengeMatches, err := h.Auth.MatchChallengePassword(ctx, msg.CSRReqMessage.ChallengePassword) if err != nil { return h.createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("error when checking password")) } - if !challengeMatches { // TODO: can this be returned safely to the client? In the end, if the password was correct, that gains a bit of info too. return h.createFailureResponse(ctx, csr, msg, microscep.BadRequest, errors.New("wrong password provided")) } } - // TODO: check if CN already exists, if renewal is allowed and if existing should be revoked; fail if not + // TODO: authorize renewal: we can authorize renewals with the challenge password (if reusable secrets are used). + // Renewals OPTIONALLY include the challenge if the existing cert is used as authentication, but client SHOULD omit the challenge. + // This means that for renewal requests we should check the certificate provided to be signed before by the CA. We could + // enforce use of the challenge if we want too. That way we could be more flexible in terms of authentication scheme (i.e. reusing + // tokens from other provisioners, calling a webhook, storing multiple secrets, allowing them to be multi-use, etc). + // Authentication by the (self-signed) certificate with an optional challenge is required; supporting renewals incl. verification + // of the client cert is not. certRep, err := h.Auth.SignCSR(ctx, csr, msg) if err != nil { diff --git a/scep/authority.go b/scep/authority.go index 47d1d41c..04e77160 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -23,7 +23,7 @@ type Interface interface { LoadProvisionerByID(string) (provisioner.Interface, error) GetLinkExplicit(provName string, absoluteLink bool, baseURL *url.URL, inputs ...string) string - GetCACertificates() ([]*x509.Certificate, error) + GetCACertificates(ctx context.Context) ([]*x509.Certificate, error) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) error SignCSR(ctx context.Context, csr *x509.CertificateRequest, msg *PKIMessage) (*PKIMessage, error) CreateFailureResponse(ctx context.Context, csr *x509.CertificateRequest, msg *PKIMessage, info FailInfoName, infoText string) (*PKIMessage, error) @@ -36,6 +36,7 @@ type Authority struct { prefix string dns string intermediateCertificate *x509.Certificate + caCerts []*x509.Certificate // TODO(hs): change to use these instead of root and intermediate service *Service signAuth SignAuthority } @@ -72,6 +73,8 @@ func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) { // in its entirety to make this more interoperable with the rest of // step-ca, I think. if ops.Service != nil { + authority.caCerts = ops.Service.certificateChain + // TODO(hs): look into refactoring SCEP into using just caCerts everywhere, if it makes sense for more elaborate SCEP configuration. Keeping it like this for clarity (for now). authority.intermediateCertificate = ops.Service.certificateChain[0] authority.service = ops.Service } @@ -82,7 +85,7 @@ func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) { var ( // TODO: check the default capabilities; https://tools.ietf.org/html/rfc8894#section-3.5.2 defaultCapabilities = []string{ - "Renewal", + "Renewal", // NOTE: removing this will result in macOS SCEP client stating the server doesn't support renewal, but it uses PKCSreq to do so. "SHA-1", "SHA-256", "AES", @@ -100,18 +103,13 @@ func (a *Authority) LoadProvisionerByID(id string) (provisioner.Interface, error // GetLinkExplicit returns the requested link from the directory. func (a *Authority) GetLinkExplicit(provName string, abs bool, baseURL *url.URL, inputs ...string) string { - // TODO: taken from ACME; move it to directory (if we need a directory in SCEP)? return a.getLinkExplicit(provName, abs, baseURL, inputs...) } // getLinkExplicit returns an absolute or partial path to the given resource and a base // URL dynamically obtained from the request for which the link is being calculated. func (a *Authority) getLinkExplicit(provisionerName string, abs bool, baseURL *url.URL, inputs ...string) string { - - // TODO: do we need to provide a way to provide a different suffix? - // Like "/cgi-bin/pkiclient.exe"? Or would it be enough to have that as the name? link := "/" + provisionerName - if abs { // Copy the baseURL value from the pointer. https://github.com/golang/go/issues/38351 u := url.URL{} @@ -137,7 +135,7 @@ func (a *Authority) getLinkExplicit(provisionerName string, abs bool, baseURL *u } // GetCACertificates returns the certificate (chain) for the CA -func (a *Authority) GetCACertificates() ([]*x509.Certificate, error) { +func (a *Authority) GetCACertificates(ctx context.Context) ([]*x509.Certificate, error) { // TODO: this should return: the "SCEP Server (RA)" certificate, the issuing CA up to and excl. the root // Some clients do need the root certificate however; also see: https://github.com/openxpki/openxpki/issues/73 @@ -153,14 +151,27 @@ func (a *Authority) GetCACertificates() ([]*x509.Certificate, error) { // Using an RA does not seem to exist in https://tools.ietf.org/html/rfc8894, but is mentioned in // https://tools.ietf.org/id/draft-nourse-scep-21.html. Will continue using the CA directly for now. // - // The certificate to use should probably depend on the (configured) Provisioner and may + // The certificate to use should probably depend on the (configured) provisioner and may // use a distinct certificate, apart from the intermediate. - if a.intermediateCertificate == nil { + p, err := provisionerFromContext(ctx) + if err != nil { + return nil, err + } + + if len(a.caCerts) == 0 { return nil, errors.New("no intermediate certificate available in SCEP authority") } - return []*x509.Certificate{a.intermediateCertificate}, nil + certs := []*x509.Certificate{} + certs = append(certs, a.caCerts[0]) + + // TODO(hs): we're adding the roots here, but they may be something different than what the RFC means. Clients are responsible to select the right cert(s) to use, though. + if p.ShouldIncludeRootsInChain() && len(a.caCerts) >= 2 { + certs = append(certs, a.caCerts[1:]...) + } + + return certs, nil } // DecryptPKIEnvelope decrypts an enveloped message @@ -211,8 +222,6 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err // SignCSR creates an x509.Certificate based on a CSR template and Cert Authority credentials // returns a new PKIMessage with CertRep data -//func (msg *PKIMessage) SignCSR(crtAuth *x509.Certificate, keyAuth *rsa.PrivateKey, template *x509.Certificate) (*PKIMessage, error) { -//func (a *Authority) SignCSR(ctx context.Context, msg *PKIMessage, template *x509.Certificate) (*PKIMessage, error) { func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, msg *PKIMessage) (*PKIMessage, error) { // TODO: intermediate storage of the request? In SCEP it's possible to request a csr/certificate @@ -220,7 +229,7 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m // poll for the status. It seems to be similar as what can happen in ACME, so might want to model // the implementation after the one in the ACME authority. Requires storage, etc. - p, err := ProvisionerFromContext(ctx) + p, err := provisionerFromContext(ctx) if err != nil { return nil, err } @@ -292,10 +301,17 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m return nil, err } + // apparently the pkcs7 library uses a global default setting for the content encryption + // algorithm to use when en- or decrypting data. We need to restore the current setting after + // the cryptographic operation, so that other usages of the library are not influenced by + // this call to Encrypt(). + encryptionAlgorithmToRestore := pkcs7.ContentEncryptionAlgorithm + pkcs7.ContentEncryptionAlgorithm = p.GetContentEncryptionAlgorithm() e7, err := pkcs7.Encrypt(deg, msg.P7.Certificates) if err != nil { return nil, err } + pkcs7.ContentEncryptionAlgorithm = encryptionAlgorithmToRestore // PKIMessageAttributes to be signed config := pkcs7.SignerInfoConfig{ @@ -434,7 +450,7 @@ func (a *Authority) CreateFailureResponse(ctx context.Context, csr *x509.Certifi // MatchChallengePassword verifies a SCEP challenge password func (a *Authority) MatchChallengePassword(ctx context.Context, password string) (bool, error) { - p, err := ProvisionerFromContext(ctx) + p, err := provisionerFromContext(ctx) if err != nil { return false, err } @@ -453,7 +469,7 @@ func (a *Authority) MatchChallengePassword(ctx context.Context, password string) // GetCACaps returns the CA capabilities func (a *Authority) GetCACaps(ctx context.Context) []string { - p, err := ProvisionerFromContext(ctx) + p, err := provisionerFromContext(ctx) if err != nil { return defaultCapabilities } diff --git a/scep/common.go b/scep/common.go index ca87841f..73b16ed4 100644 --- a/scep/common.go +++ b/scep/common.go @@ -14,9 +14,9 @@ const ( ProvisionerContextKey = ContextKey("provisioner") ) -// ProvisionerFromContext searches the context for a SCEP provisioner. +// provisionerFromContext searches the context for a SCEP provisioner. // Returns the provisioner or an error. -func ProvisionerFromContext(ctx context.Context) (Provisioner, error) { +func provisionerFromContext(ctx context.Context) (Provisioner, error) { val := ctx.Value(ProvisionerContextKey) if val == nil { return nil, errors.New("provisioner expected in request context") diff --git a/scep/provisioner.go b/scep/provisioner.go index e1b7f8b1..75820f56 100644 --- a/scep/provisioner.go +++ b/scep/provisioner.go @@ -16,4 +16,6 @@ type Provisioner interface { GetOptions() *provisioner.Options GetChallengePassword() string GetCapabilities() []string + ShouldIncludeRootsInChain() bool + GetContentEncryptionAlgorithm() int }