Make AWS credential types more explicit #4360

joelthompson · 2018-04-15T00:37:45Z

The AWS secret engine had a lot of confusing overloading with role
paramemters and how they mapped to each of the three credential types
supported. This now adds parameters to remove the overloading while
maintaining backwards compatibility.

With the change, it also becomes easier to add other feature requests.
Attaching multiple managed policies to IAM users and adding a policy
document to STS AssumedRole credentials is now also supported.

Have not yet added the TypeJSON as suggested in #4229. Wanted to get this out and get some feedback first. I also haven't updated the UI; investigating that now.

I plan on addressing the validation suggested in #2302 in a separate PR, this is already big enough on its own :)

Fixes #4229
Fixes #3751
Fixes #2817

The AWS secret engine had a lot of confusing overloading with role paramemters and how they mapped to each of the three credential types supported. This now adds parameters to remove the overloading while maintaining backwards compatibility. With the change, it also becomes easier to add other feature requests. Attaching multiple managed policies to IAM users and adding a policy document to STS AssumedRole credentials is now also supported. Fixes hashicorp#4229 Fixes hashicorp#3751 Fixes hashicorp#2817

This allows unsetting the policy_document by passing in an empty string. Previously, it would fail because the empty string isn't a valid JSON document.

tyrannosaurus-becks

Looks great! None of my comments are blockers.

tyrannosaurus-becks · 2018-04-25T22:41:01Z

builtin/logical/aws/path_roles.go

