dnsforward: allowed clients private nets

CHANGELOG.md

@@ -30,6 +30,9 @@ NOTE: Add new changes BELOW THIS COMMENT.
 
 ### Fixed
 
+- Private networks 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.

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"
@@ -30,6 +31,9 @@ type accessManager struct {
 
 	blockedHostsEng *urlfilter.DNSEngine
 
+	// privateNets is the set of IP networks those are ignored by the manager.
+	privateNets netutil.SubnetSet
+
 	// TODO(a.garipov): Create a type for a set of IP networks.
 	allowedNets []netip.Prefix
 	blockedNets []netip.Prefix
@@ -64,13 +68,20 @@ func processAccessClients(
 }
 
 // newAccessCtx creates a new accessCtx.
-func newAccessCtx(allowed, blocked, blockedHosts []string) (a *accessManager, err error) {
+func newAccessCtx(
+	allowed []string,
+	blocked []string,
+	blockedHosts []string,
+	privateNets netutil.SubnetSet,
+) (a *accessManager, err error) {
 	a = &accessManager{
 		allowedIPs: map[netip.Addr]unit{},
 		blockedIPs: map[netip.Addr]unit{},
 
 		allowedClientIDs: stringutil.NewSet(),
 		blockedClientIDs: stringutil.NewSet(),
+
+		privateNets: privateNets,
 	}
 
 	err = processAccessClients(allowed, a.allowedIPs, &a.allowedNets, a.allowedClientIDs)
@@ -138,9 +149,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.  Addresses from private nets are always allowed.
 func (a *accessManager) isBlockedIP(ip netip.Addr) (blocked bool, rule string) {
+	if a.privateNets != nil && a.privateNets.Contains(ip.AsSlice()) {
+		return false, ""
+	}
+
 	blocked = true
 	ips := a.blockedIPs
 	ipnets := a.blockedNets
@@ -241,7 +256,7 @@ func (s *Server) handleAccessSet(w http.ResponseWriter, r *http.Request) {
 	}
 
 	var a *accessManager
-	a, err = newAccessCtx(list.AllowedClients, list.DisallowedClients, list.BlockedHosts)
+	a, err = newAccessCtx(list.AllowedClients, list.DisallowedClients, list.BlockedHosts, nil)
 	if err != nil {
 		aghhttp.Error(r, w, http.StatusBadRequest, "creating access ctx: %s", err)
 

internal/dnsforward/access_test.go

@@ -4,6 +4,7 @@ import (
 	"net/netip"
 	"testing"
 
+	"github.com/AdguardTeam/golibs/netutil"
 	"github.com/AdguardTeam/urlfilter/rules"
 	"github.com/miekg/dns"
 	"github.com/stretchr/testify/assert"
@@ -14,12 +15,12 @@ func TestIsBlockedClientID(t *testing.T) {
 	clientID := "client-1"
 	clients := []string{clientID}
 
-	a, err := newAccessCtx(clients, nil, nil)
+	a, err := newAccessCtx(clients, nil, nil, nil)
 	require.NoError(t, err)
 
 	assert.False(t, a.isBlockedClientID(clientID))
 
-	a, err = newAccessCtx(nil, clients, nil)
+	a, err = newAccessCtx(nil, clients, nil, nil)
 	require.NoError(t, err)
 
 	assert.True(t, a.isBlockedClientID(clientID))
@@ -31,7 +32,7 @@ func TestIsBlockedHost(t *testing.T) {
 		"*.host.com",
 		"||host3.com^",
 		"||*^$dnstype=HTTPS",
-	})
+	}, nil)
 	require.NoError(t, err)
 
 	testCases := []struct {
@@ -103,58 +104,104 @@ 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",
 	}
 
-	allowCtx, err := newAccessCtx(clients, nil, nil)
-	require.NoError(t, err)
-
-	blockCtx, err := newAccessCtx(nil, clients, nil)
+	privateNets := netutil.SubnetSetFunc(netutil.IsLocallyServed)
+	allowCtx, err := newAccessCtx(clients, nil, nil, privateNets)
 	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.False,
 	}, {
-		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.False,
 	}, {
-		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.True,
 	}, {
-		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.True,
+	}, {
+		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 {
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
 			blocked, rule := allowCtx.isBlockedIP(tc.ip)
-			assert.Equal(t, !tc.wantBlocked, blocked)
+			tc.want(t, blocked)
 			assert.Equal(t, tc.wantRule, rule)
-		}
-	})
-
-	t.Run("block", func(t *testing.T) {
-		for _, tc := range testCases {
-			blocked, rule := blockCtx.isBlockedIP(tc.ip)
-			assert.Equal(t, tc.wantBlocked, 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",
+	}
+
+	privateNets := netutil.SubnetSetFunc(netutil.IsLocallyServed)
+	blockCtx, err := newAccessCtx(nil, clients, nil, privateNets)
+	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.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: "",
+		want:     assert.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,
+	}}
+
+	for _, tc := range testCases {
+		t.Run(tc.name, func(t *testing.T) {
+			blocked, rule := blockCtx.isBlockedIP(tc.ip)
+			tc.want(t, blocked)
+			assert.Equal(t, tc.wantRule, rule)
+		})
+	}
 }

internal/dnsforward/dnsforward.go

@@ -509,6 +509,7 @@ func (s *Server) Prepare(conf *ServerConfig) (err error) {
 		s.conf.AllowedClients,
 		s.conf.DisallowedClients,
 		s.conf.BlockedHosts,
+		s.privateNets,
 	)
 	if err != nil {
 		return fmt.Errorf("preparing access: %w", err)
@@ -700,10 +701,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.