Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Allow for scoped down group access tokens #147
Allow for scoped down group access tokens #147
Changes from all commits
073ee56
7084dc6
fef2ce9
dd42b0e
e8d24bb
2030e53
417f0cf
2bd224c
5cd6689
73381f1
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 may be reading this wrong, as it has been a while, but If this option is in
pathTokenCreate
, doesn't it apply to all token creations, or in other words, how do we prevent users from using this option, when it is not intended to be available. We can't just allow this for the currentartifactory/token/:roleName
requests. It would have to be a different path (such asartifactory/scopedToken/:roleName
), that would have to be enabled through theartifactory/admin/config
and/or it would have to have some sort ofallowScopeOverride
boolean for the role itself. Another idea would be if the scope on the role was set to some specific value, likeREQUIRE_SCOPE_OVERRIDE
, but that seems a bit janky. Maybe if it was set to admin, then a scope attribute could be required, but that would be a breaking change for anyone that intends to offer an admin token through a role. The case I see in the readme would create an admin token if the requestor did not specify a scope attribute.If we just add this capability, an unsuspecting vault/artifactory admin might update and unknowingly grant everyone that they thought had restricted access by the scope parameter in their role the ability to gain admin access. We can't just change the functionality mid-stream and suddenly require a customized vault policy to be added that filters or disallows the scope attribute, where it had previously just been path based filtering.
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 agree with @TJM.
Since this PR intention is to expand the capability to allow minting of token for groups, why not create a new path for it (similar to user token)? e.g.
artifactory/group_token/:name
, along with corresponding config pathartifactory/config/group_token/:name
?We still need the ample warnings for the
scope
field but the separate path means we won't be breaking existing paths.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.
This is not necessarily just for groups. It allows overriding scope, which is then sent to the API without validation, so it could be for any user, any group or anything else that scope can do. Now, it might be "safer" to require a group name (as a path parameter? like
artifactory/groupToken/:groupName
), which could have some input validation (no commas or spaces?), then pass in a token with group scope interpolated with that group name. That would allow for "groupToken" path to be "enabled" at the /config/admin layer, similar to the userToken stuff we were talking about before.Allowing vault users to override the scope directly could be done as well, although it seems risky to me. It should just not be done on the current
artifactory/token/:roleName
path.EDIT: with input validation, it would be best to just set a very limited set of allowed characters, including a length limit to prevent issues with people URL encoding or other "bypass" mechanisms.
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.
From my read through the code, the user and role/group paths have separate token implementations.
pathUserTokenCreate()
vs.pathTokenCreate()
Please correct me if I am wrong in these assumptions. From what I can discern, there are 2 main paths to request tokens. One for users and one for roles/groups. I proposed this change to the role/group token path as this is described as the CI/automation path. The user path functionality would remain unchanged since those are tied to a specific entity/identity in Vault.
Rather than duplicating the code for the role/group into a new path, would a feasible work around be to add another configuration field to explicitly opt in to scoped role tokens. This would retain the existing behavior during an upgrade. Requests will be validated against Vault policies before they are passed onto Artifactory.
The proposed approach is very similar to how the Vault Github plugin works. There is a single endpoint to get a token and different parameters provide different token capabilities. See https://github.com/martinbaillie/vault-plugin-secrets-github
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.
In terms of validation, you can follow similar validation code from the Artifactory provider: https://github.com/jfrog/terraform-provider-artifactory/blob/master/pkg/artifactory/resource/security/resource_artifactory_scoped_token.go#L185 and https://github.com/jfrog/terraform-provider-artifactory/blob/master/pkg/artifactory/resource/security/resource_artifactory_scoped_token.go#L335
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.
Thanks, I'll work that into my PR.
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.
PR has been updated to allow this to be opt in during plugin configuration.
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.
@alexhung, @TJM any feedback on my updates?
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.
@kkronenb Sorry about the delay. I'll take a look today.
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.
thanks for the comments, I have addressed them
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.
Incorrect indentation. I can fix this before release.