Increase test code coverage

acme/api/revoke.go

@@ -91,9 +91,9 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 	} 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.
-		// TODO: implement test case for this
 		_, err := jws.Verify(certToBeRevoked.PublicKey)
 		if err != nil {
+			// TODO(hs): possible to determine an error vs. unauthorized and thus provide an ISE vs. Unauthorized?
 			api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, "verification of jws using certificate public key failed", err))
 			return
 		}
@@ -141,7 +141,7 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) {
 // 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") {
+	if strings.Contains(t, "is already revoked") {
 		return acme.NewError(acme.ErrorAlreadyRevokedType, t)
 	}
 	return acme.WrapErrorISE(err, "error when revoking certificate")
@@ -157,7 +157,7 @@ func wrapUnauthorizedError(cert *x509.Certificate, msg string, err error) *acme.
 		acmeErr = acme.WrapError(acme.ErrorUnauthorizedType, err, msg)
 	}
 	acmeErr.Status = http.StatusForbidden
-	acmeErr.Detail = fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()) // TODO: what about other SANs?
+	acmeErr.Detail = fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()) // TODO: what about other SANs? When no Subject is in the certificate?
 
 	return acmeErr
 }

acme/api/revoke_test.go

@@ -3,66 +3,267 @@ package api
 import (
 	"bytes"
 	"context"
+	"crypto"
+	"crypto/ecdsa"
+	"crypto/rand"
+	"crypto/rsa"
 	"crypto/x509"
+	"crypto/x509/pkix"
 	"encoding/base64"
 	"encoding/json"
-	"encoding/pem"
 	"fmt"
 	"io"
+	"math/big"
 	"net/http/httptest"
 	"net/url"
-	"reflect"
 	"testing"
+	"time"
 
 	"github.com/go-chi/chi"
+	"github.com/google/go-cmp/cmp"
 	"github.com/pkg/errors"
 	"github.com/smallstep/assert"
 	"github.com/smallstep/certificates/acme"
 	"github.com/smallstep/certificates/authority"
 	"github.com/smallstep/certificates/authority/provisioner"
 	"go.step.sm/crypto/jose"
+	"go.step.sm/crypto/keyutil"
+	"go.step.sm/crypto/x509util"
 	"golang.org/x/crypto/ocsp"
 )
 
-const (
+// v is a utility function to return the pointer to an integer
-	certPEM = `-----BEGIN CERTIFICATE-----
-MIIDujCCAqKgAwIBAgIIE31FZVaPXTUwDQYJKoZIhvcNAQEFBQAwSTELMAkGA1UE
-BhMCVVMxEzARBgNVBAoTCkdvb2dsZSBJbmMxJTAjBgNVBAMTHEdvb2dsZSBJbnRl
-cm5ldCBBdXRob3JpdHkgRzIwHhcNMTQwMTI5MTMyNzQzWhcNMTQwNTI5MDAwMDAw
-WjBpMQswCQYDVQQGEwJVUzETMBEGA1UECAwKQ2FsaWZvcm5pYTEWMBQGA1UEBwwN
-TW91bnRhaW4gVmlldzETMBEGA1UECgwKR29vZ2xlIEluYzEYMBYGA1UEAwwPbWFp
-bC5nb29nbGUuY29tMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEfRrObuSW5T7q
-5CnSEqefEmtH4CCv6+5EckuriNr1CjfVvqzwfAhopXkLrq45EQm8vkmf7W96XJhC
-7ZM0dYi1/qOCAU8wggFLMB0GA1UdJQQWMBQGCCsGAQUFBwMBBggrBgEFBQcDAjAa
-BgNVHREEEzARgg9tYWlsLmdvb2dsZS5jb20wCwYDVR0PBAQDAgeAMGgGCCsGAQUF
-BwEBBFwwWjArBggrBgEFBQcwAoYfaHR0cDovL3BraS5nb29nbGUuY29tL0dJQUcy
-LmNydDArBggrBgEFBQcwAYYfaHR0cDovL2NsaWVudHMxLmdvb2dsZS5jb20vb2Nz
-cDAdBgNVHQ4EFgQUiJxtimAuTfwb+aUtBn5UYKreKvMwDAYDVR0TAQH/BAIwADAf
-BgNVHSMEGDAWgBRK3QYWG7z2aLV29YG2u2IaulqBLzAXBgNVHSAEEDAOMAwGCisG
-AQQB1nkCBQEwMAYDVR0fBCkwJzAloCOgIYYfaHR0cDovL3BraS5nb29nbGUuY29t
-L0dJQUcyLmNybDANBgkqhkiG9w0BAQUFAAOCAQEAH6RYHxHdcGpMpFE3oxDoFnP+
-gtuBCHan2yE2GRbJ2Cw8Lw0MmuKqHlf9RSeYfd3BXeKkj1qO6TVKwCh+0HdZk283
-TZZyzmEOyclm3UGFYe82P/iDFt+CeQ3NpmBg+GoaVCuWAARJN/KfglbLyyYygcQq
-0SgeDh8dRKUiaW3HQSoYvTvdTuqzwK4CXsr3b5/dAOY8uMuG/IAR3FgwTbZ1dtoW
-RvOTa8hYiU6A475WuZKyEHcwnGYe57u2I2KbMgcKjPniocj4QzgYsVAVKW3IwaOh
-yE+vPxsiUkvQHdO2fojCkY8jg70jxM+gu59tPDNbw3Uh/2Ij310FgTHsnGQMyA==
------END CERTIFICATE-----`
-)
-
 func v(v int) *int {
 	return &v
 }
 
