Skip to content

Add subtree_v1 ML-DSA-44 cosignatures to tlog_cosignature#227

Merged
lukevalenta merged 1 commit intomainfrom
lvalenta/subtree-v1
May 1, 2026
Merged

Add subtree_v1 ML-DSA-44 cosignatures to tlog_cosignature#227
lukevalenta merged 1 commit intomainfrom
lvalenta/subtree-v1

Conversation

@lukevalenta
Copy link
Copy Markdown
Contributor

@lukevalenta lukevalenta commented Apr 30, 2026

Summary

Adds the subtree/v1 cosignature format from c2sp.org/tlog-cosignature: signed-note algorithm byte 0x06 paired with ML-DSA-44.

Public API in tlog_cosignature::subtree_v1

  • build_cosigned_message: algorithm-independent body builder for the cosigned_message TLS-presentation struct. Takes raw (start, end) rather than a typed &Subtree because consumers (notably future draft-ietf-plants-merkle-tree-certs CA cosigners) may have already validated their inputs upstream and don't want a redundant construction; callers MUST ensure [start, end) is a valid subtree.
  • timestamped_signature: BE u64 timestamp || raw signature blob used inside signed-note lines. Available for callers that produce signed-note-line output with a different signature algorithm.
  • SubtreeV1CheckpointSigner: ML-DSA-44 signer producing subtree/v1 cosignatures. Implements tlog_tiles::CheckpointSigner for the checkpoint case; sign_subtree covers arbitrary subtrees and takes &tlog_tiles::Subtree so subtree validity is enforced by the type system rather than at runtime. C2SP tlog-cosignature mandates ML-DSA-44 for subtree/v1 signed-note lines, so this type is spec-faithful rather than generic.
  • SubtreeV1NoteVerifier: matching ML-DSA-44 verifier. Symmetric shape: verify_subtree takes &Subtree; the NoteVerifier::verify path constructs Subtree::new(0, checkpoint.size()) and bails on empty checkpoints.

Spec invariants enforced at the API boundary

  • [start, end) subtree validity (start < end, alignment to next-power-of-two width per draft-ietf-plants-merkle-tree-certs §4.1) is expressed in the type via &Subtree.
  • timestamp != 0 when start != 0 is rejected on verify; sign_subtree panics on the same combination since it indicates caller error.
  • log_origin is asserted to fit opaque<1..2^8-1>. The matching bound on cosigner_name is part of KeyName's invariant (see below).
  • extract_timestamp_millis rejects blobs whose length isn't exactly TIMESTAMPED_SIGNATURE_LEN, and uses checked_mul to guard seconds-to-millis conversion against overflow.

signed_note changes

  • Added KeyName::MAX_LEN = 255 and enforce it in KeyName::new. The signed-note spec itself doesn't constrain length, but every C2SP consumer that embeds a key name in a length-prefixed wire format does so as opaque<1..2^8-1>cosigner_name and log_origin in tlog-cosignature being the motivating example. Pushing the cap into the type means consumers can rely on the invariant without re-checking, and over-long names fail closed at construction. Public-API tightening — call out in the next signed_note release notes.
  • Added SignatureType::MlDsa44 = 0x06.
  • New test_key_name_validation covers empty / ASCII whitespace / Unicode whitespace / + / max-length acceptance / byte-vs-character distinction.

Workspace plumbing

  • [workspace.dependencies] adds ml-dsa = "=0.1.0-rc.8" (the = pin matches the wider RustCrypto pre-release pinning convention already documented in the comment block).
  • pkcs8 is tightened from "0.11.0-rc.11" to "=0.11.0-rc.11" so transitive resolvers don't pick the released 0.11.0 (whose API is incompatible with the rc.11 ed25519/ml-dsa pre-releases).
  • tlog_cosignature gains length_prefixed (for the TLS-style length-prefixed cosigner_name / log_origin encoding) and ml-dsa dependencies.

Test coverage

