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

Use the KID value in the login gov token validation logic #3193

Closed
2 tasks
chouinar opened this issue Dec 11, 2024 · 0 comments · Fixed by #3283
Closed
2 tasks

Use the KID value in the login gov token validation logic #3193

chouinar opened this issue Dec 11, 2024 · 0 comments · Fixed by #3283
Assignees

Comments

@chouinar
Copy link
Collaborator

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:

  • When loading public keys, make it a dict of KID -> key value
  • Get the KID from the token (see below) and grab the public key for it

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:

    data = jwt.api_jwt.decode_complete(token, options={"verify_signature": False}, algorithms=["RS256"])
    kid = data.get("header", {}).get("kid", "")

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

  • KID used
  • Tests fixed
@chouinar chouinar moved this from Icebox to Todo in Simpler.Grants.gov Product Backlog Dec 11, 2024
@chouinar chouinar self-assigned this Dec 17, 2024
@chouinar chouinar moved this from Todo to In Progress in Simpler.Grants.gov Product Backlog Dec 18, 2024
chouinar added a commit that referenced this issue Dec 18, 2024

Verified

This commit was signed with the committer’s verified signature.
pradyunsg Pradyun Gedam
@chouinar chouinar moved this from In Progress to In Review in Simpler.Grants.gov Product Backlog Dec 18, 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
Labels
None yet
Projects
Development

Successfully merging a pull request may close this issue.

1 participant