egress: tighten DNS answer filtering and TTL clamping#71
Merged
Conversation
Introduces a small internal helper that lower-cases and strips a single trailing dot. DNS names are case-insensitive per RFC 1035 and wire-format names typically carry a trailing dot that caller-supplied policy names typically do not. No call sites yet — the next commit wires it into the Answer-name filter inside ParseDNSResponse. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Previously, ParseDNSResponse collected every A record in the Answer section and associated them with the question name. An attacker controlling an allowed zone could return additional A records with different owner names — for internal IPs, cloud metadata endpoints, or any IP — and those would be installed into dynamic egress rules under the question's qname, silently broadening the allowlist. Collect only A records whose owner name is reachable from qname via the response's own CNAME chain. A multi-pass walk bounded by the Answer count handles legitimate CNAME chains while rejecting injected out-of-bailiwick records and detecting CNAME loops without infinite iteration. Also normalize the returned qname (lower-cased, trailing-dot stripped) via normalizeDNSName so downstream policy matching sees a canonical form; the policy matcher was already normalizing internally so behavior is preserved. Tests cover: legitimate multi-A response, out-of-bailiwick A record dropped, CNAME chain followed, and mixed response with both valid CNAME-A and injected unrelated-A dropped. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The existing minTTL clamp prevents rule churn from very short DNS TTLs. Add a symmetric maxTTL clamp (default 5 min) so a very long advertised TTL does not leave a dynamically-allowed IP in the rule set for hours or days. Bounds exposure if an upstream zone returns rogue long-lived answers, and keeps the working set of dynamic rules pegged to recent resolutions. Exposed via functional options (WithMinTTL, WithMaxTTL) on NewDNSInterceptor. The constructor signature gains a variadic opts parameter — backwards compatible for existing callers. WithMaxTTL(0) disables the cap; WithMinTTL(0) preserves the default. Test uses a 5ms max to keep the clamp provable in a real-time unit test without making the suite slow. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Phase 3 of the hardening series, scoped to
net/egress/. Twocorrectness improvements to the DNS interceptor that populates dynamic
firewall rules from snooped DNS responses:
Answer filter.
ParseDNSResponsepreviously collected every Arecord in the Answer section and associated it with the question
name. An allow-listed zone returning additional A records with
unrelated owner names would silently install those IPs into dynamic
egress rules. The parser now only accepts A records whose owner is
reachable from the question via the response's own CNAME chain —
preserving legitimate CDN redirects while dropping out-of-bailiwick
answers. A multi-pass walk bounded by the Answer count handles
CNAME loops without infinite iteration.
TTL cap. Existing
minTTL(60 s) kept short-TTL rules alivelong enough to avoid churn. Added a symmetric
maxTTL(5 mindefault) so a very long advertised TTL does not leave a
dynamically-allowed IP in the rule set indefinitely. Both clamps
are exposed via functional options (
WithMinTTL,WithMaxTTL)on
NewDNSInterceptor; constructor gains a variadicoptsparameter so existing callers compile unchanged.
Also adds a small internal
normalizeDNSNamehelper (lowercase +trailing-dot strip) used for both filter comparison and the returned
qname, since DNS is case-insensitive and wire-format names typically
carry a trailing dot that caller-supplied policy names do not.
Commits are bisectable; each lands with its own tests and
task verifyis green at every commit.Test plan
task verifygreen at every commitdns_test.go: out-of-bailiwick A recorddropped, legitimate CNAME chain followed, mixed response with
both valid CNAME-A and injected unrelated-A dropped
interceptor_test.go: TTL clamp verifiedwith a 5 ms
WithMaxTTL— rule goes live immediately andexpires past the clamp
brood-boxwith localreplace: VM boots,allowed hosts (
api.anthropic.com,github.com) resolvecorrectly, unknown hosts get
NXDOMAINas expected🤖 Generated with Claude Code