sneak/dnswatcher
1
0
Fork 0
Code Issues 37 Pull Requests 8 Actions Packages Projects Releases Wiki Activity

Nameserver glue/IP changes are never detected — README claims they trigger a notification #105

New Issue
Open
opened 2026-08-09 03:38:55 +02:00 by clawbot · 1 comment
clawbot commented 2026-08-09 03:38:55 +02:00
Collaborator
Copy Link

The README lists, under DNS Domain Monitoring, two events that trigger a notification (README.md:30-32):

> - NS added to or removed from the delegation.
> - NS IP address changed (glue record change).

The first works. The second is not implemented at all — not partially, not noisily, not at all.

Current state (audited against origin/main, commit 9347a28)

detectNSChanges (internal/watcher/watcher.go:272-311) diffs only the sorted list of nameserver hostnames returned by LookupNS / FindAuthoritativeNameservers. There is nowhere to put an IP even if it wanted to: state.DomainState (internal/state/state.go:38-41) holds exactly

Nameservers []string
LastChecked time.Time

There is no field for per-nameserver resolved addresses, and no code path anywhere diffs nameserver IPs.

So a delegation where ns1.example.com silently repoints from one IP to another — which is precisely the hijack/misconfiguration signal a DNS monitor should scream about, and which is invisible to anyone only watching NS names — produces no notification of any kind.

Definition of done

  1. state.DomainState gains storage for each nameserver's resolved IP addresses, keyed by nameserver hostname. Sort the addresses so the comparison is stable and the persisted JSON is deterministic.
  2. The domain check resolves each authoritative nameserver's A/AAAA addresses and records them. Reuse the existing resolver capability rather than adding a new resolution path — ResolveIPAddresses and the existing NS-host-to-IP logic used by QueryNameserver already exist; do not hand-roll a third resolver.
  3. A notification fires when a nameserver's IP set changes while its hostname stays the same. It names the domain, the nameserver hostname, and the old and new address sets.
  4. Adding or removing a nameserver still produces the existing "NS Change" notification and must not also emit a spurious glue-change notification for the nameservers that merely appeared or disappeared. Test this explicitly — it is the obvious way to get this wrong.
  5. First-run establishes the baseline silently with no notification, consistent with every other check in this codebase.
  6. State written by an older version (which has no glue field) loads without error and without emitting a false "glue changed" notification on the first check after upgrade. internal/state/state.go:64-101 already contains a backward-compatible PortState unmarshaling shim — follow that established precedent. A test must cover loading a pre-change state file.
  7. The README "State File Format" example (README.md:307-363) is updated to show the new field, in the same commit.
  8. make check is green and make test wall time stays well under the 20-second policy ceiling — note this change adds nameserver address resolution to the domain check path, so watch that the watcher tests do not grow slower. TODO.md is updated in the same commit.

The finishing commit's title must end with (closes #N) referencing this issue.

Hard constraints

  • DNS is never mocked in this repository. Do not introduce a mock, fake, or stub DNS client, resolver transport, or nameserver. Watcher-level tests may continue to use the existing resolver-interface fakes in internal/watcher/interfaces.go; that seam sits above DNS and is the established pattern.
  • Do not modify .golangci.yml, and do not change the golangci-lint version pin.

Out of scope

Per-nameserver query status / NS failure / NS recovery is tracked in #104. That issue also touches internal/state and the watcher's detection functions, so expect to rebase onto it — coordinate rather than duplicating its changes.