12 unit tests in tlog_cosignature::subtree_v1:

  • Sign-then-verify roundtrips for both checkpoint (start = 0) and arbitrary-subtree (start != 0) cases.
  • Spec's timestamp/start invariant.
  • Mismatched-input rejection (different subtree, different hash, different origin).
  • NoteVerifier::verify checkpoint reconstruction path through a real signed_note::Note.
  • extract_timestamp_millis envelope-length and overflow handling.
  • Key-ID derivation against the explicit 0x06 algorithm byte.
  • Byte-pinned regression test on the cosigned_message layout (catches any drift in the TLS presentation).
  • Pin on Subtree::new's alignment rejection so the type-level validation can't quietly weaken.

Followup

When the IETF MTC draft-04 lands, the MTC CA cosigner implementation (in a future ietf_mtc_api crate) will use build_cosigned_message directly with PKIX-defined signers (Ed25519, ECDSA, RSA, ML-DSA-44, ML-DSA-65, ML-DSA-87 per the draft). The signed-note-line wrapper isn't needed for cert-embedded cosignatures (the timestamp lives in the body); MTC sign-subtree implementations will use timestamped_signature directly with their own algorithm.

Testing

cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic, cargo test, cargo fmt --all --check, cargo machete all pass locally.

@lukevalenta
Copy link
Copy Markdown
Contributor Author

/bonk review

Copy link
Copy Markdown

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bonk review

Solid PR. Workspace builds clean (cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic) and the 9 new unit tests pass. The split into algorithm-independent body builders (build_cosigned_message, timestamped_signature) plus the spec-mandated ML-DSA-44 wrapper is the right factoring for the upcoming MTC consumer described in the followup.

What's good

  • Spec faithfulness. cosigned_message_layout_unchanged is exactly the right thing to do for a wire-format type — any drift in the TLS-presentation encoding instantly invalidates every previously-produced cosignature and a regression here would be silent without this test.
  • Sign/verify symmetry on spec invariants. start < end is asserted on sign and rejected on verify; timestamp == 0 when start != 0 is panicked on sign and rejected on verify. verify_rejects_nonzero_timestamp_with_nonzero_start even hand-crafts a cryptographically-valid blob with the bad (start, timestamp) pair to confirm the verifier rejects on the policy check rather than relying on the signature being wrong.
  • Length-bound enforcement at construction. Pushing the 255-byte cap into KeyName::MAX_LEN rather than re-checking at every encoding site is the right call: build_cosigned_message's debug_assert! on cosigner_name is then a documentation-only redundancy, while log_origin (which is a &str, not a typed KeyName) keeps the release-mode assert!. The test_key_name_validation byte-vs-character case ("é".repeat(...) covering UTF-8 byte/char skew) is the test I'd have asked for.
  • Key-ID pinning. key_id_uses_signature_type_ml_dsa_44 recomputes with the literal 0x06 rather than feeding SignatureType::MlDsa44 as u8 back through, so a future bug that misroutes the algorithm byte through the enum still gets caught.
  • pkcs8 = "=0.11.0-rc.11" pin. Tightening this to an exact pin alongside the new ml-dsa = "=0.1.0-rc.8" is correct — the released pkcs8 0.11.0 is API-incompatible with the rc.11 ed25519/ml-dsa pre-releases, and a transitive resolver would happily pick the released crate without the =.

