dvsekhvalnov / jose-jwt

Ultimate Javascript Object Signing and Encryption (JOSE), JSON Web Token (JWT) and Json Web Keys (JWK) Implementation for .NET and .NET Core
MIT License
938 stars 184 forks source link

Unencoded Payload Option, RFC 7797, Detached content support. #111

Closed EricKeij closed 4 years ago

EricKeij commented 5 years ago

Does the library support an option to NOT base64 encode the payload for signature calculation?

We are using detached payload with the b64 header set to false. In which case the payload should not be base64 encoded for signing input as specified at https://tools.ietf.org/html/rfc7797#section-3

dvsekhvalnov commented 5 years ago

Hi @EricKeij , no library don't have support for RFC 7797 at the moment. But i don't mind to add it or to accept your patch and do release to nuget (fastest option :)

Do you know other library, possible in other language, that supports RFC 7797? Usually nice to do interop testing.

Just curios what is your use case?

EricKeij commented 5 years ago

Our use case is that we use detached payload and it seemed to make sense to skip the base64 encoding as there is no need to. But we didn't really realize this RFC breaks to orginal JWT RFC stating to not use unencoded payload and thus requires support from the library.

This framework seems to support it https://github.com/web-token/jwt-doc

dvsekhvalnov commented 5 years ago

By detaching payload you mean you:

  1. encode token with some data (payload)
  2. cut out payload part from resulting jwt, replacing with empty string (e.g. have .. in the middle)
  3. receiver injects encoded payload back and validates

?

EricKeij commented 5 years ago

That's exactly what we do.

dvsekhvalnov commented 5 years ago

Ok, cool, let me ask couple more things. Are you using or planning to jose-jwt on both sides or you are using different libraries?

The reason i'm asking, 'detaching payload' is better handled by library itself then outside. Because when dropping base64url encoding, it's super easy to break validation part by simply have illegal bytes in payload (e.g. payload with dots for instance).

EricKeij commented 5 years ago

The "other side" are several other parties so likely there will be many different technologies/libraries used. We came to the conclusion to skip using the unencoded payload option as there are no great benifits. And it doesn't seem to be widely used and not supported by many libraries.

dvsekhvalnov commented 5 years ago

Ok, i see. So i'll keep ticket open, it looks interesting, will try to get some more info about community adoption for both rfc7797 and detached content. To see if it worth implementing.

idigber commented 5 years ago

Hello @dvsekhvalnov,

I was also looking to see if the library supported the Unencoded Payload Option - RFC 7797. I have used it in Jose4j(Java) and in the same library @EricKeij mentions.

@dvsekhvalnov, do you have any updates on whether you will implement it soon?

@EricKeij, did you find an alternative C# library which allows you to do this? If so kindly share with me.

Thanks

dvsekhvalnov commented 5 years ago

Hi @idigber ,

honestly i was under impression that RFC 7797 is not in wide use :)

But ok, given more than one person asking about it, i'll resurrect the ticket. You also welcome to provide contribution if you want :)

simonedamico commented 5 years ago

RFC 7797 is now adopted in the UK OpenBanking standard. Is anybody already working on this?

dvsekhvalnov commented 5 years ago

Hi @Neku , will try to add support soonish. Alternate i'm accepting pull requests if you willing to contribute.

simonedamico commented 5 years ago

It's hard for me to contribute because I can't open the solution on a Mac with a recent version of dotnet core. I ended calling a bunch of public APIs of this project to achieve the same result

dvsekhvalnov commented 5 years ago

Ok, will try to allocate time and add b64 header support.

bengisugultekin commented 5 years ago

Hi all,

@dvsekhvalnov, do you have any updates on whether you will implement it or not?

Did anyone find an alternative C# library that allows you to do this? I found that Jose4j(Java) and jwt-doc (for php) are supporting but could not find anything for C#.

Thanks in advance.

dvsekhvalnov commented 5 years ago

Hi @bengisugultekin ,

sorry had almost no time to look into that recently. Still in mood to implement but can't give any specific ETA.

If you really badly want it, best bet can be to make PR? And i'll help with compatibility testing to make sure feature works with other libraries that supports it?

magaras commented 5 years ago

That would be indeed a very nice feature.

Really nice work by the way.

Regards,

ncichris commented 5 years ago

Hi @dvsekhvalnov , First of all thank you for creating this library.

I also needed b64 option and I ended up implementing it in your library. The changes are not a lot but might have some side effects (listed under issues below), which I am not sure how you would prefer to handle them. The main idea behind my changes, is before encoding or decoding to check the header first for the b64 option. If it does not exist or if it is true then the library works like before.

