Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

auth/aws: Allow lists in binds #3907

Merged
merged 20 commits into from
Mar 2, 2018
Merged

auth/aws: Allow lists in binds #3907

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
9439d2d
auth/aws: Allow lists in binds
joelthompson Feb 4, 2018
fb33b63
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 6, 2018
8c8c001
Add guard around upgrading role entry
joelthompson Feb 6, 2018
2233aba
Respond to PR feedback
joelthompson Feb 8, 2018
ac4c31e
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 8, 2018
ebe9daf
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 22, 2018
c109477
Respond to PR feedback
joelthompson Feb 22, 2018
c342691
Fix acceptance test to use identity doc and RSA sig
joelthompson Feb 22, 2018
71bd5e1
Add some tests for aws auth list binds
joelthompson Feb 22, 2018
41c364f
Revert "Fix acceptance test to use identity doc and RSA sig"
joelthompson Feb 23, 2018
0a2b550
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 23, 2018
a9d039a
Improve tests
joelthompson Feb 23, 2018
4047948
Return empty slices instead of null in aws auth roles
joelthompson Feb 23, 2018
1f3b7f6
Update docs
joelthompson Feb 23, 2018
1b2ee76
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 23, 2018
0eb5757
Add more docs improvements
joelthompson Feb 23, 2018
d0e4532
Merge branch 'master' into auth_aws_bind_list
jefferai Feb 23, 2018
d32d856
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 24, 2018
0a71e7b
Update docs
joelthompson Feb 24, 2018
813c27b
Merge remote-tracking branch 'origin/master' into auth_aws_bind_list
joelthompson Feb 28, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
33 changes: 18 additions & 15 deletions builtin/credential/aws/backend_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1084,7 +1084,7 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
"auth_type": "ec2",
"policies": "root",
"max_ttl": "120s",
"bound_ami_id": "wrong_ami_id",
"bound_ami_id": []string{"wrong_ami_id", "wrong_ami_id2"},
"bound_account_id": accountID,
"bound_iam_role_arn": iamARN,
}
Expand All @@ -1108,10 +1108,10 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err)
}

// Place the correct AMI ID, but make the AccountID wrong
// Place the correct AMI ID in one of the values, but make the AccountID wrong
roleReq.Operation = logical.UpdateOperation
data["bound_ami_id"] = amiID
data["bound_account_id"] = "wrong-account-id"
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)
Expand All @@ -1123,9 +1123,9 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err)
}

// Place the correct AccountID, but make the wrong IAMRoleARN
data["bound_account_id"] = accountID
data["bound_iam_role_arn"] = "wrong_iam_role_arn"
// 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)
Expand All @@ -1137,8 +1137,8 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
t.Fatalf("bad: expected error response: resp:%#v\nerr:%v", resp, err)
}

