Allow specifying role-default TTLs in AWS secret engine #5138
Conversation
|
Turns out the test was easier to write than I imagined :) |
There was a problem hiding this comment.
Looks good @joelthompson! Only question I have is whether or not you're setting the 3600s / 1h default in code or not. The only reference I see is in the test, so I assume you're just letting AWS handle the default?
|
Hi @joelthompson , Is this meant specifically for STS? I assume so since for IAM there's already an option, plus there's mount-tuning and system defaults. If so, I think it needs to be renamed to be clear, like, |
|
@Xopherus, it retains the default in the same place in code: https://github.com/hashicorp/vault/pull/5138/files#diff-44c15850f659981333c0677651573294L32 @jefferai, yes, this is only for STS. Will update the parameter name. |
builtin/logical/aws/path_roles.go
Outdated
| @@ -389,6 +406,7 @@ func (r *awsRoleEntry) toResponseData() map[string]interface{} { | |||
| "policy_arns": r.PolicyArns, | |||
| "role_arns": r.RoleArns, | |||
| "policy_document": r.PolicyDocument, | |||
| "default_sts_ttl": r.DefaultSTSTTL / time.Second, | |||
There was a problem hiding this comment.
I have a slight preference for the int64(r.DefaultSTSTTL.Seconds()) flow as that paradigm makes tests nicer -- when analyzing a returned map you get int64 instead of a time.Duration that is actually an integer number of seconds even though time.Duration represents nanoseconds.
There was a problem hiding this comment.
Sure, don't have an opinion either way, was just pattern matching off other places in the code. Done.
The merge of hashicorp#5383 broke the tests due to some changes in the test style that didn't actually cause a git merge conflict. This updates the tests to the new style.
| @@ -61,6 +62,11 @@ will be passed in as the Policy parameter to the AssumeRole or | |||
| GetFederationToken API call, acting as a filter on permissions available.`, | |||
| }, | |||
|
|
|||
| "default_sts_ttl": &framework.FieldSchema{ | |||
There was a problem hiding this comment.
What do you think of naming this just sts_ttl? A value on the role will be used unless specified otherwise. The supplied value can be lesser but not greater and this can be documented.
There was a problem hiding this comment.
Most plugins just call it ttl, so sts_ttl is a better fit I would say. The only exception is databases as far as I can see.
As requested in #387 (comment)
I'm not happy with the lack of tests, I want to refactor the tests a bit to make it easier to more comprehensively test this, still thinking about it, but wanted to open this early to get some feedback.
cc @Xopherus