During Encoding if the b64 option is false then I Compact.Serialize the header only, and add to that the unencoded payload by using Encoding.UTF8.GetBytes. During Decoding if the b64 option is false, then I base 64 decode the header, and leave the payload as is. Then the data to be signed is prepared using the same method as the Encoding above.

The changes were tested on forgerock.financial, which follows the UK Open Banking Standards.

Issues:

Let me know how you would like to proceed.

Update: Although I prefer not to change the API of the library, after further playing with different scenarios using the above approach, I came to the conclusion that the detached signatures should probably have their own functions. For example:

Also for decoding instead of:

This will prevent issues mostly related to parsing tokens of the form: iOmZhbHNlLCJodHRwOlwvX.{json: payload}.2luZy5vcmcudWtcL2lh where the payload might have data with the dot "." character. Of course even in this case the parsing could find the first and last occurrence of "." and still work, but the feel the new approach seems cleaner.

Another approach could be to be more explicit about everything like Nimbus, but that would require too many changes reference: https://bitbucket.org/b_c/jose4j/wiki/JWS%20Examples#markdown-header-using-the-rfc-7797-jws-unencoded-payload-option

dvsekhvalnov commented 5 years ago

Hi @ncichris , mind sharing some code? ) Hard to comment without seeing stuff. If you don't feel MR possible yet, probably just push it somewhere and i'll take a look?

ncichris commented 5 years ago

Hi @dvsekhvalnov, although in the end I decided to use Microsoft's JsonWebTokenHandler to do the job, that option is not ideal either. I had to derive from that class and use some helper functions and reflection to make it work as I required. Another issue with JsonWebTokenHandler is that it only supports Json payloads, so I had to also create a "fake" Json object in order for most of the functionality to work as expected.

But I would be glad to polish the code I worked on this library and create a PR. In the JsonWebTokenHandler case I decided to create two new functions CreateTokenDetached to generate the token with empty payload and ValidateTokenDetached(token, payload) to validate the token by attaching the payload. It looks much cleaner and easier to understand.

I will port the code to your library and create a PR for you to review.

I appreciate your prompt response.

ncichris commented 5 years ago

Hi @dvsekhvalnov, I created a PR for your review. I removed all the project changes and tried to minimize the changes to the library as much as possible, to prevent any breaking changes. I also added a small test.

dvsekhvalnov commented 5 years ago

Cool, thanks @ncichris ! let me take a look and think a bit how to better incorporate it.

dvsekhvalnov commented 5 years ago

@ncichris , so here are my thoughts on new interface(s) so far:

  1. decoding, pretty much as you proposed, new methods:
    
    byte[] DecodeDetached(string token, object key, byte[] expectedPayload, JwsAlgorithm alg, JwtSettings settings = null);

byte[] DecodeDetached(string token, object key, byte[] expectedPayload, JwtSettings settings = null);

// Can be used something like: var payload = JWT.DecodeDetached(token, myBinaryStuff, verificationKey); //myBinaryStuff just returned back for consistency


2. Encoding there are couple possible options:

2.1 use new dedicated method(s):
``` c#
public static string EncodeDetached(string payload, object key, JwsAlgorithm algorithm, IDictionary<string, object> extraHeaders = null, JwtSettings settings = null);

public static string EncodeDetached(byte[] payload, object key, JwsAlgorithm algorithm, IDictionary<string, object> extraHeaders = null, JwtSettings settings = null);

2.2. add options object instead to affected methods:

public static string Encode(string payload, object key, JwsAlgorithm algorithm, IDictionary<string, object> extraHeaders = null, JwtSettings settings = null, JwsOptions = null);

public static string EncodeBytes(byte[] payload, object key, JwsAlgorithm algorithm, IDictionary<string, object> extraHeaders = null, JwtSettings settings = null, JwsOptions options = null);

//can be used like:
var token = JWT.Encode(payload, signingKey, JwsAlgorithm.RS256, options: new JwsOptions { DetachPayload = true, EncodePayload: false }

I'm leaning towards 2.2 with options object as it allows have options in future (possible use full serialization vs. compact, e.t.c.) without growing number of methods, it's too much of them already. But open to opinions.

Thoughts?

ncichris commented 5 years ago

Hi @dvsekhvalnov , as you said I also prefer to leave the API as close to the original as possible, to prevent breaking changes and to minimize the number of new functions.

My only concerns for the JWT.Encode 2.2 option, are the following:

  1. What happens if the "options" parameter and the "extraHeaders" parameter have conflicting values? e.g. in options EncodePayload = true but there is a b64 = false header value in the extraHeaders. A logical solution could be to throw an exception. Also if EncodePayload = false and the caller did not include the b64=false extraHeader then the library must add it. It is also required to add it as a critical header "crit" as per the RFC.
  2. Regarding the combination of EncodePayload = false, and Detatched = false, as per the RFC it is not really recommended since it can create parsing problems. For example if the payload includes the "." character then most jwt libraries would probably fail e.g. if the payload is $.02 then the base64(header).payload value will be something like sImNyaXQiOlsiYjY0Il19.$.02

Should #2 be allowed? I think it should not be allowed in normal circumstances. Link to the RFC https://tools.ietf.org/html/rfc7797

dvsekhvalnov commented 5 years ago
  1. That's fine and actually how it supposed to be :) I'll explain:

    • by default if you don't want to manage headers, library will add b64 and crit for you if you ask not to encode payload
    • if you want to manage headers yourself - it's up to you what you do, library is not stopping you. You can generate "semantically invalid" tokens if you like. Many reasons why (you have cross compatibility issues with other library, want it for testing, e.t.c.)
  2. Same thing as (1), yes unencoded payload can create problems parse issues with compact serialization (though can work in others). But RFC not saying you must detach content when using b64 or always skip encoding if you want to detach payload. Again up to you what you'd like to do, there is always default standard way but you can choose your own.

It's supposed to be low-level and give everybody primitives to do encoding/decoding with signing, encryption, e.t.c. But it was never a goal to provide semantic interpretation for jwt world, it simply too broad context. On the contras everybody can build its own JWT validator on top of library that meets exact needs :)

ncichris commented 5 years ago

Sounds good.

dvsekhvalnov commented 5 years ago

okay, sounds like plan. I think will be less work if i pick up your change @ncichris and add to existing vs2015 project than try to upgrade everything (which is like time black hole every-time). And then let you try it out in some real project(s) you have.

How does it sound?

dvsekhvalnov commented 5 years ago

https://github.com/dvsekhvalnov/jose-jwt/tree/rfc_7797

dvsekhvalnov commented 5 years ago

Hey folks, anybody interested in trying detached/unencoded support while i'm polishing everything for nuget submission? @EricKeij @bengisugultekin @ncichris ?

Code in rfc_7797 branch, cross-tested with jose4j. I can build and upload binaries (dll) somewhere to simplify testing.

magaras commented 5 years ago

Hi @dvsekhvalnov,

I am trying the new branch (_rfc7797). So far so good (b64 header support & detached token with PS256 signing algorithm).

The only thing i noticed is that in case of EncodedPayload == false, the "crit" JOSE header is reset.

In case someone wishes to have additional values in this JOSE header, they are lost. Instead, the "b64" value could be pushed into that array if not present already.

Best Regards,

dvsekhvalnov commented 5 years ago

@magaras yeah, make sense, thanks for catching it, i'll add tests.

dvsekhvalnov commented 5 years ago

@magaras pushed fix for 'crit' headers, if you'd like to give it another round ;)

magaras commented 4 years ago

Hi @dvsekhvalnov, I'give it a try next week and let you know. Wish you a pleasant weekend.

dvsekhvalnov commented 4 years ago

Okay, pretty much ready to release it on Monday. Anybody final call or comments or whatever?

magaras commented 4 years ago

Hi @dvsekhvalnov , I tested the implementation for the "crit" headers. It seems fine. Thanks,

dvsekhvalnov commented 4 years ago

@magaras thanks for testing !

Okay, all released, please find v2.5.0 on nuget.

Taraawan commented 2 years ago

RFC 7797 is adopted in the UK OpenBanking standard. Is anybody already working on this using nodejs? I use Jose library but still i am stuck can any one please guide me

dvsekhvalnov commented 2 years ago

Hi @Taraawan, didn't get the question. This is not nodejs library but rather C#/.net one.

What are you looking for?

rytis-dobranskis commented 1 year ago

Hi @dvsekhvalnov,

From OB specifications, we want to be able to validate JWT with header: { "alg": "PS256", "kid": "90210ABAD", "http://openbanking.org.uk/iat": 1501497671, "http://openbanking.org.uk/iss": "0015800001041RHAAY/HQuZPIt3ipkh33Uxytox1E", "http://openbanking.org.uk/tan": "openbanking.org.uk", "crit": [ "http://openbanking.org.uk/iat", "http://openbanking.org.uk/iss", "http://openbanking.org.uk/tan"] }

It seems that including "crit" claim with [ "http://openbanking.org.uk/iat", "http://openbanking.org.uk/iss", "http://openbanking.org.uk/tan"] results in "invalid signature" validation outcome.

Also claims starting with "http://" are not returned from JWT.Headers(jws). However I can re-de-serialize IDictionary to MyHeader just fine while decorating props with something like this [JsonProperty("http://openbanking.org.uk/iat")].

dvsekhvalnov commented 1 year ago

Hi @rytis-dobranskis ,

mind open separate issue (this one closed) and attach unit test or just sample code to try? JWT.Headers returns just IDictionary, curious to see how it missing keys.