Keats / jsonwebtoken

JWT lib in rust
MIT License
1.65k stars 263 forks source link

Decode, verify signature, validate #197

Closed btielen closed 2 years ago

btielen commented 3 years ago

The issue Thanks for all the work in this repository! When I first used the library it wasn't clear for me if the decode function would just decode or also check the signature and/or the expiration date. So I made an overview:

Function name What does it do?
dangerous_insecure_decode Just decode
dangerous_insecure_decode_with_validation Decode and validate exp, sub, iss, aud ...
verify_signature Decode and check signature
decode Decode, check signature and validate exp, sub...

This feels inconsistent to me.

Possible solution A solution could be to refactor the code and split the decoding/verification process. Something like

struct DecodingOptions {
  algorithm: 
}

fn decode(token: &str, key: DecodingKey, options: DecodingOptions) -> Result<TokenData>
struct VerificationOptions {
  algorithm: 
  ignoreSignature: bool,
  ignoreExp: bool,
  ignoreSub: bool,
  ignoreIss: bool
...
}

fn verify(token: &TokenData, key: DecodingKey, options VerificationOptions) -> Result<VerifiedToken>

Consequences

I could help!

Keats commented 3 years ago

I think dangerous_insecure_decode_with_validation and dangerous_insecure_decode can be folded into a single function. We can add a Validation::none() function that disables all validation and we have

// Equivalent to current `dangerous_insecure_decode`
dangerous_insecure_decode(token, &Validation::none());
// Equivalent to current `dangerous_insecure_decode_with_validation`
dangerous_insecure_decode(token, &Validation::default());

As for verify_signature, it's now the same as calling decode except the payload is decoded in decode. I think we can mark this private.

Keats commented 3 years ago

@btielen can you have a look at https://github.com/Keats/jsonwebtoken/pull/199 ?

fujiapple852 commented 2 years ago

@Keats for what it's worth, the new API to replace dangerous_insecure_decode_with_validation feels a little awkward to use as it involves explicit mutation of the Validation (perhaps a fluent-interface style builder pattern would be better here?) and also passing a dummy DecodingKey to decode (perhaps this can be wrapped in an Option?).

let algorithm = jsonwebtoken::decode_header(token)?.alg;
let mut validation = Validation::new(algorithm);
validation.insecure_disable_signature_validation();
Ok(jsonwebtoken::decode::<Self>(
    token,
    &DecodingKey::from_secret(&[]),
    &validation,
)?
.claims)

In any case, thank you for your work on this useful library.

Keats commented 2 years ago

Yeah it's kind of a downside but I didn't feel like it was worth improving since it's not really something that should be done often.

perhaps a fluent-interface style builder pattern would be better here?

I'm not a fan of builders, would a ValidationBuilder really be more elegant?

In any case, thank you for your work on this useful library.

Thanks!