Improve revocation authorization

acme/api/middleware.go

@@ -371,6 +371,7 @@ func (h *Handler) extractOrLookupJWK(next nextHTTP) nextHTTP {
 		}
 
 		// default to looking up the JWK based on KeyID
+		// NOTE: this is a JWK signed with the certificate private key
 		h.lookupJWK(next)(w, r)
 	}
 }

acme/api/revoke.go

@@ -5,10 +5,12 @@ import (
 	"encoding/base64"
 	"encoding/json"
 	"net/http"
+	"strings"
 
 	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/api"
 	"github.com/smallstep/certificates/authority"
+	"github.com/smallstep/certificates/authority/provisioner"
 	"github.com/smallstep/certificates/logging"
 	"go.step.sm/crypto/jose"
 	"golang.org/x/crypto/ocsp"
@@ -29,23 +31,11 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
-	if shouldCheckAccount(jws) {
+	prov, err := provisionerFromContext(ctx)
-		_, err := accountFromContext(ctx)
 	if err != nil {
 		api.WriteError(w, err)
 		return
 	}
-	}
-
-	// TODO: do checks on account, i.e. is it still valid? is it allowed to do revocations? Revocations on the to be revoked cert?
-
-	_, err = provisionerFromContext(ctx)
-	if err != nil {
-		api.WriteError(w, err)
-		return
-	}
-
-	// TODO: let provisioner authorize the revocation? Necessary per provisioner? Or can it be done by the CA, like the Revoke itself.
 
 	p, err := payloadFromContext(ctx)
 	if err != nil {
@@ -73,19 +63,39 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 	}
 
 	serial := certToBeRevoked.SerialNumber.String()
-	_, err = h.db.GetCertificateBySerial(ctx, serial)
+	existingCert, err := h.db.GetCertificateBySerial(ctx, serial)
 	if err != nil {
 		api.WriteError(w, acme.WrapErrorISE(err, "error retrieving certificate by serial"))
 		return
 	}
 
-	// if existingCert.AccountID != acc.ID {
+	if shouldCheckAccountFrom(jws) {
-	// 	api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType,
+		account, err := accountFromContext(ctx)
-	// 		"account '%s' does not own certificate '%s'", acc.ID, certID))
+		if err != nil {
-	// 	return // TODO: this check should only be performed in case acc exists (i.e. KeyID revoke)
+			api.WriteError(w, err)
-	// }
+			return
-
+		}
-	// TODO: validate the certToBeRevoked against what we know about it?
+		if !account.IsValid() {
+			api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType,
+				"account '%s' has status '%s'", account.ID, account.Status))
+			return
+		}
+		if existingCert.AccountID != account.ID {
+			api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType,
+				"account '%s' does not own certificate '%s'", account.ID, existingCert.ID))
+			return
+		}
+		// TODO: check "an account that holds authorizations for all of the identifiers in the certificate."
+	} else {
+		// if account doesn't need to be checked, the JWS should be verified to be signed by the
+		// private key that belongs to the public key in the certificate to be revoked.
+		_, err := jws.Verify(certToBeRevoked.PublicKey)
+		if err != nil {
+			api.WriteError(w, acme.WrapError(acme.ErrorUnauthorizedType, err,
+				"verification of jws using certificate public key failed"))
+			return
+		}
+	}
 
 	reasonCode := payload.ReasonCode
 	acmeErr := validateReasonCode(reasonCode)
@@ -94,10 +104,18 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	// Authorize revocation by ACME provisioner
+	ctx = provisioner.NewContextWithMethod(ctx, provisioner.RevokeMethod)
+	err = prov.AuthorizeRevoke(ctx, "")
+	if err != nil {
+		api.WriteError(w, acme.WrapErrorISE(err, "error authorizing revocation on provisioner"))
+		return
+	}
+
 	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)
+		api.WriteError(w, wrapRevokeErr(err))
 		return
 	}
 
@@ -106,6 +124,16 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 	w.Write(nil)
 }
 
