[Issue #3193] Use the KID value in login gov token validation #3283
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
mdragon
approved these changes
Dec 18, 2024
doug-s-nava
pushed a commit
that referenced
this pull request
Dec 30, 2024
## Summary Fixes #3193 ### Time to review: __5 mins__ ## Changes proposed Rather than iterate over all public keys from login.gov - instead directly use the one with the same KID (key ID) ## Context for reviewers The KID is the Key ID, in the public endpoint from an OAuth server that gives you a list of keys the ID should be present there, and the header for a given token will have the KID as well. However these weren't guaranteed in the OAuth spec so I waited to assume they existed until I tested with real login.gov - they do exist. Quite a bit of structure needed to be reorganized with this as the way we also structure the public keys went from a list to a dict. There was a lot of assumed failure cases if no key was found that worked that were removed and now the logic is a bit more directly: * If a public key can't be found refresh the public keys * If a public key is found, validate with it * Otherwise error We also no longer need to account for the "failed validation, but assume we used the wrong key" scenario since we'll have the exact key. ## Additional information Everything continues to work uneventfully locally, the KID is set (always `issuer1`) in both places.
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.
Summary
Fixes #3193
Time to review: 5 mins
Changes proposed
Rather than iterate over all public keys from login.gov - instead directly use the one with the same KID (key ID)
Context for reviewers
The KID is the Key ID, in the public endpoint from an OAuth server that gives you a list of keys the ID should be present there, and the header for a given token will have the KID as well. However these weren't guaranteed in the OAuth spec so I waited to assume they existed until I tested with real login.gov - they do exist.
Quite a bit of structure needed to be reorganized with this as the way we also structure the public keys went from a list to a dict. There was a lot of assumed failure cases if no key was found that worked that were removed and now the logic is a bit more directly:
We also no longer need to account for the "failed validation, but assume we used the wrong key" scenario since we'll have the exact key.
Additional information
Everything continues to work uneventfully locally, the KID is set (always
issuer1) in both places.