-
Notifications
You must be signed in to change notification settings - Fork 32
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
Fix panic on renewing a renewable KV v1 secret #118
base: main
Are you sure you want to change the base?
Conversation
A regression was introduced in hashicorp#17, in 2018. Better late than never, let's fix it. This will open the door to considering deleting the second copy of the KV v1 source code within the Vault repo, and just referring to this as the canonical copy. The fix selectively reverts some parts of hashicorp#17, renames `handleRead` to `handleReadOrRenew`, and adds some detailed comments to explain the subtlety.
// with leases switched on or off | ||
func LeaseSwitchedPassthroughBackend(ctx context.Context, conf *logical.BackendConfig, leases bool) (logical.Backend, error) { | ||
func LeaseSwitchedPassthroughBackendFactory(ctx context.Context, conf *logical.BackendConfig, leases bool) (logical.Backend, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewer: The changes above are just an opportunistic cleanup whilst I was here ... one of the three factory functions was inconsistently missing the Factory
suffix.
Compatibility: Should not really be an issue, no-one other than Vault should be importing this as a library and Vault doesn't (yet) call these.
InternalData: map[string]interface{}{ | ||
"secret_type": "kv", | ||
}, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note to reviewer: This is a bit fake, hacking together just enough of what a real RenewOperation request would look like. Ideally I'd like to run a real Core and generate a real lease renewal, but the relevant test helpers are not exposed for out-of-tree plugins to use. This is probably good enough, I guess.
Hi @hsimon-hashicorp ... it's me again. I wonder if you'd be able to help get the right eyes on this one too? I fear it's a bit out of the way, in a rather quiet plugin repository - but it would be great to get it merged to unlock additional cleanups in the main Vault repository. |
@maxb Hey Max, thanks for the contribution. Would you mind resolving the current merge conflicts? I'll take a look at the PR, but I'm not super familiar and don't have all of the context. I can try to help get more eyes on it. |
Thank you @raymonstah! I missed the original ping, and I'll see if we can get some eyes on it too. |
Thanks! In essence, the context is this:
|
A regression was introduced in #17, in 2018. Better late than never,
let's fix it. This will open the door to considering deleting the second
copy of the KV v1 source code within the Vault repo, and just referring
to this as the canonical copy.
The fix selectively reverts some parts of #17, renames
handleRead
tohandleReadOrRenew
, and adds some detailed comments to explain thesubtlety.