From 23770cc2a7c6f5491f1ecaa2ede4daad9c31eccb Mon Sep 17 00:00:00 2001 From: vinay-gopalan <86625824+vinay-gopalan@users.noreply.github.com> Date: Mon, 9 Aug 2021 09:40:47 -0700 Subject: [PATCH] Update genUsername to cap STS usernames at 32 chars (#12185) * update genUsername to cap STS usernames at 64 chars * add changelog * refactor tests into t.Run block * patch: remove warningExpected bool and include expected string * patch: revert sts to cap at 32 chars and add assume_role case in genUsername * update changelog * update genUsername to return error if username generated exceeds length limits * update changelog * add conditional default username template to provide custom STS usernames * update changelog * include test for failing STS length case * update comments for more clarity --- builtin/logical/aws/path_config_root.go | 3 +- builtin/logical/aws/secret_access_keys.go | 63 ++++++------ .../logical/aws/secret_access_keys_test.go | 99 +++++++++++-------- changelog/12185.txt | 3 + 4 files changed, 95 insertions(+), 73 deletions(-) create mode 100644 changelog/12185.txt diff --git a/builtin/logical/aws/path_config_root.go b/builtin/logical/aws/path_config_root.go index 77c760e02..1262980fa 100644 --- a/builtin/logical/aws/path_config_root.go +++ b/builtin/logical/aws/path_config_root.go @@ -8,7 +8,8 @@ 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 }}` +// 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 { return &framework.Path{ diff --git a/builtin/logical/aws/secret_access_keys.go b/builtin/logical/aws/secret_access_keys.go index 89fc29c52..3f20abc87 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 { @@ -47,42 +46,45 @@ 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": - // 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) + return "", fmt.Errorf("unable to initialize username template: %w", err) } um := UsernameMetadata{ + Type: "IAM", DisplayName: normalizeDisplayName(displayName), PolicyName: normalizeDisplayName(policyName), } 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 + // To prevent a custom 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" + return "", fmt.Errorf("the username generated by the template exceeds the IAM username length limits of 64 chars") } 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) + 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) + return "", fmt.Errorf("failed to generate username: %w", err) + } + // 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") } } return @@ -112,7 +114,18 @@ func (b *backend) getFederationToken(ctx context.Context, s logical.Storage, return logical.ErrorResponse(err.Error()), nil } - username, usernameWarning, 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 @@ -158,10 +171,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 } @@ -202,10 +211,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, "iam_user", usernameTemplate) + roleSessionName, roleSessionNameError = genUsername(displayName, roleName, "assume_role", usernameTemplate) // Send a 400 to Framework.OperationFunc Handler if roleSessionNameError != nil { return nil, roleSessionNameError @@ -247,10 +255,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 } @@ -291,7 +295,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 @@ -419,10 +423,6 @@ func (b *backend) secretAccessKeysCreate( resp.Secret.TTL = lease.Lease resp.Secret.MaxTTL = lease.LeaseMax - if usernameWarning != "" { - resp.AddWarning(usernameWarning) - } - return resp, nil } @@ -506,6 +506,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 b9dce2514..a3daa45e1 100644 --- a/builtin/logical/aws/secret_access_keys_test.go +++ b/builtin/logical/aws/secret_access_keys_test.go @@ -47,53 +47,70 @@ 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, - ) + type testCase struct { + name string + policy string + userType string + UsernameTemplate string + expectedError string + expectedRegex string + expectedLength int } - 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), - ) + tests := map[string]testCase{ + "Truncated to 64. No warnings expected": { + name: "name1", + policy: "policy1", + userType: "iam_user", + UsernameTemplate: defaultUserNameTemplate, + expectedError: "", + 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: defaultUserNameTemplate, + expectedError: "", + expectedRegex: `^vault-[0-9]+-[a-zA-Z0-9]+`, + expectedLength: 32, + }, + "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 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, + }, } - 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) }}`, - ) + for testDescription, testCase := range tests { + t.Run(testDescription, func(t *testing.T) { + 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) + } - 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)) - } + if err == nil { + require.Regexp(t, testCase.expectedRegex, 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 len(testUsername) > testCase.expectedLength { + t.Fatalf("expected username to be of length %d, got %d", testCase.expectedLength, len(testUsername)) + } + } + }) } } diff --git a/changelog/12185.txt b/changelog/12185.txt new file mode 100644 index 000000000..919da9e16 --- /dev/null +++ b/changelog/12185.txt @@ -0,0 +1,3 @@ +```release-note:improvement +secrets/aws: Add conditional template that allows custom usernames for both STS and IAM cases +``` \ No newline at end of file