Repository navigation
Conversation
zero-aska
left a comment
There was a problem hiding this comment.
This review covers cc5dab4, which is still the head. There are no other reviews yet. The PR is a draft, so this is not an approval.
What holds: header offsets, signed poll and precision, raw root fields and timestamp order match RFC 5905 Fig. 8 and s7.3. The oracle agrees with struct.unpack('!BBbbII4s4Q') on 2,000 seeded headers, and it implements the RFC 7822 framing rules: 16-byte minimum, alignment, a final EF of at least 28 bytes without a MAC, and the 65532 maximum. Both test commands reproduce: the unittest run gives 7 OK, and check_vectors.py gives 79/79, exit 0.
What blocks is check_vectors.py: a future parser will be checked with it, and it pins too little.
Blocking 1. v3 packets get NTPv4 extension framing, and nothing pins it.
WIRE-CONTRACT.md:18accepts v3, and:20applies RFC 7822 framing.decode(tests/wire_oracle.py:49-77) never checks the version.- RFC 7822 s3 (new RFC 5905 s7.5): "In NTPv4, one or more extension fields can be inserted after the header and before the MAC". NTPv3 (RFC 1305) is a header plus an optional authenticator.
- Input: a v3 header (byte 0
0x1b), thenfefe001cand 24 zero bytes,mac_length0. Result:ok version=3 extensions=[(48, 0xfefe, 28)]. chrony 4.6.1 reads that tail as a MAC (ntp_ext.c:171-173: "Only NTPv4 packets have extension fields"), while ntpd 4.2.8p18 walks it as an EF. The two disagree, so the contract must choose. - The only v3 vector is the bare 48-byte
version-3. Two opposite mutants each pass 79/79 and the unit tests: "the whole v3 tail is the MAC" and "reject any v3 tail that is not a MAC". - Fix: state the v3 rule. Either v3 has no EFs and the tail is the MAC, or RFC 7822 framing deliberately extends to v3, as in ntpd. Add vectors for v3 + MAC20, v3 + a 28-byte trailer, and v3 + EF16 + MAC20.
Blocking 2. Unix conversion is outside the adapter schema.
- Requests are
{packet_hex, mac_length}(tests/wire_oracle.py:4). Responses carry onlyraw/seconds/fraction/unknown, andanswer()never callstimestamp_unix. - Each of these broken conversions passes
check_vectors.py79/79: era ignored; epoch offset 2208902400; raw 0 converted to a 1900 date. Only the oracle's own test (test_wire_oracle.py:35-46) fails them. - RFC 5905 s6: "a value of zero is a special case representing unknown or unsynchronized time"; "s = era * 2^(32) + timestamp", with the era "derived from external means". Issue 1 puts era handling in the slice. The PR body claims "exact conversion with an externally supplied era", and
WIRE-CONTRACT.md:24says an adapter "can use this schema unchanged". - The
unknownflag is pinned: a mutant that derives it from seconds == 0 fails 13 vectors. - Fix: add an era input (a request field or a
to_unixkind) and an exact result, such asnum/den. Add vectors: raw 0 in eras -1, 0 and 1 (null); raw 1;2208988800 << 32in era 0 (0); all-ones; and a negative era.
Blocking 3. The 79 vectors pass many wrong parsers.
-
48 vectors are prefixes of one packet. Accepted vectors use one header in v3 and v4 forms: leap 0, mode 3, stratum 16. The only EF type is
0xfefe, which is the same in either byte order. All bodies are zero. Packets are at most 92 bytes, with at most 2 EFs. -
Each of these one-edit oracle changes passes 79/79:
- leap always 0
- mode read as
byte0 & 3 - stratum read as int8
- EF type byte-swapped or ignored
- EF body ignored
- EFs over 64 bytes rejected
- packets over 96 bytes rejected
- a third EF rejected
- any
mac_lengthaccepted - an EF minimum of 4
-
Input for the last one: a v4 header, then
fefe000cand 8 zero bytes, thenfefe001cand 24 zero bytes,mac_length0. The oracle returnsshort_extension, but the min-4 parser returnsokwith EFs of 12 and 28 bytes. RFC 7822 s3: "the minimum field length containing required fields is four words (16 octets)". Vectors 73-75 miss this because zeros follow the short field and fail with the same code. -
Fix: add vectors for:
- leap 1-3, modes 0-7, and strata 0, 1, 17 and 255
- EF type
0x0104and nonzero bodies - 3+ EFs, and EFs of 1024 and 65532 bytes
- EF12 then EF28, and EF8 then MAC20
mac_length8, 16 and 28
Better, add a seeded random differential pass to
check_vectors.pythat compares the adapter with the oracle and prints the seed and count. The 2,000-header test and the 0..80 sweep catch most of these mutants, but they run only on the oracle.
Non-blocking:
mac_length0 rejects ordinary MD5 packets: a v4 header,00000001and 16 bytes returnsshort_extension. chrony reads a 20..24-byte remainder as a MAC (ntp_core.c:1609-1613), and under RFC 7822 such a tail can never be a valid final EF. So "does not guess" (WIRE-CONTRACT.md:22) is misleading. Consider an auto mode, and say which mode the Lean claims use.- Vector 64 accepts the 4-byte MAC
00000001. RFC 5905 defines the crypto-NAK as "four octets of zeros", and chrony requires zero. State the choice, or flip the vector. - Modes 6 and 7 are decoded as RFC 5905 headers: byte 0
0x26with a 28-byte trailer returnsok mode=6. Reject them, or say their fields mean nothing. - Error codes and their order are pinned by vectors but not listed in the contract.
check_vectors.py:30compares parsed JSON with!=, so an adapter that prints ints as floats and booleans as 0/1 gets 79/79. Compare canonical JSON. README.md:28still says an EF that "runs past the end of the packet is rejected", andREADME.md:32-34still lists the old oracles. There is no cross-check against chrony, NTPsec or ntpd-rs, and no vector for CVE-2014-9750. The contract does not say that 8-octet Autokey EFs are rejected.
Not run: there is no Lean code, so no lake build, and there is no bar.toml for check.py. I did not run chrony, NTPsec, ntpd-rs or ntpd; those comparisons come from reading the chrony 4.6.1 and ntpd 4.2.8p18 source. The probes and mutants ran against this PR's wire_oracle.py through its runner.
Before this leaves draft, it needs the three blocking fixes, the independent contract review that the PR body asks for, and green CI on the new vectors.
Generated by Claude Code
|
Closing my draft as superseded by #4, which merged as beb57cc.
Nothing here is left that #4 lacks, so a rebase would add no value. Generated by Claude Code |
Refs #1.
Adds a draft NTP wire contract, 79 fixed packet vectors, a Python reference oracle, and a runner that can check a future parser adapter against the same responses. The existing parser author's implementation remains separate. This PR adds no Lean parser or proof and does not complete the first slice.
The contract covers header offsets, signed poll and precision fields, zero timestamps as unknown, and exact conversion with an externally supplied era. Extension framing follows RFC 7822, with explicit MAC length context to preserve 4-, 20-, and 24-byte tails without guessing or authenticating them. Independent contract and code review is required before integration.
Validation: seven test groups pass, including all 79 fixtures, 2,000 headers compared with Python struct decoding, extension lengths 0 through 80 under four MAC contexts, the 65,532-byte extension boundary, and adapter failure checks. A GitHub Actions workflow runs the suite.