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

Add warning when generate_lease=no_store=true when writing PKI role #14292

Merged

Conversation

cipherboy
Copy link
Contributor

@cipherboy cipherboy commented Feb 25, 2022

When no_store=true, the value of generate_lease is ignored completely
(and set to false). This means that when generate_lease=true is
specified by the caller of the API, it is silently swallowed. While
changing the behavior could break callers, setting a warning on the
response (changing from a 204->200 in the process) seems to make the
most sense.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>


This probably isn't that important (and can wait for 1.10 branching). I found it when looking at the docs around #14286 and thought it was interesting.

Can be tested with something like:

source devvault && vault secrets enable pki
vault write /pki/roles/example-com generate_lease=true no_store=true

When no_store=true, the value of generate_lease is ignored completely
(and set to false). This means that when generate_lease=true is
specified by the caller of the API, it is silently swallowed. While
changing the behavior could break callers, setting a warning on the
response (changing from a 204->200 in the process) seems to make the
most sense.

Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
Copy link
Contributor

@stevendpclark stevendpclark left a comment

Choose a reason for hiding this comment

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

lgtm!

@cipherboy cipherboy added this to the 1.11 milestone Feb 28, 2022
Signed-off-by: Alexander Scheel <alex.scheel@hashicorp.com>
@vercel vercel bot temporarily deployed to Preview – vault-storybook February 28, 2022 13:34 Inactive
@vercel vercel bot temporarily deployed to Preview – vault February 28, 2022 13:34 Inactive
@cipherboy
Copy link
Contributor Author

Thanks for the review @stevendpclark!

@cipherboy cipherboy merged commit 021570e into main Feb 28, 2022
@@ -644,6 +645,10 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data
// no_store implies generate_lease := false
if entry.NoStore {
*entry.GenerateLease = false
if data.Get("generate_lease").(bool) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it make sense to return an error when both options are set to true?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The docs have mentioned since the introduction of the no_store option that no_store=true sets generate_lease=false (regardless of its value), so I don't think we can necessarily make a breaking change at this point in time (which erring out would be). However, emitting a warning is the next best thing we can do for the time being. :-)

A future Vault v2 version potentially could make that change, or perhaps an equivalent amount of time with the warning. I dunno. But at least as it stands, we'd risk breaking people's existing e.g., Terraform definitions if we made that change now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The nice thing is a quick search shows that no public Terraform users seem to have made this mistake, but some use variables that could potentially be overridden to have the undesired behavior. Plus, I'd venture that most Terraform users keep their stuff private.

@cipherboy cipherboy deleted the cipherboy-add-warning-no-store-generate-lease-both-true branch March 7, 2022 14:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants