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

JWE: allow general (non flattened) serialization syntax #351

Merged

Conversation

Thomas-Mollard
Copy link
Contributor

@Thomas-Mollard Thomas-Mollard commented Apr 4, 2024

When we call .serialize() on a JWE with 1 single recipient, it uses the flattened serialization syntax where the member of the unique recipient (header and encrypted_key) are added in the top-level JSON object instead of in the recipients

However, according to the RFC for JWE, the general serialization syntax allows for the JSON to have a recipients with only one item

In this PR, I am adding the possibility to use the general (non flattened) serialization syntax by setting flattened=False when initializing the JWE (flattened=True is set by default)

The change I did is the "easiest" one I see as it doesn't cause any breaking change and only updates the internal objects when adding a recipient as is expected depending on the general/flattened serialization used

We could also:

  • keep flattened in __init__, always add recipient in recipients when calling .add_recipient and serialize differently depending on self.flattened when calling .serialize
  • always add recipient in recipients when calling .add_recipient, add a bool flag flattened on serialize without the check on number of recipients (different as compact) and serialize differently depending on the flagflattened when calling .serialize

Let me know if you prefer than I implement this in another way

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

Looks good, but please refactor the if/elif/else as described.

Also add at least 1 test where you set flattened to false and then verify that you get an object with only one recipient in it.

@Thomas-Mollard Thomas-Mollard requested a review from simo5 April 4, 2024 14:24
Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM, just a nit to make syntax tests happy

@simo5
Copy link
Member

simo5 commented Apr 4, 2024

please squash everything in a single commit with a comment that briefly explain what this change is about

Copy link
Member

@simo5 simo5 left a comment

Choose a reason for hiding this comment

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

LGTM, just need to squash all in a single commit

@Thomas-Mollard Thomas-Mollard force-pushed the jwe/allow-non-flattened-representation branch from 0dc11ef to 3fe1c2f Compare April 4, 2024 14:44
@Thomas-Mollard
Copy link
Contributor Author

LGTM, just need to squash all in a single commit

Thanks!
Is it OK now?

@simo5
Copy link
Member

simo5 commented Apr 4, 2024

LGTM, just need to squash all in a single commit

Thanks! Is it OK now?

A little more meat on what the change did would have been nice, but it is good enough, I will merge when tests are all green

@simo5 simo5 merged commit 1169eca into latchset:main Apr 4, 2024
14 checks passed
@Thomas-Mollard Thomas-Mollard deleted the jwe/allow-non-flattened-representation branch April 4, 2024 16:26
@Thomas-Mollard Thomas-Mollard restored the jwe/allow-non-flattened-representation branch April 4, 2024 16:26
@Thomas-Mollard Thomas-Mollard deleted the jwe/allow-non-flattened-representation branch April 4, 2024 16:26
@Thomas-Mollard
Copy link
Contributor Author

Thanks a lot for your help and handling this so quickly!
Do you plan to make a new release including this change soon by any chance?

@simo5
Copy link
Member

simo5 commented Apr 4, 2024

Making a release is not hard work, if you have a need we can arrange to make one soon.

I generally do not rush them out an instead accumulate a few changes to reduce the churn though. The only exception is security releases, I churn them out as fast as needed.

@Thomas-Mollard
Copy link
Contributor Author

There is no real urgency per se on our side to have a release as we have a workaround but it's not super pretty and it would ultimately be better to use this change

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