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

Perform key sanity check when verifying signatures #46

Closed
frasertweedale opened this issue May 25, 2017 · 7 comments
Closed

Perform key sanity check when verifying signatures #46

frasertweedale opened this issue May 25, 2017 · 7 comments

Comments

@frasertweedale
Copy link
Owner

Currently if a key is too small, the verification fails with InvalidSignature instead
of KeySizeTooSmall. Add a key sanity check step before attempting to validate
the signature so that the appropriate error gets returned to the caller.

Thanks to @begriffs for reporting.

frasertweedale added a commit that referenced this issue Jun 11, 2017
Add the 'checkJWK' function for sanity checking a JWK to make sure
it is not totally unusable.  This work is motivated by the problem
where loading a small key means that signature verification fails
with 'InvalidSignature', obscuring the fact that the verification
key is too small to be used.

The fact that the validation function may attempt to validate a
single signature against several keys makes it difficult to convey
the error for a single signature to the caller.  Instead, library
users are encouraged to explicitly apply checkJWK to all JWKs that
may be used for verification, and fail, warn, or omit offending
keys.

Fixes: #46
@frasertweedale
Copy link
Owner Author

@begriffs have a look at: 59ca5e6

Note that this step is not automatically performed during verification for a couple of reasons:

  1. An offending JWK in a JWK store might not actually be used for a particular verification; checking all JWKs in a JWKStore is too aggressive and may result in spurious verification failures.

  2. Passing the per-signature verification failures up to the caller is... quite awkward, to say the least. A single signature may be checked against multiple keys. If the all fail, we'd need to pass all the errors up, presumably with a mapping of each error the JWK that caused it, then let the user sort it out.

Instead of presenting the above problems to library users, I've decided to just implement the checkJWK sanity check function. Library users can call it themselves for each JWK that intend to use for verification, and can fail / report / omit offending keys according to their needs. It is an optional extra step. it can also aid debugging (e.g. could be used for post facto diagnostics).

Let me know what you think of the approach and its usability.

Cheers!

@begriffs
Copy link

begriffs commented Jul 2, 2017

Thanks for the helper function. I will try it out and see how it goes!

@frasertweedale
Copy link
Owner Author

@begriffs did you have any feedback re checkJWK ?

@begriffs
Copy link

Thanks, I haven't gotten around to using this function, but I filed an issue in my project so that I or someone else will use it to improve error messages for users.

@frasertweedale
Copy link
Owner Author

@begriffs righto, I'll merge it soon.

@ecthiender
Copy link

ecthiender commented Jul 30, 2018

Why can't verifyClaims function verify the size of the key and fail with KeySizeTooSmall ?

Much better UX for library users IMHO.

@frasertweedale
Copy link
Owner Author

@ecthiender it's because there can be multiple keys that could be used to verify some signature, and there is not yet a way to propagate complete information about the verification results for particular keys.

I'll think about how the API could be enhanced to provide that. If you have ideas, please share.

frasertweedale added a commit that referenced this issue Apr 13, 2022
Add the 'checkJWK' function for sanity checking a JWK to make sure
it is not totally unusable.  This work is motivated by the problem
where loading a small key means that signature verification fails
with 'InvalidSignature', obscuring the fact that the verification
key is too small to be used.

The fact that the validation function may attempt to validate a
single signature against several keys makes it difficult to convey
the error for a single signature to the caller.  Instead, library
users are encouraged to explicitly apply checkJWK to all JWKs that
may be used for verification, and fail, warn, or omit offending
keys.

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

No branches or pull requests

3 participants