Add helper for making RSA key from exponent and modulus

[zofer1]

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]

@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]

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]

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]

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.

[prince-chrismc]

left of test code? this seems out of place

[zofer1]

This was a temporary workaround to full the auto pointers

[prince-chrismc]

https://www.openssl.org/docs/man3.0/man3/RSA_free.html

Suggested change

	std::unique_ptr<RSA, decltype(&freem)> rsa(RSA_new(), freem);
	std::unique_ptr<RSA, decltype(&RSA_free)> rsa(RSA_new(), RSA_free);

[zofer1]

This was a temporary version to overcome the double free of BIGNUM due to auto pointers

[zofer1]

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]

I'd suggest keeping the unique ptr and perhaps

RSA_set0_key(rsa.get(), n.release(), e.release(), nullptr)

I noticed that's the original suggestion #298 (review)

[prince-chrismc]

Why did you remove the unique ptr? According to https://www.openssl.org/docs/man3.1/man3/BN_bin2bn.html it needs to be freed 🤔

[prince-chrismc]

Ahhhh

The n, e and d parameter values can be set by calling RSA_set0_key() and passing the new values for n, e and d as parameters to the function. The values n and e must be non-NULL the first time this function is called on a given RSA object. The value d may be NULL. On subsequent calls any of these values may be NULL which means the corresponding RSA field is left untouched. Calling this function transfers the memory management of the values to the RSA object, and therefore the values that have been passed in should not be freed by the caller after this function has been called.

Now I understand the double free comment

[prince-chrismc]

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]

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.

[zofer1]

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]

derp I broke something