Add ACME revocation handlers (#20340)

* Add ACME revocation handlers This refactors path_revoke to expose Proof of Possession verification, which is reused by ACME to allow methods 1 and 2: 1. Revocation of a certificate issued by the account, using account signature as sufficient proof. 2. Revocation of a certificate via proving possession of its private key, using this private key to create the JWS signature. We do not support the third mechanism, completing challenges equivalent to those on the existing certificate and then performing a revocation under an account which didn't issue the certificate but which did solve those challenges. We additionally create another map account->cert->order, allowing us to quickly look up if a cert was issued by this particular account. Note that the inverse lookup of cert->(account, order) lookup isn't yet possible due to Vault's storage structure. Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Update ACME pkiext tests to revoke certs Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Add auth handler checks Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> * Address review feedback Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com> --------- Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
2023-04-25 16:48:30 -04:00 · 2023-04-25 16:48:30 -04:00 · d3722a33c8
parent 3a995707b5
commit d3722a33c8
7 changed files with 276 additions and 4 deletions
--- a/builtin/logical/pki/acme_state.go
+++ b/builtin/logical/pki/acme_state.go
@ -519,6 +519,54 @@ func (a *acmeState) ListOrderIds(ac *acmeContext, accountId string) ([]string, e
 	return orderIds, nil
 }

+type acmeCertEntry struct {
+	Serial  string `json:"-"`
+	Account string `json:"-"`
+	Order   string `json:"order"`
+}
+
+func (a *acmeState) TrackIssuedCert(ac *acmeContext, accountId string, serial string, orderId string) error {
+	path := acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial)
+	entry := acmeCertEntry{
+		Order: orderId,
+	}
+
+	json, err := logical.StorageEntryJSON(path, &entry)
+	if err != nil {
+		return fmt.Errorf("error serializing acme cert entry: %w", err)
+	}
+
+	if err = ac.sc.Storage.Put(ac.sc.Context, json); err != nil {
+		return fmt.Errorf("error writing acme cert entry: %w", err)
+	}
+
+	return nil
+}
+
+func (a *acmeState) GetIssuedCert(ac *acmeContext, accountId string, serial string) (*acmeCertEntry, error) {
+	path := acmeAccountPrefix + accountId + "/certs/" + normalizeSerial(serial)
+
+	entry, err := ac.sc.Storage.Get(ac.sc.Context, path)
+	if err != nil {
+		return nil, fmt.Errorf("error loading acme cert entry: %w", err)
+	}
+
+	if entry == nil {
+		return nil, fmt.Errorf("no certificate with this serial was issued for this account")
+	}
+
+	var cert acmeCertEntry
+	err = entry.DecodeJSON(&cert)
+	if err != nil {
+		return nil, fmt.Errorf("error decoding acme cert entry: %w", err)
+	}
+
+	cert.Serial = denormalizeSerial(serial)
+	cert.Account = accountId
+
+	return &cert, nil
+}
+
 func getAuthorizationPath(accountId string, authId string) string {
 	return acmeAccountPrefix + accountId + "/authorizations/" + authId
 }
