From 258efca0fa83785695021c058f50073807447749 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 10 Jul 2021 00:28:31 +0200 Subject: [PATCH] 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},