feat: replace the way AWS sessions are created and added a new WebIdentity provider to the credentials chain

[dgozalo]

In order to use IRSA, a newest version of aws-sdk-go had to be used.

Also, vault needed to change the way it created Sessions as session.New has been deprecated and doesn't work with IRSA.

Last but not least, as Vault was creating a custom credentials chain, it is overriding the providers slice that the AWS API uses, so a WebIdentityProvider had to be added only when the IRSA environment variables are present to assume a role.

This also comes with a necessary workaround due to a bug in Kubernetes:

kubernetes/kubernetes#82573 which basically doesn't allow non-root users to access the injected token.

As vault is using a custom user and group, the usual workaround (setting securityContext.fsGroup to 65534) didn't work.

I had to set securityContext.fsGroup to 1000 (the id of the vault group) and securityContext.runAsGroup to 1000 in the Vault pod to get to read the token file correctly.

This aims to fix this #7450

[hashicorp-cla]

All committers have signed the CLA.

[dgozalo]

cc @jefferai @vishalnayak

Could you take a look a this? It's somewhat urgent for us at Jenkins-x and I'm happy to make any changes needed.

[ghost]

Seconding this, we're currently setting up vault on a dedicated EKS cluster and would love this feature for giving vault access to a dynamodb table.

[ozbillwang]

To using new IRSA, we have to upgrade all related applications running in pods to latest aws-sdk-<language> one by one.

But we can't make them all updated. Vault is one of them.

How do you deal with this situation currently? Mix with IRSA and kiam/kube2iam?

[matthewflannery]

Seconding this, would love support. We evaluated IRSA as a favourable alternative to kube2iam / kiam however landed in the same situation as @ozbillwang - Vault being a key application.

[ghost]

We're running this branch in testing and it works quite well. There's a bit of a UX issue where not being able to read the token will result in a fallback to instance roles silently, but that's more of an aws-sdk issue. Adding this to the pod spec works:

securityContext:
  runAsUser: 0
  fsGroup: 1000
  runAsGroup: 1000

[dgozalo]

I'd love to get some feedback on this (and a way to run the test-go-race test again), any ETA on when this can be looked at?

[joelthompson]

Hi @dgozalo -- in general, it helps to separate to a third-party library (in this case, the AWS golang SDK) from the code changes you're making in Vault itself. That helps reviewers focus more on the specific changes, i.e., "is this a correct update to the SDK?" or "does this code change to Vault make sense?" individually.

I've submitted #7458 to update the SDK to a recent version to both support EKS IRSAs as well as IMDSv2, so perhaps just focus this on changing the way sessions are created?

Also, one more thing to think about which is how the SDK might use VPC Endpoints to call sts:AssumeRoleWithWebIdentity. I haven't had a chance to dig into it too much, but I know there are some customers who are very sensitive to any sort of outbound internet access, so it would be ideal to route any sts:AssumeRoleWithWebIdentity calls over an STS VPC endpoint, if possible. Both the AWS Secrets Engine and Auth Method support an sts_endpoint configuration item which allows specifying a custom STS endpoint that can be used to send traffic over an STS VPC Endpoint. I don't think the user experience would be great if that parameter weren't respected for sts:AssumeRoleWithWebIdentity calls. If the SDK doesn't support it, that's probably OK, but that should at least be a documented limitation, and it would be great to open a feature request in the SDK to support it. I've also opened #5228 as a way to centralize how to make VPCEs a first-class citizen, so maybe it makes sense to add some comments there on any deltas needed?

[tyrannosaurus-becks]

Thanks for working on this! It's much appreciated!

We will need to be careful moving this through to make sure we don't break anything for anyone else. I generally concur with Joel's comments regarding separating the PRs. Also, for the most part, this looks great, just want to be sure we have great test coverage to match. It will help us understand the SDK's behavior for folks who don't have this set.

[tyrannosaurus-becks]

Does the AWS SDK handle any of these sorts of environment variables under the hood? So that we could delegate this to them?

[dgozalo]

It does handle them but only when you keep the default credentials chain.

As Vault is creating its own chain, we need to create a WebIdentityProvider and add it to the chain, so it's not something that the SDK will do by itself, that's one of the reasons that I had to handle the env variables myself instead of delegating it.

[jefferai]

I don't think this is correct.

https://github.com/aws/aws-sdk-go/blob/v1.26.8/aws/defaults/defaults.go#L93 and https://github.com/aws/aws-sdk-go/blob/v1.26.8/aws/defaults/defaults.go#L107 shows that the default cred chain consists of

	[]credentials.Provider{
		&credentials.EnvProvider{},
		&credentials.SharedCredentialsProvider{Filename: "", Profile: ""},
		RemoteCredProvider(*cfg, handlers),
	}