-func parseCertificate(data string) *x509.Certificate {
+// generateCertKeyPair generates fresh x509 certificate/key pairs for testing
-	block, _ := pem.Decode([]byte(data))
+func generateCertKeyPair() (*x509.Certificate, crypto.Signer, error) {
-	if block == nil {
+
-		panic("failed to parse certificate PEM")
+	pub, priv, err := keyutil.GenerateKeyPair("EC", "P-256", 0)
-	}
-	cert, err := x509.ParseCertificate(block.Bytes)
 	if err != nil {
-		panic("failed to parse certificate: " + err.Error())
+		return nil, nil, err
 	}
-	return cert
+
+	serial, err := rand.Int(rand.Reader, big.NewInt(1000000000000000000))
+	if err != nil {
+		return nil, nil, err
+	}
+
+	now := time.Now()
+	template := &x509.Certificate{
+		Subject:      pkix.Name{CommonName: "Test ACME Revoke Certificate"},
+		Issuer:       pkix.Name{CommonName: "Test ACME Revoke Certificate"},
+		IsCA:         false,
+		MaxPathLen:   0,
+		KeyUsage:     x509.KeyUsageCertSign | x509.KeyUsageCRLSign,
+		NotBefore:    now,
+		NotAfter:     now.Add(time.Hour),
+		SerialNumber: serial,
+	}
+
+	signer, ok := priv.(crypto.Signer)
+	if !ok {
+		return nil, nil, errors.Errorf("result is not a crypto.Signer: type %T", priv)
+	}
+
+	cert, err := x509util.CreateCertificate(template, template, pub, signer)
+
+	return cert, signer, err
+}
+
+var errUnsupportedKey = fmt.Errorf("unknown key type; only RSA and ECDSA are supported")
+
+// keyID is the account identity provided by a CA during registration.
+type keyID string
+
+// noKeyID indicates that jwsEncodeJSON should compute and use JWK instead of a KID.
+// See jwsEncodeJSON for details.
+const noKeyID = keyID("")
+
+// jwsEncodeJSON signs claimset using provided key and a nonce.
+// The result is serialized in JSON format containing either kid or jwk
+// fields based on the provided keyID value.
+//
+// If kid is non-empty, its quoted value is inserted in the protected head
+// as "kid" field value. Otherwise, JWK is computed using jwkEncode and inserted
+// as "jwk" field value. The "jwk" and "kid" fields are mutually exclusive.
+//
+// See https://tools.ietf.org/html/rfc7515#section-7.
+//
+// If nonce is empty, it will not be encoded into the header.
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwsEncodeJSON(claimset interface{}, key crypto.Signer, kid keyID, nonce, u string) ([]byte, error) {
+	alg, sha := jwsHasher(key.Public())
+	if alg == "" || !sha.Available() {
+		return nil, errUnsupportedKey
+	}
+
+	phead, err := jwsHead(alg, nonce, u, kid, key)
+	if err != nil {
+		return nil, err
+	}
+
+	var payload string
+	if claimset != nil {
+		cs, err := json.Marshal(claimset)
+		if err != nil {
+			return nil, err
+		}
+		payload = base64.RawURLEncoding.EncodeToString(cs)
+	}
+
+	payloadToSign := []byte(phead + "." + payload)
+	hash := sha.New()
+	_, _ = hash.Write(payloadToSign)
+	digest := hash.Sum(nil)
+
+	sig, err := jwsSign(key, sha, digest)
+	if err != nil {
+		return nil, err
+	}
+
+	return jwsFinal(sha, sig, phead, payload)
+}
+
+// jwsHasher indicates suitable JWS algorithm name and a hash function
+// to use for signing a digest with the provided key.
+// It returns ("", 0) if the key is not supported.
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwsHasher(pub crypto.PublicKey) (string, crypto.Hash) {
+	switch pub := pub.(type) {
+	case *rsa.PublicKey:
+		return "RS256", crypto.SHA256
+	case *ecdsa.PublicKey:
+		switch pub.Params().Name {
+		case "P-256":
+			return "ES256", crypto.SHA256
+		case "P-384":
+			return "ES384", crypto.SHA384
+		case "P-521":
+			return "ES512", crypto.SHA512
+		}
+	}
+	return "", 0
+}
+
+// jwsSign signs the digest using the given key.
+// The hash is unused for ECDSA keys.
+//
+// Note: non-stdlib crypto.Signer implementations are expected to return
+// the signature in the format as specified in RFC7518.
+// See https://tools.ietf.org/html/rfc7518 for more details.
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwsSign(key crypto.Signer, hash crypto.Hash, digest []byte) ([]byte, error) {
+	if key, ok := key.(*ecdsa.PrivateKey); ok {
+		// The key.Sign method of ecdsa returns ASN1-encoded signature.
+		// So, we use the package Sign function instead
+		// to get R and S values directly and format the result accordingly.
+		r, s, err := ecdsa.Sign(rand.Reader, key, digest)
+		if err != nil {
+			return nil, err
+		}
+		rb, sb := r.Bytes(), s.Bytes()
+		size := key.Params().BitSize / 8
+		if size%8 > 0 {
+			size++
+		}
+		sig := make([]byte, size*2)
+		copy(sig[size-len(rb):], rb)
+		copy(sig[size*2-len(sb):], sb)
+		return sig, nil
+	}
+	return key.Sign(rand.Reader, digest, hash)
+}
+
+// jwsHead constructs the protected JWS header for the given fields.
+// Since jwk and kid are mutually-exclusive, the jwk will be encoded
+// only if kid is empty. If nonce is empty, it will not be encoded.
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwsHead(alg, nonce, u string, kid keyID, key crypto.Signer) (string, error) {
+	phead := fmt.Sprintf(`{"alg":%q`, alg)
+	if kid == noKeyID {
+		jwk, err := jwkEncode(key.Public())
+		if err != nil {
+			return "", err
+		}
+		phead += fmt.Sprintf(`,"jwk":%s`, jwk)
+	} else {
+		phead += fmt.Sprintf(`,"kid":%q`, kid)
+	}
+	if nonce != "" {
+		phead += fmt.Sprintf(`,"nonce":%q`, nonce)
+	}
+	phead += fmt.Sprintf(`,"url":%q}`, u)
+	phead = base64.RawURLEncoding.EncodeToString([]byte(phead))
+	return phead, nil
+}
+
+// jwkEncode encodes public part of an RSA or ECDSA key into a JWK.
+// The result is also suitable for creating a JWK thumbprint.
+// https://tools.ietf.org/html/rfc7517
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwkEncode(pub crypto.PublicKey) (string, error) {
+	switch pub := pub.(type) {
+	case *rsa.PublicKey:
+		// https://tools.ietf.org/html/rfc7518#section-6.3.1
+		n := pub.N
+		e := big.NewInt(int64(pub.E))
+		// Field order is important.
+		// See https://tools.ietf.org/html/rfc7638#section-3.3 for details.
+		return fmt.Sprintf(`{"e":%q,"kty":"RSA","n":%q}`,
+			base64.RawURLEncoding.EncodeToString(e.Bytes()),
+			base64.RawURLEncoding.EncodeToString(n.Bytes()),
+		), nil
+	case *ecdsa.PublicKey:
+		// https://tools.ietf.org/html/rfc7518#section-6.2.1
+		p := pub.Curve.Params()
+		n := p.BitSize / 8
+		if p.BitSize%8 != 0 {
+			n++
+		}
+		x := pub.X.Bytes()
+		if n > len(x) {
+			x = append(make([]byte, n-len(x)), x...)
+		}
+		y := pub.Y.Bytes()
+		if n > len(y) {
+			y = append(make([]byte, n-len(y)), y...)
+		}
+		// Field order is important.
+		// See https://tools.ietf.org/html/rfc7638#section-3.3 for details.
+		return fmt.Sprintf(`{"crv":%q,"kty":"EC","x":%q,"y":%q}`,
+			p.Name,
+			base64.RawURLEncoding.EncodeToString(x),
+			base64.RawURLEncoding.EncodeToString(y),
+		), nil
+	}
+	return "", errUnsupportedKey
+}
+
+// jwsFinal constructs the final JWS object.
+// Implementation taken from github.com/mholt/acmez, which seems to be based on
+// https://github.com/golang/crypto/blob/master/acme/jws.go.
+func jwsFinal(sha crypto.Hash, sig []byte, phead, payload string) ([]byte, error) {
+	enc := struct {
+		Protected string `json:"protected"`
+		Payload   string `json:"payload"`
+		Sig       string `json:"signature"`
+	}{
+		Protected: phead,
+		Payload:   payload,
+		Sig:       base64.RawURLEncoding.EncodeToString(sig),
+	}
+	result, err := json.Marshal(&enc)
+	if err != nil {
+		return nil, err
+	}
+	return result, nil
 }
 
 type mockCA struct {
@@ -138,17 +339,6 @@ func Test_validateReasonCode(t *testing.T) {
 }
 
 func Test_reason(t *testing.T) {
-
-	// 	case ocsp.RemoveFromCRL:
-	// 		return "remove from crl"
-	// 	case ocsp.PrivilegeWithdrawn:
-	// 		return "privilege withdrawn"
-	// 	case ocsp.AACompromise:
-	// 		return "aa compromised"
-	// 	default:
-	// 		return "unspecified reason"
-	// 	}
-	// }
 	tests := []struct {
 		name       string
 		reasonCode int
@@ -220,7 +410,8 @@ func Test_reason(t *testing.T) {
 }
 
 func Test_revokeOptions(t *testing.T) {
-	var cert *x509.Certificate
+	cert, _, err := generateCertKeyPair()
+	assert.FatalError(t, err)
 	type args struct {
 		serial          string
 		certToBeRevoked *x509.Certificate
@@ -239,7 +430,7 @@ func Test_revokeOptions(t *testing.T) {
 			},
 			want: &authority.RevokeOptions{
 				Serial: "1234",
-				Crt:    nil,
+				Crt:    cert,
 				ACME:   true,
 			},
 		},
@@ -252,7 +443,7 @@ func Test_revokeOptions(t *testing.T) {
 			},
 			want: &authority.RevokeOptions{
 				Serial:     "1234",
-				Crt:        nil,
+				Crt:        cert,
 				ACME:       true,
 				ReasonCode: 1,
 				Reason:     "key compromised",
@@ -261,8 +452,8 @@ func Test_revokeOptions(t *testing.T) {
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			if got := revokeOptions(tt.args.serial, tt.args.certToBeRevoked, tt.args.reasonCode); !reflect.DeepEqual(got, tt.want) {
+			if got := revokeOptions(tt.args.serial, tt.args.certToBeRevoked, tt.args.reasonCode); !cmp.Equal(got, tt.want) {
-				t.Errorf("revokeOptions() = %v, want %v", got, tt.want)
+				t.Errorf("revokeOptions() diff = %s", cmp.Diff(got, tt.want))
 			}
 		})
 	}
@@ -279,7 +470,8 @@ func TestHandler_RevokeCert(t *testing.T) {
 	chiCtx := chi.NewRouteContext()
 	revokeURL := fmt.Sprintf("%s/acme/%s/revoke-cert", baseURL.String(), escProvName)
 
-	cert := parseCertificate(certPEM)
+	cert, key, err := generateCertKeyPair()
+	assert.FatalError(t, err)
 	rp := &revokePayload{
 		Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw),
 	}
@@ -295,6 +487,134 @@ func TestHandler_RevokeCert(t *testing.T) {
 	}
 
 	var tests = map[string]func(t *testing.T) test{
+		"fail/no-jws": func(t *testing.T) test {
+			ctx := context.Background()
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("jws expected in request context"),
+			}
+		},
+		"fail/nil-jws": func(t *testing.T) test {
+			ctx := context.WithValue(context.Background(), jwsContextKey, nil)
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("jws expected in request context"),
+			}
+		},
+		"fail/no-provisioner": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), jwsContextKey, jws)
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("provisioner does not exist"),
+			}
+		},
+		"fail/nil-provisioner": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), jwsContextKey, jws)
+			ctx = context.WithValue(ctx, provisionerContextKey, nil)
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("provisioner does not exist"),
+			}
+		},
+		"fail/no-payload": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), jwsContextKey, jws)
+			ctx = context.WithValue(ctx, provisionerContextKey, prov)
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("payload does not exist"),
+			}
+		},
+		"fail/nil-payload": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), jwsContextKey, jws)
+			ctx = context.WithValue(ctx, provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, nil)
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("payload does not exist"),
+			}
+		},
+		"fail/unmarshal-payload": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			malformedPayload := []byte(`{"payload":malformed?}`)
+			ctx := context.WithValue(context.Background(), jwsContextKey, jws)
+			ctx = context.WithValue(ctx, provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: malformedPayload})
+			return test{
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("error unmarshaling payload"),
+			}
+		},
 		"fail/wrong-certificate-encoding": func(t *testing.T) test {
 			rp := &revokePayload{
 				Certificate: base64.StdEncoding.EncodeToString(cert.Raw),
@@ -314,18 +634,10 @@ func TestHandler_RevokeCert(t *testing.T) {
 					},
 				},
 			}
