auth/aws: Allow lists in binds #3907

joelthompson · 2018-02-04T04:33:14Z

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.

There are still some gaps, but I think it's close enough that I'd like to get feedback from the Vault team about my overall direction before continuing. Of note, this does have a minor backwards incompatibility in that the bind_* properties that now can be lists, and so they will be lists when the role is read from clients, rather than a string, so clients need to be updated to expect a list return value.

Things remaining for me to do once my above questions are figured out:

Improve tests to cover various use cases
Update docs

Fixes #3330
Fixes #1975

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 · 2018-02-06T01:53:22Z

@jefferai -- RE: the comment about guarding the role upgrading code in #3816 (comment) I only added the guard in the lockedAWSRole method and not also in the pathRoleCreateUpdate method as I (naively) assume creating/updating roles would already be protected in secondary mode. Let me know if that's incorrect.

jefferai · 2018-02-07T15:33:19Z

builtin/credential/aws/path_login.go

-			if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) {
+			matchedWildcardBind := false
+			for _, principalARN := range roleEntry.BoundIamPrincipalARNs {
+				if strings.HasSuffix(principalARN, "*") && strutil.GlobbedStringsMatch(principalARN, fullArn) {


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

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 :)

jefferai · 2018-02-07T15:35:42Z

builtin/credential/aws/path_login.go

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


Same question here.

jefferai · 2018-02-07T15:44:59Z

builtin/credential/aws/path_role.go

@@ -436,7 +499,9 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request
 		return nil, err
 	}
 	if roleEntry == nil {
-		roleEntry = &awsRoleEntry{}
+		roleEntry = &awsRoleEntry{
+			version: currentRoleStorageVersion,


This value can't be unexported, because JSON can't marshal/unmarshal if it is. So it will end up always being 0 on read.

Good call out. I didn't want to return this to clients, and that was before I stole the ToResponseData method, so I've now exported it.

jefferai · 2018-02-07T15:46:28Z

builtin/credential/aws/path_role.go

-			roleEntry.BoundIamPrincipalID = principalID
-		} else {
-			// Need to handle the case where we're switching from a non-wildcard principal to a wildcard principal
-			roleEntry.BoundIamPrincipalID = ""


Is this case still being handled? Does it need to be?

I believe it does the right thing.

Before, the code needed to handle switching from non-widlcard to a wildcard. Now, however, it needs to handle switching from a list containing some widlcard and some non-wildcard ARNs, to a different list containing some (possibly same and possibly different) wildcard ARNs and some (possibly same and possibly different) non-wildcard ARNs. So this is no longer a special case.

It's handled by calling roleEntry.BoundIamPrincipalIDs = []string{} whenever bound_iam_principal_arn is explicitly set and then building up the BoundIamPrincipalIDs for each non-wildcard principal.

jefferai · 2018-02-07T15:46:55Z

builtin/credential/aws/path_role.go

+	DisallowReauthentication    bool          `json:"disallow_reauthentication" structs:"disallow_reauthentication" mapstructure:"disallow_reauthentication"`
+	HMACKey                     string        `json:"hmac_key" structs:"hmac_key" mapstructure:"hmac_key"`
+	Period                      time.Duration `json:"period" mapstructure:"period" structs:"period"`
+	version                     int           `json:"version" mapstructure:"version" structs:"versoin"`


Needs to be exported.

Also, there is a typo in the structs tag.

Also, if structs isn't being used anymore, which it seems it isn't, and which is my preference, just remove the structs tags :-D

done, including removing the structs tags entirely from this type.

Honestly, was never 100% sure why structs and mapstructure tags were included and I just kept including them because they had been included. If they're not needed, I'm also 100% in favor of removing them :)

jefferai · 2018-02-07T15:48:45Z

builtin/credential/aws/path_role.go

+			data[field] = []string{}
+		}
+	}
+	convertNilToEmptySlice(responseData, "bound_ami_id")


Is this really necessary? The JSON marshaling code ought to see that it's a nil slice and do the right thing.

In my testing, yes (it was a surprise to me as well). https://stackoverflow.com/a/44305910 is the best explanation I've seen of the differences between a nil slice and an empty slice. The former gets marshaled to null while the later gets marshaled to [] which makes sense to me.

null is in fact the right thing, although it's untyped; or omitempty to just not actually include it at all. But if you think it's useful to return real data, keep it as is.

I'm all in favor of less code, and it seems likely that this might get forgotten to get updated if/when additional fields are added, so removed and will return null :)

jefferai

Some minor comments, mostly looking great!

joelthompson

Thanks! Will try to get to docs and tests tomorrow.

joelthompson · 2018-02-08T06:01:17Z

