diff --git a/builtin/credential/aws/backend_test.go b/builtin/credential/aws/backend_test.go index 84b667a4a362..bb186df7ec3b 100644 --- a/builtin/credential/aws/backend_test.go +++ b/builtin/credential/aws/backend_test.go @@ -1084,12 +1084,12 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing. Data: loginInput, } - // Place the wrong AMI ID in the role data. + // Baseline role data that should succeed permit login data := map[string]interface{}{ "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, "bound_ec2_instance_id": []string{parsedIdentityDoc.InstanceID, "i-1234567"}, @@ -1102,63 +1102,64 @@ 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 + // Place correct IAM role ARN, but incorrect instance ID data["bound_iam_role_arn"] = []string{"wrong_iam_role_arn_1", iamARN, "wrong_iam_role_arn_2"} data["bound_ec2_instance_id"] = "i-1234567" - 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 instance ID 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 EC2 Instance ID + // Place correct instance ID, but substring of the IAM role ARN data["bound_ec2_instance_id"] = []string{parsedIdentityDoc.InstanceID, "i-1234567"} - resp, err = b.HandleRequest(context.Background(), roleReq) + 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) + } + + // 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_login.go b/builtin/credential/aws/path_login.go index 3a59d34e07ae..0a78b8b10fb2 100644 --- a/builtin/credential/aws/path_login.go +++ b/builtin/credential/aws/path_login.go @@ -445,8 +445,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 } @@ -498,7 +514,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 d1586024d10a..d31ab3fd72be 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 { @@ -320,7 +320,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 @@ -330,15 +330,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 @@ -354,57 +351,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) diff --git a/builtin/credential/aws/path_role_test.go b/builtin/credential/aws/path_role_test.go index efed15e2db62..1fa2ab178e65 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", "bound_ec2_instance_id": "i-12345678901234567,i-76543210987654321", @@ -605,7 +605,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": "", @@ -711,6 +711,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 } diff --git a/website/source/api/auth/aws/index.html.md b/website/source/api/auth/aws/index.html.md index 16e01c3d0df0..667c390df18a 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. - `bound_ec2_instance_id` `(list: [])` - If set, defines a constraint on the