-			acc := &acme.Account{ID: "accountID", Status: acme.StatusInvalid}
 			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
-			ctx = context.WithValue(ctx, accContextKey, acc)
 			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: wronglyEncodedPayloadBytes})
 			ctx = context.WithValue(ctx, jwsContextKey, jws)
-			ctx = context.WithValue(ctx, baseURLContextKey, baseURL)
-			ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
-			db := &acme.MockDB{}
-			ca := &mockCA{}
 			return test{
-				db:         db,
-				ca:         ca,
 				ctx:        ctx,
 				statusCode: 400,
 				err: &acme.Error{
@@ -354,18 +666,10 @@ func TestHandler_RevokeCert(t *testing.T) {
 					},
 				},
 			}
-			acc := &acme.Account{ID: "accountID", Status: acme.StatusInvalid}
 			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
-			ctx = context.WithValue(ctx, accContextKey, acc)
 			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: wrongPayloadBytes})
 			ctx = context.WithValue(ctx, jwsContextKey, jws)
-			ctx = context.WithValue(ctx, baseURLContextKey, baseURL)
-			ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
-			db := &acme.MockDB{}
-			ca := &mockCA{}
 			return test{
-				db:         db,
-				ca:         ca,
 				ctx:        ctx,
 				statusCode: 400,
 				err: &acme.Error{
@@ -375,6 +679,96 @@ func TestHandler_RevokeCert(t *testing.T) {
 				},
 			}
 		},
