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.

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 binding by EC2 instance IDs #3816

Merged
merged 23 commits into from
Mar 15, 2018
Merged
Show file tree
Hide file tree
Changes from 22 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
650f3d8
auth/aws: Allow binding by EC2 instance IDs
joelthompson Jan 19, 2018
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
5f3c341
Merge branch 'auth_aws_bind_list' into aws_ec2_bind_by_instance_id
joelthompson Feb 24, 2018
b14f055
Merge remote-tracking branch 'origin/master' into aws_ec2_bind_by_ins…
joelthompson Mar 3, 2018
c5c4f19
Reword bind/auth_type mismatch error messages
joelthompson Mar 15, 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
34 changes: 28 additions & 6 deletions builtin/credential/aws/backend_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1070,6 +1070,11 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
"nonce": "vault-client-nonce",
}

parsedIdentityDoc, err := b.parseIdentityDocument(context.Background(), storage, pkcs7)
if err != nil {
t.Fatal(err)
}

// Perform the login operation with a AMI ID that is not matching
// the bound on the role.
loginRequest := &logical.Request{
Expand All @@ -1081,12 +1086,13 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.

// Place the wrong AMI ID in the role data.
data := map[string]interface{}{
"auth_type": "ec2",
"policies": "root",
"max_ttl": "120s",
"bound_ami_id": []string{"wrong_ami_id", "wrong_ami_id2"},
"bound_account_id": accountID,
"bound_iam_role_arn": iamARN,
"auth_type": "ec2",
"policies": "root",
"max_ttl": "120s",
"bound_ami_id": []string{"wrong_ami_id", "wrong_ami_id2"},
"bound_account_id": accountID,
"bound_iam_role_arn": iamARN,
"bound_ec2_instance_id": []string{parsedIdentityDoc.InstanceID, "i-1234567"},
}

roleReq := &logical.Request{
Expand Down Expand Up @@ -1139,6 +1145,19 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.

// place a correct IAM role ARN
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)
}