Observations / minor

  1. KeyName::MAX_LEN is a public-API tightening. KeyName::new previously accepted names of any length subject to whitespace/+/non-empty rules; it now rejects names > 255 bytes. For a 0.2-series crate that's fine, and the rationale is sound (every C2SP consumer that wire-encodes a name uses opaque<1..2^8-1>). Worth a CHANGELOG/release-notes mention when signed_note is next published, since downstream code that previously accepted longer names would now hit NoteError::InvalidKeyName at construction. Not a blocker — flagging because the PR body doesn't call this out as a breaking change.

  2. extract_timestamp_millis accepts blobs that verify_subtree rejects. parse_timestamped_signature_timestamp returns Some(_) for any blob >= 2428 bytes, so a 3000-byte blob with a valid 8-byte prefix yields Ok(Some(ts * 1000)) from extract_timestamp_millis even though the same blob fails verify_subtree (because EncodedSignature::try_from requires exactly 2420 bytes in the tail). Same pattern exists in cosignature_v1 and the spec doesn't say extract_timestamp_millis has to validate full structure, so this is consistent rather than buggy — but it means callers that use extract_timestamp_millis for ordering/pruning before verification could be fed an attacker-chosen timestamp from a malformed blob. Documenting that extract_timestamp_millis is a parse-only helper, not a validation gate, would prevent that misuse.

  3. parse_timestamped_signature_timestamp uses byteorder::ReadBytesExt after a length check. Once sig_blob.len() >= 8 is established the read can't fail, so [..8].try_into().map(u64::from_be_bytes) would be slightly cleaner and avoid the Option chain — but it's a wash and matches the surrounding style.

  4. sign_subtree allows start == 0 && timestamp == 0. Spec-legal (the spec says "no statement is made"), but for the C2SP signed-note flow such a cosignature is operationally useless — a witness producing a checkpoint cosignature SHOULD set a non-zero timestamp. The doc on CheckpointSigner::sign says "trusting them not to pass zero in this path"; that comment is fine, but consider whether the inherent sign_subtree should also warn (in docs) that start == 0 && timestamp == 0 is legal-but-likely-a-bug, given that the asymmetric panic on start != 0 && timestamp != 0 reads as "we enforce spec invariants here."

  5. SubtreeV1NoteVerifier::verify only handles the checkpoint case. That's the only thing a NoteVerifier can handle (the trait gives you &[u8] msg which is the note body), and the doc is clear. Just noting that consumers verifying arbitrary-subtree cosignatures from non-checkpoint sources (e.g. an MTC sign-subtree response embedded in a custom transport) must call verify_subtree directly rather than going through the NoteVerifier trait — that's the documented contract.

crates/tlog_cosignature/src/subtree_v1.rs:296-303

The seconds/millis split between CheckpointSigner::sign (millis in) and sign_subtree (seconds in) is the right boundary, but the integer division silently truncates sub-second precision. 1_700_000_000_999 becomes 1_700_000_000 in the signed body, and a verifier comparing the extracted millis to a wall-clock timestamp will be off by up to 999ms. Per the cosignature spec the cosigned timestamp is POSIX seconds, so the truncation is correct — just want to confirm the upstream witness's freshness window accommodates this. (Not a code change request; flagging because witness implementations sometimes bake in tight clock-skew bounds.)

Nits

  • crates/tlog_cosignature/src/subtree_v1.rs:451-454 — signing_key() returns ExpandedSigningKey<MlDsa44>; the doc comment says "companion verifying key is derived via signature::Keypair" but signature::Keypair isn't imported in this file. The comment refers to the trait that ExpandedSigningKey::verifying_key() implements; consider rewording to "via ExpandedSigningKey::verifying_key" to make the reference resolvable from the file.
  • crates/tlog_cosignature/src/subtree_v1.rs:101 — TIMESTAMPED_SIGNATURE_LEN = 8 + 2420 would be clearer as 8 + ml_dsa::SignatureSize::<MlDsa44>::USIZE if the ml-dsa API exposes that constant — keeps the 2420 magic number anchored to the upstream type. If it doesn't, the explicit 2420 with the doc comment is fine.

@ask-bonk
Copy link
Copy Markdown

ask-bonk Bot commented Apr 30, 2026

Review posted to PR #227: #227

Summary of review

Verdict: Solid PR. Workspace builds clean under pedantic clippy and the 9 new tests pass.

Highlights of what's good:

  • Spec faithfulness with byte-pinned cosigned_message_layout_unchanged regression test
  • Sign/verify symmetry on the spec invariant timestamp == 0 when start != 0 (with a hand-crafted cryptographically-valid blob in the test to confirm policy enforcement)
  • KeyName::MAX_LEN pushed into the type so build_cosigned_message's debug_assert! is documentation-only
  • Key-ID test recomputes with literal 0x06 rather than round-tripping through the enum
  • pkcs8 = "=0.11.0-rc.11" exact pin is correct given the released 0.11.0 would break the rc.11 pre-releases

