auth/aws: Allow binding by EC2 instance IDs #3816

joelthompson · 2018-01-19T03:35:17Z

This allows specifying a list of EC2 instance IDs that are allowed to
bind to the role. To keep style formatting with the other bindings, this
is still called bound_ec2_instance_id rather than bound_ec2_instance_ids.

Partially fixes #3797

This allows specifying a list of EC2 instance IDs that are allowed to bind to the role. To keep style formatting with the other bindings, this is still called bound_ec2_instance_id rather than bound_ec2_instance_ids as I intend to convert the other bindings to accept lists as well (where it makes sense) and keeping them with singular names would be the easiest for backwards compatibility. Partially fixes hashicorp#3797

joelthompson · 2018-01-19T03:55:00Z

I realize that some of the decisions I made when writing this probably seem a bit questionable at first blush, so I wanted to explain them.

First, I think that eventually we'll want to convert most if not all binds to being TypeCommaStringSplice instead of TypeString. But I'm not sure of the best way to do that -- do we want to create a bunch of duplicate fields in awsRoleEntry, one for singular and one for plural (e.g., BoundAmiId of type string AND BoundAmiIds of type []string)? If we do that, we'd have to pay a one-time upgrade cost on each role and be done with it. But, without more code, it would break backwards compatibility with the API on reading (as the field names would change), and so we might have to pay the cost to re-build a comma-separated string every time the role is read. The second option is to just always split on comma upon reading from storage and re-join on comma whenever writing to storage. There's somewhat higher overhead cost to this, but it maintains backwards compatibility everywhere. I tend to favor the second option, but not sure.

Since I wasn't sure what to do with my first question, and because I wanted to get something out there to solve the request in #3797, I figured I'd try to just address that one request.

So my second decision point was, do I only support a single EC2 instance ID, or do I support multiple from the beginning? I decided on multiple because that's the direction I want to go eventually, and I don't want to half-deliver a feature to @rorybrowne that he needs to hack around.

Third, do I call it bound_ec2_instance_id (singular) or bound_ec2_instance_ids (plural)? Plural seems right, but if I make more bindings accept comma-separated lists (my first point), I'd need to either rename all of them to have plural parameter names (which requires extra effort to maintain backwards compatibility) or just keep the singular names and have them accept multiple values. Again, I think the latter is probably the right call, and if I'm going to do that, then I think the bound_ec2_instance_id parameter name should be singular just to maintain consistency with all other bindings.

Anyway, would love feedback on any of these points, happy to adjust as y'all see fit!

joelthompson · 2018-01-19T04:33:31Z

Also, it looks like Travis is failing for reasons not related to this PR, and running the aws auth backend acceptance tests worked for me when running locally.

In the aws auth method, allow a number of binds to take in lists instead of a single string value. The intended semantic is that, for each bind type set, clients must match at least one of each of the bind types set in order to authenticate.

jefferai · 2018-02-05T16:03:52Z

Hi Joel,

Decision 1: We've been slowly making this exact breaking change across the API for the sake of better consistency. So if converting to TypeCommaStringSlice, which I encourage, then yeah -- pay the one-time upgrade cost, and have the returned value come from the new role entry, and have it be returned as an array. For people that want to configure things programmatically, it's way better to have it support array in -> array out. One thing to note about the upgrades: they need to be protected. See https://github.com/hashicorp/vault/blob/master/builtin/logical/pki/path_roles.go#L319

Decision 2: Sounds good

Decision 3: I agree that plural sounds right but given that the whole backend uses singular, I guess just keep it singular. If we decide to fix this up across all of the parameters we can change this one as well, but in the meantime it keeps things consistent within the same role.

joelthompson · 2018-02-06T01:48:26Z

@jefferai -- since I've gotten to it in the meantime, I'd actually like to get #3907 merged first as it adds in the broader list support, which solve some of these questions. And ack on the upgrade guard, I've added it to #3907

Acceptance tests were failing due to hashicorp#4014 so, as a workaround for now, passing in the identity document and the RSA signature rather than the PKCS7 document.

This reverts commit c342691. The underlying issue causing the need for the workaround has been fixed, and additional testing changes have been added in hashicorp#4031 so this is no longer necessary.

Looks like these changes were dropped from prior commit

…tance_id

joelthompson · 2018-03-03T02:26:25Z

Sorry the commit history is such a mess, but this should be ready for review! :)

I tried (and hope I succeeded!) to keep the singular and plural names consistent with #3907 now that that's merged.

jefferai · 2018-03-14T14:29:53Z

builtin/credential/aws/path_role.go

@@ -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


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

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.

jefferai · 2018-03-14T14:30:17Z

builtin/credential/aws/path_login.go

