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

Fix list on password policies to list the policies containing slashes #23155

Merged
merged 5 commits into from
Sep 19, 2023

Conversation

akshya96
Copy link
Contributor

Issue: #17117
Slack conversation: https://hashicorp.slack.com/archives/C0287E435NE/p1694714032988359
Jira: https://hashicorp.atlassian.net/browse/VAULT-20052

Vault allows us to create password policies with "/" similar to other policy types like acl
Example: vault write sys/policies/password/example/example1 policy=@example_policy.hcl is allowed

but the list on password policies does not list the values after "/"
Example response: {"request_id":"9b1eadda-0511-02e0-f17f-7b9fe1d2c8b7","lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":["example/"]},"wrap_info":null,"warnings":null,"auth":null}

Making this work consistently with acl policies. This fix produces the following response:
{"request_id":"9b1eadda-0511-02e0-f17f-7b9fe1d2c8b7","lease_id":"","renewable":false,"lease_duration":0,"data":{"keys":["example/example1"]},"wrap_info":null,"warnings":null,"auth":null}

@akshya96 akshya96 marked this pull request as ready for review September 18, 2023 22:10
@github-actions github-actions bot added the hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed label Sep 18, 2023
@github-actions
Copy link

Build Results:
All builds succeeded! ✅

@github-actions
Copy link

github-actions bot commented Sep 18, 2023

CI Results:
All Go tests succeeded! ✅

@akshya96 akshya96 requested review from a team and hc-github-team-secure-vault-core and removed request for a team September 18, 2023 22:28
@akshya96 akshya96 modified the milestones: 1.15.0, 1.15.1 Sep 18, 2023
Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple of things I'd like to see before we merge this.

vault/logical_system.go Outdated Show resolved Hide resolved
@@ -4739,6 +4739,26 @@ func TestHandlePoliciesPasswordList(t *testing.T) {
},
},
},
"policy with /": {
Copy link
Contributor

Choose a reason for hiding this comment

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

It'd be nice to see a test with another level of paths, similar to the example included in the associated issue (path/to/policy).

Copy link
Contributor

Choose a reason for hiding this comment

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

Also curious to see what happens if our policy ends in /.

Copy link
Contributor Author

@akshya96 akshya96 Sep 19, 2023

Choose a reason for hiding this comment

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

Added tests for these two cases listed above

@akshya96 akshya96 requested a review from mpalmi September 19, 2023 18:43
Copy link
Contributor

@mpalmi mpalmi left a comment

Choose a reason for hiding this comment

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

Looks great!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hashicorp-contributed-pr If the PR is HashiCorp (i.e. not-community) contributed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants