Offline Rekor bundle generation and verification #247
Conversation
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
|
This is ready for an initial review, but it needs some more testing before merge (I haven't actually confirmed offline bundles work yet, only that the changes don't break online verification). My plan was to implement #194 in this PR as well, to complete the whole picture (and round out testing). But I can do that separately and use a bundle generated by |
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
I may not necessarily totally us cosign's scheme... I'm not really a fan of the duplicated fields as I mentioned in #194 (comment) But I hear the burden of implementing something twice. On the other hand, I think keeping it absolutely minimal to just the Rekor entry stored offline would allow building up to the "Sigstore bundle" (which is WIP design sigstore/cosign#2204 away from the sorta-faulty cosign bundle) |
The returned value here is not base64-encoded. Signed-off-by: William Woodruff <[email protected]>
Makes sense. Yeah, this PR is limited to only the "Rekor" bundle, not the bigger thing that I understand (maybe incorrectly?) to be unstable/soft-deprecated on |
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
…ation Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
| spec = entry_body["spec"] | ||
| expected_sig, expected_cert, expected_hash = ( | ||
| spec["signature"]["content"], | ||
| load_pem_x509_certificate( |
There was a problem hiding this comment.
This field can contain either a certificate, a certificate chain, or a public key, so we'll need to handle this. Not sure if there's other parts of the code that make this assumption also.
There was a problem hiding this comment.
I believe this is the only place we make that assumption, since the only other places we touch hashedrekord objects directly are during log entry submission and querying.
In this case, it's okay for us to fail if we don't see a PEM-encoded certificate, right? This code only runs (currently) in the context of verification against a PEM-encoded certificate, so anything else would imply that the user has given us the wrong Rekor entry.
There was a problem hiding this comment.
(More generally, I think this demonstrates that I probably need to invert how I'm matching against the offline Rekor entry -- instead of unpacking the body and making sure it matches the cert/sig/etc, I should pack the latter into a hashedrekord payload and make sure it's strictly equal to the claimed entry.)
There was a problem hiding this comment.
So there's no support for verification by signing key rather than cert? (If not, feature request!)
Assuming only verification by cert is currently supported, I do think we should handle a certificate chain too. I think it'd be fine to take only the first element in the chain and discard the remaining.
There was a problem hiding this comment.
Correct! I'll file an issue for that.
Co-authored-by: Hayden B <[email protected]> Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
Signed-off-by: William Woodruff <[email protected]>
|
I think this is good to go, at least on my side. cc @di: what are your thoughts on merging this as-is, vs. waiting for the "sigstore bundle" formal to become stable? I think we could go either way, although we'll likely end up replacing/deprecating these CLI flags once the sigstore bundle is available. As such, it might make sense to avoid the deprecation period and just wait a bit here. |
There was a problem hiding this comment.
The rekor specific bits fly over my head a bit but this looks ok to me.
- I think
verify()needs a refactor though: it's now very long and quite complicated -- that could happen in another issue though - your comment WRT inverting entry matching seems reasonable: that would likely go some way towards fixing the previous complaint?
actually: inverting the entry matching maybe isn't a good idea if the rekor entry could contain full certificate chains but we are ok with just the single certificate in our verfication input ? |
Yes, absolutely agreed. I have that earmarked in #250. |
|
Not sure why DCO is stalled here... |
Signed-off-by: William Woodruff <[email protected]>
|
Needs re-app, since I added some warnings to emphasize that the Rekor bundle isn't intended to be a long-term format. |
WIP.Closes #52.
Closes #194.