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

feat: Improved error messages/warnings #133

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

codedust
Copy link
Contributor

New: If no secret/publicKey is provided by the user, no signature check
is performed. Rather then silently skipping the signature check, this
commit introduces a warning in this case.

New: If the exp claim is not set and the token validation check is
skipped, a warning is shown.

New: After a successful signature validation a success message is shown.

Changed: The InvalidAlgorithm error message now shows the selected
algorithm and the algorithm specified in the JWT header.

New: If no signature validation is performed, the return code is now
2.

All tests have been updated accordingly.

Summary

Preflight checklist

  • Code formatted with rustfmt
  • Relevant tests added
  • Any new documentation added (I don't think this requires additional documentation).

@mike-engel
Copy link
Owner

Thanks for making this @codedust! I like all of the changes other than the new exit status. I have a feeling this may break more workflows than it seems, and I wonder if a new --validate flag should be added, which would then return an exit status of 2 like you have. What do you think?

@codedust
Copy link
Contributor Author

This is intended behavior. To prevent users from accidentally skipping signature validation, signature validation should be the default . An exit status of 0 indicates that everything is fine. If token validation fails, the exit status is set to 1.

What changes with the merge request is, that if no secret is given via the --secret flag (and signature validation cannot be performed), the exit status is set to 2. If workflows break because the jwt signature cannot be verified, it's not a bug, it's a feature.

I would strongly argue against an exit status of 0 when no signature verification has been performed (since this suggests "everything is fine").

From my point of view, the following two options would also be fine:

  • a) introduce a new --skip-signature-validation flag that explicitly sets the exit status to 0 even if the signature could not be verified
  • b) return an exit status of 1 if the signature could not be verified because of a missing secret key (instead of 2)

@codedust
Copy link
Contributor Author

Rebased to main

@mike-engel
Copy link
Owner

@codedust the original intent of this tool wasn't only for verification—it was originally just a way to view the contents of a JWT. Verification was secondary. I'm unsure how many people use it just to view the contents of a token, and how many use that in an automated fashion, where a non-zero exit code would break it. I'll need to think about it some more, but I'm still not convinced the default should be changed. I'd rather add a verify sub command

@mike-engel
Copy link
Owner

@codedust thoughts on making a verify subcommand?

@codedust
Copy link
Contributor Author

I think, adding a verify command would be a good solution. This way, decode could still be used to simply show the contents of a jwt and verify could be used to enforce signature verification. I'll give it a try.

New: If no secret/publicKey is provided by the user, no signature check
is performed. Rather then silently skipping the signature check, this
commit introduces a warning in this case.

New: If the `exp` claim is not set and the token validation check is
skipped, a warning is shown.

New: After a successful signature validation a success message is shown.

Changed: The `InvalidAlgorithm` error message now shows the selected
algorithm and the algorithm specified in the JWT header.

New: If no signature validation is performed, the return code is now
`2`.

All tests have been updated accordingly.
This commit introduces a verify command that enforces signature
validation. The decode command now does not attempt to validate the
token signature and does not expect the parameters `--secret`,
`--ignore_exp` and `--alg` any more.

This commit also includes some additional code cleanups regarding exit
codes.
@codedust
Copy link
Contributor Author

codedust commented Oct 20, 2021

I've now added the verify subcommand and rebased to main.

Now, the decode subcommand does not verify any signatures any more and does not expect the --secret, ignore_expand--alg` parameters. If this breaking change is acceptable, we're fine.

If backwards compatibility is really important, we could also add the parameters to the decode subcommand again but ignore them in the implementation. However, that's not really great cli design.

An alternative would be to add signature verification to the decode subcommand again if the --secret parameter is given. We should think about when to return an exit code of 1 in this case, though.

What do you think?

@mike-engel
Copy link
Owner

An alternative would be to add signature verification to the decode subcommand again if the --secret parameter is given. We should think about when to return an exit code of 1 in this case, though.

I kind of like this approach, since it shortcuts the occasional need to do both verification and decoding. It will probably also be useful if this tool ever handles encrypted tokens.

It seems like the exit code should always be 0 unless there was an error decoding, though

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

Successfully merging this pull request may close these issues.

2 participants