Cap the X-Forwarded-For hop walk at 64 entries (closes #124)
All checks were successful
check / check (push) Successful in 4m9s
All checks were successful
check / check (push) Successful in 4m9s
The chain walk in the rate-limit key function had no bound on hop count. It runs whenever the direct peer is a trusted proxy, which is the normal production deployment, so any client could pad X-Forwarded-For to MaxHeaderBytes (~50k hops, ~0.9 MB) and make the key function walk all of it on the unauthenticated receiver endpoint before the request was rate-limited. Only the last 64 entries are examined now; real chains are one to three hops. A chain longer than the cap runs out of hops and falls back to the peer address, the same fail-closed direction an unreadable hop already took. Bucket assignment for real chains is unchanged. Also corrects the comment on the unparseable-RemoteAddr fallback: it claimed keying on the raw value avoids collapsing those peers into one bucket, but on a Unix-socket listener every peer carries the same RemoteAddr and does share one bucket. The behaviour is fail-closed and unchanged; only the comment was wrong.
This commit is contained in:
@@ -8,7 +8,9 @@ import (
|
||||
"net/http/httptest"
|
||||
"net/netip"
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
"time"
|
||||
|
||||
"github.com/stretchr/testify/assert"
|
||||
"sneak.berlin/go/webhooker/internal/config"
|
||||
@@ -568,6 +570,45 @@ func TestRateLimitKey_ChainWalkSkipsClientPrepended(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer covers the
|
||||
// hop-walk cap. A client behind the trusted proxy can pad
|
||||
// X-Forwarded-For with tens of thousands of trusted-looking hops,
|
||||
// which costs a walk proportional to the padding and, once the walk
|
||||
// runs off the left end of the chain, reaches the entry the client
|
||||
// put there. Capping the walk stops both: the key falls back to the
|
||||
// peer address, so rotating the head of the chain mints no bucket,
|
||||
// and the run does not scale with the chain length.
|
||||
func TestRateLimitKey_LongChainCapsWalkAndFallsBackToPeer(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
// 50k hops is roughly 0.9 MB, within the default
|
||||
// MaxHeaderBytes.
|
||||
const hops = 50000
|
||||
|
||||
padding := strings.Repeat(", 10.0.0.2", hops-1)
|
||||
|
||||
start := time.Now()
|
||||
|
||||
assertSharedBucket(
|
||||
t, trustedProxies("10.0.0.0/8"), "10.0.0.1:44444",
|
||||
func(i int) map[string]string {
|
||||
return map[string]string{
|
||||
headerXFF: fmt.Sprintf("9.9.9.%d%s", i+1, padding),
|
||||
}
|
||||
},
|
||||
"a padded X-Forwarded-For chain must fall back to the "+
|
||||
"peer address, not reach the client-controlled entry "+
|
||||
"at the head of the chain",
|
||||
)
|
||||
|
||||
assert.Less(
|
||||
t, time.Since(start), 2*time.Second,
|
||||
"the capped walk must not scale with the chain length",
|
||||
)
|
||||
}
|
||||
|
||||
// TestReceiverRateLimit_IgnoresForwardedFromUntrustedPeer proves
|
||||
// the receiver limiter uses the same gated key function as the
|
||||
// POST limiters.
|
||||
|
||||
Reference in New Issue
Block a user