// place a correct EC2 Instance ID
data["bound_ec2_instance_id"] = []string{parsedIdentityDoc.InstanceID, "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)
Expand Down Expand Up @@ -1170,6 +1189,9 @@ func TestBackendAcc_LoginWithInstanceIdentityDocAndWhitelistIdentity(t *testing.
if instanceID == "" {
t.Fatalf("instance ID not present in the response object")
}
if instanceID != parsedIdentityDoc.InstanceID {
t.Fatalf("instance ID in response (%q) did not match instance ID from identity document (%q)", instanceID, parsedIdentityDoc.InstanceID)
}

_, ok := resp.Auth.Metadata["nonce"]
if ok {
Expand Down
5 changes: 5 additions & 0 deletions builtin/credential/aws/path_login.go
Original file line number Diff line number Diff line change
Expand Up @@ -384,6 +384,11 @@ func (b *backend) verifyInstanceMeetsRoleRequirements(ctx context.Context,
return nil, fmt.Errorf("nil identityDoc")
}

// Verify that the instance ID matches one of the ones set by the role
if len(roleEntry.BoundEc2InstanceIDs) > 0 && !strutil.StrListContains(roleEntry.BoundEc2InstanceIDs, *instance.InstanceId) {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason you explicitly dereference the pointer here and in the next line?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I dereference it here so I don't get a compiler error:

$ make test TEST=./builtin/credential/aws/
go generate 
# github.com/hashicorp/vault/builtin/credential/aws
/.../go/src/github.com/hashicorp/vault/builtin/credential/aws/path_login.go:388:111: cannot use instance.InstanceId (type *string) as type string in argument to strutil.StrListContains
FAIL    github.com/hashicorp/vault/builtin/credential/aws [build failed]
Makefile:34: recipe for target 'test' failed
make: *** [test] Error 2

I dereference it in the next line so that it shows up as expected in the error message.

Copy link
Member

Choose a reason for hiding this comment

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

Oh, forgot about how it's all pointers in AWS land. Sounds good!

return fmt.Errorf("instance ID %q does not belong to the role %q", *instance.InstanceId, roleName), nil
}

// Verify that the AccountID of the instance trying to login matches the
// AccountID specified as a constraint on role
if len(roleEntry.BoundAccountIDs) > 0 && !strutil.StrListContains(roleEntry.BoundAccountIDs, identityDoc.AccountID) {
Expand Down
20 changes: 20 additions & 0 deletions builtin/credential/aws/path_role.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,13 @@ 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 is only applicable when
auth_type is ec2 or inferred_entity_type is ec2_instance.`,
},
"bound_ec2_instance_id": {
Type: framework.TypeCommaStringSlice,
Description: `If set, defines a constraint on the EC2 instances to have one of the
given instance IDs. Can be a list or comma-separated string of EC2 instance
IDs. This is only applicable when auth_type is ec2 or inferred_entity_type is
ec2_instance.`,
},
"resolve_aws_unique_ids": {
Type: framework.TypeBool,
Expand Down Expand Up @@ -548,6 +555,10 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request
roleEntry.BoundIamInstanceProfileARNs = boundIamInstanceProfileARNRaw.([]string)
}

if boundEc2InstanceIDRaw, ok := data.GetOk("bound_ec2_instance_id"); ok {
roleEntry.BoundEc2InstanceIDs = boundEc2InstanceIDRaw.([]string)
}

if boundIamPrincipalARNRaw, ok := data.GetOk("bound_iam_principal_arn"); ok {
principalARNs := boundIamPrincipalARNRaw.([]string)
roleEntry.BoundIamPrincipalARNs = principalARNs
Expand Down Expand Up @@ -647,6 +658,13 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request
numBinds++
}

if len(roleEntry.BoundEc2InstanceIDs) > 0 {
if !allowEc2Binds {
return logical.ErrorResponse(fmt.Sprintf("specified bound_ec2_instance_id but not allowing ec2 auth_type or inferring %s", ec2EntityType)), nil
Copy link
Member

Choose a reason for hiding this comment

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

This language is slightly awkward in that you don't "allow" ec2 auth type.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's a little awkward, but that's exactly what all the other error messages say. I've updated everywhere to use specifying instead of allowing, I think that's more accurate.

}
numBinds++
}

if len(roleEntry.BoundIamRoleARNs) > 0 {
if !allowEc2Binds {
return logical.ErrorResponse(fmt.Sprintf("specified bound_iam_role_arn but not allowing ec2 auth_type or inferring %s", ec2EntityType)), nil
Expand Down Expand Up @@ -794,6 +812,7 @@ type awsRoleEntry struct {
AuthType string `json:"auth_type" `
BoundAmiIDs []string `json:"bound_ami_id_list"`
BoundAccountIDs []string `json:"bound_account_id_list"`
BoundEc2InstanceIDs []string `json:"bound_ec2_instance_id_list"`
BoundIamPrincipalARNs []string `json:"bound_iam_principal_arn_list"`
BoundIamPrincipalIDs []string `json:"bound_iam_principal_id_list"`
BoundIamRoleARNs []string `json:"bound_iam_role_arn_list"`
Expand Down Expand Up @@ -830,6 +849,7 @@ func (r *awsRoleEntry) ToResponseData() map[string]interface{} {
"auth_type": r.AuthType,
"bound_ami_id": r.BoundAmiIDs,
"bound_account_id": r.BoundAccountIDs,
"bound_ec2_instance_id": r.BoundEc2InstanceIDs,
"bound_iam_principal_arn": r.BoundIamPrincipalARNs,
"bound_iam_principal_id": r.BoundIamPrincipalIDs,
"bound_iam_role_arn": r.BoundIamRoleARNs,
Expand Down
2 changes: 2 additions & 0 deletions builtin/credential/aws/path_role_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -570,6 +570,7 @@ func TestAwsEc2_RoleCrud(t *testing.T) {
"bound_iam_instance_profile_arn": "arn:aws:iam::123456789012:instance-profile/MyInstanceProfile",
"bound_subnet_id": "testsubnetid",
"bound_vpc_id": "testvpcid",
"bound_ec2_instance_id": "i-12345678901234567,i-76543210987654321",
"role_tag": "testtag",
"resolve_aws_unique_ids": false,
"allow_instance_migration": true,
Expand Down Expand Up @@ -600,6 +601,7 @@ func TestAwsEc2_RoleCrud(t *testing.T) {
"bound_ami_id": []string{"testamiid"},
"bound_account_id": []string{"testaccountid"},
"bound_region": []string{"testregion"},
"bound_ec2_instance_id": []string{"i-12345678901234567", "i-76543210987654321"},
"bound_iam_principal_arn": []string{},
"bound_iam_principal_id": []string{},
"bound_iam_role_arn": []string{"arn:aws:iam::123456789012:role/MyRole"},
Expand Down
5 changes: 5 additions & 0 deletions website/source/api/auth/aws/index.html.md
Original file line number Diff line number Diff line change
Expand Up @@ -588,6 +588,10 @@ list in order to satisfy that constraint.
prefix-matched (as though it were a glob ending in `*`). 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
EC2 instances to have one of these instance IDs. 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.
- `role_tag` `(string: "")` - If set, enables the role tags for this role. The
value set for this field should be the 'key' of the tag on the EC2 instance.
The 'value' of the tag should be generated using `role/<role>/tag` endpoint.
Expand Down Expand Up @@ -681,6 +685,7 @@ list in order to satisfy that constraint.
```json
{
"bound_ami_id": ["ami-fce36987"],
"bound_ec2_instance_id": ["i-12345678901234567"],
"role_tag": "",
"policies": [
"default",
Expand Down