trees/checkpoint: Add checkpoint.Checkpoint#8830
Conversation
634f52d to
c31714e
Compare
c31714e to
e4bc604
Compare
jsha
left a comment
There was a problem hiding this comment.
Generally looks good, thanks for working on this. Sharing something that confused me, in hopes it helps other reviewers:
The signed note format defines different signature types:
- 0x01 — Ed25519 signatures as specified by this document.
- 0x04 — Timestamped Ed25519 checkpoint cosignatures, as specified by c2sp.org/tlog-cosignature.
- 0x06 — Timestamped ML-DSA-44 (sub)tree cosignatures, as specified by c2sp.org/tlog-cosignature.
We want 0x04, 0x06, or both - but probably just 0x06. Importantly, 0x06 specifies a transformation to cosigned_message before signing / verifying.
We can implement our own note.Verifier that does the transfomation at verification time, and I'm assuming that's the intended followup from this PR.
Right now the tests use note.NewVerifier, which creates a key of type 0x01. Once we implement a verifier for 0x06, we'll probably want to update the tests for verisimilitude.
jsha
left a comment
There was a problem hiding this comment.
Oh, one other nit: This PR, in the description and comments, refers to the "note body", but https://github.com/C2SP/C2SP/blob/main/signed-note.md calls the same concept the "note text". We should use "note text" or "text" here too, with "note" or "signed note" for the overall object.
48ef23d to
23bf0f0
Compare
aarongable
left a comment
There was a problem hiding this comment.
LGTM with one comment. The new clarity on when to use Unmarshal vs when to use Open is really great!
| case strings.ContainsRune(line, '\n'): | ||
| return errors.New("contains a newline") |
There was a problem hiding this comment.
Some confusion here. The function is named checkNoteLine, so one expects it to be called on a single line of text. Then the doc comment says that newline is actually acceptable per C2SP. Then the function rejects newline characters.
If the point is to allow newline characters, this check shouldn't be here. If the goal is to forbid newline characters, then it should be caught by the check on line 41. If the goal is to forbid newline characters not because C2SP says so, but because we only want to be processing a single (pre-trimmed) line, then the documentation should clarify that.
There was a problem hiding this comment.
Perhaps this would a good way to resolve it:
- validNote() checks "not valid UTF-8, no control characters except newline", and is used on the final bytes of the note on both marshal and unmarshal.
- validNoteField() checks "no newline", and is checked on each field during unmarshal. This ensures that fields don't contain their delimiter character.
jsha
left a comment
There was a problem hiding this comment.
Basically ready to go; a few small comments. Thanks for working on this!
| // emptyTreeHash is MTH({}), the RFC 6962 section 2.1 hash of the empty tree: | ||
| // SHA-256 of the empty string. | ||
| var emptyTreeHash = tlog.Hash(sha256.Sum256(nil)) |
There was a problem hiding this comment.
Why add validation specifically for the hash of the empty tree? In general we expect callers of this code to be validating the root hash. It's odd for the parser to validate in only in the specific case that the tree is empty, particularly since we will almost never see empty trees in this code (MTC specifies a null_entry for the 0th spot).
The code is harmless but I think also creates some confusion about the role of Unmarshal - whether it's doing syntactic validation, semantic validation, or both. IMO let's skip it.
There was a problem hiding this comment.
Fair, and I think having the contract be inconsistent here (e.g. audit the one kind of RFC 9162 Hash we know the value of) is the wrong choice. Good call.
|
|
||
| // Open opens a signed checkpoint note and parses its text. An error is returned | ||
| // if the note is not valid or the signature is not verified by one of the | ||
| // verifiers. |
There was a problem hiding this comment.
| // verifiers. | |
| // if the note is not valid or none of the signatures validate. |
"not verified by one of the verifiers" was ambiguous:
- any verifier rejected the signature?
- all verifiers rejected the signature?
| case strings.ContainsRune(line, '\n'): | ||
| return errors.New("contains a newline") |
There was a problem hiding this comment.
Perhaps this would a good way to resolve it:
- validNote() checks "not valid UTF-8, no control characters except newline", and is used on the final bytes of the note on both marshal and unmarshal.
- validNoteField() checks "no newline", and is checked on each field during unmarshal. This ensures that fields don't contain their delimiter character.
Add
Checkpointto represent a tlog-checkpoint note body, with.String()to serialize it and.Marshal()to serialize with field validation. AddParse()to parse an unsigned body andOpen()to verify a signed note and parse its body into aCheckpoint.A note to reviewers: this package and its tests were written with assistance from Claude Opus 4.8. That being said, it has been reviewed and extensively revised by myself before submission.