Invalid multiple algorithm logic

[ereOn]

The Validation takes a Vec of Algorithm but the validation code here incorrectly ensures that the JWT's algorithm is compatible with all the specified algorithms, instead of just ensuring it finds at least one that matches.

Put otherwise, this:

  if validation.validate_signature {
        for alg in &validation.algorithms {
            if key.family != alg.family() {
                return Err(new_error(ErrorKind::InvalidAlgorithm));
            }
        }
    }

Should be more like:

  if validation.validate_signature {
        for alg in &validation.algorithms {
            if key.family == alg.family() {
                // Success: let's move on.
            }
        }

        return Err(new_error(ErrorKind::InvalidAlgorithm));
   }

As it stands, it is impossible to use more than one algorithm in a validation because of this bug.

[Keats]

As it stands, it is impossible to use more than one algorithm in a validation because of this bug.

Yeah that's intentional. Ideally we would solve it at the type level to only allow a single family but I didn't want to change the types too much.

[beanow-at-crabnebula]

Ran into the same problem. So if I understand correctly, in a scenario where all of the validations options are identical but supporting multiple algorithms. We should maintain multiple Validation struct copies per family?

I'm curious why that's intentional.

Is it possible the Validation struct gained a responsibility to ensure a specific DecodingKey is appropriate rather than the DecodingKey's family being used for this?

[Keats]

Most of the time, the Validation struct can be defined in a const way (well lazy_static or once_cell).

Is it possible the Validation struct gained a responsibility to ensure a specific DecodingKey is appropriate rather than the DecodingKey's family being used for this?

As in passing the decoding key to the validation? It's possible but you lose the build once only approach

[beanow-at-crabnebula]

Most of the time, the Validation struct can be defined in a const way (well lazy_static or once_cell).

The issue remains that we'd still have to separate them by algorithm family.
E.g. a (static) VALIDATE_RSA, VALIDATE_EC, ...

As pointed out before the current logic is that: If anything in validation.algorithms has a different family than the DecodingKey family, return InvalidAlgorithm.
So if validation.algorithms contains mixed families, this will always be true.

For reference, my use-case is that we're allowing several algorithms and obtaining the public keys from JWKS.
I was hoping the allowlist could be enforced (again) by specifying the allowed algorithms in Validation.

[PeteX]

Yes I've got the same use case as @beanow-at-crabnebula. If the current behaviour is intentional, can I suggest updating the documentation? It currently implies that it's enough for the token to match a single algorithm from the list:

The validation will check that the alg of the header is contained in the ones provided and will error otherwise.

[mathstuf]

Yeah that's intentional.

Why though? Is there a security concern? If so, what is it so that I can better understand why my naive usage of JWT (so far) might be running afoul of best security practices.

[Keats]

You need to provide a correct key to decode so you should know which alg you're getting

[mathstuf]

I am probably using JWTs in an…unorthodox way, but the key depends on the iss claim (which is copied in the header). So there isn't necessarily a "singe algorithm" (and if that's the case…why are HS256 and HS512 both being there not conflicting?).

My use case:

• we use a robot to do merging of contributions
• during the release process, we want to "block" merging to the managing branch in question
• instead of using some data associated with every MR (labels, metadata in the description) which may easily fall out-of-sync, instead JWTs are pushed to the respository in a hidden namespace
• each token is signed by the issuer and the claims contain information such as:
  • the repository it applies to
  • the branch being locked
  • the MR which is allowed to ignore the lock (e.g., the MR which does the release)
  • the revision that the branch is supposed to be locked on (so that if it changes otherwise (like merging the release MR), the locks expire)
  • a reason so that if one does try to merge, a reason can be provided
• the robot understands which users are allowed to create locks (so that they cannot be spoofed) by matching the iss with their key

I may well be abusing JWTs here, but it really felt like they're almost exactly what is wanted here. I am currently "forcing" EdDSA due to this issue, but if someone wants to use an RSA key, I don't see why that should be disallowed.

[mathstuf]

I suppose I can defer the Validationcreation until after the header's iss's key is known…but that seems unnecessary.

[mathstuf]

See #466 to help out with building the list of algorithms from a family.