Skip to content

Commit

Permalink
Allow non-prefix-matched IAM role and instance profile ARNs in AWS au…
Browse files Browse the repository at this point in the history 
…th backend (#4071)

* 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

* Refactor tests to reduce duplication

auth/aws EC2 login tests had the same flow duplicated a few times, so
refactoring to reduce duplication

* Add tests for aws auth explicit wildcard constraints

* 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.
  • Loading branch information
@joelthompson @jefferai
joelthompson authored and jefferai committed Mar 18, 2018
1 parent aabccd5 commit 29551c0
Show file tree
Hide file tree
Showing 5 changed files with 120 additions and 64 deletions.
81 changes: 41 additions & 40 deletions builtin/credential/aws/backend_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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"},
Expand All @@ -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)
}
Expand Down
25 changes: 23 additions & 2 deletions builtin/credential/aws/path_login.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
}
Expand Down Expand Up @@ -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
}
Expand Down
27 changes: 12 additions & 15 deletions builtin/credential/aws/path_role.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import (
)

var (
currentRoleStorageVersion = 1
currentRoleStorageVersion = 2
)

func pathRole(b *backend) *framework.Path {
Expand Down Expand Up @@ -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
Expand All @@ -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
Expand All @@ -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)
Expand Down
41 changes: 39 additions & 2 deletions builtin/credential/aws/path_role_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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": "",
Expand Down Expand Up @@ -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
}
10 changes: 5 additions & 5 deletions website/source/api/auth/aws/index.html.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 29551c0

Please sign in to comment.