--- a/builtin/logical/pki/backend.go
+++ b/builtin/logical/pki/backend.go
@ -243,6 +243,7 @@ func Backend(conf *logical.BackendConfig) *backend {
 	acmePaths = append(acmePaths, pathAcmeFetchOrderCert(&b)...)
 	acmePaths = append(acmePaths, pathAcmeChallenge(&b)...)
 	acmePaths = append(acmePaths, pathAcmeAuthorization(&b)...)
+	acmePaths = append(acmePaths, pathAcmeRevoke(&b)...)

 	for _, acmePath := range acmePaths {
 		b.Backend.Paths = append(b.Backend.Paths, acmePath)
--- a/builtin/logical/pki/backend_test.go
+++ b/builtin/logical/pki/backend_test.go
@ -6816,6 +6816,7 @@ func TestProperAuthing(t *testing.T) {
 		paths[acmePrefix+"acme/directory"] = shouldBeUnauthedReadList
 		paths[acmePrefix+"acme/new-nonce"] = shouldBeUnauthedReadList
 		paths[acmePrefix+"acme/new-account"] = shouldBeUnauthedWriteOnly
+		paths[acmePrefix+"acme/revoke-cert"] = shouldBeUnauthedWriteOnly
 		paths[acmePrefix+"acme/new-order"] = shouldBeUnauthedWriteOnly
 		paths[acmePrefix+"acme/orders"] = shouldBeUnauthedWriteOnly
 		paths[acmePrefix+"acme/account/hrKmDYTvicHoHGVN2-3uzZV_BPGdE0W_dNaqYTtYqeo="] = shouldBeUnauthedWriteOnly
--- a/builtin/logical/pki/path_acme_order.go
+++ b/builtin/logical/pki/path_acme_order.go
@ -268,6 +268,11 @@ func (b *backend) acmeFinalizeOrderHandler(ac *acmeContext, _ *logical.Request,
 		return nil, err
 	}

+	if err := b.acmeState.TrackIssuedCert(ac, order.AccountId, hyphenSerialNumber, order.OrderId); err != nil {
+		b.Logger().Warn("orphaned generated ACME certificate due to error saving account->cert->order reference", "serial_number", hyphenSerialNumber, "error", err)
+		return nil, err
+	}
+
 	order.Status = ACMEOrderValid
 	order.CertificateSerialNumber = hyphenSerialNumber
 	order.CertificateExpiry = signedCertBundle.Certificate.NotAfter
--- a/builtin/logical/pki/path_acme_revoke.go
+++ b/builtin/logical/pki/path_acme_revoke.go
@ -0,0 +1,180 @@
+// Copyright (c) HashiCorp, Inc.
+// SPDX-License-Identifier: MPL-2.0
+
+package pki
+
+import (
+	"bytes"
+	"crypto"
+	"crypto/x509"
+	"encoding/base64"
+	"fmt"
+	"time"
+
+	"github.com/hashicorp/vault/sdk/framework"
+	"github.com/hashicorp/vault/sdk/logical"
+)
+
+func pathAcmeRevoke(b *backend) []*framework.Path {
+	return buildAcmeFrameworkPaths(b, patternAcmeRevoke, "/revoke-cert")
+}
+
+func patternAcmeRevoke(b *backend, pattern string) *framework.Path {
+	fields := map[string]*framework.FieldSchema{}
+	addFieldsForACMEPath(fields, pattern)
+	addFieldsForACMERequest(fields)
+
+	return &framework.Path{
+		Pattern: pattern,
+		Fields:  fields,
+		Operations: map[logical.Operation]framework.OperationHandler{
+			logical.UpdateOperation: &framework.PathOperation{
+				Callback:                    b.acmeParsedWrapper(b.acmeRevocationHandler),
+				ForwardPerformanceSecondary: false,
+				ForwardPerformanceStandby:   true,
+			},
+		},
+
+		HelpSynopsis:    "",
+		HelpDescription: "",
+	}
+}
+
+func (b *backend) acmeRevocationHandler(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}) (*logical.Response, error) {
+	var cert *x509.Certificate
+
+	rawCertificate, present := data["certificate"]
+	if present {
+		certBase64, ok := rawCertificate.(string)
+		if !ok {
+			return nil, fmt.Errorf("invalid type (%T; expected string) for field 'certificate': %w", rawCertificate, ErrMalformed)
+		}
+
+		certBytes, err := base64.RawURLEncoding.DecodeString(certBase64)
+		if err != nil {
+			return nil, fmt.Errorf("failed to base64 decode certificate: %v: %w", err, ErrMalformed)
+		}
+
+		cert, err = x509.ParseCertificate(certBytes)
+		if err != nil {
+			return nil, fmt.Errorf("failed to parse certificate: %v: %w", err, ErrMalformed)
+		}
+	} else {
+		return nil, fmt.Errorf("bad request was lacking required field 'certificate': %w", ErrMalformed)
+	}
+
+	rawReason, present := data["reason"]
+	if present {
+		reason, ok := rawReason.(float64)
+		if !ok {
+			return nil, fmt.Errorf("invalid type (%T; expected float64) for field 'reason': %w", rawReason, ErrMalformed)
+		}
+
+		if int(reason) != 0 {
+			return nil, fmt.Errorf("Vault does not support revocation reasons (got %v; expected omitted or 0/unspecified): %w", int(reason), ErrBadRevocationReason)
+		}
+	}
+
+	// If the certificate expired, there's no point in revoking it.
+	if cert.NotAfter.Before(time.Now()) {
+		return nil, fmt.Errorf("refusing to revoke expired certificate: %w", ErrMalformed)
+	}
+
+	// Fetch the CRL config as we need it to ultimately do the
+	// revocation. This should be cached and thus relatively fast.
+	config, err := b.crlBuilder.getConfigWithUpdate(acmeCtx.sc)
+	if err != nil {
+		return nil, fmt.Errorf("unable to revoke certificate: failed reading revocation config: %v: %w", err, ErrServerInternal)
+	}
+
+	// Load our certificate from storage to ensure it exists and matches
+	// what was given to us.
+	serial := serialFromCert(cert)
+	certEntry, err := fetchCertBySerial(acmeCtx.sc, "certs/", serial)
+	if err != nil {
+		return nil, fmt.Errorf("unable to revoke certificate: err reading global cert entry: %v: %w", err, ErrServerInternal)
+	}
+	if certEntry == nil {
+		return nil, fmt.Errorf("unable to revoke certificate: no global cert entry found: %w", ErrServerInternal)
+	}
+
+	// Validate that the provided certificate matches the stored
+	// certificate. This completes the chain of:
+	//
+	//     provided_auth -> provided_cert == stored cert.
+	//
+	// Allowing revocation to be safe.
+	//
+	// We use the non-subtle unsafe bytes equality check here as we have
+	// already fetched this certificate from storage, thus already leaking
+	// timing information that this cert exists. The user could thus simply
+	// fetch the cert from Vault matching this serial number via the unauthed
+	// pki/certs/:serial API endpoint.
+	if !bytes.Equal(certEntry.Value, cert.Raw) {
+		return nil, fmt.Errorf("unable to revoke certificate: supplied certificate does not match CA's stored value: %w", ErrMalformed)
+	}
+
+	// Check if it was already revoked; in this case, we do not need to
+	// revoke it again and want to respond with an appropriate error message.
+	revEntry, err := fetchCertBySerial(acmeCtx.sc, "revoked/", serial)
+	if err != nil {
+		return nil, fmt.Errorf("unable to revoke certificate: err reading revocation entry: %v: %w", err, ErrServerInternal)
+	}
+	if revEntry != nil {
+		return nil, fmt.Errorf("unable to revoke certificate: %w", ErrAlreadyRevoked)
+	}
+
+	// Finally, do the relevant permissions/authorization check as
+	// appropriate based on the type of revocation happening.
+	if !userCtx.Existing {
+		return b.acmeRevocationByPoP(acmeCtx, r, fields, userCtx, data, cert, config)
+	}
+
+	return b.acmeRevocationByAccount(acmeCtx, r, fields, userCtx, data, cert, config)
+}
+
+func (b *backend) acmeRevocationByPoP(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, cert *x509.Certificate, config *crlConfig) (*logical.Response, error) {
+	// Since this account does not exist, ensure we've gotten a private key
+	// matching the certificate's public key.
+	signer, ok := userCtx.Key.Key.(crypto.Signer)
+	if !ok {
+		return nil, fmt.Errorf("unable to revoke certificate: unable to parse JWS key of type (%T): %w", userCtx.Key.Key, ErrMalformed)
+	}
+
+	// Ensure that our PoP is indeed valid.
+	if err := validatePrivateKeyMatchesCert(signer, cert); err != nil {
+		return nil, fmt.Errorf("unable to revoke certificate: unable to verify proof of possession: %v: %w", err, ErrMalformed)
+	}
+
+	// Now it is safe to revoke.
+	b.revokeStorageLock.Lock()
+	defer b.revokeStorageLock.Unlock()
+
+	return revokeCert(acmeCtx.sc, config, cert)
+}
+
+func (b *backend) acmeRevocationByAccount(acmeCtx *acmeContext, r *logical.Request, fields *framework.FieldData, userCtx *jwsCtx, data map[string]interface{}, cert *x509.Certificate, config *crlConfig) (*logical.Response, error) {
+	// Fetch the account; disallow revocations from non-valid-status
+	// accounts.
+	account, err := b.acmeState.LoadAccount(acmeCtx, userCtx.Kid)
+	if err != nil {
+		return nil, fmt.Errorf("failed to lookup account: %w", err)
+	}
+	if account.Status != StatusValid {
+		return nil, fmt.Errorf("account isn't presently valid: %w", ErrUnauthorized)
+	}
+
+	// We only support certificates issued by this user, we don't support
+	// cross-account revocations.
+	serial := serialFromCert(cert)
+	acmeEntry, err := b.acmeState.GetIssuedCert(acmeCtx, userCtx.Kid, serial)
+	if err != nil || acmeEntry == nil {
+		return nil, fmt.Errorf("unable to revoke certificate: %v: %w", err, ErrMalformed)
+	}
+
+	// Now it is safe to revoke.
+	b.revokeStorageLock.Lock()
+	defer b.revokeStorageLock.Unlock()
+
+	return revokeCert(acmeCtx.sc, config, cert)
+}
--- a/builtin/logical/pki/path_revoke.go
+++ b/builtin/logical/pki/path_revoke.go
@ -5,6 +5,7 @@ package pki

 import (
 	"context"
+	"crypto"
 	"crypto/ecdsa"
 	"crypto/ed25519"
 	"crypto/rsa"
@ -446,6 +447,10 @@ func (b *backend) pathRevokeWriteHandleKey(req *logical.Request, certReference *
 		return fmt.Errorf("failed to parse provided private key: %w", err)
 	}

+	return validatePrivateKeyMatchesCert(signer, certReference)
+}
+
+func validatePrivateKeyMatchesCert(signer crypto.Signer, certReference *x509.Certificate) error {
 	// Finally, verify if the cert and key match. This code has been
 	// cribbed from the Go TLS config code, with minor modifications.
 	//
@ -453,7 +458,6 @@ func (b *backend) pathRevokeWriteHandleKey(req *logical.Request, certReference *
 	// components and ensure we validate exponent and curve information
 	// as well.
 	//
-	//
 	// See: https://github.com/golang/go/blob/c6a2dada0df8c2d75cf3ae599d7caed77d416fa2/src/crypto/tls/tls.go#L304-L331
 	switch certPub := certReference.PublicKey.(type) {
 	case *rsa.PublicKey:
--- a/builtin/logical/pkiext/acme_test.go
+++ b/builtin/logical/pkiext/acme_test.go
@ -65,19 +65,52 @@ func CheckCertBot(t *testing.T, vaultNetwork string, vaultNodeID string, directo
 		"--agree-tos",
 		"--no-verify-ssl",
 		"--standalone",
+		"--non-interactive",
 		"--server", directory,
 		"-d", hostname,
 	}
 	logCatCmd := []string{"cat", "/var/log/letsencrypt/letsencrypt.log"}

 	stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotCmd)
-	t.Logf("Certbot Command: %v\nstdout: %v\nstderr: %v\n", certbotCmd, string(stdout), string(stderr))
+	t.Logf("Certbot Issue Command: %v\nstdout: %v\nstderr: %v\n", certbotCmd, string(stdout), string(stderr))
 	if err != nil || retcode != 0 {
 		logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd)
 		t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr))
 	}
-	require.NoError(t, err, "got error running command")
-	require.Equal(t, 0, retcode, "expected zero retcode")
+	require.NoError(t, err, "got error running issue command")
+	require.Equal(t, 0, retcode, "expected zero retcode issue command result")
+
+	certbotRevokeCmd := []string{
+		"certbot",
+		"revoke",
+		"--no-eff-email",
+		"--email", "certbot.client@dadgarcorp.com",
+		"--agree-tos",
+		"--no-verify-ssl",
+		"--non-interactive",
+		"--no-delete-after-revoke",
+		"--cert-name", hostname,
+	}
+
+	stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotRevokeCmd)
+	t.Logf("Certbot Revoke Command: %v\nstdout: %v\nstderr: %v\n", certbotRevokeCmd, string(stdout), string(stderr))
+	if err != nil || retcode != 0 {
+		logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd)
+		t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr))
+	}
+	require.NoError(t, err, "got error running revoke command")
+	require.Equal(t, 0, retcode, "expected zero retcode revoke command result")
+
+	// Revoking twice should fail.
+	stdout, stderr, retcode, err = runner.RunCmdWithOutput(ctx, result.Container.ID, certbotRevokeCmd)
+	t.Logf("Certbot Double Revoke Command: %v\nstdout: %v\nstderr: %v\n", certbotRevokeCmd, string(stdout), string(stderr))
+	if err != nil || retcode == 0 {
+		logsStdout, logsStderr, _, _ := runner.RunCmdWithOutput(ctx, result.Container.ID, logCatCmd)
+		t.Logf("Certbot logs\nstdout: %v\nstderr: %v\n", string(logsStdout), string(logsStderr))
+	}
+
+	require.NoError(t, err, "got error running double revoke command")
+	require.NotEqual(t, 0, retcode, "expected non-zero retcode double revoke command result")

 	runner.Stop(ctx, result.Container.ID)
 }