verify: Add method that accepts unhashed message as arg#144
verify: Add method that accepts unhashed message as arg#144DarkaMaul merged 5 commits intotrailofbits:mainfrom
Conversation
This way handling timestamp_response.tst_info.message_imprint.hash_algorithm is not the callers problem. The implementation could have been squeezed into verify() but then the method signature would be more complex: I think separate methods may be cleaner. Support the six last algorithms in https://www.iana.org/assignments/hash-function-text-names/hash-function-text-names.xhtml
|
|
ah, well I guess I'll start the process on that then :( |
|
I guess it's worth pointing out that the request code only supports SHA256 and SHA512, and I think that's fine. I can use only those two here if that's preferred (I just think verify can support more algorithms for increased compatibility with timestamps coming from other implementations). The main issue with supporting more algorithms in verify is that creating test data is a bit more work. EDIT: and after looking into it, I think the testing aspect is annoying enough that I'll stick to SHA256 and SHA512 and maybe SHA384 that I can easily generate test data for |
* Add tests and assets for sha{256,384,512}
* Remove support for SHAKE and SHA224 as it turned out finding a
TSA that supports those was difficult
The test implementation is not ideal as the new assets use a different
TSA (timestamp.sigstage.dev supports SHA-384) so we need more
certificates etc... On the other hand testing with multiple timestamp
sources seems also good.
|
* Remove unneeded mention of checking algorithm * Point to correct cert chain path in test/fixtures/sigstage/README.md
jku
force-pushed
the
handle-hashing-in-verify
branch
from
e5b1f5f to
76d01e3
Compare
| verifier = VerifierBuilder().add_root_certificate(certificate).build() | ||
| try: | ||
| verifier.verify(timestamp_response, message_hash) | ||
| verifier.verify_message(timestamp_response, message) |
There was a problem hiding this comment.
Keep the original example that accepts a pre-hashed message, in case folks are using other hash algorithms that this library doesn't directly support.
There was a problem hiding this comment.
I feel like an example in the README should just show the intended golden path (which I think the new method is)... Of course it would be nice to have API docs so the options were visible there.
|
✔️ CLA sorted |
|
Hi - thanks a lot for the PR here. Just a few comments before I review this:
|
* add comment on OIDs * more accurate certificate handling in tests * refactor the the response loading fixture in tests
jku
force-pushed
the
handle-hashing-in-verify
branch
from
4931eb5 to
265b807
Compare
|
Fixed a lint issue in last commit |
This feels like a good idea.
I would suggest this refactor is not part of this PR but I can add it here if you'd like. |
Let's open another PR for this, it doesn't really belong here. I added a CHANGELOG entry and will merge this once the CI has finished. Thanks again for the work here! |
4 participants
Fixes #130, provides a new verification method with (unhashed) message as argument.
This way handling timestamp_response.tst_info.message_imprint.hash_algorithm is not the callers problem anymore.
The implementation could have been squeezed into verify() but then the method signature would be more complex: I think separate methods may be cleaner.
I orignally planned to add the modern ones from
https://www.iana.org/assignments/hash-function-text-names/hash-function-text-names.xhtml but testing difficulties lead me to only handling SHA-{256,384,512}: adding more is easy as long as we have a TSA to create the test material with.
CC @ramonpetgrave64 : I think this will work for you in sigstore-python?