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

don't create leases for AWS STS secrets #15869

Merged
merged 6 commits into from Oct 28, 2022

Conversation

bhowe34
Copy link
Contributor

@bhowe34 bhowe34 commented Jun 8, 2022

AWS STS credentials can neither be renewed nor revoked on a per credential basis (credentials can be revoked by revoking all credentials for a role issued before a given time). Given these limitations creating a lease for the credentials isn't very useful and creates lots of requests to the storage backend, especially when vault processes all existing leases on startup. For these reasons it would make sense to not create leases for AWS STS credentials.

To support existing leases during upgrade the revoke and renew logic will still check for STS leases.

@hsimon-hashicorp
Copy link
Contributor

@bhowe34, when you're ready for this to be reviewed, please don't forget to add a changelog entry! Thanks. :)

"secret_key": *tokenResp.Credentials.SecretAccessKey,
"security_token": *tokenResp.Credentials.SessionToken,
"arn": *tokenResp.AssumedRoleUser.Arn,
"ttl": uint64(tokenResp.Credentials.Expiration.Sub(time.Now()).Seconds()),
Copy link
Contributor Author

Choose a reason for hiding this comment

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

compatible with api.Secret.TokenTTL()

@bhowe34 bhowe34 marked this pull request as ready for review June 9, 2022 15:05
@bhowe34 bhowe34 requested a review from a team June 9, 2022 15:05
@calvn
Copy link
Member

calvn commented Jun 10, 2022

Hi @bhowe34, we've discussed this internally and think that this is a reasonable enhancement request. We're currently wrapping up tasks for our next major release, but will continue to track this internally and prioritize reviewing this in the near future.

@robmonte
Copy link
Member

Hello @bhowe34

Thanks for this contribution! I have reviewed the PR and I was wondering if you perhaps had a reason to not make this same change for federation tokens? The federation tokens are also short-lived STS secrets like the assumed role tokens, however maybe you have more insight into differences than I am aware of? Please let me know your thoughts!

@bhowe34
Copy link
Contributor Author

bhowe34 commented Oct 24, 2022

Hi @robmonte

I didn't make the change for federation tokens because I haven't used them and I am not familiar with how they work. I'll look into them and see if the same change makes sense in that case.

@bhowe34
Copy link
Contributor Author

bhowe34 commented Oct 24, 2022

Looks like you are correct so I made the same change for federation tokens.

Copy link
Member

@robmonte robmonte left a comment

Choose a reason for hiding this comment

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

Thanks again for your submission! Looks good to me. There's a test case to update to accommodate this change but I'll take care of that in a separate PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants