Add support for specific JOSE header types

lmm-git commented 5 months ago

Check https://www.rfc-editor.org/rfc/rfc7519#section-5.1 and https://datatracker.ietf.org/doc/html/rfc7515#section-4.1.9 for more information on when the typ header should be set and to which value.

This commit allows to skip the check altogether as warranted by RFC 7515 the use of the header is optional but if set, according to RFC 7519, it should be set to "JWT". For other token types this is different (see RFC 9068).

Resolves #160

ramosbugs commented 3 months ago

Superseded by #175

lmm-git commented 3 months ago

Hm, this is a bit disappointing as I waited for your review for three weeks :(

Also the other MR does not cover the needs for verifying other types correctly. Even more, I think it makes this library not standard compliant as e.g. it does not handle application/ strings correctly.

ramosbugs commented 3 months ago

Hey @lmm-git, I saw my unresolved comment about is_typ_check_enabled above but missed that I had agreed to merge option 3 in #160. Apologies for the miscommunication and delay (have a newborn at home as of ~3 weeks ago). I'll reopen this PR and the associated issue.

lmm-git commented 3 months ago

Thanks for your response!

How do we want to proceed now? My proposal would be to revert the commit from #175 on this branch and apply the already existing commit. Or do you like it more if I rebase on top of the current main?

(Would do either of those options tomorrow :)

will118 commented 3 months ago

Apologies, I misunderstood the status of this PR. I thought it had stalled on the review comment.

Looking forward to this being merged.

lmm-git commented 3 months ago

I addresses your comments and fixed some more typos in 81878a5957951b25551abba1b96e2296e0ba9936

With e3ccb3cac2029dc49b699d2c2ec59b71abb76b91 I introduced a new type of a normalized json web token type. I am really not sure whether it makes the code better, but some of my reasons are:

De-couple the parsing of the type field from the verification
It is clear whether some function handles a normalized type value or not without any runtime penalties
Some can use the parsing not only during verification

However, I see some reasons why not to include this commit:

It creates a new type, which type is uncommon for this crate
It makes understanding the code a bit more complicated
Users might create normalized objects containing non normalized values (I kept this possibility to allow be able to use the new_type macro and to allow creating these values from constants)

So what do you think? I am completely fine with just leaving out the second commit. It was just an experiment of me whether it makes the code better (and I am really not sure whether it does or not)

ramosbugs commented 3 months ago

Thanks!

lmm-git commented 3 months ago

Thank you for your extensive reviews!

ramosbugs / openidconnect-rs

Add support for specific JOSE header types #161