-
Notifications
You must be signed in to change notification settings - Fork 4.5k
[VAULT-35716] Handle lists of values in policy parameters #30551
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
Conversation
ad25ea6 to
487dcd8
Compare
|
CI Results: |
|
Build Results: |
changelog/30551.txt
Outdated
| @@ -0,0 +1,4 @@ | |||
| ```release-note:bug | |||
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.
I'm treating this as a bug though it's unclear if we intended for this to work. Let me know if it fits better as an improvement
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.
Updated to change since it is a difference in behavior that could cause previously passing requests to now fail
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.
Pull Request Overview
This PR fixes the issue where policy parameter checks fail when a parameter value is provided as a list. Key changes include updating the test cases to use a map-based structure for parameters and refactoring the parameter matching logic to correctly handle slice types for both allowed and denied policy assertions.
Reviewed Changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| vault/acl_test.go | Updated test cases to support map-based parameters and additional slice test scenarios. |
| vault/acl.go | Refactored parameter matching functions to distinguish between allowed and denied lists and handle slice types appropriately. |
Files not reviewed (1)
- changelog/30551.txt: Language not supported
| } | ||
|
|
||
| func valueInParameterList(v interface{}, list []interface{}) bool { | ||
| func valueInAllowedParameterList(v interface{}, list []interface{}) bool { |
Copilot
AI
May 14, 2025
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.
[nitpick] The recursive use of valueInParameterList inside valueInAllowedParameterList may cause confusion. Consider renaming or refactoring the helper functions to clearly separate slice handling from single value comparisons.
| } | ||
|
|
||
| func valueInSlice(v interface{}, list []interface{}) bool { | ||
| func valueInDeniedParameterList(v interface{}, list []interface{}) bool { |
Copilot
AI
May 14, 2025
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.
[nitpick] The similar recursion pattern in valueInDeniedParameterList using valueInParameterList could be refactored for improved clarity. Clarifying the distinct handling for slices versus single values would enhance maintainability.
|
Closing for now as making changes to how policies behave will require more in-depth discussions with engineering and product |
Description
As reported in VAULT-35716, policies created with
allowed_parametersordenied_parametersdo not work as expected if the request assigns a list of values to the allowed/denied parameter instead of a single value.eg. with a policy:
a request with parameters
{"name":"test_policy_list","policies": ["admin-policy", "admin-automation-policy"], "type": "external"}'should be denied but is allowed because we do not expectpoliciescould be a list of values and attempt to compare the entirety of it as a single value against*admin-automation-policy*and then against*admin-policy*.AFAICT nothing in our documentation prohibits using a list of values, but we do not handle the case. This PR checks if the input is a slice type, and if so, iterates through it so each value in the slice is being checked against the allow/deny list.
TODO only if you're a HashiCorp employee
backport/label that matches the desired release branch. Note that in the CE repo, the latest release branch will look likebackport/x.x.x, but older release branches will bebackport/ent/x.x.x+ent.of a public function, even if that change is in a CE file, double check that
applying the patch for this PR to the ENT repo and running tests doesn't
break any tests. Sometimes ENT only tests rely on public functions in CE
files.
in the PR description, commit message, or branch name.
description. Also, make sure the changelog is in this PR, not in your ENT PR.