Pull request 2166: 5829-trusted-ip

Updates #5829.

Squashed commit of the following:

commit 8a93b30d5bd1c40c30bd10cd3fc77c3a3a64cb71
Merge: 8e4429c48 54f77c010
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Mar 20 19:15:07 2024 +0300

    Merge branch 'master' into 5829-trusted-ip

commit 8e4429c483c0fd6fffdc93fa808adcca6678bc3e
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Mar 20 18:37:26 2024 +0300

    all: upd chlog

commit b598a8d1ea239cc574bfdfdd6a2da47792582589
Merge: 1f58bf8fd 054233962
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Mar 20 18:34:13 2024 +0300

    Merge branch 'master' into 5829-trusted-ip

commit 1f58bf8fd1bc3b3790475651cb87494885cadf66
Merge: ffb4b9a65 c64a36c94
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Wed Mar 20 17:09:09 2024 +0300

    Merge branch 'master' into 5829-trusted-ip

commit ffb4b9a65fea5555d0d401194d3fc3820b2e6766
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Mar 14 17:40:07 2024 +0300

    home: fix alignment

commit 7f11807ff13eff286be1d3bd4b796273454bdbda
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Thu Mar 14 17:35:13 2024 +0300

    all: imp code

commit 2aee9a66c70af929e28653245eb73c0f29a46e97
Author: Stanislav Chzhen <s.chzhen@adguard.com>
Date:   Mon Mar 11 18:17:58 2024 +0300

    home: real ip in logs

CHANGELOG.md

@@ -23,10 +23,17 @@ See also the [v0.107.47 GitHub milestone][ms-v0.107.47].
 NOTE: Add new changes BELOW THIS COMMENT.
 -->
 