+// wrapRevokeErr is a best effort implementation to transform an error during
+// revocation into an ACME error, so that clients can understand the error.
+func wrapRevokeErr(err error) *acme.Error {
+	t := err.Error()
+	if strings.Contains(t, "has already been revoked") {
+		return acme.NewError(acme.ErrorAlreadyRevokedType, t)
+	}
+	return acme.WrapErrorISE(err, "error when revoking certificate")
+}
+
 // logRevoke logs successful revocation of certificate
 func logRevoke(w http.ResponseWriter, ri *authority.RevokeOptions) {
 	if rl, ok := w.(logging.ResponseLogger); ok {
@@ -176,9 +204,13 @@ func reason(reasonCode int) string {
 	}
 }
 
-// shouldCheckAccount indicates whether an account should be
+// shouldCheckAccountFrom indicates whether an account should be
 // retrieved from the context, so that it can be used for
-// additional checks.
+// additional checks. This should only be done when no JWK
-func shouldCheckAccount(jws *jose.JSONWebSignature) bool {
+// can be extracted from the request, as that would indicate
+// that the revocation request was signed with a certificate
+// key pair (and not an account key pair). Looking up such
+// a JWK would result in no Account being found.
+func shouldCheckAccountFrom(jws *jose.JSONWebSignature) bool {
 	return !canExtractJWKFrom(jws)
 }

acme/common.go

@@ -30,6 +30,7 @@ var clock Clock
 // only those methods required by the ACME api/authority.
 type Provisioner interface {
 	AuthorizeSign(ctx context.Context, token string) ([]provisioner.SignOption, error)
+	AuthorizeRevoke(ctx context.Context, token string) error
 	GetID() string
 	GetName() string
 	DefaultTLSCertDuration() time.Duration
@@ -43,6 +44,7 @@ type MockProvisioner struct {
 	MgetID                  func() string
 	MgetName                func() string
 	MauthorizeSign          func(ctx context.Context, ott string) ([]provisioner.SignOption, error)
+	MauthorizeRevoke        func(ctx context.Context, token string) error
 	MdefaultTLSCertDuration func() time.Duration
 	MgetOptions             func() *provisioner.Options
 }
@@ -63,6 +65,14 @@ func (m *MockProvisioner) AuthorizeSign(ctx context.Context, ott string) ([]prov
 	return m.Mret1.([]provisioner.SignOption), m.Merr
 }
 
+// AuthorizeRevoke mock
+func (m *MockProvisioner) AuthorizeRevoke(ctx context.Context, token string) error {
+	if m.MauthorizeRevoke != nil {
+		return m.MauthorizeRevoke(ctx, token)
+	}
+	return m.Merr
+}
+
 // DefaultTLSCertDuration mock
 func (m *MockProvisioner) DefaultTLSCertDuration() time.Duration {
 	if m.MdefaultTLSCertDuration != nil {

acme/db/nosql/certificate_test.go

@@ -103,7 +103,7 @@ func TestDB_CreateCertificate(t *testing.T) {
 							*idPtr = cert.ID
 						}
 
-						countOfCmpAndSwapCalls += 1
+						countOfCmpAndSwapCalls++
 
 						return nil, true, nil
 					},

authority/provisioner/acme.go

@@ -99,6 +99,15 @@ func (p *ACME) AuthorizeSign(ctx context.Context, token string) ([]SignOption, e
 	}, nil
 }
 
+// AuthorizeRevoke is called just before the certificate is to be revoked by
+// the CA. It can be used to authorize revocation of a certificate. It
+// currently is a no-op.
+// TODO: add configuration option that toggles revocation? Or change function signature to make it more useful?
+// Or move certain logic out of the Revoke API to here? Would likely involve some more stuff in the ctx.
+func (p *ACME) AuthorizeRevoke(ctx context.Context, token string) error {
+	return nil
+}
+
 // AuthorizeRenew returns an error if the renewal is disabled.
 // NOTE: This method does not actually validate the certificate or check it's
 // revocation status. Just confirms that the provisioner that created the

authority/provisioner/provisioner_test.go

@@ -184,7 +184,6 @@ func TestUnimplementedMethods(t *testing.T) {
 		{"x5c/sshRenew", &X5C{}, SSHRenewMethod},
 		{"x5c/sshRekey", &X5C{}, SSHRekeyMethod},
 		{"x5c/sshRevoke", &X5C{}, SSHRekeyMethod},
-		{"acme/revoke", &ACME{}, RevokeMethod},
 		{"acme/sshSign", &ACME{}, SSHSignMethod},
 		{"acme/sshRekey", &ACME{}, SSHRekeyMethod},
 		{"acme/sshRenew", &ACME{}, SSHRenewMethod},