The README lists, under DNS Domain Monitoring, two events that trigger a notification (`README.md:30-32`): > - NS added to or removed from the delegation. > - **NS IP address changed (glue record change).** The first works. The second is **not implemented at all** — not partially, not noisily, not at all. ## Current state (audited against `origin/main`, commit `9347a28`) `detectNSChanges` (`internal/watcher/watcher.go:272-311`) diffs only the sorted list of nameserver **hostnames** returned by `LookupNS` / `FindAuthoritativeNameservers`. There is nowhere to put an IP even if it wanted to: `state.DomainState` (`internal/state/state.go:38-41`) holds exactly ```go Nameservers []string LastChecked time.Time ``` There is no field for per-nameserver resolved addresses, and no code path anywhere diffs nameserver IPs. So a delegation where `ns1.example.com` silently repoints from one IP to another — which is precisely the hijack/misconfiguration signal a DNS monitor should scream about, and which is invisible to anyone only watching NS *names* — produces **no notification of any kind**. ## Definition of done 1. `state.DomainState` gains storage for each nameserver's resolved IP addresses, keyed by nameserver hostname. Sort the addresses so the comparison is stable and the persisted JSON is deterministic. 2. The domain check resolves each authoritative nameserver's A/AAAA addresses and records them. Reuse the existing resolver capability rather than adding a new resolution path — `ResolveIPAddresses` and the existing NS-host-to-IP logic used by `QueryNameserver` already exist; do not hand-roll a third resolver. 3. A notification fires when a nameserver's IP set changes while its hostname stays the same. It names the domain, the nameserver hostname, and the old and new address sets. 4. Adding or removing a nameserver still produces the existing "NS Change" notification and must **not** also emit a spurious glue-change notification for the nameservers that merely appeared or disappeared. Test this explicitly — it is the obvious way to get this wrong. 5. First-run establishes the baseline silently with no notification, consistent with every other check in this codebase. 6. State written by an older version (which has no glue field) loads without error and without emitting a false "glue changed" notification on the first check after upgrade. `internal/state/state.go:64-101` already contains a backward-compatible `PortState` unmarshaling shim — follow that established precedent. A test must cover loading a pre-change state file. 7. The README "State File Format" example (`README.md:307-363`) is updated to show the new field, in the same commit. 8. `make check` is green and `make test` wall time stays well under the 20-second policy ceiling — note this change adds nameserver address resolution to the domain check path, so watch that the watcher tests do not grow slower. `TODO.md` is updated in the same commit. The finishing commit's title must end with ` (closes #N)` referencing this issue. ## Hard constraints - **DNS is never mocked in this repository.** Do not introduce a mock, fake, or stub DNS client, resolver transport, or nameserver. Watcher-level tests may continue to use the existing resolver-*interface* fakes in `internal/watcher/interfaces.go`; that seam sits above DNS and is the established pattern. - Do not modify `.golangci.yml`, and do not change the golangci-lint version pin. ## Out of scope Per-nameserver query status / NS failure / NS recovery is tracked in #104. That issue also touches `internal/state` and the watcher's detection functions, so **expect to rebase onto it** — coordinate rather than duplicating its changes.
clawbot added this to the 1.0 milestone 2026-08-09 03:38:55 +02:00
clawbot commented 2026-08-09 06:55:51 +02:00
Author
Collaborator
Copy Link

[manager] CORRECTION to the "Hard constraints" section — read this before starting.

I wrote that "watcher-level tests may continue to use the existing resolver-interface fakes in internal/watcher/interfaces.go; that seam sits above DNS and is the established pattern." That was true of main and is wrong for the state this issue will be implemented against.

PR #97 is merge-ready and awaiting @sneak, and it deletes mockResolver outright. Verified: origin/main's internal/watcher/watcher_test.go has 6 references to it; PR #97's head (a535ae8) has zero and instead builds the real iterative resolver via resolver.NewFromLogger(log). #97 also removes resolver.NewFromLoggerWithClient and deletes the TESTING.md carve-out that used to permit DNS mocks in packages consuming the resolver. The live-DNS policy now applies to every package.

Following my original wording would reinstate precisely what #97 removed.

The pattern to follow instead

Same as the correction on #104: seed the state store with a synthetic previous observation live DNS cannot match, then run against live DNS and assert the transition. #97 uses reserved .invalid nameserver names and RFC 5737 documentation addresses (192.0.2.0/24, 203.0.113.0/24) for that synthetic prior state.

This issue is a particularly good fit for that approach, because a glue change is by definition "the NS hostname stayed the same, the address set moved":

  • Record a domain's prior state with the real, live nameserver hostnames but with their addresses set to RFC 5737 documentation IPs. The next live run resolves the true addresses and must emit exactly one glue-change notification per affected nameserver, naming the old and new address sets.
  • For DoD item 4 (adding/removing a nameserver must not also emit a spurious glue notification), seed a prior state containing a .invalid nameserver alongside the real ones — the .invalid one exercises the add/remove path while the real ones stay stable.
  • For DoD item 6 (old state files without the glue field load without a false positive), the fixture is a hand-written JSON state file with no glue field. That test needs no DNS at all and belongs in internal/state.

