CreateOperation should only be implemented alongside ExistenceCheck (#18492)

* CreateOperation should only be implemented alongside ExistenceCheck

Closes #12329

Vault treats all POST or PUT HTTP requests equally - they default to
being treated as UpdateOperations, but, if a backend implements an
ExistenceCheck function, CreateOperations can be separated out when the
existence check returns false.

It follows, then, that if a CreateOperation handler is implemented
without an ExistenceCheck function, this is unreachable code - a coding
error. It's a fairly minor error in the grand scheme of things, but it
causes the generated OpenAPI spec to include x-vault-createSupported for
operations on which create can never actually be invoked - and promotes
muddled understanding of the create/update feature.

In this PR:

1) Implement a new test, which checks all builtin auth methods and
   secrets engines can be successfully initialized. (This is important
   to validate the next part.)

2) Expand upon the existing coding error checks built in to
   framework.Backend, adding a check for this misuse of CreateOperation.

3) Fix up instances of improper CreateOperation within the Vault
   repository - just two, transit and mock.

Note: At this point, the newly added test will **fail**.

There are improper uses of CreateOperation in all of the following:

    vault-plugin-auth-cf
    vault-plugin-auth-kerberos
    vault-plugin-auth-kubernetes
    vault-plugin-secrets-ad
    vault-plugin-secrets-gcpkms
    vault-plugin-secrets-kubernetes
    vault-plugin-secrets-kv
    vault-plugin-secrets-openldap
    vault-plugin-secrets-terraform

each of which needs to be fixed and updated in go.mod here, before this
new check can be added.

* Add subtests

* Add in testing of KV v2, which otherwise doesn't get tested

This is a surprisingly complicated special case

* The database plugin needs special handling as well, and add in help invocations of the builtin backends too

* Fix extra package prefix

* Add changelog

* Update 6 out of 9 plugins to needed new versions

Note, this IS an upgrade despite the apparent version numbers going
down. (That's a consequence of slightly odd release management occurring
in the plugin repositories.)

* Update to deal with code changes since branch originally created

* Perform necessary update of vault-plugin-secrets-kubernetes so that CI checks on PR can run

* Fix another instance of incorrect CreateOperation, for a test-only endpoint

By being hidden behind a Go build constraint, it had evaded notice until
now.

* Add an opportunistic test of sys/internal/specs/openapi too

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,

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
+```

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)
+		}
+	})
+}

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)
 	}
 }

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,
 			},
 		},

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",
 			},

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,
 		},

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)