search

Thalhammer / jwt-cpp

A header only library for creating and validating json web tokens in c++
https://thalhammer.github.io/jwt-cpp/
MIT License
925 stars 242 forks source link

What is the roadmap about supporting JOSE? #94

Open mohsenomidi opened 4 years ago

mohsenomidi commented 4 years ago

Dear @Thalhammer

Thanks for this useful library and recent changes are wonderful, we can use the json library like simdjson or rapidjson for performance point of view

Since you develop this jwt library, do you have any plan to imitate the library for JOSE completely, I mean jwe, jws, jwk, etc?

In order to answer the questions about future plans. This has been edited to give a summary. All contributions are welcomed! If you need a feature we will do our best to help you add it!

Thalhammer commented 4 years ago

Hi, While I absolutely think those might give a great addition without to much added stuff, I honestly don't have the time right now to read through and implement those.

However @prince-chrismc did a lot of contributions lately and @Sp3EdeR works on implementing EdDSA support right now, so going forward I am pretty sure we will implement it eventually. It's just that theres no timeline for it. If you need it soon and wanna implement it yourself I would ofc happily merge a PR if it matches the overall style of the library.

kleinmrk commented 2 years ago

@Thalhammer @prince-chrismc what is your take on loading keys from JWK? I experimented with it a bit and I can also contribute a minimal working feature but it feels like the interface is not optimal and I would like to avoid putting effort into something that will be dismissed as "clumsy to use".

Namely, it looks like there is missing a class that represents keys. @Thalhammer already mentioned something similar here. Keys are currently loaded directly from certificates/PEM encoded public keys and stored in jwt::algorithm structs. Does it make sense to introduce a class that encapsulates a key that one can pass to jwt::verifier and jwt::builder? The code could look somewhat like this:

jwt::jwk key = jwk::build_from_certificate("-----BEGIN CERTIFICATE...")
auto verify = jwt::verify().allow_algorithm(jwt::algorithm::rs256(key));
verify.verify(jwt::decode(token));

and in future one could implement reading JWK from JSON

jwt::jwk key = jwk::build_from_json(R"({"kty" : "oct", "k" : "FdFYFzERwC2uCBB46pZQi4GG85LujR8obt-KWRBICVQ"})")
auto verify = jwt::verify().allow_algorithm(jwt::algorithm::hs256(key));
verify.verify(jwt::decode(token));

Such change would probably result in decoupling jwt::algorithm structs and EVP_PKEY/whatever is used in hmac algorithms. I did not put much mental effort into how that could look like in the end. Just trying to figure out what general attitude towards such changes is :)

Thalhammer commented 2 years ago

Loading keys from jwk and jwks is definitly something I'd like to support in the future. I don't personally like jwks much because it is an invitation to loading untrusted keys and has numerous problems (CVE-2018-0114, JWKS Spoofing, just to name a few), however it is a fixed standard and if used correctly is probably fine.

Namely, it looks like there is missing a class that represents keys.

Like you already noticed, the interface (if one can call it that) is pretty suboptimal for anything more advanced. We really need to clean that part up quite a bit. I think its fine to wrap a EVP_PKEY for asymmetric and std::string for symetric keys, since we rely on openssl (or a close enough replacement) anyway. I was thinking about something like that:

classDiagram
class jwt_key {
    +type get_type()
}
class jwt_asymmetric_key {
    +std::shared_ptr<EVP_PKEY> get_key()
}
class jwt_symetric_key {
    +std::string get_key()
}

jwt_key <|-- jwt_asymmetric_key
jwt_key <|-- jwt_symetric_key

^^ Nothing special, but I wanted to try out the new inline diagrams.

We should also provide a way to convert between jwk and key. I don't think we should directly integrate the crypto part with the jwk class (which is more of a convience wrapper for parsing). But I am open for discussion on that part.

Such change would probably result in decoupling jwt::algorithm structs and EVP_PKEY/whatever is used in hmac algorithms.

Not necessarily. One could simply parse the values in the jwk in the algorithm constructor just like we parse the pem there at the moment. I think we should clean up the constructor params (especially for asymmetric) since atm half of the args is unused at any given point (You don't need the private cert for verifying if you pass in the public one and you dont need the public one if you pass the private part).

Another thing worth considering might be some sort of "keychain" concept. Jwt's have a header claim "kid" which defines the key that should be used for verifying the token, which means you can have a number of valid keys (for the same algorithm) and depending on the token one is choosen. If you want to make use of this at the moment you would have to parse the header of the decoded token, load the appropriate key (making sure not to screw up, since the value in there is not trusted yet), build a matching verifier and use that one to verify the token. What we could do now is have a interface class key_chain that provides methods for looking up the key based on kid and pass that class to the algorithms. The default case is then simply a in_memory_key_chain with a single key stored in it. A jwks would be a key_chain as well (or provide methods for converting to an in_memory_key_chain.

TLDR: Yes we need an abstraction for keys and yes we need a way to import/export jwk/jwks (as well as der, pem, ...).

prince-chrismc commented 2 years ago

but I wanted to try out the new inline diagrams.

Me too 😱 They look really good!

I can also contribute a minimal working feature but [...] I would like to avoid putting effort into something that will be dismissed

We will never be dismissive 🤗 The JOSE ecosytem is so wide. Contributions will always be considered since lots of other will benefit from the work.

Thanks a lot for sharing first ❤️

JWK(s) are growing in popularity, I think this is an excellent addition. If the first implementation is "suboptimal" that's why we are allowed unlimited PRs 😆 and it can always be improved (unlike my jokes, those seems to only get worse).

Does it make sense to introduce a class that encapsulates a key that one can pass to jwt::verifier and jwt::builder?

Yes! I think this is a great value add, it makes the whole integration easier for clients side (and maybe server)

We should also provide a way to convert between jwk and key

Big fan of this, I have done the work for RSA, so we probably should template the algo's out to be able to add more in the future https://github.com/Thalhammer/jwt-cpp/issues/160#issuecomment-910611571

Green light from me!

kleinmrk commented 2 years ago

@Thalhammer

Another thing worth considering might be some sort of "keychain" concept.

What about baking "keychain" directly into the verifier class? The verifier class already supports a similar concept via allow_algorithm interface. Thus the verifier would hold a list of keys (possibly) identifiable by key id. The verifier then can look up the corresponding key given that JWT references the key by "kid". As an example allow_key interface sounds like a logical extension to allow_algorithm.

We should also provide a way to convert between jwk and key. I don't think we should directly integrate the crypto part with the jwk class (which is more of a convience wrapper for parsing). But I am open for discussion on that part.

A JWK can also contain information about a context in which the key can be used, e.g., algorithm that is to be used with the key (RS256 vs. RS384). I believe that right now life will be easier when one could validate these parameters during verification, e.g., if JWT contains "alg":"RS256" and the key specifies "alg":"RS384" then the verifier class can report something like algorithm mismatch error. Thus I suggest keeping crypto key in the jwk class for the time being. There is still the option to separate jwk/json and crypto pieces in the future without breaking API (it should be possible to have allow_key interface with taking either jwk or some other key type as argument).

TL;DR: Should the verifier class keep a list of trusted keys (keychain)? Should crypto keys (EVP_PKEY and std::string) be part of the jwk class?

prince-chrismc commented 2 years ago

Should the verifier class keep a list of trusted keys (keychain)?

Trusted as in passed to the allow_key method you are proposing?

Should crypto keys (EVP_PKEY and std::string) be part of the jwk class?

I don't see why not, we have the notion of Algorithm and I am open to it being shared with jwk since the overlap