vault

vault copied to clipboard

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.

@maxb is attempting to deploy a commit to the HashiCorp Team on Vercel.

A member of the Team first needs to authorize it.

That concludes the 9 PRs in other plugin repositories that go along with this change.

I do note that nominally the permissions will be different

As far as I know, I don't think there's any way for this to catch out users, because via the HTTP API, there's no way to submit a request which is explicitly a CreateOperation - it's just a POST/PUT, and without an ExistenceCheck to evaluate it, it will always be categorised as an UpdateOperation - therefore the code being removed here has always been inaccessible to users.

Some users will have written ACL policies mentioning the create capability, but since both before and after, there is no way to invoke that operation on the relevant paths, nothing should change from the API user's perspective...

...unless you can see any flaw in the reasoning?

No, you're sharper than I am this morning. I seem to have forgotten to update the existence check for Transit when adding the no-create option.

I was thinking that if an ACL provided Create permissions (with or without an existence check) but not an Update permission, a user would always get a Create operation, but this isn't true; its strictly on the existence check itself.

I do note that nominally the permissions will be different

As far as I know, I don't think there's any way for this to catch out users, because via the HTTP API, there's no way to submit a request which is explicitly a CreateOperation - it's just a POST/PUT, and without an ExistenceCheck to evaluate it, it will always be categorised as an UpdateOperation - therefore the code being removed here has always been inaccessible to users.

Some users will have written ACL policies mentioning the create capability, but since both before and after, there is no way to invoke that operation on the relevant paths, nothing should change from the API user's perspective...

I wonder if adding the ExistenceCheck functions for any path specifying a create operation would be the better way to go here? That change would more than likely affect users having policies like the one you mention above, but would in the end be the correct setup.

Instead of testing for invalid path configurations, we could enforce the ExistenceCheck be specified for any paths having a create operation.

I wonder if adding the ExistenceCheck functions for any path specifying a create operation would be the better way to go here? That change would more than likely affect users having policies like the one you mention above, but would in the end be the correct setup.

I don't agree about it being the correct setup, at least not in the general case, as in many of these, CreateOperations have been specified for paths for which the create operation is semantically meaningless - for example:

• The transit cache-config in this PR, is a "configuration update" operation. I don't see a sufficient semantic difference between "setting the cache size for the first time after mounting the secrets engine" and "setting the cache size again later" to justify mapping those to create vs. update.

• One of the other secrets engines being updated is the KV v2 secrets engine. It has specific paths for invoking the (soft) delete, undelete, and destroy operations on data versions. These have "invoke operation" semantics and should be update only in common with other "invoke operation" endpoints elsewhere in Vault - there is no create-flavoured operation available at these paths.

It may be there are some individual operations within this PR set where separating create and update semantics could potentially be useful, but that would need to be assessed on a case by case basis, taking into account the need to prominently notify users that they may need to update their ACL policies (as access which would previously have been authorized by capabilities = ["update"] could then be forbidden, if it change to require create instead).

As currently written, this PR set aims to avoid all user-facing compatibility issues. Though, it does not rule out further future changes to introduce separated create operations, even to paths being changed in this PR - it merely clears away the existing unreachable code, and sorts out the OpenAPI, leaving a simpler foundation for any future change.

Instead of testing for invalid path configurations, we could enforce the ExistenceCheck be specified for any paths having a create operation.

I don't follow ... I'm already enforcing that in this PR.

I don't follow ... I'm already enforcing that in this PR.

Excellent, I didn't catch that bit.

We're going to take a look at this change in our upcoming team meeting, especially since the change affects numerous backends.

I've added the subtests, and responded on why I think it is appropriate to use panic here.

Would anyone from HashiCorp be able to assist with getting https://github.com/hashicorp/vault-plugin-secrets-terraform/pull/16 merged? That is the last of the 9 prerequisite PRs that is not yet merged.

