From 84e7d468f226843820419808d5505e4ee22436f3 Mon Sep 17 00:00:00 2001 From: Herman Slatman Date: Sat, 3 Jul 2021 00:21:17 +0200 Subject: [PATCH] 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,