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

feat(useSession): use JWE internally #997

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sandros94
Copy link

@sandros94 sandros94 commented Mar 22, 2025

Related to #994

This is a first implementation to use JWE (RFC7516) instead of iron-crypto as the internal useSession encryption.

The main goal is to provide better support for cross-platform/cross-language support in scenarios like edge networks and proxies.

Note

I left the iron-crypto implementation for now just for ease of access and testing

@sandros94 sandros94 changed the title feat: use JWE internally to useSession feat(useSession): use JWE internally Mar 22, 2025
@sandros94
Copy link
Author

sandros94 commented Mar 23, 2025

With d0d0de2 I decided to restart from scratch, as I was having issues when testing the JWEs against other tools. But while doing so I've noticed an issue with the current implementation of base64Encoder and base64Decoder in the encoder utils, in particular with tampering, thus I preferred to create two dedicated ones (in name of security 😬)

My next steps will be:

  1. try to support other password-based algorithms and encryption out of the box
  2. simplify the payload input (since it will be stringified anyway before encryption) should be project specific
  3. list and fully test with JWE implementations from other languages and ecosystems

@sandros94
Copy link
Author

All PBES2 are now supported as well as all GCM content encryption ones. I wasn't able to add CBC out of the box, but this could be done [email protected] IMO.

For now I've tested it against itself, jose and a small Go utility.

The seal/unseal logic is done, other than any changes during review. Now I'll focus on looking at useSession and the various utils to see if there is room for improvement taking advantage of some JWE specific features.

@casualmatt
Copy link

I wrote a simple test to check the compatibility of the generated JWE, and the results look good so far. I tested against two Go libraries and NGINX’s JWT module:

Golang JWE test suite
NGINX JWT auth module

@pi0
Copy link
Member

pi0 commented Mar 24, 2025

Thank you for initiating this dear @sandros94.

While JWE migration looks promising, I prefer to keep backward compatibility with ironcrypto format so users can choose either and won't cause incompatibility with current usages.

It would be cool if at least in the first PR step, we minimize changes to the session.ts and postpone changes like the second expiration.

Also to reduce bundle size we can share utils as much as possible. You mentioned there is an issue in the current base64 implementation (it is ported from Deno utils) can you elaborate more on this?

Thank you for helping on integration tests dear @casualmatt ❤️ your repo link gives 404 9is it private?)

Copy link
Contributor

autofix-ci bot commented Mar 24, 2025

Hi! I'm autofix logoautofix.ci, a bot that automatically fixes trivial issues such as code formatting in pull requests.

I would like to apply some automated changes to this pull request, but it looks like I don't have the necessary permissions to do so. To get this pull request into a mergeable state, please do one of the following two things:

  1. Allow edits by maintainers for your pull request, and then re-trigger CI (for example by pushing a new commit).
  2. Manually fix the issues identified for your pull request (see the GitHub Actions output for details on what I would like to change).

@sandros94
Copy link
Author

sandros94 commented Mar 25, 2025

While JWE migration looks promising, I prefer to keep backward compatibility with ironcrypto format so users can choose either and won't cause incompatibility with current usages.

It would be cool if at least in the first PR step, we minimize changes to the session.ts and postpone changes like the second expiration.

Oh, indeed keeping some backward compatibility would be useful. Tho I do wander how to fully support both.

I'm not familiar enough with iron-crypto, but in JWE we can have any payload, although generally a JSON, in which there are some reserved keys (iss for issuer, exp for expire and various other) and I would like to provide them via a metadata function, much like the current id and data getters, as well as a JWE header one.

To give an example, if I undesrtand the current API:

interface SessionManager {
  id(): string
  data(): T     
  update(): SessionManager
  clear(): Promise<SessionManager>
  header(): JWEHeader      // new
  metadata(): JWTMetadata  // new
}

The header is the first section of the JWE itself, but id, data and metadata would be part of the encrypted payload itself like:

interface Payload {
  id: string
  data: T
  ...metadata // all the supported and arbitrary metadata info (like `iss`, `exp`)
}

This should allow manual implementation for things like exp and other JWE specific features, while leaving the rest of the useSession implementation as it was before, except adapted options, to accomodate both implementations.

@pi0 do you think this would be a reasonable approach?

Also to reduce bundle size we can share utils as much as possible. You mentioned there is an issue in the current base64 implementation (it is ported from Deno utils) can you elaborate more on this?

I haven't deeply looked at the current base64 src code yet. But essentially it was failing the tampering test. This might be due to the fact that the deno fork is able to take care about any potential tampering and reconstruct the original data, but in a JWS/JWE implementation (as far as I understand it) it should be treated as an illegal operation and must be rejected.

I'll look into other potential utils that could be shared, one of which I can think of are key generation and wrapping (there might be some shared crypto logic).

Thank you for helping on integration tests dear @casualmatt ❤️ your repo link gives 404 9is it private?)

It is indeed private, sorry. I'm sure he'll publish it once he notices it

@sandros94
Copy link
Author

Also, on the topic of useSession API, JWE does support zip compression, but it requires full control of the payload (meaning that it should be compressed AFTER serialization, something improssilbe to do with the current implementation).

Please feel free to tell me if this might be too much of a stretch, but I also think this might be easily achievable with a custom useSession implementation that uses directly the seal/unseal utilities

@casualmatt
Copy link

casualmatt commented Mar 25, 2025

404 9is it private?

👉👈 ehh sorry 😁 it should be public now; I will probably add some bun and node test too, with a bash script to check a JTE token against all of them

keep backward compatibility with ironcrypto format so users can choose either and won't cause incompatibility with current usages.

Backward compatibility for Ironcrypto would be nice, but from what I know, it is not a standard, so I think that those who are using it have their own implementation. To avoid keeping Ironcrypto for too long after the introduction of JWE, I could write a guide on how to move from Ironcrypto to JWT/JWE.
Would that be a good idea?

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.

3 participants