From 583d60dc0d076761e68123967e114e64bf293386 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sun, 21 Mar 2021 16:42:41 +0100 Subject: [PATCH] Address (most) PR comments --- authority/authority.go | 39 ++++++------- authority/provisioner/scep.go | 6 -- ca/ca.go | 36 ++++++------ cas/apiv1/options.go | 3 +- kms/apiv1/options.go | 2 + kms/apiv1/requests.go | 3 - scep/api/api.go | 28 ++++----- scep/authority.go | 106 ++++++++++------------------------ scep/certificate.go | 69 ++-------------------- scep/database.go | 7 +++ scep/options.go | 31 ++++++++++ scep/service.go | 13 +---- 12 files changed, 127 insertions(+), 216 deletions(-) create mode 100644 scep/database.go create mode 100644 scep/options.go diff --git a/authority/authority.go b/authority/authority.go index 19de5210..e5b62602 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -353,34 +353,29 @@ func (a *Authority) init() error { // 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 { - var options casapi.Options - if a.config.AuthorityConfig.Options != nil { - options = *a.config.AuthorityConfig.Options - } + var options scep.Options // Read intermediate and create X509 signer and decrypter for default CAS. - if options.Is(casapi.SoftCAS) { - options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert) - if err != nil { - return err - } - options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ - SigningKey: a.config.IntermediateKey, - Password: []byte(a.config.Password), + options.CertificateChain, err = pemutil.ReadCertificateBundle(a.config.IntermediateCert) + if err != nil { + return err + } + options.Signer, err = a.keyManager.CreateSigner(&kmsapi.CreateSignerRequest{ + SigningKey: a.config.IntermediateKey, + Password: []byte(a.config.Password), + }) + if err != nil { + return err + } + + if km, ok := a.keyManager.(kmsapi.Decrypter); ok { + options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ + DecryptionKey: a.config.IntermediateKey, + Password: []byte(a.config.Password), }) if err != nil { return err } - - if km, ok := a.keyManager.(kmsapi.Decrypter); ok { - options.Decrypter, err = km.CreateDecrypter(&kmsapi.CreateDecrypterRequest{ - DecryptionKey: a.config.IntermediateKey, - Password: []byte(a.config.Password), - }) - if err != nil { - return err - } - } } a.scepService, err = scep.NewService(context.Background(), options) diff --git a/authority/provisioner/scep.go b/authority/provisioner/scep.go index 49d057ff..6af3dc83 100644 --- a/authority/provisioner/scep.go +++ b/authority/provisioner/scep.go @@ -102,9 +102,3 @@ func (s *SCEP) GetChallengePassword() string { func (s *SCEP) GetCapabilities() []string { return s.Capabilities } - -// Interface guards -var ( - _ Interface = (*SCEP)(nil) - //_ scep.Provisioner = (*SCEP)(nil) -) diff --git a/ca/ca.go b/ca/ca.go index 8cc7f405..4d268c5b 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -151,11 +151,10 @@ func (ca *CA) Init(config *authority.Config) (*CA, error) { scepPrefix := "scep" scepAuthority, err := scep.New(auth, scep.AuthorityOptions{ - Service: auth.GetSCEPService(), - Backdate: *config.AuthorityConfig.Backdate, - DB: auth.GetDatabase().(nosql.DB), - DNS: dns, - Prefix: scepPrefix, + Service: auth.GetSCEPService(), + DB: auth.GetDatabase().(scep.DB), + DNS: dns, + Prefix: scepPrefix, }) if err != nil { return nil, errors.Wrap(err, "error creating SCEP authority") @@ -201,8 +200,10 @@ func (ca *CA) Init(config *authority.Config) (*CA, error) { ca.auth = auth ca.srv = server.New(config.Address, handler, tlsConfig) - // TODO: instead opt for having a single server.Server but two http.Servers - // handling the HTTP vs. HTTPS handler? + // TODO: instead opt for having a single server.Server but two + // http.Servers handling the HTTP and HTTPS handler? The latter + // will probably introduce more complexity in terms of graceful + // reload. if config.InsecureAddress != "" { ca.insecureSrv = server.New(config.InsecureAddress, insecureHandler, nil) } @@ -357,13 +358,14 @@ func (ca *CA) getTLSConfig(auth *authority.Authority) (*tls.Config, error) { return tlsConfig, nil } -// func dumpRoutes(mux chi.Routes) { -// // helpful routine for logging all routes // -// walkFunc := func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { -// fmt.Printf("%s %s\n", method, route) -// return nil -// } -// if err := chi.Walk(mux, walkFunc); err != nil { -// fmt.Printf("Logging err: %s\n", err.Error()) -// } -// } +//nolint // ignore linters to allow keeping this function around for debugging +func dumpRoutes(mux chi.Routes) { + // helpful routine for logging all routes // + walkFunc := func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error { + fmt.Printf("%s %s\n", method, route) + return nil + } + if err := chi.Walk(mux, walkFunc); err != nil { + fmt.Printf("Logging err: %s\n", err.Error()) + } +} diff --git a/cas/apiv1/options.go b/cas/apiv1/options.go index b6b0ca0d..46efae3b 100644 --- a/cas/apiv1/options.go +++ b/cas/apiv1/options.go @@ -24,8 +24,7 @@ type Options struct { // Certificate and signer are the issuer certificate,along with any other bundled certificates to be returned in the chain for consumers, and signer used in SoftCAS. // They are configured in ca.json crt and key properties. CertificateChain []*x509.Certificate - Signer crypto.Signer `json:"-"` - Decrypter crypto.Decrypter `json:"-"` + Signer crypto.Signer `json:"-"` // IsCreator is set to true when we're creating a certificate authority. Is // used to skip some validations when initializing a CertificateAuthority. diff --git a/kms/apiv1/options.go b/kms/apiv1/options.go index 1574a426..7cc7f748 100644 --- a/kms/apiv1/options.go +++ b/kms/apiv1/options.go @@ -16,6 +16,8 @@ type KeyManager interface { Close() error } +// Decrypter is an interface implemented by KMSes that are used +// in operations that require decryption type Decrypter interface { CreateDecrypter(req *CreateDecrypterRequest) (crypto.Decrypter, error) } diff --git a/kms/apiv1/requests.go b/kms/apiv1/requests.go index 31097040..f6fe7dd2 100644 --- a/kms/apiv1/requests.go +++ b/kms/apiv1/requests.go @@ -138,9 +138,6 @@ type CreateDecrypterRequest struct { Decrypter crypto.Decrypter DecryptionKey string DecryptionKeyPEM []byte - TokenLabel string - PublicKey string - PublicKeyPEM []byte Password []byte } diff --git a/scep/api/api.go b/scep/api/api.go index a9e4d840..cff3d32b 100644 --- a/scep/api/api.go +++ b/scep/api/api.go @@ -4,8 +4,6 @@ import ( "context" "crypto/x509" "encoding/base64" - "errors" - "fmt" "io" "io/ioutil" "net/http" @@ -18,6 +16,8 @@ import ( "github.com/smallstep/certificates/scep" "go.mozilla.org/pkcs7" + "github.com/pkg/errors" + microscep "github.com/micromdm/scep/scep" ) @@ -75,7 +75,7 @@ func (h *Handler) Get(w http.ResponseWriter, r *http.Request) { request, err := decodeSCEPRequest(r) if err != nil { - writeError(w, fmt.Errorf("not a scep get request: %w", err)) + writeError(w, errors.Wrap(err, "not a scep get request")) return } @@ -90,11 +90,11 @@ func (h *Handler) Get(w http.ResponseWriter, r *http.Request) { case opnPKIOperation: // TODO: implement the GET for PKI operation? Default CACAPS doesn't specify this is in use, though default: - err = fmt.Errorf("unknown operation: %s", request.Operation) + err = errors.Errorf("unknown operation: %s", request.Operation) } if err != nil { - writeError(w, fmt.Errorf("get request failed: %w", err)) + writeError(w, errors.Wrap(err, "get request failed")) return } @@ -106,7 +106,7 @@ func (h *Handler) Post(w http.ResponseWriter, r *http.Request) { request, err := decodeSCEPRequest(r) if err != nil { - writeError(w, fmt.Errorf("not a scep post request: %w", err)) + writeError(w, errors.Wrap(err, "not a scep post request")) return } @@ -117,11 +117,11 @@ func (h *Handler) Post(w http.ResponseWriter, r *http.Request) { case opnPKIOperation: response, err = h.PKIOperation(ctx, request) default: - err = fmt.Errorf("unknown operation: %s", request.Operation) + err = errors.Errorf("unknown operation: %s", request.Operation) } if err != nil { - writeError(w, fmt.Errorf("post request failed: %w", err)) + writeError(w, errors.Wrap(err, "post request failed")) return } @@ -163,7 +163,7 @@ func decodeSCEPRequest(r *http.Request) (SCEPRequest, error) { Message: decodedMessage, }, nil default: - return SCEPRequest{}, fmt.Errorf("unsupported operation: %s", operation) + return SCEPRequest{}, errors.Errorf("unsupported operation: %s", operation) } case http.MethodPost: body, err := ioutil.ReadAll(io.LimitReader(r.Body, maxPayloadSize)) @@ -175,7 +175,7 @@ func decodeSCEPRequest(r *http.Request) (SCEPRequest, error) { Message: body, }, nil default: - return SCEPRequest{}, fmt.Errorf("unsupported method: %s", method) + return SCEPRequest{}, errors.Errorf("unsupported method: %s", method) } } @@ -187,7 +187,7 @@ func (h *Handler) lookupProvisioner(next nextHTTP) nextHTTP { name := chi.URLParam(r, "provisionerID") provisionerID, err := url.PathUnescape(name) if err != nil { - api.WriteError(w, fmt.Errorf("error url unescaping provisioner id '%s'", name)) + api.WriteError(w, errors.Errorf("error url unescaping provisioner id '%s'", name)) return } @@ -229,6 +229,9 @@ func (h *Handler) GetCACert(ctx context.Context) (SCEPResponse, error) { if len(certs) == 1 { response.Data = certs[0].Raw } else { + // create degenerate pkcs7 certificate structure, according to + // https://tools.ietf.org/html/rfc8894#section-4.2.1.2, because + // not signed or encrypted data has to be returned. data, err := microscep.DegenerateCertificates(certs) if err != nil { return SCEPResponse{}, err @@ -329,12 +332,11 @@ func writeSCEPResponse(w http.ResponseWriter, response SCEPResponse) { w.Header().Set("Content-Type", contentHeader(response)) _, err := w.Write(response.Data) if err != nil { - writeError(w, fmt.Errorf("error when writing scep response: %w", err)) // This could end up as an error again + writeError(w, errors.Wrap(err, "error when writing scep response")) // This could end up as an error again } } func writeError(w http.ResponseWriter, err error) { - // TODO: this probably needs to use SCEP specific errors (i.e. failInfo) scepError := &scep.Error{ Message: err.Error(), Status: http.StatusInternalServerError, // TODO: make this a param? diff --git a/scep/authority.go b/scep/authority.go index 2b6686fd..c73885e5 100644 --- a/scep/authority.go +++ b/scep/authority.go @@ -1,32 +1,24 @@ package scep import ( - "bytes" "context" + "crypto/subtle" "crypto/x509" - "errors" "fmt" "net/url" "github.com/smallstep/certificates/authority/provisioner" - database "github.com/smallstep/certificates/db" - - "github.com/smallstep/nosql" microx509util "github.com/micromdm/scep/crypto/x509util" microscep "github.com/micromdm/scep/scep" - //"github.com/smallstep/certificates/scep/pkcs7" + "github.com/pkg/errors" "go.mozilla.org/pkcs7" "go.step.sm/crypto/x509util" ) -var ( - certTable = []byte("scep_certs") -) - // Interface is the SCEP authority interface. type Interface interface { LoadProvisionerByID(string) (provisioner.Interface, error) @@ -42,28 +34,21 @@ type Interface interface { // Authority is the layer that handles all SCEP interactions. type Authority struct { - backdate provisioner.Duration - db nosql.DB - prefix string - dns string - - // dir *directory - + db DB + prefix string + dns string intermediateCertificate *x509.Certificate - - service *Service - signAuth SignAuthority + service *Service + signAuth SignAuthority } // AuthorityOptions required to create a new SCEP Authority. type AuthorityOptions struct { - // Service provides the SCEP functions to Authority + // Service provides the certificate chain, the signer and the decrypter to the Authority Service *Service - // Backdate - Backdate provisioner.Duration - // DB is the database used by nosql. - DB nosql.DB - // DNS the host used to generate accurate SCEP links. By default the authority + // DB is the database used by SCEP + DB DB + // DNS is the host used to generate accurate SCEP links. By default the authority // will use the Host from the request, so this value will only be used if // request.Host is empty. DNS string @@ -81,19 +66,7 @@ type SignAuthority interface { // New returns a new Authority that implements the SCEP interface. func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) { - if _, ok := ops.DB.(*database.SimpleDB); !ok { - // If it's not a SimpleDB then go ahead and bootstrap the DB with the - // necessary SCEP tables. SimpleDB should ONLY be used for testing. - tables := [][]byte{certTable} - for _, b := range tables { - if err := ops.DB.CreateTable(b); err != nil { - return nil, fmt.Errorf("%w: error creating table %s", err, string(b)) - } - } - } - authority := &Authority{ - backdate: ops.Backdate, db: ops.DB, prefix: ops.Prefix, dns: ops.DNS, @@ -102,7 +75,7 @@ func New(signAuth SignAuthority, ops AuthorityOptions) (*Authority, error) { // TODO: this is not really nice to do; the Service should be removed // in its entirety to make this more interoperable with the rest of - // step-ca. + // step-ca, I think. if ops.Service != nil { authority.intermediateCertificate = ops.Service.certificateChain[0] authority.service = ops.Service @@ -200,12 +173,12 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err p7c, err := pkcs7.Parse(msg.P7.Content) if err != nil { - return err + return errors.Wrap(err, "error parsing pkcs7 content") } envelope, err := p7c.Decrypt(a.intermediateCertificate, a.service.decrypter) if err != nil { - return err + return errors.Wrap(err, "error decrypting encrypted pkcs7 content") } msg.pkiEnvelope = envelope @@ -214,19 +187,19 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err case microscep.CertRep: certs, err := microscep.CACerts(msg.pkiEnvelope) if err != nil { - return err + return errors.Wrap(err, "error extracting CA certs from pkcs7 degenerate data") } - msg.CertRepMessage.Certificate = certs[0] // TODO: check correctness of this + msg.CertRepMessage.Certificate = certs[0] return nil case microscep.PKCSReq, microscep.UpdateReq, microscep.RenewalReq: csr, err := x509.ParseCertificateRequest(msg.pkiEnvelope) if err != nil { - return fmt.Errorf("parse CSR from pkiEnvelope: %w", err) + return errors.Wrap(err, "parse CSR from pkiEnvelope") } // check for challengePassword cp, err := microx509util.ParseChallengePassword(msg.pkiEnvelope) if err != nil { - return fmt.Errorf("scep: parse challenge password in pkiEnvelope: %w", err) + return errors.Wrap(err, "parse challenge password in pkiEnvelope") } msg.CSRReqMessage = µscep.CSRReqMessage{ RawDecrypted: msg.pkiEnvelope, @@ -235,7 +208,7 @@ func (a *Authority) DecryptPKIEnvelope(ctx context.Context, msg *PKIMessage) err } return nil case microscep.GetCRL, microscep.GetCert, microscep.CertPoll: - return fmt.Errorf("not implemented") //errNotImplemented + return errors.Errorf("not implemented") } return nil @@ -266,16 +239,13 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m } // Template data - data := x509util.NewTemplateData() - data.SetCommonName(csr.Subject.CommonName) - data.SetSANs(csr.DNSNames) - data.SetCertificateRequest(csr) + data := x509util.CreateTemplateData(csr.Subject.CommonName, csr.DNSNames) // Get authorizations from the SCEP provisioner. ctx = provisioner.NewContextWithMethod(ctx, provisioner.SignMethod) signOps, err := p.AuthorizeSign(ctx, "") if err != nil { - return nil, fmt.Errorf("error retrieving authorization options from SCEP provisioner: %w", err) + return nil, errors.Wrap(err, "error retrieving authorization options from SCEP provisioner") } opts := provisioner.SignOptions{ @@ -285,19 +255,20 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m templateOptions, err := provisioner.TemplateOptions(p.GetOptions(), data) if err != nil { - return nil, fmt.Errorf("error creating template options from SCEP provisioner: %w", err) + return nil, errors.Wrap(err, "error creating template options from SCEP provisioner") } signOps = append(signOps, templateOptions) certChain, err := a.signAuth.Sign(csr, opts, signOps...) if err != nil { - return nil, fmt.Errorf("error generating certificate for order %w", err) + return nil, errors.Wrap(err, "error generating certificate for order") } + // take the issued certificate (only); https://tools.ietf.org/html/rfc8894#section-3.3.2 cert := certChain[0] - // create a degenerate cert structure - deg, err := degenerateCertificates([]*x509.Certificate{cert}) + // and create a degenerate cert structure + deg, err := microscep.DegenerateCertificates([]*x509.Certificate{cert}) if err != nil { return nil, err } @@ -370,13 +341,12 @@ func (a *Authority) SignCSR(ctx context.Context, csr *x509.CertificateRequest, m CertRepMessage: cr, } - // TODO: save more data? - _, err = newCert(a.db, CertOptions{ + // store the newly created certificate + err = newCert(a.db, CertOptions{ Leaf: certChain[0], Intermediates: certChain[1:], }) if err != nil { - fmt.Println(err) return nil, err } @@ -459,7 +429,7 @@ func (a *Authority) MatchChallengePassword(ctx context.Context, password string) return false, err } - if p.GetChallengePassword() == password { + if subtle.ConstantTimeCompare([]byte(p.GetChallengePassword()), []byte(password)) == 1 { return true, nil } @@ -491,21 +461,3 @@ func (a *Authority) GetCACaps(ctx context.Context) []string { return caps } - -// degenerateCertificates creates degenerate certificates pkcs#7 type -func degenerateCertificates(certs []*x509.Certificate) ([]byte, error) { - var buf bytes.Buffer - for _, cert := range certs { - buf.Write(cert.Raw) - } - degenerate, err := pkcs7.DegenerateCertificate(buf.Bytes()) - if err != nil { - return nil, err - } - return degenerate, nil -} - -// Interface guards -var ( - _ Interface = (*Authority)(nil) -) diff --git a/scep/certificate.go b/scep/certificate.go index 5e43b762..39015af5 100644 --- a/scep/certificate.go +++ b/scep/certificate.go @@ -2,79 +2,20 @@ package scep import ( "crypto/x509" - "encoding/json" - "encoding/pem" - "fmt" - "time" - "github.com/smallstep/nosql" + "github.com/pkg/errors" ) -type certificate struct { - ID string `json:"id"` - Created time.Time `json:"created"` - Leaf []byte `json:"leaf"` - Intermediates []byte `json:"intermediates"` -} - // CertOptions options with which to create and store a cert object. type CertOptions struct { Leaf *x509.Certificate Intermediates []*x509.Certificate } -func newCert(db nosql.DB, ops CertOptions) (*certificate, error) { - - // TODO: according to the RFC this should be IssuerAndSerialNumber, - // but sscep seems to use just the serial number for getcert - - id := ops.Leaf.SerialNumber.String() - - leaf := pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: ops.Leaf.Raw, - }) - var intermediates []byte - for _, cert := range ops.Intermediates { - intermediates = append(intermediates, pem.EncodeToMemory(&pem.Block{ - Type: "CERTIFICATE", - Bytes: cert.Raw, - })...) - } - - cert := &certificate{ - ID: id, - Leaf: leaf, - Intermediates: intermediates, - Created: time.Now().UTC(), - } - certB, err := json.Marshal(cert) +func newCert(db DB, ops CertOptions) error { + err := db.StoreCertificate(ops.Leaf) if err != nil { - return nil, fmt.Errorf("%w: error marshaling certificate", err) - } - - _, swapped, err := db.CmpAndSwap(certTable, []byte(id), nil, certB) - switch { - case err != nil: - return nil, fmt.Errorf("%w: error storing certificate", err) - case !swapped: - return nil, fmt.Errorf("error storing certificate; " + - "value has changed since last read") - default: - return cert, nil + errors.Wrap(err, "error while storing certificate") } + return nil } - -// func getCert(db nosql.DB, id string) (*certificate, error) { -// b, err := db.Get(certTable, []byte(id)) -// if nosql.IsErrNotFound(err) { -// return nil, fmt.Errorf("certificate %s not found", id) -// } else if err != nil { -// return nil, fmt.Errorf("error loading certificate") -// } -// var cert certificate -// if err := json.Unmarshal(b, &cert); err != nil { -// return nil, fmt.Errorf("%w: error unmarshaling certificate", err) -// } -// return &cert, nil -// } diff --git a/scep/database.go b/scep/database.go new file mode 100644 index 00000000..f73573fd --- /dev/null +++ b/scep/database.go @@ -0,0 +1,7 @@ +package scep + +import "crypto/x509" + +type DB interface { + StoreCertificate(crt *x509.Certificate) error +} diff --git a/scep/options.go b/scep/options.go new file mode 100644 index 00000000..61dbe024 --- /dev/null +++ b/scep/options.go @@ -0,0 +1,31 @@ +package scep + +import ( + "crypto" + "crypto/x509" +) + +type Options struct { + // CertificateChain is the issuer certificate, along with any other bundled certificates + // to be returned in the chain for consumers. Configured in ca.json crt property. + CertificateChain []*x509.Certificate + // Signer signs CSRs in SCEP. Configured in ca.json key property. + Signer crypto.Signer `json:"-"` + // Decrypter decrypts encrypted SCEP messages. Configured in ca.json key property. + Decrypter crypto.Decrypter `json:"-"` +} + +// Validate checks the fields in Options. +func (o *Options) Validate() error { + // var typ Type + // if o == nil { + // typ = Type(SoftCAS) + // } else { + // typ = Type(o.Type) + // } + // // Check that the type can be loaded. + // if _, ok := LoadCertificateAuthorityServiceNewFunc(typ); !ok { + // return errors.Errorf("unsupported cas type %s", typ) + // } + return nil +} diff --git a/scep/service.go b/scep/service.go index e9a58646..508bcf77 100644 --- a/scep/service.go +++ b/scep/service.go @@ -4,9 +4,6 @@ import ( "context" "crypto" "crypto/x509" - "strings" - - "github.com/smallstep/certificates/cas/apiv1" ) // Service is a wrapper for crypto.Signer and crypto.Decrypter @@ -16,20 +13,12 @@ type Service struct { decrypter crypto.Decrypter } -func NewService(ctx context.Context, opts apiv1.Options) (*Service, error) { +func NewService(ctx context.Context, opts Options) (*Service, error) { if err := opts.Validate(); err != nil { return nil, err } - t := apiv1.Type(strings.ToLower(opts.Type)) - if t == apiv1.DefaultCAS { - t = apiv1.SoftCAS - } - - // TODO: silence the linter (temporarily) - _ = t - // TODO: should this become similar to the New CertificateAuthorityService as in x509CAService? return &Service{ certificateChain: opts.CertificateChain,