Forward PKI revocation requests received by standby nodes to active n…

…ode (#19624) (#19630)

* Forward PKI revocation requests received by standby nodes to active node

 - A refactoring that occurred in 1.13 timeframe removed what was
   considered a specific check for standby nodes that wasn't required
   as a writes should be returning ErrReadOnly.
 - That sadly exposed a long standing bug where the errors from the
   storage layer were not being properly wrapped, hiding the ErrReadOnly
   coming from a write and failing the request.

* Add cl

* Add test for basic PKI operations against standby nodes

Co-authored-by: Steven Clark <steven.clark@hashicorp.com>

builtin/logical/pki/backend_test.go

@@ -31,6 +31,8 @@ import (
 	"testing"
 	"time"
 
+	"github.com/hashicorp/vault/helper/testhelpers"
+
 	"github.com/stretchr/testify/require"
 
 	"github.com/armon/go-metrics"
@@ -6346,6 +6348,56 @@ func TestUserIDsInLeafCerts(t *testing.T) {
 	requireSubjectUserIDAttr(t, resp.Data["certificate"].(string), "humanoid")
 }
 
+// TestStandby_Operations test proper forwarding for PKI requests from a standby node to the
+// active node within a cluster.
+func TestStandby_Operations(t *testing.T) {
+	conf := &vault.CoreConfig{
+		LogicalBackends: map[string]logical.Factory{
+			"pki": Factory,
+		},
+	}
+	opts := vault.TestClusterOptions{HandlerFunc: vaulthttp.Handler}
+	cluster := vault.NewTestCluster(t, conf, &opts)
+	cluster.Start()
+	defer cluster.Cleanup()
+
+	testhelpers.WaitForActiveNodeAndStandbys(t, cluster)
+	standbyCores := testhelpers.DeriveStandbyCores(t, cluster)
+	require.Greater(t, len(standbyCores), 0, "Need at least one standby core.")
+	client := standbyCores[0].Client
+
+	mountPKIEndpoint(t, client, "pki")
+
+	_, err := client.Logical().Write("pki/root/generate/internal", map[string]interface{}{
+		"key_type":    "ec",
+		"common_name": "root-ca.com",
+		"ttl":         "600h",
+	})
+	require.NoError(t, err, "error setting up pki role: %v", err)
+
+	_, err = client.Logical().Write("pki/roles/example", map[string]interface{}{
+		"allowed_domains":  "example.com",
+		"allow_subdomains": "true",
+		"no_store":         "false", // make sure we store this cert
+		"ttl":              "5h",
+		"key_type":         "ec",
+	})
+	require.NoError(t, err, "error setting up pki role: %v", err)
+
+	resp, err := client.Logical().Write("pki/issue/example", map[string]interface{}{
+		"common_name": "test.example.com",
+	})
+	require.NoError(t, err, "error issuing certificate: %v", err)
+	require.NotNil(t, resp, "got nil response from issuing request")
+	serialOfCert := resp.Data["serial_number"].(string)
+
+	resp, err = client.Logical().Write("pki/revoke", map[string]interface{}{
+		"serial_number": serialOfCert,
+	})
+	require.NoError(t, err, "error revoking certificate: %v", err)
+	require.NotNil(t, resp, "got nil response from revoke request")
+}
+
 var (
 	initTest  sync.Once
 	rsaCAKey  string

builtin/logical/pki/crl_util.go

@@ -153,7 +153,6 @@ func (cb *crlBuilder) reloadConfigIfRequired(sc *storageContext) error {
 		}
 
 		previousConfig := cb.config
-
 		// Set the default config if none was returned to us.
 		if config != nil {
 			cb.config = *config
@@ -351,7 +350,7 @@ func (cb *crlBuilder) _getPresentDeltaWALForClearing(sc *storageContext, path st
 	// Clearing of the delta WAL occurs after a new complete CRL has been built.
 	walSerials, err := sc.Storage.List(sc.Context, path)
 	if err != nil {
-		return nil, fmt.Errorf("error fetching list of delta WAL certificates to clear: %s", err)
+		return nil, fmt.Errorf("error fetching list of delta WAL certificates to clear: %w", err)
 	}
 
 	// We _should_ remove the special WAL entries here, but we don't really
@@ -377,7 +376,7 @@ func (cb *crlBuilder) _clearDeltaWAL(sc *storageContext, walSerials []string, pa
 		}
 
 		if err := sc.Storage.Delete(sc.Context, path+serial); err != nil {
-			return fmt.Errorf("error clearing delta WAL certificate: %s", err)
+			return fmt.Errorf("error clearing delta WAL certificate: %w", err)
 		}
 	}
 
@@ -655,7 +654,7 @@ func (cb *crlBuilder) processRevocationQueue(sc *storageContext) error {
 		(!sc.Backend.System().LocalMount() && sc.Backend.System().ReplicationState().HasState(consts.ReplicationPerformanceSecondary))
 
 	if err := cb.maybeGatherQueueForFirstProcess(sc, isNotPerfPrimary); err != nil {
-		return fmt.Errorf("failed to gather first queue: %v", err)
+		return fmt.Errorf("failed to gather first queue: %w", err)
 	}
 
 	revQueue := cb.revQueue.Iterate()
@@ -970,13 +969,13 @@ func revokeCert(sc *storageContext, config *crlConfig, cert *x509.Certificate) (
 
 	revEntry, err := logical.StorageEntryJSON(revokedPath+hyphenSerial, revInfo)
 	if err != nil {
-		return nil, fmt.Errorf("error creating revocation entry")
+		return nil, fmt.Errorf("error creating revocation entry: %w", err)
 	}
 
 	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")
+		return nil, fmt.Errorf("error saving revoked certificate to new location: %w", err)
 	}
 	sc.Backend.incrementTotalRevokedCertificatesCount(certsCounted, revEntry.Key)
 
@@ -1082,7 +1081,7 @@ func writeSpecificRevocationDeltaWALs(sc *storageContext, hyphenSerial string, c
 	}
 
 	if err = sc.Storage.Put(sc.Context, walEntry); err != nil {
-		return fmt.Errorf("error saving delta CRL WAL entry")
+		return fmt.Errorf("error saving delta CRL WAL entry: %w", err)
 	}
 
 	// In order for periodic delta rebuild to be mildly efficient, we
@@ -1094,7 +1093,7 @@ func writeSpecificRevocationDeltaWALs(sc *storageContext, hyphenSerial string, c
 		return fmt.Errorf("unable to create last delta CRL WAL entry")
 	}
 	if err = sc.Storage.Put(sc.Context, lastWALEntry); err != nil {
-		return fmt.Errorf("error saving last delta CRL WAL entry")
+		return fmt.Errorf("error saving last delta CRL WAL entry: %w", err)
 	}
 
 	return nil
@@ -1841,12 +1840,12 @@ func getLocalRevokedCertEntries(sc *storageContext, issuerIDCertMap map[issuerID
 			// we should update the entry to make future CRL builds faster.
 			revokedEntry, err = logical.StorageEntryJSON(revokedPath+serial, revInfo)
 			if err != nil {
-				return nil, nil, fmt.Errorf("error creating revocation entry for existing cert: %v", serial)
+				return nil, nil, fmt.Errorf("error creating revocation entry for existing cert: %v: %w", serial, err)
 			}
 
 			err = sc.Storage.Put(sc.Context, revokedEntry)
 			if err != nil {
-				return nil, nil, fmt.Errorf("error updating revoked certificate at existing location: %v", serial)
+				return nil, nil, fmt.Errorf("error updating revoked certificate at existing location: %v: %w", serial, err)
 			}
 		}
 	}

builtin/logical/pki/path_revoke.go

@@ -12,6 +12,8 @@ import (
 	"strings"
 	"time"
 
+	"github.com/hashicorp/vault/sdk/helper/consts"
+
 	"github.com/hashicorp/vault/sdk/framework"
 	"github.com/hashicorp/vault/sdk/helper/certutil"
 	"github.com/hashicorp/vault/sdk/helper/errutil"
@@ -490,6 +492,13 @@ func (b *backend) pathRevokeWrite(ctx context.Context, req *logical.Request, dat
 		}
 	}
 
+	// 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
+	}
+
 	b.revokeStorageLock.Lock()
 	defer b.revokeStorageLock.Unlock()
 

changelog/19624.txt

@@ -0,0 +1,3 @@
+```release-note:bug
+secrets/pki: Fix PKI revocation request forwarding from standby nodes due to an error wrapping bug
+```