+		"fail/db.GetCertificateBySerial": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					return nil, errors.New("force")
+				},
+			}
+			return test{
+				db:         db,
+				ctx:        ctx,
+				statusCode: 500,
+				err:        acme.NewErrorISE("error retrieving certificate by serial"),
+			}
+		},
+		"fail/no-account": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{}, nil
+				},
+			}
+			return test{
+				db:         db,
+				ctx:        ctx,
+				statusCode: 400,
+				err:        acme.NewError(acme.ErrorAccountDoesNotExistType, "account not in context"),
+			}
+		},
+		"fail/nil-account": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			ctx = context.WithValue(ctx, accContextKey, nil)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{}, nil
+				},
+			}
+			return test{
+				db:         db,
+				ctx:        ctx,
+				statusCode: 400,
+				err:        acme.NewError(acme.ErrorAccountDoesNotExistType, "account not in context"),
+			}
+		},
 		"fail/account-not-valid": func(t *testing.T) test {
 			jws := &jose.JSONWebSignature{
 				Signatures: []jose.Signature{
@@ -459,6 +853,43 @@ func TestHandler_RevokeCert(t *testing.T) {
 				},
 			}
 		},
+		"fail/unauthorized-certificate-key": func(t *testing.T) test {
+			_, unauthorizedKey, err := generateCertKeyPair()
+			assert.FatalError(t, err)
+			rp := &revokePayload{
+				Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw),
+				ReasonCode:  v(1),
+			}
+			jwsBytes, err := jwsEncodeJSON(rp, unauthorizedKey, "", "nonce", revokeURL)
+			assert.FatalError(t, err)
+			jws, err := jose.ParseJWS(string(jwsBytes))
+			assert.FatalError(t, err)
+			unauthorizedPayloadBytes, err := json.Marshal(rp)
+			assert.FatalError(t, err)
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: unauthorizedPayloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			ctx = context.WithValue(ctx, baseURLContextKey, baseURL)
+			ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{
+						AccountID: "accountID",
+					}, nil
+				},
+			}
+			ca := &mockCA{}
+			acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "verification of jws using certificate public key failed")
+			acmeErr.Detail = "No authorization provided for name CN=Test ACME Revoke Certificate"
+			return test{
+				db:         db,
+				ca:         ca,
+				ctx:        ctx,
+				statusCode: 403,
+				err:        acmeErr,
+			}
+		},
 		"fail/certificate-revoked-check-fails": func(t *testing.T) test {
 			jws := &jose.JSONWebSignature{
 				Signatures: []jose.Signature{
@@ -524,8 +955,6 @@ func TestHandler_RevokeCert(t *testing.T) {
 			ctx = context.WithValue(ctx, accContextKey, acc)
 			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
 			ctx = context.WithValue(ctx, jwsContextKey, jws)
-			ctx = context.WithValue(ctx, baseURLContextKey, baseURL)
-			ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx)
 			db := &acme.MockDB{
 				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
 					assert.Equals(t, cert.SerialNumber.String(), serial)
@@ -551,7 +980,194 @@ func TestHandler_RevokeCert(t *testing.T) {
 				},
 			}
 		},
-		"fail/certificate-revoke-fails": func(t *testing.T) test {
+		"fail/invalid-reasoncode": func(t *testing.T) test {
+			rp := &revokePayload{
+				Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw),
+				ReasonCode:  v(7),
+			}
+			wrongReasonCodePayloadBytes, err := json.Marshal(rp)
+			assert.FatalError(t, err)
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			acc := &acme.Account{ID: "accountID", Status: acme.StatusValid}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, accContextKey, acc)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: wrongReasonCodePayloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{
+						AccountID: "accountID",
+					}, nil
+				},
+			}
+			ca := &mockCA{
+				MockIsRevoked: func(sn string) (bool, error) {
+					return false, nil
+				},
+			}
+			return test{
+				db:         db,
+				ca:         ca,
+				ctx:        ctx,
+				statusCode: 400,
+				err: &acme.Error{
+					Type:   "urn:ietf:params:acme:error:badRevocationReason",
+					Detail: "The revocation reason provided is not allowed by the server",
+					Status: 400,
+				},
+			}
+		},
+		"fail/prov.AuthorizeRevoke": func(t *testing.T) test {
+			assert.FatalError(t, err)
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			mockACMEProv := &acme.MockProvisioner{
+				MauthorizeRevoke: func(ctx context.Context, token string) error {
+					return errors.New("force")
+				},
+			}
+			acc := &acme.Account{ID: "accountID", Status: acme.StatusValid}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, mockACMEProv)
+			ctx = context.WithValue(ctx, accContextKey, acc)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{
+						AccountID: "accountID",
+					}, nil
+				},
+			}
+			ca := &mockCA{
+				MockIsRevoked: func(sn string) (bool, error) {
+					return false, nil
+				},
+			}
+			return test{
+				db:         db,
+				ca:         ca,
+				ctx:        ctx,
+				statusCode: 500,
+				err: &acme.Error{
+					Type:   "urn:ietf:params:acme:error:serverInternal",
+					Detail: "The server experienced an internal error",
+					Status: 500,
+				},
+			}
+		},
+		"fail/ca.Revoke": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			acc := &acme.Account{ID: "accountID", Status: acme.StatusValid}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, accContextKey, acc)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{
+						AccountID: "accountID",
+					}, nil
+				},
+			}
+			ca := &mockCA{
+				MockRevoke: func(ctx context.Context, opts *authority.RevokeOptions) error {
+					return errors.New("force")
+				},
+			}
+			return test{
+				db:         db,
+				ca:         ca,
+				ctx:        ctx,
+				statusCode: 500,
+				err: &acme.Error{
+					Type:   "urn:ietf:params:acme:error:serverInternal",
+					Detail: "The server experienced an internal error",
+					Status: 500,
+				},
+			}
+		},
+		"fail/ca.Revoke-already-revoked": func(t *testing.T) test {
+			jws := &jose.JSONWebSignature{
+				Signatures: []jose.Signature{
+					{
+						Protected: jose.Header{
+							Algorithm: jose.ES256,
+							KeyID:     "bar",
+							ExtraHeaders: map[jose.HeaderKey]interface{}{
+								"url": revokeURL,
+							},
+						},
+					},
+				},
+			}
+			acc := &acme.Account{ID: "accountID", Status: acme.StatusValid}
+			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
+			ctx = context.WithValue(ctx, accContextKey, acc)
+			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
+			ctx = context.WithValue(ctx, jwsContextKey, jws)
+			db := &acme.MockDB{
+				MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) {
+					assert.Equals(t, cert.SerialNumber.String(), serial)
+					return &acme.Certificate{
+						AccountID: "accountID",
+					}, nil
+				},
+			}
+			ca := &mockCA{
+				MockIsRevoked: func(sn string) (bool, error) {
+					return false, nil
+				},
+				MockRevoke: func(ctx context.Context, opts *authority.RevokeOptions) error {
+					return fmt.Errorf("certificate with serial number '%s' is already revoked", cert.SerialNumber.String())
+				},
+			}
+			return test{
+				db:         db,
+				ca:         ca,
+				ctx:        ctx,
+				statusCode: 400,
+				err:        acme.NewError(acme.ErrorAlreadyRevokedType, "certificate with serial number '%s' is already revoked", cert.SerialNumber.String()),
+			}
+		},
+		"ok/using-account-key": func(t *testing.T) test {
 			jws := &jose.JSONWebSignature{
 				Signatures: []jose.Signature{
 					{
@@ -580,40 +1196,26 @@ func TestHandler_RevokeCert(t *testing.T) {
 					}, nil
 				},
 			}
-			ca := &mockCA{
+			ca := &mockCA{}
-				MockRevoke: func(ctx context.Context, opts *authority.RevokeOptions) error {
-					return errors.New("force")
-				},
-			}
 			return test{
 				db:         db,
 				ca:         ca,
 				ctx:        ctx,
-				statusCode: 500,
+				statusCode: 200,
-				err: &acme.Error{
-					Type:   "urn:ietf:params:acme:error:serverInternal",
-					Detail: "The server experienced an internal error",
-					Status: 500,
-				},
 			}
 		},
-		"ok/using-account-key": func(t *testing.T) test {
+		"ok/using-certificate-key": func(t *testing.T) test {
-			jws := &jose.JSONWebSignature{
+			rp := &revokePayload{
-				Signatures: []jose.Signature{
+				Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw),
-					{
+				ReasonCode:  v(1),
-						Protected: jose.Header{
-							Algorithm: jose.ES256,
-							KeyID:     "bar",
-							ExtraHeaders: map[jose.HeaderKey]interface{}{
-								"url": revokeURL,
-							},
-						},
-					},
-				},
 			}
-			acc := &acme.Account{ID: "accountID", Status: acme.StatusValid}
+			jwsBytes, err := jwsEncodeJSON(rp, key, "", "nonce", revokeURL)
+			assert.FatalError(t, err)
+			jws, err := jose.ParseJWS(string(jwsBytes))
+			assert.FatalError(t, err)
+			payloadBytes, err := json.Marshal(rp)
+			assert.FatalError(t, err)
 			ctx := context.WithValue(context.Background(), provisionerContextKey, prov)
-			ctx = context.WithValue(ctx, accContextKey, acc)
 			ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: payloadBytes})
 			ctx = context.WithValue(ctx, jwsContextKey, jws)
 			ctx = context.WithValue(ctx, baseURLContextKey, baseURL)