Update genUsername to cap STS usernames at 32 chars #12185
Conversation
vinay-gopalan
requested review from
calvn,
pcman312,
jasonodonnell and
fairclothjm
There was a problem hiding this comment.
It looks like for STS federated tokens, the name is capped at 32 chars according to their docs, so we should keep the existing default template.
| } | ||
| case "sts": | ||
| // Capped at 32 chars, which leaves only a couple of characters to play | ||
| // with, so don't insert display name or policy name at all | ||
| // Capped at 32 chars | ||
| up, err := template.NewTemplate(template.Template(usernameTemplate)) | ||
| if err != nil { | ||
| return "", "", fmt.Errorf("unable to initialize username template: %w", err) |
There was a problem hiding this comment.
I can't drop comments past here, but on L84 can you add a 32 char length check, similar to what we have in?
@pcman312 mentioned offline that returning an error if custom username templates is above character limit is an option, which I think is okay to do.
There was a problem hiding this comment.
@calvn I can do that, but also wanted to mention that we only ever use the defaultSTSTemplate when generating usernames (which is always capped at 32 by default). Since the length limits are different and the feature ask was specifically for custom IAM user/role names, currently STS names are never generated by a custom user template.
There was a problem hiding this comment.
currently STS names are never generated by a custom user template
Is there a technical reason for this? If not, this feels like a deficiency in the feature.
There was a problem hiding this comment.
I'm not sure if this is a strong enough technical reason, but my understanding is since STS is capped at 32 and IAM is capped at 64, we would have to add another STS template field in the AWS config to give users the ability to set custom templates for both. Otherwise if we expect the same template to work for both the cases, then that template would need to be capped at the minimum length cap and generate both STS and IAM usernames at 32 chars.
At the time of writing this feature the ticket's ask was for IAM, so I didn't add a separate STS custom username field, but should that also be in the scope here?
There was a problem hiding this comment.
I didn't add a separate STS custom username field, but should that also be in the scope here?
We should be able to re-use the same username_template parameter for both (actually all three workflows). genUsername is already doing the heavy lifting under the hood. So, the behavior on what's the cap on a specific workflow can be conditionally determined by the userType that's provided to the func call, i.e. within their own specific switch case.
There was a problem hiding this comment.
Having the template at the config level but the credential type at the role level does make this tricky. Off hand I see two options:
- Have a single template that takes in the credential type as an argument to the template. This would allow the template to choose the max length based on the credential type
- Specify one template for each credential type
I'm not sure I have a strong opinion either way. I don't really like either option all that much, but I also don't dislike either one. Either way we choose to go, I think we should consider other possible use cases similar to this and make sure we're consistent.
There was a problem hiding this comment.
Option 1 is an interesting take. I assume it takes advantage of conditional within the template based on that value, i.e. ... {{if eq .CredType "sts"}} ... {{else}} ... {{end}}?
I like it from an API cleanliness perspective, but it makes crafting a custom username_template that works for both a bit trickier.
There was a problem hiding this comment.
Yea, that's the general idea. It makes things simpler and more complicated at the same time because a user would need to know that a custom template needs to handle all three cases.
There was a problem hiding this comment.
If we can craft our default to use this pattern and include an example in the docs, I don't mind going down this route. If user feedback is that it's too complex we can re-visit and add a separate param on the API endpoint.
vinay-gopalan
changed the title
| return "", "", fmt.Errorf("failed to generate username: %w", err) | ||
| return "", fmt.Errorf("failed to generate username: %w", err) | ||
| } | ||
| // To prevent template from exceeding STS length limits |
There was a problem hiding this comment.
Nit (same above):
| // To prevent template from exceeding STS length limits | |
| // To prevent a custom template from exceeding STS length limits |
There was a problem hiding this comment.
Ah good call! Updated
This PR updates the
genUsernamefunction in the AWS secrets engine and updates it to cap STS usernames at 64 characters instead of 32. The PR also refactors the unit tests to reflect the updates and converts them to table tests.Follow-up to #12066