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

fix: check credential expiration timestamp when generating tokens #737

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

alvaroaleman
Copy link
Member

What this PR does / why we need it:

There are two expirations which must be considered when using a signed EKS token:

  • 15 minutes after the point in time when the AWS STS request has been signed
  • The underlying AWS credentials can expire at which point the token won't be accepted

The second case is particularly common when making frequent requests while using AssumeRole or AssumeRoleWithWebRequest as mentioned in #590 as the default session timeout is 1 hour.

This PR adds an additional check fetching the AWS credential expiration and using that as the returned expiration if it is before the 15 minute token expiration.

Which issue(s) this PR fixes

Fixes #590

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Replaces #724
/assign @micahhausler

@k8s-ci-robot k8s-ci-robot added do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Aug 6, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Aug 6, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/invalid-commit-message Indicates that a PR should not merge because it has an invalid commit message. label Aug 6, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: alvaroaleman
Once this PR has been reviewed and has the lgtm label, please ask for approval from micahhausler. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@micahhausler
Copy link
Member

I'll check on this, but I think this PR is unnecessary. Most AWS temporary creds' advertised expiration is well before (as much as 50% of the cred lifetime) the actual credential is invalid. This is meant to increase the stability of an application in the event that the credential distribution method is ever impaired. Ex: If you have a 6 hour cred, but the advertised expiration is after 4 hours and normal issuance of new creds is every 4 hours, you can ride out a new credential distribution disruption for up to 2 hours.

@alvaroaleman
Copy link
Member Author

I'll check on this, but I think this PR is unnecessary. Most AWS temporary creds' advertised expiration is well before (as much as 50% of the cred lifetime) the actual credential is invalid. This is meant to increase the stability of an application in the event that the credential distribution method is ever impaired. Ex: If you have a 6 hour cred, but the advertised expiration is after 4 hours and normal issuance of new creds is every 4 hours, you can ride out a new credential distribution disruption for up to 2 hours.

This change is definitely necessary, we made this after we saw frequent auth failures which this fixes, which is why we are currently using a fork with this change. You might be right that this is less of a problem with longer-lived creds, but we for example have a max session validity of 1h for compliance reason. Without this change, it happens regularly that the creds used to sign this request expire and the returned token stops working.

@micahhausler
Copy link
Member

I'm ok adding this behavior with a flag (default off) because of the above situation where a cred might be reported as expired, but is still valid, we wouldn't want to create tokens with a reported expiration in the past.

I'll get #741 merged first which includes a test you should be able to extend for the new behavior

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 21, 2024
@micahhausler
Copy link
Member

Ok, I've merged #741, that test should be extensible for this change, with the addition of a flag

@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 1, 2024
@alvaroaleman
Copy link
Member Author

@micahhausler thanks for that, I've updated the PR, please have another look

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 22, 2024
What this PR does / why we need it:

There are two expirations which must be considered when using a signed EKS token:

* 15 minutes after the point in time when the AWS STS request has been signed
* The underlying AWS credentials can expire at which point the token won't be accepted

The second case is particularly common when making frequent requests while using AssumeRole or AssumeRoleWithWebRequest as mentioned in kubernetes-sigs#590 as the default session timeout is 1 hour.

This PR adds an additional check fetching the AWS credential expiration and using that as the returned expiration if it is before the 15 minute token expiration.
@k8s-ci-robot k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 24, 2024
@alvaroaleman
Copy link
Member Author

@micahhausler any chance you could have a look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

API Authorization from Outside EKS Cluster throws Unauthorized error after 20+ minutes
3 participants