diff --git a/builtin/logical/pki/backend_test.go b/builtin/logical/pki/backend_test.go index 9f2965be8..b6bfeb292 100644 --- a/builtin/logical/pki/backend_test.go +++ b/builtin/logical/pki/backend_test.go @@ -33,6 +33,8 @@ import ( "testing" "time" + "github.com/hashicorp/vault/helper/testhelpers" + "github.com/hashicorp/vault/sdk/helper/testhelpers/schema" "github.com/stretchr/testify/require" @@ -6408,6 +6410,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 a24d968ad..09ae85e7b 100644 --- a/builtin/logical/pki/crl_util.go +++ b/builtin/logical/pki/crl_util.go @@ -156,7 +156,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 @@ -354,7 +353,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 @@ -380,7 +379,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) } } @@ -658,7 +657,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() @@ -973,13 +972,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.ifCountEnabledIncrementTotalRevokedCertificatesCount(certsCounted, revEntry.Key) @@ -1085,7 +1084,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 @@ -1097,7 +1096,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 @@ -1844,12 +1843,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 3c86c72c2..fa6f3b648 100644 --- a/builtin/logical/pki/path_revoke.go +++ b/builtin/logical/pki/path_revoke.go @@ -16,6 +16,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" @@ -591,6 +593,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 000000000..7bc2df63e --- /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 +```