Flagged for discussion (no blockers):

  1. KeyName::MAX_LEN is a public-API tightening worth a changelog mention when signed_note is next published
  2. extract_timestamp_millis returns Ok for blobs verify_subtree rejects — consistent with cosignature_v1 but worth documenting as parse-only
  3. sign_subtree allows start == 0 && timestamp == 0 (spec-legal but operationally useless); inconsistent with the asymmetric panic for the other direction
  4. Sub-second timestamp truncation in CheckpointSigner::sign (millis → seconds via /1000) — flagging for witness clock-skew compatibility
  5. Two minor doc/clarity nits on test helper comments and the 2420 magic number

github run

Base automatically changed from lvalenta/tlog-cosignature to main April 30, 2026 13:05
@lukevalenta lukevalenta force-pushed the lvalenta/subtree-v1 branch from 7a7248e to a24be7b Compare April 30, 2026 13:11
@lukevalenta
Copy link
Copy Markdown
Contributor Author

/bonk review

Copy link
Copy Markdown

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bonk review (re-run)

Pre-push checks reproduced clean on a fresh checkout: cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic, cargo fmt --all --check, and cargo test -p tlog_cosignature -p signed_note --lib all pass (9 new subtree_v1 tests + the new test_key_name_validation green). cargo machete not available in this runner; advisory.

The earlier review on this PR already covered the substantive observations (parse-only extract_timestamp_millis semantics, KeyName::MAX_LEN being a public-API tightening worth a CHANGELOG note, sign/verify symmetry on the start != 0 ⇒ timestamp == 0 invariant, the = pkcs8 pin, the seconds/millis truncation in CheckpointSigner::sign). I won't repeat them — those still stand. A few additions from this pass:

Things the prior review didn't flag

  1. extract_timestamp_millis is subtree/v1-stricter than cosignature/v1. cosignature_v1::extract_timestamp_millis (crates/tlog_cosignature/src/cosignature_v1.rs:131) only requires 8 bytes — it'll happily decode a timestamp from a 10-byte garbage blob. The new subtree_v1 version (crates/tlog_cosignature/src/subtree_v1.rs:439-443) routes through parse_timestamped_signature_timestamp, which requires sig_blob.len() >= TIMESTAMPED_SIGNATURE_LEN (2428 bytes). That's a closer fit to the wire format, but the earlier review's framing ("consistent with cosignature_v1") slightly understates it: the new helper is stricter than its sibling, just not strict enough to reject blobs in [2428, 3000) that verify_subtree will later reject for tail-length reasons. If the documented contract is "parse-only," fine; if it's "well-formed envelope," the tail check belongs here too. Pick one and document it.

  2. extract_timestamp_millis uses unchecked ts_secs * 1000. crates/tlog_cosignature/src/subtree_v1.rs:442 — for any ts_secs > u64::MAX / 1000 (~5.85e14 — practically only attacker-supplied garbage prefixes) this overflows. In release this wraps silently and returns a bogus millis value; in debug it panics. cosignature_v1 has the same shape, so this isn't a regression, but since the new version already does a length check on the blob, it's a natural place to also checked_mul(1000) and return Err(NoteError::Timestamp) on overflow rather than propagating wrapped/panicking arithmetic to callers using the value for ordering or rate-limiting.

  3. SubtreeV1NoteVerifier: Clone clones a 1312-byte ML-DSA-44 verifying key. CheckpointSigner::verifier() returns Box::new(self.v.clone()) (line 320), and Note::verify will hold the boxed verifier for the lifetime of the verification call. Not a real cost (called once per checkpoint), but the Ed25519 sibling does the same with a 32-byte key, so worth knowing the constant factor here is ~40× larger. No action needed.

  4. build_cosigned_message has a debug_assert on cosigner_name and a release assert on log_origin. The asymmetry is documented in the doc comment ("KeyName::new already enforces this") and is correct given KeyName's new invariant. Worth confirming downstream code doesn't hand-construct a KeyName (e.g. via serde deserialize bypassing KeyName::new) — a quick grep didn't find any, but if signed_note ever adds Deserialize for KeyName, the deserializer needs to call KeyName::new to preserve the invariant the debug_assert relies on.

  5. SubtreeV1CheckpointSigner doesn't impl Clone. Inconsequential — MlDsaExpandedSigningKey<MlDsa44> may not be Clone and signers aren't typically cloned — but worth noting for callers that want to share a signer across tasks (they'll need an Arc).

Confirming prior-review items still apply post-fresh-build

  • cosigned_message_layout_unchanged is exactly the right regression test for a wire-format type. ✓
  • verify_rejects_nonzero_timestamp_with_nonzero_start correctly proves the policy check fires before signature verification by hand-crafting a cryptographically-valid blob. ✓
  • key_id_uses_signature_type_ml_dsa_44 recomputes with the literal 0x06. ✓
  • pkcs8 = "=0.11.0-rc.11" is correct. ✓

Verdict

No blockers. The PR description's "spec invariants enforced at the API boundary" framing matches what the code does. The followup #1, #2, and #5 above are minor enough to land as-is and address in a separate cleanup if at all.

@ask-bonk
Copy link
Copy Markdown

ask-bonk Bot commented Apr 30, 2026

Review posted to PR #227.

Summary

Re-ran the pre-push checks on a fresh checkout — clippy pedantic, fmt, and the 9 new subtree_v1 tests + new test_key_name_validation all pass.

The prior bonk review on this PR was thorough; I focused this pass on additions rather than duplication. New observations:

  1. extract_timestamp_millis is stricter in subtree_v1 than cosignature_v1 but not maximally strict — accepts blobs in [2428, 3000) bytes that verify_subtree rejects. Worth picking parse-only vs. well-formed-envelope and documenting.
  2. Unchecked ts_secs * 1000 in extract_timestamp_millis (line 442) — wraps in release, panics in debug for adversarial timestamp prefixes; checked_mul would be cleaner.
  3. SubtreeV1NoteVerifier::Clone copies a 1312-byte ML-DSA-44 key vs Ed25519's 32 bytes — not a problem, just ~40× larger constant.
  4. KeyName debug_assert vs log_origin release assert asymmetry depends on KeyName::new always being called — fine today, brittle if signed_note later adds Deserialize.
  5. SubtreeV1CheckpointSigner doesn't impl Clone — callers wanting to share will need Arc.

No blockers.

github run

@lukevalenta lukevalenta force-pushed the lvalenta/subtree-v1 branch 2 times, most recently from f191628 to 1fd2c92 Compare April 30, 2026 14:34
@lukevalenta
Copy link
Copy Markdown
Contributor Author

/bonk review again

Copy link
Copy Markdown

@ask-bonk ask-bonk Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/bonk review (third pass)

Pre-push checks pass on a fresh build: cargo clippy --workspace --all-targets -- -Dwarnings -Dclippy::pedantic, cargo fmt --all --check, cargo machete, and cargo test -p tlog_cosignature -p signed_note --lib all green. Test count is now 12 in tlog_cosignature (9 from the original PR body + 3 added since the prior review pass: extract_timestamp_millis_rejects_overflow, extract_timestamp_millis_rejects_wrong_length, subtree_constructor_rejects_misaligned_pairs).

What changed since the previous review pass

The author addressed every actionable item from the prior two review pass without introducing any other code drift. Verified against the diff:

  • extract_timestamp_millis overflow safety (crates/tlog_cosignature/src/subtree_v1.rs:481) — ts_secs.checked_mul(1000).ok_or(NoteError::Timestamp)? replaces the previous unchecked ts_secs * 1000. Adversarial prefixes near u64::MAX now return Err(NoteError::Timestamp) instead of wrapping in release / panicking in debug. extract_timestamp_millis_rejects_overflow pins this with a u64::MAX prefix.
  • Parse-only / well-formed-envelope dichotomy (crates/tlog_cosignature/src/subtree_v1.rs:503) — parse_timestamped_signature_timestamp now requires sig_blob.len() != TIMESTAMPED_SIGNATURE_LEN (exact match, not >=). The prior review's [2428, 3000) accept-but-fail-verify gap is closed: any blob whose length doesn't match the envelope exactly is rejected at the parse layer. extract_timestamp_millis_rejects_wrong_length pins both the too-short and too-long sides plus the exact-length acceptance with a garbage tail. The doc on extract_timestamp_millis (line 478-483) now spells out the parse-only contract: "Returning Ok(Some(_)) means the blob has the correct envelope shape … It does not mean the signature itself is valid, or that the timestamp is the one the signer actually committed to."
  • Typed Subtree parametersign_subtree and verify_subtree now take &Subtree (crates/tlog_cosignature/src/subtree_v1.rs:285-291, 402-408) rather than raw (start, end) u64s. Subtree::new enforces draft-ietf-plants-merkle-tree-certs §4.1 (lo < hi plus the next-power-of-two alignment); the previous runtime start < end check is now a type-system invariant. subtree_constructor_rejects_misaligned_pairs pins [3, 5), [16, 100), [5, 5) rejection and [8, 16) acceptance. build_cosigned_message keeps the raw-u64 shape with a documented contract for low-level consumers (e.g. MTC CA cosigners that already have validated subtree bounds).
  • Spec-legal-but-operationally-useless documentation (crates/tlog_cosignature/src/subtree_v1.rs:264-273) — sign_subtree's doc now flags that start == 0 && timestamp == 0 is spec-legal but useless for witness use, and explicitly says "callers producing checkpoint cosignatures for witness use must supply a non-zero timestamp themselves." That resolves the asymmetry concern (we don't enforce the symmetric panic for the spec-legal case, and now the doc says why).
  • extract_timestamp_millis is subtree/v1-stricter than cosignature/v1 — confirmed: cosignature_v1::extract_timestamp_millis (crates/tlog_cosignature/src/cosignature_v1.rs:131-137) still uses unchecked ts * 1000 and reads from any >= 8 byte slice. The subtree/v1 version is now both length-tight and overflow-safe. The asymmetry is fine for this PR — cosignature_v1 is unchanged and its behavior pre-existed — but worth a follow-up to bring cosignature_v1 to parity. Not a blocker for this PR.

