aws-iam-authenticator icon indicating copy to clipboard operation
aws-iam-authenticator copied to clipboard

fix: check credential expiration timestamp when generating tokens

Open alvaroaleman opened this issue 1 year ago • 11 comments

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 https://github.com/kubernetes-sigs/aws-iam-authenticator/pull/724 /assign @micahhausler

alvaroaleman avatar Aug 06 '24 22:08 alvaroaleman

[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

k8s-ci-robot avatar Aug 06 '24 22:08 k8s-ci-robot

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.

micahhausler avatar Aug 14 '24 14:08 micahhausler

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.

alvaroaleman avatar Aug 14 '24 14:08 alvaroaleman

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

micahhausler avatar Aug 15 '24 17:08 micahhausler

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

micahhausler avatar Aug 21 '24 20:08 micahhausler

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

alvaroaleman avatar Sep 01 '24 15:09 alvaroaleman

@micahhausler any chance you could have a look?

alvaroaleman avatar Oct 24 '24 02:10 alvaroaleman

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Jan 22 '25 02:01 k8s-triage-robot

It would be great if this could get a review /remove-lifecycle stale

alvaroaleman avatar Jan 22 '25 02:01 alvaroaleman

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle stale
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

k8s-triage-robot avatar Apr 22 '25 03:04 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Mark this PR as fresh with /remove-lifecycle rotten
  • Close this PR with /close
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten

k8s-triage-robot avatar May 22 '25 03:05 k8s-triage-robot

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

k8s-triage-robot avatar Jun 24 '25 20:06 k8s-triage-robot

@k8s-triage-robot: Closed this PR.

In response to this:

The Kubernetes project currently lacks enough active contributors to adequately respond to all issues and PRs.

This bot triages PRs according to the following rules:

  • After 90d of inactivity, lifecycle/stale is applied
  • After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
  • After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

  • Reopen this PR with /reopen
  • Mark this PR as fresh with /remove-lifecycle rotten
  • Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/close

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot avatar Jun 24 '25 20:06 k8s-ci-robot

PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

k8s-ci-robot avatar Jun 24 '25 20:06 k8s-ci-robot