jfrog / vault-plugin-secrets-artifactory

HashiCorp Vault Secrets Plugin for Artifactory
https://jfrog.com
Apache License 2.0
39 stars 20 forks source link

Add user_token path #113

Closed davidcorrigan714 closed 1 year ago

davidcorrigan714 commented 1 year ago

Add the artifactory/user_token/<user-name> path to support users obtaining tokens for themselves, or others I suppose if that's easier than configuring an endpoint for whatever reason. For example we use AzureAD for both Vault and Artifactory, so devs can run our internal tools which will login to Vault using SSO, get Artifactory tokens, and install them into various tool-specific locations on their machines all programmatically. An example policy for using this looks like:

path "artifactory/user_token/{{identity.entity.aliases.azure-ad-oidc.metadata.upn}}" {
  capabilities = [ "read" ]
}

I've added a few more properties to the artifactory/config endpoint to support this: user_tokens_audience, user_tokens_default_ttl, and user_tokens_max_ttl.

Got a few TODOs still, namely tests, but putting this up for initial feedback while I work on those.

github-actions[bot] commented 1 year ago

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

davidcorrigan714 commented 1 year ago

I have read the CLA Document and I hereby sign the CLA

alexhung commented 1 year ago

@davidcorrigan714 Let me know when this PR is ready to be reviewed again. (or just click on the "Re-request review" button 😄 )

davidcorrigan714 commented 1 year ago

Should be ready in another few days. Planning to add some notes on the TTL details, and a few more tests for the config path changes. I didn't see any tests for the TTL logic, which seemed a tad tedious so I'll at least triple-check that with some manual testing, or add unit tests if I can figure out how - Go is not a language I use all that often.

davidcorrigan714 commented 1 year ago

Going through the TTL tests, I'm trying to figure out what the max_ttl does on the token path. Really doesn't seem to be read or used anywhere

TJM commented 1 year ago

Going through the TTL tests, I'm trying to figure out what the max_ttl does on the token path. Really doesn't seem to be read or used anywhere

The max_ttl refers to vault leases, specifically How long you can renew the lease for. For example, let's say we want to have a token that lasts 2 hours (default_ttl) but can be renewed for up to 24h or even 7 days.

davidcorrigan714 commented 1 year ago

Going through the TTL tests, I'm trying to figure out what the max_ttl does on the token path. Really doesn't seem to be read or used anywhere

The max_ttl refers to vault leases, specifically How long you can renew the lease for. For example, let's say we want to have a token that lasts 2 hours (default_ttl) but can be renewed for up to 24h or even 7 days.

Right, and in this case I just don't see how it's being used when it's passed in as a request parameter to this specific path. I see it checking the system max lease time, the role max lease time, but not the max_ttl on the token path request. I'd expect to see data.GetOk("ttl_max") somewhere in path_token_create.go unless the Vault framework is handling that particular argument internally somewhere.

davidcorrigan714 commented 1 year ago

The argument descriptions don't read right to me either. There's the system max lease time determined by b.Backend.System().MaxLeaseTTL() and the role max lease time determined by role configs, but I don't actually see a backend max_ttl parameter on the config endpoint. There's another spot in the unit tests where it seems like someone assumed that exists but it didn't work there for me either.

davidcorrigan714 commented 1 year ago

oh mount tuning is supposed to affect the backend ttl, guess that'd be reflected in b.Backend.System().MaxLeaseTTL()

davidcorrigan714 commented 1 year ago

On the artifactory/token/\<role> endpoint I confirmed that it is respecting the max_ttl at the system, mount, and role level, but not for token requests, see:

image

I don't see that parameter in the README anywhere, and it doesn't work. Could go either way on implementing it for this new endpoint. Going to poke a bit and see if it's possible to write tests for these cases too, just not sure if the test fixtures can update the system level max_ttl values.