network: Use peer address after proxy fix for app rate limiter if ava…

…ilable (#5848)

config/localTemplate.go

@@ -311,6 +311,12 @@ type Local struct {
 	// determining the source of a connection.  If used, it should be set to the string "X-Forwarded-For", unless the
 	// proxy vendor provides another header field.  In the case of CloudFlare proxy, the "CF-Connecting-IP" header
 	// field can be used.
+	// This setting does not support multiple X-Forwarded-For HTTP headers or multiple values in in the header and always uses the last value
+	// from the last X-Forwarded-For HTTP header that corresponds to a single reverse proxy (even if it received the request from another reverse proxy or adversary node).
+	//
+	// WARNING: By enabling this option, you are trusting peers to provide accurate forwarding addresses.
+	// Bad actors can easily spoof these headers to circumvent this node's rate and connection limiting
+	// logic. Do not enable this if your node is publicly reachable or used by untrusted parties.
 	UseXForwardedForAddressField string `version[0]:""`
 
 	// ForceRelayMessages indicates whether the network library should relay messages even in the case that no NetAddress was specified.

network/requestTracker.go

@@ -20,7 +20,10 @@ import (
 	"fmt"
 	"net"
 	"net/http"
+	"net/textproto"
 	"sort"
+	"strings"
+	"sync/atomic"
 	"time"
 
 	"github.com/algorand/go-deadlock"
@@ -222,7 +225,7 @@ type RequestTracker struct {
 	log               logging.Logger
 	config            config.Local
 	// once we detect that we have a misconfigured UseForwardedForAddress, we set this and write an warning message.
-	misconfiguredUseForwardedForAddress bool
+	misconfiguredUseForwardedForAddress atomic.Bool
 
 	listener net.Listener // this is the downsteam listener
 
@@ -518,13 +521,29 @@ func (rt *RequestTracker) getForwardedConnectionAddress(header http.Header) (ip
 	if rt.config.UseXForwardedForAddressField == "" {
 		return
 	}
-	forwardedForString := header.Get(rt.config.UseXForwardedForAddressField)
+	var forwardedForString string
+	// if we're using the standard X-Forwarded-For header(s), we need to parse it.
+	// as UseXForwardedForAddressField defines, use the last value from the last X-Forwarded-For header's list of values.
+	if textproto.CanonicalMIMEHeaderKey(rt.config.UseXForwardedForAddressField) == "X-Forwarded-For" {
+		forwardedForStrings := header.Values(rt.config.UseXForwardedForAddressField)
+		if len(forwardedForStrings) != 0 {
+			forwardedForString = forwardedForStrings[len(forwardedForStrings)-1]
+			ips := strings.Split(forwardedForString, ",")
+			if len(ips) != 0 {
+				forwardedForString = strings.TrimSpace(ips[len(ips)-1])
+			} else {
+				// looks like not possble case now but it's better to handle
+				rt.log.Warnf("header X-Forwarded-For has an invalid value: '%s'", forwardedForString)
+				forwardedForString = ""
+			}
+		}
+	} else {
+		forwardedForString = header.Get(rt.config.UseXForwardedForAddressField)
+	}
+
 	if forwardedForString == "" {
-		rt.httpConnectionsMu.Lock()
-		defer rt.httpConnectionsMu.Unlock()
-		if !rt.misconfiguredUseForwardedForAddress {
+		if rt.misconfiguredUseForwardedForAddress.CompareAndSwap(false, true) {
 			rt.log.Warnf("UseForwardedForAddressField is configured as '%s', but no value was retrieved from header", rt.config.UseXForwardedForAddressField)
-			rt.misconfiguredUseForwardedForAddress = true
 		}
 		return
 	}

network/requestTracker_test.go

@@ -17,7 +17,9 @@
 package network
 
 import (
+	"bytes"
 	"math/rand"
+	"net/http"
 	"testing"
 	"time"
 
@@ -172,6 +174,7 @@ func TestRateLimiting(t *testing.T) {
 
 func TestIsLocalHost(t *testing.T) {
 	partitiontest.PartitionTest(t)
+	t.Parallel()
 
 	require.True(t, isLocalhost("localhost"))
 	require.True(t, isLocalhost("127.0.0.1"))
@@ -183,3 +186,101 @@ func TestIsLocalHost(t *testing.T) {
 	require.False(t, isLocalhost("0.0.0.0"))
 	require.False(t, isLocalhost("127.0.0.0"))
 }
+
+func TestGetForwardedConnectionAddress(t *testing.T) {
+	partitiontest.PartitionTest(t)
+	t.Parallel()
+
+	var bufNewLogger bytes.Buffer
+	log := logging.NewLogger()
+	log.SetOutput(&bufNewLogger)
+
+	rt := RequestTracker{log: log}
+	header := http.Header{}
+
+	ip := rt.getForwardedConnectionAddress(header)
+	require.Nil(t, ip)
+	msgs := bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	rt.config.UseXForwardedForAddressField = "X-Custom-Addr"
+	ip = rt.getForwardedConnectionAddress(header)
+	require.Nil(t, ip)
+	msgs = bufNewLogger.String()
+	require.NotEmpty(t, msgs)
+	require.Contains(t, msgs, "UseForwardedForAddressField is configured as 'X-Custom-Addr'")
+
+	// try again and ensure the message is not logged second time.
+	bufNewLogger.Reset()
+	ip = rt.getForwardedConnectionAddress(header)
+	require.Nil(t, ip)
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	// check a custom address can be parsed successfully.
+	header.Set("X-Custom-Addr", "123.123.123.123")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.NotNil(t, ip)
+	require.Equal(t, "123.123.123.123", ip.String())
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	// check a custom address in a form of a list can not be parsed,
+	// this is the original behavior since the Release.
+	header.Set("X-Custom-Addr", "123.123.123.123, 234.234.234.234")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.Nil(t, ip)
+	msgs = bufNewLogger.String()
+	require.NotEmpty(t, msgs)
+	require.Contains(t, msgs, "unable to parse origin address")
+
+	// "X-Forwarded-For
+	bufNewLogger.Reset()
+	rt.misconfiguredUseForwardedForAddress.Store(false)
+	rt.config.UseXForwardedForAddressField = "X-Forwarded-For"
+	header = http.Header{}
+
+	// check "X-Forwarded-For" empty value.
+	ip = rt.getForwardedConnectionAddress(header)
+	require.Nil(t, ip)
+	msgs = bufNewLogger.String()
+	require.NotEmpty(t, msgs)
+	require.Contains(t, msgs, "UseForwardedForAddressField is configured as 'X-Forwarded-For'")
+	bufNewLogger.Reset()
+
+	// check "X-Forwarded-For" single value.
+	header.Set("X-Forwarded-For", "123.123.123.123")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.NotNil(t, ip)
+	require.Equal(t, "123.123.123.123", ip.String())
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	// check "X-Forwarded-For" list values - the last one is used,
+	// this is a new behavior.
+	bufNewLogger.Reset()
+	rt.config.UseXForwardedForAddressField = "X-Forwarded-For"
+	header.Set("X-Forwarded-For", "123.123.123.123, 234.234.234.234")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.NotNil(t, ip)
+	require.Equal(t, "234.234.234.234", ip.String())
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	// check multile X-Forwarded-For headers - the last one should be used
+	header.Set("X-Forwarded-For", "127.0.0.1")
+	header.Add("X-Forwarded-For", "234.234.234.234")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.NotNil(t, ip)
+	require.Equal(t, "234.234.234.234", ip.String())
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+
+	header.Set("X-Forwarded-For", "127.0.0.1")
+	header.Add("X-Forwarded-For", "123.123.123.123, 234.234.234.234")
+	ip = rt.getForwardedConnectionAddress(header)
+	require.NotNil(t, ip)
+	require.Equal(t, "234.234.234.234", ip.String())
+	msgs = bufNewLogger.String()
+	require.Empty(t, msgs)
+}