From bb78863d33f3198cf7e6ffe894652287d7e8b52d Mon Sep 17 00:00:00 2001 From: Cuong Manh Le Date: Thu, 1 Oct 2026 15:37:42 +0700 Subject: [PATCH] fix(cli): mark AD lookups on the bypass and OS retry paths The previous change marked a rule-matched AD query as a LAN query, but two paths still reach the OS resolver without that mark, so osResolver raced the domain controller against 76.76.2.0 in plaintext: - the DNS-intercept recovery bypass, which forwards queries before rule handling runs - the OS-resolver retry catch-all after an explicit upstream fails with leak_on_upstream_failure enabled Mark a query under the detected AD domain on both paths. Other names keep the public fallback, and an explicit upstream is still tried first. Build resolvers in proxy() through newResolverFn, so tests drive proxy() and assert the LAN mark that reaches each resolver, instead of testing the predicate alone. --- cmd/cli/ad_os_lookup_test.go | 146 +++++++++++++++++++++++++++++++++++ cmd/cli/dns_proxy.go | 28 ++++++- cmd/cli/prog.go | 1 + docs/internal-domains.md | 5 +- 4 files changed, 175 insertions(+), 5 deletions(-) diff --git a/cmd/cli/ad_os_lookup_test.go b/cmd/cli/ad_os_lookup_test.go index ea1c145..8152896 100644 --- a/cmd/cli/ad_os_lookup_test.go +++ b/cmd/cli/ad_os_lookup_test.go @@ -1,9 +1,16 @@ package cli import ( + "context" + "errors" + "slices" + "strings" "testing" + "time" "github.com/miekg/dns" + + "github.com/Control-D-Inc/ctrld" ) // The auto-added AD split rules route to upstream.os, and a rule match skips @@ -37,3 +44,142 @@ func Test_isAdOSResolverLookup(t *testing.T) { t.Error("no AD domain must match nothing") } } + +// adLookupResolver records whether each resolve carried the LAN mark, which is +// what keeps osResolver off Control D's public resolver. Only the OS resolver +// answers, so an explicit upstream fails over to it. +type adLookupResolver struct { + name string + seen *[]adLookupResolve +} + +type adLookupResolve struct { + resolver string + lan bool +} + +func (r adLookupResolver) Resolve(ctx context.Context, msg *dns.Msg) (*dns.Msg, error) { + lan, _ := ctx.Value(ctrld.LanQueryCtxKey{}).(bool) + *r.seen = append(*r.seen, adLookupResolve{r.name, lan}) + if r.name != osUpstreamConfig.Name { + return nil, errors.New("upstream unreachable") + } + answer := new(dns.Msg) + answer.SetReply(msg) + return answer, nil +} + +func newAdLookupProg(t *testing.T, leak bool) (*prog, *[]adLookupResolve) { + t.Helper() + cfg := &ctrld.Config{Upstream: map[string]*ctrld.UpstreamConfig{ + "0": {Name: "explicit resolver", Type: ctrld.ResolverTypeLegacy, Endpoint: "192.0.2.53:53", Timeout: 100}, + }} + cfg.Service.LeakOnUpstreamFailure = &leak + p := &prog{cfg: cfg, adDomain: "corp.lab", um: newUpstreamMonitor(cfg)} + p.um.after = func(time.Duration, func()) {} + p.lanLoopGuard = newLoopGuard() + p.ptrLoopGuard = newLoopGuard() + t.Cleanup(func() { p.querySampler.closeExpired(time.Now().Add(querySampleWindow)) }) + + var seen []adLookupResolve + origNewResolver, origRecovery := newResolverFn, queryRecoveryFn + t.Cleanup(func() { newResolverFn, queryRecoveryFn = origNewResolver, origRecovery }) + newResolverFn = func(uc *ctrld.UpstreamConfig) (ctrld.Resolver, error) { + return adLookupResolver{name: uc.Name, seen: &seen}, nil + } + queryRecoveryFn = func(*prog, RecoveryReason) {} + return p, &seen +} + +func adLookupQuery(p *prog, name string, ufr *upstreamForResult) *proxyResponse { + msg := new(dns.Msg) + qtype := dns.TypeA + if strings.HasPrefix(name, "_") { + qtype = dns.TypeSRV + } + msg.SetQuestion(name, qtype) + ufr.srcAddr = "192.168.0.1:1234" + return p.proxy(context.Background(), &proxyRequest{msg: msg, ufr: ufr}) +} + +func wantAdLookupResolves(t *testing.T, got []adLookupResolve, want ...adLookupResolve) { + t.Helper() + if !slices.Equal(got, want) { + t.Fatalf("resolves = %+v, want %+v", got, want) + } +} + +// Drive proxy() itself, so the test fails if a path stops marking the query. +func Test_proxyKeepsAdLookupsOffThePublicResolver(t *testing.T) { + captureDebugMainLog(t) + osName := osUpstreamConfig.Name + for _, tc := range []struct { + name string + qname string + ufr *upstreamForResult + leak bool + want []adLookupResolve + }{ + { + // The auto-added AD rule has no targets, so proxy() routes it to the OS resolver. + name: "auto-added AD rule", + qname: "dc01.corp.lab.", + ufr: &upstreamForResult{matched: true}, + want: []adLookupResolve{{osName, true}}, + }, + { + name: "DC locator SRV through an AD rule", + qname: "_ldap._tcp.dc._msdcs.corp.lab.", + ufr: &upstreamForResult{matched: true}, + want: []adLookupResolve{{osName, true}}, + }, + { + name: "OS rule for a public name keeps the public fallback", + qname: "example.com.", + ufr: &upstreamForResult{matched: true}, + want: []adLookupResolve{{osName, false}}, + }, + { + name: "explicit rule for an AD name falls back to the OS resolver", + qname: "dc01.corp.lab.", + ufr: &upstreamForResult{matched: true, upstreams: []string{upstreamPrefix + "0"}}, + leak: true, + want: []adLookupResolve{{"explicit resolver", false}, {osName, true}}, + }, + { + name: "explicit rule for a public name falls back with the public resolver", + qname: "example.com.", + ufr: &upstreamForResult{matched: true, upstreams: []string{upstreamPrefix + "0"}}, + leak: true, + want: []adLookupResolve{{"explicit resolver", false}, {osName, false}}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + p, seen := newAdLookupProg(t, tc.leak) + if res := adLookupQuery(p, tc.qname, tc.ufr); res == nil || res.answer == nil { + t.Fatal("the proxy returned no answer") + } + wantAdLookupResolves(t, *seen, tc.want...) + }) + } +} + +// The DNS-intercept recovery bypass forwards queries before any rule handling. +func Test_proxyRecoveryBypassKeepsAdLookupsOffThePublicResolver(t *testing.T) { + captureDebugMainLog(t) + useDNSIntercept(t) + osName := osUpstreamConfig.Name + for _, tc := range []struct { + qname string + lan bool + }{{"dc01.corp.lab.", true}, {"example.com.", false}} { + t.Run(tc.qname, func(t *testing.T) { + p, seen := newAdLookupProg(t, false) + p.recoveryBypass.Store(true) + if res := adLookupQuery(p, tc.qname, &upstreamForResult{upstreams: []string{upstreamPrefix + "0"}}); res == nil || res.answer == nil { + t.Fatal("the proxy returned no answer") + } + wantAdLookupResolves(t, *seen, adLookupResolve{osName, tc.lan}) + }) + } +} diff --git a/cmd/cli/dns_proxy.go b/cmd/cli/dns_proxy.go index 00527af..b9246cf 100644 --- a/cmd/cli/dns_proxy.go +++ b/cmd/cli/dns_proxy.go @@ -585,9 +585,17 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { // may use. if dnsIntercept && p.recoveryBypass.Load() && !p.internalDomainExplicitUpstreams(req.ufr.upstreams) { ctrld.Log(ctx, mainLog.Load().Debug(), "Recovery bypass active: forwarding to OS resolver") - resolver, err := ctrld.NewResolver(osUpstreamConfig) + // The bypass runs before rule handling, so an Active Directory name + // needs its LAN mark here too, or it races the domain controller + // against Control D's public resolver in plaintext. + bypassCtx := ctx + if p.isAdDomainLookup(req.msg) { + bypassCtx = ctrld.LanQueryCtx(ctx) + ctrld.Log(ctx, mainLog.Load().Debug(), "active directory domain lookup, skipping public DNS") + } + resolver, err := newResolverFn(osUpstreamConfig) if err == nil { - resolveCtx, cancel := osUpstreamConfig.Context(ctx) + resolveCtx, cancel := osUpstreamConfig.Context(bypassCtx) defer cancel() answer, _ := resolver.Resolve(resolveCtx, req.msg) if answer != nil { @@ -855,7 +863,7 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { resolve1 := func(upstream string, upstreamConfig *ctrld.UpstreamConfig, msg *dns.Msg) (*dns.Msg, error) { ctrld.Log(ctx, mainLog.Load().Debug(), "sending query to %s: %s", upstream, upstreamConfig.Name) - dnsResolver, err := ctrld.NewResolver(upstreamConfig) + dnsResolver, err := newResolverFn(upstreamConfig) if err != nil { ctrld.Log(ctx, mainLog.Load().Error().Err(err), "failed to create resolver") return nil, err @@ -1082,6 +1090,12 @@ func (p *prog) proxy(ctx context.Context, req *proxyRequest) *proxyResponse { // we dont want this to happen if leakOnUpstreamFailure is false if upstreams[0] != upstreamOS { ctrld.Log(ctx, mainLog.Load().Debug(), "attempting query to OS resolver as a retry catch all") + // An Active Directory name that falls back from its explicit + // upstream still must not reach Control D's public resolver. + if p.isAdDomainLookup(req.msg) { + ctx = ctrld.LanQueryCtx(ctx) + ctrld.Log(ctx, mainLog.Load().Debug(), "active directory domain lookup, skipping public DNS") + } answer := resolve(upstreamOS, osUpstreamConfig, req.msg) if answer != nil { ctrld.Log(ctx, mainLog.Load().Debug(), "OS resolver retry query successful") @@ -2801,5 +2815,11 @@ func ValidateDefaultLocalIPsFromDelta(newState *netmon.State) { // Active Directory domain and goes to the OS resolver alone. Such a query is a // LAN lookup; osResolver must not race it against Control D's public resolver. func (p *prog) isAdOSResolverLookup(upstreams []string, msg *dns.Msg) bool { - return len(msg.Question) > 0 && len(upstreams) == 1 && upstreams[0] == upstreamOS && p.isAdDomainQuery(msg) + return len(upstreams) == 1 && upstreams[0] == upstreamOS && p.isAdDomainLookup(msg) +} + +// isAdDomainLookup reports whether msg asks for a name under the machine's +// Active Directory domain. A public answer for such a name is never correct. +func (p *prog) isAdDomainLookup(msg *dns.Msg) bool { + return len(msg.Question) > 0 && p.isAdDomainQuery(msg) } diff --git a/cmd/cli/prog.go b/cmd/cli/prog.go index 09c3809..a64941a 100644 --- a/cmd/cli/prog.go +++ b/cmd/cli/prog.go @@ -1439,6 +1439,7 @@ var ( ensureInterceptDNSTargetFn = (*prog).ensureInterceptDNSTarget removeInterceptDNSTargetFn = (*prog).removeInterceptDNSTarget initializeOsResolverWithSystemNameserversFn = ctrld.InitializeOsResolverWithSystemNameserversReason + newResolverFn = ctrld.NewResolver setDnsForRunningIfaceFn = (*prog).setDnsForRunningIface resetDNSFn = (*prog).resetDNS // refuseFallbackFatal reports a startup failure the interface-DNS fallback diff --git a/docs/internal-domains.md b/docs/internal-domains.md index dfa1f3f..492f1ad 100644 --- a/docs/internal-domains.md +++ b/docs/internal-domains.md @@ -112,7 +112,10 @@ exists for a source is never overwritten. From highest precedence to lowest: (DC hostnames, DC-locator SRV names that carry the machine name, WPAD) also went to 76.76.2.0 in plaintext; when both failed, the public NXDOMAIN could be the answer returned. An explicit-resolver rule for the AD domain is - unchanged. + unchanged. The same LAN mark applies wherever an AD name reaches the OS + resolver outside rule handling: the DNS-intercept recovery bypass, and the + OS-resolver retry after an explicit upstream fails with + `leak_on_upstream_failure` enabled. **VPN DNS auto-detection** is evaluated inside `proxy()` in DNS-intercept mode, after policy matching and independently of it, so it needs its own rule: