diff --git a/README_proxy.md b/README_proxy.md index fa48db8..f1bc830 100644 --- a/README_proxy.md +++ b/README_proxy.md @@ -257,6 +257,16 @@ The update handler now checks both `Pseudo` and `Extra`, and it accepts either: This is a library packing bug, not a sig0lease protocol issue -- real DNS traffic essentially never sets QDCOUNT > 1 anyway, and this project's own `acceptMsg` (see [server/server.go](server/server.go)) rejects any message where `len(m.Question) != 1` with FORMERR, matching the library's own `DefaultMsgAcceptFunc`. No compatibility patch was written for it. It surfaced while writing [server/transport_equivalence_test.go](server/transport_equivalence_test.go), which needed a genuine two-question wire message to test that FORMERR-rejection branch; that specific case had to be dropped since the library can't produce valid bytes for it, and the message-with-zero-questions case was kept in its place to cover the same `acceptMsg` branch. +### TCP server silently drops Ns/Extra/Pseudo (missing second Unpack) + +`(*dns.Server).serveDNS` (used internally by `(*dns.Server).ListenAndServe`, the library's own TCP server loop) unpacks each incoming message twice by design: once with `Options = MsgOptionUnpackQuestion` (header + question only, cheap enough to run before `MsgAcceptFunc` decides whether to bother with the rest), then again with `Options = MsgOptionUnpack` for the full message. In v0.6.82 (and every version checked up to and including the latest, v0.6.104 -- see [docs/upgrade-miekg-dns.md](docs/upgrade-miekg-dns.md) for the version-by-version check), the second call is simply missing: `Options` is set but `Unpack()` is never invoked again before the handler runs. `Ns`, `Extra`, and `Pseudo` are left at their header-only-parse zero values for every TCP-received message, regardless of what the client actually sent -- so a TCP `register`/`refresh` request always looked like "UPDATE without UPDATE-LEASE EDNS option" to the update handler, even though the option was present on the wire and the same bytes worked fine over UDP. + +This is a library server-loop bug, not a sig0lease protocol issue. + +### Applied patch + +`server/server.go`'s `serveTCP` no longer uses `(*dns.Server).ListenAndServe` at all. It's replaced with a custom accept loop, structurally identical to the pre-existing `serveUDP` (which already worked around this by never using the library's server in the first place): read one length-prefixed message (RFC 1035 4.2.2), call `Unpack()` exactly once with no `Options` set (an unconditional full unpack, sidestepping the two-stage split entirely), apply the shared `acceptMsg` accept/reject policy, then dispatch to the handler via a small `tcpResponseWriter`. `server/transport_equivalence_test.go`'s `TestServeTCPPreservesUpdateLeaseOption` regression-tests this directly (confirmed failing against the old library-based `serveTCP`, passing against the replacement). + ## Applied Compatibility Patches The following project-side patches are currently in place: @@ -265,6 +275,7 @@ The following project-side patches are currently in place: 2. `cmd/sig0lease/main.go` imports the compatibility package so the proxy process gets the patch before reading packets. 3. `cmd/sig0lease-client/main.go` imports the same compatibility package so client-side pack/unpack behavior stays consistent. 4. `pkg/lease.FindOption` recognizes UPDATE-LEASE whether it arrives as a direct `ERFC3597` record or under an `OPT` wrapper, for both request and response parsing. +5. `server/server.go`'s `serveTCP` replaces the library's own TCP server loop with a custom one (see above) so TCP-received messages get a real, complete `Unpack()`. # Implementation Status diff --git a/cmd/sig0lease-client/main.go b/cmd/sig0lease-client/main.go index 4d01633..4280d2c 100644 --- a/cmd/sig0lease-client/main.go +++ b/cmd/sig0lease-client/main.go @@ -106,19 +106,67 @@ func extractSignerLocationFlag(args []string) ([]string, string) { return out, location } +// keyRRFromClientKey builds a *dns.KEY RR from the already-loaded signing +// key, so callers don't need to re-derive or re-type its RDATA by hand. +func keyRRFromClientKey(clientKey *keyrec.LoadedKey, ttl uint32) *dns.KEY { + keyRR := new(dns.KEY) + keyRR.Hdr.Name = clientKey.KeyName() + keyRR.Hdr.Class = dns.ClassINET + keyRR.Hdr.TTL = ttl + keyRR.Flags = clientKey.PublicKey.Flags + keyRR.Protocol = clientKey.PublicKey.Protocol + keyRR.Algorithm = clientKey.PublicKey.Algorithm + keyRR.PublicKey = clientKey.PublicKey.PublicKey + return keyRR +} + func addSignerKeyToAdditional(msg *dns.Msg, clientKey *keyrec.LoadedKey, keyLeaseDuration uint32) { - signingKeyRR := new(dns.KEY) - signingKeyRR.Hdr.Name = clientKey.KeyName() - signingKeyRR.Hdr.Class = dns.ClassINET - signingKeyRR.Hdr.TTL = keyLeaseDuration - signingKeyRR.Flags = clientKey.PublicKey.Flags - signingKeyRR.Protocol = clientKey.PublicKey.Protocol - signingKeyRR.Algorithm = clientKey.PublicKey.Algorithm - signingKeyRR.PublicKey = clientKey.PublicKey.PublicKey + signingKeyRR := keyRRFromClientKey(clientKey, keyLeaseDuration) msg.Extra = append(msg.Extra, signingKeyRR) fmt.Printf(" ✓ Added signer KEY RR to Additional section: %s\n", signingKeyRR.String()) } +// extractSameKeyFlag pulls a bare --same-key token out of args, wherever it +// appears, returning the remaining positional args and whether it was set. +func extractSameKeyFlag(args []string) ([]string, bool) { + const flag = "--same-key" + sameKey := false + out := make([]string, 0, len(args)) + for _, a := range args { + if a == flag { + sameKey = true + continue + } + out = append(out, a) + } + return out, sameKey +} + +// extractTCPFlag pulls a bare --tcp token out of args, wherever it appears, +// returning the remaining positional args and whether it was set. Absent, +// the client uses UDP (client.New's own default). +func extractTCPFlag(args []string) ([]string, bool) { + const flag = "--tcp" + useTCP := false + out := make([]string, 0, len(args)) + for _, a := range args { + if a == flag { + useTCP = true + continue + } + out = append(out, a) + } + return out, useTCP +} + +// queryProtocol maps --tcp's presence to the protocol string client.New expects. +func queryProtocol(useTCP bool) string { + if useTCP { + return "tcp" + } + return "udp" +} + func cmdRegRefWithMode(proxyAddr string, args []string, operation string, tamper bool) { args, signerLocation := extractSignerLocationFlag(args) switch signerLocation { @@ -127,9 +175,11 @@ func cmdRegRefWithMode(proxyAddr string, args []string, operation string, tamper fmt.Fprintf(os.Stderr, "ERROR: invalid --signer=%s (expected update|additional|none)\n", signerLocation) os.Exit(1) } + args, sameKey := extractSameKeyFlag(args) + args, useTCP := extractTCPFlag(args) if len(args) < 3 { - fmt.Fprintf(os.Stderr, "Usage: sig0lease-client register|register-tamper|refresh [lease] [key-lease] [rr-spec...] [--signer=update|additional|none]\n") + fmt.Fprintf(os.Stderr, "Usage: sig0lease-client register|register-tamper|refresh [lease] [key-lease] [rr-spec...] [--signer=update|additional|none] [--same-key] [--tcp]\n") os.Exit(1) } @@ -186,6 +236,21 @@ func cmdRegRefWithMode(proxyAddr string, args []string, operation string, tamper os.Exit(1) } + if sameKey { + for _, rr := range updateKeyRRs { + if strings.EqualFold(rr.Hdr.Name, clientKey.KeyName()) { + fmt.Fprintf(os.Stderr, "ERROR: --same-key conflicts with an explicit KEY rr-spec for %s already among the rr-spec arguments\n", clientKey.KeyName()) + os.Exit(1) + } + } + if signerLocation == signerLocationNone { + fmt.Fprintf(os.Stderr, "ERROR: --same-key conflicts with --signer=none: the signing key must appear in the Update section\n") + os.Exit(1) + } + updateKeyRRs = append(updateKeyRRs, keyRRFromClientKey(clientKey, keyLeaseDuration)) + fmt.Printf(" ✓ --same-key: reusing signing key %s as the Update-section lease payload\n", clientKey.KeyName()) + } + fmt.Printf("=== sig0lease Client %s ===\n", operation) fmt.Printf("Proxy: %s\n", proxyAddr) @@ -319,7 +384,8 @@ func cmdRegRefWithMode(proxyAddr string, args []string, operation string, tamper } // Send to proxy - fmt.Printf("\nSending to proxy (%s)\n", proxyAddr) + protocol := queryProtocol(useTCP) + fmt.Printf("\nSending to proxy (%s) over %s\n", proxyAddr, protocol) // Check message before packing fmt.Printf(" Message structure before sending:\n") @@ -333,7 +399,7 @@ func cmdRegRefWithMode(proxyAddr string, args []string, operation string, tamper } fmt.Printf(" Packed size: %d bytes\n", len(signedMsg.Data)) - c := client.New(proxyAddr, "udp", 20*time.Second) + c := client.New(proxyAddr, protocol, 20*time.Second) resp, err := c.Query(signedMsg) if err != nil { fmt.Fprintf(os.Stderr, "ERROR: Failed to send query: %v\n", err) @@ -424,20 +490,23 @@ func flipOnePayloadBit(msg *dns.Msg) error { // cmdVerify checks if a key registration is active func cmdVerify(proxyAddr string, args []string) { + args, useTCP := extractTCPFlag(args) if len(args) < 1 { - fmt.Fprintf(os.Stderr, "Usage: sig0lease-client verify \n") + fmt.Fprintf(os.Stderr, "Usage: sig0lease-client verify [--tcp]\n") os.Exit(1) } zone := args[0] + protocol := queryProtocol(useTCP) fmt.Printf("=== Verifying Key Registration ===\n") fmt.Printf("Proxy: %s\n", proxyAddr) fmt.Printf("Zone: %s\n", zone) + fmt.Printf("Protocol: %s\n", protocol) // Send a standard query for the key record msg := dns.NewMsg(zone, dns.TypeKEY) - c := client.New(proxyAddr, "udp", 20*time.Second) + c := client.New(proxyAddr, protocol, 20*time.Second) resp, err := c.Query(msg) if err != nil { fmt.Fprintf(os.Stderr, "ERROR: Query failed: %v\n", err) @@ -504,7 +573,7 @@ Usage: sig0lease-client [args...] Commands: - register [lease] [key-lease] [rr-spec...] [--signer=update|additional|none] + register [lease] [key-lease] [rr-spec...] [--signer=update|additional|none] [--same-key] [--tcp] Send a sig0lease UPDATE-LEASE registration request keyname: filename of the key in the keystore (e.g., Ktest.dev.zenr.io.+015+05044) @@ -513,42 +582,78 @@ Commands: rr-spec: optional additional RR in DNS presentation format: --signer: where the signer's KEY RR should appear in the request. Tests the - proxy's signer resolution: request-provided (update/additional) vs. - resolved server-side (lease store or authoritative DNS). + proxy's signer resolution: request-provided (--signer=update/additional) vs. + resolved server-side (lease store or authoritative DNS if --signer=none). update: signer's own KEY rr-spec must also be passed; no Additional copy additional: signer KEY is placed in the Additional section (default when no matching KEY rr-spec is given) none: signer KEY is omitted entirely; proxy must resolve it from the lease store or authoritative DNS + --same-key: lease the same key used to sign the request, without retyping its + KEY rr-spec. Builds the KEY RR from the loaded signing key and adds it to + the Update section; conflicts with an explicit KEY rr-spec for the same + name and with --signer=none. + --tcp: send the request over TCP instead of the default UDP. + + Note: the server dispatches on the LEASE/KEY-LEASE combination and + requires specific RR kinds to be present for each (handlers/opcode5_handle.go): + KEY-LEASE!=0 and LEASE!=0: requires >=1 KEY RR and >=1 non-KEY RR + KEY-LEASE=0 and LEASE!=0: requires >=1 non-KEY RR and 0 KEY RRs (signer must already be managed) + KEY-LEASE!=0 and LEASE=0: requires >=1 KEY RR (KEY-only lease) + A duration with no matching RR present is rejected, not silently ignored. Example: - // Key-only registration - sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 300 0 - // Key and other RRs registration - sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 300 3600 "client.test.dev.zenr.io. 300 IN TXT \"hello\"" - // Refresh signed by an already-managed or online-only key, key omitted from the request - sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 300 3600 --signer=none - - refresh [lease] [key-lease] [rr-spec...] [--signer=update|additional|none] + // Key-only registration: lease the signing key itself (LEASE=0, KEY-LEASE!=0); + // --same-key supplies the required KEY RR without retyping it + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 0 3600 --same-key + // Same, repeating the key (no --same-key): the KEY rr-spec must be typed + // out in full + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 0 3600 "test.dev.zenr.io. 3600 IN KEY 512 3 15 s1Uf18NtAIacuPDIgMdw2SJ//8fm+xjLb5MPWqwxqzQ=" + // Key and other RRs registration (LEASE!=0 and KEY-LEASE!=0 requires both kinds present) + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 300 3600 --same-key "client.test.dev.zenr.io. 300 IN TXT \"hello\"" + // Non-KEY-only registration signed by an already-managed key, key omitted from the request + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 300 0 --signer=none "client.test.dev.zenr.io. 300 IN TXT \"hello\"" + // Register a different key than the one signing the request (delegation), + // without --same-key: only the other key is leased, and the signer's own + // KEY RR is added to Additional so the proxy can still verify it + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 0 3600 "client.test.dev.zenr.io. 3600 IN KEY 512 3 15 c2yGNXxlrWu1LX/n9AqrCp+rIbm9FWcotgnMomlrM2E=" + // Same, but also register the signer's own key-lease in the same request via + // --same-key: both KEY RRs end up in the Update section, no Additional copy + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 0 3600 --same-key "client.test.dev.zenr.io. 3600 IN KEY 512 3 15 c2yGNXxlrWu1LX/n9AqrCp+rIbm9FWcotgnMomlrM2E=" + // Same request over TCP instead of UDP + sig0lease-client 127.0.0.1:8053 register Ktest.dev.zenr.io.+015+05044 0 3600 --same-key --tcp + + refresh [lease] [key-lease] [rr-spec...] [--signer=update|additional|none] [--same-key] [--tcp] Send a sig0lease UPDATE-LEASE refresh request (8-byte variant) keyname: filename of the key in the keystore (e.g., Ktest.dev.zenr.io.+015+05044) lease: new lease duration in seconds key-lease: key-lease duration in seconds --signer: see register above + --same-key: see register above + --tcp: see register above Example: - // Key-only refresh - sig0lease-client 127.0.0.1:8053 refresh Ktest.dev.zenr.io.+015+05044 300 0 - // Key and other RRs refresh - sig0lease-client 127.0.0.1:8053 refresh Ktest.dev.zenr.io.+015+05044 300 3600 "client.test.dev.zenr.io. 300 IN TXT \"hello\"" + // Key-only refresh: renew the signing key's own lease (LEASE=0, KEY-LEASE!=0); + // --same-key supplies the required KEY RR without retyping it + sig0lease-client 127.0.0.1:8053 refresh Ktest.dev.zenr.io.+015+05044 0 3600 --same-key + // Key and other RRs refresh (LEASE!=0 and KEY-LEASE!=0 requires both kinds present) + sig0lease-client 127.0.0.1:8053 refresh Ktest.dev.zenr.io.+015+05044 300 3600 --same-key "client.test.dev.zenr.io. 300 IN TXT \"hello\"" + All the other register examples above (repetitive KEY rr-spec form, + non-KEY-only with --signer=none, registering a different key with and + without --same-key) apply the same way to refresh; just substitute the + command name. - verify + + verify [--tcp] Query if a key registration is active - + + --tcp: send the query over TCP instead of the default UDP. + Example: - sig0lease-client 127.0.0.1:8053 verify test.dev.zenr.io. client.test.dev.zenr.io. + sig0lease-client 127.0.0.1:8053 verify test.dev.zenr.io. + sig0lease-client 127.0.0.1:8053 verify test.dev.zenr.io. --tcp list-keys [keystore-dir] List available keys in keystore diff --git a/docs/upgrade-miekg-dns.md b/docs/upgrade-miekg-dns.md new file mode 100644 index 0000000..e2c4246 --- /dev/null +++ b/docs/upgrade-miekg-dns.md @@ -0,0 +1,106 @@ +# Future PR: bump codeberg.org/miekg/dns to v0.6.104, update pkg/dnscompat and pkg/lease + +Not done in this PR. This is a separate, self-contained follow-up. Everything below was +verified against real builds of the dependency in an isolated scratch module (not this +repo's go.mod/go.sum), so the facts are solid; the *decision* of how far to take it is not +made here. + +## What's actually fixed upstream + +The three shortcomings documented in `README_proxy.md` under "miekg/dns Shortcomings" were +tested against `codeberg.org/miekg/dns` versions v0.6.82 (pinned today) through v0.6.104 +(latest at time of writing): + +| Shortcoming | v0.6.82 | v0.6.95 | v0.6.104 | +|---|---|---|---| +| #1 UPDATE-LEASE unpack dispatcher gap (`"dns: no option unpack defined"`) | reproduces | reproduces | **fixed** | +| #2 unpacked form isn't OPT-wrapped | same cause as #1 | same | now unpacks as a real typed `*dns.UPDATELEASE{Lease, KeyLease}` RR | +| #3 multi-question round-trip (`"overflow name"`) | reproduces | silently truncates to 1 question (regression, not a fix) | reproduces again, same as v0.6.82 | + +The #1/#2 fix landed exactly at v0.6.104 (bisected: v0.6.103 still fails, v0.6.104 passes). +v0.6.104's `go.mod` requires `go >= 1.27.0`, so this repo's own `go` directive/toolchain +needs to move too. + +#3 needs no action: this project's own `acceptMsg` (`server/server.go`) already rejects any +message where `len(Question) != 1` with FORMERR before the bug would ever matter, and the +README already documents that reasoning. Don't chase it. + +## The catch: the native UPDATELEASE type does not support the 4-byte variant + +`pkg/lease.LeaseOption` supports both the 8-byte variant (LEASE + KEY-LEASE, always used for +encoding) and, on decode only, the legacy 4-byte variant (LEASE only), gated by +`prefer_4byte_variant` in config (`handlers/opcode5_lease_option.go`). `Encode4Byte` exists +but is currently dead code (nothing calls it) — only `Encode8Byte` is used, by both +`pkg/dnsmsg.NewLeaseUpdate` (client) and `handlers/opcode5_handle.go`'s response builder +(server). + +The upstream native type is 8-byte only by construction: + +```go +// v0.6.104 edns_types.go +type UPDATELEASE struct { + Lease uint32 + KeyLease uint32 +} +// zednspack.go: unpack() unconditionally reads two uint32s (8 bytes), no length branch. +``` + +I verified directly: if `pkg/dnscompat`'s override (`dns.CodeToRR[2] = ...ERFC3597...`) is +removed so the library's own dispatch table is used, a message carrying a genuine 4-byte +UPDATE-LEASE option (4 bytes of data, not 8) fails with `dns unpack: overflow data` — and +that failure kills the *entire message unpack*, not just that option, i.e. strictly worse +than today's behavior for any real 4-byte-variant sender. + +`pkg/dnscompat`'s current override works for both variants today only because it forces +`*dns.ERFC3597`, which stores the raw option bytes generically regardless of length — +`pkg/lease.decodeERFC` then branches on `len(data) == 4` vs `== 8`. + +**Conclusion: do not simply delete `pkg/dnscompat` and switch `pkg/lease` over to +`*dns.UPDATELEASE` decoding.** That silently breaks `prefer_4byte_variant: true` in a way +that fails the whole request rather than falling back. + +## Recommended paths (pick one, this is a product decision, not just a technical one) + +**Option A — safe, minimal.** Bump the dependency for whatever else v0.6.82→v0.6.104 carries +(review the intervening CHANGELOG.md entries for anything else relevant), but leave +`pkg/dnscompat`'s override and `pkg/lease` exactly as they are. ERFC3597 still handles both +variants; nothing about the fixed dispatcher gap needs to be consumed. Update the comment in +`pkg/dnscompat/updatelease.go` to say the override is now a deliberate choice (keeps 4-byte +support working) rather than a required workaround for a still-broken library. + +**Option B — bigger change.** If 4-byte-variant support (`prefer_4byte_variant`) is +confirmed genuinely unused/droppable (check with whoever owns that config flag — the code +comments call it "legacy"/"backward compatibility" but that's not the same as "safe to +delete"), then: +1. Remove `pkg/dnscompat/updatelease.go` and its blank imports in `cmd/sig0lease/main.go` + and `cmd/sig0lease-client/main.go` (it's the only file in that package). +2. `pkg/lease.Encode`: replace the `hex.EncodeToString` + `ERFC3597{EDNS0Code: OPTION_CODE, + Code: data}` construction with `opt.Options = append(opt.Options, &dns.UPDATELEASE{Lease: + lo.Lease, KeyLease: keyLease})`. +3. `pkg/lease.Decode`/`decodeERFC`: add a case recognizing `*dns.UPDATELEASE` directly + (`.Lease`/`.KeyLease` fields, no hex decode needed), and decide what happens to the + 4-byte path (drop it deliberately, with a config-time error if `prefer_4byte_variant` is + still set, rather than a silent behavior change). +4. `pkg/lease.FindOption`: extend the `scan` closure to also match `*dns.UPDATELEASE` (it + lands in `Pseudo`, confirmed empirically — same section ERFC3597 currently uses). +5. Update `pkg/lease`'s tests and `README_proxy.md`'s "miekg/dns Shortcomings" / + "Applied Compatibility Patches" sections to reflect whichever option was taken. + +## Testing either option + +- `go build ./... && go test ./...` +- `CLIENT_KEYSTORE_DIR=... tests/test_update.sh` — the real integration suite against a live + proxy and real authoritative test zone; confirms wire compatibility end-to-end, not just + unit-level. +- If Option B: explicitly test a `prefer_4byte_variant: true` config path end-to-end (or + confirm it's being deliberately removed) — this is exactly the case that silently breaks. + +## Explicitly out of scope for this bump + +The TCP `serveDNS` bug (a *different* library defect: `(*Server).serveDNS` in `server.go` +sets `r.Options = MsgOptionUnpack` but never calls `r.Unpack()` a second time before invoking +the handler, so `Ns`/`Extra`/`Pseudo` stay empty for every TCP-received message) is **not** +fixed by this bump — confirmed identical in `serveDNS` across v0.6.82 through v0.6.104. That +was fixed separately, in this repo, by replacing `serveTCP`'s use of `dns.Server` with a +custom accept loop mirroring `serveUDP` (see `server/server.go`). The two changes are +independent; don't conflate them. diff --git a/server/server.go b/server/server.go index 0ad23d6..2fef95d 100644 --- a/server/server.go +++ b/server/server.go @@ -3,7 +3,10 @@ package server import ( "context" + "encoding/binary" + "errors" "fmt" + "io" "net" "os/signal" "syscall" @@ -357,49 +360,195 @@ func (w *udpResponseWriter) Session() *dns.Session { return &dns.Session{Addr: w.remoteAddr} } -// serveTCP starts a TCP listener. +// tcpReadTimeout/tcpIdleTimeout mirror the dns library's own Server defaults +// (ReadTimeout/IdleTimeout) for the TCP listener this replaces. +const ( + tcpReadTimeout = 2 * time.Second + tcpIdleTimeout = 8 * time.Second +) + +// serveTCP starts a custom TCP listener that preserves EDNS options, the +// same way serveUDP already does and for the same reason: the dns library's +// own TCP server ((*dns.Server).ListenAndServe, used here previously) sets +// Options = MsgOptionUnpack to request a full unpack after MsgAcceptFunc +// runs, but never actually calls Unpack() a second time before invoking the +// handler -- so Ns/Extra/Pseudo (and therefore any EDNS option, including +// UPDATE-LEASE) stay empty for every TCP-received message, regardless of +// what the client actually sent. A single, unconditional Unpack() call, as +// serveUDP already does, sidesteps that bug entirely. Confirmed present +// through the newest available release (v0.6.104) of +// codeberg.org/miekg/dns; see docs/upgrade-miekg-dns.md. func (s *Server) serveTCP(ctx context.Context, handler dns.HandlerFunc) error { - // srv is declared separately (not :=) so NotifyStartedFunc below can - // close over it and read srv.Listener, which the library only - // populates once the bind actually succeeds. - var srv *dns.Server - srv = &dns.Server{ - Listener: nil, - Addr: s.cfg.Server.Address, - Net: "tcp", - Handler: handler, - TLSConfig: nil, - MsgAcceptFunc: s.acceptMsg, - MsgInvalidFunc: s.invalidMsg, - // Mirrors serveUDP's s.logger.Infof("UDP listener started on %s", - // conn.LocalAddr().String()): fires only after the listener has - // actually bound (unlike a log placed before ListenAndServe, which - // would fire even if the bind then failed), using the real bound - // address rather than the pre-resolve config string. - NotifyStartedFunc: func(context.Context) { - s.logger.Infof("TCP listener started on %s", srv.Listener.Addr().String()) - }, + // Listen on TCP with explicit IPv4, consistent with serveUDP. + tcpAddr, err := net.ResolveTCPAddr("tcp4", s.cfg.Server.Address) + if err != nil { + return fmt.Errorf("resolve TCP address: %w", err) + } + + ln, err := net.ListenTCP("tcp4", tcpAddr) + if err != nil { + return fmt.Errorf("listen TCP: %w", err) } - // Monitor context cancellation in the background + defer ln.Close() + + // Goroutine to force unblock AcceptTCP when ctx is canceled, mirroring + // serveUDP's shutdown handling. go func() { <-ctx.Done() - s.logger.Infof("TCP listener shutting down...") - - srv.Shutdown(ctx) + ln.Close() }() - err := srv.ListenAndServe() + s.logger.Infof("TCP listener started on %s", ln.Addr().String()) + + for { + conn, err := ln.AcceptTCP() + if err != nil { + select { + case <-ctx.Done(): + s.logger.Infof("TCP listener shutting down cleanly") + return nil + default: + s.logger.Errorf("TCP accept error: %v", err) + continue + } + } + go s.serveTCPConn(ctx, conn, handler) + } +} + +// serveTCPConn reads and handles DNS-over-TCP-framed messages (RFC 1035 +// 4.2.2: each message prefixed by a 2-byte big-endian length) from a single +// accepted connection, up to dns.MaxTCPQueries messages before closing. +// Queries on the same connection are handled one at a time, not +// concurrently: this project's own client only ever sends one query per TCP +// connection, so serializing keeps this simple and avoids racing a +// handler's response write against the connection close on shutdown. +func (s *Server) serveTCPConn(ctx context.Context, conn *net.TCPConn, handler dns.HandlerFunc) { + defer conn.Close() + + readTimeout := tcpReadTimeout + for q := 0; q < dns.MaxTCPQueries; q++ { + conn.SetReadDeadline(time.Now().Add(readTimeout)) + + rawData, err := readTCPMsg(conn) + if err != nil { + if !isClosedOrTimeout(err) { + s.logger.Debugf("TCP: read error from %s: %v", conn.RemoteAddr().String(), err) + } + return + } + + // Prevent processing new queries if shutdown signal arrived mid-read. + select { + case <-ctx.Done(): + return + default: + } + + s.logger.Debugf("TCP: Received %d bytes from %s", len(rawData), conn.RemoteAddr().String()) + + // Strict parsing: unpack raw wire message only, same as serveUDP. + msg := new(dns.Msg) + msg.Data = rawData + if err := msg.Unpack(); err != nil { + s.invalidMsg(msg, err) + continue + } + + // Same accept/reject policy as serveUDP (see acceptMsg). + switch action := s.acceptMsg(msg); action { + case dns.MsgIgnore: + continue + case dns.MsgReject, dns.MsgRejectNotImplemented: + msg.Rcode = dns.RcodeFormatError + if action == dns.MsgRejectNotImplemented { + msg.Rcode = dns.RcodeNotImplemented + } + msg.Response = true + msg.Authoritative = false + msg.Zero = false + msg.Reset() + if err := msg.Pack(); err != nil { + s.logger.Errorf("TCP: failed to pack reject response for %s: %v", conn.RemoteAddr().String(), err) + continue + } + if err := writeTCPMsg(conn, msg.Data); err != nil { + s.logger.Errorf("TCP: failed to write reject response to %s: %v", conn.RemoteAddr().String(), err) + } + continue + } + + s.logger.Debugf("After unpacking: Answer=%d, Ns=%d, Extra=%d, Pseudo=%d", + len(msg.Answer), len(msg.Ns), len(msg.Extra), len(msg.Pseudo)) + + w := &tcpResponseWriter{conn: conn} + handler(ctx, w, msg) + + // The first read on a fresh connection uses the read timeout; any + // further pipelined queries on the same connection use the longer + // idle timeout, matching the library defaults this replaces. + readTimeout = tcpIdleTimeout + } +} + +// isClosedOrTimeout reports whether err is an expected/quiet reason for a +// TCP read loop to stop (peer closed, listener closed during shutdown, or +// the read/idle deadline simply elapsed) as opposed to a real error worth +// logging. +func isClosedOrTimeout(err error) bool { + if errors.Is(err, io.EOF) || errors.Is(err, net.ErrClosed) { + return true + } + var netErr net.Error + return errors.As(err, &netErr) && netErr.Timeout() +} - // Check if ListenAndServe returned an error due to server shutdown - select { - case <-ctx.Done(): - s.logger.Infof("TCP listener shut down cleanly") - return nil - default: +// readTCPMsg reads one length-prefixed DNS message from conn. +func readTCPMsg(conn net.Conn) ([]byte, error) { + lenBuf := make([]byte, 2) + if _, err := io.ReadFull(conn, lenBuf); err != nil { + return nil, err + } + buf := make([]byte, binary.BigEndian.Uint16(lenBuf)) + if _, err := io.ReadFull(conn, buf); err != nil { + return nil, err + } + return buf, nil +} + +// writeTCPMsg writes one length-prefixed DNS message to conn. +func writeTCPMsg(conn net.Conn, data []byte) error { + lenBuf := make([]byte, 2) + binary.BigEndian.PutUint16(lenBuf, uint16(len(data))) + if _, err := conn.Write(lenBuf); err != nil { return err } + _, err := conn.Write(data) + return err } +// tcpResponseWriter implements dns.ResponseWriter for TCP. +type tcpResponseWriter struct { + conn *net.TCPConn +} + +// Write must stay a dumb passthrough -- do not add a length prefix here. +// handleRequest sends responses via resp.WriteTo(w), and the library's own +// Msg.WriteTo already adds the required 2-byte length prefix itself before +// calling w.Write, for any ResponseWriter whose Conn() isn't a *net.UDPConn +// (see codeberg.org/miekg/dns's msg.go, WriteTo). Framing again here would +// double-prefix every response and corrupt it. +func (w *tcpResponseWriter) Write(data []byte) (int, error) { + return w.conn.Write(data) +} + +func (w *tcpResponseWriter) LocalAddr() net.Addr { return w.conn.LocalAddr() } +func (w *tcpResponseWriter) RemoteAddr() net.Addr { return w.conn.RemoteAddr() } +func (w *tcpResponseWriter) Conn() net.Conn { return w.conn } +func (w *tcpResponseWriter) Close() error { return nil } // conn lifetime is owned by serveTCPConn +func (w *tcpResponseWriter) Session() *dns.Session { return nil } +func (w *tcpResponseWriter) Hijack() {} + // handleRequest is the main request handler that routes based on opcode. func (s *Server) handleRequest(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) { s.logger.Debugf("handleRequest: Received DNS message from %s", w.RemoteAddr().String()) diff --git a/server/transport_equivalence_test.go b/server/transport_equivalence_test.go index f344691..1ed2bdd 100644 --- a/server/transport_equivalence_test.go +++ b/server/transport_equivalence_test.go @@ -11,6 +11,9 @@ import ( "github.com/NetworkCommons/sig0lease/client" "github.com/NetworkCommons/sig0lease/config" "github.com/NetworkCommons/sig0lease/logging" + _ "github.com/NetworkCommons/sig0lease/pkg/dnscompat" // registers EDNS code 2 so UPDATE-LEASE unpacks; see README_proxy.md + "github.com/NetworkCommons/sig0lease/pkg/dnsmsg" + "github.com/NetworkCommons/sig0lease/pkg/lease" ) // reserveAddr grabs an OS-assigned free port on 127.0.0.1 for the given @@ -219,3 +222,68 @@ func TestUDPTCPTransportEquivalence(t *testing.T) { }) }) } + +// TestServeTCPPreservesUpdateLeaseOption is a regression test for the bug +// serveTCP's custom accept loop fixes: the dns library's own +// (*dns.Server).serveDNS (used by ListenAndServe, which serveTCP previously +// delegated to) sets Options = MsgOptionUnpack to request a full unpack +// after MsgAcceptFunc runs, but never actually calls Unpack() a second time +// before invoking the handler -- so Ns/Extra/Pseudo stayed empty for every +// TCP-received message, and the UPDATE-LEASE EDNS option (or any other EDNS +// option) silently vanished before it ever reached a handler, regardless of +// what the client sent. TestUDPTCPTransportEquivalence above doesn't catch +// this: none of its cases populate Ns/Extra/Pseudo, since acceptMsg only +// looks at the header and Question. This test asserts the option actually +// survives, identically, on both transports. +func TestServeTCPPreservesUpdateLeaseOption(t *testing.T) { + udpAddr := reserveAddr(t, "udp") + tcpAddr := reserveAddr(t, "tcp") + + logger := logging.NewLogger("debug") + udpSrv := &Server{cfg: &config.Config{Server: config.ServerConfig{Address: udpAddr}}, logger: logger} + tcpSrv := &Server{cfg: &config.Config{Server: config.ServerConfig{Address: tcpAddr}}, logger: logger} + + var gotLeaseOpt bool + handler := dns.HandlerFunc(func(ctx context.Context, w dns.ResponseWriter, r *dns.Msg) { + _, gotLeaseOpt = lease.FindOption(r) + resp := &dns.Msg{MsgHeader: r.MsgHeader, Question: r.Question} + resp.Response = true + resp.Rcode = dns.RcodeSuccess + resp.WriteTo(w) + }) + + ctx, cancel := context.WithCancel(context.Background()) + t.Cleanup(cancel) + go func() { _ = udpSrv.serveUDP(ctx, handler) }() + go func() { _ = tcpSrv.serveTCP(ctx, handler) }() + + waitReady(t, udpAddr, "udp") + waitReady(t, tcpAddr, "tcp") + + for _, proto := range []string{"udp", "tcp"} { + t.Run(proto, func(t *testing.T) { + gotLeaseOpt = false + + msg, err := dnsmsg.NewLeaseUpdate("lease-test.example.", nil, nil, 0, 0) + if err != nil { + t.Fatalf("build lease update: %v", err) + } + + addr := udpAddr + if proto == "tcp" { + addr = tcpAddr + } + c := client.New(addr, proto, 2*time.Second) + resp, err := c.Query(msg) + if err != nil { + t.Fatalf("query failed: %v", err) + } + if resp.Rcode != dns.RcodeSuccess { + t.Fatalf("got rcode=%d, want success", resp.Rcode) + } + if !gotLeaseOpt { + t.Fatalf("handler did not see the UPDATE-LEASE EDNS option -- it was lost in transit") + } + }) + } +} diff --git a/tests/test_update.sh b/tests/test_update.sh index 75a5679..a2f3cfe 100755 --- a/tests/test_update.sh +++ b/tests/test_update.sh @@ -110,6 +110,10 @@ build_binaries() { # Accepts zero or more trailing rr-specs and/or a --signer= flag, passed # through to the client binary as separate argv entries (no eval/string # concatenation, so rr-specs containing spaces/quotes are safe). +# Transport is controlled by PROXY_PROTOCOL (see utils.sh; defaults to udp, +# same udp-unless-told-otherwise mechanism as PROXY_ADDR/PROXY_PORT) -- +# every call site gets --tcp for free when PROXY_PROTOCOL=tcp, no call site +# needs to pass it itself. run_client() { local operation="$1" local keyname="$2" @@ -117,6 +121,9 @@ run_client() { local key_lease_seconds="$4" shift 4 local extra=("$@") + if [ "$PROXY_PROTOCOL" = "tcp" ]; then + extra+=(--tcp) + fi log_file run_client "operation=$operation keyname=$keyname lease=$lease_seconds key_lease=$key_lease_seconds extra=${extra[*]:-}" echo "CLIENT_KEYSTORE_DIR=\"$CLIENT_KEYSTORE_DIR\" \"$CLIENT_BIN\" \"$PROXY_URL\" $operation \"$keyname\" $lease_seconds $key_lease_seconds ${extra[*]:-}" diff --git a/tests/utils.sh b/tests/utils.sh index e6269af..9306641 100644 --- a/tests/utils.sh +++ b/tests/utils.sh @@ -12,6 +12,10 @@ CLIENT_LOG_FILE="/tmp/sig0lease_client.log" PROXY_ADDR="${PROXY_ADDR:-127.0.0.1}" PROXY_PORT="${PROXY_PORT:-8053}" PROXY_URL="$PROXY_ADDR:$PROXY_PORT" +# Transport run_client (tests/test_update.sh) uses to reach PROXY_URL. +# Defaults to udp; set PROXY_PROTOCOL=tcp to run the whole suite over TCP +# instead (passes --tcp through to sig0lease-client). +PROXY_PROTOCOL="${PROXY_PROTOCOL:-udp}" # Configuration TMP_CONFIG_FILE=""