-
Notifications
You must be signed in to change notification settings - Fork 63
verifier: Fix TSA cert chain construction #1482
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
Conversation
Using add_root_certificate() multiple times looks like a bug: it usually
works but in the case of timestamps with no embedded certs verification
seems to fails with
Certificates neither found in the answer or in the Verification Options
Signed-off-by: Jussi Kukkonen <[email protected]>
|
This seems correct to me and fixes the conformance issue in sigstore/sigstore-conformance#230 |
We don't need to modify the list so let's avoid it to make it easier to review. Signed-off-by: Jussi Kukkonen <[email protected]>
ba05137 to
211f9ef
Compare
|
Added a regression test. Current main fails the test with
|
Signed-off-by: Jussi Kukkonen <[email protected]>
52a03c3 to
d93b031
Compare
| for certificate in certificates[1:-1]: | ||
| builder = builder.add_intermediate_certificate(certificate) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Flagging, nonblocking: we could add these with add_root_certificate instead, since they're semantically root certs ("root" meaning "in the root program," not "is a self-signed certificate"). That would be slightly faster, since our verification would then be one-hop (TSR -> TSA -> TSA-CA) rather than having to chain TSA-CA up to its "root."
In practice that's probably a marginal performance benefit, however.
woodruffw
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jku!
Using add_root_certificate() multiple times looks like a bug: it usually ends up working because
However, in the case of timestamps with no embedded certs verification fails with
Certificates neither found in the answer or in the Verification OptionsThis is understandable since the rfc3161-client Verifier thinks it does not have the actual signing certificate (as it's actually added as root certificate). This case should succeed since our certificate chain contains the signing certificate -- so it is not required in the timestamp itself.
FYI @DarkaMaul