From 2b15230aa4393cc68593990f0779a3c65e35c1a6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 9 Jul 2021 17:51:31 +0200 Subject: [PATCH] Add Serial to Cert ID ACME table and lookup --- acme/api/middleware.go | 4 +- acme/api/revoke.go | 92 +++++++++++++++++++++++++----------- acme/db.go | 15 +++++- acme/db/nosql/certificate.go | 35 +++++++++++++- acme/db/nosql/nosql.go | 3 +- 5 files changed, 116 insertions(+), 33 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 3eff4571..371be7fa 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -364,13 +364,13 @@ func (h *Handler) extractOrLookupJWK(next nextHTTP) nextHTTP { } // at this point the JWS has already been verified (if correctly configured in middleware), - // and it can be used to check if a jwk exists. + // and it can be used to check if a JWK exists. if canExtractJWKFrom(jws) { h.extractJWK(next)(w, r) return } - // default to looking up the JWK based on KID + // default to looking up the JWK based on KeyID h.lookupJWK(next)(w, r) } } diff --git a/acme/api/revoke.go b/acme/api/revoke.go index bf9c3906..75eb4ee2 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -9,15 +9,17 @@ import ( "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" + "github.com/smallstep/certificates/logging" "go.step.sm/crypto/jose" "golang.org/x/crypto/ocsp" ) type revokePayload struct { Certificate string `json:"certificate"` - ReasonCode int `json:"reason"` + ReasonCode *int `json:"reason,omitempty"` } +// RevokeCert attempts to revoke a certificate. func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { ctx := r.Context() @@ -54,63 +56,99 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { var payload revokePayload err = json.Unmarshal(p.value, &payload) if err != nil { - api.WriteError(w, err) // TODO: fix error type + api.WriteError(w, acme.WrapErrorISE(err, "error unmarshaling payload")) return } certBytes, err := base64.RawURLEncoding.DecodeString(payload.Certificate) if err != nil { - api.WriteError(w, err) // TODO: fix error type + api.WriteError(w, acme.WrapErrorISE(err, "error decoding base64 certificate")) return } certToBeRevoked, err := x509.ParseCertificate(certBytes) if err != nil { - api.WriteError(w, err) // TODO: fix error type + api.WriteError(w, acme.WrapErrorISE(err, "error parsing certificate")) + return + } + + serial := certToBeRevoked.SerialNumber.String() + _, err = h.db.GetCertificateBySerial(ctx, serial) + if err != nil { + api.WriteError(w, acme.WrapErrorISE(err, "error retrieving certificate by serial")) return } - certID := certToBeRevoked.SerialNumber.String() - // TODO: retrieving the certificate to verify the account does not seem to work, so far? Results in certificate not found error. - // When Revoke is called, the certificate IS in fact found? The (h *Handler) GetCertificate function is fairly similar, too. - // existingCert, err := h.db.GetCertificate(ctx, certID) - // if err != nil { - // api.WriteError(w, acme.WrapErrorISE(err, "error retrieving certificate")) - // return - // } // if existingCert.AccountID != acc.ID { // api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType, // "account '%s' does not own certificate '%s'", acc.ID, certID)) - // return // TODO: this check should only be performed in case acc exists (i.e. KID revoke) + // return // TODO: this check should only be performed in case acc exists (i.e. KeyID revoke) // } // TODO: validate the certToBeRevoked against what we know about it? - if payload.ReasonCode < ocsp.Unspecified || payload.ReasonCode > ocsp.AACompromise { - api.WriteError(w, acme.NewError(acme.ErrorBadRevocationReasonType, "reasonCode out of bounds")) + reasonCode := payload.ReasonCode + acmeErr := validateReasonCode(reasonCode) + if acmeErr != nil { + api.WriteError(w, acmeErr) return } - // TODO: check reason code; should be allowed (based on what? and determined by Provisioner?); otherwise send error - - options := &authority.RevokeOptions{ - Serial: certID, - Reason: reason(payload.ReasonCode), - ReasonCode: payload.ReasonCode, - ACME: true, - Crt: certToBeRevoked, - } - + options := revokeOptions(serial, certToBeRevoked, reasonCode) err = h.ca.Revoke(ctx, options) if err != nil { api.WriteError(w, err) // TODO: send the right error; 400; alreadyRevoked (or something else went wrong, of course) return } + logRevoke(w, options) w.Header().Add("Link", link(h.linker.GetLink(ctx, DirectoryLinkType), "index")) w.Write(nil) } +// logRevoke logs successful revocation of certificate +func logRevoke(w http.ResponseWriter, ri *authority.RevokeOptions) { + if rl, ok := w.(logging.ResponseLogger); ok { + rl.WithFields(map[string]interface{}{ + "serial": ri.Serial, + "reasonCode": ri.ReasonCode, + "reason": ri.Reason, + "passiveOnly": ri.PassiveOnly, + "ACME": ri.ACME, + }) + } +} + +// validateReasonCode validates the revocation reason +func validateReasonCode(reasonCode *int) *acme.Error { + if reasonCode != nil && ((*reasonCode < ocsp.Unspecified || *reasonCode > ocsp.AACompromise) || *reasonCode == 7) { + return acme.NewError(acme.ErrorBadRevocationReasonType, "reasonCode out of bounds") + } + // NOTE: it's possible to add additional requirements to the reason code: + // The server MAY disallow a subset of reasonCodes from being + // used by the user. If a request contains a disallowed reasonCode, + // then the server MUST reject it with the error type + // "urn:ietf:params:acme:error:badRevocationReason" + // No additional checks have been implemented so far. + return nil +} + +// revokeOptions determines the the RevokeOptions for the Authority to use in revocation +func revokeOptions(serial string, certToBeRevoked *x509.Certificate, reasonCode *int) *authority.RevokeOptions { + opts := &authority.RevokeOptions{ + Serial: serial, + ACME: true, + Crt: certToBeRevoked, + } + if reasonCode != nil { // NOTE: when implementing CRL and/or OCSP, and reason code is missing, CRL entry extension should be omitted + opts.Reason = reason(*reasonCode) + opts.ReasonCode = *reasonCode + } + return opts +} + +// reason transforms an integer reason code to a +// textual description of the revocation reason. func reason(reasonCode int) string { switch reasonCode { case ocsp.Unspecified: @@ -138,9 +176,9 @@ func reason(reasonCode int) string { } } -// shouldUseAccount indicates whether an account should be +// shouldCheckAccount indicates whether an account should be // retrieved from the context, so that it can be used for -// additional checks +// additional checks. func shouldCheckAccount(jws *jose.JSONWebSignature) bool { return !canExtractJWKFrom(jws) } diff --git a/acme/db.go b/acme/db.go index d678fef4..c4b79a66 100644 --- a/acme/db.go +++ b/acme/db.go @@ -28,6 +28,7 @@ type DB interface { CreateCertificate(ctx context.Context, cert *Certificate) error GetCertificate(ctx context.Context, id string) (*Certificate, error) + GetCertificateBySerial(ctx context.Context, serial string) (*Certificate, error) CreateChallenge(ctx context.Context, ch *Challenge) error GetChallenge(ctx context.Context, id, authzID string) (*Challenge, error) @@ -54,8 +55,9 @@ type MockDB struct { MockGetAuthorization func(ctx context.Context, id string) (*Authorization, error) MockUpdateAuthorization func(ctx context.Context, az *Authorization) error - MockCreateCertificate func(ctx context.Context, cert *Certificate) error - MockGetCertificate func(ctx context.Context, id string) (*Certificate, error) + MockCreateCertificate func(ctx context.Context, cert *Certificate) error + MockGetCertificate func(ctx context.Context, id string) (*Certificate, error) + MockGetCertificateBySerial func(ctx context.Context, serial string) (*Certificate, error) MockCreateChallenge func(ctx context.Context, ch *Challenge) error MockGetChallenge func(ctx context.Context, id, authzID string) (*Challenge, error) @@ -180,6 +182,15 @@ func (m *MockDB) GetCertificate(ctx context.Context, id string) (*Certificate, e return m.MockRet1.(*Certificate), m.MockError } +func (m *MockDB) GetCertificateBySerial(ctx context.Context, serial string) (*Certificate, error) { + if m.MockGetCertificateBySerial != nil { + return m.MockGetCertificateBySerial(ctx, serial) + } else if m.MockError != nil { + return nil, m.MockError + } + return m.MockRet1.(*Certificate), m.MockError +} + // CreateChallenge mock func (m *MockDB) CreateChallenge(ctx context.Context, ch *Challenge) error { if m.MockCreateChallenge != nil { diff --git a/acme/db/nosql/certificate.go b/acme/db/nosql/certificate.go index d3e15833..ee37c570 100644 --- a/acme/db/nosql/certificate.go +++ b/acme/db/nosql/certificate.go @@ -21,6 +21,11 @@ type dbCert struct { Intermediates []byte `json:"intermediates"` } +type dbSerial struct { + Serial string `json:"serial"` + CertificateID string `json:"certificateID"` +} + // CreateCertificate creates and stores an ACME certificate type. func (db *DB) CreateCertificate(ctx context.Context, cert *acme.Certificate) error { var err error @@ -49,7 +54,17 @@ func (db *DB) CreateCertificate(ctx context.Context, cert *acme.Certificate) err Intermediates: intermediates, CreatedAt: time.Now().UTC(), } - return db.save(ctx, cert.ID, dbch, nil, "certificate", certTable) + err = db.save(ctx, cert.ID, dbch, nil, "certificate", certTable) + if err != nil { + return err + } + + serial := cert.Leaf.SerialNumber.String() + dbSerial := &dbSerial{ + Serial: serial, + CertificateID: cert.ID, + } + return db.save(ctx, serial, dbSerial, nil, "serial", certBySerialTable) } // GetCertificate retrieves and unmarshals an ACME certificate type from the @@ -80,6 +95,24 @@ func (db *DB) GetCertificate(ctx context.Context, id string) (*acme.Certificate, }, nil } +// GetCertificateBySerial retrieves and unmarshals an ACME certificate type from the +// datastore based on a certificate serial number. +func (db *DB) GetCertificateBySerial(ctx context.Context, serial string) (*acme.Certificate, error) { + b, err := db.db.Get(certBySerialTable, []byte(serial)) + if nosql.IsErrNotFound(err) { + return nil, acme.NewError(acme.ErrorMalformedType, "certificate with serial %s not found", serial) + } else if err != nil { + return nil, errors.Wrapf(err, "error loading certificate ID for serial %s", serial) + } + + dbSerial := new(dbSerial) + if err := json.Unmarshal(b, dbSerial); err != nil { + return nil, errors.Wrapf(err, "error unmarshaling certificate with serial %s", serial) + } + + return db.GetCertificate(ctx, dbSerial.CertificateID) +} + func parseBundle(b []byte) ([]*x509.Certificate, error) { var ( err error diff --git a/acme/db/nosql/nosql.go b/acme/db/nosql/nosql.go index 052f5729..c179d2ed 100644 --- a/acme/db/nosql/nosql.go +++ b/acme/db/nosql/nosql.go @@ -19,6 +19,7 @@ var ( orderTable = []byte("acme_orders") ordersByAccountIDTable = []byte("acme_account_orders_index") certTable = []byte("acme_certs") + certBySerialTable = []byte("acme_serial_certs_index") ) // DB is a struct that implements the AcmeDB interface. @@ -29,7 +30,7 @@ type DB struct { // New configures and returns a new ACME DB backend implemented using a nosql DB. func New(db nosqlDB.DB) (*DB, error) { tables := [][]byte{accountTable, accountByKeyIDTable, authzTable, - challengeTable, nonceTable, orderTable, ordersByAccountIDTable, certTable} + challengeTable, nonceTable, orderTable, ordersByAccountIDTable, certTable, certBySerialTable} for _, b := range tables { if err := db.CreateTable(b); err != nil { return nil, errors.Wrapf(err, "error creating table %s",