New observations from this pass

crates/tlog_cosignature/src/subtree_v1.rs:330

let subtree = Subtree::new(0, checkpoint.size()).map_err(|_| NoteError::Format)?;

Reasonable mapping, but NoteError::Format reads a little generic for "the checkpoint claims size 0." A 0-sized checkpoint is the only failure mode here (Subtree::new(0, n) only fails for n == 0, since 0 & (s-1) == 0 for any power-of-two s). Either the doc on this line could note that, or a more specific error variant could be introduced — but it's a minor clarity nit, not a bug. The comment three lines up does explain the failure mode, so this is largely already addressed.

crates/tlog_cosignature/src/subtree_v1.rs:478-484

extract_timestamp_millis always returns Ok(Some(_)) on success — never Ok(None). The trait signature Result<Option<u64>, NoteError> allows three outcomes; this impl produces only two. The Ed25519 cosignature/v1 impl (crates/tlog_cosignature/src/cosignature_v1.rs:131-137) and static_ct_api's impl (crates/static_ct_api/src/static_ct.rs:579) follow the same pattern, so this is consistent with the established convention. The Ed25519NoteVerifier in signed_note/src/ed25519.rs:38 is the impl that uses Ok(None) for the "no timestamp here" case. Worth confirming the Option<u64> layer is the right factoring vs. having two separate trait methods, but that's a signed_note-level question, not something to address in this PR.

crates/tlog_cosignature/src/subtree_v1.rs:599