@maxb https://github.com/hashicorp/vault-plugin-secrets-terraform/pull/16 has been approved and merged. Thanks!

This PR is currently waiting for the version of vault-plugin-secrets-kubernetes in Vault to be upgraded to v0.4.0 or later.

Hi @benashz , @cipherboy ,

Thank you for reviewing this PR in the past. All the related changes have finally made it through the journey of being merged into plugin repositories, and the plugins upgraded in the Vault repo.

This PR is now a candidate to be actually merged, if you're able to give it another look?

@maxb @mpalmi @miagilepner It looks like the activity counters needs to be updated, at least as of this last run date:

2023-05-30T14:57:37.057Z [INFO]  TestRaft_Configuration_Docker.core-0: http2: panic serving 172.17.0.5:59660: Pattern internal/counters/activity/write$ defines a CreateOperation but no ExistenceCheck
goroutine 1011 [running]:
net/http.(*http2serverConn).runHandler.func1()
	/opt/hostedtoolcache/go/1.20.4/x64/src/net/http/h2_bundle.go:6042 +0x145
panic({0x56e7f20, 0xc0023a1890})
	/opt/hostedtoolcache/go/1.20.4/x64/src/runtime/panic.go:884 +0x213
github.com/hashicorp/vault/sdk/framework.(*Backend).init(0xc001de20f0)
	/home/runner/work/vault/vault/sdk/framework/backend.go:484 +0x319
sync.(*Once).doSlow(0xc001536550?, 0x0?)
	/opt/hostedtoolcache/go/1.20.4/x64/src/sync/once.go:74 +0xc2
sync.(*Once).Do(...)
	/opt/hostedtoolcache/go/1.20.4/x64/src/sync/once.go:65
github.com/hashicorp/vault/sdk/framework.(*Backend).HandleExistenceCheck(0xc001de20f0?, {0x7615c90, 0xc001ffe630}, 0xc0012ff500)
	/home/runner/work/vault/vault/sdk/framework/backend.go:150 +0x8c
github.com/hashicorp/vault/vault.(*Router).routeCommon(0xc000ccc3c0, {0x7615c90, 0xc001ffe630}, 0xc0012ff500, 0x1)
	/home/runner/work/vault/vault/vault/router.go:779 +0x1859
github.com/hashicorp/vault/vault.(*Router).RouteExistenceCheck(0xc000ccc3c0?, {0x7615c90?, 0xc001ffe630?}, 0xc002306d24?)
	/home/runner/work/vault/vault/vault/router.go:558 +0x28
github.com/hashicorp/vault/vault.(*Core).CheckToken(0xc000165000, {0x7615c90, 0xc001ffe630}, 0xc0012ff500, 0x1)
	/home/runner/work/vault/vault/vault/request_handling.go:331 +0x60a

It looks like its still a valid test failure:

https://github.com/hashicorp/vault/blob/3ca33976762c3ea250367baf25325c0432bbaaa5/vault/logical_system_activity_write_testonly.go#L29-L45

Maybe it should be moved to UpdateOperation?

Ah, this has eluded the regular testing since it is not compiled unless a build constraint is set. Updated.

Hi @benashz , @ncabatoff ,

Would one (or both!) of you be able to respond to the open question in the thread above https://github.com/hashicorp/vault/pull/18492#pullrequestreview-1224905038, which is AFAIK the only thing holding this back from being merged ?

It concerns the correctness of reporting incorrect coding via panic. I put forward the case that it is fine, because:

• Panic is being used to report a programmer mistake - not a failure condition that may conditionally arise at runtime.

• Panic is already being used to report other kinds of programmer mistake in the same function - i.e. I'm staying in line with existing precedent.

• I have supplied additional test coverage which will ensure that both the existing panics and my new panic will be reliably detected during unit tests.

Thanks!

I think your reasoning on the panic makes perfect sense @maxb and I've spoken with Nick out of band and he agreed.

I think this is ready to merge as others have already approved the code changes.