Allow non-prefix-matched IAM role and instance profile ARNs in AWS auth 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.

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)
+	if err := updateRoleExpectLoginFail(roleReq, loginRequest); err != nil {
+		t.Fatal(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)
-	}
-
-	// 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)
 	}

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
 			}

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)

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
 }

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