builtin/credential/aws/path_login.go

-			if !strutil.GlobbedStringsMatch(roleEntry.BoundIamPrincipalARN, fullArn) {
+			matchedWildcardBind := false
+			for _, principalARN := range roleEntry.BoundIamPrincipalARNs {
+				if strings.HasSuffix(principalARN, "*") && strutil.GlobbedStringsMatch(principalARN, fullArn) {


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 :)

joelthompson · 2018-02-08T06:07:28Z

builtin/credential/aws/path_role.go

@@ -436,7 +499,9 @@ func (b *backend) pathRoleCreateUpdate(ctx context.Context, req *logical.Request
 		return nil, err
 	}
 	if roleEntry == nil {
-		roleEntry = &awsRoleEntry{}
+		roleEntry = &awsRoleEntry{
+			version: currentRoleStorageVersion,


Good call out. I didn't want to return this to clients, and that was before I stole the ToResponseData method, so I've now exported it.

joelthompson · 2018-02-08T06:17:09Z

builtin/credential/aws/path_role.go

-			roleEntry.BoundIamPrincipalID = principalID
-		} else {
-			// Need to handle the case where we're switching from a non-wildcard principal to a wildcard principal
-			roleEntry.BoundIamPrincipalID = ""


I believe it does the right thing.

Before, the code needed to handle switching from non-widlcard to a wildcard. Now, however, it needs to handle switching from a list containing some widlcard and some non-wildcard ARNs, to a different list containing some (possibly same and possibly different) wildcard ARNs and some (possibly same and possibly different) non-wildcard ARNs. So this is no longer a special case.

It's handled by calling roleEntry.BoundIamPrincipalIDs = []string{} whenever bound_iam_principal_arn is explicitly set and then building up the BoundIamPrincipalIDs for each non-wildcard principal.

joelthompson · 2018-02-08T06:24:14Z

builtin/credential/aws/path_role.go

+	DisallowReauthentication    bool          `json:"disallow_reauthentication" structs:"disallow_reauthentication" mapstructure:"disallow_reauthentication"`
+	HMACKey                     string        `json:"hmac_key" structs:"hmac_key" mapstructure:"hmac_key"`
+	Period                      time.Duration `json:"period" mapstructure:"period" structs:"period"`
+	version                     int           `json:"version" mapstructure:"version" structs:"versoin"`


done, including removing the structs tags entirely from this type.

Honestly, was never 100% sure why structs and mapstructure tags were included and I just kept including them because they had been included. If they're not needed, I'm also 100% in favor of removing them :)

joelthompson · 2018-02-08T06:34:15Z

builtin/credential/aws/path_role.go

+			data[field] = []string{}
+		}
+	}
+	convertNilToEmptySlice(responseData, "bound_ami_id")


In my testing, yes (it was a surprise to me as well). https://stackoverflow.com/a/44305910 is the best explanation I've seen of the differences between a nil slice and an empty slice. The former gets marshaled to null while the later gets marshaled to [] which makes sense to me.

vishalnayak · 2018-02-08T16:34:33Z

builtin/credential/aws/path_role.go

-	HMACKey                    string        `json:"hmac_key" structs:"hmac_key" mapstructure:"hmac_key"`
-	Period                     time.Duration `json:"period" mapstructure:"period" structs:"period"`
+	AuthType                    string        `json:"auth_type" mapstructure:"auth_type"`
+	BoundAmiID                  string        `json:"bound_ami_id,omitempty" mapstructure:"bound_ami_id"`


Can we segregate the old string fields at the bottom of the struct and have a "// Deprecated" comment on them. This is just to improve readability.

Good call out :) done

vishalnayak · 2018-02-08T16:55:38Z

builtin/credential/aws/path_role.go

-		roleEntry.BoundIamPrincipalID = principalId
-		upgraded = true
+		roleEntry.Version = 1
+		fallthrough


Why do we need a fallthrough here?

Right now, we don't. But, it's more future safe. The idea is that each case would upgrade the role by only a single version for simplicity (I started trying to tear my hair out trying to think about different upgrade scenarios and ordering, which is why I went ahead and implemented this versioning scheme), so, in this case, from 0 to 1. Now, let's say we have a version 2 (which I've conceptually proposed in a separate issue). This case would upgrade the role from version 0 to version 1. We'd still need to upgrade the role to version 2, which is what the fallthrough would achieve. We don't need to rely on whoever adds the case 2 remembering to add the fallthrough to the version 0 case; it's already there.

Sounds good.

vishalnayak · 2018-02-08T17:04:27Z

builtin/credential/aws/path_role.go

