-
Notifications
You must be signed in to change notification settings - Fork 21
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: add unit test for online tlog verification #296
base: main
Are you sure you want to change the base?
feat: add unit test for online tlog verification #296
Conversation
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
@cmurphy @haydentherapper I tried to add mock test by creating mock Rekor Entry client and adding inclusion proof to GenerateTlogEntry but I could not figure out how to:
It kept on failing while verifying rekorLogEntry. I don't know enough about rekor internals to create them properly. Meanwhile I have created a test that verifies one of the examples using sigstore public rekor. |
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
The test that uses public rekor is passing I have also added the unfinished test to the PR, What I don't understand is
|
The CT RFC has an explainer on merkle tree hash calculations: https://www.rfc-editor.org/rfc/rfc6962.html#section-2.1 The simplest merkle tree will contain only a single leaf, and the root hash will be the SHA-256 hash of the leaf prepended by a null character. The inclusion proof won't need to include additional hashes because there is only one hash needed to verify the leaf's inclusion in the log: https://gist.github.com/cmurphy/1b1203f4fec5b2363c30901d4684c2c4#file-treesize1-go I think simplifying If you did need to generate a more complex tree, the RFC explains how the root hash is calculated by concatenating the root hashes of its subtrees, and it would look something like this: https://gist.github.com/cmurphy/1b1203f4fec5b2363c30901d4684c2c4#file-treesize2-go
It would be better if the unit tests did not make real network calls to the public infrastructure. Unit tests should be self-contained and never rely on external services, because:
|
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
Thank you so much for the amazing explanation @cmurphy, I learned a lot about how tlogs actually work. I fixed inclusion and checkpoint but I still had this problem where SET verification were failing. I noticed that while verifying SETs in offline mode we are converting the log IDs to hex: Lines 267 to 272 in 5845298
But the rekor library does not do that https://github.com/sigstore/rekor/blob/8ca864f6b2bcece1b0b9b004938375bbe8fff03c/pkg/verify/verify.go#L190-L195 When I convert logID to hex in my mock log entry anon, SET verification using rekor library starts to pass, see: Is there a reason why we do that? I don't understand why I should convert it to hex in my mock to make it work |
It looks like rekor stores the LogID of the log entry as hex encoded: https://github.com/sigstore/rekor/blob/1e6728717d221a87792a8fc0c4cf588cc31dceb0/pkg/api/entries.go#L92 It doesn't need to re-encoded it during verification. But the mock should ensure the log entry is encoded the way the verifier expects it to be. |
okay, makes sense now. I have hex encoded the log entry and the PR is ready for review |
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.
Could you update the PR description to reflect the current purpose of this PR and add a reference to the issue it's addressing?
Co-authored-by: Colleen Murphy <cmurphy@users.noreply.github.com> Signed-off-by: Vishal Choudhary <vishal.chdhry.work@gmail.com>
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
pkg/verify/tlog_test.go
Outdated
IntegratedTime: swag.Int64(tlogEntry.IntegratedTime().Unix()), | ||
LogIndex: swag.Int64(tlogEntry.LogIndex()), | ||
LogID: swag.String(hex.EncodeToString([]byte(tlogEntry.LogKeyID()))), | ||
Verification: tlogEntry.Verification(), |
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.
Looking closer at the functional changes in entry.go that were needed to make this work, I'm seeing that this test is tautological - it's using the entity returned by the attestation as the response to the mock rekor client, and effectively comparing it to itself. I think this can be avoided, and the changes in entry.go can be discarded, if we create the mocked response from scratch with some added helpers to VirtualSigstore: https://gist.github.com/cmurphy/44a742f9f08e5cb56bc79dccfbd3b327
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.
@cmurphy that makes sense, thank you so much for helping with this!
Signed-off-by: Vishal Choudhary <vishal.choudhary@nirmata.com>
|
||
statement := []byte(`{"_type":"https://in-toto.io/Statement/v0.1","predicateType":"customFoo","subject":[{"name":"subject","digest":{"sha256":"deadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeefdeadbeef"}}],"predicate":{}}`) | ||
integratedTime := time.Now().Add(5 * time.Minute) | ||
entity, err := virtualSigstore.AttestAtTime("foo@example.com", "issuer", statement, integratedTime, true) |
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.
One last thing - this change adds one test, TestOnlineVerification
, but it's possible to have inclusion proofs without verifying them online and vice versa. Could there be an additional test that tests online verification without the inclusion proof, and maybe one that tests with the inclusion proof with offline verification?
Summary
Closes: #53
Adds test for online verification of artifact transparency log using mock rekor
Release Note
Documentation