Some checks failed
CI / ci (push) Has been cancelled
ClientIP used r.RemoteAddr and ignored X-Forwarded-For. That was right about not trusting the header from an untrusted peer, but it is only half the design: behind a proxy every request arrives from the proxy, so the limiter key in handlers.go was "<proxy-ip>|<username>" for every client on the internet. One attacker locked out a username for everyone, an attacker rotating usernames never hit a limit, and sessions.ip recorded the proxy so the device list was useless. - VELLUM_TRUSTED_PROXIES is a CIDR list. X-Forwarded-For is honoured only when RemoteAddr is inside it, and the chain is walked right-to-left so the first address that is not itself a trusted proxy wins. Everything to its left was appended by a client we do not control, so a spoofed leftmost entry is discarded. - VELLUM_PUBLIC_URL is the external origin. CookieSecure now defaults to true when it is https://, so a TLS deployment that forgets VELLUM_COOKIE_SECURE no longer ships a plaintext session cookie. - A malformed CIDR fails the boot rather than being skipped: silently dropping a proxy network widens the trust boundary, which is the exact failure this setting exists to prevent. ClientIP became a method on Authenticator so it can see the trusted set, which is why handlers.go and main.go changed as well. Verified red-green: neutering the trusted-peer path fails five ClientIP tests, including the spoofed-leftmost case. Known gap, pre-existing and not fixed here: this limiter still covers only /api/v1/auth/login. Jellyfin AuthenticateByName and Subsonic auth call auth.Authenticate with no limiting, so the same brute force is available through those two surfaces.
118 lines
3.3 KiB
Go
118 lines
3.3 KiB
Go
package auth
|
|
|
|
import (
|
|
"net"
|
|
"net/http"
|
|
"net/http/httptest"
|
|
"testing"
|
|
)
|
|
|
|
// clientIPFor builds an Authenticator trusting the given CIDRs and returns the
|
|
// address it derives from a request arriving from peer with header XFF.
|
|
func clientIPFor(t *testing.T, peer, xff string, trusted ...string) string {
|
|
t.Helper()
|
|
a := &Authenticator{}
|
|
for _, c := range trusted {
|
|
_, n, err := net.ParseCIDR(c)
|
|
if err != nil {
|
|
t.Fatalf("parse test CIDR %q: %v", c, err)
|
|
}
|
|
a.TrustedProxies = append(a.TrustedProxies, n)
|
|
}
|
|
r := httptest.NewRequest(http.MethodGet, "/", nil)
|
|
r.RemoteAddr = peer
|
|
if xff != "" {
|
|
r.Header.Set("X-Forwarded-For", xff)
|
|
}
|
|
return a.ClientIP(r)
|
|
}
|
|
|
|
func TestClientIPUntrustedPeerIgnoresForwardedFor(t *testing.T) {
|
|
// The security case: an untrusted peer sending X-Forwarded-For must not be
|
|
// believed, or anyone could evade (or poison) the login rate limiter.
|
|
got := clientIPFor(t, "203.0.113.9:44321", "1.2.3.4", "172.16.0.0/12")
|
|
if got != "203.0.113.9" {
|
|
t.Errorf("ClientIP = %q, want the peer 203.0.113.9", got)
|
|
}
|
|
}
|
|
|
|
func TestClientIPTrustedPeerUsesForwardedFor(t *testing.T) {
|
|
// The reverse-proxy case: every request arrives from the proxy, so without
|
|
// the header every client shares one limiter key.
|
|
got := clientIPFor(t, "172.20.0.5:51000", "1.2.3.4", "172.16.0.0/12")
|
|
if got != "1.2.3.4" {
|
|
t.Errorf("ClientIP = %q, want the client 1.2.3.4 from the header", got)
|
|
}
|
|
}
|
|
|
|
func TestClientIPChain(t *testing.T) {
|
|
tests := []struct {
|
|
name string
|
|
peer string
|
|
xff string
|
|
trusted []string
|
|
want string
|
|
}{
|
|
{
|
|
name: "no trusted proxies at all",
|
|
peer: "172.20.0.5:51000", xff: "1.2.3.4",
|
|
want: "172.20.0.5",
|
|
},
|
|
{
|
|
name: "trusted peer without header",
|
|
peer: "172.20.0.5:51000",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "172.20.0.5",
|
|
},
|
|
{
|
|
name: "spans proxies on the right of the client",
|
|
peer: "172.20.0.5:51000", xff: "1.2.3.4, 172.18.0.1",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "1.2.3.4",
|
|
},
|
|
{
|
|
// A client-supplied leftmost entry is attacker-controlled, so the
|
|
// walk must stop at the first untrusted hop counting from the right.
|
|
name: "ignores a spoofed leftmost entry",
|
|
peer: "172.20.0.5:51000", xff: "9.9.9.9, 1.2.3.4, 172.18.0.1",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "1.2.3.4",
|
|
},
|
|
{
|
|
name: "every hop trusted falls back to the leftmost",
|
|
peer: "172.20.0.5:51000", xff: "172.19.0.1, 172.18.0.1, 172.17.0.1",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "172.19.0.1",
|
|
},
|
|
{
|
|
name: "multiple trusted networks",
|
|
peer: "127.0.0.1:51000", xff: "1.2.3.4",
|
|
trusted: []string{"172.16.0.0/12", "127.0.0.1/32"},
|
|
want: "1.2.3.4",
|
|
},
|
|
{
|
|
name: "unparseable hop is not trusted",
|
|
peer: "172.20.0.5:51000", xff: "not-an-ip",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "not-an-ip",
|
|
},
|
|
{
|
|
name: "remote address without a port",
|
|
peer: "172.20.0.5", xff: "1.2.3.4",
|
|
trusted: []string{"172.16.0.0/12"},
|
|
want: "1.2.3.4",
|
|
},
|
|
{
|
|
name: "untrusted peer without a port",
|
|
peer: "203.0.113.9", xff: "1.2.3.4",
|
|
want: "203.0.113.9",
|
|
},
|
|
}
|
|
for _, tc := range tests {
|
|
t.Run(tc.name, func(t *testing.T) {
|
|
if got := clientIPFor(t, tc.peer, tc.xff, tc.trusted...); got != tc.want {
|
|
t.Errorf("ClientIP = %q, want %q", got, tc.want)
|
|
}
|
|
})
|
|
}
|
|
}
|