-		principalID, err := b.resolveArnToUniqueIDFunc(ctx, req.Storage, roleEntry.BoundIamPrincipalARN)
-		if err != nil {
-			return logical.ErrorResponse(fmt.Sprintf("unable to resolve ARN %#v to internal ID: %#v", roleEntry.BoundIamPrincipalARN, err)), nil
+	} else if roleEntry.ResolveAWSUniqueIDs && len(roleEntry.BoundIamPrincipalIDs) == 0 {


I wonder if this should be an else if block or should it be an independent if. Don't we want to resolve the ARNs in the role anyways?

Yep, realized that I could reduce code duplication with this and the above block by making it an independent if so I've done that :)

vishalnayak · 2018-02-08T17:08:11Z

builtin/credential/aws/path_role.go

+func (r *awsRoleEntry) ToResponseData() map[string]interface{} {
+	responseData := map[string]interface{}{
+		"auth_type":                      r.AuthType,
+		"bound_ami_id":                   r.BoundAmiIDs,


Although I see the reason as to why the keys in the response map to the older fields and the values to the newer, I am feeling a little off about it.

The other option that is bugging me is to return both the singular and slice keys with deprecation notice for the former.

@jefferai Thoughts?

Never mind. Ignore this comment.

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.

joelthompson · 2018-02-22T08:17:23Z

I think I've got the code in a good spot and I've started adding some tests, but I want to take some time to re-review the tests and possibly improve the testing. I also haven't updated any of the docs yet.

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

joelthompson · 2018-02-23T05:19:47Z

@jefferai @vishalnayak -- I think I've responded to all your feedback thus far and I've got reasonable tests and documentation, so this should be ready for you to review when you get a chance :)

It believe (but am not sure) that Travis failure is unrelated. Best guess, it looks to be unhappy because f54832b didn't revert the changes to helper/certutil/certutil_test.go which added calls to x509.MarshalPKCS8PrivateKey (new in Go 1.10).

vishalnayak · 2018-02-23T14:33:47Z

website/source/api/auth/aws/index.html.md

@@ -548,52 +550,63 @@ inferencing configuration of that role.
  "ec2"). Only those bindings applicable to the auth type chosen will be allowed
  to be configured on the role.
 - `bound_ami_id` `(string: "")` - If set, defines a constraint on the EC2


s/(string: "")/(list: []) for all the fields that are upgraded to TypeStringCommaSlice

done (FWIW, I just copied what was done for the PKI secret backend, e..g, http://localhost:4567/api/secret/pki/index.html#ou-1).

vishalnayak

One minor comment on the docs. Other than that this LGTM!

joelthompson added 3 commits February 3, 2018 23:27

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.

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

Merged

jefferai reviewed Feb 7, 2018

View reviewed changes

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

joelthompson commented Feb 8, 2018

View reviewed changes

vishalnayak reviewed Feb 8, 2018

View reviewed changes

joelthompson mentioned this pull request Feb 13, 2018

Permit non-prefix matching for bound_iam_role_arn #3261

Closed

jefferai added this to the 0.10 milestone Feb 14, 2018

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

joelthompson added 4 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

joelthompson added 2 commits February 22, 2018 21:05

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

joelthompson added 4 commits February 22, 2018 23:16

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

joelthompson changed the title ~~WIP: auth/aws: Allow lists in binds~~ auth/aws: Allow lists in binds Feb 23, 2018

Add more docs improvements

0eb5757

Looks like these changes were dropped from prior commit

vishalnayak reviewed Feb 23, 2018

View reviewed changes

vishalnayak previously approved these changes Feb 23, 2018

View reviewed changes

jefferai and others added 3 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

joelthompson dismissed vishalnayak’s stale review via 0a71e7b February 24, 2018 01:50

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

813c27b

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

jefferai approved these changes Mar 2, 2018

View reviewed changes

jefferai merged commit 8a115c7 into hashicorp:master Mar 2, 2018

joelthompson deleted the auth_aws_bind_list branch March 2, 2018 16:10

paddycarver mentioned this pull request Apr 9, 2018

EC2 constraints for vault_aws_auth_backend_role should be lists hashicorp/terraform-provider-vault#92

Closed

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

auth/aws: Allow lists in binds #3907

auth/aws: Allow lists in binds #3907

Conversation

joelthompson commented Feb 4, 2018 • edited Loading

joelthompson commented Feb 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

joelthompson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joelthompson commented Feb 22, 2018 • edited Loading

joelthompson commented Feb 23, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vishalnayak left a comment

Choose a reason for hiding this comment

joelthompson commented Feb 4, 2018 •

edited

Loading

joelthompson commented Feb 22, 2018 •

edited

Loading