From d767d4cb80dd384a12b3b5eae72c1b0b661bb213 Mon Sep 17 00:00:00 2001 From: Steven Clark Date: Mon, 20 Mar 2023 10:58:36 -0400 Subject: [PATCH] Forward PKI revocation requests received by standby nodes to active node (#19624) * 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 --- builtin/logical/pki/backend_test.go | 52 +++++++++++++++++++++++++++++ builtin/logical/pki/crl_util.go | 19 +++++------ builtin/logical/pki/path_revoke.go | 9 +++++ changelog/19624.txt | 3 ++ 4 files changed, 73 insertions(+), 10 deletions(-) create mode 100644 changelog/19624.txt diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 51f986b5329c..570afe82f4ac 100644 --- a/builtin/logical/pki/backend_test.go +++ b/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 diff --git a/builtin/logical/pki/crl_util.go b/builtin/logical/pki/crl_util.go index 7cd975f75f1c..641e90c8a761 100644 --- a/builtin/logical/pki/crl_util.go +++ b/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) } } } diff --git a/builtin/logical/pki/path_revoke.go b/builtin/logical/pki/path_revoke.go index d488450aaa18..f35daefb50fc 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/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() diff --git a/changelog/19624.txt b/changelog/19624.txt new file mode 100644 index 000000000000..7bc2df63ea85 --- /dev/null +++ b/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 +```