From e530b1354cfef520214a1398e15660154a17d834 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 13:58:36 -0700 Subject: [PATCH 01/12] update genUsername to cap STS usernames at 64 chars --- builtin/logical/aws/secret_access_keys.go | 55 ++++------- .../logical/aws/secret_access_keys_test.go | 96 ++++++++++--------- 2 files changed, 70 insertions(+), 81 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 89fc29c525ebb..1b7fcdb0783a7 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -20,7 +20,6 @@ import ( const ( secretAccessKeyType = "access_keys" storageKey = "config/root" - defaultSTSTemplate = `{{ printf "vault-%d-%d" (unix_time) (random 20) | truncate 32 }}` ) func secretAccessKeys(b *backend) *framework.Secret { @@ -48,42 +47,26 @@ func secretAccessKeys(b *backend) *framework.Secret { } func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, warning string, err error) { - switch userType { - case "iam_user": - // IAM users are capped at 64 chars; this leaves, after the beginning and - // end added below, 42 chars to play with. - up, err := template.NewTemplate(template.Template(usernameTemplate)) - if err != nil { - return "", "", fmt.Errorf("unable to initialize username template: %w", err) - } - - um := UsernameMetadata{ - DisplayName: normalizeDisplayName(displayName), - PolicyName: normalizeDisplayName(policyName), - } + // IAM and STS usernames are capped at 64 chars; this leaves, after the beginning and + // end added below, 42 chars to play with. + up, err := template.NewTemplate(template.Template(usernameTemplate)) + if err != nil { + return "", "", fmt.Errorf("unable to initialize username template: %w", err) + } - ret, err = up.Generate(um) - if err != nil { - return "", "", fmt.Errorf("failed to generate username: %w", err) - } - // 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" - } - 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 - up, err := template.NewTemplate(template.Template(usernameTemplate)) - if err != nil { - return "", "", fmt.Errorf("unable to initialize username template: %w", err) - } + um := UsernameMetadata{ + DisplayName: normalizeDisplayName(displayName), + PolicyName: normalizeDisplayName(policyName), + } - um := UsernameMetadata{} - ret, err = up.Generate(um) - if err != nil { - return "", "", fmt.Errorf("failed to generate username: %w", err) - } + ret, err = up.Generate(um) + if err != nil { + return "", "", fmt.Errorf("failed to generate username: %w", err) + } + // To prevent template from exceeding IAM/STS length limits + if len(ret) > 64 { + ret = ret[0:64] + warning = fmt.Sprintf("the calling token's %s user name was truncated to 64 characters to fit within username length limits", userType) } return } @@ -112,7 +95,7 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, return logical.ErrorResponse(err.Error()), nil } - username, usernameWarning, usernameError := genUsername(displayName, policyName, "sts", defaultSTSTemplate) + username, usernameWarning, usernameError := genUsername(displayName, policyName, "sts", defaultUserNameTemplate) // Send a 400 to Framework.OperationFunc Handler if usernameError != nil { return nil, usernameError diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index b9dce2514bb91..b4544f46010cc 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -2,6 +2,7 @@ package aws import ( "context" + "fmt" "strings" "testing" @@ -47,53 +48,58 @@ func TestNormalizeDisplayName_NormNotRequired(t *testing.T) { } func TestGenUsername(t *testing.T) { + type testCase struct { + name string + policy string + userType string + UsernameTemplate string + warningExpected bool + } + + tests := map[string]testCase{ + `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`: { + name: "name1", + policy: "policy1", + userType: "iam_user", + UsernameTemplate: `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`, + warningExpected: false, + }, + `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`: { + name: "this---is---a---very---long---name", + policy: "long------policy------name", + userType: "iam_user", + UsernameTemplate: `{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`, + warningExpected: true, + }, + } + + for k, v := range tests { + testUsername, warning, err := genUsername(v.name, v.policy, v.userType, v.UsernameTemplate) + if err != nil { + t.Fatalf( + "expected no err; got %s", + err, + ) + } - 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)) - } + expectedUsernameRegex := k + require.Regexp(t, expectedUsernameRegex, testUsername) + // IAM/STS usernames are capped at 64 characters + if len(testUsername) > 64 { + t.Fatalf( + "expected username to be of length 64, got %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), - ) + if v.warningExpected { + if warning == "" || !strings.Contains(warning, fmt.Sprintf("calling token's %s user name was truncated to 64 characters", v.userType)) { + 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)) + } + } } } From 8e78bd99d82ea5e004b3945f0a421de7ede9a669 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 14:03:03 -0700 Subject: [PATCH 02/12] add changelog --- changelog/12185.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 changelog/12185.txt diff --git a/changelog/12185.txt b/changelog/12185.txt new file mode 100644 index 0000000000000..4dda1f41bd936 --- /dev/null +++ b/changelog/12185.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/aws: Cap STS usernames at 64 characters +``` \ No newline at end of file From c1af45963fe61106f127b9ec9cac2981475eedaa Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 15:35:46 -0700 Subject: [PATCH 03/12] refactor tests into t.Run block --- builtin/logical/aws/secret_access_keys.go | 3 +- .../logical/aws/secret_access_keys_test.go | 51 +++++++++---------- 2 files changed, 26 insertions(+), 28 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 1b7fcdb0783a7..dd03104a37f94 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -47,8 +47,7 @@ func secretAccessKeys(b *backend) *framework.Secret { } func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, warning string, err error) { - // IAM and STS usernames are capped at 64 chars; this leaves, after the beginning and - // end added below, 42 chars to play with. + // IAM and STS usernames 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) diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index b4544f46010cc..f7acab5de41b8 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -54,52 +54,51 @@ func TestGenUsername(t *testing.T) { userType string UsernameTemplate string warningExpected bool + expectedRegex string } tests := map[string]testCase{ - `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`: { + "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: false, + expectedRegex: `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`, }, - `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`: { + "Too long. Warning expected": { name: "this---is---a---very---long---name", policy: "long------policy------name", userType: "iam_user", UsernameTemplate: `{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`, warningExpected: true, + expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`, }, } - for k, v := range tests { - testUsername, warning, err := genUsername(v.name, v.policy, v.userType, v.UsernameTemplate) - if err != nil { - t.Fatalf( - "expected no err; got %s", - err, - ) - } - - expectedUsernameRegex := k - require.Regexp(t, expectedUsernameRegex, testUsername) - // IAM/STS usernames are capped at 64 characters - if len(testUsername) > 64 { - t.Fatalf( - "expected username to be of length 64, got %d", - len(testUsername), - ) - } + 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) + } - if v.warningExpected { - if warning == "" || !strings.Contains(warning, fmt.Sprintf("calling token's %s user name was truncated to 64 characters", v.userType)) { - t.Fatalf("expected a truncate warning; received empty string") + expectedUsernameRegex := testCase.expectedRegex + require.Regexp(t, expectedUsernameRegex, testUsername) + // IAM/STS usernames are capped at 64 characters + if len(testUsername) > 64 { + t.Fatalf("expected username to be of length 64, got %d", len(testUsername)) } - if len(testUsername) != 64 { - t.Fatalf("expected a username cap at 64 chars; got length: %d", len(testUsername)) + + if testCase.warningExpected { + if warning == "" || !strings.Contains(warning, fmt.Sprintf("calling token's %s user name was truncated to 64 characters", testCase.userType)) { + 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)) + } } - } + }) } } From 76cd008ffa7abb343d06643d1358c091150fb5dd Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 15:59:08 -0700 Subject: [PATCH 04/12] patch: remove warningExpected bool and include expected string --- .../logical/aws/secret_access_keys_test.go | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index f7acab5de41b8..287f486c124af 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -2,7 +2,6 @@ package aws import ( "context" - "fmt" "strings" "testing" @@ -53,7 +52,7 @@ func TestGenUsername(t *testing.T) { policy string userType string UsernameTemplate string - warningExpected bool + warningExpected string expectedRegex string } @@ -63,7 +62,7 @@ func TestGenUsername(t *testing.T) { policy: "policy1", userType: "iam_user", UsernameTemplate: `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`, - warningExpected: false, + warningExpected: "", expectedRegex: `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`, }, "Too long. Warning expected": { @@ -71,7 +70,7 @@ func TestGenUsername(t *testing.T) { policy: "long------policy------name", userType: "iam_user", UsernameTemplate: `{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`, - warningExpected: true, + warningExpected: "calling token's iam_user user name was truncated to 64 characters", expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`, }, } @@ -90,13 +89,12 @@ func TestGenUsername(t *testing.T) { t.Fatalf("expected username to be of length 64, got %d", len(testUsername)) } - if testCase.warningExpected { - if warning == "" || !strings.Contains(warning, fmt.Sprintf("calling token's %s user name was truncated to 64 characters", testCase.userType)) { - 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)) - } + if !strings.Contains(warning, testCase.warningExpected) { + t.Fatalf("expected a truncate warning %s; received %s", testCase.warningExpected, warning) + } + + if len(warning) > 0 && len(testUsername) != 64 { + t.Fatalf("expected a username cap at 64 chars; got length: %d", len(testUsername)) } }) } From db3152aead3c8d8ee3e9e0bb6a4cbd80df0320eb Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 16:57:45 -0700 Subject: [PATCH 05/12] patch: revert sts to cap at 32 chars and add assume_role case in genUsername --- builtin/logical/aws/secret_access_keys.go | 54 ++++++++++++------- .../logical/aws/secret_access_keys_test.go | 23 +++++--- 2 files changed, 52 insertions(+), 25 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index dd03104a37f94..1b7ef023476f0 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -20,6 +20,7 @@ import ( const ( secretAccessKeyType = "access_keys" storageKey = "config/root" + defaultSTSTemplate = `{{ printf "vault-%d-%d" (unix_time) (random 20) | truncate 32 }}` ) func secretAccessKeys(b *backend) *framework.Secret { @@ -47,25 +48,40 @@ func secretAccessKeys(b *backend) *framework.Secret { } func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, warning string, err error) { - // IAM and STS usernames 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) - } + switch userType { + 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) + } - um := UsernameMetadata{ - DisplayName: normalizeDisplayName(displayName), - PolicyName: normalizeDisplayName(policyName), - } + um := UsernameMetadata{ + DisplayName: normalizeDisplayName(displayName), + PolicyName: normalizeDisplayName(policyName), + } - ret, err = up.Generate(um) - if err != nil { - return "", "", fmt.Errorf("failed to generate username: %w", err) - } - // To prevent template from exceeding IAM/STS length limits - if len(ret) > 64 { - ret = ret[0:64] - warning = fmt.Sprintf("the calling token's %s user name was truncated to 64 characters to fit within username length limits", userType) + ret, err = up.Generate(um) + if err != nil { + return "", "", fmt.Errorf("failed to generate username: %w", err) + } + // To prevent template from exceeding IAM length limits + if len(ret) > 64 { + ret = ret[0:64] + warning = fmt.Sprintf("the calling token's %s user name was truncated to 64 characters to fit within username length limits", userType) + } + case "sts": + // Capped at 32 chars + up, err := template.NewTemplate(template.Template(usernameTemplate)) + if err != nil { + return "", "", fmt.Errorf("unable to initialize username template: %w", err) + } + + um := UsernameMetadata{} + ret, err = up.Generate(um) + if err != nil { + return "", "", fmt.Errorf("failed to generate username: %w", err) + } } return } @@ -94,7 +110,7 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, return logical.ErrorResponse(err.Error()), nil } - username, usernameWarning, usernameError := genUsername(displayName, policyName, "sts", defaultUserNameTemplate) + username, usernameWarning, usernameError := genUsername(displayName, policyName, "sts", defaultSTSTemplate) // Send a 400 to Framework.OperationFunc Handler if usernameError != nil { return nil, usernameError @@ -187,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 diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index 287f486c124af..b9c65dbe1b6f0 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -54,6 +54,7 @@ func TestGenUsername(t *testing.T) { UsernameTemplate string warningExpected string expectedRegex string + expectedLength int } tests := map[string]testCase{ @@ -64,14 +65,25 @@ func TestGenUsername(t *testing.T) { 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": { name: "this---is---a---very---long---name", policy: "long------policy------name", - userType: "iam_user", + userType: "assume_role", UsernameTemplate: `{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}`, - warningExpected: "calling token's iam_user user name was truncated to 64 characters", + 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, }, } @@ -84,8 +96,7 @@ func TestGenUsername(t *testing.T) { expectedUsernameRegex := testCase.expectedRegex require.Regexp(t, expectedUsernameRegex, testUsername) - // IAM/STS usernames are capped at 64 characters - if len(testUsername) > 64 { + if len(testUsername) > testCase.expectedLength { t.Fatalf("expected username to be of length 64, got %d", len(testUsername)) } @@ -93,8 +104,8 @@ func TestGenUsername(t *testing.T) { t.Fatalf("expected a truncate warning %s; received %s", testCase.warningExpected, warning) } - if len(warning) > 0 && len(testUsername) != 64 { - t.Fatalf("expected a username cap at 64 chars; got length: %d", len(testUsername)) + if len(warning) > 0 && len(testUsername) != testCase.expectedLength { + t.Fatalf("expected a username cap at %d chars; got length: %d", testCase.expectedLength, len(testUsername)) } }) } From 11dc5ceb2139bd58ca7b35fe9eeb0dfa480e887b Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Tue, 27 Jul 2021 16:58:32 -0700 Subject: [PATCH 06/12] update changelog --- changelog/12185.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12185.txt b/changelog/12185.txt index 4dda1f41bd936..fbba63dee08bc 100644 --- a/changelog/12185.txt +++ b/changelog/12185.txt @@ -1,3 +1,3 @@ ```release-note:improvement -secrets/aws: Cap STS usernames at 64 characters +secrets/aws: Refactor genUsername tests into table tests ``` \ No newline at end of file From 852b2399c121f09508ac98478c0463d61a54befb Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Wed, 28 Jul 2021 13:26:35 -0700 Subject: [PATCH 07/12] update genUsername to return error if username generated exceeds length limits --- builtin/logical/aws/secret_access_keys.go | 36 ++++++------------- .../logical/aws/secret_access_keys_test.go | 30 +++++++--------- 2 files changed, 23 insertions(+), 43 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 1b7ef023476f0..d788f13b2b570 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -20,7 +20,8 @@ import ( const ( secretAccessKeyType = "access_keys" storageKey = "config/root" - defaultSTSTemplate = `{{ printf "vault-%d-%d" (unix_time) (random 20) | truncate 32 }}` + // STS usernames are capped at 32 chars + defaultSTSTemplate = `{{ printf "vault-%d-%d" (unix_time) (random 20) | truncate 32 }}` ) func secretAccessKeys(b *backend) *framework.Secret { @@ -47,13 +48,13 @@ func secretAccessKeys(b *backend) *framework.Secret { } } -func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, warning string, err error) { +func genUsername(displayName, policyName, userType, usernameTemplate string) (ret string, err error) { switch userType { 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) + return "", fmt.Errorf("unable to initialize username template: %w", err) } um := UsernameMetadata{ @@ -63,24 +64,22 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re ret, err = up.Generate(um) if err != nil { - return "", "", fmt.Errorf("failed to generate username: %w", err) + return "", fmt.Errorf("failed to generate username: %w", err) } // To prevent template from exceeding IAM length limits if len(ret) > 64 { - ret = ret[0:64] - warning = fmt.Sprintf("the calling token's %s user name was truncated to 64 characters to fit within username length limits", userType) + return "", fmt.Errorf("the username generated by the template exceeds the IAM/STS username length limits of 64 chars") } case "sts": - // Capped at 32 chars up, err := template.NewTemplate(template.Template(usernameTemplate)) if err != nil { - return "", "", fmt.Errorf("unable to initialize username template: %w", err) + return "", fmt.Errorf("unable to initialize username template: %w", err) } um := UsernameMetadata{} ret, err = up.Generate(um) if err != nil { - return "", "", fmt.Errorf("failed to generate username: %w", err) + return "", fmt.Errorf("failed to generate username: %w", err) } } return @@ -110,7 +109,7 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, return logical.ErrorResponse(err.Error()), nil } - username, usernameWarning, usernameError := genUsername(displayName, policyName, "sts", defaultSTSTemplate) + username, usernameError := genUsername(displayName, policyName, "sts", defaultSTSTemplate) // Send a 400 to Framework.OperationFunc Handler if usernameError != nil { return nil, usernameError @@ -156,10 +155,6 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, // STS are purposefully short-lived and aren't renewable resp.Secret.Renewable = false - if usernameWarning != "" { - resp.AddWarning(usernameWarning) - } - return resp, nil } @@ -200,10 +195,9 @@ func (b *backend) assumeRole(ctx context.Context, s logical.Storage, usernameTemplate = defaultUserNameTemplate } - roleSessionNameWarning := "" var roleSessionNameError error if roleSessionName == "" { - roleSessionName, roleSessionNameWarning, roleSessionNameError = genUsername(displayName, roleName, "assume_role", usernameTemplate) + roleSessionName, roleSessionNameError = genUsername(displayName, roleName, "assume_role", usernameTemplate) // Send a 400 to Framework.OperationFunc Handler if roleSessionNameError != nil { return nil, roleSessionNameError @@ -245,10 +239,6 @@ func (b *backend) assumeRole(ctx context.Context, s logical.Storage, // STS are purposefully short-lived and aren't renewable resp.Secret.Renewable = false - if roleSessionNameWarning != "" { - resp.AddWarning(roleSessionNameWarning) - } - return resp, nil } @@ -289,7 +279,7 @@ func (b *backend) secretAccessKeysCreate( usernameTemplate = defaultUserNameTemplate } - username, usernameWarning, usernameError := genUsername(displayName, policyName, "iam_user", usernameTemplate) + username, usernameError := genUsername(displayName, policyName, "iam_user", usernameTemplate) // Send a 400 to Framework.OperationFunc Handler if usernameError != nil { return nil, usernameError @@ -417,10 +407,6 @@ func (b *backend) secretAccessKeysCreate( resp.Secret.TTL = lease.Lease resp.Secret.MaxTTL = lease.LeaseMax - if usernameWarning != "" { - resp.AddWarning(usernameWarning) - } - return resp, nil } diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index b9c65dbe1b6f0..259f16061b29f 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -52,7 +52,7 @@ func TestGenUsername(t *testing.T) { policy string userType string UsernameTemplate string - warningExpected string + expectedError string expectedRegex string expectedLength int } @@ -63,7 +63,7 @@ func TestGenUsername(t *testing.T) { policy: "policy1", userType: "iam_user", UsernameTemplate: `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`, - warningExpected: "", + expectedError: "", expectedRegex: `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`, expectedLength: 64, }, @@ -72,7 +72,7 @@ func TestGenUsername(t *testing.T) { policy: "policy1", userType: "sts", UsernameTemplate: `{{ printf "vault-%s-%s" (unix_time) (random 20) | truncate 32 }}`, - warningExpected: "", + expectedError: "", expectedRegex: `^vault-[0-9]+-[a-zA-Z0-9]+`, expectedLength: 32, }, @@ -81,7 +81,7 @@ func TestGenUsername(t *testing.T) { 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", + expectedError: "the username generated by the template exceeds the IAM/STS username length limits of 64 chars", expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`, expectedLength: 64, }, @@ -89,23 +89,17 @@ func TestGenUsername(t *testing.T) { 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) + testUsername, err := genUsername(testCase.name, testCase.policy, testCase.userType, testCase.UsernameTemplate) + if err != nil && !strings.Contains(err.Error(), testCase.expectedError) { + t.Fatalf("expected an error %s; instead received %s", testCase.expectedError, 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 err == nil { + require.Regexp(t, testCase.expectedRegex, testUsername) - if len(warning) > 0 && len(testUsername) != testCase.expectedLength { - t.Fatalf("expected a username cap at %d chars; got length: %d", testCase.expectedLength, len(testUsername)) + if len(testUsername) > testCase.expectedLength { + t.Fatalf("expected username to be of length %d, got %d", testCase.expectedLength, len(testUsername)) + } } }) } From 7cba4b0967389760c4924c7547f5e7a74b59d692 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Wed, 28 Jul 2021 13:27:49 -0700 Subject: [PATCH 08/12] update changelog --- changelog/12185.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12185.txt b/changelog/12185.txt index fbba63dee08bc..412b75e01db60 100644 --- a/changelog/12185.txt +++ b/changelog/12185.txt @@ -1,3 +1,3 @@ ```release-note:improvement -secrets/aws: Refactor genUsername tests into table tests +secrets/aws: Refactor genUsername to return error if generated username exceeds length limits ``` \ No newline at end of file From c68bfe05ec410f8e90f5e687933d8d927fd19c73 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Fri, 6 Aug 2021 14:39:27 -0700 Subject: [PATCH 09/12] add conditional default username template to provide custom STS usernames --- builtin/logical/aws/path_config_root.go | 2 +- builtin/logical/aws/secret_access_keys.go | 21 +++++++++++++++---- .../logical/aws/secret_access_keys_test.go | 6 +++--- 3 files changed, 21 insertions(+), 8 deletions(-) diff --git a/builtin/logical/aws/path_config_root.go b/builtin/logical/aws/path_config_root.go index 77c760e02a6f6..b1e12aa437a60 100644 --- a/builtin/logical/aws/path_config_root.go +++ b/builtin/logical/aws/path_config_root.go @@ -8,7 +8,7 @@ import ( "github.com/hashicorp/vault/sdk/logical" ) -const defaultUserNameTemplate = `{{ printf "vault-%s-%s-%s" (printf "%s-%s" (.DisplayName) (.PolicyName) | truncate 42) (unix_time) (random 20) | truncate 64 }}` +const defaultUserNameTemplate = `{{ if (eq .Type "STS") }}{{ printf "vault-%s-%s" (unix_time) (random 20) | truncate 32 }}{{ else }}{{ printf "vault-%s-%s-%s" (printf "%s-%s" (.DisplayName) (.PolicyName) | truncate 42) (unix_time) (random 20) | truncate 64 }}{{ end }}` func pathConfigRoot(b *backend) *framework.Path { return &framework.Path{ diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index d788f13b2b570..a8f0af47990d7 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -20,8 +20,6 @@ import ( const ( secretAccessKeyType = "access_keys" storageKey = "config/root" - // STS usernames are capped at 32 chars - defaultSTSTemplate = `{{ printf "vault-%d-%d" (unix_time) (random 20) | truncate 32 }}` ) func secretAccessKeys(b *backend) *framework.Secret { @@ -58,6 +56,7 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re } um := UsernameMetadata{ + Type: "IAM", DisplayName: normalizeDisplayName(displayName), PolicyName: normalizeDisplayName(policyName), } @@ -76,7 +75,9 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re return "", fmt.Errorf("unable to initialize username template: %w", err) } - um := UsernameMetadata{} + um := UsernameMetadata{ + Type: "STS", + } ret, err = up.Generate(um) if err != nil { return "", fmt.Errorf("failed to generate username: %w", err) @@ -109,7 +110,18 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, return logical.ErrorResponse(err.Error()), nil } - username, usernameError := genUsername(displayName, policyName, "sts", defaultSTSTemplate) + config, err := readConfig(ctx, s) + if err != nil { + return nil, fmt.Errorf("unable to read configuration: %w", err) + } + + // Set as defaultUsernameTemplate if not provided + usernameTemplate := config.UsernameTemplate + if usernameTemplate == "" { + usernameTemplate = defaultUserNameTemplate + } + + username, usernameError := genUsername(displayName, policyName, "sts", usernameTemplate) // Send a 400 to Framework.OperationFunc Handler if usernameError != nil { return nil, usernameError @@ -490,6 +502,7 @@ func convertPolicyARNs(policyARNs []string) []*sts.PolicyDescriptorType { } type UsernameMetadata struct { + Type string DisplayName string PolicyName string } diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index 259f16061b29f..4ba52fb9350db 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -62,7 +62,7 @@ func TestGenUsername(t *testing.T) { name: "name1", policy: "policy1", userType: "iam_user", - UsernameTemplate: `{{ printf "vault-%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) | truncate 64 }}`, + UsernameTemplate: defaultUserNameTemplate, expectedError: "", expectedRegex: `^vault-name1-policy1-[0-9]+-[a-zA-Z0-9]+`, expectedLength: 64, @@ -71,7 +71,7 @@ func TestGenUsername(t *testing.T) { name: "name1", policy: "policy1", userType: "sts", - UsernameTemplate: `{{ printf "vault-%s-%s" (unix_time) (random 20) | truncate 32 }}`, + UsernameTemplate: defaultUserNameTemplate, expectedError: "", expectedRegex: `^vault-[0-9]+-[a-zA-Z0-9]+`, expectedLength: 32, @@ -80,7 +80,7 @@ func TestGenUsername(t *testing.T) { 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) }}`, + UsernameTemplate: `{{ if (eq .Type "IAM") }}{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}{{ end }}`, expectedError: "the username generated by the template exceeds the IAM/STS username length limits of 64 chars", expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`, expectedLength: 64, From 0dd2dfac88a6b347a704b1ea21f9e57649bdbe33 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Fri, 6 Aug 2021 14:40:29 -0700 Subject: [PATCH 10/12] update changelog --- changelog/12185.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changelog/12185.txt b/changelog/12185.txt index 412b75e01db60..919da9e16e19f 100644 --- a/changelog/12185.txt +++ b/changelog/12185.txt @@ -1,3 +1,3 @@ ```release-note:improvement -secrets/aws: Refactor genUsername to return error if generated username exceeds length limits +secrets/aws: Add conditional template that allows custom usernames for both STS and IAM cases ``` \ No newline at end of file From 212711b20edf2816bc64ffb4cd68de2296f14ef3 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Fri, 6 Aug 2021 15:15:20 -0700 Subject: [PATCH 11/12] include test for failing STS length case --- builtin/logical/aws/path_config_root.go | 1 + builtin/logical/aws/secret_access_keys.go | 6 +++++- builtin/logical/aws/secret_access_keys_test.go | 15 ++++++++++++--- 3 files changed, 18 insertions(+), 4 deletions(-) diff --git a/builtin/logical/aws/path_config_root.go b/builtin/logical/aws/path_config_root.go index b1e12aa437a60..1262980fa8066 100644 --- a/builtin/logical/aws/path_config_root.go +++ b/builtin/logical/aws/path_config_root.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/vault/sdk/logical" ) +// A single default template that supports both the different credential types (IAM/STS) that are capped at differing length limits (64 chars/32 chars respectively) const defaultUserNameTemplate = `{{ if (eq .Type "STS") }}{{ printf "vault-%s-%s" (unix_time) (random 20) | truncate 32 }}{{ else }}{{ printf "vault-%s-%s-%s" (printf "%s-%s" (.DisplayName) (.PolicyName) | truncate 42) (unix_time) (random 20) | truncate 64 }}{{ end }}` func pathConfigRoot(b *backend) *framework.Path { diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index a8f0af47990d7..6dcbbe13e33c9 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -67,7 +67,7 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re } // To prevent template from exceeding IAM length limits if len(ret) > 64 { - return "", fmt.Errorf("the username generated by the template exceeds the IAM/STS username length limits of 64 chars") + return "", fmt.Errorf("the username generated by the template exceeds the IAM username length limits of 64 chars") } case "sts": up, err := template.NewTemplate(template.Template(usernameTemplate)) @@ -82,6 +82,10 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re if err != nil { return "", fmt.Errorf("failed to generate username: %w", err) } + // To prevent template from exceeding STS length limits + if len(ret) > 32 { + return "", fmt.Errorf("the username generated by the template exceeds the STS username length limits of 32 chars") + } } return } diff --git a/builtin/logical/aws/secret_access_keys_test.go b/builtin/logical/aws/secret_access_keys_test.go index 4ba52fb9350db..a3daa45e16ae2 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -76,15 +76,24 @@ func TestGenUsername(t *testing.T) { expectedRegex: `^vault-[0-9]+-[a-zA-Z0-9]+`, expectedLength: 32, }, - "Too long. Warning expected": { + "Too long. Error expected — IAM": { name: "this---is---a---very---long---name", policy: "long------policy------name", userType: "assume_role", UsernameTemplate: `{{ if (eq .Type "IAM") }}{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}{{ end }}`, - expectedError: "the username generated by the template exceeds the IAM/STS username length limits of 64 chars", - expectedRegex: `this---is---a---very---long---name-long------policy------name-[0-9][0-9]`, + expectedError: "the username generated by the template exceeds the IAM username length limits of 64 chars", + expectedRegex: "", expectedLength: 64, }, + "Too long. Error expected — STS": { + name: "this---is---a---very---long---name", + policy: "long------policy------name", + userType: "sts", + UsernameTemplate: `{{ if (eq .Type "STS") }}{{ printf "%s-%s-%s-%s" (.DisplayName) (.PolicyName) (unix_time) (random 20) }}{{ end }}`, + expectedError: "the username generated by the template exceeds the STS username length limits of 32 chars", + expectedRegex: "", + expectedLength: 32, + }, } for testDescription, testCase := range tests { From c220b938ce0ba76953820f510a6447aacb3bced7 Mon Sep 17 00:00:00 2001 From: Vinay Gopalan Date: Fri, 6 Aug 2021 15:22:34 -0700 Subject: [PATCH 12/12] update comments for more clarity --- builtin/logical/aws/secret_access_keys.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 6dcbbe13e33c9..3f20abc8744b8 100644 --- a/builtin/logical/aws/secret_access_keys.go +++ b/builtin/logical/aws/secret_access_keys.go @@ -65,7 +65,7 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re if err != nil { return "", fmt.Errorf("failed to generate username: %w", err) } - // To prevent template from exceeding IAM length limits + // To prevent a custom template from exceeding IAM length limits if len(ret) > 64 { return "", fmt.Errorf("the username generated by the template exceeds the IAM username length limits of 64 chars") } @@ -82,7 +82,7 @@ func genUsername(displayName, policyName, userType, usernameTemplate string) (re if err != nil { return "", fmt.Errorf("failed to generate username: %w", err) } - // To prevent template from exceeding STS length limits + // To prevent a custom template from exceeding STS length limits if len(ret) > 32 { return "", fmt.Errorf("the username generated by the template exceeds the STS username length limits of 32 chars") }