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

Port Python use of jwcrypto to new Rust library #8299

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

Conversation

mmastrac
Copy link
Contributor

@mmastrac mmastrac commented Feb 4, 2025

This migrates all Python code from jwcrypto to gel-jwt.

As part of the work, the auth-ext code is reorganized a bit so all the token types are managed centrally and given common names.

Server notes:

  • The allow and denylist for the JWT claims are loaded into the JWKSet's default_validation_context and checked internally at token validation time. These are still reloadable and can be updates as files on disk change.
  • Both PEM and JSON JWK json files are supported. If JWK auto-generation is enabled and the JWK filename ends in .json, a JSON-format JWK will be created (along with a key ID) allowing for eventual key rotation.

auth_ext notes

  • We start to cache remote JWKSets (10 mins) as those are unlikely to change often and it's likely they will cause HTTP failures at scale.
  • Each JWT type used by auth_ext has its own Python token type and derived HKDF key to ensure keys can never accidentally be used across auth_ext token domains (most keys already had this).

Additional changes:

  • Token expiration and allowlists are checked within the new gel-jwt library so we can remove all of that handling internally.
  • auth_ext testcases were refactored a bit and some common code was pulled out into generate_and_serve_jwk

@mmastrac mmastrac force-pushed the gel_jwt branch 2 times, most recently from 8943243 to 0a7f752 Compare February 6, 2025 01:41
mmastrac added a commit that referenced this pull request Feb 6, 2025
Minor housekeeping -- trying to rename internal crates to `gel-*`.
Extracted from #8299.

Some internal modules moved in `gel-auth` in anticipation of adding some
more auth methods.

As part of the work, trimmed some deps using `cargo-shear`.
@mmastrac mmastrac force-pushed the gel_jwt branch 5 times, most recently from 3fed1c7 to 6bf2a2f Compare February 7, 2025 23:26
@mmastrac mmastrac changed the title [WIP] Rust implementation of JWT Port Python use of jwcrypto to new Rust library Feb 7, 2025
@mmastrac mmastrac marked this pull request as ready for review February 7, 2025 23:27
mmastrac added a commit that referenced this pull request Feb 11, 2025
Extracted from #8299

This implements JWT signing and validation in Rust.

The implementation is provided by the external crate `jsonwebtoken`, but
we manage all key loading and storage as that crate is somewhat lacking
in that department. The backend crypto for this is provided by Ring,
though the external crate may offer pluggable backends in the future.

The crate provides an interface `KeyRegistry` that can be used to
generate, load and save `JWK`s using one of three algorithms: `HS256`,
`RS256` or `EC256`.
Copy link
Contributor

@scotttrinh scotttrinh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

gorgeous 🏆

) -> str:
signing_ctx = jwt_auth.SigningCtx()
signing_ctx.set_expiry(int(expires_in.total_seconds()))
signing_ctx.set_not_before(30)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we setting an nbf here? Maybe related to removing the iat check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a safeguard to prevent tokens issued in the future from being used for extended periods of time. I noticed that the Rust library doesn't check that iat is in the past, so it prevents tokens from the future from being used.

It's pretty unlikely you could trick a server into issuing a token from the future, but if you did you could potentially have a long-lived token.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I specifically wanted to understand why we removed the iat handling from the email verification JWT. I don't recall 100% of the details here, but I remember switching to iat from exp for a specific reason. I'll see if I can resurrect it. Something about handling old verification emails (someone forgot to verify, waited too long, and tried to verify again) and still being able to send a refreshed one.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, yes -- there's a skip_expiration_check on the verification token JWT method that explicitly ignores the expiry for the case of resending validation emails.

So basically we still check the signature in that case, but we ignore the timestamps. That's the only case I could find where that was required.

We might want to limit how far we extend the expiry to resend an email (maybe 30 days?) but for now I believe it acts the same way as before.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think the iat-related logic was explicitly to still limit the validity to some reasonable time frame without using an expiration time. I'll take another look at the old logic and do a little bit of research from back when I implemented this to be sure. I'm not sure I trust that there is an existing test for this case, so might be worth writing one in the course of figuring out what the actual behavior is meant to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked at the old source -- it appears that we were using iat to do a pseudo 24-hour expiration without an 'exp' token. I put all of the expiration times into constants in the jwt module and confirmed that they were the same as the previous.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There was some explicit reason I avoided an exp time back when I wrote it, but I can't quite recall. When I get a little space to go back into the history a bit, I'll give a final approval on this. I'd rather keep the existing behavior to not block landing this, but I'm also fine with sitting on it to be sure.

deepbuzin pushed a commit that referenced this pull request Feb 18, 2025
Minor housekeeping -- trying to rename internal crates to `gel-*`.
Extracted from #8299.

Some internal modules moved in `gel-auth` in anticipation of adding some
more auth methods.

As part of the work, trimmed some deps using `cargo-shear`.
deepbuzin pushed a commit that referenced this pull request Feb 18, 2025
Extracted from #8299

This implements JWT signing and validation in Rust.

The implementation is provided by the external crate `jsonwebtoken`, but
we manage all key loading and storage as that crate is somewhat lacking
in that department. The backend crypto for this is provided by Ring,
though the external crate may offer pluggable backends in the future.

The crate provides an interface `KeyRegistry` that can be used to
generate, load and save `JWK`s using one of three algorithms: `HS256`,
`RS256` or `EC256`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants