CreateOperation should only be implemented alongside ExistenceCheck #18492

maxb · 2022-12-20T13:31:32Z

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:

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.)
Expand upon the existing coding error checks built in to
framework.Backend, adding a check for this misuse of CreateOperation.
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.

Closes hashicorp#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.

vercel · 2022-12-20T13:31:36Z

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

A member of the Team first needs to authorize it.

maxb · 2022-12-20T14:04:53Z

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

cipherboy

From a crypto side, I'm fine with this change. I do note that nominally the permissions will be different, but am otherwise leaving this and the rest of the changes to Ecosystem's and Core's reviews.

maxb · 2022-12-20T15:07:40Z

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?

cipherboy · 2022-12-20T15:17:14Z

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.

benashz · 2022-12-20T15:24:07Z

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.

maxb · 2022-12-20T15:44:55Z

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.

maxb · 2022-12-20T15:46:59Z

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.

benashz · 2022-12-20T15:58:10Z

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

Excellent, I didn't catch that bit.

benashz · 2022-12-20T16:22:29Z

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

benashz

Looking good. I added some suggestions that should be addressed.

benashz · 2022-12-20T17:50:37Z

sdk/framework/backend.go

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


We shouldn't be panicking in production code. Perhaps the panic should be conditional during testing. Also it would be good to log a warning in all cases.

Whilst I agree that it's a very bad thing to panic as a way to report runtime failure conditions, this code is different:

It's a compile time constant condition - either the Backend paths config has been set up in a way which is considered bad, or not, and if it is triggered, the only fix is to recompile the code.

It's consistent with the two other pre-existing checks that conditionally panic in this function already - the detection of empty patterns, and the detection of patterns which are invalid regexps (by way of the use of MustCompile).

@ncabatoff Are you fine with the panic?

sdk/framework/backend.go

helper/builtinplugins/builtinplugins_test.go

maxb · 2022-12-20T23:09:56Z

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

This is a surprisingly complicated special case

…nvocations of the builtin backends too

) See hashicorp/vault#18492

) * CreateOperation should only be implemented alongside ExistenceCheck See hashicorp/vault#18492 * Remove CreateOperation cases in tests too

) * CreateOperation should only be implemented alongside ExistenceCheck See hashicorp/vault#18492 * Change CreateOperation to UpdateOperation in tests, too

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

) See hashicorp/vault#18492

maxb · 2023-03-09T14:18:05Z

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

) * CreateOperation should only be implemented alongside ExistenceCheck See hashicorp/vault#18492 * A follow-up to removing incorrect CreateOperation usage It turns out the pathRolesWrite function contained dedicated conditionals to deal with CreateOperation, ignoring the fact it never had an ExistenceCheck, so this code path could never be reached! Prune away the unused code, and also update the tests.

fairclothjm · 2023-03-09T15:30:17Z

@maxb hashicorp/vault-plugin-secrets-terraform#16 has been approved and merged. Thanks!

maxb · 2023-04-27T10:04:57Z

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

…I checks on PR can run

maxb · 2023-05-26T09:10:51Z

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?

marcboudreau

Looks good to me from Vault Core's perspective.

cipherboy · 2023-06-29T15:46:22Z

@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:

vault/vault/logical_system_activity_write_testonly.go

Lines 29 to 45 in 3ca3397

    
           		Pattern:         "internal/counters/activity/write$", 
        
           		HelpDescription: helpText, 
        
           		HelpSynopsis:    helpText, 
        
           		Fields: map[string]*framework.FieldSchema{ 
        
           			"input": { 
        
           				Type:        framework.TypeString, 
        
           				Description: "JSON input for generating mock data", 
        
           			}, 
        
           		}, 
        
           		Operations: map[logical.Operation]framework.OperationHandler{ 
        
           			logical.CreateOperation: &framework.PathOperation{ 
        
           				Callback: b.handleActivityWriteData, 
        
           				Summary:  "Write activity log data", 
        
           			}, 
        
           		}, 
        
           	} 
        
           }

Maybe it should be moved to UpdateOperation?

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

maxb · 2023-06-29T16:05:46Z

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

maxb · 2023-07-15T08:11:35Z

Hi @benashz , @ncabatoff ,

Would one (or both!) of you be able to respond to the open question in the thread above #18492 (review), 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!

banks · 2023-07-18T12:43:42Z

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.

maxb requested a review from a team December 20, 2022 13:31

maxb mentioned this pull request Dec 20, 2022

[VAULT-5501] Remove CreateOperation from /transit/cache-config #14919

Closed

cipherboy requested a review from a team December 20, 2022 14:44

cipherboy approved these changes Dec 20, 2022

View reviewed changes

cipherboy requested a review from a team December 20, 2022 14:46

benashz self-requested a review December 20, 2022 17:52

benashz reviewed Dec 20, 2022

View reviewed changes

Add subtests

