Fixes the defect the operator spotted in #370 immediately after it shipped: auth.ClientIP ignored X-Forwarded-For whenever RemoteAddr was public, so a proxy on a public address — a separate host, or a CDN, i.e. anyone running this publicly, since public means TLS means a proxy — recorded the PROXY for every session. created_ip and last_ip were then always equal and the "Address changed" signal could never fire. The feature looked like it worked and reported nothing. Replaced with the standard trusted-hop model (Rails, Caddy, Traefik, nginx). XFF grows left-to-right as each proxy appends the peer it received from, so for client -> CDN -> own-proxy -> app the app sees [client, CDN] with RemoteAddr = own-proxy, and the client sits at XFF[len - hops]: 0 RemoteAddr, XFF ignored — no proxy 1 the address your own proxy observed 2 through a CDN in front of your proxy Default 1, per the operator: publicly reachable means a TLS terminator in front. The cost is real and stated rather than hidden. hops >= 1 DECLARES that a proxy exists; set it with no proxy, or deeper than the actual chain, and the index reaches attacker-supplied entries, letting a visitor choose which address their own session shows — defeating exactly the detection #370 is for. That's inherent to the model, which is why 0 is a first-class value and the admin card says "count your proxies, don't guess high" instead of just exposing a number. Both mis-set shapes are pinned by tests so they stay known consequences rather than surprises. Migration 0053 + internal/netsettings, cached under an RWMutex. That's not an optimisation: ClientIP runs in RequireUser for every authenticated request, so a per-request query would put the database on the critical path of the whole API. New() always returns a usable service so a boot-time DB hiccup degrades to the default instead of breaking that path (rule #131), and Hops() is nil-safe because test routers construct middleware without it. RequireUser now takes a func() int rather than an int — the value is operator-editable at runtime while the middleware is built once at boot, and reading it per request is what makes a save take effect with no restart (rule #25). The admin card is verifiable, not just configurable: it reports the address the CURRENT setting resolves THIS request to, the raw forwarded chain, and the socket peer — so you set the number, save, and confirm the address matches the machine you're on. It also counts the arriving chain and says how many proxies that implies. GET/PUT both return that payload, PUT recomputed under the new value, so the effect is visible without a reload. Also fixes styling in the #370 card that CI could not catch: text-destructive and bg-destructive don't exist in this Tailwind config — the palette is colors.action.destructive — so the "Address changed" warning and the sign-out-others button were rendering unstyled. Both now use text-action-destructive / bg-action-destructive / text-action-fg. Not done here: requestlog.go still logs raw RemoteAddr and will disagree with the sessions UI about who connected. Left for its own change.
112 lines
3.9 KiB
Go
112 lines
3.9 KiB
Go
package auth
|
|
|
|
import (
|
|
"net"
|
|
"net/http"
|
|
"strings"
|
|
)
|
|
|
|
// ClientIP returns the caller's address, reading through trustedProxyHops
|
|
// reverse proxies (#2453).
|
|
//
|
|
// X-Forwarded-For grows left-to-right: every proxy APPENDS the peer it
|
|
// received the request from. For client -> CDN -> own-proxy -> Minstrel the
|
|
// app sees XFF = [client, CDN] and RemoteAddr = own-proxy. Each trusted proxy
|
|
// therefore accounts for one entry counting from the right, and the first
|
|
// address we were NOT told to trust is the client:
|
|
//
|
|
// hops 0 -> RemoteAddr; XFF ignored entirely
|
|
// hops 1 -> XFF[1] = CDN — trusting only our own proxy, the most we can
|
|
// honestly claim is the address it told us about
|
|
// hops 2 -> XFF[0] = client
|
|
//
|
|
// This replaces an earlier heuristic that ignored XFF whenever RemoteAddr was
|
|
// public. That was safe but useless in the deployment that matters: a proxy
|
|
// on a public address (separate host, or a CDN) meant every session recorded
|
|
// the proxy, so the active-sessions surface could never show an address
|
|
// change (#370).
|
|
//
|
|
// # What the operator is asserting
|
|
//
|
|
// hops >= 1 is a DECLARATION that a proxy sits in front. Two ways to get it
|
|
// wrong, both worth understanding rather than papering over:
|
|
//
|
|
// - Set to 1+ with NO proxy: any client can forge X-Forwarded-For and pick
|
|
// what its own session row shows, defeating the compromise detection.
|
|
// - Set HIGHER than the real chain: the index runs past the proxy-written
|
|
// entries into attacker-supplied ones, same result.
|
|
//
|
|
// Both are inherent to the trusted-hop model — Rails, Caddy, Traefik and
|
|
// nginx all behave this way — which is why 0 is a first-class value and the
|
|
// admin card tells the operator to count their proxies.
|
|
func ClientIP(r *http.Request, trustedProxyHops int) string {
|
|
remote := hostOf(r.RemoteAddr)
|
|
if trustedProxyHops <= 0 {
|
|
return remote
|
|
}
|
|
chain := forwardedChain(r)
|
|
if len(chain) == 0 {
|
|
// No forwarding header: either there's genuinely no proxy, or one is
|
|
// misconfigured. The socket peer is the only thing we actually know.
|
|
return remote
|
|
}
|
|
// Clamp rather than reject: a chain shorter than the configured depth
|
|
// means the operator over-counted, and the leftmost entry is the closest
|
|
// thing to a client on offer. The caveat above covers the risk.
|
|
idx := len(chain) - trustedProxyHops
|
|
if idx < 0 {
|
|
idx = 0
|
|
}
|
|
if ip := net.ParseIP(chain[idx]); ip != nil {
|
|
return ip.String()
|
|
}
|
|
// A proxy wrote something that isn't an address. Positional meaning is
|
|
// lost, so fall back to what we can verify ourselves.
|
|
return remote
|
|
}
|
|
|
|
// forwardedChain returns the X-Forwarded-For entries in wire order, or the
|
|
// single X-Real-IP value when XFF is absent.
|
|
//
|
|
// Entries are kept verbatim, including unparseable ones: their POSITION is
|
|
// what carries meaning here, so silently dropping a malformed hop would
|
|
// shift every index and could hand back an attacker-supplied entry.
|
|
func forwardedChain(r *http.Request) []string {
|
|
raw := r.Header.Get("X-Forwarded-For")
|
|
if strings.TrimSpace(raw) == "" {
|
|
// Some proxies set only X-Real-IP, which by construction is a single
|
|
// hop — the address that proxy saw.
|
|
if real := strings.TrimSpace(r.Header.Get("X-Real-IP")); real != "" {
|
|
return []string{real}
|
|
}
|
|
return nil
|
|
}
|
|
parts := strings.Split(raw, ",")
|
|
out := make([]string, 0, len(parts))
|
|
for _, p := range parts {
|
|
if p = strings.TrimSpace(p); p != "" {
|
|
out = append(out, p)
|
|
}
|
|
}
|
|
return out
|
|
}
|
|
|
|
// hopsOf reads a trusted-depth accessor, treating a nil one as "trust
|
|
// nothing". Test contexts and any future caller that hasn't wired the
|
|
// settings service get the safe reading rather than a panic.
|
|
func hopsOf(fn func() int) int {
|
|
if fn == nil {
|
|
return 0
|
|
}
|
|
return fn()
|
|
}
|
|
|
|
// hostOf strips the port from a RemoteAddr, tolerating values that have none.
|
|
func hostOf(remoteAddr string) string {
|
|
host, _, err := net.SplitHostPort(remoteAddr)
|
|
if err != nil {
|
|
return strings.TrimSpace(remoteAddr)
|
|
}
|
|
return host
|
|
}
|