CLI deciding if token exchange needed should not look at ID token expiry #1873
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1873 +/- ##
==========================================
- Coverage 38.13% 38.12% -0.01%
==========================================
Files 346 346
Lines 43630 43628 -2
==========================================
- Hits 16638 16634 -4
- Misses 26477 26479 +2
Partials 515 515 ☔ View full report in Codecov by Sentry. |
This fixes a small mistake in PR #1864. When the "pinniped login oidc" CLI command is deciding if the RFC8693 token exchange is needed, it should not look at the expiry of the ID token. This mistake would cause the RFC8693 token exchange to happen when the OIDC provider is not a Pinniped Supervisor, which would fail because most other providers do not support that type of token exchange. It does not matter if the current ID token is close to expiring when deciding if the RFC8693 token exchange is needed, because the token exchange is going to yield a new ID token anyway. It does matter if the current ID token is close to expiring if the CLI decides that it is not going to perform the token exchange, and this commit does not change that logic.
cfryanr
force-pushed
the
1864_followup
branch
from
c10b4bf
to
64b0e69
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While trying to reproduce the issue reported in #1868, I found an issue that was caused by PR #1864.
This PR fixes a small mistake in PR #1864. When the "pinniped login oidc" CLI command is deciding if the RFC8693 token exchange is needed, it should not look at the expiry of the ID token. This mistake would cause the RFC8693 token exchange to happen when the OIDC provider is not a Pinniped Supervisor, which would fail because most other providers do not support that type of token exchange.
It does not matter if the current ID token is close to expiring when deciding if the RFC8693 token exchange is needed, because the token exchange is going to yield a new ID token anyway. However, it does matter if the current ID token is close to expiring if the CLI decides that it is not going to perform the token exchange, and this PR does not change that logic.
This PR also adds a new hack script to make it easy to set up a local kind cluster to manually test the style of configuration which was impacted by this bug. It configures the Concierge to use an OIDC issuer which is not the Pinniped Supervisor.
This PR also updates the documentation related to using the Concierge with an OIDC issuer which is not the Supervisor to clarify some points. Rendered preview here.
Release note:
None because #1864 has not been included in any release yet.