From 9caf0d54c607e284f14862b40a10ba7170432d37 Mon Sep 17 00:00:00 2001
From: Dimitry Kolyshev <dkolyshev@adguard.com>
Date: Thu, 18 May 2023 11:32:35 +0300
Subject: [PATCH] dnsforward: allowed clients local

---
 CHANGELOG.md                       |   4 ++
 internal/dnsforward/access.go      |   9 ++-
 internal/dnsforward/access_test.go | 112 ++++++++++++++++++++---------
 internal/dnsforward/dnsforward.go  |   2 +
 4 files changed, 91 insertions(+), 36 deletions(-)

diff --git a/CHANGELOG.md b/CHANGELOG.md
index 58dd6c16..611cb089 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -30,6 +30,9 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Fixed
 
+- Locally served address are now ignored from allowed/blocked clients access
+  lists.  These addresses are always allowed ([#5799]).
+
 - Unquoted IPv6 bind hosts with trailing colons erroneously considered
   unspecified addresses are now properly validated ([#5752]).
 
@@ -42,6 +45,7 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 [#1577]: https://github.com/AdguardTeam/AdGuardHome/issues/1577
 [#5716]: https://github.com/AdguardTeam/AdGuardHome/issues/5716
+[#5799]: https://github.com/AdguardTeam/AdGuardHome/issues/5799
 
 <!--
 NOTE: Add new changes ABOVE THIS COMMENT.
diff --git a/internal/dnsforward/access.go b/internal/dnsforward/access.go
index 12f5f3c7..08e12c3f 100644
--- a/internal/dnsforward/access.go
+++ b/internal/dnsforward/access.go
@@ -10,6 +10,7 @@ import (
 	"github.com/AdguardTeam/AdGuardHome/internal/aghalg"
 	"github.com/AdguardTeam/AdGuardHome/internal/aghhttp"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/golibs/stringutil"
 	"github.com/AdguardTeam/urlfilter"
 	"github.com/AdguardTeam/urlfilter/filterlist"
@@ -138,9 +139,13 @@ func (a *accessManager) isBlockedHost(host string, qt rules.RRType) (ok bool) {
 	return ok
 }
 
-// isBlockedIP returns the status of the IP address blocking as well as the rule
-// that blocked it.
+// isBlockedIP returns the status of the IP address blocking as well as the
+// rule that blocked it.  Locally served addresses are always allowed.
 func (a *accessManager) isBlockedIP(ip netip.Addr) (blocked bool, rule string) {
+	if netutil.IsLocallyServedAddr(ip) {
+		return false, ""
+	}
+
 	blocked = true
 	ips := a.blockedIPs
 	ipnets := a.blockedNets
diff --git a/internal/dnsforward/access_test.go b/internal/dnsforward/access_test.go
index d5d7da26..ff5af594 100644
--- a/internal/dnsforward/access_test.go
+++ b/internal/dnsforward/access_test.go
@@ -103,7 +103,7 @@ func TestIsBlockedHost(t *testing.T) {
 	}
 }
 
-func TestIsBlockedIP(t *testing.T) {
+func TestAccessManager_IsBlockedIP_allow(t *testing.T) {
 	clients := []string{
 		"1.2.3.4",
 		"5.6.7.8/24",
@@ -112,49 +112,93 @@ func TestIsBlockedIP(t *testing.T) {
 	allowCtx, err := newAccessCtx(clients, nil, nil)
 	require.NoError(t, err)
 
+	testCases := []struct {
+		ip       netip.Addr
+		want     assert.BoolAssertionFunc
+		name     string
+		wantRule string
+	}{{
+		ip:       netip.MustParseAddr("1.2.3.4"),
+		name:     "match_ip",
+		wantRule: "1.2.3.4",
+		want:     assert.False,
+	}, {
+		ip:       netip.MustParseAddr("5.6.7.100"),
+		name:     "match_cidr",
+		wantRule: "5.6.7.8/24",
+		want:     assert.False,
+	}, {
+		ip:       netip.MustParseAddr("9.2.3.4"),
+		name:     "no_match_ip",
+		wantRule: "",
+		want:     assert.True,
+	}, {
+		ip:       netip.MustParseAddr("9.6.7.100"),
+		name:     "no_match_cidr",
+		wantRule: "",
+		want:     assert.True,
+	}, {
+		ip:       netip.MustParseAddr("127.0.0.1"),
+		name:     "locally_served_ip",
+		wantRule: "",
+		want:     assert.False,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			blocked, rule := allowCtx.isBlockedIP(tc.ip)
+			tc.want(t, blocked)
+			assert.Equal(t, tc.wantRule, rule)
+		})
+	}
+}
+
+func TestAccessManager_IsBlockedIP_block(t *testing.T) {
+	clients := []string{
+		"1.2.3.4",
+		"5.6.7.8/24",
+	}
+
 	blockCtx, err := newAccessCtx(nil, clients, nil)
 	require.NoError(t, err)
 
 	testCases := []struct {
-		ip          netip.Addr
-		name        string
-		wantRule    string
-		wantBlocked bool
+		ip       netip.Addr
+		want     assert.BoolAssertionFunc
+		name     string
+		wantRule string
 	}{{
-		ip:          netip.MustParseAddr("1.2.3.4"),
-		name:        "match_ip",
-		wantRule:    "1.2.3.4",
-		wantBlocked: true,
+		ip:       netip.MustParseAddr("1.2.3.4"),
+		name:     "match_ip",
+		wantRule: "1.2.3.4",
+		want:     assert.True,
 	}, {
-		ip:          netip.MustParseAddr("5.6.7.100"),
-		name:        "match_cidr",
-		wantRule:    "5.6.7.8/24",
-		wantBlocked: true,
+		ip:       netip.MustParseAddr("5.6.7.100"),
+		name:     "match_cidr",
+		wantRule: "5.6.7.8/24",
+		want:     assert.True,
 	}, {
-		ip:          netip.MustParseAddr("9.2.3.4"),
-		name:        "no_match_ip",
-		wantRule:    "",
-		wantBlocked: false,
+		ip:       netip.MustParseAddr("9.2.3.4"),
+		name:     "no_match_ip",
+		wantRule: "",
+		want:     assert.False,
 	}, {
-		ip:          netip.MustParseAddr("9.6.7.100"),
-		name:        "no_match_cidr",
-		wantRule:    "",
-		wantBlocked: false,
+		ip:       netip.MustParseAddr("9.6.7.100"),
+		name:     "no_match_cidr",
+		wantRule: "",
+		want:     assert.False,
+	}, {
+		ip:       netip.MustParseAddr("127.0.0.1"),
+		name:     "locally_served_ip",
+		wantRule: "",
+		want:     assert.False,
 	}}
 
-	t.Run("allow", func(t *testing.T) {
-		for _, tc := range testCases {
-			blocked, rule := allowCtx.isBlockedIP(tc.ip)
-			assert.Equal(t, !tc.wantBlocked, blocked)
-			assert.Equal(t, tc.wantRule, rule)
-		}
-	})
-
-	t.Run("block", func(t *testing.T) {
-		for _, tc := range testCases {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			blocked, rule := blockCtx.isBlockedIP(tc.ip)
-			assert.Equal(t, tc.wantBlocked, blocked)
+			tc.want(t, blocked)
 			assert.Equal(t, tc.wantRule, rule)
-		}
-	})
+		})
+	}
 }
diff --git a/internal/dnsforward/dnsforward.go b/internal/dnsforward/dnsforward.go
index 887fff41..1aeea73d 100644
--- a/internal/dnsforward/dnsforward.go
+++ b/internal/dnsforward/dnsforward.go
@@ -700,10 +700,12 @@ func (s *Server) IsBlockedClient(ip netip.Addr, clientID string) (blocked bool,
 	blockedByIP := false
 	if ip != (netip.Addr{}) {
 		blockedByIP, rule = s.access.isBlockedIP(ip)
+		log.Debug("by ip %v", blockedByIP)
 	}
 
 	allowlistMode := s.access.allowlistMode()
 	blockedByClientID := s.access.isBlockedClientID(clientID)
+	log.Debug("by client id %v", blockedByClientID)
 
 	// Allow if at least one of the checks allows in allowlist mode, but block
 	// if at least one of the checks blocks in blocklist mode.