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

Create NoteVerifier, verification library functions (v2) #119

Merged
merged 10 commits into from
Mar 25, 2025

Conversation

jku
Copy link
Member

@jku jku commented Mar 21, 2025

This replaces #114:

  • create NoteVerifier
  • Add verify package to contain log entry verification methods
  • verification methods now take a (sumdb) note.Verifier as argument on colleens suggestion
  • Add basic tests for verify

haydentherapper and others added 3 commits March 21, 2025 09:27
Signed-off-by: Hayden B <8418760+haydentherapper@users.noreply.github.com>
Refactor keyid calculation out of both constructors

Also remove the unused context argument

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Use SafeInt64
* fix VerifyLogEntry error return value

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Copy link

codecov bot commented Mar 21, 2025

Codecov Report

Attention: Patch coverage is 35.82090% with 43 lines in your changes missing coverage. Please review.

Project coverage is 26.84%. Comparing base (82230be) to head (c619068).
Report is 17 commits behind head on main.

Files with missing lines Patch % Lines
pkg/note/note.go 17.77% 34 Missing and 3 partials ⚠️
pkg/verify/verify.go 72.72% 4 Missing and 2 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #119      +/-   ##
==========================================
+ Coverage   21.61%   26.84%   +5.23%     
==========================================
  Files          27       29       +2     
  Lines        1712     1892     +180     
==========================================
+ Hits          370      508     +138     
- Misses       1308     1346      +38     
- Partials       34       38       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

tessera functions accept the sumdb Verifier so this likely makes client
code more consistent: it does mean caller needs to handle
    v, e := note.NewNoteVerifier(origin, verifier)
if they have a signature.Verifier.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
switch pk := pubKey.(type) {
var err error

switch pk := key.(type) {
case *ecdsa.PublicKey:
keyID, err = ecdsaKeyHash(pk)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unlike other keytypes, ecdsaKeyHash does not hash "origin". I don't have the context to understand if this is an issue or no

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jku jku mentioned this pull request Mar 21, 2025
6 tasks
@jku
Copy link
Member Author

jku commented Mar 21, 2025

I've had a first look at the tests and I think I will need a bit of help there, I'm not really sure how to generate the test data. I will have another go on monday but feel free to leave ideas here if you have some

Copy link
Member

@loosebazooka loosebazooka left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I looked into testing here, we kind of have 2 options:

  1. we assume the underlying note library is reliable and just ensure we are calling it correctly by calling it directly in a test and comparing the result.
  2. manually generate a note and sign it and then compare the result from an out of band signing.

@cmurphy
Copy link
Contributor

cmurphy commented Mar 21, 2025

@jku here's a starting point for the tests: https://gist.github.com/cmurphy/30e1beb758fe952959a27320c0a8d141

For TestVerifyInclusionProof, I pulled the values from the original verify_test.go and reformatted them into the TLE and Checkpoint structure. Some things needed to be hex decoded or base64 decoded.

For TestVerifyCheckpoint I started from the test in verify_test.go but then used https://github.com/transparency-dev/trillian-tessera/blob/ae724376e1ace4046767511c72c6006bde3ec87e/append_lifecycle.go#L298-L316 to generate a signed checkpoint envelope.

jku and others added 5 commits March 24, 2025 15:13
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
* Tests are roughly like TestInclusion TestCheckpoint in
  https://github.com/sigstore/rekor/blob/main/pkg/verify/verify_test.go
  (the entry content is from there as well)
* The signed checkpoint envelope creation in TestVerifyCheckpoint is from
  https://github.com/transparency-dev/trillian-tessera/blob/ae724376e1ace4046767511c72c6006bde3ec87e/append_lifecycle.go#L298-L316

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Also refactor a bit, to avoid unnecessary error handling

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku

This comment was marked as outdated.

Signed-off-by: Jussi Kukkonen <jkukkonen@google.com>
@jku jku marked this pull request as ready for review March 24, 2025 16:19
@jku jku requested review from a team as code owners March 24, 2025 16:19
Copy link
Contributor

@cmurphy cmurphy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm!

@jku jku merged commit 725537e into sigstore:main Mar 25, 2025
13 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants