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

Generalize JWT-based authentication #1992

Draft
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

jpolchlo
Copy link

@jpolchlo jpolchlo commented Nov 2, 2023

Hi! I'm filing this largely as a discussion PR. It's probably pretty WIP-py, since these changes haven't been stress-tested in any actual use, but it solves at least part of the problem of deploying the CMR into environments that don't use EDL as their authentication source, since EDL-issued JWTs have some slightly non-standard features, and the use case I'm addressing here is to allow authentication via Cognito with federation to Login.gov, which are constituted using more standard fields.

The changes here are actually quite mild, and many are in docstrings and function names. The real meat is to be found in the handling of the JWKS, which is properly an array of web keys and not just a single JSON object, and there are also some minor changes that handle the fields in a JWT in a less EDL-specific fashion.

I want to provide some supporting documentation to justify the changes made within:

  • In re the move away from using uid as the JWT field for the user name, I found this spec suggesting that username should be expected (and this is how Cognito packages it).
  • In re changes to cmr.common.util/is-jwt-token?, I'm preferring to look for the required alg header field, and the optional—but needed for this implementation—kid field.

This all needs better testing, and I'm still new to CMR and having some issues running the unit test suite. Pointers welcome. Still lying in wait is the interaction between these tokens and the access control app. I'm also working on some machine-to-machine authentication using non-EDL OIDC identity providers, and that may introduce some wrinkles down the road.

Happy to get some feedback here. Thanks!

@jceaser
Copy link
Contributor

jceaser commented Nov 16, 2023

if this is a work in progress, is it better as a draft pr?

@jpolchlo
Copy link
Author

jpolchlo commented Nov 16, 2023

That may be so. That kind of thing seems to vary between repos. But if you're going to look at and comment on draft PRs and not just leave them be until they convert to official PRs, then this can be a draft. As mentioned, this is being filed as a point of discussion, so I'm hoping to have some of that.

@jceaser
Copy link
Contributor

jceaser commented Nov 17, 2023

Yea I think so, making draft. It also lets people know that if the request has been open for a while, you may need to rebase before using it as you're closer to a fork for the duration your open.

Another point for this direction, the merge may never happen with this exact request, but sections may be implemented as arguments are won. I would have to study this request more before I could say more.

Lastly, by policy we do not merge anything without a ticket number as we need something to track not just for development which you have provided, but for documentation, testing and deployment.

@jpolchlo jpolchlo marked this pull request as draft November 17, 2023 19:11
@jpolchlo
Copy link
Author

OK, thanks for the feedback. I've converted to draft.

I'm interested in how to integrate into the development process; the idea of many forks diverging does not fill me with warm feelings. Sadly, I'm getting the idea that the tickets you're referring to are internal to NASA's Jira? And it's opaque to the outside world? I'd like to be able to participate in the documentation and testing, but I'm perceiving that I won't be allowed.

Just fill me in on how you think a productive interaction can be formed here. I'd like to be able to at least try to achieve eventual consistency with your upstream version and our fork (to the extent that it's possible given diverging interests).

…WT bearer tokens; also be sure to properly catch when the JWT does not match the available key IDs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants