-
Notifications
You must be signed in to change notification settings - Fork 66
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
Conversation
Codecov ReportAttention: Patch coverage is
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. |
ec60bd4
to
e4cdb42
Compare
OIDCClients
OIDCClients
Although this is covered by integration tests, it probably also deserves some manual testing before merge. |
deploy/supervisor/config.supervisor.pinniped.dev_oidcclients.yaml
Outdated
Show resolved
Hide resolved
deploy/supervisor/config.supervisor.pinniped.dev_oidcclients.yaml
Outdated
Show resolved
Hide resolved
internal/federationdomain/clientregistry/clientregistry_test.go
Outdated
Show resolved
Hide resolved
internal/federationdomain/clientregistry/clientregistry_test.go
Outdated
Show resolved
Hide resolved
internal/federationdomain/clientregistry/clientregistry_test.go
Outdated
Show resolved
Hide resolved
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, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
1e5fd2f
to
57a07a4
Compare
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 intypes_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: