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 Json serialization support #150

Merged

Conversation

gjermystreeva
Copy link
Contributor

@gjermystreeva gjermystreeva commented Dec 17, 2020

  • Addresses support JWE JSON serialization too #75 (support JWE JSON serialization too)
  • Have not (intentionally) broken any functionality or introduced regressions. (Other that net40 removal from project which was broken already?)
  • Supports "General JWE JSON Serialization" and "Flattened JWE JSON Serialization"
  • Choice of general / flattened on encrypt made by library based on whether >1 recipient where SerialzationMode.smJson is specified.
  • Handles either general or flattened on decrypt where SerialzationMode.smJson is specified.
  • IKeyManagement implementations have to now support a WrapKey method. (Using an existing CEK). For a couple that is not valid (re-using an existing CEK) - unittests for that.
  • Have added multiple framework targets for the unit test project.
  • Have added GitHub actions for unit testing on a matrix of frameworks + OSes. Ideally before merge should fix or mark as expected failures where there is not support / failing tests. Most failures are pre-existing but now exposed without having to setup the environments ones self.
  • I had changed the implementation in JWT.cs to call into new JWE.cs - but backed-out for now as would remove net461 support, however I'm not convinced how good the support left in master is or should be for net461. I can clean things up if can remove net461.
  • Oh - no tests yet for passing a custom JsonMapper - I'm not 100% sure whether need to or should support that.

@gjermystreeva gjermystreeva marked this pull request as ready for review December 17, 2020 17:53
@dvsekhvalnov
Copy link
Owner

Hi @gjermystreeva ,

sorry for delay, i started looking into your PR. Very much appreciate your effort.
Given amount of changes, it will take some time for me to refresh spec in my head, so bear with me :)

  1. Did i capture it correctly, desired interface to use it is:
string json = Jose.Jwe.Encrypt("top secret information", new [] {new JweRecipient(JweAlgorithm.A256KW, key), new JweRecipient(JweAlgorithm.ECDH_ES, ecKey)}, JweEncryption.A256GCM);

?

  1. Would it make sense to collapse to existing interface may be? Something like:
string json = Jose.JWT.Encode("top secret information", new [] {new JweRecipient(JweAlgorithm.A256KW, key), new JweRecipient(JweAlgorithm.ECDH_ES, ecKey)}, JweEncryption.A256GCM); 

seems it should fit with another overload (though may be we have too much overloads..).

  1. By chance have you tried (or know) it against any other library that supports jwe-json serialization? Compatibility is always important thing.

jose-jwt/jwe/JWE.cs Outdated Show resolved Hide resolved
jose-jwt/jwe/JWE.cs Outdated Show resolved Hide resolved
@gjermystreeva
Copy link
Contributor Author

Hi @dvsekhvalnov,

Hi @gjermystreeva ,

sorry for delay, i started looking into your PR. Very much appreciate your effort.
Given amount of changes, it will take some time for me to refresh spec in my head, so bear with me :)

  1. Did i capture it correctly, desired interface to use it is:
string json = Jose.Jwe.Encrypt("top secret information", new [] {new JweRecipient(JweAlgorithm.A256KW, key), new JweRecipient(JweAlgorithm.ECDH_ES, ecKey)}, JweEncryption.A256GCM);

Yes - that is basically correct, though currently you also need to supply the mode parameter (SerializationMode.smJson) if you want Json serialized (which you need in this example as 2 recipients).
JweTest.cs "EncryptDecrypt_ModeGeneralJsonRoundTripMultipleRecipients_ValidRecipientsCanDecrypt" shows a typical call.

  1. Would it make sense to collapse to existing interface may be? Something like:
string json = Jose.JWT.Encode("top secret information", new [] {new JweRecipient(JweAlgorithm.A256KW, key), new JweRecipient(JweAlgorithm.ECDH_ES, ecKey)}, JweEncryption.A256GCM); 

seems it should fit with another overload (though may be we have too much overloads..).

I didn't want to overload the existing JWT class any further, but preferred a new JWE specific API, similar to the discussion in #75.
Similar to how the API is split for example in https://jwcrypto.readthedocs.io/en/latest/index.html

  1. By chance have you tried (or know) it against any other library that supports jwe-json serialization? Compatibility is always important thing.

I haven't explicitly got tests yet against other libraries, but I have got unit tests based on examples from rfc7516 as well as the useful rfc7520 which has extra examples.

I have read through the codebase for Python's jwcrypto library and also go-jose for reference.

(My plan is to do some testing against Python's jwcrypto library also.)

jose-jwt/jwe/JWE.cs Outdated Show resolved Hide resolved
jose-jwt/jwe/JWE.cs Outdated Show resolved Hide resolved
jose-jwt/jwe/JWE.cs Outdated Show resolved Hide resolved
@gjermystreeva
Copy link
Contributor Author

Hi @dvsekhvalnov - I made an additional change commit 64e74e4... removed need to pass serialization mode from JWE.Decrypt as it makes it easier for end user I think.

@dvsekhvalnov
Copy link
Owner

Ok, i think time to merge it. Thank you so much for contribution!

I'll spend some time testing cross-compatibility and make sure no regressions, as well as try to test new JWE JSON serialization before publishing release. Stay tuned.

@dvsekhvalnov dvsekhvalnov merged commit 4a7209d into dvsekhvalnov:master Jan 27, 2021
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