Improve handling of ACME revocation

acme/api/revoke.go

@@ -4,18 +4,23 @@ import (
 	"crypto/x509"
 	"encoding/base64"
 	"encoding/json"
-	"fmt"
 	"net/http"
 
+	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/api"
 	"github.com/smallstep/certificates/authority"
+	"golang.org/x/crypto/ocsp"
 )
 
+type revokePayload struct {
+	Certificate string `json:"certificate"`
+	ReasonCode  int    `json:"reason"`
+}
+
 func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 
 	// TODO: support the non-kid case, i.e. JWK with the public key of the cert
 	// base the account + certificate JWK instead of the kid (which is now the case)
-	// TODO: handle errors; sent the right ACME response back
 
 	ctx := r.Context()
 	_, err := accountFromContext(ctx)
@@ -28,52 +33,100 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 
 	_, err = provisionerFromContext(ctx)
 	if err != nil {
-		fmt.Println(err)
+		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 {
-		fmt.Println(err)
+		api.WriteError(w, err)
+		return
 	}
 
-	type revokedCert struct {
+	var payload revokePayload
-		Certificate string `json:"certificate"`
+	err = json.Unmarshal(p.value, &payload)
-		Reason      int    `json:"reason"` // TODO: is optional; handle accordingly
-	}
-
-	var rc revokedCert
-	err = json.Unmarshal(p.value, &rc)
 	if err != nil {
-		fmt.Println("error:", err)
+		api.WriteError(w, err) // TODO: fix error type
+		return
 	}
 
-	c, err := base64.RawURLEncoding.DecodeString(rc.Certificate)
+	certBytes, err := base64.RawURLEncoding.DecodeString(payload.Certificate)
 	if err != nil {
-		fmt.Println("error:", err)
+		api.WriteError(w, err) // TODO: fix error type
+		return
 	}
 
-	certToBeRevoked, err := x509.ParseCertificate(c)
+	certToBeRevoked, err := x509.ParseCertificate(certBytes)
 	if err != nil {
-		fmt.Println("error: failed to parse certificate: " + err.Error())
+		api.WriteError(w, err) // TODO: fix error type
+		return
 	}
 
-	// TODO: check reason code; should be allowed; otherwise send error
+	certID := certToBeRevoked.SerialNumber.String()
+	// TODO: retrieving the certificate to verify the account does not seem to work? 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)
+	// }
+
+	// 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"))
+		return
+	}
+
+	// TODO: check reason code; should be allowed (based on what? and determined by Provisioner?); otherwise send error
 
 	options := &authority.RevokeOptions{
-		Serial:      certToBeRevoked.SerialNumber.String(),
+		Serial:     certID,
-		Reason:      "test", // TODO: map it to the reason based on code?
+		Reason:     reason(payload.ReasonCode),
-		ReasonCode:  rc.Reason,
+		ReasonCode: payload.ReasonCode,
-		PassiveOnly: false,
+		ACME:       true,
-		MTLS:        true, // TODO: should be false, I guess, but results in error:  authority.Revoke; error parsing token: square/go-jose: compact JWS format must have three parts (OTT)
 		Crt:        certToBeRevoked,
-		OTT:         "",
 	}
+
 	err = h.ca.Revoke(ctx, options)
 	if err != nil {
-		fmt.Println("error: ", err.Error()) // TODO: send the right error; 400; alreadyRevoked (or something else went wrong, of course)
+		api.WriteError(w, err) // TODO: send the right error; 400; alreadyRevoked (or something else went wrong, of course)
+		return
 	}
 
 	w.Write(nil)
 }
+
+func reason(reasonCode int) string {
+	switch reasonCode {
+	case ocsp.Unspecified:
+		return "unspecified reason"
+	case ocsp.KeyCompromise:
+		return "key compromised"
+	case ocsp.CACompromise:
+		return "ca compromised"
+	case ocsp.AffiliationChanged:
+		return "affiliation changed"
+	case ocsp.Superseded:
+		return "superseded"
+	case ocsp.CessationOfOperation:
+		return "cessation of operation"
+	case ocsp.CertificateHold:
+		return "certificate hold"
+	case ocsp.RemoveFromCRL:
+		return "remove from crl"
+	case ocsp.PrivilegeWithdrawn:
+		return "privilege withdrawn"
+	case ocsp.AACompromise:
+		return "aa compromised"
+	default:
+		return "unspecified reason"
+	}
+}

authority/tls.go

@@ -307,6 +307,7 @@ type RevokeOptions struct {
 	ReasonCode  int
 	PassiveOnly bool
 	MTLS        bool
+	ACME        bool
 	Crt         *x509.Certificate
 	OTT         string
 }
@@ -324,9 +325,10 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		errs.WithKeyVal("reason", revokeOpts.Reason),
 		errs.WithKeyVal("passiveOnly", revokeOpts.PassiveOnly),
 		errs.WithKeyVal("MTLS", revokeOpts.MTLS),
+		errs.WithKeyVal("ACME", revokeOpts.ACME),
 		errs.WithKeyVal("context", provisioner.MethodFromContext(ctx).String()),
 	}
-	if revokeOpts.MTLS {
+	if revokeOpts.MTLS || revokeOpts.ACME {
 		opts = append(opts, errs.WithKeyVal("certificate", base64.StdEncoding.EncodeToString(revokeOpts.Crt.Raw)))
 	} else {
 		opts = append(opts, errs.WithKeyVal("token", revokeOpts.OTT))
@@ -344,8 +346,8 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error
 		p   provisioner.Interface
 		err error
 	)
-	// If not mTLS then get the TokenID of the token.
+	// If not mTLS nor ACME, then get the TokenID of the token.
-	if !revokeOpts.MTLS {
+	if !(revokeOpts.MTLS || revokeOpts.ACME) {
 		token, err := jose.ParseSigned(revokeOpts.OTT)
 		if err != nil {
 			return errs.Wrap(http.StatusUnauthorized, err,