let msg = build_cosigned_message(&name, 12345, "log/origin", 4, 8, &Hash([0u8; HASH_SIZE]));

The hand-crafted-blob test hardcodes the body parameters and assumes build_cosigned_message's signature won't drift. Since cosigned_message_layout_unchanged independently pins the byte layout, drift in build_cosigned_message's parameter order would surface there first; the test as written is fine.

KeyName invariant durability

The debug_assert! on cosigner_name's length in build_cosigned_message (line 162-165) relies on KeyName::new being the only construction path. Confirmed: KeyName(String) has a private inner field, no Deserialize impl, no public constructor other than new. The single KeyName::new call site for parsing-from-wire (crates/signed_note/src/lib.rs:471, in NoteSignature::from_bytes) routes through the constructor and so enforces the cap. If signed_note ever adds a serde::Deserialize impl for KeyName, the deserializer must call KeyName::new (or duplicate the validation) to preserve the invariant the debug_assert rests on — flagging because the convention isn't documented anywhere.

Verdict

No blockers. The PR is in noticeably better shape than it was at the first review pass: the spec invariants are now enforced both at the API boundary (typed Subtree, KeyName::MAX_LEN) and at the parse boundary (exact-length envelope check, checked_mul), with regression tests pinning each. The stacked cosignature_v1 parity follow-up (overflow safety, length tightening) is the only loose thread, and it's pre-existing rather than introduced here.

@ask-bonk
Copy link
Copy Markdown

ask-bonk Bot commented Apr 30, 2026

Review posted to PR #227.

Summary

Third review pass on PR #227. All pre-push checks pass on a fresh build (clippy pedantic, fmt, machete, tests). Test count is now 12 (vs the 9 in the original PR body) — the author added the three regression tests prior reviews asked for: extract_timestamp_millis_rejects_overflow, extract_timestamp_millis_rejects_wrong_length, subtree_constructor_rejects_misaligned_pairs.

The author addressed every actionable item from prior passes, verified against the diff:

  1. extract_timestamp_millis overflow safety — now uses checked_mul(1000) returning Err(NoteError::Timestamp) instead of wrapping/panicking
  2. Parse-only / well-formed-envelope dichotomy — parse_timestamped_signature_timestamp now requires exact TIMESTAMPED_SIGNATURE_LEN length, closing the [2428, 3000) accept-but-fail-verify gap; doc explicitly spells out parse-only contract
  3. Typed Subtree parameter on sign_subtree/verify_subtree — replaces raw u64s, encodes start < end and the alignment rules in the type system
  4. Doc on sign_subtree now flags start == 0 && timestamp == 0 as spec-legal-but-useless for witness use

New observations (none are blockers):

  • cosignature_v1::extract_timestamp_millis still has the pre-existing ts * 1000 overflow and accepts any >= 8 byte slice — subtree/v1 is now stricter than its sibling. Worth a follow-up PR to bring parity.
  • NoteError::Format for the 0-size checkpoint case is a minor clarity nit
  • extract_timestamp_millis never returns Ok(None) — the Option layer in the trait is unused by all timestamp-bearing impls; trait-level question not for this PR
  • KeyName invariant durability confirmed: only construction path is KeyName::new; if signed_note ever adds Deserialize for KeyName, the deserializer must route through new

Verdict: No blockers. PR is in noticeably better shape than at the first pass — spec invariants enforced both at the API boundary (typed Subtree, KeyName::MAX_LEN) and parse boundary (exact-length envelope, checked_mul).

github run

@lukevalenta
Copy link
Copy Markdown
Contributor Author

