Allow globbing dis/allowed_policies_glob in token roles #7277
Conversation
|
|
|
Not sure Boolean flags is better than simply having separate config params. One issue with this flag is it necessarily must apply to allowed and disallowed. It's simpler though. |
Separate config params as in having both an allowed_policies and an allowed_policies_glob param for example? If so I did consider this but then it seems unlikely that someone would want to mix the glob and non glob versions in practice (since not using a glob character in a policy would be the same behaviour in both _glob and non-glob fields) and for consistency with other parts of vault (the pki secrets engine uses a flag like this implementation to toggle globbing on hostnames). The only benefit I could see to separate fields would be if someone has a policy with a * character in it already, but that seems unlikely that someone would want to use glob matching while also having glob characters in policy names as there is a very high chance of human error in that instance. That said, we can't change the default behaviour for this reason though, and an explicit opt-in requirement however implemented is safest. |
|
Any update on merging this @jefferai? I notice there's a go race test that failed, but from what I can tell it looks to be unrelated to this PR but happy to fix it up if that's not the case. |
|
Is there anything blocking this getting merged? @catsby? |
|
Any movement on this? Would be useful. |
|
Any update on this? This is one of the oldest PR's now. Due to the time it's been since I opened this I'm not sure if it would cleanly merge into master, but if it's possible to get this merged I'm happy to rebase the PR onto HEAD if needed but I won't bother if it's just going to sit again for years. |
|
I'm also happy to move this to use separate fields rather than a flag to control the behaviour (although I note using a flag matches the pki secrets engine hence using that option originally) if that's what's blocking this from being merged. |
|
Thanks @pmmukh for bringing this one up! This has in fact been sitting for a while, so I'm going to take this back to our engineers and see how the team wants to move forward. Apologies for the delay! |
Happy to do the work to get this merge-able again if there's a way forward to get this merged! Would be super useful to simplify a few workflows in my team. |
|
Hi @nvx! Yeah we've been chatting internally, and I think the current approach of a boolean config to enable globbing makes sense to us, so if you could please update this PR and resolve the conflicts, I'll be happy to give it a review! |
Will do. I just noticed strutil has moved out of this repo and into https://github.com/hashicorp/go-secure-stdlib though. This PR currently adds two funcs StrListSubsetGlob and RemoveGlobs to the strutil package. Should I throw in a PR there to add those funcs first then once that's merged update this PR to leverage it. Or just make some helper funcs to achieve the same task inside the vault/token_store.go file instead for ease of the PR. If the former I wouldn't be able to finish updating this PR until the go-secure-stdlib changes have been merged though, so if it's easy enough to get stuff merged in that repo that seems like the best option. Working under the assumption that getting go-secure-stdlib updated isn't hard I've submitted hashicorp/go-secure-stdlib#8 |
|
Ah, thanks for pointing out this requires changes in https://github.com/hashicorp/go-secure-stdlib! We discussed the process around changes to that repo, and how we'd like to handle this is the former approach you put out, to merge the changes into go-secure-stdlib first, then use that code in this PR. I'll go ahead and give that PR a review, then once merged we can cut a new release on that lib and use that here. |
…ame as allowed_policies and disallowed_policies but allow glob matching.
nvx
force-pushed
the
feature/token-glob-policy
branch
from
f6d7137 to
cedd6c9
Compare
nvx
changed the title
|
This has been updated to be allowed_policies_glob and disallowed_policies_glob. Rebased onto the latest main and squished to a single commit to hide the sausage making. In the end due to having to change some logic to work for both allowed_policies and allowed_policies_glob I no longer need any changes to stdlib now too. |
|
Another thing, could you please update the docs in https://github.com/hashicorp/vault/blob/main/website/content/api-docs/auth/token.mdx as well ? |
|
Ah also, I think this PR needs a changelog added |
|
@pmmukh All done. Let me know if I've missed anything or if anything new pops up! |
…ob token role feature.
nvx
force-pushed
the
feature/token-glob-policy
branch
from
03da5f5 to
285c927
Compare
|
Just fixed up a missing go fmt in the tests. Squashed it into the previous commit for cleanliness. |
There was a problem hiding this comment.
Prat asked me to have a look at this PR as well. Given the sensitive nature of this part of the code, I looked at the changes quite closely and they look good to me. I did not scrutinize the test changes in great detail, I merely satisfied myself that they looked to be testing the right things, and I trust Prat to find any implementation issues with them.
|
Thanks for the 👀 and double checking the PR, nick! |
Based off the existing work in PR #5815 rebased onto latest master.
This adds globbing support to the allowed_policies and disallowed_policies token role fields. This functionality is opt-in, controlled by the role flag allow_glob_policies, to preserve existing behaviour modelled after allow_glob_domains in the pki secrets engine.