Add tlog_mirror crate with wire format and default ticket authenticator#236
Conversation
|
/bonk review |
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Did a careful review against the c2sp.org/tlog-mirror spec and ran the pre-push checks. Wire-format math, ticket scheme, and test coverage are solid. A few things to address before this lands:
Must-fix
1. Stale TicketSealer references break cargo doc (error.rs, wire.rs)
Three intra-doc links to crate::TicketSealer survive from an earlier AEAD-based design — the type is now TicketMacer. Confirmed locally:
$ RUSTDOCFLAGS=\"-D rustdoc::broken_intra_doc_links\" cargo doc -p tlog_mirror --no-deps
error: unresolved link to \`crate::TicketSealer\`
--> crates/tlog_mirror/src/wire.rs:64:26
error: unresolved link to \`crate::TicketSealer\`
--> crates/tlog_mirror/src/error.rs:62:41
Locations:
crates/tlog_mirror/src/wire.rs:64— ``TicketSealer``crates/tlog_mirror/src/error.rs:62— `Errors returned by `TicketSealer`.`
While you're in error.rs, the TicketError variant docs are also AEAD-flavored leftovers that no longer match the implementation:
- Line 65: "sealed ticket too short: ... 12-byte nonce" — there is no nonce; the threshold is
TAG_LEN = 16. - Line 70: "AEAD authentication failed: either the ciphertext or the nonce has been tampered with" — no AEAD, no nonce, no ciphertext.
Suggest rewording to refer to TicketMacer, TAG_LEN, and HMAC tag verification.
2. cargo package -p tlog_mirror fails — missing README.md
crates/tlog_mirror/Cargo.toml:3 hard-codes readme = \"README.md\", but no README.md exists in the crate directory. Sibling crates (tlog_witness, tlog_cosignature, static_ct_api) use readme.workspace = true, which resolves to the workspace root README during packaging.
$ cargo package -p tlog_mirror --no-verify --allow-dirty --list
error: readme \`README.md\` does not appear to exist (relative to \`.../crates/tlog_mirror\`).
This will also block any future cargo publish. Easiest fix: change to readme.workspace = true, matching siblings.
3. Integer overflow in package_ranges iterator (wire.rs:185)
let next_boundary = (start / PACKAGE_ALIGNMENT + 1) * PACKAGE_ALIGNMENT;This overflows u64 whenever start / 256 == u64::MAX / 256, i.e. roughly start >= 0xFFFF_FFFF_FFFF_FF00. In debug builds this panics with attempt to multiply with overflow; in release it wraps to 0, which makes end = next_boundary.min(upload_end) = 0, and since self.next = end = 0, the iterator silently restarts and yields wrong ranges.
Reachable from network input because AddEntriesRequestHeader::read_from accepts any upload_end up to u64::MAX. No real log will ever hit 2^64 entries, but a malicious or buggy peer absolutely can put any 8-byte value on the wire.
Suggested fix — saturate at upload_end:
let next_boundary = (start / PACKAGE_ALIGNMENT)
.checked_add(1)
.and_then(|q| q.checked_mul(PACKAGE_ALIGNMENT))
.unwrap_or(self.upload_end);
let end = next_boundary.min(self.upload_end);Add a regression test, e.g. package_ranges(u64::MAX - 1, u64::MAX) should yield (u64::MAX - 1, u64::MAX) and then terminate.
Should-fix
4. PR description claims publish = false but the manifest doesn't have it
The PR description says the crate is publish = false while the spec stabilizes, but crates/tlog_mirror/Cargo.toml has only [package.metadata.release] release = false — that's a cargo-release automation marker, not a cargo publish gate. (static_ct_api, which is explicitly published per AGENTS.md, has the same metadata.release block.) Either add publish = false under [package] or amend the PR description so it doesn't promise a guard that isn't there. Worth being explicit since the AGENTS.md "crates.io" boundary is an approval gate.
Nits / discussion
5. parse_decimal_u64 is stricter than the spec text
The spec says "in decimal" without explicitly forbidding leading zeros / + / whitespace. The current parser rejects all of those, which is fine for serializing — we only want one canonical form — but slightly aggressive when parsing a body produced by another mirror implementation. The inline comment already justifies this ("we want a single canonical encoding for each integer"), so I'm fine leaving it as-is, but worth flagging in case there's appetite for a Postel split (lenient parser, strict serializer).
6. Test gap (low priority)
EntryPackage::read_from converts num_entries: u64 to usize and only fails on 32-bit targets. Not realistically reachable from a tlog mirror, but a #[cfg(target_pointer_width = \"32\")] test is essentially free if you want to lock the behavior in.
Pre-push checks I ran locally on the head commit:
|
Review submitted. Summary of what I posted on PR #236: Must-fix (3):
Should-fix (1): PR description says Discussion/nits (2): strict decimal parser is more strict than spec text (currently inline-justified; flagged for awareness); 32-bit target test gap (low priority). Locally clippy/test/fmt all pass; only |
lukevalenta
force-pushed
the
lvalenta/tlog-mirror-wire-format
branch
from
e68442a to
789acbb
Compare
|
/bonk review again |
|
@lukevalenta Bonk workflow was cancelled. View workflow run · To retry, trigger Bonk again. |
There was a problem hiding this comment.
Re-reviewed against the previous review's findings and against the spec. All three must-fix items and the should-fix item are addressed; pre-push checks (including the two that previously failed) all pass locally on the head commit.
Previous review items — verified fixed
1. Stale TicketSealer references / AEAD verbiage ✅
wire.rs:64now points tocrate::TicketMacer.error.rs:62rewords to[\TicketMacer`](crate::TicketMacer)`.error.rs:65–75rewords the variants to refer toTAG_LEN(16 bytes) and HMAC tag verification — no more "12-byte nonce" / "AEAD authentication failed".RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc -p tlog_mirror --no-depsnow passes.
The only remaining mention of "AEAD" in the crate is in ticket.rs:34, in the doc paragraph that explicitly invites operators to roll their own AEAD-based scheme — that's intentional, not a leftover.
2. Missing README breaks cargo package ✅
crates/tlog_mirror/Cargo.toml:10now usesreadme.workspace = true, matching siblings.cargo package -p tlog_mirror --no-verify --allow-dirty --listsucceeds and includesREADME.md(resolved from the workspace root).
3. Integer overflow in package_ranges ✅
wire.rs:189–192now useschecked_add(1).and_then(|q| q.checked_mul(PACKAGE_ALIGNMENT)).unwrap_or(self.upload_end), with an inline comment explaining the saturation.- New regression test
package_ranges_no_overflow_at_u64_max(wire.rs:540–553) covers bothu64::MAX - 1, u64::MAXandedge, u64::MAX(whereedge = u64::MAX - (u64::MAX % 256)). Both yield a single correct range and terminate. - Total tests: 35 (up from 34).
4. publish = false not in manifest ✅
crates/tlog_mirror/Cargo.toml:5now setspublish = falsewith a comment explaining it'll be lifted once the spec stabilizes.[package.metadata.release] release = falseis also retained for cargo-release.
Pre-push checks (head commit 789acbb)
cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic ✅
cargo test ✅ (full workspace green; tlog_mirror 35/35)
cargo fmt --all --check ✅
RUSTDOCFLAGS="-D rustdoc::broken_intra_doc_links" cargo doc -p tlog_mirror --no-deps ✅
cargo package -p tlog_mirror --no-verify --allow-dirty --list ✅
(cargo machete not installed in this sandbox; CI runs it.)
Tiny nit (non-blocking)
The PR body still says "34 unit tests covering …". With the overflow regression test added, it's 35. Worth a one-character tweak before merge if you're amending the description; otherwise no action needed.
LGTM — approving.
|
Re-review submitted as APPROVED on PR #236. Summary of what I posted: All four previously-flagged items are fixed on head commit
All five pre-push checks (clippy, test, fmt, doc, package) pass locally; tests went from 34 to 35. Only nit: PR description still says "34 unit tests"; it's 35 now. Non-blocking. Approved. |
lukevalenta
requested review from
a team,
bwesterb,
cjpatton,
lbaquerofierro,
mendess and
rozbb
as code owners
lukevalenta
force-pushed
the
lvalenta/tlog-mirror-wire-format
branch
from
789acbb to
d2978b2
Compare
lukevalenta
force-pushed
the
lvalenta/tlog-mirror-wire-format
branch
2 times, most recently
from
499b2e0 to
21b0ffa
Compare
Implements the wire-format pieces of c2sp.org/tlog-mirror: - `AddEntriesRequestHeader`: read/write framing for the add-entries request body header (u16 log_origin, u64 upload_start/end, u16 ticket). - `EntryPackage`: read/write framing for a single entry package (u16-prefixed entries + u8 num_hashes <= 63 + 32-byte hashes). - `package_ranges`: deterministic iterator over the spec's 256-aligned `[start, end)` intervals derived from upload_start and upload_end. Callers iterate this rather than recomputing the rounding math. - `MirrorInfo`: read/write framing for the `text/x.tlog.mirror-info` 409 Conflict response body (3 newline-terminated lines: tree size, next entry, base64-encoded ticket). - `TicketSealer`: default opaque-ticket authenticator using deterministic AES-256-GCM-SIV over the payload, with the log origin as associated data. Encryption enforces the spec's ticket opacity so clients cannot depend on the internal format; confidentiality is not required because pending checkpoints are public. The mirror's `add-checkpoint` endpoint reuses `tlog_witness::add_checkpoint` per spec, so this crate does not duplicate it.
lukevalenta
force-pushed
the
lvalenta/tlog-mirror-wire-format
branch
from
21b0ffa to
3c7e8b9
Compare
|
/bonk review |
|
APIError: Invalid Anthropic API Key |
2 similar comments
|
APIError: Invalid Anthropic API Key |
|
APIError: Invalid Anthropic API Key |
|
@lukevalenta Bonk workflow failed. Check the logs for details. View workflow run · To retry, trigger Bonk again. |
|
/bonk review |
|
LGTM – Approved All previously identified issues have been addressed:
All pre‑push checks (clippy, test, fmt, doc, package) pass locally. No remaining blockers. 🚀 |
| /// # Errors | ||
| /// [`ParseError::Io`] on short reads, or [`ParseError::TooManyHashes`] | ||
| /// if `num_hashes` exceeds 63. | ||
| pub fn read_from<R: Read>(mut reader: R, num_entries: u64) -> Result<Self, ParseError> { |
There was a problem hiding this comment.
The specification limits each entry package to at most 256 entries, but read_from accepts any u64 and passes it to Vec::with_capacity. An incorrect count could cause a very large allocation or panic. Could we reject counts above PACKAGE_ALIGNMENT before allocating? It may also be worth rejecting packages with more than 256 entries in write_to, so the type cannot serialize an invalid package
2 participants
NOTE(lvalenta): I used Claude to help generate this PR, but have reviewed and approve the changes.
Introduces the
tlog_mirrorcrate, implementing the wire-format pieces of c2sp.org/tlog-mirror.