From 3b12ff2cc2a0720fc74f6f501ed66c5437977d39 Mon Sep 17 00:00:00 2001 From: Stanislav Chzhen Date: Wed, 20 Mar 2024 19:25:59 +0300 Subject: [PATCH] Pull request 2166: 5829-trusted-ip Updates #5829. Squashed commit of the following: commit 8a93b30d5bd1c40c30bd10cd3fc77c3a3a64cb71 Merge: 8e4429c48 54f77c010 Author: Stanislav Chzhen Date: Wed Mar 20 19:15:07 2024 +0300 Merge branch 'master' into 5829-trusted-ip commit 8e4429c483c0fd6fffdc93fa808adcca6678bc3e Author: Stanislav Chzhen Date: Wed Mar 20 18:37:26 2024 +0300 all: upd chlog commit b598a8d1ea239cc574bfdfdd6a2da47792582589 Merge: 1f58bf8fd 054233962 Author: Stanislav Chzhen Date: Wed Mar 20 18:34:13 2024 +0300 Merge branch 'master' into 5829-trusted-ip commit 1f58bf8fd1bc3b3790475651cb87494885cadf66 Merge: ffb4b9a65 c64a36c94 Author: Stanislav Chzhen Date: Wed Mar 20 17:09:09 2024 +0300 Merge branch 'master' into 5829-trusted-ip commit ffb4b9a65fea5555d0d401194d3fc3820b2e6766 Author: Stanislav Chzhen Date: Thu Mar 14 17:40:07 2024 +0300 home: fix alignment commit 7f11807ff13eff286be1d3bd4b796273454bdbda Author: Stanislav Chzhen Date: Thu Mar 14 17:35:13 2024 +0300 all: imp code commit 2aee9a66c70af929e28653245eb73c0f29a46e97 Author: Stanislav Chzhen Date: Mon Mar 11 18:17:58 2024 +0300 home: real ip in logs --- CHANGELOG.md | 7 ++++ internal/home/auth.go | 47 +++++++++++++++---------- internal/home/auth_internal_test.go | 6 ++-- internal/home/authhttp.go | 44 ++++++++++++----------- internal/home/authhttp_internal_test.go | 14 ++++---- internal/home/home.go | 4 ++- 6 files changed, 72 insertions(+), 50 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 028eae8e..ce48676f 100644 --- a/CHANGELOG.md +++ b/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 + diff --git a/internal/home/auth.go b/internal/home/auth.go index 72d52a57..18bc5668 100644 --- a/internal/home/auth.go +++ b/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 - rateLimiter *authRateLimiter - sessions map[string]*session - users []webUser - lock sync.Mutex - sessionTTL uint32 + trustedProxies netutil.SubnetSet + db *bbolt.DB + rateLimiter *authRateLimiter + sessions map[string]*session + users []webUser + 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 -func InitAuth(dbFilename string, users []webUser, sessionTTL uint32, rateLimiter *authRateLimiter) *Auth { +// InitAuth initializes the global authentication object. +func InitAuth( + dbFilename string, + users []webUser, + sessionTTL uint32, + rateLimiter *authRateLimiter, + trustedProxies netutil.SubnetSet, +) (a *Auth) { log.Info("Initializing auth module: %s", dbFilename) - a := &Auth{ - sessionTTL: sessionTTL, - rateLimiter: rateLimiter, - sessions: make(map[string]*session), - users: users, + a = &Auth{ + sessionTTL: sessionTTL, + rateLimiter: rateLimiter, + sessions: make(map[string]*session), + 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 { diff --git a/internal/home/auth_internal_test.go b/internal/home/auth_internal_test.go index eae8095c..6be17061 100644 --- a/internal/home/auth_internal_test.go +++ b/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() diff --git a/internal/home/authhttp.go b/internal/home/authhttp.go index d04e0ca3..ff264393 100644 --- a/internal/home/authhttp.go +++ b/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) - if ip != nil { + ip, err = netip.ParseAddr(v) + 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) - ip = net.ParseIP(ipStrs[0]) - if ip != nil { + ipStr, _, _ := strings.Cut(s, ",") + ip, err = netip.ParseAddr(ipStr) + 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) diff --git a/internal/home/authhttp_internal_test.go b/internal/home/authhttp_internal_test.go index aceefa97..db6ce34e 100644 --- a/internal/home/authhttp_internal_test.go +++ b/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 { diff --git a/internal/home/home.go b/internal/home/home.go index 47ed6b2d..3f80cb07 100644 --- a/internal/home/home.go +++ b/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") }