From 2d357da99b23a72756e2f1aee0cf4e2fee7f9dfe Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 26 Nov 2021 17:27:42 +0100 Subject: [PATCH] Add tests for ACME revocation --- acme/api/handler.go | 6 +- acme/api/revoke.go | 58 +++- acme/api/revoke_test.go | 670 ++++++++++++++++++++++++++++++++++++++++ acme/common.go | 1 + acme/errors.go | 2 +- acme/order_test.go | 4 + authority/authority.go | 13 + authority/authorize.go | 14 +- ca/ca.go | 2 +- 9 files changed, 738 insertions(+), 32 deletions(-) create mode 100644 acme/api/revoke_test.go diff --git a/acme/api/handler.go b/acme/api/handler.go index a459c0af..d6153184 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -104,13 +104,13 @@ func (h *Handler) Route(r api.Router) { return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(next)))))) } extractPayloadByJWK := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.extractJWK(h.verifyAndExtractJWSPayload(next))) + return validatingMiddleware(h.validateJWS(h.extractJWK(h.verifyAndExtractJWSPayload(next)))) } extractPayloadByKid := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.lookupJWK(h.verifyAndExtractJWSPayload(next))) + return validatingMiddleware(h.validateJWS(h.lookupJWK(h.verifyAndExtractJWSPayload(next)))) } extractPayloadByKidOrJWK := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.extractOrLookupJWK(h.verifyAndExtractJWSPayload(next))) + return validatingMiddleware(h.validateJWS(h.extractOrLookupJWK(h.verifyAndExtractJWSPayload(next)))) } r.MethodFunc("POST", getPath(NewAccountLinkType, "{provisionerID}"), extractPayloadByJWK(h.NewAccount)) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 4633b7c9..e92058e8 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -4,6 +4,7 @@ import ( "crypto/x509" "encoding/base64" "encoding/json" + "fmt" "net/http" "strings" @@ -37,28 +38,30 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { return } - p, err := payloadFromContext(ctx) + payload, err := payloadFromContext(ctx) if err != nil { api.WriteError(w, err) return } - var payload revokePayload - err = json.Unmarshal(p.value, &payload) + var p revokePayload + err = json.Unmarshal(payload.value, &p) if err != nil { api.WriteError(w, acme.WrapErrorISE(err, "error unmarshaling payload")) return } - certBytes, err := base64.RawURLEncoding.DecodeString(payload.Certificate) + certBytes, err := base64.RawURLEncoding.DecodeString(p.Certificate) if err != nil { - api.WriteError(w, acme.WrapErrorISE(err, "error decoding base64 certificate")) + // in this case the most likely cause is a client that didn't properly encode the certificate + api.WriteError(w, acme.WrapError(acme.ErrorMalformedType, err, "error base64url decoding payload certificate property")) return } certToBeRevoked, err := x509.ParseCertificate(certBytes) if err != nil { - api.WriteError(w, acme.WrapErrorISE(err, "error parsing certificate")) + // in this case a client may have encoded something different than a certificate + api.WriteError(w, acme.WrapError(acme.ErrorMalformedType, err, "error parsing certificate")) return } @@ -76,28 +79,38 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { return } if !account.IsValid() { - api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType, - "account '%s' has status '%s'", account.ID, account.Status)) + api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, fmt.Sprintf("account '%s' has status '%s'", account.ID, account.Status), nil)) return } - if existingCert.AccountID != account.ID { - api.WriteError(w, acme.NewError(acme.ErrorUnauthorizedType, - "account '%s' does not own certificate '%s'", account.ID, existingCert.ID)) + if existingCert.AccountID != account.ID { // TODO: combine with the below; ony one of the two has to be true + api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, fmt.Sprintf("account '%s' does not own certificate '%s'", account.ID, existingCert.ID), nil)) return } - // TODO: check "an account that holds authorizations for all of the identifiers in the certificate." + // TODO: check and implement "an account that holds authorizations for all of the identifiers in the certificate." + // In that case the certificate may not have been created by this account, but another account that was authorized before. } 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 { - api.WriteError(w, acme.WrapError(acme.ErrorUnauthorizedType, err, - "verification of jws using certificate public key failed")) + api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, "verification of jws using certificate public key failed", err)) return } } - reasonCode := payload.ReasonCode + hasBeenRevokedBefore, err := h.ca.IsRevoked(serial) + if err != nil { + api.WriteError(w, acme.WrapErrorISE(err, "error retrieving revocation status of certificate")) + return + } + + if hasBeenRevokedBefore { + api.WriteError(w, acme.NewError(acme.ErrorAlreadyRevokedType, "certificate was already revoked")) + return + } + + reasonCode := p.ReasonCode acmeErr := validateReasonCode(reasonCode) if acmeErr != nil { api.WriteError(w, acmeErr) @@ -134,6 +147,21 @@ func wrapRevokeErr(err error) *acme.Error { return acme.WrapErrorISE(err, "error when revoking certificate") } +// unauthorizedError returns an ACME error indicating the request was +// not authorized to revoke the certificate. +func wrapUnauthorizedError(cert *x509.Certificate, msg string, err error) *acme.Error { + var acmeErr *acme.Error + if err == nil { + acmeErr = acme.NewError(acme.ErrorUnauthorizedType, msg) + } else { + 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? + + return acmeErr +} + // logRevoke logs successful revocation of certificate func logRevoke(w http.ResponseWriter, ri *authority.RevokeOptions) { if rl, ok := w.(logging.ResponseLogger); ok { diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go new file mode 100644 index 00000000..033934c6 --- /dev/null +++ b/acme/api/revoke_test.go @@ -0,0 +1,670 @@ +package api + +import ( + "bytes" + "context" + "crypto/x509" + "encoding/base64" + "encoding/json" + "encoding/pem" + "fmt" + "io" + "net/http/httptest" + "net/url" + "reflect" + "testing" + + "github.com/go-chi/chi" + "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" + "golang.org/x/crypto/ocsp" +) + +const ( + 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 { + block, _ := pem.Decode([]byte(data)) + if block == nil { + panic("failed to parse certificate PEM") + } + cert, err := x509.ParseCertificate(block.Bytes) + if err != nil { + panic("failed to parse certificate: " + err.Error()) + } + return cert +} + +type mockCA struct { + MockIsRevoked func(sn string) (bool, error) + MockRevoke func(ctx context.Context, opts *authority.RevokeOptions) error +} + +func (m *mockCA) Sign(cr *x509.CertificateRequest, opts provisioner.SignOptions, signOpts ...provisioner.SignOption) ([]*x509.Certificate, error) { + return nil, nil +} + +func (m *mockCA) IsRevoked(sn string) (bool, error) { + if m.MockIsRevoked != nil { + return m.MockIsRevoked(sn) + } + return false, nil +} + +func (m *mockCA) Revoke(ctx context.Context, opts *authority.RevokeOptions) error { + if m.MockRevoke != nil { + return m.MockRevoke(ctx, opts) + } + return nil +} + +func (m *mockCA) LoadProvisionerByName(string) (provisioner.Interface, error) { + return nil, nil +} + +func Test_validateReasonCode(t *testing.T) { + tests := []struct { + name string + reasonCode *int + want *acme.Error + }{ + { + name: "ok", + reasonCode: v(ocsp.Unspecified), + want: nil, + }, + { + name: "fail/too-low", + reasonCode: v(-1), + want: acme.NewError(acme.ErrorBadRevocationReasonType, "reasonCode out of bounds"), + }, + { + name: "fail/too-high", + reasonCode: v(11), + want: acme.NewError(acme.ErrorBadRevocationReasonType, "reasonCode out of bounds"), + }, + { + name: "fail/missing-7", + reasonCode: v(7), + + want: acme.NewError(acme.ErrorBadRevocationReasonType, "reasonCode out of bounds"), + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateReasonCode(tt.reasonCode) + if (err != nil) != (tt.want != nil) { + t.Errorf("validateReasonCode() = %v, want %v", err, tt.want) + } + if err != nil { + assert.Equals(t, err.Type, tt.want.Type) + assert.Equals(t, err.Detail, tt.want.Detail) + assert.Equals(t, err.Status, tt.want.Status) + assert.Equals(t, err.Err.Error(), tt.want.Err.Error()) + assert.Equals(t, err.Detail, tt.want.Detail) + } + }) + } +} + +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 + want string + }{ + { + name: "unspecified reason", + reasonCode: ocsp.Unspecified, + want: "unspecified reason", + }, + { + name: "key compromised", + reasonCode: ocsp.KeyCompromise, + want: "key compromised", + }, + { + name: "ca compromised", + reasonCode: ocsp.CACompromise, + want: "ca compromised", + }, + { + name: "affiliation changed", + reasonCode: ocsp.AffiliationChanged, + want: "affiliation changed", + }, + { + name: "superseded", + reasonCode: ocsp.Superseded, + want: "superseded", + }, + { + name: "cessation of operation", + reasonCode: ocsp.CessationOfOperation, + want: "cessation of operation", + }, + { + name: "certificate hold", + reasonCode: ocsp.CertificateHold, + want: "certificate hold", + }, + { + name: "remove from crl", + reasonCode: ocsp.RemoveFromCRL, + want: "remove from crl", + }, + { + name: "privilege withdrawn", + reasonCode: ocsp.PrivilegeWithdrawn, + want: "privilege withdrawn", + }, + { + name: "aa compromised", + reasonCode: ocsp.AACompromise, + want: "aa compromised", + }, + { + name: "default", + reasonCode: -1, + want: "unspecified reason", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := reason(tt.reasonCode); got != tt.want { + t.Errorf("reason() = %v, want %v", got, tt.want) + } + }) + } +} + +func Test_revokeOptions(t *testing.T) { + var cert *x509.Certificate + type args struct { + serial string + certToBeRevoked *x509.Certificate + reasonCode *int + } + tests := []struct { + name string + args args + want *authority.RevokeOptions + }{ + { + name: "ok/no-reasoncode", + args: args{ + serial: "1234", + certToBeRevoked: cert, + }, + want: &authority.RevokeOptions{ + Serial: "1234", + Crt: nil, + ACME: true, + }, + }, + { + name: "ok/including-reasoncode", + args: args{ + serial: "1234", + certToBeRevoked: cert, + reasonCode: v(ocsp.KeyCompromise), + }, + want: &authority.RevokeOptions{ + Serial: "1234", + Crt: nil, + ACME: true, + ReasonCode: 1, + Reason: "key compromised", + }, + }, + } + 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) { + t.Errorf("revokeOptions() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestHandler_RevokeCert(t *testing.T) { + prov := &provisioner.ACME{ + Type: "ACME", + Name: "testprov", + } + escProvName := url.PathEscape(prov.GetName()) + baseURL := &url.URL{Scheme: "https", Host: "test.ca.smallstep.com"} + + chiCtx := chi.NewRouteContext() + revokeURL := fmt.Sprintf("%s/acme/%s/revoke-cert", baseURL.String(), escProvName) + + cert := parseCertificate(certPEM) + rp := &revokePayload{ + Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), + } + payloadBytes, err := json.Marshal(rp) + assert.FatalError(t, err) + + type test struct { + db acme.DB + ca acme.CertificateAuthority + ctx context.Context + statusCode int + err *acme.Error + } + + var tests = map[string]func(t *testing.T) test{ + "fail/wrong-certificate-encoding": func(t *testing.T) test { + rp := &revokePayload{ + Certificate: base64.StdEncoding.EncodeToString(cert.Raw), + } + wronglyEncodedPayloadBytes, 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.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{ + Type: "urn:ietf:params:acme:error:malformed", + Status: 400, + Detail: "The request message was malformed", + }, + } + }, + "fail/no-certificate-encoded": func(t *testing.T) test { + rp := &revokePayload{ + Certificate: base64.RawURLEncoding.EncodeToString([]byte{}), + } + wrongPayloadBytes, 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.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{ + Type: "urn:ietf:params:acme:error:malformed", + Status: 400, + Detail: "The request message was malformed", + }, + } + }, + "fail/account-not-valid": 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.StatusInvalid} + 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) + 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{} + return test{ + db: db, + ca: ca, + ctx: ctx, + statusCode: 403, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Detail: fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()), + Status: 403, + }, + } + }, + "fail/account-not-authorized": 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) + 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: "differentAccountID", + }, nil + }, + } + ca := &mockCA{} + return test{ + db: db, + ca: ca, + ctx: ctx, + statusCode: 403, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Detail: fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()), + Status: 403, + }, + } + }, + "fail/certificate-revoked-check-fails": 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) + 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{ + MockIsRevoked: func(sn string) (bool, error) { + return false, 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/certificate-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) + 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{ + MockIsRevoked: func(sn string) (bool, error) { + return true, nil + }, + } + return test{ + db: db, + ca: ca, + ctx: ctx, + statusCode: 400, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:alreadyRevoked", + Detail: "Certificate already revoked", + Status: 400, + }, + } + }, + "fail/certificate-revoke-fails": 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) + 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{ + 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, + }, + } + }, + "ok/using-account-key": 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) + 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{} + return test{ + db: db, + ca: ca, + ctx: ctx, + statusCode: 200, + } + }, + } + for name, setup := range tests { + tc := setup(t) + t.Run(name, func(t *testing.T) { + h := &Handler{linker: NewLinker("dns", "acme"), db: tc.db, ca: tc.ca} + req := httptest.NewRequest("POST", revokeURL, nil) + req = req.WithContext(tc.ctx) + w := httptest.NewRecorder() + h.RevokeCert(w, req) + res := w.Result() + + assert.Equals(t, res.StatusCode, tc.statusCode) + + body, err := io.ReadAll(res.Body) + res.Body.Close() + assert.FatalError(t, err) + + if res.StatusCode >= 400 && assert.NotNil(t, tc.err) { + var ae acme.Error + assert.FatalError(t, json.Unmarshal(bytes.TrimSpace(body), &ae)) + + assert.Equals(t, ae.Type, tc.err.Type) + assert.Equals(t, ae.Detail, tc.err.Detail) + assert.Equals(t, ae.Identifier, tc.err.Identifier) + assert.Equals(t, ae.Subproblems, tc.err.Subproblems) + assert.Equals(t, res.Header["Content-Type"], []string{"application/problem+json"}) + } else { + assert.True(t, bytes.Equal(bytes.TrimSpace(body), []byte{})) + assert.Equals(t, int64(0), req.ContentLength) + assert.Equals(t, []string{fmt.Sprintf("<%s/acme/%s/directory>;rel=\"index\"", baseURL.String(), escProvName)}, res.Header["Link"]) + } + }) + } +} diff --git a/acme/common.go b/acme/common.go index 2613c397..0c9e83dc 100644 --- a/acme/common.go +++ b/acme/common.go @@ -12,6 +12,7 @@ import ( // CertificateAuthority is the interface implemented by a CA authority. type CertificateAuthority interface { Sign(cr *x509.CertificateRequest, opts provisioner.SignOptions, signOpts ...provisioner.SignOption) ([]*x509.Certificate, error) + IsRevoked(sn string) (bool, error) Revoke(context.Context, *authority.RevokeOptions) error LoadProvisionerByName(string) (provisioner.Interface, error) } diff --git a/acme/errors.go b/acme/errors.go index 6ecf0912..a5c820ba 100644 --- a/acme/errors.go +++ b/acme/errors.go @@ -147,7 +147,7 @@ var ( }, ErrorAlreadyRevokedType: { typ: officialACMEPrefix + ErrorAlreadyRevokedType.String(), - details: "Certificate already Revoked", + details: "Certificate already revoked", status: 400, }, ErrorBadCSRType: { diff --git a/acme/order_test.go b/acme/order_test.go index 4b940bdb..a90982a6 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -287,6 +287,10 @@ func (m *mockSignAuth) LoadProvisionerByName(name string) (provisioner.Interface return m.ret1.(provisioner.Interface), m.err } +func (m *mockSignAuth) IsRevoked(sn string) (bool, error) { + return false, nil +} + func (m *mockSignAuth) Revoke(context.Context, *authority.RevokeOptions) error { return nil } diff --git a/authority/authority.go b/authority/authority.go index aa8698d7..b6fcdf23 100644 --- a/authority/authority.go +++ b/authority/authority.go @@ -588,6 +588,19 @@ func (a *Authority) CloseForReload() { } } +// IsRevoked returns whether or not a certificate has been +// revoked before. +func (a *Authority) IsRevoked(sn string) (bool, error) { + // Check the passive revocation table. + if lca, ok := a.adminDB.(interface { + IsRevoked(string) (bool, error) + }); ok { + return lca.IsRevoked(sn) + } + + return a.db.IsRevoked(sn) +} + // requiresDecrypter returns whether the Authority // requires a KMS that provides a crypto.Decrypter // Currently this is only required when SCEP is diff --git a/authority/authorize.go b/authority/authorize.go index a4e7e591..5108f567 100644 --- a/authority/authorize.go +++ b/authority/authorize.go @@ -274,19 +274,9 @@ func (a *Authority) authorizeRevoke(ctx context.Context, token string) error { // // TODO(mariano): should we authorize by default? func (a *Authority) authorizeRenew(cert *x509.Certificate) error { - var err error - var isRevoked bool - var opts = []interface{}{errs.WithKeyVal("serialNumber", cert.SerialNumber.String())} - - // Check the passive revocation table. serial := cert.SerialNumber.String() - if lca, ok := a.adminDB.(interface { - IsRevoked(string) (bool, error) - }); ok { - isRevoked, err = lca.IsRevoked(serial) - } else { - isRevoked, err = a.db.IsRevoked(serial) - } + var opts = []interface{}{errs.WithKeyVal("serialNumber", serial)} + isRevoked, err := a.IsRevoked(serial) if err != nil { return errs.Wrap(http.StatusInternalServerError, err, "authority.authorizeRenew", opts...) } diff --git a/ca/ca.go b/ca/ca.go index c76e8c0a..da0fb874 100644 --- a/ca/ca.go +++ b/ca/ca.go @@ -442,7 +442,7 @@ func (ca *CA) getTLSConfig(auth *authority.Authority) (*tls.Config, error) { return tlsConfig, nil } -// shouldMountSCEPEndpoints returns if the CA should be +// shouldServeSCEPEndpoints returns if the CA should be // configured with endpoints for SCEP. This is assumed to be // true if a SCEPService exists, which is true in case a // SCEP provisioner was configured.