Ready for human review ✨ (I'll mark future PRs as draft until AI nits are cleaned up to hopefully reduce the noise)

Comment thread crates/tlog_cosignature/src/subtree_v1.rs Outdated
Comment thread Cargo.toml
Implements the `subtree/v1` cosignature format from
c2sp.org/tlog-cosignature: signed-note algorithm byte `0x06` paired
with ML-DSA-44.

Public API in `tlog_cosignature::subtree_v1`:

- `build_cosigned_message`: algorithm-independent body builder for the
  `cosigned_message` TLS-presentation struct. Takes raw `(start, end)`
  rather than a typed `&Subtree` because consumers (notably future
  draft-ietf-plants-merkle-tree-certs CA cosigners) may have already
  validated their inputs upstream and don't want a redundant
  construction; callers MUST ensure `[start, end)` is a valid subtree.
- `timestamped_signature`: `BE u64 timestamp || raw signature` blob
  used inside signed-note lines. Available for callers that produce
  signed-note-line output with a different algorithm.
- `SubtreeV1CheckpointSigner`: ML-DSA-44 signer. Implements
  `tlog_tiles::CheckpointSigner` for the checkpoint case;
  `sign_subtree` covers arbitrary subtrees and takes
  `&tlog_tiles::Subtree` so subtree validity is enforced by the type
  system rather than at runtime.
- `SubtreeV1NoteVerifier`: matching ML-DSA-44 verifier. Symmetric
  shape: `verify_subtree` takes `&Subtree`; the `NoteVerifier::verify`
  path constructs `Subtree::new(0, checkpoint.size())` and bails on
  empty checkpoints.

Spec invariants enforced at the API boundary:

- `[start, end)` subtree validity (start < end, alignment to next-
  power-of-two width per draft-ietf-plants-merkle-tree-certs §4.1) is
  expressed in the type via `&Subtree`.
- `timestamp != 0` when `start != 0` is rejected on verify;
  `sign_subtree` panics on the same combination.
- `log_origin` is asserted to fit `opaque<1..2^8-1>`. The matching
  bound on `cosigner_name` is part of `KeyName`'s invariant.
- `extract_timestamp_millis` rejects blobs whose length isn't exactly
  `TIMESTAMPED_SIGNATURE_LEN`, and uses `checked_mul` to guard
  seconds-to-millis conversion against overflow.

`signed_note` changes:

- New `KeyName::MAX_LEN = 255` enforced in `KeyName::new`. Pushes the
  TLS-presentation `opaque<1..2^8-1>` cap into the type so consumers
  can rely on it without re-checking. Public-API tightening (over-long
  names now fail at construction); call out in the next `signed_note`
  release notes.
- New `SignatureType::MlDsa44 = 0x06`.
- New `test_key_name_validation` covering empty / ASCII whitespace /
  Unicode whitespace / `+` / max-length acceptance / byte-vs-character
  distinction.

Workspace plumbing:

- Adds `ml-dsa = "=0.1.0-rc.8"`. The `=` pin matches the wider
  RustCrypto pre-release pinning convention already documented in the
  comment block.
- Tightens `pkcs8 = "=0.11.0-rc.11"` so transitive resolvers don't
  pick the released `0.11.0` (whose API is incompatible with the
  rc.11 ed25519/ml-dsa pre-releases).
- `tlog_cosignature` gains `length_prefixed` (for the TLS-style
  length-prefixed `cosigner_name` / `log_origin` encoding) and
  `ml-dsa` deps.

12 unit tests in `tlog_cosignature::subtree_v1` cover sign-then-verify
roundtrips for both checkpoint and arbitrary-subtree cases, the
timestamp/start invariant, mismatched-input rejection, the
`NoteVerifier::verify` checkpoint reconstruction path, the
`extract_timestamp_millis` envelope-length and overflow handling, the
key-ID derivation against the explicit `0x06` algorithm byte, a
byte-pinned regression test on the `cosigned_message` layout, and a
pin on `Subtree::new`'s alignment rejection so the type-level
validation can't quietly weaken.
@lukevalenta lukevalenta force-pushed the lvalenta/subtree-v1 branch from 1fd2c92 to 481d1cc Compare May 1, 2026 13:35
@lukevalenta lukevalenta merged commit 9748706 into main May 1, 2026
9 checks passed
@lukevalenta lukevalenta deleted the lvalenta/subtree-v1 branch May 1, 2026 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants