Thalhammer / jwt-cpp

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

Add helper for making RSA key from exponent and modulus #298

Closed zofer1 closed 1 year ago

zofer1 commented 1 year ago

While evaluating JWT-CPP I have found a case where we have the public key defined as modulus and exponent. I have added a wrapper function to allow this functionality and hide the openssl details for this.

I have also added adding a set of claims defined as json to a verifier to allow static configuration files in the application level.

For the case where we may apply external claims verification we only want to verify the signature only and skip the claims verification. For this I have split the verify functions accordingly while maintaining a BWC.

closes #271

prince-chrismc commented 1 year ago

@zofer1 this looks really interesting, would you mind splitting the two features in 2 PRs?

The exp+mod has been requested in the past #271 and #160 and this looks similar to what I've suggested... however it is missing unit tests which I would live to see :)

The second half, I am a little hesitant on

and skip the claims verification

What's missing from the current verify that it's not suitable?

You changes look minimal but I would rather not expose an API where devs can make easy mistakes that introduce security vulnerabilities

zofer1 commented 1 year ago

I can split it, not an issue and also add some unit test. It will take some time to do that. I understand the security concern given that the entire specs around this JWT is very generic and basically says that security concerns must be addressed by implementation. However, if one have an implementation with heavy logic of claims processing this might not be a good fit for the hook method of verify_check_fn_t. For this reason I thought it would be a good idea to either do just signature verification or with the basic exp+nbf. For me this can be a "nice to have" thing. The last addition I do think that can help is adding static verifiers from a json.

Thalhammer commented 1 year ago

Hi, thanks for your additions.

While I think the possibility to construct a key from components, as well as the split is a nice thing, I don't think the with_claims function fits with jwt-cpp. Loading a config feels like something that should be handled by the application.

zofer1 commented 1 year ago

Hi, thanks for your additions.

While I think the possibility to construct a key from components, as well as the split is a nice thing, I don't think the with_claims function fits with jwt-cpp. Loading a config feels like something that should be handled by the application.

The inspiration for this is the parse_jwks function. While this is based on rfc7517#section-5 there is nothing about claim set. Of course application can do this logic but relying on the heavy templating seems nicer from the inside of the package.

I do have additional wrapper in the application layer that is doing additional work. this work results in feeding a verifier with subsequent calls to with_claim which I suggest to replace with call to with_claims. This gives automatic handling for the special cases and saves the effort to handle this specifically in application code.

zofer1 commented 1 year ago

Sorry I am new to working with GitHub so I thought I only have draft of the pull request and I knew there are some additional changes I need to make. I have created a new PR with relevant changes, I hope you find it better :-). This PR includes just the first part we have agreed on, that is the create RSA pub key from components including unit test

prince-chrismc commented 1 year ago

I just noticed that the 1.0.2 CI failed and looking into it, I noticed this function was not invented then https://www.google.com/search?sitesearch=www.openssl.org&q=RSA_set0_key

so it will need some ifdef logic to disable this function

dr0pdb commented 1 year ago

Just FYI since it might be of interest, I ended up implementing this for RSA and EC keys. Link for the code is in this comment - https://github.com/Thalhammer/jwt-cpp/issues/271#issuecomment-1673084618. So may be it can used as a reference for adding the support for EC keys as well.

zofer1 commented 1 year ago

Just FYI since it might be of interest, I ended up implementing this for RSA and EC keys. Link for the code is in this comment - #271 (comment). So may be it can used as a reference for adding the support for EC keys as well.

Yes I wanted to get to that sometime. For me there is a learning curve in both cryptography and open source contribution so I planned to do this after the first PR is approved. Now that I see it takes time, I will probably get to that by the time it is approved and add it here.

prince-chrismc commented 1 year ago

derp I broke something