diff --git a/builtin/logical/transit/path_cache_config.go b/builtin/logical/transit/path_cache_config.go index f8f0cea551..e2d69eb5ae 100644 --- a/builtin/logical/transit/path_cache_config.go +++ b/builtin/logical/transit/path_cache_config.go @@ -45,15 +45,6 @@ func (b *backend) pathCacheConfig() *framework.Path { OperationSuffix: "cache", }, }, - - logical.CreateOperation: &framework.PathOperation{ - Callback: b.pathCacheConfigWrite, - Summary: "Configures a new cache of the specified size", - DisplayAttrs: &framework.DisplayAttributes{ - OperationVerb: "configure", - OperationSuffix: "cache", - }, - }, }, HelpSynopsis: pathCacheConfigHelpSyn, diff --git a/changelog/18492.txt b/changelog/18492.txt new file mode 100644 index 0000000000..6b0b3b5077 --- /dev/null +++ b/changelog/18492.txt @@ -0,0 +1,3 @@ +```release-note:improvement +framework: Make it an error for `CreateOperation` to be defined without an `ExistenceCheck`, thereby fixing misleading `x-vault-createSupported` in OpenAPI +``` diff --git a/helper/builtinplugins/builtinplugins_test.go b/helper/builtinplugins/builtinplugins_test.go new file mode 100644 index 0000000000..92ab12ca23 --- /dev/null +++ b/helper/builtinplugins/builtinplugins_test.go @@ -0,0 +1,150 @@ +package builtinplugins + +import ( + "testing" + + "github.com/hashicorp/go-hclog" + logicalKv "github.com/hashicorp/vault-plugin-secrets-kv" + "github.com/hashicorp/vault/api" + logicalDb "github.com/hashicorp/vault/builtin/logical/database" + vaulthttp "github.com/hashicorp/vault/http" + "github.com/hashicorp/vault/sdk/helper/consts" + "github.com/hashicorp/vault/sdk/helper/logging" + "github.com/hashicorp/vault/sdk/logical" + "github.com/hashicorp/vault/vault" +) + +// TestBuiltinPluginsWork exists to confirm that all the credential and secrets plugins in Registry can successfully be +// initialized. Database plugins are excluded as there is no general way to initialize them - they require +// plugin-specific configuration at the time of initialization. +// +// This detects coding errors which would cause the plugins to panic on initialization - various aspects of the +// configuration of a framework.Backend are checked during Backend.init(), which runs as a sync.Once function triggered +// upon first request. +// +// In this test, a help request is used to trigger that initialization, since it is valid for all plugins. +func TestBuiltinPluginsWork(t *testing.T) { + cluster := vault.NewTestCluster( + t, + &vault.CoreConfig{ + BuiltinRegistry: Registry, + LogicalBackends: map[string]logical.Factory{ + // This needs to be here for madly overcomplicated reasons, otherwise we end up mounting a KV v1 even + // when we try to explicitly mount a KV v2... + // + // vault.NewCore hardcodes "kv" to vault.PassthroughBackendFactory if no explicit entry is configured, + // and this hardcoding is re-overridden in command.logicalBackends to point back to the real KV plugin. + // As far as I can tell, nothing at all relies upon the definition of "kv" in builtinplugins.Registry, + // as it always gets resolved via the logicalBackends map and the pluginCatalog is never queried. + "kv": logicalKv.Factory, + // Semi-similarly, "database" is added in command.logicalBackends and not at all in + // builtinplugins.Registry, so we need to add it here to be able to test it! + "database": logicalDb.Factory, + }, + Logger: logging.NewVaultLogger(hclog.Trace), + PendingRemovalMountsAllowed: true, + }, + &vault.TestClusterOptions{ + HandlerFunc: vaulthttp.Handler, + NumCores: 1, + }, + ) + + cluster.Start() + defer cluster.Cleanup() + + cores := cluster.Cores + vault.TestWaitActive(t, cores[0].Core) + client := cores[0].Client + + for _, authType := range append( + Registry.Keys(consts.PluginTypeCredential), + "token", + ) { + deprecationStatus, _ := Registry.DeprecationStatus(authType, consts.PluginTypeCredential) + if deprecationStatus == consts.Removed { + continue + } + + t.Run("Auth Method "+authType, func(t *testing.T) { + // This builtin backend is automatically mounted and should not be mounted again + if authType != "token" { + if err := client.Sys().EnableAuthWithOptions(authType, &api.EnableAuthOptions{ + Type: authType, + }); err != nil { + t.Fatal(err) + } + } + + if _, err := client.Logical().ReadWithData( + "auth/"+authType, + map[string][]string{"help": {"1"}}, + ); err != nil { + t.Fatal(err) + } + }) + } + + for _, secretsType := range append( + Registry.Keys(consts.PluginTypeSecrets), + "database", + "cubbyhole", + "identity", + "sys", + ) { + deprecationStatus, _ := Registry.DeprecationStatus(secretsType, consts.PluginTypeSecrets) + if deprecationStatus == consts.Removed { + continue + } + + t.Run("Secrets Engine "+secretsType, func(t *testing.T) { + switch secretsType { + // These three builtin backends are automatically mounted and should not be mounted again + case "cubbyhole": + case "identity": + case "sys": + + default: + if err := client.Sys().Mount(secretsType, &api.MountInput{ + Type: secretsType, + }); err != nil { + t.Fatal(err) + } + } + + if _, err := client.Logical().ReadWithData( + secretsType, + map[string][]string{"help": {"1"}}, + ); err != nil { + t.Fatal(err) + } + }) + } + + t.Run("Secrets Engine kv v2", func(t *testing.T) { + if err := client.Sys().Mount("kv-v2", &api.MountInput{ + Type: "kv", + Options: map[string]string{ + "version": "2", + }, + }); err != nil { + t.Fatal(err) + } + + if _, err := client.Logical().ReadWithData( + "kv-v2", + map[string][]string{"help": {"1"}}, + ); err != nil { + t.Fatal(err) + } + }) + + // This last part is not strictly necessary for original purpose of this test (checking the plugins initialize + // without errors), but whilst we have a test Vault with one of everything mounted, let's also test that the full + // OpenAPI document generation succeeds too. + t.Run("Whole OpenAPI document", func(t *testing.T) { + if _, err := client.Logical().Read("sys/internal/specs/openapi"); err != nil { + t.Fatal(err) + } + }) +} diff --git a/sdk/framework/backend.go b/sdk/framework/backend.go index d1337c659a..4da7d57149 100644 --- a/sdk/framework/backend.go +++ b/sdk/framework/backend.go @@ -486,12 +486,28 @@ func (b *Backend) WriteSafeReplicationState() bool { !replicationState.HasState(consts.ReplicationPerformanceStandby) } +// init runs as a sync.Once function from any plugin entry point which needs to route requests by paths. +// It may panic if a coding error in the plugin is detected. +// For builtin plugins, this is unit tested in helper/builtinplugins/builtinplugins_test.go. +// For other plugins, any unit test that attempts to perform any request to the plugin will exercise these checks. func (b *Backend) init() { b.pathsRe = make([]*regexp.Regexp, len(b.Paths)) for i, p := range b.Paths { + // Detect the coding error of failing to initialise Pattern if len(p.Pattern) == 0 { panic(fmt.Sprintf("Routing pattern cannot be blank")) } + + // Detect the coding error of attempting to define a CreateOperation without defining an ExistenceCheck + if p.ExistenceCheck == nil { + if _, ok := p.Operations[logical.CreateOperation]; ok { + panic(fmt.Sprintf("Pattern %v defines a CreateOperation but no ExistenceCheck", p.Pattern)) + } + if _, ok := p.Callbacks[logical.CreateOperation]; ok { + panic(fmt.Sprintf("Pattern %v defines a CreateOperation but no ExistenceCheck", p.Pattern)) + } + } + // Automatically anchor the pattern if p.Pattern[0] != '^' { p.Pattern = "^" + p.Pattern @@ -499,6 +515,8 @@ func (b *Backend) init() { if p.Pattern[len(p.Pattern)-1] != '$' { p.Pattern = p.Pattern + "$" } + + // Detect the coding error of an invalid Pattern b.pathsRe[i] = regexp.MustCompile(p.Pattern) } } diff --git a/sdk/plugin/mock/path_errors.go b/sdk/plugin/mock/path_errors.go index f5e5b124fc..9c765c67ad 100644 --- a/sdk/plugin/mock/path_errors.go +++ b/sdk/plugin/mock/path_errors.go @@ -36,7 +36,6 @@ func errorPaths(b *backend) []*framework.Path { "err_type": {Type: framework.TypeInt}, }, Callbacks: map[logical.Operation]framework.OperationFunc{ - logical.CreateOperation: b.pathErrorRPCRead, logical.UpdateOperation: b.pathErrorRPCRead, }, }, diff --git a/vault/logical_system_activity_write_testonly.go b/vault/logical_system_activity_write_testonly.go index aa89770928..53ddf02502 100644 --- a/vault/logical_system_activity_write_testonly.go +++ b/vault/logical_system_activity_write_testonly.go @@ -36,7 +36,7 @@ func (b *SystemBackend) activityWritePath() *framework.Path { }, }, Operations: map[logical.Operation]framework.OperationHandler{ - logical.CreateOperation: &framework.PathOperation{ + logical.UpdateOperation: &framework.PathOperation{ Callback: b.handleActivityWriteData, Summary: "Write activity log data", }, diff --git a/vault/logical_system_activity_write_testonly_test.go b/vault/logical_system_activity_write_testonly_test.go index f104d82ad7..03f61e9848 100644 --- a/vault/logical_system_activity_write_testonly_test.go +++ b/vault/logical_system_activity_write_testonly_test.go @@ -38,47 +38,47 @@ func TestSystemBackend_handleActivityWriteData(t *testing.T) { }, { name: "empty write fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, wantError: logical.ErrInvalidRequest, }, { name: "wrong key fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"other": "data"}, wantError: logical.ErrInvalidRequest, }, { name: "incorrectly formatted data fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": "data"}, wantError: logical.ErrInvalidRequest, }, { name: "incorrect json data fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": `{"other":"json"}`}, wantError: logical.ErrInvalidRequest, }, { name: "empty write value fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": `{"write":[],"data":[]}`}, wantError: logical.ErrInvalidRequest, }, { name: "empty data value fails", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": `{"write":["WRITE_PRECOMPUTED_QUERIES"],"data":[]}`}, wantError: logical.ErrInvalidRequest, }, { name: "correctly formatted data succeeds", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": `{"write":["WRITE_PRECOMPUTED_QUERIES"],"data":[{"current_month":true,"all":{"clients":[{"count":5}]}}]}`}, }, { name: "entities with multiple segments", - operation: logical.CreateOperation, + operation: logical.UpdateOperation, input: map[string]interface{}{"input": `{"write":["WRITE_ENTITIES"],"data":[{"current_month":true,"num_segments":3,"all":{"clients":[{"count":5}]}}]}`}, wantPaths: 3, }, diff --git a/vault/logical_system_helpers.go b/vault/logical_system_helpers.go index c702b39ae0..057765a80d 100644 --- a/vault/logical_system_helpers.go +++ b/vault/logical_system_helpers.go @@ -89,6 +89,14 @@ var ( return logical.ErrorResponse("enterprise-only feature"), logical.ErrUnsupportedPath }, } + + // There is a correctness check that verifies there is an ExistenceFunc for all paths that have + // a CreateOperation, so we must define a stub one to pass that check if needed. + if operation == logical.CreateOperation { + path.ExistenceCheck = func(context.Context, *logical.Request, *framework.FieldData) (bool, error) { + return false, nil + } + } } results = append(results, path)