Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Update genUsername to cap STS usernames at 32 chars #12185

Merged
merged 12 commits into from Aug 9, 2021
Merged

Update genUsername to cap STS usernames at 32 chars #12185

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
e530b13
update genUsername to cap STS usernames at 64 chars
vinay-gopalan Jul 27, 2021
8e78bd9
add changelog
vinay-gopalan Jul 27, 2021
c1af459
refactor tests into t.Run block
vinay-gopalan Jul 27, 2021
76cd008
patch: remove warningExpected bool and include expected string
vinay-gopalan Jul 27, 2021
db3152a
patch: revert sts to cap at 32 chars and add assume_role case in genU…
vinay-gopalan Jul 27, 2021
11dc5ce
update changelog
vinay-gopalan Jul 27, 2021
852b239
update genUsername to return error if username generated exceeds leng…
vinay-gopalan Jul 28, 2021
7cba4b0
update changelog
vinay-gopalan Jul 28, 2021
c68bfe0
add conditional default username template to provide custom STS usern…
vinay-gopalan Aug 6, 2021
0dd2dfa
update changelog
vinay-gopalan Aug 6, 2021
212711b
include test for failing STS length case
vinay-gopalan Aug 6, 2021
c220b93
update comments for more clarity
vinay-gopalan Aug 6, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 5 additions & 7 deletions builtin/logical/aws/secret_access_keys.go
View file
Open in desktop
Expand Up @@ -49,9 +49,8 @@ func secretAccessKeys(b *backend) *framework.Secret {

func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, warning string, err error) {
switch userType {
case "iam_user":
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
// IAM users are capped at 64 chars; this leaves, after the beginning and
// end added below, 42 chars to play with.
case "iam_user", "assume_role":
// IAM users are capped at 64 chars
up, err := template.NewTemplate(template.Template(usernameTemplate))
if err != nil {
return "", "", fmt.Errorf("unable to initialize username template: %w", err)
Expand All @@ -69,11 +68,10 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re
// To prevent template from exceeding IAM length limits
if len(ret) > 64 {
ret = ret[0:64]
warning = "the calling token display name/IAM policy name were truncated to 64 characters to fit within IAM username length limits"
warning = fmt.Sprintf("the calling token's %s user name was truncated to 64 characters to fit within username length limits", userType)
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
}
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
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
up, err := template.NewTemplate(template.Template(usernameTemplate))
if err != nil {
return "", "", fmt.Errorf("unable to initialize username template: %w", err)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can't drop comments past here, but on L84 can you add a 32 char length check, similar to what we have in?

vault/builtin/logical/aws/secret_access_keys.go

Line 70 in 86c1cdf

if len(ret) > 64 {

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. 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
  2. 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.

Copy link
Member

@calvn calvn Jul 29, 2021 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@calvn @pcman312 I have added a template that generates usernames of differing lengths based on a conditional template and updated the tests till they all pass. Let me know what you think!

Expand Down Expand Up @@ -205,7 +203,7 @@ func (b *backend) assumeRole(ctx context.Context, s logical.Storage,
roleSessionNameWarning := ""
var roleSessionNameError error
if roleSessionName == "" {
roleSessionName, roleSessionNameWarning, roleSessionNameError = genUsername(displayName, roleName, "iam_user", usernameTemplate)
roleSessionName, roleSessionNameWarning, roleSessionNameError = genUsername(displayName, roleName, "assume_role", usernameTemplate)
// Send a 400 to Framework.OperationFunc Handler
if roleSessionNameError != nil {
return nil, roleSessionNameError
Expand Down
108 changes: 61 additions & 47 deletions builtin/logical/aws/secret_access_keys_test.go
View file
Open in desktop
Expand Up @@ -47,53 +47,67 @@ func TestNormalizeDisplayName_NormNotRequired(t *testing.T) {
}

func TestGenUsername(t *testing.T) {

testUsername, warning, err := genUsername("name1", "policy1", "iam_user", `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`)
if err != nil {
t.Fatalf(
"expected no err; got %s",
err,
)
}

expectedUsernameRegex := `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`
require.Regexp(t, expectedUsernameRegex, testUsername)
// IAM usernames are capped at 64 characters
if len(testUsername) > 64 {
t.Fatalf(
"expected IAM username to be of length 64, got %d",
len(testUsername),
)
}

testUsername, warning, err = genUsername(
"this---is---a---very---long---name",
"long------policy------name",
"iam_user",
`{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`,
)

if warning == "" || !strings.Contains(warning, "calling token display name/IAM policy name were truncated to 64 characters") {
t.Fatalf("expected a truncate warning; received empty string")
}
if len(testUsername) != 64 {
t.Fatalf("expected a username cap at 64 chars; got length: %d", len(testUsername))
}

testUsername, warning, err = genUsername("name1", "policy1", "sts", defaultSTSTemplate)
if strings.Contains(testUsername, "name1") || strings.Contains(testUsername, "policy1") {
t.Fatalf(
"expected sts username to not contain display name or policy name; got %s",
testUsername,
)
}
// STS usernames are capped at 64 characters
if len(testUsername) > 32 {
t.Fatalf(
"expected sts username to be under 32 chars; got %s of length %d",
testUsername,
len(testUsername),
)
type testCase struct {
name string
policy string
userType string
UsernameTemplate string
warningExpected string
expectedRegex string
expectedLength int
}

tests := map[string]testCase{
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
"Truncated to 64. No warnings expected": {
name: "name1",
policy: "policy1",
userType: "iam_user",
UsernameTemplate: `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`,
warningExpected: "",
expectedRegex: `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`,
expectedLength: 64,
},
"Truncated to 32. No warnings expected": {
name: "name1",
policy: "policy1",
userType: "sts",
UsernameTemplate: `{{ printf "vault-%s-%s" (unix_time) (random 20) | truncate 32 }}`,
warningExpected: "",
expectedRegex: `^vault-[0-9]+-[a-zA-Z0-9]+`,
expectedLength: 32,
},
"Too long. Warning expected": {
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
name: "this---is---a---very---long---name",
policy: "long------policy------name",
userType: "assume_role",
UsernameTemplate: `{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`,
warningExpected: "calling token's assume_role user name was truncated to 64 characters",
expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`,
expectedLength: 64,
},
}

for testDescription, testCase := range tests {
t.Run(testDescription, func(t *testing.T) {
testUsername, warning, err := genUsername(testCase.name, testCase.policy, testCase.userType, testCase.UsernameTemplate)
if err != nil {
t.Fatalf("expected no err; got %s", err)
}

expectedUsernameRegex := testCase.expectedRegex
require.Regexp(t, expectedUsernameRegex, testUsername)
if len(testUsername) > testCase.expectedLength {
t.Fatalf("expected username to be of length 64, got %d", len(testUsername))
}

if !strings.Contains(warning, testCase.warningExpected) {
t.Fatalf("expected a truncate warning %s; received %s", testCase.warningExpected, warning)
}

if len(warning) > 0 && len(testUsername) != testCase.expectedLength {
t.Fatalf("expected a username cap at %d chars; got length: %d", testCase.expectedLength, len(testUsername))
}
})
}
}

Expand Down
3 changes: 3 additions & 0 deletions changelog/12185.txt
View file
Open in desktop
@@ -0,0 +1,3 @@
```release-note:improvement
secrets/aws: Refactor genUsername tests into table tests
vinay-gopalan marked this conversation as resolved.
Show resolved Hide resolved
```