All three of these are added by us in the same order. As this includes EnvProvider and that checks AWS_ROLE_SESSION_NAME it shouldn't be necessary to manaully check env vars.

[jefferai]

Another way to approach this might be, if wanting a WebIdentityRoleProvider is a general-purpose thing, suggest in the aws-sdk-go repo to simply make it part of the default set.

[dgozalo]

I've been looking into this more carefully. I can see some different behaviour providing your own credentials chain vs using the default one:

• Using the default credentials chains, the sdk detects the WebIdentityProvider Env variables correctly and internally it calls NewWebIdentityRoleProvider.
  This means that calling any AWS service without creating the custom credentials chain makes IRSA work by default.

• When using the custom credentials chain with the call to NewWebIdentityRoleProvider commented, the WebIdentityProvider configuration gets overridden by the provided chain providers.

This means that we in fact need to provide the WebIdentityProvider if we are to create a custom credentials chain because none of the other providers will ever call the WebIdentity within their Retrieve implemented functions.

[dgozalo]

Hi @dgozalo -- in general, it helps to separate to a third-party library (in this case, the AWS golang SDK) from the code changes you're making in Vault itself. That helps reviewers focus more on the specific changes, i.e., "is this a correct update to the SDK?" or "does this code change to Vault make sense?" individually.

I've submitted #7458 to update the SDK to a recent version to both support EKS IRSAs as well as IMDSv2, so perhaps just focus this on changing the way sessions are created?

Also, one more thing to think about which is how the SDK might use VPC Endpoints to call sts:AssumeRoleWithWebIdentity. I haven't had a chance to dig into it too much, but I know there are some customers who are very sensitive to any sort of outbound internet access, so it would be ideal to route any sts:AssumeRoleWithWebIdentity calls over an STS VPC endpoint, if possible. Both the AWS Secrets Engine and Auth Method support an sts_endpoint configuration item which allows specifying a custom STS endpoint that can be used to send traffic over an STS VPC Endpoint. I don't think the user experience would be great if that parameter weren't respected for sts:AssumeRoleWithWebIdentity calls. If the SDK doesn't support it, that's probably OK, but that should at least be a documented limitation, and it would be great to open a feature request in the SDK to support it. I've also opened #5228 as a way to centralize how to make VPCEs a first-class citizen, so maybe it makes sense to add some comments there on any deltas needed?

Hi @joelthompson thanks for the feedback.

I agree, the changes to the third party library should be done in a different PR, but I needed this in order to verify that this was working correctly. Now that your PR has been approved I can remove that part from my PR and focus on the logic.

About the VPC Endpoints, I'll take a look at how the SDK handles it and try to find a way to use them, as I agree it's a pretty sensitive part of the code and it needs to be handled properly, and warn users if it's not supported yet by the SDK.

[tyrannosaurus-becks]

@dgozalo thanks again for this PR! I've just merged the big SDK update this PR was waiting on. Would you be willing to merge in master and use the version of the SDK that's now on master?

[dgozalo]

@dgozalo thanks again for this PR! I've just merged the big SDK update this PR was waiting on. Would you be willing to merge in master and use the version of the SDK that's now on master?

Excellent, I was waiting for that PR to merge, I'll start working on the suggested changes ASAP.

[jefferai]

Added DNM tag, see comment.

[jefferai]

Can you please ensure you're using Go 1.13 and go mod tidy?

[robertlestak]

@jefferai Per your suggestion, we have rebased this PR on top of the go-kms-wrapping branch, ensured usage of go 1.13, and ran a go mod tidy.

These changes are available in our fork here: https://github.com/umg/vault/commit/7f86b76a64f40d250d2919523dd6016a280c0a6b

This is now working as expected in our environment, please let us know if there is any additional changes required for this to be merged into upstream.

[ingshtrom]

barring other feedback, it looks like you have some conflicts that need to be resolved as well.

[jujugrrr]

Could you take a look a this? It's somewhat urgent for us at Jenkins-x and I'm happy to make any changes needed.

It's definitely a good feature we would like too! thanks for this @dgozalo
@dgozalo would you have time to look at the remaining conflict and #7738 (comment)?

[dgozalo]

I'll have some more time to work on this again and prioritize it so it can be merged soon.

[tyrannosaurus-becks]

Hi! I will likely be able to look at this PR this week or next. Would it be possible to respond to the other feedback I've given, and also to resolve merge conflicts? That'll get this ready for further review. Thank you!

[tyrannosaurus-becks]

Thank you! Looks awesome. Much appreciated.

[karabijavad]

any idea when this code will be released into a vault version?

[kalafut]

@karabijavad This was just released in 1.4.3. https://github.com/hashicorp/vault/blob/master/CHANGELOG.md#143