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

Make ID token lifetime configurable for OIDCClients #1914

Merged
merged 8 commits into from
Apr 25, 2024

Conversation

cfryanr
Copy link
Member

@cfryanr cfryanr commented Apr 16, 2024

This PR adds a new configuration option to OIDCClient resources to allow the ID tokens issued by authcode flows and refresh flows to have a custom lifetime, within certain reasonable limits of min/max allowed lifetimes. See the new field introduced in types_oidcclient.go.tmpl in the diffs of this PR.

Note that this PR borrows heavily from the proof-of-concept PR #1857. The difference is that this PR is intended to be production-quality and to be eventually merged. This PR does not include anything from the POC PR about adjusting token lifetimes for sessions started using GitHub PATs, but that could be added later after this PR is merged, if desired.

Release note:

TBD

Copy link

codecov bot commented Apr 16, 2024

Codecov Report

Attention: Patch coverage is 86.51163% with 29 lines in your changes are missing coverage. Please review.

Project coverage is 38.45%. Comparing base (5fe94c4) to head (57a07a4).

Files Patch % Lines
...derationdomain/idtokenlifespan/idtoken_lifespan.go 44.44% 10 Missing ⚠️
...upervisor/config/v1alpha1/zz_generated.deepcopy.go 56.25% 7 Missing ⚠️
internal/federationdomain/oidc/oidc.go 94.17% 6 Missing ⚠️
.../federationdomain/endpoints/token/token_handler.go 83.33% 2 Missing and 1 partial ⚠️
internal/testutil/oidcclient.go 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1914      +/-   ##
==========================================
- Coverage   38.63%   38.45%   -0.18%     
==========================================
  Files         349      350       +1     
  Lines       44511    44680     +169     
==========================================
- Hits        17196    17182      -14     
- Misses      26799    26983     +184     
+ Partials      516      515       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@cfryanr cfryanr force-pushed the configurable_id_token_length branch 2 times, most recently from ec60bd4 to e4cdb42 Compare April 17, 2024 19:48
@cfryanr cfryanr marked this pull request as ready for review April 18, 2024 19:24
@cfryanr cfryanr changed the title WIP: Make ID token lifetime configurable for OIDCClients Make ID token lifetime configurable for OIDCClients Apr 18, 2024
@cfryanr
Copy link
Member Author

cfryanr commented Apr 18, 2024

Although this is covered by integration tests, it probably also deserves some manual testing before merge.

requireClaimsAreNotEqual(t, "iat", claimsOfFirstIDToken, tokenClaims) // issued at
require.Greater(t, tokenClaims["iat"], claimsOfFirstIDToken["iat"])
requireClaimsAreNotEqual(t, "exp", claimsOfFirstIDToken, tokenClaims) // expires at
if test.authcodeExchange.want.wantIDTokenLifetimeSeconds == 0 {
// If the ID token lifetime of the original ID token was not customized by configuration,
Copy link
Member

Choose a reason for hiding this comment

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

Is there a two minute delay in the test? I should go look. I would imagine that this assertion is always true: for a specific client, a new token will always exp after an earlier token. I think understanding this test probably means I need to look at this whole test. If the new token is a refreshed token, could we call that refreshedToken? Otherwise it's not clear from this diff whether the new token is a refreshed token with exactly two minute lifetime or a result of a new authorization with the configured lifetime.

Copy link
Member Author

Choose a reason for hiding this comment

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

There is a one second sleep in this test to make sure that the new ID token returned by the token exchange will be more obviously different than the original ID token in its timestamp-related claims.

The lifetime of the original ID token could be the default two minutes, or it could be configured to be 4242 seconds, as is done in one test of the test table. The lifetime of the new ID token returned by the token exchange will always be 2 minutes (not configurable).

In the 4242 case, I do an authcode flow and my original ID token expires 4242 seconds (~70 minutes) from now. Then I wait one second. Then I do the token exchange and get a new ID token which expires 2 minutes from now. So in this case, my original ID token is valid for much longer than my new one.

The assertion was previously assuming that both tokens will have a 2 minute lifetime, with ~1 second delay in between when they were issued, in which case the newer token should always expire after the older token.

@cfryanr cfryanr force-pushed the configurable_id_token_length branch from 1e5fd2f to 57a07a4 Compare April 24, 2024 22:06
@cfryanr cfryanr merged commit b99da0c into main Apr 25, 2024
42 checks passed
@cfryanr cfryanr deleted the configurable_id_token_length branch April 25, 2024 22:50
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

3 participants