a15b5d7

maxb added 2 commits December 22, 2022 18:37

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

157ab4b

This is a surprisingly complicated special case

The database plugin needs special handling as well, and add in help i…

8104ce8

…nvocations of the builtin backends too

maxcoulombe pushed a commit to hashicorp/vault-plugin-auth-kerberos that referenced this pull request Jan 19, 2023

CreateOperation should only be implemented alongside ExistenceCheck (#79

f64ecfb

) See hashicorp/vault#18492

maxcoulombe pushed a commit to hashicorp/vault-plugin-secrets-ad that referenced this pull request Jan 19, 2023

CreateOperation should only be implemented alongside ExistenceCheck (#95

9d0ee57

) See hashicorp/vault#18492

maxb added 2 commits January 19, 2023 16:32

Merge remote-tracking branch 'upstream/main' into createoperation

2f73408

Update 6 out of 9 plugins to needed new versions

73b1c5d

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

benashz mentioned this pull request Feb 7, 2023

Vault 1.13 updates hashicorp/vault-plugin-secrets-gcpkms#23

Merged

kpcraig pushed a commit to hashicorp/vault-plugin-secrets-kubernetes that referenced this pull request Feb 8, 2023

CreateOperation should only be implemented alongside ExistenceCheck (#19

077bb00

) See hashicorp/vault#18492

maxb added 2 commits April 27, 2023 10:48

Merge branch 'main' into createoperation

f98ad02

Update to deal with code changes since branch originally created

2b62f40

maxb added 2 commits April 27, 2023 11:07

Perform necessary update of vault-plugin-secrets-kubernetes so that C…

dfa5250

…I checks on PR can run

Merge branch 'main' into createoperation

df74b05

marcboudreau approved these changes May 26, 2023

View reviewed changes

maxb added 3 commits June 29, 2023 16:51

Merge branch 'main' into createoperation

b801719

Fix another instance of incorrect CreateOperation, for a test-only en…

ce12136

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

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

5d18f82

Merge branch 'main' into createoperation

c9c7fa5

banks merged commit 00e13ab into hashicorp:main Jul 18, 2023
84 of 88 checks passed

maxb deleted the createoperation branch July 18, 2023 12:56

alexhung mentioned this pull request Jul 26, 2023

build(deps): bump github.com/hashicorp/vault/sdk from 0.9.1 to 0.9.2 jfrog/vault-plugin-secrets-artifactory#117

Closed

maxb mentioned this pull request Aug 3, 2023

test/sign defines a CreateOperation but no ExistenceCheck #22173

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CreateOperation should only be implemented alongside ExistenceCheck #18492

CreateOperation should only be implemented alongside ExistenceCheck #18492

maxb commented Dec 20, 2022

vercel bot commented Dec 20, 2022

maxb commented Dec 20, 2022

cipherboy left a comment

maxb commented Dec 20, 2022

cipherboy commented Dec 20, 2022

benashz commented Dec 20, 2022

maxb commented Dec 20, 2022

maxb commented Dec 20, 2022

benashz commented Dec 20, 2022

benashz commented Dec 20, 2022

benashz left a comment

benashz Dec 20, 2022

maxb Dec 20, 2022

cipherboy Jun 29, 2023

maxb commented Dec 20, 2022

maxb commented Mar 9, 2023

fairclothjm commented Mar 9, 2023

maxb commented Apr 27, 2023

maxb commented May 26, 2023

marcboudreau left a comment

cipherboy commented Jun 29, 2023

maxb commented Jun 29, 2023

maxb commented Jul 15, 2023

banks commented Jul 18, 2023

CreateOperation should only be implemented alongside ExistenceCheck #18492

CreateOperation should only be implemented alongside ExistenceCheck #18492

Conversation

maxb commented Dec 20, 2022

vercel bot commented Dec 20, 2022

maxb commented Dec 20, 2022

cipherboy left a comment

Choose a reason for hiding this comment

maxb commented Dec 20, 2022

cipherboy commented Dec 20, 2022

benashz commented Dec 20, 2022

maxb commented Dec 20, 2022

maxb commented Dec 20, 2022

benashz commented Dec 20, 2022

benashz commented Dec 20, 2022

benashz left a comment

Choose a reason for hiding this comment

benashz Dec 20, 2022

Choose a reason for hiding this comment

maxb Dec 20, 2022

Choose a reason for hiding this comment

cipherboy Jun 29, 2023

Choose a reason for hiding this comment

maxb commented Dec 20, 2022

maxb commented Mar 9, 2023

fairclothjm commented Mar 9, 2023

maxb commented Apr 27, 2023

maxb commented May 26, 2023

marcboudreau left a comment

Choose a reason for hiding this comment

cipherboy commented Jun 29, 2023

maxb commented Jun 29, 2023

maxb commented Jul 15, 2023

banks commented Jul 18, 2023