Add support for verifying Sunlight checkpoints #99
Conversation
AlCutter
force-pushed
the
solar_signature
branch
4 times, most recently
from
ffad4c5 to
c99d6d7
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
- Coverage 82.57% 76.99% -5.58%
==========================================
Files 5 6 +1
Lines 241 313 +72
==========================================
+ Hits 199 241 +42
- Misses 30 42 +12
- Partials 12 30 +18 ☔ View full report in Codecov by Sentry. |
AlCutter
force-pushed
the
solar_signature
branch
from
6be7b6c to
4cc87c0
Compare
| } | ||
|
|
||
| // NewRFC6962Verifier creates a note verifier for Sunlight/RFC6962 checkpoint signatures. | ||
| func NewRFC6962Verifier(vkey string) (note.Verifier, error) { |
There was a problem hiding this comment.
I have one of these, named just the same, in an (for now) untagged version of the Sunlight module.
https://pkg.go.dev/filippo.io/sunlight@main#NewRFC6962Verifier
What do you think of it? Would it make sense to import it from here?
A key difference is that it allows extracting/validating the timestamp, which I think is critical for verification.
I didn't make a string encoding for it though. Should we specify those?
There was a problem hiding this comment.
Aha, I was trying to do this "clean room" from the spec :)
In general I really like the string vkey idea; it makes it very easy to poke through configs in existing code (e.g. the witness), so I'm all up for specifying for all the note-compatible verifiers we're cooking up!
On the extracting the timestamp, the way I was going to go was the same as I've done here for TimestampCoSigv1: https://github.com/transparency-dev/formats/blob/main/note/note_cosigv1.go#L116-L127
Passing the call-back in at verifier c'tor time means that in apps like the witness you can't really create verifiers out at "config parsing" time (with corresponding error handling there) and then pass them down into other funcs/modules without adding some other complexity around how you pass that callback in too. It also means that the verifier is no longer safe to use from multiple go-routines.
It "feels good" to have the timestamp extracted during verification available, but OTOH, since note.Note already has a mechanism to convey which sigs (if any) were verified, I figured it's probably ok to have the application logic which relies on those signatures then go and explicitly extract the "extra" info present in them. Any malicious code which could modify/insert/remove entries from the note.Sigs[] immediately after the note.Open() returns can also probably just nobble the signature verification/timestamp extraction itself, or the callback and its side effects, etc.
wdyt?
AlCutter
force-pushed
the
solar_signature
branch
2 times, most recently
from
4856457 to
043c733
Compare
AlCutter
force-pushed
the
solar_signature
branch
from
dbdce9e to
dc192e9
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #99 +/- ##
==========================================
- Coverage 82.57% 76.99% -5.58%
==========================================
Files 5 6 +1
Lines 241 313 +72
==========================================
+ Hits 199 241 +42
- Misses 30 42 +12
- Partials 12 30 +18 ☔ View full report in Codecov by Sentry. |
| @@ -3,6 +3,9 @@ module github.com/transparency-dev/formats | |||
| go 1.21 | |||
|
|
|||
| require ( | |||
| github.com/google/certificate-transparency-go v1.1.8 | |||
There was a problem hiding this comment.
As discussed, please open an issue about this dependency. Copying out the CT bits we need is reasonable as a solution. We worked very hard to keep the number of deps for this repo to a minimum. This now implicitly depends on trillian, which is a big dependency load that was a prime part of the motivation towards creating these smaller repos.
note/note_rfc6962.go
Outdated
| return v, nil | ||
| } | ||
|
|
||
| // SignedTreeHead represents the structure returned by the get-sth CT method |
There was a problem hiding this comment.
nit: signedTreeHead (lower case s).
Passim.
| // rfc6962LogName returns a sunlight checkpoint compatible log name from the | ||
| // passed in CT log root URL. | ||
| // | ||
| // "For example, a log with submission prefix https://rome.ct.example.com/2024h1/ will use rome.ct.example.com/2024h1 as the checkpoint origin line" |
There was a problem hiding this comment.
The quotes around this sentence have me asking a lot of questions. I suspect it's an accident. If not, it needs an attribution.
There was a problem hiding this comment.
Linked to Sunlight spec
This PR adds support for verifying sunlight style checkpoints.
It provides:
note.Verifierimplementation of the sunlight RFC69620x05signature schemeSigned Tree Headstructures directly to sunlight signed checkpoint format.TODO: figure out best way to avoid the dep on ct-go #105