Keats / jsonwebtoken

JWT lib in rust
MIT License
1.64k stars 260 forks source link

Fix algorithm family validation #265

Closed Jonathas-Conceicao closed 1 year ago

Jonathas-Conceicao commented 1 year ago

I've noticed that when Validation has algorithms of different families only the first one was being validated.

For example. if we have:

let mut validation = jsonwebtoken::Validation::new(jsonwebtoken::Algorithm::RS256);
validation.algorithms.push(jsonwebtoken::Algorithm::RS384);
validation.algorithms.push(jsonwebtoken::Algorithm::ES384);
validation.algorithms.push(jsonwebtoken::Algorithm::ES256);

Only AlgorithmFamily::Rsa would be accepted in the JWT instead of AlgorithmFamily::Ec also being acceptable.

This change ensures that all algorithm families are checked.

Keats commented 1 year ago

I'm not very satisfied with this bit tbh, I'd want to be able to specify only from a family to avoid weird behaviours like that

otavio commented 1 year ago

But this would require an API change, wouldn't it?

Keats commented 1 year ago

Yep it would, I'm just thinking what it could look like.

Jonathas-Conceicao commented 1 year ago

Yeah, I agree that the current API for adding expected algorithms is misleading. I think making the AlgorithmFamily enum public and using it instead would show the expected behavior more clearly.

I do, however, feel like the Validation should still support checking against different families, as a particular protocol I was implementing supports both RSA and EC for its JWT.

Jonathas-Conceicao commented 1 year ago

Either way, I'm closing this, as I think the PR is pointless if the intention is to rework the public API.

Keats commented 1 year ago

I do, however, feel like the Validation should still support checking against different families, as a particular protocol I was implementing supports both RSA and EC for its JWT.

I think it's better to check the header to see the alg rather than letting the library pick the alg among multiple families. We don't have the None alg so that's better but still