From d53bcaf830980ea18e958adf027fa4ec544f7e91 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 2 Jul 2021 22:51:15 +0200 Subject: [PATCH 01/20] Add base logic for ACME revoke-cert --- acme/api/handler.go | 1 + acme/api/revoke.go | 79 +++++++++++++++++++++++++++++++++++++++++++++ acme/common.go | 2 ++ 3 files changed, 82 insertions(+) create mode 100644 acme/api/revoke.go diff --git a/acme/api/handler.go b/acme/api/handler.go index 2a6d3a02..06bd4bb4 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -111,6 +111,7 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("POST", getPath(AuthzLinkType, "{provisionerID}", "{authzID}"), extractPayloadByKid(h.isPostAsGet(h.GetAuthorization))) r.MethodFunc("POST", getPath(ChallengeLinkType, "{provisionerID}", "{authzID}", "{chID}"), extractPayloadByKid(h.GetChallenge)) r.MethodFunc("POST", getPath(CertificateLinkType, "{provisionerID}", "{certID}"), extractPayloadByKid(h.isPostAsGet(h.GetCertificate))) + r.MethodFunc("POST", getPath(RevokeCertLinkType, "{provisionerID}"), extractPayloadByKid(h.RevokeCert)) // TODO: check kid vs. jws; revoke can do both } // GetNonce just sets the right header since a Nonce is added to each response diff --git a/acme/api/revoke.go b/acme/api/revoke.go new file mode 100644 index 00000000..ab537007 --- /dev/null +++ b/acme/api/revoke.go @@ -0,0 +1,79 @@ +package api + +import ( + "crypto/x509" + "encoding/base64" + "encoding/json" + "fmt" + "net/http" + + "github.com/smallstep/certificates/api" + "github.com/smallstep/certificates/authority" +) + +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) + 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 { + fmt.Println(err) + } + + // 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) + } + + type revokedCert struct { + Certificate string `json:"certificate"` + 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) + } + + c, err := base64.RawURLEncoding.DecodeString(rc.Certificate) + if err != nil { + fmt.Println("error:", err) + } + + certToBeRevoked, err := x509.ParseCertificate(c) + if err != nil { + fmt.Println("error: failed to parse certificate: " + err.Error()) + } + + // TODO: check reason code; should be allowed; otherwise send error + + options := &authority.RevokeOptions{ + Serial: certToBeRevoked.SerialNumber.String(), + Reason: "test", // TODO: map it to the reason based on code? + ReasonCode: rc.Reason, + PassiveOnly: false, + 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) + } + + w.Write(nil) +} diff --git a/acme/common.go b/acme/common.go index 26552c61..77a219a0 100644 --- a/acme/common.go +++ b/acme/common.go @@ -5,12 +5,14 @@ import ( "crypto/x509" "time" + "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" ) // 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) + Revoke(context.Context, *authority.RevokeOptions) error LoadProvisionerByID(string) (provisioner.Interface, error) } From 84e7d468f226843820419808d5505e4ee22436f3 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 3 Jul 2021 00:21:17 +0200 Subject: [PATCH 02/20] Improve handling of ACME revocation --- acme/api/revoke.go | 103 ++++++++++++++++++++++++++++++++++----------- authority/tls.go | 8 ++-- 2 files changed, 83 insertions(+), 28 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index ab537007..3dda7098 100644 --- a/acme/api/revoke.go +++ b/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 { - Certificate string `json:"certificate"` - Reason int `json:"reason"` // TODO: is optional; handle accordingly - } - - var rc revokedCert - err = json.Unmarshal(p.value, &rc) + var payload revokePayload + err = json.Unmarshal(p.value, &payload) 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(), - Reason: "test", // TODO: map it to the reason based on code? - ReasonCode: rc.Reason, - PassiveOnly: false, - 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: "", + Serial: certID, + Reason: reason(payload.ReasonCode), + ReasonCode: payload.ReasonCode, + ACME: true, + Crt: certToBeRevoked, } + 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" + } +} diff --git a/authority/tls.go b/authority/tls.go index b7b2f936..7d1579cf 100644 --- a/authority/tls.go +++ b/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 !revokeOpts.MTLS { + // If not mTLS nor ACME, then get the TokenID of the token. + if !(revokeOpts.MTLS || revokeOpts.ACME) { token, err := jose.ParseSigned(revokeOpts.OTT) if err != nil { return errs.Wrap(http.StatusUnauthorized, err, From 0e56932e76367570b68f3b163c1266c236275629 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 3 Jul 2021 01:56:14 +0200 Subject: [PATCH 03/20] Add support for revocation using JWK --- acme/api/handler.go | 12 +++++++++--- acme/api/middleware.go | 31 +++++++++++++++++++++++++++++++ acme/api/revoke.go | 24 +++++++++++++++++++----- 3 files changed, 59 insertions(+), 8 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index 06bd4bb4..8ec12a93 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -94,11 +94,17 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("GET", getPath(DirectoryLinkType, "{provisionerID}"), h.baseURLFromRequest(h.lookupProvisioner(h.GetDirectory))) r.MethodFunc("HEAD", getPath(DirectoryLinkType, "{provisionerID}"), h.baseURLFromRequest(h.lookupProvisioner(h.GetDirectory))) + validatingMiddleware := func(next nextHTTP) nextHTTP { + return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(next)))))) + } extractPayloadByJWK := func(next nextHTTP) nextHTTP { - return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(h.validateJWS(h.extractJWK(h.verifyAndExtractJWSPayload(next))))))))) + return validatingMiddleware(h.extractJWK(h.verifyAndExtractJWSPayload(next))) } extractPayloadByKid := func(next nextHTTP) nextHTTP { - return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(h.validateJWS(h.lookupJWK(h.verifyAndExtractJWSPayload(next))))))))) + return validatingMiddleware(h.lookupJWK(h.verifyAndExtractJWSPayload(next))) + } + extractPayloadByKidOrJWK := func(next nextHTTP) nextHTTP { + return validatingMiddleware(h.extractOrLookupJWK(h.verifyAndExtractJWSPayload(next))) } r.MethodFunc("POST", getPath(NewAccountLinkType, "{provisionerID}"), extractPayloadByJWK(h.NewAccount)) @@ -111,7 +117,7 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("POST", getPath(AuthzLinkType, "{provisionerID}", "{authzID}"), extractPayloadByKid(h.isPostAsGet(h.GetAuthorization))) r.MethodFunc("POST", getPath(ChallengeLinkType, "{provisionerID}", "{authzID}", "{chID}"), extractPayloadByKid(h.GetChallenge)) r.MethodFunc("POST", getPath(CertificateLinkType, "{provisionerID}", "{certID}"), extractPayloadByKid(h.isPostAsGet(h.GetCertificate))) - r.MethodFunc("POST", getPath(RevokeCertLinkType, "{provisionerID}"), extractPayloadByKid(h.RevokeCert)) // TODO: check kid vs. jws; revoke can do both + r.MethodFunc("POST", getPath(RevokeCertLinkType, "{provisionerID}"), extractPayloadByKidOrJWK(h.RevokeCert)) } // GetNonce just sets the right header since a Nonce is added to each response diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 50f7146f..3cee79c5 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -352,6 +352,37 @@ func (h *Handler) lookupJWK(next nextHTTP) nextHTTP { } } +// extractOrLookupJWK forwards handling to either extractJWK or +// lookupJWK based on the presence of a JWK or a KID, respectively. +func (h *Handler) extractOrLookupJWK(next nextHTTP) nextHTTP { + return func(w http.ResponseWriter, r *http.Request) { + ctx := r.Context() + jws, err := jwsFromContext(ctx) + if err != nil { + api.WriteError(w, err) + return + } + + // 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. + if canExtractJWKFrom(jws) { + h.extractJWK(next)(w, r) + return + } + + // default to looking up the JWK based on KID + h.lookupJWK(next)(w, r) + } +} + +// canExtractJWKFrom checks if the JWS has a JWK that can be extracted +func canExtractJWKFrom(jws *jose.JSONWebSignature) bool { + if len(jws.Signatures) == 0 { + return false + } + return jws.Signatures[0].Protected.JSONWebKey != nil +} + // verifyAndExtractJWSPayload extracts the JWK from the JWS and saves it in the context. // Make sure to parse and validate the JWS before running this middleware. func (h *Handler) verifyAndExtractJWSPayload(next nextHTTP) nextHTTP { diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 3dda7098..bf9c3906 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -9,6 +9,7 @@ import ( "github.com/smallstep/certificates/acme" "github.com/smallstep/certificates/api" "github.com/smallstep/certificates/authority" + "go.step.sm/crypto/jose" "golang.org/x/crypto/ocsp" ) @@ -19,16 +20,21 @@ type revokePayload struct { 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) - ctx := r.Context() - _, err := accountFromContext(ctx) + jws, err := jwsFromContext(ctx) if err != nil { api.WriteError(w, err) return } + if shouldCheckAccount(jws) { + _, 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) @@ -65,7 +71,7 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { } certID := certToBeRevoked.SerialNumber.String() - // TODO: retrieving the certificate to verify the account does not seem to work? Results in certificate not found error. + // 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 { @@ -101,6 +107,7 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { return } + w.Header().Add("Link", link(h.linker.GetLink(ctx, DirectoryLinkType), "index")) w.Write(nil) } @@ -130,3 +137,10 @@ func reason(reasonCode int) string { return "unspecified reason" } } + +// shouldUseAccount indicates whether an account should be +// retrieved from the context, so that it can be used for +// additional checks +func shouldCheckAccount(jws *jose.JSONWebSignature) bool { + return !canExtractJWKFrom(jws) +} From 16fe07d4dce6113001902b704da1b1805823a459 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 3 Jul 2021 02:10:16 +0200 Subject: [PATCH 04/20] Fix mockSignAuth --- acme/order_test.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/acme/order_test.go b/acme/order_test.go index 993a92f2..2127599a 100644 --- a/acme/order_test.go +++ b/acme/order_test.go @@ -10,6 +10,7 @@ import ( "github.com/pkg/errors" "github.com/smallstep/assert" + "github.com/smallstep/certificates/authority" "github.com/smallstep/certificates/authority/provisioner" ) @@ -283,6 +284,10 @@ func (m *mockSignAuth) LoadProvisionerByID(id string) (provisioner.Interface, er return m.ret1.(provisioner.Interface), m.err } +func (m *mockSignAuth) Revoke(context.Context, *authority.RevokeOptions) error { + return nil +} + func TestOrder_Finalize(t *testing.T) { type test struct { o *Order From 2b15230aa4393cc68593990f0779a3c65e35c1a6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 9 Jul 2021 17:51:31 +0200 Subject: [PATCH 05/20] Add Serial to Cert ID ACME table and lookup --- acme/api/middleware.go | 4 +- acme/api/revoke.go | 92 +++++++++++++++++++++++++----------- acme/db.go | 15 +++++- acme/db/nosql/certificate.go | 35 +++++++++++++- acme/db/nosql/nosql.go | 3 +- 5 files changed, 116 insertions(+), 33 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 3eff4571..371be7fa 100644 --- a/acme/api/middleware.go +++ b/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) } } diff --git a/acme/api/revoke.go b/acme/api/revoke.go index bf9c3906..75eb4ee2 100644 --- a/acme/api/revoke.go +++ b/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) } diff --git a/acme/db.go b/acme/db.go index d678fef4..c4b79a66 100644 --- a/acme/db.go +++ b/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 { diff --git a/acme/db/nosql/certificate.go b/acme/db/nosql/certificate.go index d3e15833..ee37c570 100644 --- a/acme/db/nosql/certificate.go +++ b/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 diff --git a/acme/db/nosql/nosql.go b/acme/db/nosql/nosql.go index 052f5729..c179d2ed 100644 --- a/acme/db/nosql/nosql.go +++ b/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", From 97165f184478134e44b2e6c8ed206650364bd7e6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 9 Jul 2021 22:48:03 +0200 Subject: [PATCH 06/20] Fix test mocking for CreateCertificate --- acme/db.go | 1 + acme/db/nosql/certificate_test.go | 40 ++++++++++++++++++++++--------- 2 files changed, 30 insertions(+), 11 deletions(-) diff --git a/acme/db.go b/acme/db.go index c4b79a66..67053269 100644 --- a/acme/db.go +++ b/acme/db.go @@ -182,6 +182,7 @@ func (m *MockDB) GetCertificate(ctx context.Context, id string) (*Certificate, e return m.MockRet1.(*Certificate), m.MockError } +// GetCertificateBySerial mock func (m *MockDB) GetCertificateBySerial(ctx context.Context, serial string) (*Certificate, error) { if m.MockGetCertificateBySerial != nil { return m.MockGetCertificateBySerial(ctx, serial) diff --git a/acme/db/nosql/certificate_test.go b/acme/db/nosql/certificate_test.go index 4ec4589e..f5a8b67f 100644 --- a/acme/db/nosql/certificate_test.go +++ b/acme/db/nosql/certificate_test.go @@ -31,6 +31,7 @@ func TestDB_CreateCertificate(t *testing.T) { err error _id *string } + countOfCmpAndSwapCalls := 0 var tests = map[string]func(t *testing.T) test{ "fail/cmpAndSwap-error": func(t *testing.T) test { cert := &acme.Certificate{ @@ -75,18 +76,35 @@ func TestDB_CreateCertificate(t *testing.T) { return test{ db: &db.MockNoSQLDB{ MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) { - *idPtr = string(key) - assert.Equals(t, bucket, certTable) - assert.Equals(t, key, []byte(cert.ID)) - assert.Equals(t, old, nil) + if countOfCmpAndSwapCalls == 0 { + *idPtr = string(key) + assert.Equals(t, bucket, certTable) + assert.Equals(t, key, []byte(cert.ID)) + assert.Equals(t, old, nil) + + dbc := new(dbCert) + assert.FatalError(t, json.Unmarshal(nu, dbc)) + assert.Equals(t, dbc.ID, string(key)) + assert.Equals(t, dbc.ID, cert.ID) + assert.Equals(t, dbc.AccountID, cert.AccountID) + assert.True(t, clock.Now().Add(-time.Minute).Before(dbc.CreatedAt)) + assert.True(t, clock.Now().Add(time.Minute).After(dbc.CreatedAt)) + } + if countOfCmpAndSwapCalls == 1 { + assert.Equals(t, bucket, certBySerialTable) + assert.Equals(t, key, []byte(cert.Leaf.SerialNumber.String())) + assert.Equals(t, old, nil) + + dbs := new(dbSerial) + assert.FatalError(t, json.Unmarshal(nu, dbs)) + assert.Equals(t, dbs.Serial, string(key)) + assert.Equals(t, dbs.CertificateID, cert.ID) + + *idPtr = cert.ID + } + + countOfCmpAndSwapCalls += 1 - dbc := new(dbCert) - assert.FatalError(t, json.Unmarshal(nu, dbc)) - assert.Equals(t, dbc.ID, string(key)) - assert.Equals(t, dbc.ID, cert.ID) - assert.Equals(t, dbc.AccountID, cert.AccountID) - assert.True(t, clock.Now().Add(-time.Minute).Before(dbc.CreatedAt)) - assert.True(t, clock.Now().Add(time.Minute).After(dbc.CreatedAt)) return nil, true, nil }, }, From 258efca0fa83785695021c058f50073807447749 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 10 Jul 2021 00:28:31 +0200 Subject: [PATCH 07/20] Improve revocation authorization --- acme/api/middleware.go | 1 + acme/api/revoke.go | 82 ++++++++++++++++------- acme/common.go | 10 +++ acme/db/nosql/certificate_test.go | 2 +- authority/provisioner/acme.go | 9 +++ authority/provisioner/provisioner_test.go | 1 - 6 files changed, 78 insertions(+), 27 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 371be7fa..cb1df487 100644 --- a/acme/api/middleware.go +++ b/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) } } diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 75eb4ee2..4633b7c9 100644 --- a/acme/api/revoke.go +++ b/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,24 +31,12 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { return } - if shouldCheckAccount(jws) { - _, 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) + prov, 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 { api.WriteError(w, err) @@ -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 { - // 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. KeyID revoke) - // } - - // TODO: validate the certToBeRevoked against what we know about it? + if shouldCheckAccountFrom(jws) { + account, err := accountFromContext(ctx) + if err != nil { + api.WriteError(w, err) + return + } + 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. -func shouldCheckAccount(jws *jose.JSONWebSignature) bool { +// additional checks. This should only be done when no JWK +// 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) } diff --git a/acme/common.go b/acme/common.go index ef1bf26e..2613c397 100644 --- a/acme/common.go +++ b/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 { diff --git a/acme/db/nosql/certificate_test.go b/acme/db/nosql/certificate_test.go index f5a8b67f..baa19383 100644 --- a/acme/db/nosql/certificate_test.go +++ b/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 }, diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index d81b0231..25821051 100644 --- a/authority/provisioner/acme.go +++ b/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 diff --git a/authority/provisioner/provisioner_test.go b/authority/provisioner/provisioner_test.go index 3d895277..330d1b57 100644 --- a/authority/provisioner/provisioner_test.go +++ b/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}, From c7a9c13060c2f8c257572e77eb5a859347c75647 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 12 Nov 2021 16:37:44 +0100 Subject: [PATCH 08/20] Add tests for extractOrLookupJWK middleware --- acme/api/middleware.go | 3 + acme/api/middleware_test.go | 188 ++++++++++++++++++++++++++++++++++++ 2 files changed, 191 insertions(+) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index 55bc96bf..99980f50 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -378,6 +378,9 @@ func (h *Handler) extractOrLookupJWK(next nextHTTP) nextHTTP { // canExtractJWKFrom checks if the JWS has a JWK that can be extracted func canExtractJWKFrom(jws *jose.JSONWebSignature) bool { + if jws == nil { + return false + } if len(jws.Signatures) == 0 { return false } diff --git a/acme/api/middleware_test.go b/acme/api/middleware_test.go index e8d22d53..1cc93de7 100644 --- a/acme/api/middleware_test.go +++ b/acme/api/middleware_test.go @@ -1473,3 +1473,191 @@ func TestHandler_validateJWS(t *testing.T) { }) } } + +func Test_canExtractJWKFrom(t *testing.T) { + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + type args struct { + jws *jose.JSONWebSignature + } + tests := []struct { + name string + args args + want bool + }{ + { + name: "no-jws", + args: args{ + jws: nil, + }, + want: false, + }, + { + name: "no-signatures", + args: args{ + jws: &jose.JSONWebSignature{ + Signatures: []jose.Signature{}, + }, + }, + want: false, + }, + { + name: "no-jwk", + args: args{ + jws: &jose.JSONWebSignature{ + Signatures: []jose.Signature{ + { + Protected: jose.Header{}, + }, + }, + }, + }, + want: false, + }, + { + name: "ok", + args: args{ + jws: &jose.JSONWebSignature{ + Signatures: []jose.Signature{ + { + Protected: jose.Header{ + JSONWebKey: jwk, + }, + }, + }, + }, + }, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := canExtractJWKFrom(tt.args.jws); got != tt.want { + t.Errorf("canExtractJWKFrom() = %v, want %v", got, tt.want) + } + }) + } +} + +func TestHandler_extractOrLookupJWK(t *testing.T) { + u := "https://ca.smallstep.com/acme/account" + type test struct { + db acme.DB + linker Linker + statusCode int + ctx context.Context + err *acme.Error + next func(w http.ResponseWriter, r *http.Request) + } + var tests = map[string]func(t *testing.T) test{ + "ok/extract": func(t *testing.T) test { + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + kid, err := jwk.Thumbprint(crypto.SHA256) + assert.FatalError(t, err) + pub := jwk.Public() + pub.KeyID = base64.RawURLEncoding.EncodeToString(kid) + so := new(jose.SignerOptions) + so.WithHeader("jwk", pub) + signer, err := jose.NewSigner(jose.SigningKey{ + Algorithm: jose.SignatureAlgorithm(jwk.Algorithm), + Key: jwk.Key, + }, so) + assert.FatalError(t, err) + signed, err := signer.Sign([]byte("foo")) + assert.FatalError(t, err) + raw, err := signed.CompactSerialize() + assert.FatalError(t, err) + parsedJWS, err := jose.ParseJWS(raw) + assert.FatalError(t, err) + assert.FatalError(t, err) + acc := &acme.Account{Status: "valid"} + return test{ + linker: NewLinker("dns", "acme"), + db: &acme.MockDB{ + MockGetAccountByKeyID: func(ctx context.Context, kid string) (*acme.Account, error) { + assert.Equals(t, kid, pub.KeyID) + return acc, nil + }, + }, + ctx: context.WithValue(context.Background(), jwsContextKey, parsedJWS), + statusCode: 200, + next: func(w http.ResponseWriter, r *http.Request) { + w.Write(testBody) + }, + } + }, + "ok/lookup": func(t *testing.T) test { + prov := newProv() + provName := url.PathEscape(prov.GetName()) + baseURL := &url.URL{Scheme: "https", Host: "test.ca.smallstep.com"} + jwk, err := jose.GenerateJWK("EC", "P-256", "ES256", "sig", "", 0) + assert.FatalError(t, err) + accID := "accID" + prefix := fmt.Sprintf("%s/acme/%s/account/", baseURL, provName) + so := new(jose.SignerOptions) + so.WithHeader("kid", fmt.Sprintf("%s%s", prefix, accID)) + signer, err := jose.NewSigner(jose.SigningKey{ + Algorithm: jose.SignatureAlgorithm(jwk.Algorithm), + Key: jwk.Key, + }, so) + assert.FatalError(t, err) + jws, err := signer.Sign([]byte("baz")) + assert.FatalError(t, err) + raw, err := jws.CompactSerialize() + assert.FatalError(t, err) + parsedJWS, err := jose.ParseJWS(raw) + assert.FatalError(t, err) + assert.FatalError(t, err) + //acc := &acme.Account{Status: "valid", Key: jwk} + acc := &acme.Account{ID: "accID", Key: jwk, Status: "valid"} + ctx := context.WithValue(context.Background(), provisionerContextKey, prov) + ctx = context.WithValue(ctx, baseURLContextKey, baseURL) + ctx = context.WithValue(ctx, jwsContextKey, parsedJWS) + return test{ + linker: NewLinker("test.ca.smallstep.com", "acme"), + db: &acme.MockDB{ + MockGetAccount: func(ctx context.Context, accID string) (*acme.Account, error) { + assert.Equals(t, accID, acc.ID) + return acc, nil + }, + }, + ctx: ctx, + statusCode: 200, + next: func(w http.ResponseWriter, r *http.Request) { + w.Write(testBody) + }, + } + }, + } + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + h := &Handler{db: tc.db, linker: tc.linker} + req := httptest.NewRequest("GET", u, nil) + req = req.WithContext(tc.ctx) + w := httptest.NewRecorder() + h.extractOrLookupJWK(tc.next)(w, req) + res := w.Result() + + assert.Equals(t, res.StatusCode, tc.statusCode) + + body, err := ioutil.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.Equals(t, bytes.TrimSpace(body), testBody) + } + }) + } +} From 42f56d6906b2e5731457da71f18bc979906d1a0e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 12 Nov 2021 17:03:41 +0100 Subject: [PATCH 09/20] Set golangci-lint version to v1.41.0 instead of latest Checking if this solves the issue with new linting issues that, at least locally, seem to have been introduced between v1.41.0 and latest (v1.43.0). --- .github/workflows/test.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 96655664..d3cc28e0 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -33,7 +33,7 @@ jobs: uses: golangci/golangci-lint-action@v2 with: # Optional: version of golangci-lint to use in form of v1.2 or v1.2.3 or `latest` to use the latest version - version: 'latest' + version: 'v1.41.0' # Optional: working directory, useful for monorepos # working-directory: somedir From 29f9730485d1036f09e06f70bba9b21f5016c1a2 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 12 Nov 2021 17:13:10 +0100 Subject: [PATCH 10/20] Satisfy golangci-lint --- acme/challenge.go | 7 +++++-- acme/order.go | 4 +++- 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/acme/challenge.go b/acme/challenge.go index b880708c..c3f8dde8 100644 --- a/acme/challenge.go +++ b/acme/challenge.go @@ -26,8 +26,11 @@ import ( type ChallengeType string const ( - HTTP01 ChallengeType = "http-01" - DNS01 ChallengeType = "dns-01" + // HTTP01 is the http-01 ACME challenge type + HTTP01 ChallengeType = "http-01" + // DNS01 is the dns-01 ACME challenge type + DNS01 ChallengeType = "dns-01" + // TLSALPN01 is the tls-alpn-01 ACME challenge type TLSALPN01 ChallengeType = "tls-alpn-01" ) diff --git a/acme/order.go b/acme/order.go index 237c6979..bd820da1 100644 --- a/acme/order.go +++ b/acme/order.go @@ -17,7 +17,9 @@ import ( type IdentifierType string const ( - IP IdentifierType = "ip" + // IP is the ACME ip identifier type + IP IdentifierType = "ip" + // DNS is the ACME dns identifier type DNS IdentifierType = "dns" ) From ed295ca15d3c66bb14eec894c7ac2acf1b3a5ebc Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 25 Nov 2021 00:44:21 +0100 Subject: [PATCH 11/20] Fix linting issue --- acme/api/middleware_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/acme/api/middleware_test.go b/acme/api/middleware_test.go index d5aaee21..7b8ae828 100644 --- a/acme/api/middleware_test.go +++ b/acme/api/middleware_test.go @@ -1641,7 +1641,7 @@ func TestHandler_extractOrLookupJWK(t *testing.T) { assert.Equals(t, res.StatusCode, tc.statusCode) - body, err := ioutil.ReadAll(res.Body) + body, err := io.ReadAll(res.Body) res.Body.Close() assert.FatalError(t, err) From 2d357da99b23a72756e2f1aee0cf4e2fee7f9dfe Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 26 Nov 2021 17:27:42 +0100 Subject: [PATCH 12/20] 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. From 4d01cf8135216223e66544f455fcc3827a0eb7c6 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sun, 28 Nov 2021 20:30:36 +0100 Subject: [PATCH 13/20] Increase test code coverage --- acme/api/revoke.go | 6 +- acme/api/revoke_test.go | 796 +++++++++++++++++++++++++++++++++++----- 2 files changed, 702 insertions(+), 100 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index e92058e8..7ae93152 100644 --- a/acme/api/revoke.go +++ b/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 } diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index 033934c6..2feae989 100644 --- a/acme/api/revoke_test.go +++ b/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 ( - 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-----` -) - +// v is a utility function to return the pointer to an integer 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) +// generateCertKeyPair generates fresh x509 certificate/key pairs for testing +func generateCertKeyPair() (*x509.Certificate, crypto.Signer, error) { + + pub, priv, err := keyutil.GenerateKeyPair("EC", "P-256", 0) 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) { - t.Errorf("revokeOptions() = %v, want %v", got, tt.want) + if got := revokeOptions(tt.args.serial, tt.args.certToBeRevoked, tt.args.reasonCode); !cmp.Equal(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{ - MockRevoke: func(ctx context.Context, opts *authority.RevokeOptions) error { - return errors.New("force") - }, - } + ca := &mockCA{} 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, - }, + statusCode: 200, } }, - "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, - }, - }, - }, - }, + "ok/using-certificate-key": func(t *testing.T) test { + rp := &revokePayload{ + Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), + ReasonCode: v(1), } - 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) From a7fbbc47483986899bb420942b0c5de515520371 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sun, 28 Nov 2021 21:20:57 +0100 Subject: [PATCH 14/20] Add tests for GetCertificateBySerial --- acme/api/revoke.go | 6 +- acme/api/revoke_test.go | 24 ++--- acme/db/nosql/certificate_test.go | 145 ++++++++++++++++++++++++++++-- authority/provisioner/acme.go | 2 +- 4 files changed, 152 insertions(+), 25 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 7ae93152..1c664dde 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -82,11 +82,11 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, fmt.Sprintf("account '%s' has status '%s'", account.ID, account.Status), nil)) return } - if existingCert.AccountID != account.ID { // TODO: combine with the below; ony one of the two has to be true + if existingCert.AccountID != account.ID { // TODO(hs): combine this check with the one 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 and implement "an account that holds authorizations for all of the identifiers in the certificate." + // TODO(hs): 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 @@ -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? When no Subject is in the certificate? + acmeErr.Detail = fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()) // TODO(hs): what about other SANs? When no Subject is in the certificate? return acmeErr } diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index 2feae989..05952240 100644 --- a/acme/api/revoke_test.go +++ b/acme/api/revoke_test.go @@ -616,10 +616,10 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "fail/wrong-certificate-encoding": func(t *testing.T) test { - rp := &revokePayload{ + wrongPayload := &revokePayload{ Certificate: base64.StdEncoding.EncodeToString(cert.Raw), } - wronglyEncodedPayloadBytes, err := json.Marshal(rp) + wronglyEncodedPayloadBytes, err := json.Marshal(wrongPayload) assert.FatalError(t, err) jws := &jose.JSONWebSignature{ Signatures: []jose.Signature{ @@ -648,10 +648,10 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "fail/no-certificate-encoded": func(t *testing.T) test { - rp := &revokePayload{ + emptyPayload := &revokePayload{ Certificate: base64.RawURLEncoding.EncodeToString([]byte{}), } - wrongPayloadBytes, err := json.Marshal(rp) + wrongPayloadBytes, err := json.Marshal(emptyPayload) assert.FatalError(t, err) jws := &jose.JSONWebSignature{ Signatures: []jose.Signature{ @@ -856,15 +856,15 @@ func TestHandler_RevokeCert(t *testing.T) { "fail/unauthorized-certificate-key": func(t *testing.T) test { _, unauthorizedKey, err := generateCertKeyPair() assert.FatalError(t, err) - rp := &revokePayload{ + jwsPayload := &revokePayload{ Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), - ReasonCode: v(1), + ReasonCode: v(2), } 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) + unauthorizedPayloadBytes, err := json.Marshal(jwsPayload) assert.FatalError(t, err) ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: unauthorizedPayloadBytes}) @@ -981,11 +981,11 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "fail/invalid-reasoncode": func(t *testing.T) test { - rp := &revokePayload{ + invalidReasonPayload := &revokePayload{ Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), ReasonCode: v(7), } - wrongReasonCodePayloadBytes, err := json.Marshal(rp) + wrongReasonCodePayloadBytes, err := json.Marshal(invalidReasonPayload) assert.FatalError(t, err) jws := &jose.JSONWebSignature{ Signatures: []jose.Signature{ @@ -1205,16 +1205,10 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "ok/using-certificate-key": func(t *testing.T) test { - rp := &revokePayload{ - Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), - ReasonCode: v(1), - } 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, payloadContextKey, &payloadInfo{value: payloadBytes}) ctx = context.WithValue(ctx, jwsContextKey, jws) diff --git a/acme/db/nosql/certificate_test.go b/acme/db/nosql/certificate_test.go index 8b6b6ef3..d64b3015 100644 --- a/acme/db/nosql/certificate_test.go +++ b/acme/db/nosql/certificate_test.go @@ -1,10 +1,12 @@ package nosql import ( + "bytes" "context" "crypto/x509" "encoding/json" "encoding/pem" + "fmt" "testing" "time" @@ -14,7 +16,6 @@ import ( "github.com/smallstep/certificates/db" "github.com/smallstep/nosql" nosqldb "github.com/smallstep/nosql/database" - "go.step.sm/crypto/pemutil" ) @@ -31,7 +32,6 @@ func TestDB_CreateCertificate(t *testing.T) { err error _id *string } - countOfCmpAndSwapCalls := 0 var tests = map[string]func(t *testing.T) test{ "fail/cmpAndSwap-error": func(t *testing.T) test { cert := &acme.Certificate{ @@ -76,7 +76,10 @@ func TestDB_CreateCertificate(t *testing.T) { return test{ db: &db.MockNoSQLDB{ MCmpAndSwap: func(bucket, key, old, nu []byte) ([]byte, bool, error) { - if countOfCmpAndSwapCalls == 0 { + if !bytes.Equal(bucket, certTable) && !bytes.Equal(bucket, certBySerialTable) { + t.Fail() + } + if bytes.Equal(bucket, certTable) { *idPtr = string(key) assert.Equals(t, bucket, certTable) assert.Equals(t, key, []byte(cert.ID)) @@ -90,7 +93,7 @@ func TestDB_CreateCertificate(t *testing.T) { assert.True(t, clock.Now().Add(-time.Minute).Before(dbc.CreatedAt)) assert.True(t, clock.Now().Add(time.Minute).After(dbc.CreatedAt)) } - if countOfCmpAndSwapCalls == 1 { + if bytes.Equal(bucket, certBySerialTable) { assert.Equals(t, bucket, certBySerialTable) assert.Equals(t, key, []byte(cert.Leaf.SerialNumber.String())) assert.Equals(t, old, nil) @@ -103,8 +106,6 @@ func TestDB_CreateCertificate(t *testing.T) { *idPtr = cert.ID } - countOfCmpAndSwapCalls++ - return nil, true, nil }, }, @@ -335,3 +336,135 @@ func Test_parseBundle(t *testing.T) { }) } } + +func TestDB_GetCertificateBySerial(t *testing.T) { + leaf, err := pemutil.ReadCertificate("../../../authority/testdata/certs/foo.crt") + assert.FatalError(t, err) + inter, err := pemutil.ReadCertificate("../../../authority/testdata/certs/intermediate_ca.crt") + assert.FatalError(t, err) + root, err := pemutil.ReadCertificate("../../../authority/testdata/certs/root_ca.crt") + assert.FatalError(t, err) + + certID := "certID" + serial := "" + type test struct { + db nosql.DB + err error + acmeErr *acme.Error + } + var tests = map[string]func(t *testing.T) test{ + "fail/not-found": func(t *testing.T) test { + return test{ + db: &db.MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + if bytes.Equal(bucket, certBySerialTable) { + return nil, nosqldb.ErrNotFound + } + return nil, errors.New("wrong table") + }, + }, + acmeErr: acme.NewError(acme.ErrorMalformedType, "certificate with serial %s not found", serial), + } + }, + "fail/db-error": func(t *testing.T) test { + return test{ + db: &db.MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + if bytes.Equal(bucket, certBySerialTable) { + return nil, errors.New("force") + } + return nil, errors.New("wrong table") + }, + }, + err: fmt.Errorf("error loading certificate ID for serial %s", serial), + } + }, + "fail/unmarshal-dbSerial": func(t *testing.T) test { + return test{ + db: &db.MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + if bytes.Equal(bucket, certBySerialTable) { + return []byte(`{"serial":malformed!}`), nil + } + return nil, errors.New("wrong table") + }, + }, + err: fmt.Errorf("error unmarshaling certificate with serial %s", serial), + } + }, + "ok": func(t *testing.T) test { + return test{ + db: &db.MockNoSQLDB{ + MGet: func(bucket, key []byte) ([]byte, error) { + + if bytes.Equal(bucket, certBySerialTable) { + certSerial := dbSerial{ + Serial: serial, + CertificateID: certID, + } + + b, err := json.Marshal(certSerial) + assert.FatalError(t, err) + + return b, nil + } + + if bytes.Equal(bucket, certTable) { + cert := dbCert{ + ID: certID, + AccountID: "accountID", + OrderID: "orderID", + Leaf: pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: leaf.Raw, + }), + Intermediates: append(pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: inter.Raw, + }), pem.EncodeToMemory(&pem.Block{ + Type: "CERTIFICATE", + Bytes: root.Raw, + })...), + CreatedAt: clock.Now(), + } + b, err := json.Marshal(cert) + assert.FatalError(t, err) + + return b, nil + } + return nil, errors.New("wrong table") + }, + }, + } + }, + } + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + d := DB{db: tc.db} + cert, err := d.GetCertificateBySerial(context.Background(), serial) + if err != nil { + switch k := err.(type) { + case *acme.Error: + if assert.NotNil(t, tc.acmeErr) { + assert.Equals(t, k.Type, tc.acmeErr.Type) + assert.Equals(t, k.Detail, tc.acmeErr.Detail) + assert.Equals(t, k.Status, tc.acmeErr.Status) + assert.Equals(t, k.Err.Error(), tc.acmeErr.Err.Error()) + assert.Equals(t, k.Detail, tc.acmeErr.Detail) + } + default: + if assert.NotNil(t, tc.err) { + assert.HasPrefix(t, err.Error(), tc.err.Error()) + } + } + } else if assert.Nil(t, tc.err) { + assert.Equals(t, cert.ID, certID) + assert.Equals(t, cert.AccountID, "accountID") + assert.Equals(t, cert.OrderID, "orderID") + assert.Equals(t, cert.Leaf, leaf) + assert.Equals(t, cert.Intermediates, []*x509.Certificate{inter, root}) + } + }) + } +} diff --git a/authority/provisioner/acme.go b/authority/provisioner/acme.go index 25821051..c8950568 100644 --- a/authority/provisioner/acme.go +++ b/authority/provisioner/acme.go @@ -102,7 +102,7 @@ func (p *ACME) AuthorizeSign(ctx context.Context, token string) ([]SignOption, e // 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? +// TODO(hs): 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 From bae1d256eeea659cfea4a016730312f8c025ac5a Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 2 Dec 2021 10:59:56 +0100 Subject: [PATCH 15/20] Improve tests for JWK vs. KID revoke auth flow The logic for both test cases is fairly similar, but with some small differences. Made those clearer by means of some comments. Also added some comments to the middleware logic that decided whether to extract JWK or lookup by KID. --- acme/api/middleware.go | 11 ++++++----- acme/api/middleware_test.go | 10 +++------- 2 files changed, 9 insertions(+), 12 deletions(-) diff --git a/acme/api/middleware.go b/acme/api/middleware.go index e0bbcdda..d701f240 100644 --- a/acme/api/middleware.go +++ b/acme/api/middleware.go @@ -262,11 +262,11 @@ func (h *Handler) extractJWK(next nextHTTP) nextHTTP { // Store the JWK in the context. ctx = context.WithValue(ctx, jwkContextKey, jwk) - // Get Account or continue to generate a new one. + // Get Account OR continue to generate a new one OR continue Revoke with certificate private key acc, err := h.db.GetAccountByKeyID(ctx, jwk.KeyID) switch { case errors.Is(err, acme.ErrNotFound): - // For NewAccount requests ... + // For NewAccount and Revoke requests ... break case err != nil: api.WriteError(w, err) @@ -364,14 +364,15 @@ 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. This flow is used when the ACME client + // signed the payload with a certificate private key. if canExtractJWKFrom(jws) { h.extractJWK(next)(w, r) return } - // default to looking up the JWK based on KeyID - // NOTE: this is a JWK signed with the certificate private key + // default to looking up the JWK based on KeyID. This flow is used when the ACME client + // signed the payload with an account private key. h.lookupJWK(next)(w, r) } } diff --git a/acme/api/middleware_test.go b/acme/api/middleware_test.go index 7b8ae828..050b46a5 100644 --- a/acme/api/middleware_test.go +++ b/acme/api/middleware_test.go @@ -1557,7 +1557,7 @@ func TestHandler_extractOrLookupJWK(t *testing.T) { pub := jwk.Public() pub.KeyID = base64.RawURLEncoding.EncodeToString(kid) so := new(jose.SignerOptions) - so.WithHeader("jwk", pub) + so.WithHeader("jwk", pub) // JWK for certificate private key flow signer, err := jose.NewSigner(jose.SigningKey{ Algorithm: jose.SignatureAlgorithm(jwk.Algorithm), Key: jwk.Key, @@ -1569,14 +1569,12 @@ func TestHandler_extractOrLookupJWK(t *testing.T) { assert.FatalError(t, err) parsedJWS, err := jose.ParseJWS(raw) assert.FatalError(t, err) - assert.FatalError(t, err) - acc := &acme.Account{Status: "valid"} return test{ linker: NewLinker("dns", "acme"), db: &acme.MockDB{ MockGetAccountByKeyID: func(ctx context.Context, kid string) (*acme.Account, error) { assert.Equals(t, kid, pub.KeyID) - return acc, nil + return nil, acme.ErrNotFound }, }, ctx: context.WithValue(context.Background(), jwsContextKey, parsedJWS), @@ -1595,7 +1593,7 @@ func TestHandler_extractOrLookupJWK(t *testing.T) { accID := "accID" prefix := fmt.Sprintf("%s/acme/%s/account/", baseURL, provName) so := new(jose.SignerOptions) - so.WithHeader("kid", fmt.Sprintf("%s%s", prefix, accID)) + so.WithHeader("kid", fmt.Sprintf("%s%s", prefix, accID)) // KID for account private key flow signer, err := jose.NewSigner(jose.SigningKey{ Algorithm: jose.SignatureAlgorithm(jwk.Algorithm), Key: jwk.Key, @@ -1607,8 +1605,6 @@ func TestHandler_extractOrLookupJWK(t *testing.T) { assert.FatalError(t, err) parsedJWS, err := jose.ParseJWS(raw) assert.FatalError(t, err) - assert.FatalError(t, err) - //acc := &acme.Account{Status: "valid", Key: jwk} acc := &acme.Account{ID: "accID", Key: jwk, Status: "valid"} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, baseURLContextKey, baseURL) From 06bb97c91e3d94c615b2b07010d765a91c990a95 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 2 Dec 2021 16:25:35 +0100 Subject: [PATCH 16/20] Add logic for Account authorizations and improve tests --- acme/api/revoke.go | 139 ++++++- acme/api/revoke_test.go | 777 ++++++++++++++++++++++++------------ acme/db.go | 18 +- acme/db/nosql/authz.go | 34 ++ acme/db/nosql/authz_test.go | 153 +++++++ acme/order.go | 3 + go.mod | 1 + 7 files changed, 845 insertions(+), 280 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 1c664dde..209bc204 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -1,6 +1,8 @@ package api import ( + "bytes" + "context" "crypto/x509" "encoding/base64" "encoding/json" @@ -66,35 +68,36 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { } serial := certToBeRevoked.SerialNumber.String() - existingCert, err := h.db.GetCertificateBySerial(ctx, serial) + dbCert, err := h.db.GetCertificateBySerial(ctx, serial) if err != nil { api.WriteError(w, acme.WrapErrorISE(err, "error retrieving certificate by serial")) return } + if !bytes.Equal(dbCert.Leaf.Raw, certToBeRevoked.Raw) { + // this should never happen + api.WriteError(w, acme.NewErrorISE("certificate raw bytes are not equal")) + return + } + if shouldCheckAccountFrom(jws) { account, err := accountFromContext(ctx) if err != nil { api.WriteError(w, err) return } - if !account.IsValid() { - api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, fmt.Sprintf("account '%s' has status '%s'", account.ID, account.Status), nil)) + acmeErr := h.isAccountAuthorized(ctx, dbCert, certToBeRevoked, account) + if acmeErr != nil { + api.WriteError(w, acmeErr) return } - if existingCert.AccountID != account.ID { // TODO(hs): combine this check with the one 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(hs): 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. _, 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)) + api.WriteError(w, wrapUnauthorizedError(certToBeRevoked, nil, "verification of jws using certificate public key failed", err)) return } } @@ -137,6 +140,107 @@ func (h *Handler) RevokeCert(w http.ResponseWriter, r *http.Request) { w.Write(nil) } +// isAccountAuthorized checks if an ACME account that was retrieved earlier is authorized +// to revoke the certificate. An Account must always be valid in order to revoke a certificate. +// In case the certificate retrieved from the database belongs to the Account, the Account is +// authorized. If the certificate retrieved from the database doesn't belong to the Account, +// the identifiers in the certificate are extracted and compared against the (valid) Authorizations +// that are stored for the ACME Account. If these sets match, the Account is considered authorized +// to revoke the certificate. If this check fails, the client will receive an unauthorized error. +func (h *Handler) isAccountAuthorized(ctx context.Context, dbCert *acme.Certificate, certToBeRevoked *x509.Certificate, account *acme.Account) *acme.Error { + if !account.IsValid() { + return wrapUnauthorizedError(certToBeRevoked, nil, fmt.Sprintf("account '%s' has status '%s'", account.ID, account.Status), nil) + } + certificateBelongsToAccount := dbCert.AccountID == account.ID + if certificateBelongsToAccount { + return nil // return early; skip relatively expensive database check + } + requiredIdentifiers := extractIdentifiers(certToBeRevoked) + if len(requiredIdentifiers) == 0 { + return wrapUnauthorizedError(certToBeRevoked, nil, "cannot authorize revocation without providing identifiers to authorize", nil) + } + authzs, err := h.db.GetAuthorizationsByAccountID(ctx, account.ID) + if err != nil { + return acme.WrapErrorISE(err, "error retrieving authorizations for Account %s", account.ID) + } + authorizedIdentifiers := map[string]acme.Identifier{} + for _, authz := range authzs { + // Only valid Authorizations are included + if authz.Status != acme.StatusValid { + continue + } + authorizedIdentifiers[identifierKey(authz.Identifier)] = authz.Identifier + } + if len(authorizedIdentifiers) == 0 { + unauthorizedIdentifiers := []acme.Identifier{} + for _, identifier := range requiredIdentifiers { + unauthorizedIdentifiers = append(unauthorizedIdentifiers, identifier) + } + return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have valid authorizations", account.ID), nil) + } + unauthorizedIdentifiers := []acme.Identifier{} + for key := range requiredIdentifiers { + _, ok := authorizedIdentifiers[key] + if !ok { + unauthorizedIdentifiers = append(unauthorizedIdentifiers, requiredIdentifiers[key]) + } + } + if len(unauthorizedIdentifiers) != 0 { + return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have authorizations for all identifiers", account.ID), nil) + } + + return nil +} + +// identifierKey creates a unique key for an ACME identifier using +// the following format: ip|127.0.0.1; dns|*.example.com +func identifierKey(identifier acme.Identifier) string { + if identifier.Type == acme.IP { + return "ip|" + identifier.Value + } + if identifier.Type == acme.DNS { + return "dns|" + identifier.Value + } + return "unsupported|" + identifier.Value +} + +// extractIdentifiers extracts ACME identifiers from an x509 certificate and +// creates a map from them. The map ensures that double SANs are deduplicated. +// The Subject CommonName is included, because RFC8555 7.4 states that DNS +// identifiers can come from either the CommonName or a DNS SAN or both. When +// authorizing issuance, the DNS identifier must be in the request and will be +// included in the validation (see Order.sans()) as of now. This means that the +// CommonName will in fact have an authorization available. +func extractIdentifiers(cert *x509.Certificate) map[string]acme.Identifier { + result := map[string]acme.Identifier{} + for _, name := range cert.DNSNames { + identifier := acme.Identifier{ + Type: acme.DNS, + Value: name, + } + result[identifierKey(identifier)] = identifier + } + for _, ip := range cert.IPAddresses { + identifier := acme.Identifier{ + Type: acme.IP, + Value: ip.String(), + } + result[identifierKey(identifier)] = identifier + } + // TODO(hs): should we include the CommonName or not? + if cert.Subject.CommonName != "" { + identifier := acme.Identifier{ + // assuming only DNS can be in Common Name (RFC8555, 7.4); RFC8738 + // IP Identifier Validation Extension does not state anything about this. + // This logic is in accordance with the logic in order.canonicalize() + Type: acme.DNS, + Value: cert.Subject.CommonName, + } + result[identifierKey(identifier)] = identifier + } + return result +} + // 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 { @@ -149,15 +253,24 @@ func wrapRevokeErr(err error) *acme.Error { // 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 { +func wrapUnauthorizedError(cert *x509.Certificate, unauthorizedIdentifiers []acme.Identifier, 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(hs): what about other SANs? When no Subject is in the certificate? + acmeErr.Status = http.StatusForbidden // RFC8555 7.6 shows example with 403 + + switch { + case len(unauthorizedIdentifiers) > 0: + identifier := unauthorizedIdentifiers[0] // picking the first; compound may be an option too? + acmeErr.Detail = fmt.Sprintf("No authorization provided for name %s", identifier.Value) + case cert.Subject.String() != "": + acmeErr.Detail = fmt.Sprintf("No authorization provided for name %s", cert.Subject.CommonName) + default: + acmeErr.Detail = "No authorization provided" + } return acmeErr } diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index 05952240..cf036abb 100644 --- a/acme/api/revoke_test.go +++ b/acme/api/revoke_test.go @@ -14,6 +14,8 @@ import ( "fmt" "io" "math/big" + "net" + "net/http" "net/http/httptest" "net/url" "testing" @@ -37,6 +39,10 @@ func v(v int) *int { return &v } +func generateSerial() (*big.Int, error) { + return rand.Int(rand.Reader, big.NewInt(1000000000000000000)) +} + // generateCertKeyPair generates fresh x509 certificate/key pairs for testing func generateCertKeyPair() (*x509.Certificate, crypto.Signer, error) { @@ -45,15 +51,16 @@ func generateCertKeyPair() (*x509.Certificate, crypto.Signer, error) { return nil, nil, err } - serial, err := rand.Int(rand.Reader, big.NewInt(1000000000000000000)) + serial, err := generateSerial() if err != nil { return nil, nil, err } now := time.Now() template := &x509.Certificate{ - Subject: pkix.Name{CommonName: "Test ACME Revoke Certificate"}, + Subject: pkix.Name{CommonName: "127.0.0.1"}, Issuer: pkix.Name{CommonName: "Test ACME Revoke Certificate"}, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, IsCA: false, MaxPathLen: 0, KeyUsage: x509.KeyUsageCertSign | x509.KeyUsageCRLSign, @@ -453,7 +460,7 @@ 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); !cmp.Equal(got, tt.want) { - t.Errorf("revokeOptions() diff = %s", cmp.Diff(got, tt.want)) + t.Errorf("revokeOptions() diff =\n%s", cmp.Diff(got, tt.want)) } }) } @@ -478,6 +485,20 @@ func TestHandler_RevokeCert(t *testing.T) { payloadBytes, 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, + }, + }, + }, + }, + } + type test struct { db acme.DB ca acme.CertificateAuthority @@ -504,19 +525,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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, @@ -525,19 +533,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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{ @@ -547,19 +542,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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{ @@ -569,19 +551,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -592,19 +561,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -621,19 +577,6 @@ func TestHandler_RevokeCert(t *testing.T) { } wronglyEncodedPayloadBytes, err := json.Marshal(wrongPayload) 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, - }, - }, - }, - }, - } ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: wronglyEncodedPayloadBytes}) ctx = context.WithValue(ctx, jwsContextKey, jws) @@ -651,23 +594,10 @@ func TestHandler_RevokeCert(t *testing.T) { emptyPayload := &revokePayload{ Certificate: base64.RawURLEncoding.EncodeToString([]byte{}), } - wrongPayloadBytes, err := json.Marshal(emptyPayload) + emptyPayloadBytes, err := json.Marshal(emptyPayload) 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, - }, - }, - }, - }, - } ctx := context.WithValue(context.Background(), provisionerContextKey, prov) - ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: wrongPayloadBytes}) + ctx = context.WithValue(ctx, payloadContextKey, &payloadInfo{value: emptyPayloadBytes}) ctx = context.WithValue(ctx, jwsContextKey, jws) return test{ ctx: ctx, @@ -680,19 +610,6 @@ 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) @@ -708,27 +625,37 @@ func TestHandler_RevokeCert(t *testing.T) { 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, - }, - }, - }, - }, - } + "fail/different-certificate-contents": func(t *testing.T) test { + aDifferentCert, _, err := generateCertKeyPair() + assert.FatalError(t, err) 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 &acme.Certificate{ + Leaf: aDifferentCert, + }, nil + }, + } + return test{ + db: db, + ctx: ctx, + statusCode: 500, + err: acme.NewErrorISE("certificate raw bytes are not equal"), + } + }, + "fail/no-account": func(t *testing.T) test { + 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{ + Leaf: cert, + }, nil }, } return test{ @@ -739,19 +666,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -759,7 +673,9 @@ func TestHandler_RevokeCert(t *testing.T) { 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 &acme.Certificate{ + Leaf: cert, + }, nil }, } return test{ @@ -770,19 +686,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -795,6 +698,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -806,25 +710,12 @@ func TestHandler_RevokeCert(t *testing.T) { statusCode: 403, err: &acme.Error{ Type: "urn:ietf:params:acme:error:unauthorized", - Detail: fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()), + Detail: "No authorization provided for name 127.0.0.1", 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, - }, - }, - }, - }, - } + "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { acc := &acme.Account{ID: "accountID", Status: acme.StatusValid} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) ctx = context.WithValue(ctx, accContextKey, acc) @@ -837,6 +728,49 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "differentAccountID", + Leaf: cert, + }, nil + }, + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + return nil, errors.New("force") + }, + } + ca := &mockCA{} + return test{ + db: db, + ca: ca, + ctx: ctx, + statusCode: 500, + err: acme.NewErrorISE("error retrieving authorizations for Account %s", "accountID"), + } + }, + "fail/account-not-authorized": func(t *testing.T) test { + 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", + Leaf: cert, + }, nil + }, + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + assert.Equals(t, "accountID", accountID) + return []*acme.Authorization{ + { + AccountID: "accountID", + Status: acme.StatusValid, + Identifier: acme.Identifier{ + Type: acme.IP, + Value: "127.0.1.0", + }, + }, }, nil }, } @@ -848,7 +782,7 @@ func TestHandler_RevokeCert(t *testing.T) { statusCode: 403, err: &acme.Error{ Type: "urn:ietf:params:acme:error:unauthorized", - Detail: fmt.Sprintf("No authorization provided for name %s", cert.Subject.String()), + Detail: "No authorization provided for name 127.0.0.1", Status: 403, }, } @@ -862,13 +796,13 @@ func TestHandler_RevokeCert(t *testing.T) { } jwsBytes, err := jwsEncodeJSON(rp, unauthorizedKey, "", "nonce", revokeURL) assert.FatalError(t, err) - jws, err := jose.ParseJWS(string(jwsBytes)) + parsedJWS, err := jose.ParseJWS(string(jwsBytes)) assert.FatalError(t, err) unauthorizedPayloadBytes, err := json.Marshal(jwsPayload) 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, jwsContextKey, parsedJWS) ctx = context.WithValue(ctx, baseURLContextKey, baseURL) ctx = context.WithValue(ctx, chi.RouteCtxKey, chiCtx) db := &acme.MockDB{ @@ -876,12 +810,13 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, 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" + acmeErr.Detail = "No authorization provided for name 127.0.0.1" return test{ db: db, ca: ca, @@ -891,19 +826,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -916,6 +838,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -937,19 +860,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -960,6 +870,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -985,31 +896,19 @@ func TestHandler_RevokeCert(t *testing.T) { Certificate: base64.RawURLEncoding.EncodeToString(cert.Raw), ReasonCode: v(7), } - wrongReasonCodePayloadBytes, err := json.Marshal(invalidReasonPayload) + invalidReasonCodePayloadBytes, err := json.Marshal(invalidReasonPayload) 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, payloadContextKey, &payloadInfo{value: invalidReasonCodePayloadBytes}) 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", + Leaf: cert, }, nil }, } @@ -1032,19 +931,6 @@ func TestHandler_RevokeCert(t *testing.T) { }, "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") @@ -1060,6 +946,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -1081,19 +968,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -1104,6 +978,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -1125,19 +1000,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -1148,6 +1010,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -1168,19 +1031,6 @@ func TestHandler_RevokeCert(t *testing.T) { } }, "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) @@ -1193,6 +1043,7 @@ func TestHandler_RevokeCert(t *testing.T) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ AccountID: "accountID", + Leaf: cert, }, nil }, } @@ -1218,7 +1069,8 @@ func TestHandler_RevokeCert(t *testing.T) { MockGetCertificateBySerial: func(ctx context.Context, serial string) (*acme.Certificate, error) { assert.Equals(t, cert.SerialNumber.String(), serial) return &acme.Certificate{ - AccountID: "accountID", + AccountID: "someDifferentAccountID", + Leaf: cert, }, nil }, } @@ -1264,3 +1116,400 @@ func TestHandler_RevokeCert(t *testing.T) { }) } } + +func TestHandler_isAccountAuthorized(t *testing.T) { + type test struct { + db acme.DB + ctx context.Context + existingCert *acme.Certificate + certToBeRevoked *x509.Certificate + account *acme.Account + err *acme.Error + } + accountID := "accountID" + var tests = map[string]func(t *testing.T) test{ + "fail/account-invalid": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusInvalid, + } + certToBeRevoked := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + } + return test{ + ctx: context.TODO(), + certToBeRevoked: certToBeRevoked, + account: account, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Status: http.StatusForbidden, + Detail: "No authorization provided for name 127.0.0.1", + Err: errors.New("account 'accountID' has status 'invalid'"), + }, + } + }, + "fail/no-certificate-identifiers": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusValid, + } + certToBeRevoked := &x509.Certificate{} + existingCert := &acme.Certificate{ + AccountID: "differentAccountID", + } + return test{ + ctx: context.TODO(), + existingCert: existingCert, + certToBeRevoked: certToBeRevoked, + account: account, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Status: http.StatusForbidden, + Detail: "No authorization provided", + Err: errors.New("cannot authorize revocation without providing identifiers to authorize"), + }, + } + }, + "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusValid, + } + certToBeRevoked := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + } + existingCert := &acme.Certificate{ + AccountID: "differentAccountID", + } + return test{ + db: &acme.MockDB{ + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + assert.Equals(t, "accountID", accountID) + return nil, errors.New("force") + }, + }, + ctx: context.TODO(), + existingCert: existingCert, + certToBeRevoked: certToBeRevoked, + account: account, + err: acme.NewErrorISE("error retrieving authorizations for Account %s: force", accountID), + } + }, + "fail/no-valid-authorizations": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusValid, + } + certToBeRevoked := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + } + existingCert := &acme.Certificate{ + AccountID: "differentAccountID", + } + return test{ + db: &acme.MockDB{ + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + assert.Equals(t, "accountID", accountID) + return []*acme.Authorization{ + { + AccountID: accountID, + Status: acme.StatusInvalid, + }, + }, nil + }, + }, + ctx: context.TODO(), + existingCert: existingCert, + certToBeRevoked: certToBeRevoked, + account: account, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Status: http.StatusForbidden, + Detail: "No authorization provided for name 127.0.0.1", + Err: errors.New("account 'accountID' does not have valid authorizations"), + }, + } + }, + "fail/authorizations-do-not-match-identifiers": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusValid, + } + certToBeRevoked := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + } + existingCert := &acme.Certificate{ + AccountID: "differentAccountID", + } + return test{ + db: &acme.MockDB{ + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + assert.Equals(t, "accountID", accountID) + return []*acme.Authorization{ + { + AccountID: accountID, + Status: acme.StatusValid, + Identifier: acme.Identifier{ + Type: acme.IP, + Value: "127.0.0.2", + }, + }, + }, nil + }, + }, + ctx: context.TODO(), + existingCert: existingCert, + certToBeRevoked: certToBeRevoked, + account: account, + err: &acme.Error{ + Type: "urn:ietf:params:acme:error:unauthorized", + Status: http.StatusForbidden, + Detail: "No authorization provided for name 127.0.0.1", + Err: errors.New("account 'accountID' does not have authorizations for all identifiers"), + }, + } + }, + "ok": func(t *testing.T) test { + account := &acme.Account{ + ID: accountID, + Status: acme.StatusValid, + } + certToBeRevoked := &x509.Certificate{ + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + } + existingCert := &acme.Certificate{ + AccountID: "differentAccountID", + } + return test{ + db: &acme.MockDB{ + MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + assert.Equals(t, "accountID", accountID) + return []*acme.Authorization{ + { + AccountID: accountID, + Status: acme.StatusValid, + Identifier: acme.Identifier{ + Type: acme.IP, + Value: "127.0.0.1", + }, + }, + }, nil + }, + }, + ctx: context.TODO(), + existingCert: existingCert, + certToBeRevoked: certToBeRevoked, + account: account, + err: nil, + } + }, + } + for name, setup := range tests { + tc := setup(t) + t.Run(name, func(t *testing.T) { + h := &Handler{db: tc.db} + acmeErr := h.isAccountAuthorized(tc.ctx, tc.existingCert, tc.certToBeRevoked, tc.account) + + expectError := tc.err != nil + gotError := acmeErr != nil + if expectError != gotError { + t.Errorf("expected: %t, got: %t", expectError, gotError) + return + } + + if !gotError { + return // nothing to check; return early + } + + assert.Equals(t, acmeErr.Err.Error(), tc.err.Err.Error()) + assert.Equals(t, acmeErr.Type, tc.err.Type) + assert.Equals(t, acmeErr.Status, tc.err.Status) + assert.Equals(t, acmeErr.Detail, tc.err.Detail) + assert.Equals(t, acmeErr.Identifier, tc.err.Identifier) + assert.Equals(t, acmeErr.Subproblems, tc.err.Subproblems) + + }) + } +} + +func Test_identifierKey(t *testing.T) { + tests := []struct { + name string + identifier acme.Identifier + want string + }{ + { + name: "ip", + identifier: acme.Identifier{ + Type: acme.IP, + Value: "10.0.0.1", + }, + want: "ip|10.0.0.1", + }, + { + name: "dns", + identifier: acme.Identifier{ + Type: acme.DNS, + Value: "*.example.com", + }, + want: "dns|*.example.com", + }, + { + name: "unknown", + identifier: acme.Identifier{ + Type: "InvalidType", + Value: "<<>>", + }, + want: "unsupported|<<>>", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := identifierKey(tt.identifier) + if !cmp.Equal(tt.want, got) { + t.Errorf("identifierKey() diff = \n%s", cmp.Diff(tt.want, got)) + } + }) + } +} + +func Test_extractIdentifiers(t *testing.T) { + tests := []struct { + name string + cert *x509.Certificate + want map[string]acme.Identifier + }{ + { + name: "ip", + cert: &x509.Certificate{ + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, + }, + want: map[string]acme.Identifier{ + "ip|127.0.0.1": { + Type: acme.IP, + Value: "127.0.0.1", + }, + }, + }, + { + name: "dns", + cert: &x509.Certificate{ + DNSNames: []string{"*.example.com"}, + }, + want: map[string]acme.Identifier{ + "dns|*.example.com": { + Type: acme.DNS, + Value: "*.example.com", + }, + }, + }, + { + name: "dns-subject", + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "www.example.com", + }, + }, + want: map[string]acme.Identifier{ + "dns|www.example.com": { + Type: acme.DNS, + Value: "www.example.com", + }, + }, + }, + { + name: "ip-subject", + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + }, + want: map[string]acme.Identifier{ + "dns|127.0.0.1": { // this is the currently expected behavior + Type: acme.DNS, + Value: "127.0.0.1", + }, + }, + }, + { + name: "combined", + cert: &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "127.0.0.1", + }, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, + DNSNames: []string{"*.example.com", "www.example.com"}, + }, + want: map[string]acme.Identifier{ + "ip|127.0.0.1": { + Type: acme.IP, + Value: "127.0.0.1", + }, + "ip|127.0.0.2": { + Type: acme.IP, + Value: "127.0.0.2", + }, + "dns|*.example.com": { + Type: acme.DNS, + Value: "*.example.com", + }, + "dns|www.example.com": { + Type: acme.DNS, + Value: "www.example.com", + }, + "dns|127.0.0.1": { // this is the currently expected behavior + Type: acme.DNS, + Value: "127.0.0.1", + }, + }, + }, + { + name: "ip-duplicates", + cert: &x509.Certificate{ + IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, + }, + want: map[string]acme.Identifier{ + "ip|127.0.0.1": { + Type: acme.IP, + Value: "127.0.0.1", + }, + "ip|127.0.0.2": { + Type: acme.IP, + Value: "127.0.0.2", + }, + }, + }, + { + name: "dns-duplicates", + cert: &x509.Certificate{ + DNSNames: []string{"*.example.com", "www.example.com", "www.example.com"}, + }, + want: map[string]acme.Identifier{ + "dns|*.example.com": { + Type: acme.DNS, + Value: "*.example.com", + }, + "dns|www.example.com": { + Type: acme.DNS, + Value: "www.example.com", + }, + }, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := extractIdentifiers(tt.cert) + if !cmp.Equal(tt.want, got) { + t.Errorf("extractIdentifiers() diff=\n%s", cmp.Diff(tt.want, got)) + } + }) + } +} diff --git a/acme/db.go b/acme/db.go index 67053269..1675c7e7 100644 --- a/acme/db.go +++ b/acme/db.go @@ -25,6 +25,7 @@ type DB interface { CreateAuthorization(ctx context.Context, az *Authorization) error GetAuthorization(ctx context.Context, id string) (*Authorization, error) UpdateAuthorization(ctx context.Context, az *Authorization) error + GetAuthorizationsByAccountID(ctx context.Context, accountID string) ([]*Authorization, error) CreateCertificate(ctx context.Context, cert *Certificate) error GetCertificate(ctx context.Context, id string) (*Certificate, error) @@ -51,9 +52,10 @@ type MockDB struct { MockCreateNonce func(ctx context.Context) (Nonce, error) MockDeleteNonce func(ctx context.Context, nonce Nonce) error - MockCreateAuthorization func(ctx context.Context, az *Authorization) error - MockGetAuthorization func(ctx context.Context, id string) (*Authorization, error) - MockUpdateAuthorization func(ctx context.Context, az *Authorization) error + MockCreateAuthorization func(ctx context.Context, az *Authorization) error + MockGetAuthorization func(ctx context.Context, id string) (*Authorization, error) + MockUpdateAuthorization func(ctx context.Context, az *Authorization) error + MockGetAuthorizationsByAccountID func(ctx context.Context, accountID string) ([]*Authorization, error) MockCreateCertificate func(ctx context.Context, cert *Certificate) error MockGetCertificate func(ctx context.Context, id string) (*Certificate, error) @@ -162,6 +164,16 @@ func (m *MockDB) UpdateAuthorization(ctx context.Context, az *Authorization) err return m.MockError } +// GetAuthorizationsByAccountID mock +func (m *MockDB) GetAuthorizationsByAccountID(ctx context.Context, accountID string) ([]*Authorization, error) { + if m.MockGetAuthorizationsByAccountID != nil { + return m.MockGetAuthorizationsByAccountID(ctx, accountID) + } else if m.MockError != nil { + return nil, m.MockError + } + return nil, m.MockError +} + // CreateCertificate mock func (m *MockDB) CreateCertificate(ctx context.Context, cert *Certificate) error { if m.MockCreateCertificate != nil { diff --git a/acme/db/nosql/authz.go b/acme/db/nosql/authz.go index 6decbe4f..01cb7fed 100644 --- a/acme/db/nosql/authz.go +++ b/acme/db/nosql/authz.go @@ -116,3 +116,37 @@ func (db *DB) UpdateAuthorization(ctx context.Context, az *acme.Authorization) e nu.Error = az.Error return db.save(ctx, old.ID, nu, old, "authz", authzTable) } + +// GetAuthorizationsByAccountID retrieves and unmarshals ACME authz types from the database. +func (db *DB) GetAuthorizationsByAccountID(ctx context.Context, accountID string) ([]*acme.Authorization, error) { + entries, err := db.db.List(authzTable) + if err != nil { + return nil, errors.Wrapf(err, "error listing authz") + } + authzs := []*acme.Authorization{} + for _, entry := range entries { + dbaz := new(dbAuthz) + if err = json.Unmarshal(entry.Value, dbaz); err != nil { + return nil, errors.Wrapf(err, "error unmarshaling dbAuthz key '%s' into dbAuthz struct", string(entry.Key)) + } + // Filter out all dbAuthzs that don't belong to the accountID. This + // could be made more efficient with additional data structures mapping the + // Account ID to authorizations. Not trivial to do, though. + if dbaz.AccountID != accountID { + continue + } + authzs = append(authzs, &acme.Authorization{ + ID: dbaz.ID, + AccountID: dbaz.AccountID, + Identifier: dbaz.Identifier, + Status: dbaz.Status, + Challenges: nil, // challenges not required for current use case + Wildcard: dbaz.Wildcard, + ExpiresAt: dbaz.ExpiresAt, + Token: dbaz.Token, + Error: dbaz.Error, + }) + } + + return authzs, nil +} diff --git a/acme/db/nosql/authz_test.go b/acme/db/nosql/authz_test.go index 01c255dc..2e5dd3ea 100644 --- a/acme/db/nosql/authz_test.go +++ b/acme/db/nosql/authz_test.go @@ -3,9 +3,11 @@ package nosql import ( "context" "encoding/json" + "fmt" "testing" "time" + "github.com/google/go-cmp/cmp" "github.com/pkg/errors" "github.com/smallstep/assert" "github.com/smallstep/certificates/acme" @@ -614,3 +616,154 @@ func TestDB_UpdateAuthorization(t *testing.T) { }) } } + +func TestDB_GetAuthorizationsByAccountID(t *testing.T) { + azID := "azID" + accountID := "accountID" + type test struct { + db nosql.DB + err error + acmeErr *acme.Error + authzs []*acme.Authorization + } + var tests = map[string]func(t *testing.T) test{ + "fail/db.List-error": func(t *testing.T) test { + return test{ + db: &db.MockNoSQLDB{ + MList: func(bucket []byte) ([]*nosqldb.Entry, error) { + assert.Equals(t, bucket, authzTable) + return nil, errors.New("force") + }, + }, + err: errors.New("error listing authz: force"), + } + }, + "fail/unmarshal": func(t *testing.T) test { + b := []byte(`{malformed}`) + return test{ + db: &db.MockNoSQLDB{ + MList: func(bucket []byte) ([]*nosqldb.Entry, error) { + assert.Equals(t, bucket, authzTable) + return []*nosqldb.Entry{ + { + Bucket: bucket, + Key: []byte(azID), + Value: b, + }, + }, nil + }, + }, + authzs: nil, + err: fmt.Errorf("error unmarshaling dbAuthz key '%s' into dbAuthz struct", azID), + } + }, + "ok": func(t *testing.T) test { + now := clock.Now() + dbaz := &dbAuthz{ + ID: azID, + AccountID: accountID, + Identifier: acme.Identifier{ + Type: "dns", + Value: "test.ca.smallstep.com", + }, + Status: acme.StatusValid, + Token: "token", + CreatedAt: now, + ExpiresAt: now.Add(5 * time.Minute), + ChallengeIDs: []string{"foo", "bar"}, + Wildcard: true, + } + b, err := json.Marshal(dbaz) + assert.FatalError(t, err) + + return test{ + db: &db.MockNoSQLDB{ + MList: func(bucket []byte) ([]*nosqldb.Entry, error) { + assert.Equals(t, bucket, authzTable) + return []*nosqldb.Entry{ + { + Bucket: bucket, + Key: []byte(azID), + Value: b, + }, + }, nil + }, + }, + authzs: []*acme.Authorization{ + { + ID: dbaz.ID, + AccountID: dbaz.AccountID, + Token: dbaz.Token, + Identifier: dbaz.Identifier, + Status: dbaz.Status, + Challenges: nil, + Wildcard: dbaz.Wildcard, + ExpiresAt: dbaz.ExpiresAt, + Error: dbaz.Error, + }, + }, + } + }, + "ok/skip-different-account": func(t *testing.T) test { + now := clock.Now() + dbaz := &dbAuthz{ + ID: azID, + AccountID: "differentAccountID", + Identifier: acme.Identifier{ + Type: "dns", + Value: "test.ca.smallstep.com", + }, + Status: acme.StatusValid, + Token: "token", + CreatedAt: now, + ExpiresAt: now.Add(5 * time.Minute), + ChallengeIDs: []string{"foo", "bar"}, + Wildcard: true, + } + b, err := json.Marshal(dbaz) + assert.FatalError(t, err) + + return test{ + db: &db.MockNoSQLDB{ + MList: func(bucket []byte) ([]*nosqldb.Entry, error) { + assert.Equals(t, bucket, authzTable) + return []*nosqldb.Entry{ + { + Bucket: bucket, + Key: []byte(azID), + Value: b, + }, + }, nil + }, + }, + authzs: []*acme.Authorization{}, + } + }, + } + for name, run := range tests { + tc := run(t) + t.Run(name, func(t *testing.T) { + d := DB{db: tc.db} + if azs, err := d.GetAuthorizationsByAccountID(context.Background(), accountID); err != nil { + switch k := err.(type) { + case *acme.Error: + if assert.NotNil(t, tc.acmeErr) { + assert.Equals(t, k.Type, tc.acmeErr.Type) + assert.Equals(t, k.Detail, tc.acmeErr.Detail) + assert.Equals(t, k.Status, tc.acmeErr.Status) + assert.Equals(t, k.Err.Error(), tc.acmeErr.Err.Error()) + assert.Equals(t, k.Detail, tc.acmeErr.Detail) + } + default: + if assert.NotNil(t, tc.err) { + assert.HasPrefix(t, err.Error(), tc.err.Error()) + } + } + } else if assert.Nil(t, tc.err) { + if !cmp.Equal(azs, tc.authzs) { + t.Errorf("db.GetAuthorizationsByAccountID() diff =\n%s", cmp.Diff(azs, tc.authzs)) + } + } + }) + } +} diff --git a/acme/order.go b/acme/order.go index bd820da1..d4a4c300 100644 --- a/acme/order.go +++ b/acme/order.go @@ -290,6 +290,9 @@ func canonicalize(csr *x509.CertificateRequest) (canonicalized *x509.Certificate // MUST appear either in the commonName portion of the requested subject // name or in an extensionRequest attribute [RFC2985] requesting a // subjectAltName extension, or both. + // TODO(hs): we might want to check if the CommonName is in fact a DNS (and cannot + // be parsed as IP). This is related to https://github.com/smallstep/cli/pull/576 + // (ACME IP SANS) if csr.Subject.CommonName != "" { // nolint:gocritic canonicalized.DNSNames = append(csr.DNSNames, csr.Subject.CommonName) diff --git a/go.mod b/go.mod index 394eb1a4..830cab82 100644 --- a/go.mod +++ b/go.mod @@ -18,6 +18,7 @@ require ( github.com/go-kit/kit v0.10.0 // indirect github.com/go-piv/piv-go v1.7.0 github.com/golang/mock v1.6.0 + github.com/google/go-cmp v0.5.6 github.com/google/uuid v1.3.0 github.com/googleapis/gax-go/v2 v2.0.5 github.com/konsorten/go-windows-terminal-sequences v1.0.2 // indirect From 47a8a3c46344bbf4262e3e08804bd4fe96ab7e8e Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 2 Dec 2021 17:11:36 +0100 Subject: [PATCH 17/20] Add test case for ACME Revoke to Authority --- authority/tls.go | 1 + authority/tls_test.go | 17 +++++++++++++++++ db/db.go | 1 + 3 files changed, 19 insertions(+) diff --git a/authority/tls.go b/authority/tls.go index 9cd938a4..6e213ca6 100644 --- a/authority/tls.go +++ b/authority/tls.go @@ -360,6 +360,7 @@ func (a *Authority) Revoke(ctx context.Context, revokeOpts *RevokeOptions) error ReasonCode: revokeOpts.ReasonCode, Reason: revokeOpts.Reason, MTLS: revokeOpts.MTLS, + ACME: revokeOpts.ACME, RevokedAt: time.Now().UTC(), } diff --git a/authority/tls_test.go b/authority/tls_test.go index 03beb5c1..060b3bff 100644 --- a/authority/tls_test.go +++ b/authority/tls_test.go @@ -1267,6 +1267,23 @@ func TestAuthority_Revoke(t *testing.T) { }, } }, + "ok/ACME": func() test { + _a := testAuthority(t, WithDatabase(&db.MockAuthDB{})) + + crt, err := pemutil.ReadCertificate("./testdata/certs/foo.crt") + assert.FatalError(t, err) + + return test{ + auth: _a, + opts: &RevokeOptions{ + Crt: crt, + Serial: "102012593071130646873265215610956555026", + ReasonCode: reasonCode, + Reason: reason, + ACME: true, + }, + } + }, } for name, f := range tests { tc := f() diff --git a/db/db.go b/db/db.go index 2643e577..6d48723f 100644 --- a/db/db.go +++ b/db/db.go @@ -104,6 +104,7 @@ type RevokedCertificateInfo struct { RevokedAt time.Time TokenID string MTLS bool + ACME bool } // IsRevoked returns whether or not a certificate with the given identifier From 004fc054d570908479915306d98ca07761ccd491 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Fri, 3 Dec 2021 15:06:28 +0100 Subject: [PATCH 18/20] Fix PR comments --- acme/api/handler.go | 8 ++++---- acme/api/revoke.go | 5 ++--- 2 files changed, 6 insertions(+), 7 deletions(-) diff --git a/acme/api/handler.go b/acme/api/handler.go index d6153184..09ca03a3 100644 --- a/acme/api/handler.go +++ b/acme/api/handler.go @@ -101,16 +101,16 @@ func (h *Handler) Route(r api.Router) { r.MethodFunc("HEAD", getPath(DirectoryLinkType, "{provisionerID}"), h.baseURLFromRequest(h.lookupProvisioner(h.GetDirectory))) validatingMiddleware := func(next nextHTTP) nextHTTP { - return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(next)))))) + return h.baseURLFromRequest(h.lookupProvisioner(h.addNonce(h.addDirLink(h.verifyContentType(h.parseJWS(h.validateJWS(next))))))) } extractPayloadByJWK := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.validateJWS(h.extractJWK(h.verifyAndExtractJWSPayload(next)))) + return validatingMiddleware(h.extractJWK(h.verifyAndExtractJWSPayload(next))) } extractPayloadByKid := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.validateJWS(h.lookupJWK(h.verifyAndExtractJWSPayload(next)))) + return validatingMiddleware(h.lookupJWK(h.verifyAndExtractJWSPayload(next))) } extractPayloadByKidOrJWK := func(next nextHTTP) nextHTTP { - return validatingMiddleware(h.validateJWS(h.extractOrLookupJWK(h.verifyAndExtractJWSPayload(next)))) + return validatingMiddleware(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 209bc204..4d31c7be 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -205,7 +205,7 @@ func identifierKey(identifier acme.Identifier) string { } // extractIdentifiers extracts ACME identifiers from an x509 certificate and -// creates a map from them. The map ensures that double SANs are deduplicated. +// creates a map from them. The map ensures that duplicate SANs are deduplicated. // The Subject CommonName is included, because RFC8555 7.4 states that DNS // identifiers can come from either the CommonName or a DNS SAN or both. When // authorizing issuance, the DNS identifier must be in the request and will be @@ -227,7 +227,6 @@ func extractIdentifiers(cert *x509.Certificate) map[string]acme.Identifier { } result[identifierKey(identifier)] = identifier } - // TODO(hs): should we include the CommonName or not? if cert.Subject.CommonName != "" { identifier := acme.Identifier{ // assuming only DNS can be in Common Name (RFC8555, 7.4); RFC8738 @@ -302,7 +301,7 @@ func validateReasonCode(reasonCode *int) *acme.Error { return nil } -// revokeOptions determines the the RevokeOptions for the Authority to use in revocation +// revokeOptions determines 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, From 0524122191cecc55086d1eadedfe4f47158ba219 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Wed, 8 Dec 2021 16:28:14 +0100 Subject: [PATCH 19/20] Remove authorization flow for different Account private keys As discussed in https://github.com/smallstep/certificates/issues/767, we opted for not including this authorization flow to prevent users from getting OOMs. We can add the functionality back when the underlying data store can provide access to a long list of Authorizations more efficiently, for example when a callback is implemented. --- acme/api/revoke.go | 95 ++-------- acme/api/revoke_test.go | 379 ++++++++++------------------------------ 2 files changed, 103 insertions(+), 371 deletions(-) diff --git a/acme/api/revoke.go b/acme/api/revoke.go index 4d31c7be..d01e401c 100644 --- a/acme/api/revoke.go +++ b/acme/api/revoke.go @@ -153,91 +153,22 @@ func (h *Handler) isAccountAuthorized(ctx context.Context, dbCert *acme.Certific } certificateBelongsToAccount := dbCert.AccountID == account.ID if certificateBelongsToAccount { - return nil // return early; skip relatively expensive database check - } - requiredIdentifiers := extractIdentifiers(certToBeRevoked) - if len(requiredIdentifiers) == 0 { - return wrapUnauthorizedError(certToBeRevoked, nil, "cannot authorize revocation without providing identifiers to authorize", nil) - } - authzs, err := h.db.GetAuthorizationsByAccountID(ctx, account.ID) - if err != nil { - return acme.WrapErrorISE(err, "error retrieving authorizations for Account %s", account.ID) - } - authorizedIdentifiers := map[string]acme.Identifier{} - for _, authz := range authzs { - // Only valid Authorizations are included - if authz.Status != acme.StatusValid { - continue - } - authorizedIdentifiers[identifierKey(authz.Identifier)] = authz.Identifier - } - if len(authorizedIdentifiers) == 0 { - unauthorizedIdentifiers := []acme.Identifier{} - for _, identifier := range requiredIdentifiers { - unauthorizedIdentifiers = append(unauthorizedIdentifiers, identifier) - } - return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have valid authorizations", account.ID), nil) - } - unauthorizedIdentifiers := []acme.Identifier{} - for key := range requiredIdentifiers { - _, ok := authorizedIdentifiers[key] - if !ok { - unauthorizedIdentifiers = append(unauthorizedIdentifiers, requiredIdentifiers[key]) - } - } - if len(unauthorizedIdentifiers) != 0 { - return wrapUnauthorizedError(certToBeRevoked, unauthorizedIdentifiers, fmt.Sprintf("account '%s' does not have authorizations for all identifiers", account.ID), nil) + return nil // return early } - return nil -} + // TODO(hs): according to RFC8555: 7.6, a server MUST consider the following accounts authorized + // to revoke a certificate: + // + // o the account that issued the certificate. + // o an account that holds authorizations for all of the identifiers in the certificate. + // + // We currently only support the first case. The second might result in step going OOM when + // large numbers of Authorizations are involved when the current nosql interface is in use. + // We want to protect users from this failure scenario, so that's why it hasn't been added yet. + // This issue is tracked in https://github.com/smallstep/certificates/issues/767 -// identifierKey creates a unique key for an ACME identifier using -// the following format: ip|127.0.0.1; dns|*.example.com -func identifierKey(identifier acme.Identifier) string { - if identifier.Type == acme.IP { - return "ip|" + identifier.Value - } - if identifier.Type == acme.DNS { - return "dns|" + identifier.Value - } - return "unsupported|" + identifier.Value -} - -// extractIdentifiers extracts ACME identifiers from an x509 certificate and -// creates a map from them. The map ensures that duplicate SANs are deduplicated. -// The Subject CommonName is included, because RFC8555 7.4 states that DNS -// identifiers can come from either the CommonName or a DNS SAN or both. When -// authorizing issuance, the DNS identifier must be in the request and will be -// included in the validation (see Order.sans()) as of now. This means that the -// CommonName will in fact have an authorization available. -func extractIdentifiers(cert *x509.Certificate) map[string]acme.Identifier { - result := map[string]acme.Identifier{} - for _, name := range cert.DNSNames { - identifier := acme.Identifier{ - Type: acme.DNS, - Value: name, - } - result[identifierKey(identifier)] = identifier - } - for _, ip := range cert.IPAddresses { - identifier := acme.Identifier{ - Type: acme.IP, - Value: ip.String(), - } - result[identifierKey(identifier)] = identifier - } - if cert.Subject.CommonName != "" { - identifier := acme.Identifier{ - // assuming only DNS can be in Common Name (RFC8555, 7.4); RFC8738 - // IP Identifier Validation Extension does not state anything about this. - // This logic is in accordance with the logic in order.canonicalize() - Type: acme.DNS, - Value: cert.Subject.CommonName, - } - result[identifierKey(identifier)] = identifier - } - return result + // not authorized; fail closed. + return wrapUnauthorizedError(certToBeRevoked, nil, fmt.Sprintf("account '%s' is not authorized", account.ID), nil) } // wrapRevokeErr is a best effort implementation to transform an error during diff --git a/acme/api/revoke_test.go b/acme/api/revoke_test.go index cf036abb..4ff54405 100644 --- a/acme/api/revoke_test.go +++ b/acme/api/revoke_test.go @@ -715,35 +715,6 @@ func TestHandler_RevokeCert(t *testing.T) { }, } }, - "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { - 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", - Leaf: cert, - }, nil - }, - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - return nil, errors.New("force") - }, - } - ca := &mockCA{} - return test{ - db: db, - ca: ca, - ctx: ctx, - statusCode: 500, - err: acme.NewErrorISE("error retrieving authorizations for Account %s", "accountID"), - } - }, "fail/account-not-authorized": func(t *testing.T) test { acc := &acme.Account{ID: "accountID", Status: acme.StatusValid} ctx := context.WithValue(context.Background(), provisionerContextKey, prov) @@ -1150,101 +1121,13 @@ func TestHandler_isAccountAuthorized(t *testing.T) { }, } }, - "fail/no-certificate-identifiers": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{} - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: &acme.Error{ - Type: "urn:ietf:params:acme:error:unauthorized", - Status: http.StatusForbidden, - Detail: "No authorization provided", - Err: errors.New("cannot authorize revocation without providing identifiers to authorize"), - }, - } - }, - "fail/db.GetAuthorizationsByAccountID-error": func(t *testing.T) test { + "fail/different-account": func(t *testing.T) test { account := &acme.Account{ ID: accountID, Status: acme.StatusValid, } certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - } - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - db: &acme.MockDB{ - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - assert.Equals(t, "accountID", accountID) - return nil, errors.New("force") - }, - }, - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: acme.NewErrorISE("error retrieving authorizations for Account %s: force", accountID), - } - }, - "fail/no-valid-authorizations": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - } - existingCert := &acme.Certificate{ - AccountID: "differentAccountID", - } - return test{ - db: &acme.MockDB{ - MockGetAuthorizationsByAccountID: func(ctx context.Context, accountID string) ([]*acme.Authorization, error) { - assert.Equals(t, "accountID", accountID) - return []*acme.Authorization{ - { - AccountID: accountID, - Status: acme.StatusInvalid, - }, - }, nil - }, - }, - ctx: context.TODO(), - existingCert: existingCert, - certToBeRevoked: certToBeRevoked, - account: account, - err: &acme.Error{ - Type: "urn:ietf:params:acme:error:unauthorized", - Status: http.StatusForbidden, - Detail: "No authorization provided for name 127.0.0.1", - Err: errors.New("account 'accountID' does not have valid authorizations"), - }, - } - }, - "fail/authorizations-do-not-match-identifiers": func(t *testing.T) test { - account := &acme.Account{ - ID: accountID, - Status: acme.StatusValid, - } - certToBeRevoked := &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, + IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, } existingCert := &acme.Certificate{ AccountID: "differentAccountID", @@ -1259,7 +1142,7 @@ func TestHandler_isAccountAuthorized(t *testing.T) { Status: acme.StatusValid, Identifier: acme.Identifier{ Type: acme.IP, - Value: "127.0.0.2", + Value: "127.0.0.1", }, }, }, nil @@ -1272,8 +1155,8 @@ func TestHandler_isAccountAuthorized(t *testing.T) { err: &acme.Error{ Type: "urn:ietf:params:acme:error:unauthorized", Status: http.StatusForbidden, - Detail: "No authorization provided for name 127.0.0.1", - Err: errors.New("account 'accountID' does not have authorizations for all identifiers"), + Detail: "No authorization provided", + Err: errors.New("account 'accountID' is not authorized"), }, } }, @@ -1286,7 +1169,7 @@ func TestHandler_isAccountAuthorized(t *testing.T) { IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, } existingCert := &acme.Certificate{ - AccountID: "differentAccountID", + AccountID: "accountID", } return test{ db: &acme.MockDB{ @@ -1340,176 +1223,94 @@ func TestHandler_isAccountAuthorized(t *testing.T) { } } -func Test_identifierKey(t *testing.T) { - tests := []struct { - name string - identifier acme.Identifier - want string - }{ - { - name: "ip", - identifier: acme.Identifier{ - Type: acme.IP, - Value: "10.0.0.1", - }, - want: "ip|10.0.0.1", +func Test_wrapUnauthorizedError(t *testing.T) { + type test struct { + cert *x509.Certificate + unauthorizedIdentifiers []acme.Identifier + msg string + err error + want *acme.Error + } + var tests = map[string]func(t *testing.T) test{ + "unauthorizedIdentifiers": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name 127.0.0.1" + return test{ + err: nil, + cert: nil, + unauthorizedIdentifiers: []acme.Identifier{ + { + Type: acme.IP, + Value: "127.0.0.1", + }, + }, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, - { - name: "dns", - identifier: acme.Identifier{ - Type: acme.DNS, - Value: "*.example.com", - }, - want: "dns|*.example.com", + "subject": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name test.example.com" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "test.example.com", + }, + } + return test{ + err: nil, + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, - { - name: "unknown", - identifier: acme.Identifier{ - Type: "InvalidType", - Value: "<<>>", - }, - want: "unsupported|<<>>", + "wrap-subject": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "verification of jws using certificate public key failed: square/go-jose: error in cryptographic primitive") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided for name test.example.com" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "test.example.com", + }, + } + return test{ + err: errors.New("square/go-jose: error in cryptographic primitive"), + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "verification of jws using certificate public key failed", + want: acmeErr, + } + }, + "default": func(t *testing.T) test { + acmeErr := acme.NewError(acme.ErrorUnauthorizedType, "account 'accountID' is not authorized") + acmeErr.Status = http.StatusForbidden + acmeErr.Detail = "No authorization provided" + cert := &x509.Certificate{ + Subject: pkix.Name{ + CommonName: "", + }, + } + return test{ + err: nil, + cert: cert, + unauthorizedIdentifiers: []acme.Identifier{}, + msg: "account 'accountID' is not authorized", + want: acmeErr, + } }, } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := identifierKey(tt.identifier) - if !cmp.Equal(tt.want, got) { - t.Errorf("identifierKey() diff = \n%s", cmp.Diff(tt.want, got)) - } - }) - } -} - -func Test_extractIdentifiers(t *testing.T) { - tests := []struct { - name string - cert *x509.Certificate - want map[string]acme.Identifier - }{ - { - name: "ip", - cert: &x509.Certificate{ - IPAddresses: []net.IP{net.ParseIP("127.0.0.1")}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - }, - }, - { - name: "dns", - cert: &x509.Certificate{ - DNSNames: []string{"*.example.com"}, - }, - want: map[string]acme.Identifier{ - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - }, - }, - { - name: "dns-subject", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "www.example.com", - }, - }, - want: map[string]acme.Identifier{ - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - }, - }, - { - name: "ip-subject", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - }, - want: map[string]acme.Identifier{ - "dns|127.0.0.1": { // this is the currently expected behavior - Type: acme.DNS, - Value: "127.0.0.1", - }, - }, - }, - { - name: "combined", - cert: &x509.Certificate{ - Subject: pkix.Name{ - CommonName: "127.0.0.1", - }, - IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, - DNSNames: []string{"*.example.com", "www.example.com"}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - "ip|127.0.0.2": { - Type: acme.IP, - Value: "127.0.0.2", - }, - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - "dns|127.0.0.1": { // this is the currently expected behavior - Type: acme.DNS, - Value: "127.0.0.1", - }, - }, - }, - { - name: "ip-duplicates", - cert: &x509.Certificate{ - IPAddresses: []net.IP{net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.1"), net.ParseIP("127.0.0.2")}, - }, - want: map[string]acme.Identifier{ - "ip|127.0.0.1": { - Type: acme.IP, - Value: "127.0.0.1", - }, - "ip|127.0.0.2": { - Type: acme.IP, - Value: "127.0.0.2", - }, - }, - }, - { - name: "dns-duplicates", - cert: &x509.Certificate{ - DNSNames: []string{"*.example.com", "www.example.com", "www.example.com"}, - }, - want: map[string]acme.Identifier{ - "dns|*.example.com": { - Type: acme.DNS, - Value: "*.example.com", - }, - "dns|www.example.com": { - Type: acme.DNS, - Value: "www.example.com", - }, - }, - }, - } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - got := extractIdentifiers(tt.cert) - if !cmp.Equal(tt.want, got) { - t.Errorf("extractIdentifiers() diff=\n%s", cmp.Diff(tt.want, got)) - } + for name, prep := range tests { + tc := prep(t) + t.Run(name, func(t *testing.T) { + acmeErr := wrapUnauthorizedError(tc.cert, tc.unauthorizedIdentifiers, tc.msg, tc.err) + assert.Equals(t, acmeErr.Err.Error(), tc.want.Err.Error()) + assert.Equals(t, acmeErr.Type, tc.want.Type) + assert.Equals(t, acmeErr.Status, tc.want.Status) + assert.Equals(t, acmeErr.Detail, tc.want.Detail) + assert.Equals(t, acmeErr.Identifier, tc.want.Identifier) + assert.Equals(t, acmeErr.Subproblems, tc.want.Subproblems) }) } } From 00539d07c924f5217888a5e849e92ae7e3f6c87b Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Thu, 9 Dec 2021 09:42:38 +0100 Subject: [PATCH 20/20] Add changelog entry for ACME revocation --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5e9d0bc4..b41a90e3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/spec/v2.0.0. ## [Unreleased - 0.18.1] - DATE ### Added +- Support for ACME revocation. ### Changed ### Deprecated ### Removed