+	return logical.ListResponse(append(entries, legacyEntries...)), nil
+}
+
+func (b *backend) pathRolesDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {


I notice that some of these framework.OperationFunc are methods of the back end, and some aren't. Some do need the b to do stuff, and others don't. Would it sense to have all of them be methods of the back end for consistency? Or to only attach them when they need something off the back end?

Personally, I would make them all be methods of the backend. However, I'm not the most familiar with golang idioms, so I figured I'd leave it the way I found it, and also to not add additional things to this PR. I'd be happy to submit a follow up to make this change, though.

Ah! I didn't realize it was already that way. It's fine the way it is, I think. I don't think there's a "right" way to do it, I've never seen anything like that in the style guides. :-)

tyrannosaurus-becks · 2018-04-25T22:53:16Z

builtin/logical/aws/path_roles.go

+	if r.InvalidData != "" {
+		respData["invalid_data"] = r.InvalidData
+	}
+	return respData


Am I right in presuming it's intentional that prohibit_flexible_cred_path and version are omitted?

Yep. version is purely an implementation detail about how the role data is stored in the internal storage, to make upgrading the way roles are stored easier in the future. prohibit_flexible_cred_path is also an implementation detail to securely enable backwards compatibility and also unify the /creds and /sts path. My thinking is, if clients are using the "old" parameters in defining the role, then they'll use the same /creds or /sts path used before to retrieve credentials. And if they're using the "new" parameters introduced in this PR, then it won't matter which of the two endpoints are used.

Of course, this has the potential to add confusion. I think omitting these is the right balance, but I'd love to hear any suggestions on how it could be improved! :)

👍 That sounds good!

tyrannosaurus-becks · 2018-04-25T22:54:29Z

builtin/logical/aws/path_user.go

 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 	"github.com/mitchellh/mapstructure"
 )

 func pathUser(b *backend) *framework.Path {
 	return &framework.Path{
-		Pattern: "creds/" + framework.GenericNameRegex("name"),
+		Pattern: "(creds|sts)/" + framework.GenericNameRegex("name"),


Clever way to handle those still using sts!

tyrannosaurus-becks · 2018-04-25T23:02:49Z

builtin/logical/aws/secret_access_keys.go

@@ -234,7 +236,7 @@ func (b *backend) secretAccessKeysCreate(
 		"security_token": nil,
 	}, map[string]interface{}{
 		"username": username,
-		"policy":   policy,
+		"policy":   role,


Should this also be role.PolicyDocument?

That's a good question. Previously, the policy variable was overloaded to be either the policy document or the managed policy ARN, and regardless of which one it was, it was getting stuck in the metadata. Now that an IAM user can have both a policy document and managed policy ARNs, it didn't seem right to pick one or the other to put in the metadata, but rather to put both in so as to not lose information. But, I'm open to suggestions about a better way of doing this!

joelthompson

Thanks! I also left an open question in #4229 (comment) about whether the parameter in defining a role should be credential_type or credential_types. I did the former, but I'm starting to lean more towards the latter and would like to know if there are other opinions.

joelthompson · 2018-04-27T02:44:21Z

builtin/logical/aws/secret_access_keys.go

@@ -234,7 +236,7 @@ func (b *backend) secretAccessKeysCreate(
 		"security_token": nil,
 	}, map[string]interface{}{
 		"username": username,
-		"policy":   policy,
+		"policy":   role,


That's a good question. Previously, the policy variable was overloaded to be either the policy document or the managed policy ARN, and regardless of which one it was, it was getting stuck in the metadata. Now that an IAM user can have both a policy document and managed policy ARNs, it didn't seem right to pick one or the other to put in the metadata, but rather to put both in so as to not lose information. But, I'm open to suggestions about a better way of doing this!

joelthompson · 2018-04-27T03:35:44Z

builtin/logical/aws/path_user.go

 	"github.com/hashicorp/vault/logical"
 	"github.com/hashicorp/vault/logical/framework"
 	"github.com/mitchellh/mapstructure"
 )

 func pathUser(b *backend) *framework.Path {
 	return &framework.Path{
-		Pattern: "creds/" + framework.GenericNameRegex("name"),
+		Pattern: "(creds|sts)/" + framework.GenericNameRegex("name"),


joelthompson · 2018-04-27T03:38:27Z

builtin/logical/aws/path_roles.go

+	return logical.ListResponse(append(entries, legacyEntries...)), nil
+}
+
+func (b *backend) pathRolesDelete(ctx context.Context, req *logical.Request, d *framework.FieldData) (*logical.Response, error) {


Personally, I would make them all be methods of the backend. However, I'm not the most familiar with golang idioms, so I figured I'd leave it the way I found it, and also to not add additional things to this PR. I'd be happy to submit a follow up to make this change, though.

joelthompson · 2018-04-27T03:43:23Z

builtin/logical/aws/path_roles.go

+	if r.InvalidData != "" {
+		respData["invalid_data"] = r.InvalidData
+	}
+	return respData


Yep. version is purely an implementation detail about how the role data is stored in the internal storage, to make upgrading the way roles are stored easier in the future. prohibit_flexible_cred_path is also an implementation detail to securely enable backwards compatibility and also unify the /creds and /sts path. My thinking is, if clients are using the "old" parameters in defining the role, then they'll use the same /creds or /sts path used before to retrieve credentials. And if they're using the "new" parameters introduced in this PR, then it won't matter which of the two endpoints are used.

Of course, this has the potential to add confusion. I think omitting these is the right balance, but I'd love to hear any suggestions on how it could be improved! :)

chrishoffman

This is looking really good! I like the simplification of the api.

My only real concern is the upgrade on write scenario for roles. I think this can be simplified quite a bit by just upgrading the entry on read and doing the right thing with the upsert. It is one more storage write but I think that is ok. Even if there is a failure lower, the role will already be upgraded. There are also some scenarios where the policy may get deleted during the upgrade and fail further down meaning the entry is gone.

chrishoffman · 2018-05-08T14:19:51Z

builtin/logical/aws/path_roles.go

+		return logical.ErrorResponse("missing role name"), nil
+	}
+
+	b.roleMutex.Lock()


I think you can remove the mutex on write and just use the read role method with the lock to upgrade the record and return the updated entry. If the entry is nil, you an still default it to and empty role.

The reason for the mutex is to prevent race conditions when multiple write entries are made at the same time on the HTTP endpoint. Or is this something I don't need to worry about?

We don't guarantee order of requests and the upgrade itself should lock, so it is safe to not worry about locks for the http requests.

Do you mean the upgrade of the role entry from legacy to current should take the lock? I still don't think that's safe and we still need the lock. Imagine the following scenario:

Request 1 comes in to change the policy_document paramemter of an existing, already-upgraded role

Request 2 comes in at the same time to change the role_arns of the same role.

Request 1 and Request 2 both read the same, old role data

Request 1 and Request 2 both modify the same role in different ways

Request 1 and Request 2 will race to write different role data to the storage backend, and the loser of the race will have the request lost

If the semantics of the write involved writing the entirety of the role at once, it would be fine. But since the semantics also allow modifying only some parameters of the role in place, we need a write lock here.

Or am I missing something else?

I think it's fine...I don't anticipate that this is going to be called a ton so contention shouldn't be bad.

chrishoffman · 2018-05-08T14:22:14Z

builtin/logical/aws/path_roles.go

+func upgradeLegacyPolicyEntry(entry string) *awsRoleEntry {
+	var newRoleEntry *awsRoleEntry
+	if strings.HasPrefix(entry, "arn:") {
+		arnRegex := regexp.MustCompile(`^arn:aws[a-z-]*:iam::(?P<account_num>\d{12}|aws):(?P<entity_type>\w+)/(?P<role_path>.+)$`)


It looks like the AWS SDK has a ARN parser. Can this be used?

https://docs.aws.amazon.com/sdk-for-go/api/aws/arn/#Parse

Yes! I hadn't seen that before, looks perfect. It requires adding it to the vendor; I've pulled that into a separate PR to keep this cleaner: #5048

chrishoffman · 2018-05-08T14:33:02Z

builtin/logical/aws/path_roles.go

+		return roleEntry, nil
+	}
+
+	b.roleMutex.Lock()


After obtaining the write lock, you will want to check if the role again to make sure it has not been upgraded.

Good catch. I really wish golang had a native concept of upgradable locks :/

chrishoffman · 2018-05-08T14:36:03Z

builtin/logical/aws/path_roles.go

+			},
+
+			"policy_document": &framework.FieldSchema{
+				Type: framework.TypeString, // TODO: Investigate adding a TypeJSON


I'm not sure a TypeJSON is going to be possible since you will need to know the struct you are unmarshalling to. I think TypeString is fine here and validation just moves to the path handler.

Yeah, @jefferai had suggested it a little while ago, and it would really make this more user friendly, but it'll probably require a bit of gymnastics to make it work robustly.

chrishoffman · 2018-05-08T14:40:36Z

builtin/logical/aws/path_roles.go

 }

+func nonLockedSetAwsRole(ctx context.Context, s logical.Storage, roleName string, roleEntry *awsRoleEntry) error {


I don't think you need to express the lock requirement in the function name. This is either going to be locked while upgrading or during a regular write, we don't guarantee order of requests, so it is fine that there is no lock in that case.

I don't have strong feelings either way. I mostly did it to maintain some symmetry with the nonLockedRoleRead/lockedRoleRead methods.

chrishoffman · 2018-05-08T14:44:59Z

builtin/logical/aws/path_user.go

+	case iamUserCred:
+		return b.secretAccessKeysCreate(ctx, req.Storage, req.DisplayName, roleName, role)
+	case assumedRoleCred:
+		if ttl == 0 {


Why a default value here? Core will now handle setting defaults based on the tuned mount setting.

For no good reasons.... :)

The default value of 3600 really only made sense for assumed role or federation token creds and didn't make sense for IAM user creds. I missed the fact that this parameter was being ignored for IAM user creds.

Given that, does it make sense to check and add a warning if a ttl is passed when requesting IAM user creds?

chrishoffman · 2018-05-08T14:46:56Z

builtin/logical/aws/path_user.go

+		}
+		return b.assumeRole(ctx, req.Storage, req.DisplayName, roleName, roleArn, role.PolicyDocument, ttl)
+	case federationTokenCred:
+		if ttl == 0 {


Same question on default value.

chrishoffman · 2018-05-08T15:00:13Z

builtin/logical/aws/path_roles.go

+			return logical.ErrorResponse("cannot supply deprecated role or policy parameters with policy_document"), nil
+		}
+		var compacted string
+		if policyDocumentRaw.(string) == "" {


You can simplify this with something like this:

compacted := policyDocumentRaw.(string) if len(compacted) > 0 { ...

Thanks! Done.

chrishoffman · 2018-05-08T15:01:08Z

builtin/logical/aws/path_roles.go

+		roleEntry.ProhibitFlexibleCredPath = false
+	}
+
+	if len(roleEntry.CredentialTypes) < 1 {


Can you make this == 0?

chrishoffman · 2018-05-08T15:06:36Z

builtin/logical/aws/path_roles.go

+
+func (b *backend) lockedRoleRead(ctx context.Context, s logical.Storage, roleName string) (*awsRoleEntry, error) {
+	b.roleMutex.RLock()
+	roleEntry, err := nonLockedRoleRead(ctx, s, roleName)


Based on some of the other suggestions, the need for the nonLocked method may go away. If so, can you just inline this logic.

jefferai · 2018-07-18T15:22:28Z

Hi @joelthompson, just wanted to see if you were going to be able to get to the feedback soon.

This gets rid of the duplicated role upgrade code between both role reading and role writing by handling the upgrade all in the role reading.

joelthompson · 2018-08-04T20:14:08Z

@chrishoffman I think I largely agree with your sentiment about simplifying the role upgrade code, so take a look at the most recent commit I pushed and let me know if that's what you were thinking.

I'll wait for #5048 to get merged, then merge master into my PR, fix the merge conflict in the docs, and use the AWS SDK ARN parser, and hopefully this should (finally!) be good to go.

This adds the AWS ARN parser into the vendor as suggested by hashicorp#4360 (comment)

The testAccStepReadUser and testAccStepReadSTS were virtually identical, so they are consolidated into a single method with the path passed in.

This adds the AWS ARN parser into the vendor as suggested by #4360 (comment)

jefferai · 2018-08-16T10:36:38Z

builtin/logical/aws/secret_access_keys.go

+	assumeRoleInput := &sts.AssumeRoleInput{
+		RoleSessionName: aws.String(username),
+		RoleArn:         aws.String(roleArn),
+		DurationSeconds: &lifeTimeInSeconds,


I definitely read this as "a life time in seconds"

jefferai

Looks awesome! I'm going to merge this, if anyone else on the team wants to review it before beta (or between now and final) there's still time. Thanks, Joel!

For hashicorp#4360

This was referenced Apr 15, 2018

multiple arn options for aws backend over vault client (cli) #2817

Closed

Feature Request - AWS Secret Backend: Policy support to STS #3751

Closed

joelthompson added 2 commits April 15, 2018 16:33

Add missing write action to STS endpoint

cc2933c

Allow unsetting policy_document with empty string

42b41da

This allows unsetting the policy_document by passing in an empty string. Previously, it would fail because the empty string isn't a valid JSON document.

jefferai modified the milestones: 0.10.1, 0.10.2 Apr 16, 2018

tyrannosaurus-becks previously approved these changes Apr 25, 2018

View reviewed changes

joelthompson commented Apr 27, 2018

View reviewed changes

chrishoffman reviewed May 8, 2018

View reviewed changes

jefferai modified the milestones: 0.10.2, 0.10.3 May 22, 2018

jefferai modified the milestones: 0.10.3, 0.10.4 Jun 12, 2018

chrishoffman modified the milestones: 0.10.4, 0.10.5 Jul 19, 2018

jefferai mentioned this pull request Jul 20, 2018

AWS Lease length doesn't seem to work #387

Closed

joelthompson mentioned this pull request Aug 4, 2018

Vendor AWS ARN parser #5048

Merged

Respond to some PR feedback

1d77a85

joelthompson dismissed tyrannosaurus-becks’s stale review via 1d77a85 August 4, 2018 19:32

Refactor and simplify role reading/upgrading

db0e9e2

This gets rid of the duplicated role upgrade code between both role reading and role writing by handling the upgrade all in the role reading.

joelthompson added a commit to joelthompson/vault that referenced this pull request Aug 4, 2018

Vendor AWS ARN parser

141a4bc

This adds the AWS ARN parser into the vendor as suggested by hashicorp#4360 (comment)

joelthompson mentioned this pull request Aug 4, 2018

Add support for AWS STS external ID #5033

Closed

Eliminate duplicated AWS secret test code

6bb274f

The testAccStepReadUser and testAccStepReadSTS were virtually identical, so they are consolidated into a single method with the path passed in.

briankassouf pushed a commit that referenced this pull request Aug 6, 2018

Vendor AWS ARN parser (#5048)

f7150ae

This adds the AWS ARN parser into the vendor as suggested by #4360 (comment)

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

a682119

Switch to use AWS ARN parser

523d206

jefferai modified the milestones: 0.10.5 , 0.11 Aug 13, 2018

jefferai reviewed Aug 16, 2018

View reviewed changes

jefferai approved these changes Aug 16, 2018

View reviewed changes

jefferai merged commit 8275802 into hashicorp:master Aug 16, 2018

joelthompson mentioned this pull request Aug 20, 2018

WIP: Update UI to Support Explicit AWS Credential Types #5139

Closed

6 tasks

joelthompson added a commit to joelthompson/vault that referenced this pull request Aug 28, 2018

Add upgrade notes for AWS Secret Engine update

df7c3c9

For hashicorp#4360

joelthompson mentioned this pull request Aug 28, 2018

Add upgrade notes for AWS Secret Engine update #5205

Closed

joelthompson deleted the refactor_aws_secret branch August 29, 2018 02:17

moosilauke18 mentioned this pull request Dec 12, 2018

Update policy_arn to be more in line with vault 0.11.0 hashicorp/terraform-provider-vault#259

Closed

anatolebeuzon mentioned this pull request May 3, 2019

aws_secret_backend_role: support role_arns argument hashicorp/terraform-provider-vault#407

Merged

		}

		func nonLockedSetAwsRole(ctx context.Context, s logical.Storage, roleName string, roleEntry *awsRoleEntry) error {

Make AWS credential types more explicit #4360

Make AWS credential types more explicit #4360

Conversation

joelthompson commented Apr 15, 2018

tyrannosaurus-becks 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

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

chrishoffman 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

joelthompson Aug 4, 2018 • edited Loading

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jefferai commented Jul 18, 2018

joelthompson commented Aug 4, 2018

Choose a reason for hiding this comment

jefferai left a comment

Choose a reason for hiding this comment

joelthompson Aug 4, 2018 •

edited

Loading