Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

--alg should be required when performing signature validation #134

Open
codedust opened this issue Jul 11, 2021 · 3 comments · May be fixed by #135
Open

--alg should be required when performing signature validation #134

codedust opened this issue Jul 11, 2021 · 3 comments · May be fixed by #135

Comments

@codedust
Copy link
Contributor

When no algorithm is given during signature validation, HS256 is used by default. This leads to an error message stating that the JWT has a different algorithm than the one the user provided. PR #133 improves on this by showing the algorithm specified in the JWT and the algorithm used for signature validation (HS256 per default) but still, this is not ideal.

I would therefore suggest to require the --alg parameter when a secret is given (-S).

Current behavior:

$ JWT=`cargo run -- encode '{"field":"value"}' --secret 1234567890 --alg 'HS512' --exp '+1 year'`
$ cargo run -- decode -S 1234567890 $JWT 
The JWT provided has a different signing algorithm than the one you provided

Token header
------------
{
  "typ": "JWT",
  "alg": "HS512"
}

Token claims
------------
{
  "exp": 1657552783,
  "field": "value",
  "iat": 1625995831
}

Expected behavior:

$ JWT=`cargo run -- encode '{"field":"value"}' --secret 1234567890 --alg 'HS512' --exp '+1 year'`
$ cargo run -- decode -S 1234567890 $JWT 
error: The following required arguments were not provided:
    --alg <algorithm>

Even better, it would be nice to allow for specifying multiple valid algorithms like this:

$ cargo run -- decode -S 1234567890 --algs HS256,HS384,HS512 $JWT 
@mike-engel
Copy link
Owner

I mostly set a default because it seems like the most common algorithm is HS256, which would allow most users to avoid having to add an extra flag.

Has the industry moved onto a different algorithm by default? Maybe we can error if the algorithm doesn't match, rather than continue to decode? I think that might be a good middle ground solution which helps everyone. What do you think?

@codedust
Copy link
Contributor Author

From my experience, asymmetric algorithms like PS512 and PS512 are very common (and an absolute must) in scenarios where the authorization server and the resource server are not operated by the same entity. This article gives a great overview over asymmetric algorithms in the context of JWTs.

In general, being aware of the choice of algorithms for signature validation is a good thing. However, setting sane defaults is a good thing, too. With #133, the new warning message provides useful feedback when the algorithms didn't match:

The JWT provided has a different signing algorithm ({:?}) than the one selected for validation ({:?}){/$}

Maybe the actual value of this change is the ability to provide a list of algorithms for signature validation. This is very useful when multiple variations of the same algorithm (e.g. PS256, PS384 and PS512) should be accepted.

To cut a long story short, I think we can also add the HSxxx algorithms as a default choice when #133 is merged. Would that be in your interest?

                      Arg::with_name("algorithms")
                         .help("a comma-separated list of algorithms to be used for signature validation. All algorithms need to be of the same family (HMAC, RSA, EC).")
+                        .default_value("HS512,HS384,HS256")
                         .takes_value(true)
                         .long("algs")
                         .short("A")
                         .possible_values(&SupportedAlgorithms::variants())

@mike-engel
Copy link
Owner

@codedust that works for me, I think

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants