Verify login credentials before spending rate-limit budget (closes #150)
All checks were successful
check / check (push) Successful in 2m54s
All checks were successful
check / check (push) Successful in 2m54s
With TRUSTED_PROXIES empty behind the reverse proxy production is required to run behind, every login POST keyed on the proxy's address and shared one 5/minute bucket. A stranger sending five POSTs a minute -- 0.08 requests per second, from anywhere -- kept that bucket permanently full, and the operator's own correct password was answered 429 indefinitely with no second administrative path. The login POST no longer has a pre-emptive limiter. The handler verifies credentials first and spends budget only on a FAILED attempt, so a correct password is never throttled whatever the counters hold. Three things follow, and are implemented together because the first is unsafe without the other two: - Failures are counted per (client bucket, submitted username), five per minute, after which further failures get 429 with a Retry-After. A successful login clears the counter, so mistyping and then succeeding does not leave the operator throttled. - Both key sets are capped at 1024 entries. The submitted username is attacker-controlled, so past the first cap failures fall back to a counter keyed on the client alone, and past both caps a failure is answered as throttled without being recorded. Tracked state stays under half a megabyte and does not grow with invented usernames. - Concurrent Argon2id verifications are capped at two, a 128 MB ceiling at 64 MB per hash. Every password-hashing endpoint takes a slot, including the password-change endpoint, which holds one across both its hashes. A request that waits five seconds without a slot is answered 503 and no hash runs for it. An unknown username is verified against a dummy hash instead of returning early, so a nonexistent account costs the same time as a real one and the response cannot be used to enumerate usernames. The password-change limiter is unchanged: RequireAuth runs ahead of it, so only a request already carrying a valid session reaches its bucket. Also adds the missing test for the third bucketKey call site, where the peer is a trusted proxy but the forwarded chain names no client. Every existing test of that fallback uses an IPv4 proxy, where bucketKey is the identity function, so dropping the /64 masking there left the suite green. README and the TRUSTED_PROXIES startup warning updated: a shared bucket now costs precision, not the availability of the admin path.
This commit is contained in:
@@ -20,14 +20,14 @@ import (
|
||||
"sneak.berlin/go/webhooker/internal/middleware"
|
||||
)
|
||||
|
||||
func TestLoginRateLimit_AllowsGET(t *testing.T) {
|
||||
func TestPostRateLimit_AllowsGET(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
var callCount int
|
||||
|
||||
handler := m.LoginRateLimit()(http.HandlerFunc(
|
||||
handler := m.PasswordChangeRateLimit()(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
callCount++
|
||||
|
||||
@@ -39,7 +39,7 @@ func TestLoginRateLimit_AllowsGET(t *testing.T) {
|
||||
for i := range 20 {
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(),
|
||||
http.MethodGet, "/pages/login", nil,
|
||||
http.MethodGet, "/user/admin/password", nil,
|
||||
)
|
||||
req.RemoteAddr = "192.168.1.1:12345"
|
||||
|
||||
@@ -110,20 +110,6 @@ func runPostLimitTest(
|
||||
assert.Equal(t, limit, callCount)
|
||||
}
|
||||
|
||||
func TestLoginRateLimit_LimitsPOST(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
runPostLimitTest(
|
||||
t,
|
||||
m.LoginRateLimit(),
|
||||
middleware.LoginRateLimitConst,
|
||||
"/pages/login",
|
||||
"10.0.0.1:12345",
|
||||
)
|
||||
}
|
||||
|
||||
func TestPasswordChangeRateLimit_LimitsPOST(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
@@ -138,19 +124,19 @@ func TestPasswordChangeRateLimit_LimitsPOST(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
func TestLoginRateLimit_IndependentPerIP(t *testing.T) {
|
||||
func TestPostRateLimit_IndependentPerIP(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m, _ := testMiddleware(t, config.EnvironmentDev)
|
||||
|
||||
handler := m.LoginRateLimit()(http.HandlerFunc(
|
||||
handler := m.PasswordChangeRateLimit()(http.HandlerFunc(
|
||||
func(w http.ResponseWriter, _ *http.Request) {
|
||||
w.WriteHeader(http.StatusOK)
|
||||
},
|
||||
))
|
||||
|
||||
// Exhaust limit for IP1
|
||||
for range middleware.LoginRateLimitConst {
|
||||
for range middleware.PasswordChangeRateLimitConst {
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(),
|
||||
http.MethodPost, "/pages/login", nil,
|
||||
@@ -367,7 +353,14 @@ func TestReceiverRateLimit_CountsEveryMethod(t *testing.T) {
|
||||
}
|
||||
|
||||
const (
|
||||
loginPath = "/pages/login"
|
||||
// limitedPath is the endpoint these tests drive the shared POST
|
||||
// rate limiter through. It is the password-change path: since
|
||||
// the login POST verifies credentials before spending any
|
||||
// budget, the password-change limiter is the only pre-emptive
|
||||
// POST limiter left, and it is what pins the shared key
|
||||
// function's behaviour here.
|
||||
limitedPath = "/user/admin/password"
|
||||
|
||||
headerXFF = "X-Forwarded-For"
|
||||
headerReal = "X-Real-IP"
|
||||
headerTrue = "True-Client-IP"
|
||||
@@ -415,10 +408,10 @@ func assertSharedBucket(
|
||||
m := rateLimitMiddleware(
|
||||
t, &config.Config{TrustedProxies: proxies},
|
||||
)
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
for i := range middleware.LoginRateLimitConst {
|
||||
w := postWithHeaders(handler, peer, loginPath, headers(i))
|
||||
for i := range middleware.PasswordChangeRateLimitConst {
|
||||
w := postWithHeaders(handler, peer, limitedPath, headers(i))
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
"request %d should pass", i,
|
||||
@@ -426,8 +419,8 @@ func assertSharedBucket(
|
||||
}
|
||||
|
||||
w := postWithHeaders(
|
||||
handler, peer, loginPath,
|
||||
headers(middleware.LoginRateLimitConst),
|
||||
handler, peer, limitedPath,
|
||||
headers(middleware.PasswordChangeRateLimitConst),
|
||||
)
|
||||
assert.Equal(t, http.StatusTooManyRequests, w.Code, msg)
|
||||
}
|
||||
@@ -549,24 +542,24 @@ func TestRateLimitKey_ForwardedHonouredFromTrustedPeer(
|
||||
m := rateLimitMiddleware(t, &config.Config{
|
||||
TrustedProxies: trustedProxies(trustedProxyCIDR),
|
||||
})
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
const peer = trustedPeer
|
||||
|
||||
first := map[string]string{headerXFF: clientIPv4}
|
||||
|
||||
for range middleware.LoginRateLimitConst {
|
||||
postWithHeaders(handler, peer, loginPath, first)
|
||||
for range middleware.PasswordChangeRateLimitConst {
|
||||
postWithHeaders(handler, peer, limitedPath, first)
|
||||
}
|
||||
|
||||
w := postWithHeaders(handler, peer, loginPath, first)
|
||||
w := postWithHeaders(handler, peer, limitedPath, first)
|
||||
assert.Equal(
|
||||
t, http.StatusTooManyRequests, w.Code,
|
||||
"the forwarded client's own bucket must fill up",
|
||||
)
|
||||
|
||||
w = postWithHeaders(
|
||||
handler, peer, loginPath,
|
||||
handler, peer, limitedPath,
|
||||
map[string]string{headerXFF: clientIPv4Alt},
|
||||
)
|
||||
assert.Equal(
|
||||
@@ -662,7 +655,7 @@ func TestRateLimitKey_LongChainAllocationIsBounded(t *testing.T) {
|
||||
})
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodPost, loginPath, nil,
|
||||
context.Background(), http.MethodPost, limitedPath, nil,
|
||||
)
|
||||
req.RemoteAddr = trustedPeer
|
||||
req.Header.Set(
|
||||
@@ -869,7 +862,7 @@ func clientKeyFor(
|
||||
t.Helper()
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodPost, loginPath, nil,
|
||||
context.Background(), http.MethodPost, limitedPath, nil,
|
||||
)
|
||||
req.RemoteAddr = remoteAddr
|
||||
|
||||
@@ -1019,23 +1012,23 @@ func TestRateLimitKey_UnparseablePeerKeepsDistinctBuckets(
|
||||
)
|
||||
}
|
||||
|
||||
// TestLoginRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural
|
||||
// TestPostRateLimit_IPv6SharesBucketWithinSlash64 is the behavioural
|
||||
// half, and the regression test for the bypass itself: a client that
|
||||
// rotates source addresses inside its own routed /64 must stay in one
|
||||
// bucket. Reverting the masking makes this test fail, because each
|
||||
// rotated address would mint a fresh bucket and nothing would be
|
||||
// rejected.
|
||||
func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) {
|
||||
func TestPostRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m := rateLimitMiddleware(t, &config.Config{})
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
for i := range middleware.LoginRateLimitConst {
|
||||
for i := range middleware.PasswordChangeRateLimitConst {
|
||||
w := postWithHeaders(
|
||||
handler,
|
||||
fmt.Sprintf("[2001:db8:1:2::%d]:44444", i+1),
|
||||
loginPath, nil,
|
||||
limitedPath, nil,
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code, "request %d should pass", i,
|
||||
@@ -1043,7 +1036,7 @@ func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) {
|
||||
}
|
||||
|
||||
w := postWithHeaders(
|
||||
handler, "[2001:db8:1:2::ffff]:44444", loginPath, nil,
|
||||
handler, "[2001:db8:1:2::ffff]:44444", limitedPath, nil,
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusTooManyRequests, w.Code,
|
||||
@@ -1052,23 +1045,23 @@ func TestLoginRateLimit_IPv6SharesBucketWithinSlash64(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestLoginRateLimit_IPv6IndependentAcrossSlash64 is the other side
|
||||
// TestPostRateLimit_IPv6IndependentAcrossSlash64 is the other side
|
||||
// of the trade: bucketing by /64 must not merge separate allocations,
|
||||
// so a client in a different /64 keeps its own limit.
|
||||
func TestLoginRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) {
|
||||
func TestPostRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m := rateLimitMiddleware(t, &config.Config{})
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
for range middleware.LoginRateLimitConst + 1 {
|
||||
for range middleware.PasswordChangeRateLimitConst + 1 {
|
||||
postWithHeaders(
|
||||
handler, "[2001:db8:1:2::1]:44444", loginPath, nil,
|
||||
handler, "[2001:db8:1:2::1]:44444", limitedPath, nil,
|
||||
)
|
||||
}
|
||||
|
||||
w := postWithHeaders(
|
||||
handler, "[2001:db8:1:3::1]:44444", loginPath, nil,
|
||||
handler, "[2001:db8:1:3::1]:44444", limitedPath, nil,
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
@@ -1076,23 +1069,23 @@ func TestLoginRateLimit_IPv6IndependentAcrossSlash64(t *testing.T) {
|
||||
)
|
||||
}
|
||||
|
||||
// TestLoginRateLimit_IPv4IndependentPerAddress guards against the
|
||||
// TestPostRateLimit_IPv4IndependentPerAddress guards against the
|
||||
// masking leaking into IPv4: two addresses one apart must still hold
|
||||
// separate buckets.
|
||||
func TestLoginRateLimit_IPv4IndependentPerAddress(t *testing.T) {
|
||||
func TestPostRateLimit_IPv4IndependentPerAddress(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
m := rateLimitMiddleware(t, &config.Config{})
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
for range middleware.LoginRateLimitConst + 1 {
|
||||
for range middleware.PasswordChangeRateLimitConst + 1 {
|
||||
postWithHeaders(
|
||||
handler, clientIPv4+":44444", loginPath, nil,
|
||||
handler, clientIPv4+":44444", limitedPath, nil,
|
||||
)
|
||||
}
|
||||
|
||||
w := postWithHeaders(
|
||||
handler, clientIPv4Alt+":44444", loginPath, nil,
|
||||
handler, clientIPv4Alt+":44444", limitedPath, nil,
|
||||
)
|
||||
assert.Equal(
|
||||
t, http.StatusOK, w.Code,
|
||||
@@ -1112,7 +1105,7 @@ func forwardedKeyFor(
|
||||
t.Helper()
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(), http.MethodPost, loginPath, nil,
|
||||
context.Background(), http.MethodPost, limitedPath, nil,
|
||||
)
|
||||
req.RemoteAddr = trustedPeer
|
||||
req.Header.Set(headerXFF, forwarded)
|
||||
@@ -1177,11 +1170,77 @@ func TestRateLimitKey_ForwardedIPv6BucketsByPrefix(t *testing.T) {
|
||||
}
|
||||
}
|
||||
|
||||
// TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64 is the
|
||||
// TestRateLimitKey_TrustedPeerUnusableForwardedMasksPeer covers the
|
||||
// third bucketKey call site: the peer IS a trusted proxy, but the
|
||||
// forwarded chain cannot name a client, so the key falls back to the
|
||||
// peer address — and that fallback owes the same /64 masking every
|
||||
// other key gets.
|
||||
//
|
||||
// Every existing test of this fallback uses an IPv4 proxy, where
|
||||
// bucketKey is the identity function, so replacing the call with
|
||||
// peer.String() leaves the whole suite green. Only operator-listed
|
||||
// addresses reach this line and the fallback is fail-closed, so this
|
||||
// pins behaviour rather than fixing a defect.
|
||||
func TestRateLimitKey_TrustedPeerUnusableForwardedMasksPeer(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
|
||||
const (
|
||||
proxyCIDR = "2001:db8:ffff::/48"
|
||||
proxyPeer = "[2001:db8:ffff:1::5]:44444"
|
||||
wantKey = "2001:db8:ffff:1::/64"
|
||||
)
|
||||
|
||||
m := rateLimitMiddleware(t, &config.Config{
|
||||
TrustedProxies: trustedProxies(proxyCIDR),
|
||||
})
|
||||
|
||||
for _, tc := range []struct {
|
||||
name string
|
||||
forwarded string
|
||||
about string
|
||||
}{{
|
||||
name: "absent",
|
||||
about: "no X-Forwarded-For at all falls back to the peer",
|
||||
}, {
|
||||
name: "unreadable-hop",
|
||||
forwarded: "unknown",
|
||||
about: "a hop that is not a bare address ends the walk " +
|
||||
"and falls back to the peer",
|
||||
}, {
|
||||
name: "all-hops-trusted",
|
||||
forwarded: "2001:db8:ffff:2::9",
|
||||
about: "a chain naming only trusted proxies names no " +
|
||||
"client, so the peer is used",
|
||||
}} {
|
||||
t.Run(tc.name, func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
req := httptest.NewRequestWithContext(
|
||||
context.Background(),
|
||||
http.MethodPost, limitedPath, nil,
|
||||
)
|
||||
req.RemoteAddr = proxyPeer
|
||||
|
||||
if tc.forwarded != "" {
|
||||
req.Header.Set(headerXFF, tc.forwarded)
|
||||
}
|
||||
|
||||
assert.Equal(
|
||||
t, wantKey,
|
||||
middleware.ClientKeyForTest(m, req),
|
||||
"%s, masked to its /64", tc.about,
|
||||
)
|
||||
})
|
||||
}
|
||||
}
|
||||
|
||||
// TestPostRateLimit_ForwardedIPv6SharesBucketWithinSlash64 is the
|
||||
// behavioural half on the production path: behind a trusted proxy, a
|
||||
// client rotating source addresses inside its own routed /64 must
|
||||
// stay in one bucket.
|
||||
func TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64(
|
||||
func TestPostRateLimit_ForwardedIPv6SharesBucketWithinSlash64(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
@@ -1198,10 +1257,10 @@ func TestLoginRateLimit_ForwardedIPv6SharesBucketWithinSlash64(
|
||||
)
|
||||
}
|
||||
|
||||
// TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64 is the
|
||||
// TestPostRateLimit_ForwardedIPv6IndependentAcrossSlash64 is the
|
||||
// other side of that trade on the same path: bucketing by /64 must
|
||||
// not merge two allocations reaching the proxy.
|
||||
func TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64(
|
||||
func TestPostRateLimit_ForwardedIPv6IndependentAcrossSlash64(
|
||||
t *testing.T,
|
||||
) {
|
||||
t.Parallel()
|
||||
@@ -1209,15 +1268,15 @@ func TestLoginRateLimit_ForwardedIPv6IndependentAcrossSlash64(
|
||||
m := rateLimitMiddleware(t, &config.Config{
|
||||
TrustedProxies: trustedProxies(trustedProxyCIDR),
|
||||
})
|
||||
handler := m.LoginRateLimit()(okHandler())
|
||||
handler := m.PasswordChangeRateLimit()(okHandler())
|
||||
|
||||
spent := map[string]string{headerXFF: clientIPv6}
|
||||
for range middleware.LoginRateLimitConst + 1 {
|
||||
postWithHeaders(handler, trustedPeer, loginPath, spent)
|
||||
for range middleware.PasswordChangeRateLimitConst + 1 {
|
||||
postWithHeaders(handler, trustedPeer, limitedPath, spent)
|
||||
}
|
||||
|
||||
w := postWithHeaders(
|
||||
handler, trustedPeer, loginPath,
|
||||
handler, trustedPeer, limitedPath,
|
||||
map[string]string{headerXFF: clientIPv6Other},
|
||||
)
|
||||
assert.Equal(
|
||||
|
||||
Reference in New Issue
Block a user