Use note-style formatting for SubtreeSignature - #281
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #281 +/- ##
==========================================
+ Coverage 82.57% 87.32% +4.75%
==========================================
Files 5 10 +5
Lines 241 994 +753
==========================================
+ Hits 199 868 +669
- Misses 30 125 +95
+ Partials 12 1 -11 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| func SubtreeTimestamp(s []byte) (time.Time, error) { | ||
| sigLine, ok := strings.CutPrefix(string(s), "— ") | ||
| if !ok { | ||
| return time.UnixMilli(0), errMalformedSig |
There was a problem hiding this comment.
nit, should this be time.UnixMilli(0).UTC()?
There was a problem hiding this comment.
I've dinged this func entirely, with the other changes the timestamp will always be zero here anyway.
However, I regret having time.Time involved here because of exactly this abiguity, so I've deprecated CoSigV1Timestamp (I could actually just flat out remove it - we're still at v0) and replaced with CosignatureTimestamp which returns the underlying uint64 instead, much cleaner.
| type SubtreeSigner interface { | ||
| note.Signer | ||
| // SignSubtree returns a note-style signature line over the subtree described by the provided arguments. | ||
| SignSubtree(timestamp uint64, logOrigin string, start, end uint64, root []byte) ([]byte, error) |
There was a problem hiding this comment.
In a different PR, should we remove "timestamp" from the argument list? Is there a spec allowed use case at the moment for signing subtrees with a non-0 timestamp?
There was a problem hiding this comment.
I've rolled it in here, may as well tidy that up while I'm here!
| } | ||
| sigRaw = sigRaw[keyHashSize:] | ||
| t := binary.BigEndian.Uint64(sigRaw[:timestampSize]) | ||
| // Timestamp must be zero if start > 0. |
There was a problem hiding this comment.
From https://c2sp.org/tlog-witness@main#sign-subtree:
If the cosignature format supports timestamps, the timestamp MUST be zero.
So maybe this should always be 0? I can see an argument for keeping the current behaviour to make it possible to call VerifySubtree on full tree signatures. But in practice, will VerifySubtree be ever called on a checkpoint signature?
There was a problem hiding this comment.
I've left it as is for now, mainly because:
Semantically, a v1 subtree cosignature is a statement that the subtree with the specified root hash is consistent with all other historical views observed by the cosigner of the log identified by the origin line. If the timestamp is not zero, it is also a statement that, as of the specified time, this is the largest consistent tree the cosigner has observed for the log.
Practically, you're probably right that nobody would ever use this path to verify a checkpoint sig, but technically we'd be refusing to validate a signature which was actually valid, which feels wrong.
There was a problem hiding this comment.
Makes sense! Maybe leave a comment then to explain why then?
aaf4fd6 to
1f26c2c
Compare
| if timestamp > math.MaxInt64 { | ||
| return nil, errInvalidTimestamp | ||
| } | ||
| if start > end { |
There was a problem hiding this comment.
This is good, but still leaves the door open for invalid subtrees.
Specs say The half-open interval [start, end) MUST be a valid subtree per [draft-ietf-plants-merkle-tree-certs-03](https://datatracker.ietf.org/doc/html/draft-ietf-plants-merkle-tree-certs-03), Section 4.1, and end MUST be less than or equal to the checkpoint size.
So in the interest of clarity, I'd say that either we should remove this check, or we should do a full check with something along the lines of https://github.com/transparency-dev/merkle/blob/a490ef305a5bc3e556495fd824681d090d832d27/proof/proof.go#L311.
There was a problem hiding this comment.
Yeah, fair; I've added the full check.
| } | ||
| sigRaw = sigRaw[keyHashSize:] | ||
| t := binary.BigEndian.Uint64(sigRaw[:timestampSize]) | ||
| // Timestamp must be zero if start > 0. |
There was a problem hiding this comment.
Makes sense! Maybe leave a comment then to explain why then?
1f26c2c to
41ac158
Compare
3 participants
This PR updates the support for subtree signatures to return/consume note-style signature lines.
Adds a small inter-op test with a subtree signature created by
torchwoodto check compatibility.