mtpublisher: Replace the stub with a tlog-mirror client - #8973
mtpublisher: Replace the stub with a tlog-mirror client#8973beautifulentropy wants to merge 3 commits into
Conversation
|
@beautifulentropy, this PR appears to contain configuration and/or SQL schema changes. Please ensure that a corresponding deployment ticket has been filed with the new values. |
e55287c to
bffa98f
Compare
bffa98f to
bf00239
Compare
bf00239 to
4eb420e
Compare
4eb420e to
8ecc8ba
Compare
8ecc8ba to
6158fbc
Compare
| pubKey, err := loadMLDSAPublicKey(c.MTPublisher.Mirror.PublicKeyFile) | ||
| cmd.FailOnError(err, "Loading mirror public key") |
There was a problem hiding this comment.
nit: move this down to line 122. This pubkey isn't used until we construct the mirror, so all the MTCA-loading code shouldn't be between those two operations.
| if err != nil { | ||
| t.Fatalf("Marshal: %s", err) | ||
| } | ||
| _, err = verifier.FilterByVerify(text, line) |
There was a problem hiding this comment.
You're throwing away the signature returned by FilterByVerify, so why not just use verifier.Verify here?
| return fmt.Errorf("add-checkpoint at tree size %d got 409 with mirror tree size %d after retrying", tree.N, mirrorSize) | ||
| } | ||
| retried = true | ||
| oldSize = mirrorSize |
There was a problem hiding this comment.
Why save this to a local variable and then only write it to m.oldSize after the retry succeeds? The mirror has affirmatively told us that its current size is mirrorSize, I don't see any reason to trust our own understanding of the mirror's size (from a successful write) more than the mirror's own understanding of its current size (from the conflict response).
Is there an error case I'm misunderstanding?
| if err != nil { | ||
| return nil, fmt.Errorf("marshaling the checkpoint: %w", err) | ||
| } | ||
| timestampedMirrorCosignature, err := m.verifier.FilterByVerify(noteText, subtreeCosignatureLines) |
There was a problem hiding this comment.
Feels weird to call this a timestamped mirror cosignature, since it has to be the zero timestamp.
There was a problem hiding this comment.
FWIW, I think this naming makes sense because it's referring to the data format (timestamped_signature from https://github.com/C2SP/C2SP/blob/main/tlog-cosignature.md#format) and contrasts with the rawMirrorSignature below.
There was a problem hiding this comment.
Yep, I thought about that. The name makes sense, but IMO is surprising enough that it can cause a reader (as it caused me!) to stop and question what they think they know about subtree cosignatures. I think a name like zeroTimestampMirrorCosignature would be clearer, but we're very much into bikeshed territory. I'm happy with whatever path Samantha picks.
| return fmt.Errorf("assembling checkpoint %d signed note: %w", latest.ID, err) | ||
| } | ||
| timestampedMirrorCosig, err := cosignature.TimestampedSignature(text, []byte(cosigLine), p.verifier) | ||
| _, _, err = checkpoint.Open(signedNoteForMirror, p.caVerifier) |
There was a problem hiding this comment.
Here we're using Open just for its validation capabilities, because we're throwing away the results of parsing the note.
That means that we have four ways to validate a checkpoint:
- checkpoint.Open(signedNote, verifier)
- verifier.FilterByVerify(noteText, signatureLines)
- verifier.Verify(noteText, timestampedSignature)
- verifier.VerifyCheckpoint(origin, tree, timestampedSignature)
(I'm ignoring the top-level cosignature.SignatureLine, which also does verification, because it is removed in the next PR in this stack.)
I'm of the opinion that this is too much. I'm not exactly sure what the best solution is -- obviously each of these methods is convenient in certain places, and less convenient in others -- but it suggests to me that we don't have a clear sense of what a checkpoint is. This feeling seems corroborated to me by the fact that we don't even have a struct type representing a whole Checkpoint, including signatures: we only have a struct for the Checkpoint note text, and signatures are only ever attached to their note in the form of serialized text.
I think the underlying issue is that everything else is optimized to only handle true checkpoint objects when absolutely necessary: the MTCA doesn't produce checkpoints, it just produces raw signatures and tosses those in the database to be turned into checkpoints later; the tlog-mirror add-entries endpoint doesn't return a checkpoint when you're done uploading, it just returns signature lines and trusts you to construct the rest of the checkpoint object.
We could instead take the principled stand that we only ever pass around checkpoint objects. The MTCA's signing routine produces a signature, immediately bundles it into a Checkpoint object with signature line, and returns that. We use a new checkpoint.RawSignature() accessor method to extract the sig bytes and write them to the database, and we write checkpoint.Text() to S3. Similarly, treedb.LatestCheckpoint() would return a Checkpoint object so that the publisher doesn't have to do all this assembly work here, and mirror.Cosign would return a Checkpoint object that can have the signature extracted for the database just like the MTCA's.
Obviously this design sketch goes beyond what should be part of this stack of PRs. But maybe you have other ideas about how to prevent the proliferation of similar-but-not-identical verification functions, and maybe some of your ideas are better and less invasive than mine.
3 participants
Cleanup:
Closes #8944