Add JWKS parsing

[ghost]

builds on top of #112

closes #112

[Thalhammer]

Looks, good, but I have a couple of ideas/additions:

• Liniting fails, (there is a clang-format file in the root that should work with most ide's)
• None of the functions have error_code overloads. We moved the entire library to allow usage without (nonfatal) exception and I'd like to continue this.
• I really dislike the way asymetric keys are passed to the library right now (4 strings for public,private key and a password for each one). I'd like to move to a class abstracting this into a single argument (along with convenience methods) eventually and imho this sounds like a good chance to do so.
• Testing could be more extensive (take a look at the coverage report), but we can add this later once its feature stable.

[ghost]

Thanks for the feedback

1. I'll fix the linting issues right away
2. For the error_code overloads, I'm thinking of targetting the following functions - what are your thoughts?

jwks::get_jwk
jwk::get_algorithm
jwk::get_key_id
jwk::get_x5c
jwk::get_jwk_claim

I will add more unit tests and think of a way to abstract away the asymmetric-keys like you suggested

[Thalhammer]

I was actually thinking more about the parsing and not the accessors. I am not a computer rn, but you could take a look at the existing code (in particular the verifier) to check which methods have error_codes.

[ghost]

The parse_jwk and parse_jwks methods invoke the constructors of their respective classes. These methods throw an exception only when the json is malformed, or when there are no keys available. I'm assuming that this is similar to the decode method, which also throws an exception in case of invalid input.

However, if you feel this shouldn't throw an exception and instead use the error_code mechanism - i will make the change, just wanted to confirm if that's the intended behavior.

[Thalhammer]

I'm assuming that this is similar to the decode method, which also throws an exception in case of invalid input.

I stand corrected on my own library. You are right they do throw on invalid input.
That said it might be a wise idea to move forward to using error_codes there as well.
@prince-chrismc what do you think about this ?

We normally have both overloads, one that takes an error_code and only throws if something really bad happens and one that basically calls the error_code overload and throws if the error code is set.

But I think it might be a good idea to get it merged in some form at least and then iron out this stuff. Shouldn't be a problem as long as it is not part of a release.

[ghost]

I agree - I can send follow-up PR's to change the parse_jwks, parse_jwk, and decode to take in an error_code. I've added a few more unit tests - the classes jwks and jwk are more or less completely covered now.

[Thalhammer]

Ok so I finally came around to actually download the branch and take a look at the jwk specification.
I noticed you only expose a selection of the valid properties (alg, kid and x5c), while

• kty
• use
• key_ops
• x5u
• x5t
• x5t#S256

dont have an accessor. Was this a deliberate decision (e.g. to keep the interface simpler) or an oversight ?
Depending on the kty there can also be other fields like x & y in elliptic curve keys (I don't think they should have direct accessors directly inside the jwk, rather the algorithm should probably get a constructor or static function to load a key from jwk).

It might also be a good idea to check that members required by the spec (e.g. kty) are present and have the correct format.

I don't think it makes sense to add parsing of the keys right now, instead I think we should merge this with the basic structure (which should already be helpfull to people) and follow up with a number of other pr's.
In particular I'd like to do the following:

• Get the base stuff error_code ready and finish transition to error_code interfaces in normal jwt parsing
• Add some sort of "key" type as a member of the algorithm (since they differ vastly) which has functions for parsing various formats (where they make sense), as well as parsing from jwk.
• Add some sort of keychain class which is a list of keys grouped by algorithm (and keyid?)
• Some way to add all keys in a jwk to the allowed algorithms in a verifier.

I am still wondering whether or not to take a look at encryption. Since we already depend on openssl/libressl we would not introduce new dependencies by adding it, but I wonder how many people actually use it (i.e. if its worth the effort).

[ghost]

It was an oversight - I had mistakenly assumed the OP had completely implemented the spec, my bad - I should have verified my assumptions.

I agree with your list of TODO items - and I think the next step after merging this is to transition to error_code interfaces and stabilize the API before adding more features

[prince-chrismc]

You are right they do throw on invalid input.
That said it might be a wise idea to move forward to using error_codes there as well.
@prince-chrismc what do you think about this ?

No objections! 👍

[prince-chrismc]

Looks out of place 🤔

[Thalhammer]

@tchinmai7 Thanks for the work you put into this :)

[ghost]

You're welcome, and Thank you very much for this awesome library!