// place the correct IAM role ARN
data["bound_iam_role_arn"] = iamARN
// 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)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("bad: failed to create role: resp:%#v\nerr:%v", resp, err)
Expand Down Expand Up @@ -1456,7 +1456,7 @@ func TestBackendAcc_LoginWithCallerIdentity(t *testing.T) {

// configuring the valid role we'll be able to login to
roleData := map[string]interface{}{
"bound_iam_principal_arn": entity.canonicalArn(),
"bound_iam_principal_arn": []string{entity.canonicalArn(), "arn:aws:iam::123456789012:role/FakeRoleArn1*"}, // Fake ARN MUST be wildcard terminated because we're resolving unique IDs, and the wildcard termination prevents unique ID resolution
"policies": "root",
"auth_type": iamAuthType,
}
Expand Down Expand Up @@ -1489,16 +1489,19 @@ func TestBackendAcc_LoginWithCallerIdentity(t *testing.T) {
}

fakeArn := "arn:aws:iam::123456789012:role/somePath/FakeRole"
fakeArn2 := "arn:aws:iam::123456789012:role/somePath/FakeRole2"
fakeArnResolverCount := 0
fakeArnResolver := func(ctx context.Context, s logical.Storage, arn string) (string, error) {
if arn == fakeArn {
return fmt.Sprintf("FakeUniqueIdFor%s", fakeArn), nil
if strings.HasPrefix(arn, fakeArn) {
fakeArnResolverCount++
return fmt.Sprintf("FakeUniqueIdFor%s%d", arn, fakeArnResolverCount), nil
}
return b.resolveArnToRealUniqueId(context.Background(), s, arn)
}
b.resolveArnToUniqueIDFunc = fakeArnResolver

// now we're creating the invalid role we won't be able to login to
roleData["bound_iam_principal_arn"] = fakeArn
roleData["bound_iam_principal_arn"] = []string{fakeArn, fakeArn2}
roleRequest.Path = "role/" + testInvalidRoleName
resp, err = b.HandleRequest(context.Background(), roleRequest)
if err != nil || (resp != nil && resp.IsError()) {
Expand Down Expand Up @@ -1630,11 +1633,11 @@ func TestBackendAcc_LoginWithCallerIdentity(t *testing.T) {
wildcardRoleName := "valid_wildcard"
wildcardEntity := *entity
wildcardEntity.FriendlyName = "*"
roleData["bound_iam_principal_arn"] = wildcardEntity.canonicalArn()
roleData["bound_iam_principal_arn"] = []string{wildcardEntity.canonicalArn(), "arn:aws:iam::123456789012:role/DoesNotExist/Vault_Fake_Role*"}
roleRequest.Path = "role/" + wildcardRoleName
resp, err = b.HandleRequest(context.Background(), roleRequest)
if err != nil || (resp != nil && resp.IsError()) {
t.Fatalf("bad: failed to create wildcard role: resp:%#v\nerr:%v", resp, err)
t.Fatalf("bad: failed to create wildcard roles: resp:%#v\nerr:%v", resp, err)
}

loginData["role"] = wildcardRoleName
Expand Down
103 changes: 66 additions & 37 deletions builtin/credential/aws/path_login.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -386,7 +386,7 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context,

// Verify that the AccountID of the instance trying to login matches the
// AccountID specified as a constraint on role
if roleEntry.BoundAccountID != "" && identityDoc.AccountID != roleEntry.BoundAccountID {
if len(roleEntry.BoundAccountIDs) > 0 && !strutil.StrListContains(roleEntry.BoundAccountIDs, identityDoc.AccountID) {
return fmt.Errorf("account ID %q does not belong to role %q", identityDoc.AccountID, roleName), nil
}

Expand All @@ -399,54 +399,61 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context,
// already calling the API to validate the Instance ID anyway, so it shouldn't
// matter. The benefit is that we have the exact same code whether auth_type
// is ec2 or iam.
if roleEntry.BoundAmiID != "" {
if len(roleEntry.BoundAmiIDs) > 0 {
if instance.ImageId == nil {
return nil, fmt.Errorf("AMI ID in the instance description is nil")
}
if roleEntry.BoundAmiID != *instance.ImageId {
if !strutil.StrListContains(roleEntry.BoundAmiIDs, *instance.ImageId) {
return fmt.Errorf("AMI ID %q does not belong to role %q", instance.ImageId, roleName), nil
}
}

// Validate the SubnetID if corresponding bound was set on the role
if roleEntry.BoundSubnetID != "" {
if len(roleEntry.BoundSubnetIDs) > 0 {
if instance.SubnetId == nil {
return nil, fmt.Errorf("subnet ID in the instance description is nil")
}
if roleEntry.BoundSubnetID != *instance.SubnetId {
if !strutil.StrListContains(roleEntry.BoundSubnetIDs, *instance.SubnetId) {
return fmt.Errorf("subnet ID %q does not satisfy the constraint on role %q", *instance.SubnetId, roleName), nil
}
}

// Validate the VpcID if corresponding bound was set on the role
if roleEntry.BoundVpcID != "" {
if len(roleEntry.BoundVpcIDs) > 0 {
if instance.VpcId == nil {
return nil, fmt.Errorf("VPC ID in the instance description is nil")
}
if roleEntry.BoundVpcID != *instance.VpcId {
if !strutil.StrListContains(roleEntry.BoundVpcIDs, *instance.VpcId) {
return fmt.Errorf("VPC ID %q does not satisfy the constraint on role %q", *instance.VpcId, roleName), nil
}
}

// Check if the IAM instance profile ARN of the instance trying to
// login, matches the IAM instance profile ARN specified as a constraint
// on the role
if roleEntry.BoundIamInstanceProfileARN != "" {
if len(roleEntry.BoundIamInstanceProfileARNs) > 0 {
if instance.IamInstanceProfile == nil {
return nil, fmt.Errorf("IAM instance profile in the instance description is nil")
}
if instance.IamInstanceProfile.Arn == nil {
return nil, fmt.Errorf("IAM instance profile ARN in the instance description is nil")
}
iamInstanceProfileARN := *instance.IamInstanceProfile.Arn
if !strings.HasPrefix(iamInstanceProfileARN, roleEntry.BoundIamInstanceProfileARN) {
matchesInstanceProfile := false
for _, boundInstanceProfileARN := range roleEntry.BoundIamInstanceProfileARNs {
if strings.HasPrefix(iamInstanceProfileARN, boundInstanceProfileARN) {
matchesInstanceProfile = true
break
}
}
if !matchesInstanceProfile {
return fmt.Errorf("IAM instance profile ARN %q does not satisfy the constraint role %q", iamInstanceProfileARN, roleName), nil
}
}

// Check if the IAM role ARN of the instance trying to login, matches
// the IAM role ARN specified as a constraint on the role.
if roleEntry.BoundIamRoleARN != "" {
if len(roleEntry.BoundIamRoleARNs) > 0 {
if instance.IamInstanceProfile == nil {
return nil, fmt.Errorf("IAM instance profile in the instance description is nil")
}
Expand Down Expand Up @@ -484,7 +491,14 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context,
return nil, fmt.Errorf("IAM role ARN could not be fetched")
}

if !strings.HasPrefix(iamRoleARN, roleEntry.BoundIamRoleARN) {
matchesInstanceRoleARN := false
for _, boundIamRoleARN := range roleEntry.BoundIamRoleARNs {
if strings.HasPrefix(iamRoleARN, boundIamRoleARN) {
matchesInstanceRoleARN = true
break
}
}
if !matchesInstanceRoleARN {
return fmt.Errorf("IAM role ARN %q does not satisfy the constraint role %q", iamRoleARN, roleName), nil
}
}
Expand Down Expand Up @@ -588,7 +602,7 @@ func (b *backend) pathLoginUpdateEc2(ctx context.Context, req *logical.Request,

// Verify that the `Region` of the instance trying to login matches the
// `Region` specified as a constraint on role
if roleEntry.BoundRegion != "" && identityDocParsed.Region != roleEntry.BoundRegion {
if len(roleEntry.BoundRegions) > 0 && !strutil.StrListContains(roleEntry.BoundRegions, identityDocParsed.Region) {
return logical.ErrorResponse(fmt.Sprintf("Region %q does not satisfy the constraint on role %q", identityDocParsed.Region, roleName)), nil
}

Expand Down Expand Up @@ -939,17 +953,20 @@ func (b *backend) pathLoginRenewIam(ctx context.Context, req *logical.Request, d
// read the role directly to know what the bind is. It's a relatively small amount of leakage, in
// some fairly corner cases, and in the most likely error case (role has been changed to a new ARN),
// the error message is identical.
if roleEntry.BoundIamPrincipalARN != "" {
if len(roleEntry.BoundIamPrincipalARNs) > 0 {
// We might not get here if all bindings were on the inferred entity, which we've already validated
// above
// As with logins, there are three ways to pass this check:
// 1: clientUserId is in roleEntry.BoundIamPrincipalIDs (entries in roleEntry.BoundIamPrincipalIDs
// implies that roleEntry.ResolveAWSUniqueIDs is true)
// 2: roleEntry.ResolveAWSUniqueIDs is false and canonical_arn is in roleEntry.BoundIamPrincipalARNs
// 3: Full ARN matches one of the wildcard globs in roleEntry.BoundIamPrincipalARNs
clientUserId, ok := req.Auth.Metadata["client_user_id"]
if ok && roleEntry.BoundIamPrincipalID != "" {
// Resolving unique IDs is enabled and the auth metadata contains the unique ID, so checking the
// unique ID is authoritative at this stage
if roleEntry.BoundIamPrincipalID != clientUserId {
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn)
}
} else if strings.HasSuffix(roleEntry.BoundIamPrincipalARN, "*") {
switch {
case ok && strutil.StrListContains(roleEntry.BoundIamPrincipalIDs, clientUserId): // check 1 passed
case !roleEntry.ResolveAWSUniqueIDs && strutil.StrListContains(roleEntry.BoundIamPrincipalARNs, canonicalArn): // check 2 passed
default:
// check 3 is a bit more complex, so we do it last
fullArn := b.getCachedUserId(clientUserId)
if fullArn == "" {
entity, err := parseIamArn(canonicalArn)
Expand All @@ -967,11 +984,16 @@ func (b *backend) pathLoginRenewIam(ctx context.Context, req *logical.Request, d
b.setCachedUserId(clientUserId, fullArn)
}
}
if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) {
matchedWildcardBind := false
for _, principalARN := range roleEntry.BoundIamPrincipalARNs {
if strings.HasSuffix(principalARN, "*") && strutil.GlobbedStringsMatch(principalARN, fullArn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is prefix matching sufficient here, do you think, or should this support globs anywhere in the ARN?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The prior code only supported prefix matching, so I'm just keeping that.

It's a good question as to whether globs anywhere in the ARN should be supported; I haven't seen any requests for it either on the mailing list or in the issue tracker, and if general wildcards should be supported, let's address that in a separate PR :)

matchedWildcardBind = true
break
}
}
if !matchedWildcardBind {
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn)
}
} else if roleEntry.BoundIamPrincipalARN != canonicalArn {
return nil, fmt.Errorf("role no longer bound to ARN %q", canonicalArn)
}
}

Expand Down Expand Up @@ -1189,15 +1211,19 @@ func (b *backend) pathLoginUpdateIam(ctx context.Context, req *logical.Request,

// The role creation should ensure that either we're inferring this is an EC2 instance
// or that we're binding an ARN
// The only way BoundIamPrincipalID could get set is if BoundIamPrincipalARN was also set and
// resolving to internal IDs was turned on, which can't be turned off. So, there should be no
// way for this to be set and not match BoundIamPrincipalARN
if roleEntry.BoundIamPrincipalID != "" {
if callerUniqueId != roleEntry.BoundIamPrincipalID {
return logical.ErrorResponse(fmt.Sprintf("expected IAM %s %s to resolve to unique AWS ID %q but got %q instead", entity.Type, entity.FriendlyName, roleEntry.BoundIamPrincipalID, callerUniqueId)), nil
}
} else if roleEntry.BoundIamPrincipalARN != "" {
if strings.HasSuffix(roleEntry.BoundIamPrincipalARN, "*") {
if len(roleEntry.BoundIamPrincipalARNs) > 0 {
// As with renews, there are three ways to pass this check:
// 1: callerUniqueId is in roleEntry.BoundIamPrincipalIDs (entries in roleEntry.BoundIamPrincipalIDs
// implies that roleEntry.ResolveAWSUniqueIDs is true)
// 2: roleEntry.ResolveAWSUniqueIDs is false and entity.canonicalArn() is in roleEntry.BoundIamPrincipalARNs
// 3: Full ARN matches one of the wildcard globs in roleEntry.BoundIamPrincipalARNs
// Need to be able to handle pathological configurations such as roleEntry.BoundIamPrincipalARNs looking something like:
// arn:aw:iam::123456789012:{user/UserName,user/path/*,role/RoleName,role/path/*}
switch {
case strutil.StrListContains(roleEntry.BoundIamPrincipalIDs, callerUniqueId): // check 1 passed
case !roleEntry.ResolveAWSUniqueIDs && strutil.StrListContains(roleEntry.BoundIamPrincipalARNs, entity.canonicalArn()): // check 2 passed
default:
// evaluate check 3
fullArn := b.getCachedUserId(callerUniqueId)
if fullArn == "" {
fullArn, err = b.fullArn(ctx, entity, req.Storage)
Expand All @@ -1209,13 +1235,16 @@ func (b *backend) pathLoginUpdateIam(ctx context.Context, req *logical.Request,
}
b.setCachedUserId(callerUniqueId, fullArn)
}
if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) {
// Note: Intentionally giving the exact same error message as a few lines below. Otherwise, we might leak information
// about whether the bound IAM principal ARN is a wildcard or not, and what that wildcard is.
matchedWildcardBind := false
for _, principalARN := range roleEntry.BoundIamPrincipalARNs {
if strings.HasSuffix(principalARN, "*") && strutil.GlobbedStringsMatch(principalARN, fullArn) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same question here.

matchedWildcardBind = true
break
}
}
if !matchedWildcardBind {
return logical.ErrorResponse(fmt.Sprintf("IAM Principal %q does not belong to the role %q", callerID.Arn, roleName)), nil
}
} else if roleEntry.BoundIamPrincipalARN != entity.canonicalArn() {
return logical.ErrorResponse(fmt.Sprintf("IAM Principal %q does not belong to the role %q", callerID.Arn, roleName)), nil
}
}

Expand Down
Loading