Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Skip Formatting For NIL Secret #18163

Merged
merged 5 commits into from
Dec 1, 2022
Merged

Skip Formatting For NIL Secret #18163

merged 5 commits into from
Dec 1, 2022

Conversation

ltcarbonell
Copy link
Contributor

@ltcarbonell ltcarbonell commented Nov 30, 2022

Vault is currently panicking when trying to patch with the -field parameter set. This occurs because we are expecting there to be metadata around the secret, but none is getting returned due to the flag being set. A check has been added to ensure that the secret is populated before trying to access the Data from underneath it.

Previous Error:

$ vault kv patch -field=version  -method=rw -mount=kv /secrets test=- <<< "new"
4
= Secret Path =
kv/data/secrets

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x30 pc=0x103aeab1c]

goroutine 1 [running]:
github.com/hashicorp/vault/command.(*KVPatchCommand).Run(0x14000aca440, {0x140001a0030, 0x5, 0x5})
	/Users/runner/work/vault/vault/command/kv_patch.go:232 +0x3fc
github.com/mitchellh/cli.(*CLI).Run(0x1400062edc0)
	/Users/runner/go/pkg/mod/github.com/mitchellh/[email protected]/cli.go:262 +0x4a8
github.com/hashicorp/vault/command.RunCustom({0x140001a0010?, 0x7?, 0x7?}, 0x140000021a0?)
 	/Users/runner/work/vault/vault/command/main.go:237 +0x930
github.com/hashicorp/vault/command.Run(...)
	/Users/runner/work/vault/vault/command/main.go:142
main.main()
	/Users/runner/work/vault/vault/main.go:10 +0x54

After change:

$ vault kv patch -field=version  -method=rw -mount=kv /secrets test=- <<< "new2"     
10
= Secret Path =
kv/data/secrets

UPDATE:
updated output

$ vault kv put secret/test foo=bar3               
== Secret Path ==
secret/data/test
 
======= Metadata =======
Key                Value
---                -----
created_time       2022-12-01T16:03:45.398495Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            8

$ vault kv put -field=version secret/test foo=bar3
9

$ vault kv patch -field=version secret/test foo=bar4
10

$ vault kv patch  secret/test foo=bar4 
== Secret Path ==
secret/data/test

======= Metadata =======
Key                Value
---                -----
created_time       2022-12-01T16:04:59.014316Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            11

@ltcarbonell ltcarbonell changed the title Skip formatting for a nil secret data Skip Formatting For NIL Secret Nov 30, 2022
Copy link
Contributor

@averche averche left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@davidadeleon davidadeleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this does address the panic, I do wonder if we want to continue displaying the secret path helper text when the field flag is provided? The behavior from this change doesn't match the behavior of other KV commands like kv get or kv put where we don't print that information.

Ex:

❯ vault kv put secret/test foo=bar3
== Secret Path ==
secret/data/test

======= Metadata =======
Key                Value
---                -----
created_time       2022-11-30T19:22:20.217002Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            5
❯ vault kv put -field=version secret/test foo=bar4
6

@ltcarbonell
Copy link
Contributor Author

While this does address the panic, I do wonder if we want to continue displaying the secret path helper text when the field flag is provided? The behavior from this change doesn't match the behavior of other KV commands like kv get or kv put where we don't print that information.

Ex:

❯ vault kv put secret/test foo=bar3
== Secret Path ==
secret/data/test

======= Metadata =======
Key                Value
---                -----
created_time       2022-11-30T19:22:20.217002Z
custom_metadata    <nil>
deletion_time      n/a
destroyed          false
version            5
❯ vault kv put -field=version secret/test foo=bar4
6

Thats a good point. Especially if the expectation is that this result will be piped into other commands, the extra information might do more harm than good. I will update.

Copy link
Contributor

@davidadeleon davidadeleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ltcarbonell ltcarbonell merged commit 06b4def into main Dec 1, 2022
AnPucel pushed a commit that referenced this pull request Jan 14, 2023
* Skip formatting for a nil secret data

* Same change for put

* Add changelog

* update changelog

* modify filtered output
@ltcarbonell ltcarbonell deleted the kv2-patch-cli-panic branch September 25, 2024 21:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants