diff --git a/CHANGELOG.md b/CHANGELOG.md index d0368126..5330a640 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,8 +20,12 @@ NOTE: Add new changes BELOW THIS COMMENT. ### Fixed +- Incorrect client IP logging in failed authentication attempts when using a proxy ([#8198]). + - Incorrect logger behavior in case `-v` flag is added. +[#8198]: https://github.com/AdguardTeam/AdGuardHome/issues/8198 + diff --git a/internal/home/auth_internal_test.go b/internal/home/auth_internal_test.go index 11af39e8..22dcfcf6 100644 --- a/internal/home/auth_internal_test.go +++ b/internal/home/auth_internal_test.go @@ -31,7 +31,7 @@ func TestAuth_UsersList(t *testing.T) { auth, err := newAuth(testutil.ContextWithTimeout(t, testTimeout), &authConfig{ baseLogger: testLogger, rateLimiter: emptyRateLimiter{}, - trustedProxies: nil, + trustedProxies: testTrustedProxies, dbFilename: sessionsDB, users: nil, sessionTTL: testTimeout, diff --git a/internal/home/authhttp.go b/internal/home/authhttp.go index 1f3a325a..b9393e07 100644 --- a/internal/home/authhttp.go +++ b/internal/home/authhttp.go @@ -108,12 +108,12 @@ func (web *webAPI) handleLogin(w http.ResponseWriter, r *http.Request) { return } - var remoteIP string + var remoteIPStr string // The real IP address of the client [realIP] cannot be used here without // taking trusted proxies into account due to security issues: // // See https://github.com/AdguardTeam/AdGuardHome/issues/2799. - if remoteIP, err = netutil.SplitHost(r.RemoteAddr); err != nil { + if remoteIPStr, err = netutil.SplitHost(r.RemoteAddr); err != nil { writeErrorWithIP( r, w, @@ -127,13 +127,13 @@ func (web *webAPI) handleLogin(w http.ResponseWriter, r *http.Request) { } if rateLimiter := web.auth.rateLimiter; rateLimiter != nil { - if left := rateLimiter.check(remoteIP); left > 0 { + if left := rateLimiter.check(remoteIPStr); left > 0 { w.Header().Set(httphdr.RetryAfter, strconv.Itoa(int(left.Seconds()))) writeErrorWithIP( r, w, http.StatusTooManyRequests, - remoteIP, + remoteIPStr, "auth: blocked for %s", left, ) @@ -147,24 +147,38 @@ func (web *webAPI) handleLogin(w http.ResponseWriter, r *http.Request) { web.logger.ErrorContext( ctx, "getting real ip", - "remote_ip", remoteIP, + "remote_ip", remoteIPStr, slogutil.KeyError, err, ) } - cookie, err := newCookie(ctx, web.auth, req, remoteIP) + remoteIP, err := netip.ParseAddr(remoteIPStr) if err != nil { - logIP := remoteIP - if web.auth.trustedProxies.Contains(ip.Unmap()) { - logIP = ip.String() - } + writeErrorWithIP( + r, + w, + http.StatusInternalServerError, + r.RemoteAddr, + "auth: parsing remote address: %s", + err, + ) + return + } + + logIP := remoteIPStr + if web.auth.trustedProxies.Contains(remoteIP.Unmap()) { + logIP = ip.String() + } + + cookie, err := newCookie(ctx, web.auth, req, remoteIPStr) + if err != nil { writeErrorWithIP(r, w, http.StatusForbidden, logIP, "%s", err) return } - web.logger.InfoContext(ctx, "successful login", "user", req.Name, "ip", ip) + web.logger.InfoContext(ctx, "successful login", "user", req.Name, "ip", logIP) http.SetCookie(w, cookie) diff --git a/internal/home/authhttp_internal_test.go b/internal/home/authhttp_internal_test.go index c65cbd5a..4fc3ea76 100644 --- a/internal/home/authhttp_internal_test.go +++ b/internal/home/authhttp_internal_test.go @@ -467,7 +467,7 @@ func TestAuth_ServeHTTP_auth(t *testing.T) { auth, err := newAuth(testutil.ContextWithTimeout(t, testTimeout), &authConfig{ baseLogger: testLogger, rateLimiter: emptyRateLimiter{}, - trustedProxies: nil, + trustedProxies: testTrustedProxies, dbFilename: sessionsDB, users: users, sessionTTL: testTTL * time.Second, @@ -623,7 +623,7 @@ func TestAuth_ServeHTTP_logout(t *testing.T) { auth, err := newAuth(testutil.ContextWithTimeout(t, testTimeout), &authConfig{ baseLogger: testLogger, rateLimiter: emptyRateLimiter{}, - trustedProxies: nil, + trustedProxies: testTrustedProxies, dbFilename: sessionsDB, users: users, sessionTTL: testTTL * time.Second, diff --git a/internal/home/home_internal_test.go b/internal/home/home_internal_test.go index e6d3b913..78b03154 100644 --- a/internal/home/home_internal_test.go +++ b/internal/home/home_internal_test.go @@ -3,11 +3,13 @@ package home import ( "cmp" "net/http" + "net/netip" "testing" "github.com/AdguardTeam/AdGuardHome/internal/agh" "github.com/AdguardTeam/AdGuardHome/internal/aghhttp" "github.com/AdguardTeam/golibs/logutil/slogutil" + "github.com/AdguardTeam/golibs/netutil" "github.com/AdguardTeam/golibs/testutil" "github.com/stretchr/testify/require" ) @@ -15,6 +17,9 @@ import ( // testLogger is a common logger for tests. var testLogger = slogutil.NewDiscardLogger() +// testTrustedProxies is a common trusted proxies set for tests. +var testTrustedProxies = netutil.SliceSubnetSet([]netip.Prefix{}) + // newTestWeb is a helper that creates new webAPI and fills it's config with // given values. If conf is nil, the default configuration will be used. func newTestWeb( diff --git a/internal/home/profilehttp_internal_test.go b/internal/home/profilehttp_internal_test.go index 88c9c138..7b82e7a3 100644 --- a/internal/home/profilehttp_internal_test.go +++ b/internal/home/profilehttp_internal_test.go @@ -62,7 +62,7 @@ func TestWeb_HandleGetProfile(t *testing.T) { auth, err := newAuth(testutil.ContextWithTimeout(t, testTimeout), &authConfig{ baseLogger: testLogger, rateLimiter: emptyRateLimiter{}, - trustedProxies: nil, + trustedProxies: testTrustedProxies, dbFilename: sessionsDB, users: nil, sessionTTL: testTTL * time.Second,