From 3e2a4f1736aa5a145846209f1c7d813e97d835fa Mon Sep 17 00:00:00 2001 From: "LamTrinh.Dev" Date: Fri, 4 Sep 2026 02:24:31 +0700 Subject: [PATCH 01/52] Correct name for `Mikrotik RouterOS`. --- app/src/pages/setup/guides/Routers.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/pages/setup/guides/Routers.tsx b/app/src/pages/setup/guides/Routers.tsx index 23eba321..9c787316 100644 --- a/app/src/pages/setup/guides/Routers.tsx +++ b/app/src/pages/setup/guides/Routers.tsx @@ -278,7 +278,7 @@ const StampsTab = ({ deps }: { deps: RoutersGuideDeps }) => { const buildRouterTabs = (deps: RoutersGuideDeps): RouterTabDef[] => [ { key: 'mikrotik', - label: 'Mikrotik Router OS', + label: 'Mikrotik RouterOS', content: (
Access the device’s command-line interface, and enter the following commands:} /> From 2308c5bd83e358a3f77b287762167d0a53af56ca Mon Sep 17 00:00:00 2001 From: Maciek Date: Wed, 9 Sep 2026 10:21:46 +0200 Subject: [PATCH 02/52] docs(app): add Privacy FAQ section with QNAME minimisation and EDNS Client Subnet answers Signed-off-by: Maciek --- app/src/pages/legal/FAQ.tsx | 29 +++++++++++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-) diff --git a/app/src/pages/legal/FAQ.tsx b/app/src/pages/legal/FAQ.tsx index 4f214b1e..c112ef4f 100644 --- a/app/src/pages/legal/FAQ.tsx +++ b/app/src/pages/legal/FAQ.tsx @@ -397,6 +397,20 @@ export default function FAQ(): JSX.Element {
); + const qnameMinimisation = ( +
+

Yes. When a resolver looks up a name, it walks the DNS hierarchy from the root servers down. Without QNAME minimisation it repeats the full name (for example mail.example.com) to every server on that path, so the root and top-level-domain servers learn which hosts you visit. With QNAME minimisation (RFC 9156) each server is asked only for the part it is responsible for.

+

All modDNS resolvers minimise query names. You can confirm it from any device that uses your profile by running dig TXT qnamemintest.internet.nl: the answer contains HOORAY only when the resolver actually minimised the query on the wire.

+
+ ); + + const ednsClientSubnet = ( +
+

No. EDNS Client Subnet (ECS, RFC 7871) is a mechanism by which a resolver attaches part of your IP address to the queries it sends to authoritative DNS servers, mainly so that content delivery networks can pick a server near you. It also reveals your approximate network location to every authoritative server involved in a lookup.

+

modDNS does not use ECS. Your address is never attached to upstream queries, and if your device adds an ECS option to its own queries it is discarded before the lookup leaves our resolver. Authoritative servers only see the address of the modDNS server location that handled your query. There is no setting to turn ECS on.

+
+ ); + const whatIsDNSSEC = (

DNSSEC stands for Domain Name System Security Extensions. It's a security protocol that adds digital signatures to DNS records to ensure their authenticity and integrity. This helps prevent DNS spoofing attacks, where malicious actors could redirect users to fake websites.

@@ -507,6 +521,13 @@ export default function FAQ(): JSX.Element { question="What is modDNS?" answer="modDNS is a privacy-focused DNS service that helps protect privacy and improve security by blocking ads, trackers, and malicious domains. It supports modern DNS protocols including DNS-over-HTTPS (DoH), DNS-over-TLS (DoT), and DNS-over-QUIC (DoQ)." /> + + + + + From b0f6df1a8beb325fbf329c793931a332da4d0e5c Mon Sep 17 00:00:00 2001 From: Maciek Date: Wed, 9 Sep 2026 10:30:06 +0200 Subject: [PATCH 03/52] docs(app): explain how to get modDNS in the FAQ Basics section Signed-off-by: Maciek --- app/src/pages/legal/FAQ.tsx | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/app/src/pages/legal/FAQ.tsx b/app/src/pages/legal/FAQ.tsx index c112ef4f..71d2fab4 100644 --- a/app/src/pages/legal/FAQ.tsx +++ b/app/src/pages/legal/FAQ.tsx @@ -9,6 +9,7 @@ import modDNSLogoLightTheme from '@/assets/logos/modDNS-light-theme.svg'; import { useTheme } from "@/components/theme-provider"; import AuthFooter from "@/components/auth/AuthFooter"; import { parseDnsServerLocations, firstAddress } from "@/lib/dnsServerLocations"; +import { LINKS } from "@/pages/landing/links"; interface FAQItemProps { question: string; @@ -397,6 +398,13 @@ export default function FAQ(): JSX.Element {
); + const howToGetModDNS = ( +
+

modDNS is included in the IVPN Plus and IVPN Pro Suite plans. There is no standalone modDNS subscription, and it is not part of the IVPN Standard plan. See ivpn.net/pricing for current plans.

+

Once you have an eligible IVPN plan, start modDNS from your IVPN account area. IVPN sends you to a one-time signup link where you create your modDNS login with an email and password or a passkey. Your modDNS access then follows your IVPN subscription automatically.

+
+ ); + const qnameMinimisation = (

Yes. When a resolver looks up a name, it walks the DNS hierarchy from the root servers down. Without QNAME minimisation it repeats the full name (for example mail.example.com) to every server on that path, so the root and top-level-domain servers learn which hosts you visit. With QNAME minimisation (RFC 9156) each server is asked only for the part it is responsible for.

@@ -521,6 +529,10 @@ export default function FAQ(): JSX.Element { question="What is modDNS?" answer="modDNS is a privacy-focused DNS service that helps protect privacy and improve security by blocking ads, trackers, and malicious domains. It supports modern DNS protocols including DNS-over-HTTPS (DoH), DNS-over-TLS (DoT), and DNS-over-QUIC (DoQ)." /> + Date: Wed, 9 Sep 2026 10:41:42 +0200 Subject: [PATCH 04/52] docs(app): state that QNAME minimisation is always on in the FAQ Signed-off-by: Maciek --- app/src/pages/legal/FAQ.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/pages/legal/FAQ.tsx b/app/src/pages/legal/FAQ.tsx index 71d2fab4..294eecdd 100644 --- a/app/src/pages/legal/FAQ.tsx +++ b/app/src/pages/legal/FAQ.tsx @@ -408,7 +408,7 @@ export default function FAQ(): JSX.Element { const qnameMinimisation = (

Yes. When a resolver looks up a name, it walks the DNS hierarchy from the root servers down. Without QNAME minimisation it repeats the full name (for example mail.example.com) to every server on that path, so the root and top-level-domain servers learn which hosts you visit. With QNAME minimisation (RFC 9156) each server is asked only for the part it is responsible for.

-

All modDNS resolvers minimise query names. You can confirm it from any device that uses your profile by running dig TXT qnamemintest.internet.nl: the answer contains HOORAY only when the resolver actually minimised the query on the wire.

+

modDNS applies this on every resolver location and for every profile. It is always on and there is no setting that disables it.

); From 87586f922b5011e163afefb3bd44a580a5c203e8 Mon Sep 17 00:00:00 2001 From: Maciek Date: Wed, 9 Sep 2026 10:47:09 +0200 Subject: [PATCH 05/52] docs(app): refresh the FAQ last-updated date Signed-off-by: Maciek --- app/src/pages/legal/FAQ.tsx | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/src/pages/legal/FAQ.tsx b/app/src/pages/legal/FAQ.tsx index 294eecdd..404401e8 100644 --- a/app/src/pages/legal/FAQ.tsx +++ b/app/src/pages/legal/FAQ.tsx @@ -111,7 +111,7 @@ function FAQSection({ title, children, globalToggleSignal, globalToggleState }: ); } -const FAQ_LAST_UPDATED = 'September 3, 2026'; +const FAQ_LAST_UPDATED = 'September 9, 2026'; const CODE_CLASS = "text-[var(--shadcn-ui-app-foreground)] px-2 py-0.5 rounded text-sm font-mono border border-[var(--shadcn-ui-app-border)]"; const TABLE_CELL_CLASS = "border border-[var(--shadcn-ui-app-border)] px-3 py-2 text-left align-top"; From f4e01aed9de37fed98c758ed9890a988a3a820ca Mon Sep 17 00:00:00 2001 From: Maciek Date: Wed, 9 Sep 2026 13:26:17 +0200 Subject: [PATCH 06/52] fix(app): Remove duplicate Limited Access banners on Account and Profile Settings pages Signed-off-by: Maciek --- app/src/components/AccountSubscription.tsx | 17 +---------------- .../pages/settings/ProfileManagementSection.tsx | 2 -- 2 files changed, 1 insertion(+), 18 deletions(-) diff --git a/app/src/components/AccountSubscription.tsx b/app/src/components/AccountSubscription.tsx index abc3844b..8c168a1f 100644 --- a/app/src/components/AccountSubscription.tsx +++ b/app/src/components/AccountSubscription.tsx @@ -77,7 +77,7 @@ export default function AccountSubscription() { const isInactive = sub.status === "inactive"; const isPendingDelete = sub.status === "pending_delete"; // signup-reset retired const isCutOff = isInactive || isPendingDelete; - const hasAlerts = isLimited || isCutOff || sub.outage || !!error; + const hasAlerts = sub.outage || !!error; const statusBadge = syncing ? @@ -111,21 +111,6 @@ export default function AccountSubscription() { {/* Alerts — rendered first, meant to be placed above the cards by parent */} {hasAlerts && (
- {isLimited && ( -
- -
-

- Limited Access Mode -

-

- Your modDNS account is in limited access mode. To regain full access add time to your{" "} - IVPN account. -

-
-
- )} - {sub.outage && (
diff --git a/app/src/pages/settings/ProfileManagementSection.tsx b/app/src/pages/settings/ProfileManagementSection.tsx index 541015b8..484d4a41 100644 --- a/app/src/pages/settings/ProfileManagementSection.tsx +++ b/app/src/pages/settings/ProfileManagementSection.tsx @@ -4,7 +4,6 @@ import type { ModelProfile } from "@/api/client/api"; import { ModelProfileUpdateOperationEnum, ModelProfileUpdatePathEnum } from "@/api/client/api"; import { useAppStore } from "@/store/general"; import { useSubscriptionGuard } from "@/hooks/useSubscriptionGuard"; -import LimitedAccessBanner from "@/components/LimitedAccessBanner"; import { toast } from "sonner"; import DeleteProfileDialog from "@/pages/settings/DeleteProfileDialog"; import QueryLogsSection from "./QueryLogsSection"; @@ -385,7 +384,6 @@ export default function ProfileManagementSection({ profiles }: ProfileManagement return ( <> -
{/* BLOCKLISTS + CUSTOM RULES — mutations blocked in LA */}
From a006cba1764d225233507e73455b5bd1880e3a7c Mon Sep 17 00:00:00 2001 From: Maciek Date: Fri, 4 Sep 2026 12:15:15 +0200 Subject: [PATCH 07/52] fix(dnscheck): open GeoIP reader once and degrade instead of crashing on lookup failure Signed-off-by: Maciek --- dnscheck/api/check_test.go | 129 +++++++++++ dnscheck/config/config.go | 22 +- dnscheck/config/config_test.go | 22 ++ dnscheck/dns/handler.go | 55 ++--- dnscheck/dns/handler_geolookup_test.go | 216 ++++++++++++++++++ dnscheck/dns/handler_malformed_test.go | 4 + dnscheck/dns/server.go | 15 +- dnscheck/internal/maxmind/maxmind.go | 50 ++-- dnscheck/internal/maxmind/maxmind_test.go | 124 ++++++++++ .../maxmind/testdata/GeoLite2-ASN.mmdb | Bin 0 -> 833 bytes .../maxmind/testdata/GeoLite2-City.mmdb | Bin 0 -> 493 bytes dnscheck/internal/maxmind/testdata/README.md | 12 + tests/config/dnscheck.env | 1 - tests/scripts/generate_stub_mmdb.py | 37 ++- 14 files changed, 613 insertions(+), 74 deletions(-) create mode 100644 dnscheck/api/check_test.go create mode 100644 dnscheck/config/config_test.go create mode 100644 dnscheck/dns/handler_geolookup_test.go create mode 100644 dnscheck/internal/maxmind/maxmind_test.go create mode 100644 dnscheck/internal/maxmind/testdata/GeoLite2-ASN.mmdb create mode 100644 dnscheck/internal/maxmind/testdata/GeoLite2-City.mmdb create mode 100644 dnscheck/internal/maxmind/testdata/README.md diff --git a/dnscheck/api/check_test.go b/dnscheck/api/check_test.go new file mode 100644 index 00000000..14060c05 --- /dev/null +++ b/dnscheck/api/check_test.go @@ -0,0 +1,129 @@ +package api + +import ( + "encoding/json" + "errors" + "io" + "net/http" + "net/http/httptest" + "testing" + + "github.com/dnscheck/cache" + "github.com/dnscheck/config" + "github.com/dnscheck/dns" +) + +type memCache struct { + saved map[string][]byte +} + +func (c *memCache) SaveQueryData(key string, value []byte) error { + if c.saved == nil { + c.saved = map[string][]byte{} + } + c.saved[key] = value + return nil +} + +func (c *memCache) GetQueryData(key string) ([]byte, error) { + v, ok := c.saved[key] + if !ok { + return nil, errors.New(ErrEntryNotFound) + } + return v, nil +} + +func (c *memCache) DeleteQueryData(key string) error { delete(c.saved, key); return nil } + +const ( + testHMACKey = "test-key" + testSubdomain = "abcdefghijkl-profile1" + testHost = testSubdomain + ".check.example.test" +) + +func newTestServer(c *memCache) *APIServer { + s := NewServer(&config.Config{ + API: &config.APIConfig{ApiAllowOrigin: "*"}, + Cache: &config.CacheConfig{HMACKey: testHMACKey}, + }, c) + s.RegisterRoutes() + return s +} + +func get(t *testing.T, s *APIServer, host string) (*http.Response, []byte) { + t.Helper() + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = host + resp, err := s.App.Test(req) + if err != nil { + t.Fatalf("request failed: %v", err) + } + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + return resp, body +} + +// specRef: dnscheck-behaviour.md #A1 +func TestDnsCheckRejectsHostWithoutSubdomain(t *testing.T) { + resp, _ := get(t, newTestServer(&memCache{}), "localhost") + if resp.StatusCode != http.StatusInternalServerError { + t.Errorf("status = %d, want 500", resp.StatusCode) + } +} + +// specRef: dnscheck-behaviour.md #A1 +func TestDnsCheckRejectsMalformedSubdomain(t *testing.T) { + resp, _ := get(t, newTestServer(&memCache{}), "short.check.example.test") + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("status = %d, want 400", resp.StatusCode) + } +} + +// specRef: dnscheck-behaviour.md #A2 +func TestDnsCheckReturnsDisconnectedWhenNoRecord(t *testing.T) { + resp, body := get(t, newTestServer(&memCache{}), testHost) + if resp.StatusCode != http.StatusNotFound { + t.Errorf("status = %d, want 404", resp.StatusCode) + } + var er ErrResponse + if err := json.Unmarshal(body, &er); err != nil || er.Error != StatusDisconnected { + t.Errorf("body = %s, want error=%q", body, StatusDisconnected) + } +} + +// The record is keyed by an HMAC of the subdomain, only status and profile ID +// are returned, and the entry is deleted on first read. +// +// specRef: dnscheck-behaviour.md #A3, #A4 +func TestDnsCheckReturnsNarrowRecordOnceOnly(t *testing.T) { + c := &memCache{} + rec, _ := json.Marshal(dns.DNSLogRecord{ + Status: dns.StatusConfigured, ProfileId: "profile1", + IPAddress: "203.0.113.5", ASN: 64512, ASNOrganization: "OURS", + }) + if err := c.SaveQueryData(cache.HMACKey(testHMACKey, testSubdomain), rec); err != nil { + t.Fatal(err) + } + s := newTestServer(c) + + resp, body := get(t, s, testHost) + if resp.StatusCode != http.StatusOK { + t.Fatalf("status = %d, want 200 (body %s)", resp.StatusCode, body) + } + var got map[string]any + if err := json.Unmarshal(body, &got); err != nil { + t.Fatalf("body is not JSON: %s", body) + } + if got["status"] != dns.StatusConfigured || got["profile_id"] != "profile1" { + t.Errorf("body = %s", body) + } + for _, leaked := range []string{"ip_address", "asn", "asn_organization"} { + if _, ok := got[leaked]; ok { + t.Errorf("response leaks %q: %s", leaked, body) + } + } + + if resp, _ := get(t, s, testHost); resp.StatusCode != http.StatusNotFound { + t.Errorf("second read status = %d, want 404 (delete-on-read)", resp.StatusCode) + } +} diff --git a/dnscheck/config/config.go b/dnscheck/config/config.go index 55be33a4..26762e80 100644 --- a/dnscheck/config/config.go +++ b/dnscheck/config/config.go @@ -39,21 +39,15 @@ type CacheConfig struct { HMACKey string } -// GeoLookupConfig represents access to MaxMind GeoIP database +// GeoLookupConfig represents access to the MaxMind GeoIP ASN database type GeoLookupConfig struct { - DBFile string DBASNFile string } // IsValid check whether config section is valid func (cfg *GeoLookupConfig) IsValid() error { - if cfg.DBFile == "" { - return errors.New("[GeoIP] DBFile is required") - - } if cfg.DBASNFile == "" { - return errors.New("[GeoIP] DBISP is required") - + return errors.New("GEOIP_DB_ASN_FILE environment variable is required") } return nil } @@ -77,6 +71,13 @@ func New() (*Config, error) { return nil, errors.New("CACHE_HMAC_KEY environment variable is required") } + geoLookup := &GeoLookupConfig{ + DBASNFile: os.Getenv("GEOIP_DB_ASN_FILE"), + } + if err := geoLookup.IsValid(); err != nil { + return nil, err + } + return &Config{ Server: &AuthoritativeDNSServerConfig{ Domain: os.Getenv("DNS_AUTH_SERVER_DOMAIN"), @@ -92,9 +93,6 @@ func New() (*Config, error) { TTL: ttl, HMACKey: cacheHMACKey, }, - GeoLookupConfig: &GeoLookupConfig{ - DBFile: os.Getenv("GEOIP_DB_FILE"), - DBASNFile: os.Getenv("GEOIP_DB_ASN_FILE"), - }, + GeoLookupConfig: geoLookup, }, nil } diff --git a/dnscheck/config/config_test.go b/dnscheck/config/config_test.go new file mode 100644 index 00000000..ae992c63 --- /dev/null +++ b/dnscheck/config/config_test.go @@ -0,0 +1,22 @@ +package config + +import "testing" + +// specRef: dnscheck-behaviour.md #S2 +func TestNewRequiresASNDatabasePath(t *testing.T) { + t.Setenv("CACHE_HMAC_KEY", "test-key") + t.Setenv("GEOIP_DB_ASN_FILE", "") + + if _, err := New(); err == nil { + t.Fatal("expected an error when GEOIP_DB_ASN_FILE is unset") + } + + t.Setenv("GEOIP_DB_ASN_FILE", "/opt/dnscheck/GeoLite2-ASN.mmdb") + cfg, err := New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.GeoLookupConfig.DBASNFile != "/opt/dnscheck/GeoLite2-ASN.mmdb" { + t.Errorf("DBASNFile = %q", cfg.GeoLookupConfig.DBASNFile) + } +} diff --git a/dnscheck/dns/handler.go b/dnscheck/dns/handler.go index f8ab113e..82b11981 100644 --- a/dnscheck/dns/handler.go +++ b/dnscheck/dns/handler.go @@ -2,13 +2,14 @@ package dns import ( "encoding/json" - "errors" + "fmt" "net" "regexp" "strings" "time" "github.com/dnscheck/cache" + "github.com/dnscheck/internal/maxmind" "github.com/miekg/dns" "github.com/rs/zerolog/log" ) @@ -77,22 +78,18 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { record := DNSLogRecord{} - udp := strings.HasPrefix(w.RemoteAddr().Network(), "udp") - var extractionMode string - if udp { - extractionMode = "udp" - } else { - extractionMode = "tcp" - } - IPAddress, _, err := h.extractIPAddressAndHostname(w, extractionMode) + IPAddress, err := clientIP(w.RemoteAddr()) if err != nil { - log.Warn().Err(err).Msgf("Error resolving address %s %s, defaulting to hostname None", w.RemoteAddr().Network(), w.RemoteAddr().String()) + log.Warn().Err(err).Str("remote", w.RemoteAddr().String()).Msg("Cannot determine client IP address") return } + // A failed lookup degrades to "no ASN information"; the IP-range check + // below still decides the status and the answer is still written. lookupData, err := h.srv.GeoLookup.GetGeoLookup(IPAddress) - if err != nil { - log.Error().Err(err).Msgf("Error getting GeoLookup for %s", IPAddress) + if err != nil || lookupData == nil { + log.Error().Err(err).Msg("GeoIP lookup failed, continuing without ASN") + lookupData = &maxmind.GeoLookup{IPAddress: IPAddress} } record.IPAddress = IPAddress @@ -224,33 +221,17 @@ func (h *Handler) createSOA() []dns.RR { } } -func (h *Handler) extractIPAddressAndHostname(w dns.ResponseWriter, extractionMode string) (IPAddress string, hostname string, err error) { - switch extractionMode { - case "udp": - addr, err := net.ResolveUDPAddr(w.RemoteAddr().Network(), w.RemoteAddr().String()) - if err != nil { - return "", "", err - } - IPAddress = addr.IP.String() - hostnames, err := net.LookupAddr(IPAddress) - if err == nil && len(hostnames) > 0 { - hostname = hostnames[0] - } - case "tcp": - addr, err := net.ResolveTCPAddr(w.RemoteAddr().Network(), w.RemoteAddr().String()) - if err != nil { - return "", "", err - } - IPAddress = addr.IP.String() - hostnames, err := net.LookupAddr(IPAddress) - if err == nil && len(hostnames) > 0 { - hostname = hostnames[0] - } +// clientIP returns the transport-level source address of the query. It is read +// straight from the socket address and never resolved. +func clientIP(addr net.Addr) (string, error) { + switch a := addr.(type) { + case *net.UDPAddr: + return a.IP.String(), nil + case *net.TCPAddr: + return a.IP.String(), nil default: - return "", "", errors.New("invalid extraction mode") + return "", fmt.Errorf("unsupported remote address type %T", addr) } - - return IPAddress, hostname, nil } func FindStringSubmatchMap(rs string, s string) map[string]string { diff --git a/dnscheck/dns/handler_geolookup_test.go b/dnscheck/dns/handler_geolookup_test.go new file mode 100644 index 00000000..5872a364 --- /dev/null +++ b/dnscheck/dns/handler_geolookup_test.go @@ -0,0 +1,216 @@ +package dns + +import ( + "encoding/json" + "errors" + "net" + "testing" + + "github.com/dnscheck/config" + "github.com/dnscheck/internal/maxmind" + "github.com/miekg/dns" +) + +const ( + testDomain = "check.example.test" + testSubdomain = "abcdefghijkl-profile1" + testOurASN = 64512 +) + +type fakeGeoLookup struct { + result *maxmind.GeoLookup + err error +} + +func (f *fakeGeoLookup) GetGeoLookup(ip string) (*maxmind.GeoLookup, error) { + return f.result, f.err +} + +type memCache struct { + saved map[string][]byte +} + +func (c *memCache) SaveQueryData(key string, value []byte) error { + if c.saved == nil { + c.saved = map[string][]byte{} + } + c.saved[key] = value + return nil +} +func (c *memCache) GetQueryData(key string) ([]byte, error) { return c.saved[key], nil } +func (c *memCache) DeleteQueryData(key string) error { delete(c.saved, key); return nil } + +// tcpCaptureWriter reports a TCP remote address so both transports are covered. +type tcpCaptureWriter struct{ captureWriter } + +func (w *tcpCaptureWriter) RemoteAddr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(203, 0, 113, 5), Port: 40000} +} +func (w *tcpCaptureWriter) Network() string { return "tcp" } + +func newTestHandler(geo GeoLookuper, cache *memCache) *Handler { + return &Handler{srv: &DNSServer{ + Config: &config.Config{ + Server: &config.AuthoritativeDNSServerConfig{ + Domain: testDomain, + IPAddress: "192.0.2.1", + ASN: testOurASN, + IPRange: "198.51.100.", + }, + Cache: &config.CacheConfig{HMACKey: "test-key"}, + }, + Cache: cache, + GeoLookup: geo, + }} +} + +func checkQuery() *dns.Msg { + req := new(dns.Msg) + req.SetQuestion(testSubdomain+"."+testDomain+".", dns.TypeA) + return req +} + +func savedRecord(t *testing.T, c *memCache) DNSLogRecord { + t.Helper() + if len(c.saved) != 1 { + t.Fatalf("expected exactly one saved record, got %d", len(c.saved)) + } + var rec DNSLogRecord + for _, raw := range c.saved { + if err := json.Unmarshal(raw, &rec); err != nil { + t.Fatalf("saved record is not JSON: %v", err) + } + } + return rec +} + +// A failing GeoIP lookup must degrade to "no ASN information", not crash or go +// silent: the A answer is still written and the record is saved with status +// derived from the IP range alone. +// +// specRef: dnscheck-behaviour.md #D6 +func TestServeDNSDegradesWhenGeoLookupFails(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{err: errors.New("lookup failed")}, cache) + w := &captureWriter{} + + defer func() { + if r := recover(); r != nil { + t.Fatalf("ServeDNS panicked on a failed GeoIP lookup: %v", r) + } + }() + + h.ServeDNS(w, checkQuery()) + + if w.msg == nil || len(w.msg.Answer) != 1 { + t.Fatalf("expected an A answer despite the lookup failure, got %+v", w.msg) + } + rec := savedRecord(t, cache) + if rec.Status != StatusUnconfigured { + t.Errorf("status = %q, want %q", rec.Status, StatusUnconfigured) + } + if rec.ASN != 0 || rec.ASNOrganization != "" { + t.Errorf("expected empty ASN fields, got ASN=%d org=%q", rec.ASN, rec.ASNOrganization) + } + if rec.IPAddress != "203.0.113.5" { + t.Errorf("IPAddress = %q, want 203.0.113.5", rec.IPAddress) + } +} + +// specRef: dnscheck-behaviour.md #D7 +func TestServeDNSMarksConfiguredWhenASNMatches(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{ + IPAddress: "203.0.113.5", ASN: testOurASN, ASNOrganization: "OURS", + }}, cache) + + req := checkQuery() + opt := &dns.OPT{Hdr: dns.RR_Header{Name: ".", Rrtype: dns.TypeOPT}} + opt.Option = append(opt.Option, &dns.EDNS0_LOCAL{Code: ProfileIdAdditionalSectionCode, Data: []byte("profile1")}) + req.Extra = append(req.Extra, opt) + + h.ServeDNS(&captureWriter{}, req) + + rec := savedRecord(t, cache) + if rec.Status != StatusConfigured { + t.Errorf("status = %q, want %q", rec.Status, StatusConfigured) + } + if rec.ProfileId != "profile1" { + t.Errorf("profile_id = %q, want profile1", rec.ProfileId) + } + if rec.ASN != testOurASN || rec.ASNOrganization != "OURS" { + t.Errorf("ASN fields not carried into record: %+v", rec) + } +} + +// specRef: dnscheck-behaviour.md #D8 +func TestServeDNSMarksUnconfiguredWhenNeitherASNNorRangeMatch(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{ + IPAddress: "203.0.113.5", ASN: 15169, ASNOrganization: "GOOGLE", + }}, cache) + + h.ServeDNS(&captureWriter{}, checkQuery()) + + rec := savedRecord(t, cache) + if rec.Status != StatusUnconfigured { + t.Errorf("status = %q, want %q", rec.Status, StatusUnconfigured) + } + if rec.ASN != 15169 { + t.Errorf("ASN = %d, want 15169", rec.ASN) + } +} + +// The remote address is taken from the transport as-is; it is never resolved. +// +// specRef: dnscheck-behaviour.md #D4 +func TestServeDNSExtractsClientIPOverTCP(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{}}, cache) + + h.ServeDNS(&tcpCaptureWriter{}, checkQuery()) + + rec := savedRecord(t, cache) + if rec.IPAddress != "203.0.113.5" { + t.Errorf("IPAddress = %q, want 203.0.113.5", rec.IPAddress) + } +} + +// A queries outside the check domain get the authoritative A answer and leave no +// trace: no lookup, no cache entry. +// +// specRef: dnscheck-behaviour.md #D2 +func TestServeDNSAnswersForeignDomainWithoutRecord(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{err: errors.New("must not be called")}, cache) + w := &captureWriter{} + + req := new(dns.Msg) + req.SetQuestion("www.other.example.", dns.TypeA) + h.ServeDNS(w, req) + + if w.msg == nil || len(w.msg.Answer) != 1 { + t.Fatalf("expected an A answer, got %+v", w.msg) + } + if len(cache.saved) != 0 { + t.Errorf("expected no saved record, got %d", len(cache.saved)) + } +} + +// specRef: dnscheck-behaviour.md #D3 +func TestServeDNSIgnoresMalformedSubdomain(t *testing.T) { + cache := &memCache{} + h := newTestHandler(&fakeGeoLookup{err: errors.New("must not be called")}, cache) + w := &captureWriter{} + + req := new(dns.Msg) + req.SetQuestion("short-profile1."+testDomain+".", dns.TypeA) + h.ServeDNS(w, req) + + if len(cache.saved) != 0 { + t.Errorf("expected no saved record for a malformed subdomain, got %d", len(cache.saved)) + } + if w.msg != nil { + t.Errorf("expected no response for a malformed subdomain, got %+v", w.msg) + } +} diff --git a/dnscheck/dns/handler_malformed_test.go b/dnscheck/dns/handler_malformed_test.go index edb392e0..ce09bd9d 100644 --- a/dnscheck/dns/handler_malformed_test.go +++ b/dnscheck/dns/handler_malformed_test.go @@ -35,6 +35,8 @@ func (w *captureWriter) Network() string { return "udp" } // nil Question slice. Indexing Question[0] then panics, and the handler runs in a // goroutine per packet, so the panic would terminate the process -- taking the DNS // listener and the HTTP API sharing it down together. +// +// specRef: dnscheck-behaviour.md #D1 func TestServeDNSHandlesMissingQuestionSection(t *testing.T) { // 12-byte header, QDCOUNT=1, everything else zero. Passes the library's // length and accept checks. @@ -79,6 +81,8 @@ func TestServeDNSHandlesMissingQuestionSection(t *testing.T) { } // QDCOUNT=0 with no question: same code path, pinned for completeness. +// +// specRef: dnscheck-behaviour.md #D1 func TestServeDNSHandlesZeroQuestionCount(t *testing.T) { req := new(dns.Msg) req.Id = 1234 diff --git a/dnscheck/dns/server.go b/dnscheck/dns/server.go index 230693c4..f361c1b8 100644 --- a/dnscheck/dns/server.go +++ b/dnscheck/dns/server.go @@ -1,12 +1,19 @@ package dns import ( + "fmt" + "github.com/dnscheck/cache" "github.com/dnscheck/config" "github.com/dnscheck/internal/maxmind" "github.com/miekg/dns" ) +// GeoLookuper resolves a client IP to its ASN record. +type GeoLookuper interface { + GetGeoLookup(ip string) (*maxmind.GeoLookup, error) +} + // DNSServer represents a DNS server type DNSServer struct { Config *config.Config @@ -15,7 +22,7 @@ type DNSServer struct { DNSTCP *dns.Server Cache cache.Cache - GeoLookup *maxmind.GeoLookupManager + GeoLookup GeoLookuper } // New creates a new DNS server @@ -25,7 +32,11 @@ func New(config *config.Config, cache cache.Cache) (*DNSServer, error) { Cache: cache, } - srv.GeoLookup = maxmind.NewGeoLookupManager(config.GeoLookupConfig.DBFile, config.GeoLookupConfig.DBASNFile) + geoLookup, err := maxmind.NewGeoLookupManager(config.GeoLookupConfig.DBASNFile) + if err != nil { + return nil, fmt.Errorf("geoip: %w", err) + } + srv.GeoLookup = geoLookup // DNS srv.DNSTCP = &dns.Server{Addr: ":53", Net: "tcp"} diff --git a/dnscheck/internal/maxmind/maxmind.go b/dnscheck/internal/maxmind/maxmind.go index ce6fdd31..ad2b0fb1 100644 --- a/dnscheck/internal/maxmind/maxmind.go +++ b/dnscheck/internal/maxmind/maxmind.go @@ -4,40 +4,52 @@ import ( "fmt" "net" - "github.com/rs/zerolog/log" - "github.com/oschwald/geoip2-golang" ) +// GeoLookupManager answers ASN lookups from a MaxMind database that is opened +// once and shared by every request; geoip2.Reader is safe for concurrent use. type GeoLookupManager struct { - DBFile string - DBASNFile string + asnDB *geoip2.Reader } -func NewGeoLookupManager(dbFile, dbASNFile string) *GeoLookupManager { - return &GeoLookupManager{ - DBFile: dbFile, - DBASNFile: dbASNFile, +// NewGeoLookupManager opens the ASN database and fails if the file is missing, +// unreadable or not an ASN-capable database type. +func NewGeoLookupManager(dbASNFile string) (*GeoLookupManager, error) { + asnDB, err := geoip2.Open(dbASNFile) + if err != nil { + return nil, fmt.Errorf("cannot open geoip ASN database %q: %w", dbASNFile, err) } + + // geoip2 only reports a database/method mismatch at lookup time, so probe + // once here rather than on every request. + if _, err := asnDB.ASN(net.IPv4(192, 0, 2, 1)); err != nil { + asnDB.Close() + return nil, fmt.Errorf("geoip database %q does not support ASN lookups: %w", dbASNFile, err) + } + + return &GeoLookupManager{asnDB: asnDB}, nil } +// Close releases the underlying database. +func (g *GeoLookupManager) Close() error { + return g.asnDB.Close() +} + +// GetGeoLookup returns the ASN record for ip. An address that is not in the +// database yields an empty record and no error. func (g *GeoLookupManager) GetGeoLookup(ip string) (*GeoLookup, error) { ipnet := net.ParseIP(ip) - ipDB, err := geoip2.Open(g.DBFile) - if err != nil { - return nil, fmt.Errorf("cannot open geoip database: %v", err) + if ipnet == nil { + return nil, fmt.Errorf("invalid IP address %q", ip) } - defer ipDB.Close() - ispDB, err := geoip2.Open(g.DBASNFile) + asn, err := g.asnDB.ASN(ipnet) if err != nil { - return nil, fmt.Errorf("cannot open geoip ISP database: %v", err) + return nil, fmt.Errorf("cannot get ASN: %w", err) } - defer ispDB.Close() - - asn, err := ispDB.ASN(ipnet) - if err != nil { - log.Error().Err(err).Msg("cannot get ASN") + if asn == nil { + asn = &geoip2.ASN{} } return &GeoLookup{ diff --git a/dnscheck/internal/maxmind/maxmind_test.go b/dnscheck/internal/maxmind/maxmind_test.go new file mode 100644 index 00000000..ac53c442 --- /dev/null +++ b/dnscheck/internal/maxmind/maxmind_test.go @@ -0,0 +1,124 @@ +package maxmind + +import ( + "os" + "path/filepath" + "testing" +) + +const ( + asnFixture = "testdata/GeoLite2-ASN.mmdb" + cityFixture = "testdata/GeoLite2-City.mmdb" +) + +// specRef: dnscheck-behaviour.md #S2 +func TestNewGeoLookupManagerRejectsMissingFile(t *testing.T) { + _, err := NewGeoLookupManager(filepath.Join(t.TempDir(), "missing.mmdb")) + if err == nil { + t.Fatal("expected an error for a missing database file") + } +} + +// specRef: dnscheck-behaviour.md #S2 +func TestNewGeoLookupManagerRejectsCorruptFile(t *testing.T) { + path := filepath.Join(t.TempDir(), "garbage.mmdb") + if err := os.WriteFile(path, []byte("this is not an mmdb file"), 0o600); err != nil { + t.Fatal(err) + } + _, err := NewGeoLookupManager(path) + if err == nil { + t.Fatal("expected an error for a corrupt database file") + } +} + +// specRef: dnscheck-behaviour.md #S3 +func TestNewGeoLookupManagerRejectsNonASNDatabase(t *testing.T) { + _, err := NewGeoLookupManager(cityFixture) + if err == nil { + t.Fatal("expected an error when the ASN path points at a City database") + } +} + +// specRef: dnscheck-behaviour.md #D5 +func TestGetGeoLookupReturnsASNForKnownIP(t *testing.T) { + g, err := NewGeoLookupManager(asnFixture) + if err != nil { + t.Fatalf("open fixture: %v", err) + } + defer g.Close() + + got, err := g.GetGeoLookup("8.8.8.8") + if err != nil { + t.Fatalf("lookup: %v", err) + } + if got.ASN != 15169 || got.ASNOrganization != "GOOGLE" { + t.Errorf("got ASN=%d org=%q, want 15169 GOOGLE", got.ASN, got.ASNOrganization) + } + if got.IPAddress != "8.8.8.8" { + t.Errorf("got IPAddress=%q, want 8.8.8.8", got.IPAddress) + } +} + +// An address absent from the database is not an error: the record is empty and +// the caller falls back to its IP-range check. +// +// specRef: dnscheck-behaviour.md #D6 +func TestGetGeoLookupUnknownIPYieldsEmptyRecord(t *testing.T) { + g, err := NewGeoLookupManager(asnFixture) + if err != nil { + t.Fatalf("open fixture: %v", err) + } + defer g.Close() + + got, err := g.GetGeoLookup("203.0.113.5") + if err != nil { + t.Fatalf("lookup: %v", err) + } + if got == nil { + t.Fatal("got nil record for an unknown IP, want an empty record") + } + if got.ASN != 0 || got.ASNOrganization != "" { + t.Errorf("got ASN=%d org=%q, want empty record", got.ASN, got.ASNOrganization) + } +} + +// specRef: dnscheck-behaviour.md #D6 +func TestGetGeoLookupRejectsUnparseableIP(t *testing.T) { + g, err := NewGeoLookupManager(asnFixture) + if err != nil { + t.Fatalf("open fixture: %v", err) + } + defer g.Close() + + got, err := g.GetGeoLookup("not-an-ip") + if err == nil { + t.Fatal("expected an error for an unparseable IP") + } + if got != nil { + t.Errorf("expected a nil record alongside the error, got %+v", got) + } +} + +// Readers are opened once at startup and shared by every request goroutine. +// +// specRef: dnscheck-behaviour.md #S1 +func TestGetGeoLookupIsSafeForConcurrentUse(t *testing.T) { + g, err := NewGeoLookupManager(asnFixture) + if err != nil { + t.Fatalf("open fixture: %v", err) + } + defer g.Close() + + done := make(chan error, 32) + for i := 0; i < 32; i++ { + go func() { + _, err := g.GetGeoLookup("8.8.8.8") + done <- err + }() + } + for i := 0; i < 32; i++ { + if err := <-done; err != nil { + t.Errorf("concurrent lookup: %v", err) + } + } +} diff --git a/dnscheck/internal/maxmind/testdata/GeoLite2-ASN.mmdb b/dnscheck/internal/maxmind/testdata/GeoLite2-ASN.mmdb new file mode 100644 index 0000000000000000000000000000000000000000..9e2b2d5f87ac8debfbc7363f6d2798f69c928f8c GIT binary patch literal 833 zcmZXQNpI6Y7>3`3uT{fN8C?5Ks-o1L{$BUfg|KY#9`tH@hI^aag?a~j{_&j$B1jh zlX-dyIGy_$lC!|MymB7+NPdxciFi3rKa=RdcwV^*TqD0uoFLvHP7-et)l|2DAHZ#* z+TNx5y*!-)rgOhf@&K60D-VHk?iIkueHJituacXx%48_ws^xi>ZGP3DjcB9VbbsvCf>Se^UdyY_>xS2C=uXSk%PqZP zdq#cGXRAK1U;Ol_+-TUQZn{>@HO*$twFaqF>5Km4kYm{1`Bf(g%7|( z;LVe-U^52|_VCN3o&2W*Fav-9C32PwNPjs8Fi-UYStb|BC32ZuA$|WUz#7%-cM87o-D;|T(uy^p^`{uD};oW?r+YAYY$Q+(nFc8rL%Ntl(- zVpMC(j6DitrBhJZf6XkN4Fp=r_At()AU&ft8*x1O-`7ccTa(6-3 Date: Mon, 7 Sep 2026 09:11:58 +0200 Subject: [PATCH 08/52] chore(dnscheck): Remove unused maxmind code Signed-off-by: Maciek --- dnscheck/internal/maxmind/country.go | 277 --------------------------- 1 file changed, 277 deletions(-) delete mode 100644 dnscheck/internal/maxmind/country.go diff --git a/dnscheck/internal/maxmind/country.go b/dnscheck/internal/maxmind/country.go deleted file mode 100644 index b96b1ef3..00000000 --- a/dnscheck/internal/maxmind/country.go +++ /dev/null @@ -1,277 +0,0 @@ -package maxmind - -type Country struct { - Code string - Name string -} - -func GetCountries() []Country { - return []Country{ - {Code: "A1", Name: "Anonymous Proxy"}, - {Code: "A2", Name: "Satellite Provider"}, - {Code: "O1", Name: "Other Country"}, - {Code: "AD", Name: "Andorra"}, - {Code: "AE", Name: "United Arab Emirates"}, - {Code: "AF", Name: "Afghanistan"}, - {Code: "AG", Name: "Antigua and Barbuda"}, - {Code: "AI", Name: "Anguilla"}, - {Code: "AL", Name: "Albania"}, - {Code: "AM", Name: "Armenia"}, - {Code: "AO", Name: "Angola"}, - {Code: "AP", Name: "Asia/Pacific Region"}, - {Code: "AQ", Name: "Antarctica"}, - {Code: "AR", Name: "Argentina"}, - {Code: "AS", Name: "American Samoa"}, - {Code: "AT", Name: "Austria"}, - {Code: "AU", Name: "Australia"}, - {Code: "AW", Name: "Aruba"}, - {Code: "AX", Name: "Aland Islands"}, - {Code: "AZ", Name: "Azerbaijan"}, - {Code: "BA", Name: "Bosnia and Herzegovina"}, - {Code: "BB", Name: "Barbados"}, - {Code: "BD", Name: "Bangladesh"}, - {Code: "BE", Name: "Belgium"}, - {Code: "BF", Name: "Burkina Faso"}, - {Code: "BG", Name: "Bulgaria"}, - {Code: "BH", Name: "Bahrain"}, - {Code: "BI", Name: "Burundi"}, - {Code: "BJ", Name: "Benin"}, - {Code: "BL", Name: "Saint Barthelemy"}, - {Code: "BM", Name: "Bermuda"}, - {Code: "BN", Name: "Brunei Darussalam"}, - {Code: "BO", Name: "Bolivia"}, - {Code: "BQ", Name: "Bonaire, Saint Eustatius and Saba"}, - {Code: "BR", Name: "Brazil"}, - {Code: "BS", Name: "Bahamas"}, - {Code: "BT", Name: "Bhutan"}, - {Code: "BV", Name: "Bouvet Island"}, - {Code: "BW", Name: "Botswana"}, - {Code: "BY", Name: "Belarus"}, - {Code: "BZ", Name: "Belize"}, - {Code: "CA", Name: "Canada"}, - {Code: "CC", Name: "Cocos (Keeling) Islands"}, - {Code: "CD", Name: "Congo, The Democratic Republic of the"}, - {Code: "CF", Name: "Central African Republic"}, - {Code: "CG", Name: "Congo"}, - {Code: "CH", Name: "Switzerland"}, - {Code: "CI", Name: "Cote d'Ivoire"}, - {Code: "CK", Name: "Cook Islands"}, - {Code: "CL", Name: "Chile"}, - {Code: "CM", Name: "Cameroon"}, - {Code: "CN", Name: "China"}, - {Code: "CO", Name: "Colombia"}, - {Code: "CR", Name: "Costa Rica"}, - {Code: "CU", Name: "Cuba"}, - {Code: "CV", Name: "Cape Verde"}, - {Code: "CW", Name: "Curacao"}, - {Code: "CX", Name: "Christmas Island"}, - {Code: "CY", Name: "Cyprus"}, - {Code: "CZ", Name: "Czech Republic"}, - {Code: "DE", Name: "Germany"}, - {Code: "DJ", Name: "Djibouti"}, - {Code: "DK", Name: "Denmark"}, - {Code: "DM", Name: "Dominica"}, - {Code: "DO", Name: "Dominican Republic"}, - {Code: "DZ", Name: "Algeria"}, - {Code: "EC", Name: "Ecuador"}, - {Code: "EE", Name: "Estonia"}, - {Code: "EG", Name: "Egypt"}, - {Code: "EH", Name: "Western Sahara"}, - {Code: "ER", Name: "Eritrea"}, - {Code: "ES", Name: "Spain"}, - {Code: "ET", Name: "Ethiopia"}, - {Code: "EU", Name: "Europe"}, - {Code: "FI", Name: "Finland"}, - {Code: "FJ", Name: "Fiji"}, - {Code: "FK", Name: "Falkland Islands (Malvinas)"}, - {Code: "FM", Name: "Micronesia, Federated States of"}, - {Code: "FO", Name: "Faroe Islands"}, - {Code: "FR", Name: "France"}, - {Code: "GA", Name: "Gabon"}, - {Code: "GB", Name: "United Kingdom"}, - {Code: "GD", Name: "Grenada"}, - {Code: "GE", Name: "Georgia"}, - {Code: "GF", Name: "French Guiana"}, - {Code: "GG", Name: "Guernsey"}, - {Code: "GH", Name: "Ghana"}, - {Code: "GI", Name: "Gibraltar"}, - {Code: "GL", Name: "Greenland"}, - {Code: "GM", Name: "Gambia"}, - {Code: "GN", Name: "Guinea"}, - {Code: "GP", Name: "Guadeloupe"}, - {Code: "GQ", Name: "Equatorial Guinea"}, - {Code: "GR", Name: "Greece"}, - {Code: "GS", Name: "South Georgia and the South Sandwich Islands"}, - {Code: "GT", Name: "Guatemala"}, - {Code: "GU", Name: "Guam"}, - {Code: "GW", Name: "Guinea-Bissau"}, - {Code: "GY", Name: "Guyana"}, - {Code: "HK", Name: "Hong Kong"}, - {Code: "HM", Name: "Heard Island and McDonald Islands"}, - {Code: "HN", Name: "Honduras"}, - {Code: "HR", Name: "Croatia"}, - {Code: "HT", Name: "Haiti"}, - {Code: "HU", Name: "Hungary"}, - {Code: "ID", Name: "Indonesia"}, - {Code: "IE", Name: "Ireland"}, - {Code: "IL", Name: "Israel"}, - {Code: "IM", Name: "Isle of Man"}, - {Code: "IN", Name: "India"}, - {Code: "IO", Name: "British Indian Ocean Territory"}, - {Code: "IQ", Name: "Iraq"}, - {Code: "IR", Name: "Iran, Islamic Republic of"}, - {Code: "IS", Name: "Iceland"}, - {Code: "IT", Name: "Italy"}, - {Code: "JE", Name: "Jersey"}, - {Code: "JM", Name: "Jamaica"}, - {Code: "JO", Name: "Jordan"}, - {Code: "JP", Name: "Japan"}, - {Code: "KE", Name: "Kenya"}, - {Code: "KG", Name: "Kyrgyzstan"}, - {Code: "KH", Name: "Cambodia"}, - {Code: "KI", Name: "Kiribati"}, - {Code: "KM", Name: "Comoros"}, - {Code: "KN", Name: "Saint Kitts and Nevis"}, - {Code: "KP", Name: "Korea, Democratic People's Republic of"}, - {Code: "KR", Name: "Korea, Republic of"}, - {Code: "KW", Name: "Kuwait"}, - {Code: "KY", Name: "Cayman Islands"}, - {Code: "KZ", Name: "Kazakhstan"}, - {Code: "LA", Name: "Lao People's Democratic Republic"}, - {Code: "LB", Name: "Lebanon"}, - {Code: "LC", Name: "Saint Lucia"}, - {Code: "LI", Name: "Liechtenstein"}, - {Code: "LK", Name: "Sri Lanka"}, - {Code: "LR", Name: "Liberia"}, - {Code: "LS", Name: "Lesotho"}, - {Code: "LT", Name: "Lithuania"}, - {Code: "LU", Name: "Luxembourg"}, - {Code: "LV", Name: "Latvia"}, - {Code: "LY", Name: "Libyan Arab Jamahiriya"}, - {Code: "MA", Name: "Morocco"}, - {Code: "MC", Name: "Monaco"}, - {Code: "MD", Name: "Moldova, Republic of"}, - {Code: "ME", Name: "Montenegro"}, - {Code: "MF", Name: "Saint Martin"}, - {Code: "MG", Name: "Madagascar"}, - {Code: "MH", Name: "Marshall Islands"}, - {Code: "MK", Name: "Macedonia"}, - {Code: "ML", Name: "Mali"}, - {Code: "MM", Name: "Myanmar"}, - {Code: "MN", Name: "Mongolia"}, - {Code: "MO", Name: "Macao"}, - {Code: "MP", Name: "Northern Mariana Islands"}, - {Code: "MQ", Name: "Martinique"}, - {Code: "MR", Name: "Mauritania"}, - {Code: "MS", Name: "Montserrat"}, - {Code: "MT", Name: "Malta"}, - {Code: "MU", Name: "Mauritius"}, - {Code: "MV", Name: "Maldives"}, - {Code: "MW", Name: "Malawi"}, - {Code: "MX", Name: "Mexico"}, - {Code: "MY", Name: "Malaysia"}, - {Code: "MZ", Name: "Mozambique"}, - {Code: "NA", Name: "Namibia"}, - {Code: "NC", Name: "New Caledonia"}, - {Code: "NE", Name: "Niger"}, - {Code: "NF", Name: "Norfolk Island"}, - {Code: "NG", Name: "Nigeria"}, - {Code: "NI", Name: "Nicaragua"}, - {Code: "NL", Name: "Netherlands"}, - {Code: "NO", Name: "Norway"}, - {Code: "NP", Name: "Nepal"}, - {Code: "NR", Name: "Nauru"}, - {Code: "NU", Name: "Niue"}, - {Code: "NZ", Name: "New Zealand"}, - {Code: "OM", Name: "Oman"}, - {Code: "PA", Name: "Panama"}, - {Code: "PE", Name: "Peru"}, - {Code: "PF", Name: "French Polynesia"}, - {Code: "PG", Name: "Papua New Guinea"}, - {Code: "PH", Name: "Philippines"}, - {Code: "PK", Name: "Pakistan"}, - {Code: "PL", Name: "Poland"}, - {Code: "PM", Name: "Saint Pierre and Miquelon"}, - {Code: "PN", Name: "Pitcairn"}, - {Code: "PR", Name: "Puerto Rico"}, - {Code: "PS", Name: "Palestinian Territory"}, - {Code: "PT", Name: "Portugal"}, - {Code: "PW", Name: "Palau"}, - {Code: "PY", Name: "Paraguay"}, - {Code: "QA", Name: "Qatar"}, - {Code: "RE", Name: "Reunion"}, - {Code: "RO", Name: "Romania"}, - {Code: "RS", Name: "Serbia"}, - {Code: "RU", Name: "Russian Federation"}, - {Code: "RW", Name: "Rwanda"}, - {Code: "SA", Name: "Saudi Arabia"}, - {Code: "SB", Name: "Solomon Islands"}, - {Code: "SC", Name: "Seychelles"}, - {Code: "SD", Name: "Sudan"}, - {Code: "SE", Name: "Sweden"}, - {Code: "SG", Name: "Singapore"}, - {Code: "SH", Name: "Saint Helena"}, - {Code: "SI", Name: "Slovenia"}, - {Code: "SJ", Name: "Svalbard and Jan Mayen"}, - {Code: "SK", Name: "Slovakia"}, - {Code: "SL", Name: "Sierra Leone"}, - {Code: "SM", Name: "San Marino"}, - {Code: "SN", Name: "Senegal"}, - {Code: "SO", Name: "Somalia"}, - {Code: "SR", Name: "Suriname"}, - {Code: "SS", Name: "South Sudan"}, - {Code: "ST", Name: "Sao Tome and Principe"}, - {Code: "SV", Name: "El Salvador"}, - {Code: "SX", Name: "Sint Maarten"}, - {Code: "SY", Name: "Syrian Arab Republic"}, - {Code: "SZ", Name: "Swaziland"}, - {Code: "TC", Name: "Turks and Caicos Islands"}, - {Code: "TD", Name: "Chad"}, - {Code: "TF", Name: "French Southern Territories"}, - {Code: "TG", Name: "Togo"}, - {Code: "TH", Name: "Thailand"}, - {Code: "TJ", Name: "Tajikistan"}, - {Code: "TK", Name: "Tokelau"}, - {Code: "TL", Name: "Timor-Leste"}, - {Code: "TM", Name: "Turkmenistan"}, - {Code: "TN", Name: "Tunisia"}, - {Code: "TO", Name: "Tonga"}, - {Code: "TR", Name: "Turkey"}, - {Code: "TT", Name: "Trinidad and Tobago"}, - {Code: "TV", Name: "Tuvalu"}, - {Code: "TW", Name: "Taiwan"}, - {Code: "TZ", Name: "Tanzania, United Republic of"}, - {Code: "UA", Name: "Ukraine"}, - {Code: "UG", Name: "Uganda"}, - {Code: "UM", Name: "United States Minor Outlying Islands"}, - {Code: "US", Name: "United States"}, - {Code: "UY", Name: "Uruguay"}, - {Code: "UZ", Name: "Uzbekistan"}, - {Code: "VA", Name: "Holy See (Vatican City State)"}, - {Code: "VC", Name: "Saint Vincent and the Grenadines"}, - {Code: "VE", Name: "Venezuela"}, - {Code: "VG", Name: "Virgin Islands, British"}, - {Code: "VI", Name: "Virgin Islands, U.S."}, - {Code: "VN", Name: "Vietnam"}, - {Code: "VU", Name: "Vanuatu"}, - {Code: "WF", Name: "Wallis and Futuna"}, - {Code: "WS", Name: "Samoa"}, - {Code: "YE", Name: "Yemen"}, - {Code: "YT", Name: "Mayotte"}, - {Code: "ZA", Name: "South Africa"}, - {Code: "ZM", Name: "Zambia"}, - {Code: "ZW", Name: "Zimbabwe"}, - } -} - -func GetCountryByCode(countryCode string) string { - for _, country := range GetCountries() { - if country.Code == countryCode { - return country.Name - } - } - - // @TODO Notify Sentry - - return "Unknown" -} From 031551bbc3734ef2cfc4ab22ec42f01caa747a4e Mon Sep 17 00:00:00 2001 From: Maciek Date: Mon, 7 Sep 2026 10:15:02 +0200 Subject: [PATCH 09/52] fix(dnscheck): stop storing and logging client identifiers, wire cache TTL, require CIDR IP range Signed-off-by: Maciek --- dnscheck/.env.sample | 2 +- dnscheck/api/check.go | 6 +- dnscheck/api/check_test.go | 60 ++++++++-- dnscheck/api/server.go | 12 +- dnscheck/cache/bigcache.go | 8 +- dnscheck/cache/cache.go | 7 +- dnscheck/config/config.go | 30 ++++- dnscheck/config/config_test.go | 67 ++++++++++- dnscheck/dns/handler.go | 39 +++---- dnscheck/dns/handler_geolookup_test.go | 147 ++++++++++++++++++++----- dnscheck/dns/models.go | 9 +- dnscheck/internal/maxmind/types.go | 1 - dnscheck/main.go | 2 +- tests/config/dnscheck.env | 4 +- 14 files changed, 310 insertions(+), 84 deletions(-) diff --git a/dnscheck/.env.sample b/dnscheck/.env.sample index c940a3c9..870d578f 100644 --- a/dnscheck/.env.sample +++ b/dnscheck/.env.sample @@ -2,7 +2,7 @@ DNS_AUTH_SERVER_DOMAIN="test.moddns.net" DNS_AUTH_SERVER_IP_ADDRESS="127.0.0.1" DNS_AUTH_SERVER_ASN="" -DNS_AUTH_SERVER_IP_RANGE="10.5." +DNS_AUTH_SERVER_IP_RANGE="10.5.0.0/16" ### API CONFIG API_PORT=":3000" diff --git a/dnscheck/api/check.go b/dnscheck/api/check.go index f7f489d9..17ed772c 100644 --- a/dnscheck/api/check.go +++ b/dnscheck/api/check.go @@ -17,7 +17,6 @@ var subdomainRegex = regexp.MustCompile(dns.SubdomainRegexPattern) func (s *APIServer) DnsCheck() fiber.Handler { handler := func(c *fiber.Ctx) error { host := c.Hostname() - log.Debug().Str("host", host).Msg("Host") hostParts := strings.Split(host, ".") if len(hostParts) < 2 { log.Error().Msg(ErrInvalidHostHeader) @@ -27,13 +26,12 @@ func (s *APIServer) DnsCheck() fiber.Handler { subdomain := strings.ToLower(hostParts[0]) if !subdomainRegex.MatchString(subdomain) { - log.Warn().Str("subdomain", subdomain).Msg("Invalid subdomain format") + log.Warn().Msg("Invalid subdomain format") return c.Status(fiber.StatusBadRequest).JSON(fiber.Map{"error": "invalid request"}) } // get data from cache cacheKey := cache.HMACKey(s.Config.Cache.HMACKey, subdomain) - log.Debug().Str("ID", subdomain).Msg("Getting query data") data, err := s.Cache.GetQueryData(cacheKey) if err != nil { return HandleError(c, err, ErrFailedToGetQueryData) @@ -42,7 +40,7 @@ func (s *APIServer) DnsCheck() fiber.Handler { // Delete-on-read: each subdomain is single-use (frontend generates a fresh // nanoid per poll), so delete immediately to minimize the replay window. if delErr := s.Cache.DeleteQueryData(cacheKey); delErr != nil { - log.Warn().Err(delErr).Str("ID", subdomain).Msg("Failed to delete cache entry after read") + log.Warn().Err(delErr).Msg("Failed to delete cache entry after read") } var dnsRecord dns.DNSLogRecord diff --git a/dnscheck/api/check_test.go b/dnscheck/api/check_test.go index 14060c05..688cd1fc 100644 --- a/dnscheck/api/check_test.go +++ b/dnscheck/api/check_test.go @@ -1,16 +1,20 @@ package api import ( + "bytes" "encoding/json" "errors" "io" "net/http" "net/http/httptest" + "strings" "testing" "github.com/dnscheck/cache" "github.com/dnscheck/config" "github.com/dnscheck/dns" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" ) type memCache struct { @@ -42,10 +46,15 @@ const ( ) func newTestServer(c *memCache) *APIServer { + return newTestServerWithAccessLog(c, io.Discard) +} + +func newTestServerWithAccessLog(c *memCache, accessLog io.Writer) *APIServer { s := NewServer(&config.Config{ API: &config.APIConfig{ApiAllowOrigin: "*"}, Cache: &config.CacheConfig{HMACKey: testHMACKey}, }, c) + s.AccessLog = accessLog s.RegisterRoutes() return s } @@ -97,10 +106,7 @@ func TestDnsCheckReturnsDisconnectedWhenNoRecord(t *testing.T) { // specRef: dnscheck-behaviour.md #A3, #A4 func TestDnsCheckReturnsNarrowRecordOnceOnly(t *testing.T) { c := &memCache{} - rec, _ := json.Marshal(dns.DNSLogRecord{ - Status: dns.StatusConfigured, ProfileId: "profile1", - IPAddress: "203.0.113.5", ASN: 64512, ASNOrganization: "OURS", - }) + rec, _ := json.Marshal(dns.DNSLogRecord{Status: dns.StatusConfigured, ProfileId: "profile1"}) if err := c.SaveQueryData(cache.HMACKey(testHMACKey, testSubdomain), rec); err != nil { t.Fatal(err) } @@ -117,13 +123,51 @@ func TestDnsCheckReturnsNarrowRecordOnceOnly(t *testing.T) { if got["status"] != dns.StatusConfigured || got["profile_id"] != "profile1" { t.Errorf("body = %s", body) } - for _, leaked := range []string{"ip_address", "asn", "asn_organization"} { - if _, ok := got[leaked]; ok { - t.Errorf("response leaks %q: %s", leaked, body) - } + if len(got) != 2 { + t.Errorf("response must carry exactly status and profile_id: %s", body) } if resp, _ := get(t, s, testHost); resp.StatusCode != http.StatusNotFound { t.Errorf("second read status = %d, want 404 (delete-on-read)", resp.StatusCode) } } + +// The Host header (probe ID + profile ID) and the client address must not be +// logged by the handler or the access-log middleware. +// +// specRef: dnscheck-behaviour.md #A5 +func TestDnsCheckLogsCarryNoClientIdentifiers(t *testing.T) { + var buf bytes.Buffer + prev := log.Logger + prevLevel := zerolog.GlobalLevel() + log.Logger = zerolog.New(&buf) + zerolog.SetGlobalLevel(zerolog.TraceLevel) + t.Cleanup(func() { log.Logger = prev; zerolog.SetGlobalLevel(prevLevel) }) + + c := &memCache{} + rec, _ := json.Marshal(dns.DNSLogRecord{Status: dns.StatusConfigured, ProfileId: "profile1"}) + _ = c.SaveQueryData(cache.HMACKey(testHMACKey, testSubdomain), rec) + // The access log shares the buffer so the middleware format is covered too. + s := newTestServerWithAccessLog(c, &buf) + + for _, host := range []string{testHost, testHost, "short.check.example.test", "localhost"} { + req := httptest.NewRequest(http.MethodGet, "/", nil) + req.Host = host + req.RemoteAddr = "203.0.113.5:40000" + resp, err := s.App.Test(req) + if err != nil { + t.Fatal(err) + } + resp.Body.Close() + } + + out := buf.String() + if !strings.Contains(out, "GET") { + t.Fatalf("expected access-log lines in output, got:\n%s", out) + } + for _, secret := range []string{"203.0.113.5", testSubdomain, "profile1", "check.example.test"} { + if strings.Contains(out, secret) { + t.Errorf("log output contains %q:\n%s", secret, out) + } + } +} diff --git a/dnscheck/api/server.go b/dnscheck/api/server.go index 49f34551..cba8fd94 100644 --- a/dnscheck/api/server.go +++ b/dnscheck/api/server.go @@ -1,6 +1,9 @@ package api import ( + "io" + "os" + "github.com/dnscheck/cache" "github.com/dnscheck/config" @@ -19,6 +22,8 @@ type APIServer struct { Config *config.Config Validator *APIValidator Cache cache.Cache + // AccessLog receives the per-request access log; defaults to stdout. + AccessLog io.Writer } // NewServer inititiates database connection and sets up API endpoints @@ -38,13 +43,18 @@ func NewServer(config *config.Config, cache cache.Cache) *APIServer { Config: config, Validator: apiValidator, Cache: cache, + AccessLog: os.Stdout, } } // RegisterRoutes registers API endpoints func (s *APIServer) RegisterRoutes() { s.App.Use(requestid.New()) - s.App.Use(logger.New()) + // Default format includes ${ip}; the client address is not logged. + s.App.Use(logger.New(logger.Config{ + Format: "${time} | ${status} | ${latency} | ${method} | ${path} | ${error}\n", + Output: s.AccessLog, + })) s.App.Use(limiter.New( limiter.Config{ Max: 100, diff --git a/dnscheck/cache/bigcache.go b/dnscheck/cache/bigcache.go index 7f98b5da..41976791 100644 --- a/dnscheck/cache/bigcache.go +++ b/dnscheck/cache/bigcache.go @@ -7,16 +7,14 @@ import ( "github.com/allegro/bigcache/v3" ) -const expirationTime = 1 * time.Minute - type BigCache struct { cache *bigcache.BigCache } -// NewBigcache creates a new BigCache instance -func NewBigcache() (*BigCache, error) { +// NewBigcache creates a new BigCache instance with the given entry lifetime. +func NewBigcache(ttl time.Duration) (*BigCache, error) { queriesCache := &BigCache{} - cache, err := bigcache.New(context.Background(), bigcache.DefaultConfig(expirationTime)) + cache, err := bigcache.New(context.Background(), bigcache.DefaultConfig(ttl)) if err != nil { return nil, err } diff --git a/dnscheck/cache/cache.go b/dnscheck/cache/cache.go index 10d1e673..3e577c1c 100644 --- a/dnscheck/cache/cache.go +++ b/dnscheck/cache/cache.go @@ -2,6 +2,7 @@ package cache import ( "errors" + "time" ) const CacheTypeBigCache = "bigcache" @@ -13,11 +14,11 @@ type Cache interface { DeleteQueryData(key string) error } -// New creates a new Cache instance -func New(cacheType string) (Cache, error) { +// New creates a new Cache instance whose entries expire after ttl. +func New(cacheType string, ttl time.Duration) (Cache, error) { switch cacheType { case CacheTypeBigCache: - return NewBigcache() + return NewBigcache(ttl) } return nil, errors.New("unknown cache type") } diff --git a/dnscheck/config/config.go b/dnscheck/config/config.go index 26762e80..880ade66 100644 --- a/dnscheck/config/config.go +++ b/dnscheck/config/config.go @@ -2,11 +2,18 @@ package config import ( "errors" + "fmt" + "net" "os" "strconv" "time" ) +// DefaultCacheTTL bounds how long a check record stays in memory. The frontend +// reads it within milliseconds of the DNS query, so this only needs to cover +// resolver retries. +const DefaultCacheTTL = 15 * time.Second + // Config represents the application configuration type Config struct { Server *AuthoritativeDNSServerConfig @@ -20,7 +27,8 @@ type AuthoritativeDNSServerConfig struct { Domain string IPAddress string ASN uint - IPRange string + // IPRange is the CIDR block our resolvers query from; required. + IPRange *net.IPNet } // APIConfig represents the API configuration @@ -54,10 +62,22 @@ func (cfg *GeoLookupConfig) IsValid() error { // New creates a new Config instance func New() (*Config, error) { - cacheTTL := os.Getenv("CACHE_TTL") - ttl, err := time.ParseDuration(cacheTTL) + ttl := DefaultCacheTTL + if raw := os.Getenv("CACHE_TTL"); raw != "" { + parsed, err := time.ParseDuration(raw) + if err != nil || parsed <= 0 { + return nil, fmt.Errorf("CACHE_TTL must be a positive duration, got %q", raw) + } + ttl = parsed + } + + rawRange := os.Getenv("DNS_AUTH_SERVER_IP_RANGE") + if rawRange == "" { + return nil, errors.New("DNS_AUTH_SERVER_IP_RANGE environment variable is required") + } + _, ipRange, err := net.ParseCIDR(rawRange) if err != nil { - ttl = 1 * time.Minute + return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE must be CIDR notation (e.g. 10.5.0.0/16), got %q", rawRange) } asn := os.Getenv("DNS_AUTH_SERVER_ASN") @@ -83,7 +103,7 @@ func New() (*Config, error) { Domain: os.Getenv("DNS_AUTH_SERVER_DOMAIN"), IPAddress: os.Getenv("DNS_AUTH_SERVER_IP_ADDRESS"), ASN: uint(asnUint), - IPRange: os.Getenv("DNS_AUTH_SERVER_IP_RANGE"), + IPRange: ipRange, }, API: &APIConfig{ Port: os.Getenv("API_PORT"), diff --git a/dnscheck/config/config_test.go b/dnscheck/config/config_test.go index ae992c63..a016c28e 100644 --- a/dnscheck/config/config_test.go +++ b/dnscheck/config/config_test.go @@ -1,10 +1,14 @@ package config -import "testing" +import ( + "testing" + "time" +) // specRef: dnscheck-behaviour.md #S2 func TestNewRequiresASNDatabasePath(t *testing.T) { t.Setenv("CACHE_HMAC_KEY", "test-key") + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "10.5.0.0/16") t.Setenv("GEOIP_DB_ASN_FILE", "") if _, err := New(); err == nil { @@ -20,3 +24,64 @@ func TestNewRequiresASNDatabasePath(t *testing.T) { t.Errorf("DBASNFile = %q", cfg.GeoLookupConfig.DBASNFile) } } + +// specRef: dnscheck-behaviour.md #S4 +func TestNewParsesIPRangeAsCIDR(t *testing.T) { + t.Setenv("CACHE_HMAC_KEY", "test-key") + t.Setenv("GEOIP_DB_ASN_FILE", "/opt/dnscheck/GeoLite2-ASN.mmdb") + + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "10.5.0.0/16") + cfg, err := New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Server.IPRange == nil || cfg.Server.IPRange.String() != "10.5.0.0/16" { + t.Errorf("IPRange = %v, want 10.5.0.0/16", cfg.Server.IPRange) + } + + // The range is what makes a query "ours"; without it every check would + // depend on the ASN alone, so an unset value is a boot error. + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "") + if _, err := New(); err == nil { + t.Fatal("expected an error when DNS_AUTH_SERVER_IP_RANGE is unset") + } + + // The legacy string-prefix form is rejected so a misconfiguration fails at + // boot instead of silently matching the wrong addresses. + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "10.5.") + if _, err := New(); err == nil { + t.Fatal("expected an error for a non-CIDR IP range") + } +} + +// specRef: dnscheck-behaviour.md #S5 +func TestNewCacheTTLDefaultsAndValidates(t *testing.T) { + t.Setenv("CACHE_HMAC_KEY", "test-key") + t.Setenv("GEOIP_DB_ASN_FILE", "/opt/dnscheck/GeoLite2-ASN.mmdb") + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "10.5.0.0/16") + + t.Setenv("CACHE_TTL", "") + cfg, err := New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Cache.TTL != DefaultCacheTTL { + t.Errorf("TTL = %v, want default %v", cfg.Cache.TTL, DefaultCacheTTL) + } + + t.Setenv("CACHE_TTL", "30s") + cfg, err = New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if cfg.Cache.TTL != 30*time.Second { + t.Errorf("TTL = %v, want 30s", cfg.Cache.TTL) + } + + for _, bad := range []string{"soon", "-5s", "0"} { + t.Setenv("CACHE_TTL", bad) + if _, err := New(); err == nil { + t.Errorf("expected an error for CACHE_TTL=%q", bad) + } + } +} diff --git a/dnscheck/dns/handler.go b/dnscheck/dns/handler.go index 82b11981..f19cff44 100644 --- a/dnscheck/dns/handler.go +++ b/dnscheck/dns/handler.go @@ -32,7 +32,6 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { defer func() { if rec := recover(); rec != nil { log.Error().Interface("panic", rec). - Str("remote", w.RemoteAddr().String()). Msg("Recovered from panic while serving DNS request") } }() @@ -41,8 +40,7 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { // body, so a message can declare a question yet carry none. That unpacks // without error, leaving Question empty here. if len(r.Question) == 0 { - log.Debug().Str("remote", w.RemoteAddr().String()). - Msg("Rejecting DNS request with no question section") + log.Debug().Msg("Rejecting DNS request with no question section") m := new(dns.Msg) m.SetRcode(r, dns.RcodeFormatError) if err := w.WriteMsg(m); err != nil { @@ -51,7 +49,7 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { return } - log.Debug().Str("protocol", w.RemoteAddr().Network()).Str("qtype", dns.Type(r.Question[0].Qtype).String()).Msgf("Received DNS request: %s", r.Question[0].Name) + log.Debug().Str("protocol", w.RemoteAddr().Network()).Str("qtype", dns.Type(r.Question[0].Qtype).String()).Msg("Received DNS request") msg := dns.Msg{} msg.SetReply(r) @@ -72,35 +70,32 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { } if !matched { - log.Warn().Str("subdomain", subdomain).Msg("Unidentified subdomain") + log.Warn().Msg("Unidentified subdomain") return } record := DNSLogRecord{} - IPAddress, err := clientIP(w.RemoteAddr()) + clientAddr, err := clientIP(w.RemoteAddr()) if err != nil { - log.Warn().Err(err).Str("remote", w.RemoteAddr().String()).Msg("Cannot determine client IP address") + log.Warn().Err(err).Msg("Cannot determine client IP address") return } // A failed lookup degrades to "no ASN information"; the IP-range check // below still decides the status and the answer is still written. - lookupData, err := h.srv.GeoLookup.GetGeoLookup(IPAddress) + lookupData, err := h.srv.GeoLookup.GetGeoLookup(clientAddr.String()) if err != nil || lookupData == nil { log.Error().Err(err).Msg("GeoIP lookup failed, continuing without ASN") - lookupData = &maxmind.GeoLookup{IPAddress: IPAddress} + lookupData = &maxmind.GeoLookup{} } - record.IPAddress = IPAddress - record.ASN = lookupData.ASN - record.ASNOrganization = lookupData.ASNOrganization - // decide whether IP address or ASN is from modDNS - log.Trace().Bool("isOurIPRange", strings.HasPrefix(IPAddress, h.srv.Config.Server.IPRange)). - Bool("isOurASN", lookupData.ASN == h.srv.Config.Server.ASN). + isOurIPRange := h.srv.Config.Server.IPRange.Contains(clientAddr) + isOurASN := lookupData.ASN != 0 && lookupData.ASN == h.srv.Config.Server.ASN + log.Trace().Bool("isOurIPRange", isOurIPRange).Bool("isOurASN", isOurASN). Msg("Checking if IP address or ASN is from our range") - if strings.HasPrefix(IPAddress, h.srv.Config.Server.IPRange) || lookupData.ASN == h.srv.Config.Server.ASN { + if isOurIPRange || isOurASN { profileId := h.extractConfiguredProfileId(r) record.Status = StatusConfigured record.ProfileId = profileId @@ -114,9 +109,9 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { } cacheKey := cache.HMACKey(h.srv.Config.Cache.HMACKey, subdomain) if err = h.srv.Cache.SaveQueryData(cacheKey, recordBytes); err != nil { - log.Error().Err(err).Str("ID", subdomain).Msg("Failed to save record") + log.Error().Err(err).Msg("Failed to save record") } - log.Debug().Str("ID", subdomain).Msg("Record saved") + log.Debug().Msg("Record saved") } msg.Answer = append(msg.Answer, &dns.A{ @@ -223,14 +218,14 @@ func (h *Handler) createSOA() []dns.RR { // clientIP returns the transport-level source address of the query. It is read // straight from the socket address and never resolved. -func clientIP(addr net.Addr) (string, error) { +func clientIP(addr net.Addr) (net.IP, error) { switch a := addr.(type) { case *net.UDPAddr: - return a.IP.String(), nil + return a.IP, nil case *net.TCPAddr: - return a.IP.String(), nil + return a.IP, nil default: - return "", fmt.Errorf("unsupported remote address type %T", addr) + return nil, fmt.Errorf("unsupported remote address type %T", addr) } } diff --git a/dnscheck/dns/handler_geolookup_test.go b/dnscheck/dns/handler_geolookup_test.go index 5872a364..9a94998e 100644 --- a/dnscheck/dns/handler_geolookup_test.go +++ b/dnscheck/dns/handler_geolookup_test.go @@ -1,14 +1,18 @@ package dns import ( + "bytes" "encoding/json" "errors" "net" + "strings" "testing" "github.com/dnscheck/config" "github.com/dnscheck/internal/maxmind" "github.com/miekg/dns" + "github.com/rs/zerolog" + "github.com/rs/zerolog/log" ) const ( @@ -18,11 +22,13 @@ const ( ) type fakeGeoLookup struct { - result *maxmind.GeoLookup - err error + result *maxmind.GeoLookup + err error + askedIP string } func (f *fakeGeoLookup) GetGeoLookup(ip string) (*maxmind.GeoLookup, error) { + f.askedIP = ip return f.result, f.err } @@ -48,14 +54,23 @@ func (w *tcpCaptureWriter) RemoteAddr() net.Addr { } func (w *tcpCaptureWriter) Network() string { return "tcp" } -func newTestHandler(geo GeoLookuper, cache *memCache) *Handler { +func mustCIDR(t *testing.T, s string) *net.IPNet { + t.Helper() + _, n, err := net.ParseCIDR(s) + if err != nil { + t.Fatal(err) + } + return n +} + +func newTestHandler(t *testing.T, geo GeoLookuper, cache *memCache) *Handler { return &Handler{srv: &DNSServer{ Config: &config.Config{ Server: &config.AuthoritativeDNSServerConfig{ Domain: testDomain, IPAddress: "192.0.2.1", ASN: testOurASN, - IPRange: "198.51.100.", + IPRange: mustCIDR(t, "198.51.100.0/24"), }, Cache: &config.CacheConfig{HMACKey: "test-key"}, }, @@ -80,6 +95,17 @@ func savedRecord(t *testing.T, c *memCache) DNSLogRecord { if err := json.Unmarshal(raw, &rec); err != nil { t.Fatalf("saved record is not JSON: %v", err) } + // Data minimisation: the stored record must hold nothing beyond what the + // HTTP side returns. + var keys map[string]json.RawMessage + if err := json.Unmarshal(raw, &keys); err != nil { + t.Fatal(err) + } + for k := range keys { + if k != "status" && k != "profile_id" { + t.Errorf("stored record carries unexpected field %q: %s", k, raw) + } + } } return rec } @@ -91,7 +117,7 @@ func savedRecord(t *testing.T, c *memCache) DNSLogRecord { // specRef: dnscheck-behaviour.md #D6 func TestServeDNSDegradesWhenGeoLookupFails(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{err: errors.New("lookup failed")}, cache) + h := newTestHandler(t, &fakeGeoLookup{err: errors.New("lookup failed")}, cache) w := &captureWriter{} defer func() { @@ -109,18 +135,57 @@ func TestServeDNSDegradesWhenGeoLookupFails(t *testing.T) { if rec.Status != StatusUnconfigured { t.Errorf("status = %q, want %q", rec.Status, StatusUnconfigured) } - if rec.ASN != 0 || rec.ASNOrganization != "" { - t.Errorf("expected empty ASN fields, got ASN=%d org=%q", rec.ASN, rec.ASNOrganization) +} + +// With no ASN information the IP-range check alone can still mark the query +// as ours. +// +// specRef: dnscheck-behaviour.md #D6, #D7 +func TestServeDNSFallsBackToIPRangeWhenGeoLookupFails(t *testing.T) { + cache := &memCache{} + h := newTestHandler(t, &fakeGeoLookup{err: errors.New("lookup failed")}, cache) + w := &rangeCaptureWriter{} + + h.ServeDNS(w, checkQuery()) + + if rec := savedRecord(t, cache); rec.Status != StatusConfigured { + t.Errorf("status = %q, want %q for an in-range client", rec.Status, StatusConfigured) } - if rec.IPAddress != "203.0.113.5" { - t.Errorf("IPAddress = %q, want 203.0.113.5", rec.IPAddress) +} + +// rangeCaptureWriter reports a client inside DNS_AUTH_SERVER_IP_RANGE. +type rangeCaptureWriter struct{ captureWriter } + +func (w *rangeCaptureWriter) RemoteAddr() net.Addr { + return &net.UDPAddr{IP: net.IPv4(198, 51, 100, 7), Port: 40000} +} + +// A CIDR range must not match by string prefix: 198.51.100.0/24 is not +// 198.51.10.x. +// +// specRef: dnscheck-behaviour.md #D7 +func TestServeDNSIPRangeIsCIDRNotStringPrefix(t *testing.T) { + cache := &memCache{} + h := newTestHandler(t, &fakeGeoLookup{result: &maxmind.GeoLookup{}}, cache) + w := &nearMissCaptureWriter{} + + h.ServeDNS(w, checkQuery()) + + if rec := savedRecord(t, cache); rec.Status != StatusUnconfigured { + t.Errorf("status = %q, want %q for an out-of-range client", rec.Status, StatusUnconfigured) } } +type nearMissCaptureWriter struct{ captureWriter } + +func (w *nearMissCaptureWriter) RemoteAddr() net.Addr { + return &net.UDPAddr{IP: net.IPv4(198, 51, 10, 7), Port: 40000} +} + // specRef: dnscheck-behaviour.md #D7 func TestServeDNSMarksConfiguredWhenASNMatches(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{ + h := newTestHandler(t, &fakeGeoLookup{result: &maxmind.GeoLookup{ IPAddress: "203.0.113.5", ASN: testOurASN, ASNOrganization: "OURS", }}, cache) @@ -138,27 +203,20 @@ func TestServeDNSMarksConfiguredWhenASNMatches(t *testing.T) { if rec.ProfileId != "profile1" { t.Errorf("profile_id = %q, want profile1", rec.ProfileId) } - if rec.ASN != testOurASN || rec.ASNOrganization != "OURS" { - t.Errorf("ASN fields not carried into record: %+v", rec) - } } // specRef: dnscheck-behaviour.md #D8 func TestServeDNSMarksUnconfiguredWhenNeitherASNNorRangeMatch(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{ + h := newTestHandler(t, &fakeGeoLookup{result: &maxmind.GeoLookup{ IPAddress: "203.0.113.5", ASN: 15169, ASNOrganization: "GOOGLE", }}, cache) h.ServeDNS(&captureWriter{}, checkQuery()) - rec := savedRecord(t, cache) - if rec.Status != StatusUnconfigured { + if rec := savedRecord(t, cache); rec.Status != StatusUnconfigured { t.Errorf("status = %q, want %q", rec.Status, StatusUnconfigured) } - if rec.ASN != 15169 { - t.Errorf("ASN = %d, want 15169", rec.ASN) - } } // The remote address is taken from the transport as-is; it is never resolved. @@ -166,13 +224,52 @@ func TestServeDNSMarksUnconfiguredWhenNeitherASNNorRangeMatch(t *testing.T) { // specRef: dnscheck-behaviour.md #D4 func TestServeDNSExtractsClientIPOverTCP(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{result: &maxmind.GeoLookup{}}, cache) + geo := &fakeGeoLookup{result: &maxmind.GeoLookup{}} + h := newTestHandler(t, geo, cache) h.ServeDNS(&tcpCaptureWriter{}, checkQuery()) - rec := savedRecord(t, cache) - if rec.IPAddress != "203.0.113.5" { - t.Errorf("IPAddress = %q, want 203.0.113.5", rec.IPAddress) + savedRecord(t, cache) + if geo.askedIP != "203.0.113.5" { + t.Errorf("looked up %q, want the TCP socket address 203.0.113.5", geo.askedIP) + } +} + +// Nothing that identifies the client or the probe may reach the logs, at any +// level: not the source address, the query name, the subdomain, or the profile +// ID. +// +// specRef: dnscheck-behaviour.md #D9 +func TestServeDNSLogsCarryNoClientIdentifiers(t *testing.T) { + var buf bytes.Buffer + prev := log.Logger + prevLevel := zerolog.GlobalLevel() + log.Logger = zerolog.New(&buf) + zerolog.SetGlobalLevel(zerolog.TraceLevel) + t.Cleanup(func() { log.Logger = prev; zerolog.SetGlobalLevel(prevLevel) }) + + cache := &memCache{} + h := newTestHandler(t, &fakeGeoLookup{err: errors.New("lookup failed")}, cache) + req := checkQuery() + opt := &dns.OPT{Hdr: dns.RR_Header{Name: ".", Rrtype: dns.TypeOPT}} + opt.Option = append(opt.Option, &dns.EDNS0_LOCAL{Code: ProfileIdAdditionalSectionCode, Data: []byte("profile1")}) + req.Extra = append(req.Extra, opt) + h.ServeDNS(&captureWriter{}, req) + + // Also drive the malformed-subdomain and no-question paths. + bad := new(dns.Msg) + bad.SetQuestion("short-profile1."+testDomain+".", dns.TypeA) + h.ServeDNS(&captureWriter{}, bad) + h.ServeDNS(&captureWriter{}, new(dns.Msg)) + + out := buf.String() + if out == "" { + t.Fatal("expected some log output at trace level") + } + for _, secret := range []string{"203.0.113.5", testSubdomain, "profile1", "short-profile1"} { + if strings.Contains(out, secret) { + t.Errorf("log output contains %q:\n%s", secret, out) + } } } @@ -182,7 +279,7 @@ func TestServeDNSExtractsClientIPOverTCP(t *testing.T) { // specRef: dnscheck-behaviour.md #D2 func TestServeDNSAnswersForeignDomainWithoutRecord(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{err: errors.New("must not be called")}, cache) + h := newTestHandler(t, &fakeGeoLookup{err: errors.New("must not be called")}, cache) w := &captureWriter{} req := new(dns.Msg) @@ -200,7 +297,7 @@ func TestServeDNSAnswersForeignDomainWithoutRecord(t *testing.T) { // specRef: dnscheck-behaviour.md #D3 func TestServeDNSIgnoresMalformedSubdomain(t *testing.T) { cache := &memCache{} - h := newTestHandler(&fakeGeoLookup{err: errors.New("must not be called")}, cache) + h := newTestHandler(t, &fakeGeoLookup{err: errors.New("must not be called")}, cache) w := &captureWriter{} req := new(dns.Msg) diff --git a/dnscheck/dns/models.go b/dnscheck/dns/models.go index 979d0188..60755332 100644 --- a/dnscheck/dns/models.go +++ b/dnscheck/dns/models.go @@ -5,12 +5,11 @@ const ( StatusUnconfigured = "unconfigured" ) +// DNSLogRecord is what the DNS side stores for the HTTP side to read. The +// client IP and ASN only ever feed the status decision and are not kept. type DNSLogRecord struct { - Status string `json:"status"` - ProfileId string `json:"profile_id"` - IPAddress string `json:"ip_address"` - ASN uint `json:"asn"` - ASNOrganization string `json:"asn_organization"` + Status string `json:"status"` + ProfileId string `json:"profile_id"` } // DNSCheckResponse is the minimal HTTP response payload. diff --git a/dnscheck/internal/maxmind/types.go b/dnscheck/internal/maxmind/types.go index 16b91f25..6652c387 100644 --- a/dnscheck/internal/maxmind/types.go +++ b/dnscheck/internal/maxmind/types.go @@ -4,5 +4,4 @@ type GeoLookup struct { IPAddress string `json:"ip_address"` ASN uint `json:"asn"` ASNOrganization string `json:"asn_organization"` - IsIvpnServer bool `json:"is_ivpn_server"` } diff --git a/dnscheck/main.go b/dnscheck/main.go index 3c51c8ab..edc90d61 100644 --- a/dnscheck/main.go +++ b/dnscheck/main.go @@ -24,7 +24,7 @@ func main() { log.Fatal().Err(err).Msg("Failed to read app configuration") } - cache, err := cache.New(cache.CacheTypeBigCache) + cache, err := cache.New(cache.CacheTypeBigCache, cfg.Cache.TTL) if err != nil { log.Fatal().Err(err).Msg("Failed to create cache") } diff --git a/tests/config/dnscheck.env b/tests/config/dnscheck.env index 7d5118dd..979301fb 100644 --- a/tests/config/dnscheck.env +++ b/tests/config/dnscheck.env @@ -2,7 +2,7 @@ DNS_AUTH_SERVER_DOMAIN="test.moddns.net" DNS_AUTH_SERVER_IP_ADDRESS="127.0.0.1" DNS_AUTH_SERVER_ASN="" -DNS_AUTH_SERVER_IP_RANGE="10.5." +DNS_AUTH_SERVER_IP_RANGE="10.5.0.0/16" # ## API CONFIG API_PORT=":3000" @@ -10,7 +10,7 @@ API_PORT=":3000" API_ALLOW_ORIGIN="*" # ## CACHE CONFIG -CACHE_TTL=1m +CACHE_TTL=15s CACHE_HMAC_KEY="test-hmac-secret-key" # ## GEO LOOKUP CONFIG From bfb6d4e584fa6b7f718b4f37b4960f64b77a7cf5 Mon Sep 17 00:00:00 2001 From: Maciek Date: Mon, 7 Sep 2026 10:37:43 +0200 Subject: [PATCH 10/52] feat(dnscheck): accept a comma-separated CIDR list for the trusted IP range Signed-off-by: Maciek --- dnscheck/config/config.go | 47 +++++++++++++++++++++----- dnscheck/config/config_test.go | 28 +++++++++++++-- dnscheck/dns/handler.go | 2 +- dnscheck/dns/handler_geolookup_test.go | 22 +++++++++++- 4 files changed, 86 insertions(+), 13 deletions(-) diff --git a/dnscheck/config/config.go b/dnscheck/config/config.go index 880ade66..26ee5bd0 100644 --- a/dnscheck/config/config.go +++ b/dnscheck/config/config.go @@ -6,6 +6,7 @@ import ( "net" "os" "strconv" + "strings" "time" ) @@ -27,8 +28,19 @@ type AuthoritativeDNSServerConfig struct { Domain string IPAddress string ASN uint - // IPRange is the CIDR block our resolvers query from; required. - IPRange *net.IPNet + // IPRanges are the CIDR blocks our resolvers query from; at least one is + // required. PoPs sit in unrelated address blocks, so this is a list. + IPRanges []*net.IPNet +} + +// ContainsIP reports whether ip falls inside any configured range. +func (c *AuthoritativeDNSServerConfig) ContainsIP(ip net.IP) bool { + for _, r := range c.IPRanges { + if r.Contains(ip) { + return true + } + } + return false } // APIConfig represents the API configuration @@ -71,13 +83,9 @@ func New() (*Config, error) { ttl = parsed } - rawRange := os.Getenv("DNS_AUTH_SERVER_IP_RANGE") - if rawRange == "" { - return nil, errors.New("DNS_AUTH_SERVER_IP_RANGE environment variable is required") - } - _, ipRange, err := net.ParseCIDR(rawRange) + ipRanges, err := parseIPRanges(os.Getenv("DNS_AUTH_SERVER_IP_RANGE")) if err != nil { - return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE must be CIDR notation (e.g. 10.5.0.0/16), got %q", rawRange) + return nil, err } asn := os.Getenv("DNS_AUTH_SERVER_ASN") @@ -103,7 +111,7 @@ func New() (*Config, error) { Domain: os.Getenv("DNS_AUTH_SERVER_DOMAIN"), IPAddress: os.Getenv("DNS_AUTH_SERVER_IP_ADDRESS"), ASN: uint(asnUint), - IPRange: ipRange, + IPRanges: ipRanges, }, API: &APIConfig{ Port: os.Getenv("API_PORT"), @@ -116,3 +124,24 @@ func New() (*Config, error) { GeoLookupConfig: geoLookup, }, nil } + +// parseIPRanges parses a comma-separated list of CIDR blocks. Every entry must +// parse and at least one is required. +func parseIPRanges(raw string) ([]*net.IPNet, error) { + var ranges []*net.IPNet + for _, part := range strings.Split(raw, ",") { + part = strings.TrimSpace(part) + if part == "" { + continue + } + _, n, err := net.ParseCIDR(part) + if err != nil { + return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE entries must be CIDR notation (e.g. 10.5.0.0/16 or 198.51.100.7/32), got %q", part) + } + ranges = append(ranges, n) + } + if len(ranges) == 0 { + return nil, errors.New("DNS_AUTH_SERVER_IP_RANGE environment variable is required (comma-separated CIDR list)") + } + return ranges, nil +} diff --git a/dnscheck/config/config_test.go b/dnscheck/config/config_test.go index a016c28e..29e6be21 100644 --- a/dnscheck/config/config_test.go +++ b/dnscheck/config/config_test.go @@ -1,6 +1,7 @@ package config import ( + "net" "testing" "time" ) @@ -35,8 +36,31 @@ func TestNewParsesIPRangeAsCIDR(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if cfg.Server.IPRange == nil || cfg.Server.IPRange.String() != "10.5.0.0/16" { - t.Errorf("IPRange = %v, want 10.5.0.0/16", cfg.Server.IPRange) + if len(cfg.Server.IPRanges) != 1 || cfg.Server.IPRanges[0].String() != "10.5.0.0/16" { + t.Errorf("IPRanges = %v, want [10.5.0.0/16]", cfg.Server.IPRanges) + } + + // PoPs live in unrelated blocks, so a comma-separated list is accepted; + // whitespace and a trailing comma are tolerated. + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "198.51.100.7/32, 203.0.113.0/24,") + cfg, err = New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if len(cfg.Server.IPRanges) != 2 { + t.Fatalf("IPRanges = %v, want two entries", cfg.Server.IPRanges) + } + if !cfg.Server.ContainsIP(net.ParseIP("198.51.100.7")) || !cfg.Server.ContainsIP(net.ParseIP("203.0.113.9")) { + t.Errorf("ContainsIP does not cover both configured ranges: %v", cfg.Server.IPRanges) + } + if cfg.Server.ContainsIP(net.ParseIP("198.51.100.8")) { + t.Errorf("/32 entry must match a single address only") + } + + // One bad entry fails the whole list. + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "198.51.100.7/32,10.5.") + if _, err := New(); err == nil { + t.Fatal("expected an error when one list entry is not CIDR") } // The range is what makes a query "ours"; without it every check would diff --git a/dnscheck/dns/handler.go b/dnscheck/dns/handler.go index f19cff44..13c9b0da 100644 --- a/dnscheck/dns/handler.go +++ b/dnscheck/dns/handler.go @@ -91,7 +91,7 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { } // decide whether IP address or ASN is from modDNS - isOurIPRange := h.srv.Config.Server.IPRange.Contains(clientAddr) + isOurIPRange := h.srv.Config.Server.ContainsIP(clientAddr) isOurASN := lookupData.ASN != 0 && lookupData.ASN == h.srv.Config.Server.ASN log.Trace().Bool("isOurIPRange", isOurIPRange).Bool("isOurASN", isOurASN). Msg("Checking if IP address or ASN is from our range") diff --git a/dnscheck/dns/handler_geolookup_test.go b/dnscheck/dns/handler_geolookup_test.go index 9a94998e..0fb8e1e7 100644 --- a/dnscheck/dns/handler_geolookup_test.go +++ b/dnscheck/dns/handler_geolookup_test.go @@ -70,7 +70,7 @@ func newTestHandler(t *testing.T, geo GeoLookuper, cache *memCache) *Handler { Domain: testDomain, IPAddress: "192.0.2.1", ASN: testOurASN, - IPRange: mustCIDR(t, "198.51.100.0/24"), + IPRanges: []*net.IPNet{mustCIDR(t, "198.51.100.0/24"), mustCIDR(t, "192.0.2.77/32")}, }, Cache: &config.CacheConfig{HMACKey: "test-key"}, }, @@ -160,6 +160,26 @@ func (w *rangeCaptureWriter) RemoteAddr() net.Addr { return &net.UDPAddr{IP: net.IPv4(198, 51, 100, 7), Port: 40000} } +// Any configured range qualifies, including a single-address /32. +// +// specRef: dnscheck-behaviour.md #D7 +func TestServeDNSMatchesAnyConfiguredRange(t *testing.T) { + cache := &memCache{} + h := newTestHandler(t, &fakeGeoLookup{result: &maxmind.GeoLookup{}}, cache) + + h.ServeDNS(&singleAddrCaptureWriter{}, checkQuery()) + + if rec := savedRecord(t, cache); rec.Status != StatusConfigured { + t.Errorf("status = %q, want %q for a client matching the second range", rec.Status, StatusConfigured) + } +} + +type singleAddrCaptureWriter struct{ captureWriter } + +func (w *singleAddrCaptureWriter) RemoteAddr() net.Addr { + return &net.UDPAddr{IP: net.IPv4(192, 0, 2, 77), Port: 40000} +} + // A CIDR range must not match by string prefix: 198.51.100.0/24 is not // 198.51.10.x. // From c38cc686e720868af9402ae2b4f4627930801877 Mon Sep 17 00:00:00 2001 From: Maciek Date: Mon, 7 Sep 2026 12:07:05 +0200 Subject: [PATCH 11/52] feat(dnscheck): accept a labelled comma-separated CIDR list for the trusted IP range Signed-off-by: Maciek --- dnscheck/config/config.go | 53 +++++++++++++++++++++----- dnscheck/config/config_test.go | 24 +++++++++++- dnscheck/dns/handler_geolookup_test.go | 5 ++- dnscheck/main.go | 4 ++ 4 files changed, 73 insertions(+), 13 deletions(-) diff --git a/dnscheck/config/config.go b/dnscheck/config/config.go index 26ee5bd0..04f7cb85 100644 --- a/dnscheck/config/config.go +++ b/dnscheck/config/config.go @@ -30,19 +30,43 @@ type AuthoritativeDNSServerConfig struct { ASN uint // IPRanges are the CIDR blocks our resolvers query from; at least one is // required. PoPs sit in unrelated address blocks, so this is a list. - IPRanges []*net.IPNet + IPRanges []IPRange +} + +// IPRange is one trusted source block. Label is operator-facing only (the PoP +// name) and plays no part in matching. +type IPRange struct { + Label string + Net *net.IPNet +} + +// String renders "label net" or just "net" when unlabelled. +func (r IPRange) String() string { + if r.Label == "" { + return r.Net.String() + } + return r.Label + " " + r.Net.String() } // ContainsIP reports whether ip falls inside any configured range. func (c *AuthoritativeDNSServerConfig) ContainsIP(ip net.IP) bool { for _, r := range c.IPRanges { - if r.Contains(ip) { + if r.Net.Contains(ip) { return true } } return false } +// IPRangesString lists the configured ranges for the startup log. +func (c *AuthoritativeDNSServerConfig) IPRangesString() string { + parts := make([]string, 0, len(c.IPRanges)) + for _, r := range c.IPRanges { + parts = append(parts, r.String()) + } + return strings.Join(parts, ", ") +} + // APIConfig represents the API configuration type APIConfig struct { Port string @@ -125,23 +149,32 @@ func New() (*Config, error) { }, nil } -// parseIPRanges parses a comma-separated list of CIDR blocks. Every entry must -// parse and at least one is required. -func parseIPRanges(raw string) ([]*net.IPNet, error) { - var ranges []*net.IPNet +// parseIPRanges parses a comma-separated list of CIDR blocks, each optionally +// prefixed with a label ("tor1=198.51.100.7/32"), the key=value,key=value +// convention used by e.g. docker --label. Every entry must parse and at least +// one is required. +func parseIPRanges(raw string) ([]IPRange, error) { + var ranges []IPRange for _, part := range strings.Split(raw, ",") { part = strings.TrimSpace(part) if part == "" { continue } - _, n, err := net.ParseCIDR(part) + label, cidr := "", part + if i := strings.Index(part, "="); i >= 0 { + label, cidr = strings.TrimSpace(part[:i]), strings.TrimSpace(part[i+1:]) + if label == "" { + return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE entry %q has an empty label before '='", part) + } + } + _, n, err := net.ParseCIDR(cidr) if err != nil { - return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE entries must be CIDR notation (e.g. 10.5.0.0/16 or 198.51.100.7/32), got %q", part) + return nil, fmt.Errorf("DNS_AUTH_SERVER_IP_RANGE entries must be [label=]CIDR (e.g. 10.5.0.0/16 or tor1=198.51.100.7/32), got %q", part) } - ranges = append(ranges, n) + ranges = append(ranges, IPRange{Label: label, Net: n}) } if len(ranges) == 0 { - return nil, errors.New("DNS_AUTH_SERVER_IP_RANGE environment variable is required (comma-separated CIDR list)") + return nil, errors.New("DNS_AUTH_SERVER_IP_RANGE environment variable is required (comma-separated [label=]CIDR list)") } return ranges, nil } diff --git a/dnscheck/config/config_test.go b/dnscheck/config/config_test.go index 29e6be21..57497b48 100644 --- a/dnscheck/config/config_test.go +++ b/dnscheck/config/config_test.go @@ -36,8 +36,8 @@ func TestNewParsesIPRangeAsCIDR(t *testing.T) { if err != nil { t.Fatalf("unexpected error: %v", err) } - if len(cfg.Server.IPRanges) != 1 || cfg.Server.IPRanges[0].String() != "10.5.0.0/16" { - t.Errorf("IPRanges = %v, want [10.5.0.0/16]", cfg.Server.IPRanges) + if len(cfg.Server.IPRanges) != 1 || cfg.Server.IPRanges[0].Net.String() != "10.5.0.0/16" || cfg.Server.IPRanges[0].Label != "" { + t.Errorf("IPRanges = %v, want one unlabelled 10.5.0.0/16", cfg.Server.IPRanges) } // PoPs live in unrelated blocks, so a comma-separated list is accepted; @@ -63,6 +63,26 @@ func TestNewParsesIPRangeAsCIDR(t *testing.T) { t.Fatal("expected an error when one list entry is not CIDR") } + // Entries may carry an operator-facing label; it never affects matching. + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "tor1=198.51.100.7/32, lab = 203.0.113.0/24,192.0.2.0/24") + cfg, err = New() + if err != nil { + t.Fatalf("unexpected error: %v", err) + } + if got := cfg.Server.IPRangesString(); got != "tor1 198.51.100.7/32, lab 203.0.113.0/24, 192.0.2.0/24" { + t.Errorf("IPRangesString() = %q", got) + } + if !cfg.Server.ContainsIP(net.ParseIP("198.51.100.7")) || !cfg.Server.ContainsIP(net.ParseIP("192.0.2.9")) { + t.Errorf("labelled and unlabelled entries must both match: %v", cfg.Server.IPRanges) + } + + for _, bad := range []string{"=198.51.100.7/32", "tor1=", "tor1=10.5."} { + t.Setenv("DNS_AUTH_SERVER_IP_RANGE", bad) + if _, err := New(); err == nil { + t.Errorf("expected an error for DNS_AUTH_SERVER_IP_RANGE=%q", bad) + } + } + // The range is what makes a query "ours"; without it every check would // depend on the ASN alone, so an unset value is a boot error. t.Setenv("DNS_AUTH_SERVER_IP_RANGE", "") diff --git a/dnscheck/dns/handler_geolookup_test.go b/dnscheck/dns/handler_geolookup_test.go index 0fb8e1e7..ca591699 100644 --- a/dnscheck/dns/handler_geolookup_test.go +++ b/dnscheck/dns/handler_geolookup_test.go @@ -70,7 +70,10 @@ func newTestHandler(t *testing.T, geo GeoLookuper, cache *memCache) *Handler { Domain: testDomain, IPAddress: "192.0.2.1", ASN: testOurASN, - IPRanges: []*net.IPNet{mustCIDR(t, "198.51.100.0/24"), mustCIDR(t, "192.0.2.77/32")}, + IPRanges: []config.IPRange{ + {Label: "lab", Net: mustCIDR(t, "198.51.100.0/24")}, + {Net: mustCIDR(t, "192.0.2.77/32")}, + }, }, Cache: &config.CacheConfig{HMACKey: "test-key"}, }, diff --git a/dnscheck/main.go b/dnscheck/main.go index edc90d61..a5676d63 100644 --- a/dnscheck/main.go +++ b/dnscheck/main.go @@ -23,6 +23,10 @@ func main() { if err != nil { log.Fatal().Err(err).Msg("Failed to read app configuration") } + // Infrastructure addresses, not client data: safe to log so operators can + // confirm which PoPs a running instance trusts. + log.Info().Str("ip_ranges", cfg.Server.IPRangesString()).Uint("asn", cfg.Server.ASN). + Msg("Trusted resolver sources") cache, err := cache.New(cache.CacheTypeBigCache, cfg.Cache.TTL) if err != nil { From 84d95f8bde76f50255bf989282ee52b1248b4a7c Mon Sep 17 00:00:00 2001 From: Maciek Date: Mon, 7 Sep 2026 12:34:32 +0200 Subject: [PATCH 12/52] fix(app,dnscheck): stop embedding the profile ID in the connection-check hostname Signed-off-by: Maciek --- app/src/hooks/useDnsConnectionStatus.ts | 8 +++--- dnscheck/api/check_test.go | 26 +++++++++++++++--- dnscheck/dns/handler.go | 18 +++++-------- dnscheck/dns/handler_geolookup_test.go | 36 ++++++++++++++++++++++++- 4 files changed, 68 insertions(+), 20 deletions(-) diff --git a/app/src/hooks/useDnsConnectionStatus.ts b/app/src/hooks/useDnsConnectionStatus.ts index b2b9dadd..5d34a27f 100644 --- a/app/src/hooks/useDnsConnectionStatus.ts +++ b/app/src/hooks/useDnsConnectionStatus.ts @@ -35,9 +35,10 @@ export function useDnsConnectionStatus(pollMs: number = 5000, options?: { enable setError(''); const alphabet = '0123456789ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz'; const nanoid = customAlphabet(alphabet, 12); - const randID = nanoid(); - const profileId = activeProfile?.profile_id || ''; - const subdomain = `${randID}-${profileId}`; + // The probe name is pure randomness. The profile ID is reported back by + // the proxy (EDNS0), never sent in the hostname, where it would be visible + // to the resolver chain and in the TLS SNI. + const subdomain = nanoid(); const dnsCheckDomain = import.meta.env.VITE_DNS_CHECK_DOMAIN || 'test.moddns.net'; const url = `https://${subdomain}.${dnsCheckDomain}/`; const response = await axios.get(url); @@ -71,7 +72,6 @@ export function useDnsConnectionStatus(pollMs: number = 5000, options?: { enable executeDnsCheck(); intervalRef.current = setInterval(() => executeDnsCheck(), pollMs); return () => { if (intervalRef.current) clearInterval(intervalRef.current); }; - // eslint-disable-next-line react-hooks/exhaustive-deps }, [activeProfile?.profile_id, enabled, pollMs]); const getCurrentProfileName = () => { diff --git a/dnscheck/api/check_test.go b/dnscheck/api/check_test.go index 688cd1fc..6cbf7afa 100644 --- a/dnscheck/api/check_test.go +++ b/dnscheck/api/check_test.go @@ -41,7 +41,7 @@ func (c *memCache) DeleteQueryData(key string) error { delete(c.saved, key); ret const ( testHMACKey = "test-key" - testSubdomain = "abcdefghijkl-profile1" + testSubdomain = "abcdefghijkl" testHost = testSubdomain + ".check.example.test" ) @@ -82,9 +82,27 @@ func TestDnsCheckRejectsHostWithoutSubdomain(t *testing.T) { // specRef: dnscheck-behaviour.md #A1 func TestDnsCheckRejectsMalformedSubdomain(t *testing.T) { - resp, _ := get(t, newTestServer(&memCache{}), "short.check.example.test") - if resp.StatusCode != http.StatusBadRequest { - t.Errorf("status = %d, want 400", resp.StatusCode) + for _, label := range []string{"short", "abcdefghijklm", "abcdefghijkl-", "abcdefghij_l"} { + resp, _ := get(t, newTestServer(&memCache{}), label+".check.example.test") + if resp.StatusCode != http.StatusBadRequest { + t.Errorf("label %q: status = %d, want 400", label, resp.StatusCode) + } + } +} + +// The previous frontend bundle appended "-"; it keeps working until +// every client has picked up the new bundle. +// +// specRef: dnscheck-behaviour.md #A1 +func TestDnsCheckToleratesLegacySuffix(t *testing.T) { + c := &memCache{} + rec, _ := json.Marshal(dns.DNSLogRecord{Status: dns.StatusConfigured, ProfileId: "profile1"}) + legacy := testSubdomain + "-profile1" + _ = c.SaveQueryData(cache.HMACKey(testHMACKey, legacy), rec) + + resp, _ := get(t, newTestServer(c), legacy+".check.example.test") + if resp.StatusCode != http.StatusOK { + t.Errorf("status = %d, want 200 for a legacy-format label", resp.StatusCode) } } diff --git a/dnscheck/dns/handler.go b/dnscheck/dns/handler.go index 13c9b0da..e6921fb3 100644 --- a/dnscheck/dns/handler.go +++ b/dnscheck/dns/handler.go @@ -15,13 +15,16 @@ import ( ) const ( - // SubdomainRegexPattern validates the expected dnscheck subdomain format: - // 12 alphanumeric chars (nanoid), a dash, then the profile ID. - SubdomainRegexPattern = `^[a-zA-Z0-9]{12}-[a-zA-Z0-9-]+$` + // SubdomainRegexPattern validates the dnscheck probe label: 12 alphanumeric + // chars (nanoid). A "-suffix" is tolerated for clients still running the + // previous frontend bundle, which appended the profile ID. + SubdomainRegexPattern = `^[a-zA-Z0-9]{12}(-[a-zA-Z0-9-]+)?$` ProfileIdAdditionalSectionCode = 0xfeed TTL = 300 ) +var subdomainRegex = regexp.MustCompile(SubdomainRegexPattern) + type Handler struct { srv *DNSServer } @@ -62,14 +65,7 @@ func (h *Handler) ServeDNS(w dns.ResponseWriter, r *dns.Msg) { if strings.Contains(domain, h.srv.Config.Server.Domain) { subdomain := strings.Split(domain, ".")[0] - // Regex to identify the subdomain with the first part being exactly 12 characters - matched, err := regexp.MatchString(SubdomainRegexPattern, subdomain) - if err != nil { - log.Error().Err(err).Msg("Failed to compile regex") - return - } - - if !matched { + if !subdomainRegex.MatchString(subdomain) { log.Warn().Msg("Unidentified subdomain") return } diff --git a/dnscheck/dns/handler_geolookup_test.go b/dnscheck/dns/handler_geolookup_test.go index ca591699..293249bc 100644 --- a/dnscheck/dns/handler_geolookup_test.go +++ b/dnscheck/dns/handler_geolookup_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "errors" "net" + "os" "strings" "testing" @@ -17,7 +18,7 @@ import ( const ( testDomain = "check.example.test" - testSubdomain = "abcdefghijkl-profile1" + testSubdomain = "abcdefghijkl" testOurASN = 64512 ) @@ -54,6 +55,13 @@ func (w *tcpCaptureWriter) RemoteAddr() net.Addr { } func (w *tcpCaptureWriter) Network() string { return "tcp" } +// Handler logs are noise in test output; the log-hygiene test re-enables them +// on its own buffer. +func TestMain(m *testing.M) { + zerolog.SetGlobalLevel(zerolog.Disabled) + os.Exit(m.Run()) +} + func mustCIDR(t *testing.T, s string) *net.IPNet { t.Helper() _, n, err := net.ParseCIDR(s) @@ -296,6 +304,32 @@ func TestServeDNSLogsCarryNoClientIdentifiers(t *testing.T) { } } +// specRef: dnscheck-behaviour.md #D3 +func TestServeDNSProbeLabelFormat(t *testing.T) { + cases := []struct { + label string + saved bool + }{ + {"abcdefghijkl", true}, // current frontend: bare nanoid + {"ABCdef123456", true}, // mixed alphabet + {"abcdefghijkl-profile1", true}, // previous frontend bundle: tolerated + {"abcdefghijk", false}, // 11 chars + {"abcdefghijklm", false}, // 13 chars + {"abcdefghijkl-", false}, // dangling separator + {"abcdefghij_l", false}, // non-alphanumeric + } + for _, tc := range cases { + cache := &memCache{} + h := newTestHandler(t, &fakeGeoLookup{result: &maxmind.GeoLookup{}}, cache) + req := new(dns.Msg) + req.SetQuestion(tc.label+"."+testDomain+".", dns.TypeA) + h.ServeDNS(&captureWriter{}, req) + if got := len(cache.saved) == 1; got != tc.saved { + t.Errorf("label %q: record saved = %v, want %v", tc.label, got, tc.saved) + } + } +} + // A queries outside the check domain get the authoritative A answer and leave no // trace: no lookup, no cache entry. // From 21db4eab21c8bcf0530b865ea609c2fabda69a04 Mon Sep 17 00:00:00 2001 From: Maciek Date: Mon, 7 Sep 2026 13:47:11 +0200 Subject: [PATCH 13/52] test(e2e): unskip the dnscheck connection-status suite and route it inside the test stack Signed-off-by: Maciek --- tests/dns_tests/test_connection_status.py | 642 ++++------------------ tests/docker-compose.yml | 9 +- tests/libs/settings.py | 3 + 3 files changed, 112 insertions(+), 542 deletions(-) diff --git a/tests/dns_tests/test_connection_status.py b/tests/dns_tests/test_connection_status.py index e6db177f..48ea9d89 100644 --- a/tests/dns_tests/test_connection_status.py +++ b/tests/dns_tests/test_connection_status.py @@ -1,572 +1,134 @@ -""" -Backend E2E tests for DNS Connection Status Check feature. +"""Backend E2E tests for the DNS connection-status check (dnscheck). + +Flow under test, end to end through public interfaces only: + +1. DoH query for ``