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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Feb 28, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9b5c2c7
Add warning when generate_lease=no_store=true
cipherboy Feb 25, 2022
f7efac1
Add changelog entry
cipherboy Feb 28, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion builtin/logical/pki/path_roles.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -583,6 +583,7 @@ func (b *backend) pathRoleList(ctx context.Context, req *logical.Request, d *fra

func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data *framework.FieldData) (*logical.Response, error) {
var err error
var resp *logical.Response
name := data.Get("name").(string)

entry := &roleEntry{
Expand Down Expand Up @@ -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.

resp = &logical.Response{}
resp.AddWarning("mutually exclusive values no_store=true and generate_lease=true were both specified; no_store=true takes priority")
}
} else {
*entry.GenerateLease = data.Get("generate_lease").(bool)
}
Expand Down Expand Up @@ -694,7 +699,7 @@ func (b *backend) pathRoleCreate(ctx context.Context, req *logical.Request, data
return nil, err
}

return nil, nil
return resp, nil
}

func parseKeyUsages(input []string) int {
Expand Down