+### Changed
+
+- Failed authentication attempts show the originating IP address in the logs, if
+  the request came from a trusted proxy ([#5829]).
+
 ### Deprecated
 
 - Node.JS 16.  Future versions will require at least Node.JS 18 to build.
 
+[#5829]: https://github.com/AdguardTeam/AdGuardHome/issues/5829
+
 <!--
 NOTE: Add new changes ABOVE THIS COMMENT.
 -->

internal/home/auth.go

@@ -11,6 +11,7 @@ import (
 
 	"github.com/AdguardTeam/golibs/errors"
 	"github.com/AdguardTeam/golibs/log"
+	"github.com/AdguardTeam/golibs/netutil"
 	"go.etcd.io/bbolt"
 	"golang.org/x/crypto/bcrypt"
 )
@@ -51,14 +52,15 @@ func (s *session) deserialize(data []byte) bool {
 	return true
 }
 
-// Auth - global object
+// Auth is the global authentication object.
 type Auth struct {
-	db          *bbolt.DB
+	trustedProxies netutil.SubnetSet
-	rateLimiter *authRateLimiter
+	db             *bbolt.DB
-	sessions    map[string]*session
+	rateLimiter    *authRateLimiter
-	users       []webUser
+	sessions       map[string]*session
-	lock        sync.Mutex
+	users          []webUser
-	sessionTTL  uint32
+	lock           sync.Mutex
+	sessionTTL     uint32
 }
 
 // webUser represents a user of the Web UI.
@@ -69,15 +71,22 @@ type webUser struct {
 	PasswordHash string `yaml:"password"`
 }
 
-// InitAuth - create a global object
+// InitAuth initializes the global authentication object.
-func InitAuth(dbFilename string, users []webUser, sessionTTL uint32, rateLimiter *authRateLimiter) *Auth {
+func InitAuth(
+	dbFilename string,
+	users []webUser,
+	sessionTTL uint32,
+	rateLimiter *authRateLimiter,
+	trustedProxies netutil.SubnetSet,
+) (a *Auth) {
 	log.Info("Initializing auth module: %s", dbFilename)
 
-	a := &Auth{
+	a = &Auth{
-		sessionTTL:  sessionTTL,
+		sessionTTL:     sessionTTL,
-		rateLimiter: rateLimiter,
+		rateLimiter:    rateLimiter,
-		sessions:    make(map[string]*session),
+		sessions:       make(map[string]*session),
-		users:       users,
+		users:          users,
+		trustedProxies: trustedProxies,
 	}
 	var err error
 	a.db, err = bbolt.Open(dbFilename, 0o644, nil)
@@ -95,7 +104,7 @@ func InitAuth(dbFilename string, users []webUser, sessionTTL uint32, rateLimiter
 	return a
 }
 
-// Close - close module
+// Close closes the authentication database.
 func (a *Auth) Close() {
 	_ = a.db.Close()
 }
@@ -104,7 +113,8 @@ func bucketName() []byte {
 	return []byte("sessions-2")
 }
 
-// load sessions from file, remove expired sessions
+// loadSessions loads sessions from the database file and removes expired
+// sessions.
 func (a *Auth) loadSessions() {
 	tx, err := a.db.Begin(true)
 	if err != nil {
@@ -156,7 +166,8 @@ func (a *Auth) loadSessions() {
 	log.Debug("auth: loaded %d sessions from DB (removed %d expired)", len(a.sessions), removed)
 }
 
-// store session data in file
+// addSession adds a new session to the list of sessions and saves it in the
+// database file.
 func (a *Auth) addSession(data []byte, s *session) {
 	name := hex.EncodeToString(data)
 	a.lock.Lock()
@@ -167,7 +178,7 @@ func (a *Auth) addSession(data []byte, s *session) {
 	}
 }
 
-// store session data in file
+// storeSession saves a session in the database file.
 func (a *Auth) storeSession(data []byte, s *session) bool {
 	tx, err := a.db.Begin(true)
 	if err != nil {

internal/home/auth_internal_test.go

@@ -37,7 +37,7 @@ func TestAuth(t *testing.T) {
 		Name:         "name",
 		PasswordHash: "$2y$05$..vyzAECIhJPfaQiOK17IukcQnqEgKJHy0iETyYqxn3YXJl8yZuo2",
 	}}
-	a := InitAuth(fn, nil, 60, nil)
+	a := InitAuth(fn, nil, 60, nil, nil)
 	s := session{}
 
 	user := webUser{Name: "name"}
@@ -66,7 +66,7 @@ func TestAuth(t *testing.T) {
 	a.Close()
 
 	// load saved session
-	a = InitAuth(fn, users, 60, nil)
+	a = InitAuth(fn, users, 60, nil, nil)
 
 	// the session is still alive
 	assert.Equal(t, checkSessionOK, a.checkSession(sessStr))
@@ -82,7 +82,7 @@ func TestAuth(t *testing.T) {
 	time.Sleep(3 * time.Second)
 
 	// load and remove expired sessions
-	a = InitAuth(fn, users, 60, nil)
+	a = InitAuth(fn, users, 60, nil, nil)
 	assert.Equal(t, checkSessionNotFound, a.checkSession(sessStr))
 
 	a.Close()

internal/home/authhttp.go

@@ -4,8 +4,8 @@ import (
 	"encoding/hex"
 	"encoding/json"
 	"fmt"
-	"net"
 	"net/http"
+	"net/netip"
 	"path"
 	"strconv"
 	"strings"
@@ -78,7 +78,7 @@ func (a *Auth) newCookie(req loginJSON, addr string) (c *http.Cookie, err error)
 // a well-maintained third-party module.
 //
 // TODO(a.garipov): Support header Forwarded from RFC 7329.
-func realIP(r *http.Request) (ip net.IP, err error) {
+func realIP(r *http.Request) (ip netip.Addr, err error) {
 	proxyHeaders := []string{
 		httphdr.CFConnectingIP,
 		httphdr.TrueClientIP,
@@ -87,8 +87,8 @@ func realIP(r *http.Request) (ip net.IP, err error) {
 
 	for _, h := range proxyHeaders {
 		v := r.Header.Get(h)
-		ip = net.ParseIP(v)
+		ip, err = netip.ParseAddr(v)
-		if ip != nil {
+		if err == nil {
 			return ip, nil
 		}
 	}
@@ -96,20 +96,20 @@ func realIP(r *http.Request) (ip net.IP, err error) {
 	// If none of the above yielded any results, get the leftmost IP address
 	// from the X-Forwarded-For header.
 	s := r.Header.Get(httphdr.XForwardedFor)
-	ipStrs := strings.SplitN(s, ", ", 2)
+	ipStr, _, _ := strings.Cut(s, ",")
-	ip = net.ParseIP(ipStrs[0])
+	ip, err = netip.ParseAddr(ipStr)
-	if ip != nil {
+	if err == nil {
 		return ip, nil
 	}
 
 	// When everything else fails, just return the remote address as understood
 	// by the stdlib.
-	ipStr, err := netutil.SplitHost(r.RemoteAddr)
+	ipStr, err = netutil.SplitHost(r.RemoteAddr)
 	if err != nil {
-		return nil, fmt.Errorf("getting ip from client addr: %w", err)
+		return netip.Addr{}, fmt.Errorf("getting ip from client addr: %w", err)
 	}
 
-	return net.ParseIP(ipStr), nil
+	return netip.ParseAddr(ipStr)
 }
 
 // writeErrorWithIP is like [aghhttp.Error], but includes the remote IP address
@@ -142,8 +142,6 @@ func handleLogin(w http.ResponseWriter, r *http.Request) {
 	// to security issues.
 	//
 	// See https://github.com/AdguardTeam/AdGuardHome/issues/2799.
-	//
-	// TODO(e.burkov): Use realIP when the issue will be fixed.
 	if remoteIP, err = netutil.SplitHost(r.RemoteAddr); err != nil {
 		writeErrorWithIP(
 			r,
@@ -173,20 +171,24 @@ func handleLogin(w http.ResponseWriter, r *http.Request) {
 		}
 	}
 
-	cookie, err := Context.auth.newCookie(req, remoteIP)
-	if err != nil {
-		writeErrorWithIP(r, w, http.StatusForbidden, remoteIP, "%s", err)
-
-		return
-	}
-
-	// Use realIP here, since this IP address is only used for logging.
 	ip, err := realIP(r)
 	if err != nil {
 		log.Error("auth: getting real ip from request with remote ip %s: %s", remoteIP, err)
 	}
 
-	log.Info("auth: user %q successfully logged in from ip %v", req.Name, ip)
+	cookie, err := Context.auth.newCookie(req, remoteIP)
+	if err != nil {
+		logIP := remoteIP
+		if Context.auth.trustedProxies.Contains(ip.Unmap()) {
+			logIP = ip.String()
+		}
+
+		writeErrorWithIP(r, w, http.StatusForbidden, logIP, "%s", err)
+
+		return
+	}
+
+	log.Info("auth: user %q successfully logged in from ip %s", req.Name, ip)
 
 	http.SetCookie(w, cookie)
 

internal/home/authhttp_internal_test.go

@@ -1,8 +1,8 @@
 package home
 
 import (
-	"net"
 	"net/http"
+	"net/netip"
 	"net/textproto"
 	"net/url"
 	"path/filepath"
@@ -39,7 +39,7 @@ func TestAuthHTTP(t *testing.T) {
 	users := []webUser{
 		{Name: "name", PasswordHash: "$2y$05$..vyzAECIhJPfaQiOK17IukcQnqEgKJHy0iETyYqxn3YXJl8yZuo2"},
 	}
-	Context.auth = InitAuth(fn, users, 60, nil)
+	Context.auth = InitAuth(fn, users, 60, nil, nil)
 
 	handlerCalled := false
 	handler := func(_ http.ResponseWriter, _ *http.Request) {
@@ -125,13 +125,13 @@ func TestRealIP(t *testing.T) {
 		header     http.Header
 		remoteAddr string
 		wantErrMsg string
-		wantIP     net.IP
+		wantIP     netip.Addr
 	}{{
 		name:       "success_no_proxy",
 		header:     nil,
 		remoteAddr: remoteAddr,
 		wantErrMsg: "",
-		wantIP:     net.IPv4(1, 2, 3, 4),
+		wantIP:     netip.MustParseAddr("1.2.3.4"),
 	}, {
 		name: "success_proxy",
 		header: http.Header{
@@ -139,7 +139,7 @@ func TestRealIP(t *testing.T) {
 		},
 		remoteAddr: remoteAddr,
 		wantErrMsg: "",
-		wantIP:     net.IPv4(1, 2, 3, 5),
+		wantIP:     netip.MustParseAddr("1.2.3.5"),
 	}, {
 		name: "success_proxy_multiple",
 		header: http.Header{
@@ -149,14 +149,14 @@ func TestRealIP(t *testing.T) {
 		},
 		remoteAddr: remoteAddr,
 		wantErrMsg: "",
-		wantIP:     net.IPv4(1, 2, 3, 6),
+		wantIP:     netip.MustParseAddr("1.2.3.6"),
 	}, {
 		name:       "error_no_proxy",
 		header:     nil,
 		remoteAddr: "1:::2",
 		wantErrMsg: `getting ip from client addr: address 1:::2: ` +
 			`too many colons in address`,
-		wantIP: nil,
+		wantIP: netip.Addr{},
 	}}
 
 	for _, tc := range testCases {

internal/home/home.go

@@ -674,8 +674,10 @@ func initUsers() (auth *Auth, err error) {
 		log.Info("authratelimiter is disabled")
 	}
 
+	trustedProxies := netutil.SliceSubnetSet(netutil.UnembedPrefixes(config.DNS.TrustedProxies))
+
 	sessionTTL := config.HTTPConfig.SessionTTL.Seconds()
-	auth = InitAuth(sessFilename, config.Users, uint32(sessionTTL), rateLimiter)
+	auth = InitAuth(sessFilename, config.Users, uint32(sessionTTL), rateLimiter, trustedProxies)
 	if auth == nil {
 		return nil, errors.Error("initializing auth module failed")
 	}