Add Serial to Cert ID ACME table and lookup

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)
 	}
 }

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)
 }

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 {

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

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",