Permit non-prefix matching for bound_iam_role_arn

[pfhayes]

Feature Request:
Currently bound_iam_role_arn (and bound_iam_instance_profile_arn) is validated by prefix matching, but we would like it to be an exact match because some roles may be prefixes of others

Would this be desirable? I'm happy to make a PR, but what would be the best way for this to manifest? As a new parameter on the Create Role endpoint that opts in to exact matching?

Environment:

• Vault Version: Vault v0.8.1 ('8d76a41854608c547a233f2e6292ae5355154695')
• Operating System/Architecture: All

[pfhayes]

Relevant matching code is here:

vault/builtin/credential/aws/path_login.go

Line 485 in a4f4e5b

if !strings.HasPrefix(iamRoleARN, roleEntry.BoundIamRoleARN) {

[joelthompson]

Hi @pfhayes -- thinking super tactically, if you want to bind to the exact IAM role ARN, then you could just use the iam auth_type and specify the bound_iam_principal_arn and not bother with the bound_iam_role_arn. Functionally, it should be identical, except that bound_iam_principal_arn doesn't do the automatic prefix matching (though in 0.8.2, it'll allow you to put a wildcard at the end to glob, but that's something you'd have to opt in to explicitly). This won't work for binding to the instance profile ARN, but might be good enough for you.

More broadly, I largely agree with your sentiment, and I think it would have been better to have made the prefix matching something users explicitly opt in to, e.g., by adding a wildcard at the end, which is how it'll work in 0.8.2 with the bound_iam_principal_arn. But, there's already a lot of users depending on the current behavior, so I don't think we could break it. A couple of possible thoughts:

1. (harder) Change the behavior to match that of bound_iam_principal_arn, and include upgrade logic in the roles so all old roles will have bound_iam_role_arn and bound_iam_instance_profile_arn appended with a wildcard, but all new roles going forward won't. This makes it more consistent with AWS and the bound_iam_principal_arn semantics. The downside is it could break existing scripts, though I don't think the downside would be that bad. There's already some auto-upgrade code that could just be extended in

   vault/builtin/credential/aws/path_role.go

   Lines 293 to 333 in b0f8d16

   	// If needed, updates the role entry and returns a bool indicating if it was updated
   	// (and thus needs to be persisted)
   	func (b *backend) upgradeRoleEntry(s logical.Storage, roleEntry *awsRoleEntry) (bool, error) {
   	if roleEntry == nil {
   	return false, fmt.Errorf("received nil roleEntry")
   	}
   	var upgraded bool
   	// Check if the value held by role ARN field is actually an instance profile ARN
   	if roleEntry.BoundIamRoleARN != "" && strings.Contains(roleEntry.BoundIamRoleARN, ":instance-profile/") {
   	// If yes, move it to the correct field
   	roleEntry.BoundIamInstanceProfileARN = roleEntry.BoundIamRoleARN
   	// 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
   	}
   	if roleEntry.AuthType == iamAuthType &&
   	roleEntry.ResolveAWSUniqueIDs &&
   	roleEntry.BoundIamPrincipalARN != "" &&
   	roleEntry.BoundIamPrincipalID == "" &&
   	!strings.HasSuffix(roleEntry.BoundIamPrincipalARN, "*") {
   	principalId, err := b.resolveArnToUniqueIDFunc(s, roleEntry.BoundIamPrincipalARN)
   	if err != nil {
   	return false, err
   	}
   	roleEntry.BoundIamPrincipalID = principalId
   	upgraded = true
   	}
   	return upgraded, nil
   	}

2. (easier) What you suggest and add a new flag on the role, but it feels like roles already have too many flags, this would just complicate things. Upside is it's completely backwards compatible.
3. (easiest) Don't prefix match if the ARN ends with $. Upside is it's completely backwards compatible, and it'll make sense to people used to writing regexes. Downside is that the semantics diverge further from bound_iam_principal_arn.

If you can't get by with just using bound_iam_principal_arn then I think the first option is probably the best. @jefferai @vishalnayak -- thoughts on this?

[jefferai]

I feel ambiguous about all of those options. Not that they're bad, just that I see the pros and cons of each and am not sure. For the moment I'd like to see if your suggestion works for the OP and we can kick this down the road...unless you really feel like tackling this is a worthwhile thing anyways.

[pfhayes]

Any of these solutions would work for me... to be honest the easiest thing to do on my end is just rename the roles :)

Overall the prefix matching seems like quite a "gotcha" that could result in exposing permissions that users may not be aware of. I only noticed it incidentally while consulting the docs for something else. Furthermore someone might create a new IAM role whose name is a prefix of another and silently take on this behavior. for the benefit of those future users the autoupgrade solution seems the safest to me

[joelthompson]

Hey @pfhayes -- just to be explicit, when you say that any of these solutions would work for you, does that also include using the iam auth type and switching to using bound_iam_principal_arn?

Overall the prefix matching seems like quite a "gotcha" that could result in exposing permissions that users may not be aware of. I only noticed it incidentally while consulting the docs for something else. Furthermore someone might create a new IAM role whose name is a prefix of another and silently take on this behavior.

I totally agree, or somebody might create a role for which another role's name is a prefix and accidentally add access. But I'm also sympathetic with @jefferai's desire to kick the can down the road; it's not clear to me that transition cost is worth it.

[pfhayes]

I didn't actually attempt switching to IAM. Switching to a new auth type seemed burdensome enough that I just renamed the roles

[jefferai]

@pfhayes Unless you really don't want ec2 inferencing, you're generally better off using the IAM method as you don't need to deal with whitelists and nonces.

[joelthompson]

Was thinking about this while working on #3907 -- that PR introduces some backwards incompatibilities; if we're already biting the bullet of backwards-incompatible changes, it might make sense to do option 1 above and add another small incompatibility to the mix. The explicit role versioning I included in #3907 should make the internal upgrade path much more straightforward as well. Thoughts @jefferai?

[jefferai]

@joelthompson SGTM