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

Implement more lax JWT decoder #5583

Merged
merged 1 commit into from
Feb 21, 2025
Merged

Implement more lax JWT decoder #5583

merged 1 commit into from
Feb 21, 2025

Conversation

ChrisPenner
Copy link
Contributor

Overview

The jwt lib fails on EdDSA jwts even though it doesn't verify the signature 😞

This means switching to EDDSA HashJwts isn't possible for now; which is fine I can keep those as HS256 JWTs, but I want to fix this now so we can potentially switch it in the distant future if we have to :)

Plus this removes the dependency on jwt, which is probably good since we should be using jose for everything where possible.

Implementation notes

Manually unpack and decode the jwt when reading HashJWTs rather than depending on the jwt lib.
It's really easy to do, and it's part of the spec so it's not going to change.

Test coverage

Tested against Share's push/pull transcripts

The `jwt` lib failes on EdDSA jwts even though it doesn't verify the signature :/
@ChrisPenner ChrisPenner marked this pull request as ready for review February 19, 2025 20:25
@aryairani
Copy link
Contributor

Is it:

  • The jwt lib fails on EdDSA jwts
  • We were planning to use jwt for this, but we can't
  • PR gets rid of jwt altogether
  • There's nothing else we can easily use to do EdDSA right now, but maybe we can later with a bigger refactor

?

@aryairani aryairani merged commit 72797fd into trunk Feb 21, 2025
32 checks passed
@aryairani aryairani deleted the eddsa-jwt-decoder branch February 21, 2025 14:06
@ChrisPenner
Copy link
Contributor Author

ChrisPenner commented Feb 25, 2025

@aryairani

The jwt lib fails on EdDSA jwts

Yes.

We were planning to use jwt for this, but we can't

jwt was already in use, but is incompatible with EdDSA jwts, which we may use for HashJWTs (or other JWTs) in the future.

PR gets rid of jwt altogether

Yes. This was the only thing we were using it for, and if it can't do that part right then we don't need it at all. I re-implemented the function we were using in a dozen lines

There's nothing else we can easily use to do EdDSA right now, but maybe we can later with a bigger refactor

This change allows trunk builds of unison to use EdDSA right now, but since old clients would break upon receiving EdDSA jwts we shouldn't switch yet. This PR changes the code to be future proof so we can support any flavour JWT someday in the future once enough folks have upgraded :)

No rush though, the backend can continue to support old JWTs.

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