@@ -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) {


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

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.

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

jefferai · 2018-03-14T14:30:48Z

Two minor comments, LGTM otherwise!

"allowing" is a bit of an awkward word, changing it to specifying to be more accurate.

jefferai · 2018-03-15T16:19:35Z

Thanks!

joelthompson mentioned this pull request Jan 19, 2018

Support AWS EC2 Auth Bound By Instance ID and Security Group #3797

Closed

jefferai added this to the 0.9.3 milestone Jan 19, 2018

jefferai modified the milestones: 0.9.3, 0.9.4 Jan 28, 2018

auth/aws: Allow lists in binds

9439d2d

In the aws auth method, allow a number of binds to take in lists instead of a single string value. The intended semantic is that, for each bind type set, clients must match at least one of each of the bind types set in order to authenticate.

joelthompson added 2 commits February 5, 2018 20:41

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

fb33b63

Add guard around upgrading role entry

8c8c001

joelthompson mentioned this pull request Feb 6, 2018

auth/aws: Allow lists in binds #3907

Merged

2 tasks

joelthompson added 2 commits February 8, 2018 01:34

Respond to PR feedback

2233aba

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

ac4c31e

jefferai modified the milestones: 0.9.4, 0.10 Feb 14, 2018

jefferai modified the milestones: 0.10, 0.9.5 Feb 21, 2018

joelthompson added 11 commits February 21, 2018 23:30

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

ebe9daf

Respond to PR feedback

c109477

Fix acceptance test to use identity doc and RSA sig

c342691

Acceptance tests were failing due to hashicorp#4014 so, as a workaround for now, passing in the identity document and the RSA signature rather than the PKCS7 document.

Add some tests for aws auth list binds

71bd5e1

Revert "Fix acceptance test to use identity doc and RSA sig"

41c364f

This reverts commit c342691. The underlying issue causing the need for the workaround has been fixed, and additional testing changes have been added in hashicorp#4031 so this is no longer necessary.

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

0a2b550

Improve tests

a9d039a

Return empty slices instead of null in aws auth roles

4047948

Update docs

1f3b7f6

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

1b2ee76

Add more docs improvements

0eb5757

Looks like these changes were dropped from prior commit

jefferai and others added 4 commits February 23, 2018 11:12

Merge branch 'master' into auth_aws_bind_list

d0e4532

Merge remote-tracking branch 'origin/master' into auth_aws_bind_list

d32d856

Update docs

0a71e7b

Merge branch 'auth_aws_bind_list' into aws_ec2_bind_by_instance_id

5f3c341

jefferai modified the milestones: 0.9.5, 0.9.6 Feb 28, 2018

Merge remote-tracking branch 'origin/master' into aws_ec2_bind_by_ins…

b14f055

…tance_id

vishalnayak self-assigned this Mar 5, 2018

jefferai reviewed Mar 14, 2018

View reviewed changes

Reword bind/auth_type mismatch error messages

c5c4f19

"allowing" is a bit of an awkward word, changing it to specifying to be more accurate.

jefferai merged commit d349f5b into hashicorp:master Mar 15, 2018

joelthompson deleted the aws_ec2_bind_by_instance_id branch March 15, 2018 18:02

JayH5 mentioned this pull request May 30, 2018

vault_aws_auth_backend_role missing bound_ec2_instance_id parameter hashicorp/terraform-provider-vault#134

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

auth/aws: Allow binding by EC2 instance IDs #3816

auth/aws: Allow binding by EC2 instance IDs #3816

joelthompson commented Jan 19, 2018 •

edited

Loading

joelthompson commented Jan 19, 2018

joelthompson commented Jan 19, 2018

jefferai commented Feb 5, 2018

joelthompson commented Feb 6, 2018

joelthompson commented Mar 3, 2018

jefferai Mar 14, 2018

joelthompson Mar 15, 2018

jefferai Mar 14, 2018

joelthompson Mar 15, 2018

jefferai Mar 15, 2018

jefferai commented Mar 14, 2018

jefferai commented Mar 15, 2018

auth/aws: Allow binding by EC2 instance IDs #3816

auth/aws: Allow binding by EC2 instance IDs #3816

Conversation

joelthompson commented Jan 19, 2018 • edited Loading

joelthompson commented Jan 19, 2018

joelthompson commented Jan 19, 2018

jefferai commented Feb 5, 2018

joelthompson commented Feb 6, 2018

joelthompson commented Mar 3, 2018

jefferai Mar 14, 2018

Choose a reason for hiding this comment

joelthompson Mar 15, 2018

Choose a reason for hiding this comment

jefferai Mar 14, 2018

Choose a reason for hiding this comment

joelthompson Mar 15, 2018

Choose a reason for hiding this comment

jefferai Mar 15, 2018

Choose a reason for hiding this comment

jefferai commented Mar 14, 2018

jefferai commented Mar 15, 2018

joelthompson commented Jan 19, 2018 •

edited

Loading