From dbf42f37772ab7d5108d7d4daa7bc9722c59f74c Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Fri, 2 Mar 2018 22:15:11 -0500 Subject: [PATCH 1/4] Update aws auth docs with new semantics Moving away from implicitly globbed bound_iam_role_arn and bound_iam_instance_profile_arn variables to make them explicit --- website/source/api/auth/aws/index.html.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/website/source/api/auth/aws/index.html.md b/website/source/api/auth/aws/index.html.md index 23a63b1bae56..986fbb1d4748 100644 --- a/website/source/api/auth/aws/index.html.md +++ b/website/source/api/auth/aws/index.html.md @@ -576,16 +576,16 @@ list in order to satisfy that constraint. comma-separated string or a JSON array. - `bound_iam_role_arn` `(list: [])` - If set, defines a constraint on the authenticating EC2 instance that it must match one of the IAM role ARNs specified by - this parameter. The value is refix-matched (as though it were a glob ending - in `*`). The configured IAM user or EC2 instance role must be allowed to + this parameter. Wildcards are supported at the end of the ARN to allow for + prefix matching. The configured IAM user or EC2 instance role must be allowed to execute the `iam:GetInstanceProfile` action if this is specified. This constraint is checked by the ec2 auth method as well as the iam auth method only when inferring an EC2 instance. This is a comma-separated string or a JSON array. - `bound_iam_instance_profile_arn` `(list: [])` - If set, defines a constraint - on the EC2 instances to be associated with an IAM instance profile ARN which - has a prefix that matches one of the values specified by this parameter. The value is - prefix-matched (as though it were a glob ending in `*`). This constraint is + on the EC2 instances to be associated with an IAM instance profile ARN. + Wildcards are supported at the end of the ARN to allow for prefix matching. + This constraint is checked by the ec2 auth method as well as the iam auth method only when inferring an ec2 instance. This is a comma-separated string or a JSON array. - `role_tag` `(string: "")` - If set, enables the role tags for this role. The From 80f93f6bc543cd010ebddf1f895befa58c1dab01 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Fri, 2 Mar 2018 22:59:07 -0500 Subject: [PATCH 2/4] Refactor tests to reduce duplication auth/aws EC2 login tests had the same flow duplicated a few times, so refactoring to reduce duplication --- builtin/credential/aws/backend_test.go | 50 ++++++++++-------------- builtin/credential/aws/path_role_test.go | 4 +- 2 files changed, 23 insertions(+), 31 deletions(-) diff --git a/builtin/credential/aws/backend_test.go b/builtin/credential/aws/backend_test.go index 2e8f0eb3e9d7..a395cdd50681 100644 --- a/builtin/credential/aws/backend_test.go +++ b/builtin/credential/aws/backend_test.go @@ -1084,7 +1084,7 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing. "auth_type": "ec2", "policies": "root", "max_ttl": "120s", - "bound_ami_id": []string{"wrong_ami_id", "wrong_ami_id2"}, + "bound_ami_id": []string{"wrong_ami_id", amiID, "wrong_ami_id2"}, "bound_account_id": accountID, "bound_iam_role_arn": iamARN, } @@ -1096,50 +1096,42 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing. Data: data, } - // Save the role with wrong AMI ID - resp, err := b.HandleRequest(context.Background(), roleReq) - if err != nil && (resp != nil && resp.IsError()) { - t.Fatalf("bad: resp: %#v\nerr:%v", resp, err) + updateRoleExpectLoginFail := func(roleRequest, loginRequest *logical.Request) error { + resp, err := b.HandleRequest(context.Background(), roleRequest) + if err != nil || (resp != nil && resp.IsError()) { + return fmt.Errorf("bad: failed to create role: resp:%#v\nerr:%v", resp, err) + } + resp, err = b.HandleRequest(context.Background(), loginRequest) + if err != nil || resp == nil || (resp != nil && !resp.IsError()) { + return fmt.Errorf("bad: expected login failure: resp:%#v\nerr:%v", resp, err) + } + return nil } - // Expect failure when tried to login with wrong AMI ID - resp, err = b.HandleRequest(context.Background(), loginRequest) - if err != nil || resp == nil || (resp != nil && !resp.IsError()) { - t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err) + // Test a role with the wrong AMI ID + data["bound_ami_id"] = []string{"ami-1234567", "ami-7654321"} + if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil { + t.Fatal(err) } - // Place the correct AMI ID in one of the values, but make the AccountID wrong roleReq.Operation = logical.UpdateOperation + // Place the correct AMI ID in one of the values, but make the AccountID wrong data["bound_ami_id"] = []string{"wrong_ami_id_1", amiID, "wrong_ami_id_2"} data["bound_account_id"] = []string{"wrong-account-id", "wrong-account-id-2"} - resp, err = b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("bad: failed to create role: resp:%#v\nerr:%v", resp, err) - } - - // Expect failure when tried to login with incorrect AccountID - resp, err = b.HandleRequest(context.Background(), loginRequest) - if err != nil || resp == nil || (resp != nil && !resp.IsError()) { - t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err) + if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil { + t.Fatal(err) } // Place the correct AccountID in one of the values, but make the wrong IAMRoleARN data["bound_account_id"] = []string{"wrong-account-id-1", accountID, "wrong-account-id-2"} data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn", "wrong_iam_role_arn_2"} - resp, err = b.HandleRequest(context.Background(), roleReq) - if err != nil || (resp != nil && resp.IsError()) { - t.Fatalf("bad: failed to create role: resp:%#v\nerr:%v", resp, err) - } - - // Attempt to login and expect a fail because IAM Role ARN is wrong - resp, err = b.HandleRequest(context.Background(), loginRequest) - if err != nil || resp == nil || (resp != nil && !resp.IsError()) { - t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err) + if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil { + t.Fatal(err) } // place a correct IAM role ARN data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn_1", iamARN, "wrong_iam_role_arn_2"} - resp, err = b.HandleRequest(context.Background(), roleReq) + resp, err := b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("bad: failed to create role: resp:%#v\nerr:%v", resp, err) } diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index 18f6a3e40aad..d0367b601e06 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -567,7 +567,7 @@ func TestAwsEc2_RoleCrud(t *testing.T) { "bound_account_id": "testaccountid", "bound_region": "testregion", "bound_iam_role_arn": "arn:aws:iam::123456789012:role/MyRole", - "bound_iam_instance_profile_arn": "arn:aws:iam::123456789012:instance-profile/MyInstanceProfile", + "bound_iam_instance_profile_arn": "arn:aws:iam::123456789012:instance-profile/MyInstancePro*", "bound_subnet_id": "testsubnetid", "bound_vpc_id": "testvpcid", "role_tag": "testtag", @@ -603,7 +603,7 @@ func TestAwsEc2_RoleCrud(t *testing.T) { "bound_iam_principal_arn": []string{}, "bound_iam_principal_id": []string{}, "bound_iam_role_arn": []string{"arn:aws:iam::123456789012:role/MyRole"}, - "bound_iam_instance_profile_arn": []string{"arn:aws:iam::123456789012:instance-profile/MyInstanceProfile"}, + "bound_iam_instance_profile_arn": []string{"arn:aws:iam::123456789012:instance-profile/MyInstancePro*"}, "bound_subnet_id": []string{"testsubnetid"}, "bound_vpc_id": []string{"testvpcid"}, "inferred_entity_type": "", From 58a7b87f32dfe741111e85ed1b6d42d489f23456 Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Fri, 2 Mar 2018 23:14:19 -0500 Subject: [PATCH 3/4] Add tests for aws auth explicit wildcard constraints --- builtin/credential/aws/backend_test.go | 18 ++++++++++-- builtin/credential/aws/path_role_test.go | 37 ++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 2 deletions(-) diff --git a/builtin/credential/aws/backend_test.go b/builtin/credential/aws/backend_test.go index a395cdd50681..cdaa8aac5b45 100644 --- a/builtin/credential/aws/backend_test.go +++ b/builtin/credential/aws/backend_test.go @@ -1129,8 +1129,22 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing. t.Fatal(err) } - // place a correct IAM role ARN - data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn_1", iamARN, "wrong_iam_role_arn_2"} + // place a substring of the IAM role ARN + data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn", iamARN[:len(iamARN)-2], "wrong_iam_role_arn_2"} + if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil { + t.Fatal(err) + } + + // place a wildcard in the middle of the role ARN + // The :31 gets arn:aws:iam::123456789012:role/ + // This test relies on the role name having at least two characters + data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn", fmt.Sprintf("%s*%s", iamARN[:31], iamARN[32:])} + if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil { + t.Fatal(err) + } + + // place a globbed IAM role ARN + data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn_1", fmt.Sprintf("%s*", iamARN[:len(iamARN)-2]), "wrong_iam_role_arn_2"} resp, err := b.HandleRequest(context.Background(), roleReq) if err != nil || (resp != nil && resp.IsError()) { t.Fatalf("bad: failed to create role: resp:%#v\nerr:%v", resp, err) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index d0367b601e06..045352f59a28 100644 --- a/builtin/credential/aws/path_role_test.go +++ b/builtin/credential/aws/path_role_test.go @@ -709,6 +709,43 @@ func TestAwsEc2_RoleDurationSeconds(t *testing.T) { } } +func TestRoleEntryUpgradeV1(t *testing.T) { + config := logical.TestBackendConfig() + storage := &logical.InmemStorage{} + config.StorageView = storage + b, err := Backend(config) + if err != nil { + t.Fatal(err) + } + + err = b.Setup(context.Background(), config) + if err != nil { + t.Fatal(err) + } + + roleEntryToUpgrade := &awsRoleEntry{ + BoundIamRoleARNs: []string{"arn:aws:iam::123456789012:role/my_role_prefix"}, + BoundIamInstanceProfileARNs: []string{"arn:aws:iam::123456789012:instance-profile/my_profile-prefix"}, + Version: 1, + } + expected := &awsRoleEntry{ + BoundIamRoleARNs: []string{"arn:aws:iam::123456789012:role/my_role_prefix*"}, + BoundIamInstanceProfileARNs: []string{"arn:aws:iam::123456789012:instance-profile/my_profile-prefix*"}, + Version: currentRoleStorageVersion, + } + + upgraded, err := b.upgradeRoleEntry(context.Background(), storage, roleEntryToUpgrade) + if err != nil { + t.Fatalf("error upgrading role entry: %#v", err) + } + if !upgraded { + t.Fatalf("expected to upgrade role entry %#v but got no upgrade", roleEntryToUpgrade) + } + if !reflect.DeepEqual(*roleEntryToUpgrade, *expected) { + t.Fatalf("bad: expected upgraded role of %#v, got %#v instead", expected, roleEntryToUpgrade) + } +} + func resolveArnToFakeUniqueId(ctx context.Context, s logical.Storage, arn string) (string, error) { return "FakeUniqueId1", nil } From 84ea5f2ccaacf8a0d81a681194626fc8cc128a7b Mon Sep 17 00:00:00 2001 From: Joel Thompson Date: Fri, 2 Mar 2018 23:46:54 -0500 Subject: [PATCH 4/4] Remove implicit prefix matching from AWS auth backend In the aws auth backend, bound_iam_role_arn and bound_iam_instance_profile_arn were ALWAYS prefix matched, and there was no way to opt out of this implicit prefix matching. This now makes the implicit prefix matching an explicit opt-in feature by requiring users to specify a * at the end of an ARN if they want the prefix matching. --- builtin/credential/aws/path_login.go | 25 +++++++++++++++++++++++-- builtin/credential/aws/path_role.go | 27 ++++++++++++--------------- 2 files changed, 35 insertions(+), 17 deletions(-) diff --git a/builtin/credential/aws/path_login.go b/builtin/credential/aws/path_login.go index c6d98ab86e86..8bd584e2f8fd 100644 --- a/builtin/credential/aws/path_login.go +++ b/builtin/credential/aws/path_login.go @@ -440,8 +440,24 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context, } iamInstanceProfileARN := *instance.IamInstanceProfile.Arn matchesInstanceProfile := false + // NOTE: Can't use strutil.StrListContainsGlob. A * is a perfectly valid character in the "path" component + // of an ARN. See, e.g., https://docs.aws.amazon.com/IAM/latest/APIReference/API_CreateInstanceProfile.html : + // The path allows strings "containing any ASCII character from the ! (\u0021) thru the DEL character + // (\u007F), including most punctuation characters, digits, and upper and lowercased letters." + // So, e.g., arn:aws:iam::123456789012:instance-profile/Some*Path/MyProfileName is a perfectly valid instance + // profile ARN, and it wouldn't be correct to expand the * in the middle as a wildcard. + // If a user wants to match an IAM instance profile arn beginning with arn:aws:iam::123456789012:instance-profile/foo* + // then bound_iam_instance_profile_arn would need to be arn:aws:iam::123456789012:instance-profile/foo** + // Wanting to exactly match an ARN that has a * at the end is not a valid use case. The * is only valid in the + // path; it's not valid in the name. That means no valid ARN can ever end with a *. For example, + // arn:aws:iam::123456789012:instance-profile/Foo* is NOT valid as an instance profile ARN, so no valid instance + // profile ARN could ever equal that value. for _, boundInstanceProfileARN := range roleEntry.BoundIamInstanceProfileARNs { - if strings.HasPrefix(iamInstanceProfileARN, boundInstanceProfileARN) { + switch { + case strings.HasSuffix(boundInstanceProfileARN, "*") && strings.HasPrefix(iamInstanceProfileARN, boundInstanceProfileARN[:len(boundInstanceProfileARN)-1]): + matchesInstanceProfile = true + break + case iamInstanceProfileARN == boundInstanceProfileARN: matchesInstanceProfile = true break } @@ -493,7 +509,12 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context, matchesInstanceRoleARN := false for _, boundIamRoleARN := range roleEntry.BoundIamRoleARNs { - if strings.HasPrefix(iamRoleARN, boundIamRoleARN) { + switch { + // as with boundInstanceProfileARN, can't use strutil.StrListContainsGlob because * can validly exist in the middle of an ARN + case strings.HasSuffix(boundIamRoleARN, "*") && strings.HasPrefix(iamRoleARN, boundIamRoleARN[:len(boundIamRoleARN)-1]): + matchesInstanceRoleARN = true + break + case iamRoleARN == boundIamRoleARN: matchesInstanceRoleARN = true break } diff --git a/builtin/credential/aws/path_role.go b/builtin/credential/aws/path_role.go index 2a1d463dc7ec..ac88f4452736 100644 --- a/builtin/credential/aws/path_role.go +++ b/builtin/credential/aws/path_role.go @@ -14,7 +14,7 @@ import ( ) var ( - currentRoleStorageVersion = 1 + currentRoleStorageVersion = 2 ) func pathRole(b *backend) *framework.Path { @@ -313,7 +313,7 @@ func (b *backend) upgradeRoleEntry(ctx context.Context, s logical.Storage, roleE if roleEntry == nil { return false, fmt.Errorf("received nil roleEntry") } - var upgraded bool + upgraded := roleEntry.Version < currentRoleStorageVersion switch roleEntry.Version { case 0: // Check if the value held by role ARN field is actually an instance profile ARN @@ -323,15 +323,12 @@ func (b *backend) upgradeRoleEntry(ctx context.Context, s logical.Storage, roleE // Reset the old field roleEntry.BoundIamRoleARN = "" - - upgraded = true } // Check if there was no pre-existing AuthType set (from older versions) if roleEntry.AuthType == "" { // then default to the original behavior of ec2 roleEntry.AuthType = ec2AuthType - upgraded = true } // Check if we need to resolve the unique ID on the role @@ -347,57 +344,57 @@ func (b *backend) upgradeRoleEntry(ctx context.Context, s logical.Storage, roleE roleEntry.BoundIamPrincipalID = principalId // Not setting roleEntry.BoundIamPrincipalARN to "" here so that clients can see the original // ARN that the role was bound to - upgraded = true } // Check if we need to convert individual string values to lists if roleEntry.BoundAmiID != "" { roleEntry.BoundAmiIDs = []string{roleEntry.BoundAmiID} roleEntry.BoundAmiID = "" - upgraded = true } if roleEntry.BoundAccountID != "" { roleEntry.BoundAccountIDs = []string{roleEntry.BoundAccountID} roleEntry.BoundAccountID = "" - upgraded = true } if roleEntry.BoundIamPrincipalARN != "" { roleEntry.BoundIamPrincipalARNs = []string{roleEntry.BoundIamPrincipalARN} roleEntry.BoundIamPrincipalARN = "" - upgraded = true } if roleEntry.BoundIamPrincipalID != "" { roleEntry.BoundIamPrincipalIDs = []string{roleEntry.BoundIamPrincipalID} roleEntry.BoundIamPrincipalID = "" - upgraded = true } if roleEntry.BoundIamRoleARN != "" { roleEntry.BoundIamRoleARNs = []string{roleEntry.BoundIamRoleARN} roleEntry.BoundIamRoleARN = "" - upgraded = true } if roleEntry.BoundIamInstanceProfileARN != "" { roleEntry.BoundIamInstanceProfileARNs = []string{roleEntry.BoundIamInstanceProfileARN} roleEntry.BoundIamInstanceProfileARN = "" - upgraded = true } if roleEntry.BoundRegion != "" { roleEntry.BoundRegions = []string{roleEntry.BoundRegion} roleEntry.BoundRegion = "" - upgraded = true } if roleEntry.BoundSubnetID != "" { roleEntry.BoundSubnetIDs = []string{roleEntry.BoundSubnetID} roleEntry.BoundSubnetID = "" - upgraded = true } if roleEntry.BoundVpcID != "" { roleEntry.BoundVpcIDs = []string{roleEntry.BoundVpcID} roleEntry.BoundVpcID = "" - upgraded = true } roleEntry.Version = 1 fallthrough + case 1: + // Make BoundIamRoleARNs and BoundIamInstanceProfileARNs explicitly prefix-matched + for i, arn := range roleEntry.BoundIamRoleARNs { + roleEntry.BoundIamRoleARNs[i] = fmt.Sprintf("%s*", arn) + } + for i, arn := range roleEntry.BoundIamInstanceProfileARNs { + roleEntry.BoundIamInstanceProfileARNs[i] = fmt.Sprintf("%s*", arn) + } + roleEntry.Version = 2 + fallthrough case currentRoleStorageVersion: default: return false, fmt.Errorf("unrecognized role version: %q", roleEntry.Version)