-
Notifications
You must be signed in to change notification settings - Fork 16
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
Use the KID value in the login gov token validation logic #3193
Comments
github-project-automation
bot
moved this to Icebox
in Simpler.Grants.gov Product Backlog
Dec 11, 2024
chouinar
moved this from In Progress
to In Review
in Simpler.Grants.gov Product Backlog
Dec 18, 2024
github-project-automation
bot
moved this from In Review
to Done
in Simpler.Grants.gov Product Backlog
Dec 19, 2024
doug-s-nava
pushed a commit
that referenced
this issue
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
Summary
OAuth public JWKs have a KID (key ID) that can be used to quickly link the public key used to the OAuth object.
When implementing the OAuth logic, I wasn't sure if the token we received from login.gov would have the KID value set in the header, but it turns out that it does.
Our logic right now inefficiently iterates over every key and checks each one when we could directly do it instead.
In the https://github.com/HHS/simpler-grants-gov/blob/main/api/src/auth/login_gov_jwt_auth.py instead of iterating over a list of public keys, we should instead:
Note that the refresh logic present should remain (if the public key isn't found, do a refresh and check again).
To get the KID from a token, you can do something like:
There may be additional adjustments needed for tests to work as the way the public keys are setup in tests is a bit complex.
Acceptance criteria
The text was updated successfully, but these errors were encountered: