Refactor the PKI revocation handler to prep for unified revocation (#18685)

* Rename revokeCert variable to identify serial number formatting

* Refactor out lease specific behavior out of revokeCert

 - Isolate the specific behavior regarding revoking lease specific
   certificates outside of the revokeCert function and into the only
   caller that leveraged used it.
 - This allows us to simplify revokeCert a little bit and keeps the
   function purely about revoking a certificate

* Within revokeCert short circuit the already revoked use-case

 - Make the function a little easier to process by exiting early
   if the certificate has already been revoked.

* Do not load certificates from storage multiple times during revocation

 - Isolate the loading of a certificate and parsing of a certificate
   into a single attempt, either when provided the certificate for BYOC
   revocation or strictly from storage for the other revocation types.

* With BYOC write certificate entry using dashes not the legacy colon char

builtin/logical/pki/crl_util.go

@@ -7,7 +7,6 @@ import (
 	"crypto/x509/pkix"
 	"fmt"
 	"math/big"
-	"strings"
 	"sync"
 	"time"
 
@@ -468,7 +467,7 @@ func fetchIssuerMapForRevocationChecking(sc *storageContext) (map[issuerID]*x509
 }
 
 // Revokes a cert, and tries to be smart about error recovery
-func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Response, error) {
+func revokeCert(sc *storageContext, cert *x509.Certificate) (*logical.Response, error) {
 	// As this backend is self-contained and this function does not hook into
 	// third parties to manage users or resources, if the mount is tainted,
 	// revocation doesn't matter anyways -- the CRL that would be written will
@@ -478,6 +477,9 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 		return nil, nil
 	}
 
+	colonSerial := serialFromCert(cert)
+	hyphenSerial := normalizeSerial(colonSerial)
+
 	// Validate that no issuers match the serial number to be revoked. We need
 	// to gracefully degrade to the legacy cert bundle when it is required, as
 	// secondary PR clusters might not have been upgraded, but still need to
@@ -490,16 +492,13 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 	// Ensure we don't revoke an issuer via this API; use /issuer/:issuer_ref/revoke
 	// instead.
 	for issuer, certificate := range issuerIDCertMap {
-		colonSerial := strings.ReplaceAll(strings.ToLower(serial), "-", ":")
 		if colonSerial == serialFromCert(certificate) {
 			return logical.ErrorResponse(fmt.Sprintf("adding issuer (id: %v) to its own CRL is not allowed", issuer)), nil
 		}
 	}
 
-	alreadyRevoked := false
 	var revInfo revocationInfo
-
+	revEntry, err := fetchCertBySerial(sc, revokedPath, colonSerial)
-	revEntry, err := fetchCertBySerial(sc, revokedPath, serial)
 	if err != nil {
 		switch err.(type) {
 		case errutil.UserError:
@@ -510,77 +509,51 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 	}
 	if revEntry != nil {
 		// Set the revocation info to the existing values
-		alreadyRevoked = true
 		err = revEntry.DecodeJSON(&revInfo)
 		if err != nil {
 			return nil, fmt.Errorf("error decoding existing revocation info")
 		}
+
+		resp := &logical.Response{
+			Data: map[string]interface{}{
+				"revocation_time": revInfo.RevocationTime,
+			},
+		}
+		if !revInfo.RevocationTimeUTC.IsZero() {
+			resp.Data["revocation_time_rfc3339"] = revInfo.RevocationTimeUTC.Format(time.RFC3339Nano)
+		}
+		return resp, nil
 	}
 
-	if !alreadyRevoked {
+	// Add a little wiggle room because leases are stored with a second
-		certEntry, err := fetchCertBySerial(sc, "certs/", serial)
+	// granularity
-		if err != nil {
+	if cert.NotAfter.Before(time.Now().Add(2 * time.Second)) {
-			switch err.(type) {
+		response := &logical.Response{}
-			case errutil.UserError:
+		response.AddWarning(fmt.Sprintf("certificate with serial %s already expired; refusing to add to CRL", colonSerial))
-				return logical.ErrorResponse(err.Error()), nil
+		return response, nil
-			default:
-				return nil, err
-			}
-		}
-		if certEntry == nil {
-			if fromLease {
-				// We can't write to revoked/ or update the CRL anyway because we don't have the cert,
-				// and there's no reason to expect this will work on a subsequent
-				// retry.  Just give up and let the lease get deleted.
-				return nil, nil
-			}
-			return logical.ErrorResponse(fmt.Sprintf("certificate with serial %s not found", serial)), nil
-		}
-
-		cert, err := x509.ParseCertificate(certEntry.Value)
-		if err != nil {
-			return nil, fmt.Errorf("error parsing certificate: %w", err)
-		}
-		if cert == nil {
-			return nil, fmt.Errorf("got a nil certificate")
-		}
-
-		// Add a little wiggle room because leases are stored with a second
-		// granularity
-		if cert.NotAfter.Before(time.Now().Add(2 * time.Second)) {
-			response := &logical.Response{}
-			response.AddWarning(fmt.Sprintf("certificate with serial %s already expired; refusing to add to CRL", serial))
-			return response, nil
-		}
-
-		// Compatibility: Don't revoke CAs if they had leases. New CAs going
-		// forward aren't issued leases.
-		if cert.IsCA && fromLease {
-			return nil, nil
-		}
-
-		currTime := time.Now()
-		revInfo.CertificateBytes = certEntry.Value
-		revInfo.RevocationTime = currTime.Unix()
-		revInfo.RevocationTimeUTC = currTime.UTC()
-
-		// We may not find an issuer with this certificate; that's fine so
-		// ignore the return value.
-		associateRevokedCertWithIsssuer(&revInfo, cert, issuerIDCertMap)
-
-		revEntry, err = logical.StorageEntryJSON(revokedPath+normalizeSerial(serial), revInfo)
-		if err != nil {
-			return nil, fmt.Errorf("error creating revocation entry")
-		}
-
-		certsCounted := sc.Backend.certsCounted.Load()
-		err = sc.Storage.Put(sc.Context, revEntry)
-		if err != nil {
-			return nil, fmt.Errorf("error saving revoked certificate to new location")
-		}
-		sc.Backend.incrementTotalRevokedCertificatesCount(certsCounted, revEntry.Key)
 	}
 
+	currTime := time.Now()
+	revInfo.CertificateBytes = cert.Raw
+	revInfo.RevocationTime = currTime.Unix()
+	revInfo.RevocationTimeUTC = currTime.UTC()
+
+	// We may not find an issuer with this certificate; that's fine so
+	// ignore the return value.
+	associateRevokedCertWithIsssuer(&revInfo, cert, issuerIDCertMap)
+
+	revEntry, err = logical.StorageEntryJSON(revokedPath+hyphenSerial, revInfo)
+	if err != nil {
+		return nil, fmt.Errorf("error creating revocation entry")
+	}
+
+	certsCounted := sc.Backend.certsCounted.Load()
+	err = sc.Storage.Put(sc.Context, revEntry)
+	if err != nil {
+		return nil, fmt.Errorf("error saving revoked certificate to new location")
+	}
+	sc.Backend.incrementTotalRevokedCertificatesCount(certsCounted, revEntry.Key)
+
 	// Fetch the config and see if we need to rebuild the CRL. If we have
 	// auto building enabled, we will wait for the next rebuild period to
 	// actually rebuild it.
@@ -603,7 +576,7 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 				return nil, fmt.Errorf("error encountered during CRL building: %w", crlErr)
 			}
 		}
-	} else if !alreadyRevoked {
+	} else {
 		// Regardless of whether or not we've presently enabled Delta CRLs,
 		// we should always write the Delta WAL in case it is enabled in the
 		// future. We could trigger another full CRL rebuild instead (to avoid
@@ -619,7 +592,7 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 		//
 		// Currently we don't store any data in the WAL entry.
 		var walInfo deltaWALInfo
-		walEntry, err := logical.StorageEntryJSON(deltaWALPath+normalizeSerial(serial), walInfo)
+		walEntry, err := logical.StorageEntryJSON(deltaWALPath+hyphenSerial, walInfo)
 		if err != nil {
 			return nil, fmt.Errorf("unable to create delta CRL WAL entry")
 		}
@@ -631,7 +604,7 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 		// In order for periodic delta rebuild to be mildly efficient, we
 		// should write the last revoked delta WAL entry so we know if we
 		// have new revocations that we should rebuild the delta WAL for.
-		lastRevSerial := lastWALInfo{Serial: serial}
+		lastRevSerial := lastWALInfo{Serial: colonSerial}
 		lastWALEntry, err := logical.StorageEntryJSON(deltaWALLastRevokedSerial, lastRevSerial)
 		if err != nil {
 			return nil, fmt.Errorf("unable to create last delta CRL WAL entry")
@@ -641,15 +614,12 @@ func revokeCert(sc *storageContext, serial string, fromLease bool) (*logical.Res
 		}
 	}
 
-	resp := &logical.Response{
+	return &logical.Response{
 		Data: map[string]interface{}{
-			"revocation_time": revInfo.RevocationTime,
+			"revocation_time":         revInfo.RevocationTime,
+			"revocation_time_rfc3339": revInfo.RevocationTimeUTC.Format(time.RFC3339Nano),
 		},
-	}
+	}, nil
-	if !revInfo.RevocationTimeUTC.IsZero() {
-		resp.Data["revocation_time_rfc3339"] = revInfo.RevocationTimeUTC.Format(time.RFC3339Nano)
-	}
-	return resp, nil
 }
 
 func buildCRLs(sc *storageContext, forceNew bool) error {

builtin/logical/pki/path_revoke.go

@@ -9,11 +9,9 @@ import (
 	"crypto/x509"
 	"encoding/pem"
 	"fmt"
-	"strings"
 
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/certutil"
-	"github.com/hashicorp/vault/sdk/helper/consts"
 	"github.com/hashicorp/vault/sdk/helper/errutil"
 	"github.com/hashicorp/vault/sdk/logical"
 )
@@ -138,7 +136,7 @@ func pathRotateDeltaCRL(b *backend) *framework.Path {
 	}
 }
 
-func (b *backend) pathRevokeWriteHandleCertificate(ctx context.Context, req *logical.Request, certPem string) (string, bool, []byte, error) {
+func (b *backend) pathRevokeWriteHandleCertificate(ctx context.Context, req *logical.Request, certPem string) (string, bool, *x509.Certificate, error) {
 	// This function handles just the verification of the certificate against
 	// the global issuer set, checking whether or not it is importable.
 	//
@@ -208,7 +206,7 @@ func (b *backend) pathRevokeWriteHandleCertificate(ctx context.Context, req *log
 			// imported this certificate, likely when we issued it. We don't
 			// need to re-verify the signature as we assume it was already
 			// verified when it was imported.
-			return serial, false, certEntry.Value, nil
+			return serial, false, certReferenceStored, nil
 		}
 	}
 
@@ -242,13 +240,13 @@ func (b *backend) pathRevokeWriteHandleCertificate(ctx context.Context, req *log
 	}
 
 	if foundMatchingIssuer {
-		return serial, true, certReference.Raw, nil
+		return serial, true, certReference, nil
 	}
 
 	return serial, false, nil, errutil.UserError{Err: "unable to verify signature on presented cert from any present issuer in this mount; certificates from previous CAs will need to have their issuing CA and key re-imported if revocation is necessary"}
 }
 
-func (b *backend) pathRevokeWriteHandleKey(ctx context.Context, req *logical.Request, cert []byte, keyPem string) error {
+func (b *backend) pathRevokeWriteHandleKey(req *logical.Request, certReference *x509.Certificate, keyPem string) error {
 	if keyPem == "" {
 		// The only way to get here should be via the /revoke endpoint;
 		// validate the path one more time and return an error if necessary.
@@ -261,12 +259,6 @@ func (b *backend) pathRevokeWriteHandleKey(ctx context.Context, req *logical.Req
 		return nil
 	}
 
-	// Parse the certificate for reference.
-	certReference, err := x509.ParseCertificate(cert)
-	if err != nil {
-		return errutil.UserError{Err: fmt.Sprintf("certificate could not be parsed: %v", err)}
-	}
-
 	// Now parse the key's PEM block.
 	pemBlock, _ := pem.Decode([]byte(keyPem))
 	if pemBlock == nil {
@@ -347,16 +339,25 @@ func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, dat
 		}
 	}
 
+	writeCert := false
+	var cert *x509.Certificate
 	var serial string
-	if haveSerial {
+
+	sc := b.makeStorageContext(ctx, req.Storage)
+
+	if haveCert {
+		var err error
+		serial, writeCert, cert, err = b.pathRevokeWriteHandleCertificate(ctx, req, rawCertificate.(string))
+		if err != nil {
+			return nil, err
+		}
+	} else {
 		// Easy case: this cert should be in storage already.
 		serial = rawSerial.(string)
 		if len(serial) == 0 {
 			return logical.ErrorResponse("The serial number must be provided"), nil
 		}
 
-		// Here, fetch the certificate from disk to validate we can revoke it.
-		sc := b.makeStorageContext(ctx, req.Storage)
 		certEntry, err := fetchCertBySerial(sc, "certs/", serial)
 		if err != nil {
 			switch err.(type) {
@@ -366,67 +367,43 @@ func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, dat
 				return nil, err
 			}
 		}
-		if certEntry == nil {
-			return logical.ErrorResponse(fmt.Sprintf("certificate with serial %s not found or was already revoked", serial)), nil
-		}
 
-		// Now, if the user provided a key, we'll have to make sure the key
+		if certEntry != nil {
-		// and stored certificate match.
+			cert, err = x509.ParseCertificate(certEntry.Value)
-		if err := b.pathRevokeWriteHandleKey(ctx, req, certEntry.Value, keyPem); err != nil {
+			if err != nil {
-			return nil, err
+				return nil, fmt.Errorf("error parsing certificate: %w", err)
+			}
 		}
-	} else {
+	}
-		// Otherwise, we've gotta parse the certificate from the request and
+
-		// then import it into cluster-local storage. Before writing the
+	if cert == nil {
-		// certificate (and forwarding), we want to verify this certificate
+		return logical.ErrorResponse(fmt.Sprintf("certificate with serial %s not found or was already revoked", serial)), nil
-		// was actually signed by one of our present issuers.
+	}
-		var err error
+
-		var writeCert bool
+	// Before we write the certificate, we've gotta verify the request in
-		var certBytes []byte
+	// the event of a PoP-based revocation scheme; we don't want to litter
-		serial, writeCert, certBytes, err = b.pathRevokeWriteHandleCertificate(ctx, req, rawCertificate.(string))
+	// storage with issued-but-not-revoked certificates.
+	if err := b.pathRevokeWriteHandleKey(req, cert, keyPem); err != nil {
+		return nil, err
+	}
+
+	// At this point, a forward operation will occur if we're on a standby
+	// node as we're now attempting to write the bytes of the cert out to
+	// disk.
+	if writeCert {
+		err := req.Storage.Put(ctx, &logical.StorageEntry{
+			Key:   "certs/" + normalizeSerial(serial),
+			Value: cert.Raw,
+		})
 		if err != nil {
 			return nil, err
 		}
-
-		// Before we write the certificate, we've gotta verify the request in
-		// the event of a PoP-based revocation scheme; we don't want to litter
-		// storage with issued-but-not-revoked certificates.
-		if err := b.pathRevokeWriteHandleKey(ctx, req, certBytes, keyPem); err != nil {
-			return nil, err
-		}
-
-		// At this point, a forward operation will occur if we're on a standby
-		// node as we're now attempting to write the bytes of the cert out to
-		// disk.
-		if writeCert {
-			err = req.Storage.Put(ctx, &logical.StorageEntry{
-				Key:   "certs/" + serial,
-				Value: certBytes,
-			})
-			if err != nil {
-				return nil, err
-			}
-		}
-
-		// Finally, we have a valid serial number to use for BYOC revocation!
 	}
 
-	// Assumption: this check is cheap. Call this twice, in the cert-import
-	// case, to allow cert verification to get rejected on the standby node,
-	// but we still need it to protect the serial number case.
-	if b.System().ReplicationState().HasState(consts.ReplicationPerformanceStandby) {
-		return nil, logical.ErrReadOnly
-	}
-
-	// We store and identify by lowercase colon-separated hex, but other
-	// utilities use dashes and/or uppercase, so normalize
-	serial = strings.ReplaceAll(strings.ToLower(serial), "-", ":")
-
 	b.revokeStorageLock.Lock()
 	defer b.revokeStorageLock.Unlock()
 
-	sc := b.makeStorageContext(ctx, req.Storage)
+	return revokeCert(sc, cert)
-	return revokeCert(sc, serial, false)
 }
 
 func (b *backend) pathRotateCRLRead(ctx context.Context, req *logical.Request, _ *framework.FieldData) (*logical.Response, error) {

builtin/logical/pki/secret_certs.go

@@ -2,6 +2,7 @@ package pki
 
 import (
 	"context"
+	"crypto/x509"
 	"fmt"
 
 	"github.com/hashicorp/vault/sdk/framework"
@@ -49,9 +50,28 @@ func (b *backend) secretCredsRevoke(ctx context.Context, req *logical.Request, _
 	defer b.revokeStorageLock.Unlock()
 
 	sc := b.makeStorageContext(ctx, req.Storage)
-	resp, err := revokeCert(sc, serialInt.(string), true)
+
-	if resp == nil && err == nil {
+	certEntry, err := fetchCertBySerial(sc, "certs/", serialInt.(string))
-		b.Logger().Warn("expired certificate revoke failed because not found in storage, treating as success", "serial", serialInt.(string))
+	if err != nil {
+		return nil, err
 	}
-	return resp, err
+	if certEntry == nil {
+		// We can't write to revoked/ or update the CRL anyway because we don't have the cert,
+		// and there's no reason to expect this will work on a subsequent
+		// retry.  Just give up and let the lease get deleted.
+		b.Logger().Warn("expired certificate revoke failed because not found in storage, treating as success", "serial", serialInt.(string))
+		return nil, nil
+	}
+
+	cert, err := x509.ParseCertificate(certEntry.Value)
+	if err != nil {
+		return nil, fmt.Errorf("error parsing certificate: %w", err)
+	}
+
+	// Compatibility: Don't revoke CAs if they had leases. New CAs going forward aren't issued leases.
+	if cert.IsCA {
+		return nil, nil
+	}
+
+	return revokeCert(sc, cert)
 }