Keep assertions tolerant. Live DNS may return multiple A records, AAAA records, or a set that changes between runs — assert that a glue-change notification fired and that it names the right nameserver, not that the new address set equals a hardcoded list. #97 generalised its assertions for exactly this reason.

Consequences

  1. Do not add, restore, or recreate mockResolver, NewFromLoggerWithClient, or any fake/stub DNS client, resolver, transport, or nameserver. DNS is never mocked in this repository, full stop. The port checker, TLS checker, and notifier remain legitimate test doubles — they are not DNS.
  2. Implement on top of #97, not current main.
  3. This issue adds nameserver address resolution to the domain-check path, on top of a watcher suite that #97 already moved to live DNS. That is a real wall-time risk against the 20-second make test ceiling. Measure before and after and report both numbers in the PR.

Still blocked on #97 merging, and additionally sequenced behind #104, which changes the same state and detection code. I am not dispatching an implementer for this yet.

**[manager] CORRECTION to the "Hard constraints" section — read this before starting.** I wrote that "watcher-level tests may continue to use the existing resolver-*interface* fakes in `internal/watcher/interfaces.go`; that seam sits above DNS and is the established pattern." **That was true of `main` and is wrong for the state this issue will be implemented against.** [PR #97](https://git.eeqj.de/sneak/dnswatcher/pulls/97) is `merge-ready` and awaiting @sneak, and it **deletes `mockResolver` outright.** Verified: `origin/main`'s `internal/watcher/watcher_test.go` has 6 references to it; PR #97's head (`a535ae8`) has zero and instead builds the real iterative resolver via `resolver.NewFromLogger(log)`. #97 also removes `resolver.NewFromLoggerWithClient` and deletes the `TESTING.md` carve-out that used to permit DNS mocks in packages consuming the resolver. **The live-DNS policy now applies to every package.** Following my original wording would reinstate precisely what #97 removed. ## The pattern to follow instead Same as the correction on #104: **seed the state store with a synthetic previous observation live DNS cannot match, then run against live DNS and assert the transition.** #97 uses reserved `.invalid` nameserver names and RFC 5737 documentation addresses (`192.0.2.0/24`, `203.0.113.0/24`) for that synthetic prior state. This issue is a particularly good fit for that approach, because a glue change is by definition "the NS hostname stayed the same, the address set moved": - Record a domain's prior state with the **real, live** nameserver hostnames but with their addresses set to RFC 5737 documentation IPs. The next live run resolves the true addresses and must emit exactly one glue-change notification per affected nameserver, naming the old and new address sets. - For DoD item 4 (adding/removing a nameserver must **not** also emit a spurious glue notification), seed a prior state containing a `.invalid` nameserver alongside the real ones — the `.invalid` one exercises the add/remove path while the real ones stay stable. - For DoD item 6 (old state files without the glue field load without a false positive), the fixture is a hand-written JSON state file with no glue field. That test needs no DNS at all and belongs in `internal/state`. Keep assertions tolerant. Live DNS may return multiple A records, AAAA records, or a set that changes between runs — assert that a glue-change notification fired and that it names the right nameserver, **not** that the new address set equals a hardcoded list. #97 generalised its assertions for exactly this reason. ## Consequences 1. **Do not add, restore, or recreate `mockResolver`, `NewFromLoggerWithClient`, or any fake/stub DNS client, resolver, transport, or nameserver.** DNS is never mocked in this repository, full stop. The port checker, TLS checker, and notifier remain legitimate test doubles — they are not DNS. 2. **Implement on top of #97, not current `main`.** 3. This issue adds nameserver address resolution to the domain-check path, on top of a watcher suite that #97 already moved to live DNS. That is a real wall-time risk against the 20-second `make test` ceiling. Measure before and after and report both numbers in the PR. **Still blocked on #97 merging**, and additionally sequenced behind #104, which changes the same state and detection code. I am not dispatching an implementer for this yet.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
merge-ready
All checks pass, reviewed, rebased, ready for merge
 needs-checks
make check does not pass cleanly
 needs-rebase
PR has merge conflicts or is behind main
 needs-review
Code review not yet done
 needs-rework
Code review found issues to fix
No Label
Milestone
No items
No Milestone 1.0
Projects
Clear projects
No project
Assignees
Clear assignees
clawbot sneak
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: sneak/dnswatcher#105
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?