diff --git a/.env.example b/.env.example index 5a417b5..dcbcd63 100644 --- a/.env.example +++ b/.env.example @@ -3,6 +3,9 @@ # Address the web UI is published on. 127.0.0.1 keeps it host-local; set 0.0.0.0 # to reach it from another machine or from Windows when ARTEX runs inside WSL. ARTEX_BIND_ADDR=127.0.0.1 +# Reverse proxies (comma-separated CIDRs) whose X-Forwarded-For the login rate +# limit trusts. Empty: the header is ignored. See "Reverse proxy deployment" in README.md. +ARTEX_TRUSTED_PROXIES= POSTGRES_USER=artex POSTGRES_PASSWORD=change-me-please POSTGRES_DB=artex diff --git a/CHANGELOG.md b/CHANGELOG.md index 440c8c2..f79a6d6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,7 +7,7 @@ All notable changes to this project are documented in this file. The format is b ### Changed - **The binary listens on `127.0.0.1:8787` by default**: the `-addr` default was `:8787`, every interface. Running `artex` or `./start.sh` without `-addr` now accepts connections from the local machine only; pass `-addr :8787` to listen on every interface. Docker is unchanged: the image starts the server with `-addr :8787` inside the container, and `docker-compose.yml` already publishes it on `127.0.0.1` by default (`ARTEX_BIND_ADDR`). -- **New passwords need at least 12 characters** (was 8), at first-run setup (`POST /api/auth/init` and the `/setup` page) and in `POST /api/auth/change-password`. Login does not check length, so a password set under the old minimum keeps working until it is changed. `reset-password.sh` does not check length. +- **New passwords need at least 12 characters** (was 8), at first-run setup (`POST /api/auth/init` and the `/setup` page) and in `POST /api/auth/change-password`. Login does not check length, so a password set under the old minimum keeps working until it is changed. `reset-password.sh` enforces the same minimum. ### Fixed @@ -20,7 +20,7 @@ All notable changes to this project are documented in this file. The format is b ### Security -- **Login rate limit**: `POST /api/auth/login` allows 10 attempts per minute per client IP. The next attempt gets 429 with a `Retry-After` header (seconds) and a JSON error, and the IP is blocked for 1 minute. Each attempt while blocked, and each later block, doubles the wait, up to 15 minutes. A successful login clears the IP's count. The IP is the TCP peer address; `X-Forwarded-For` and similar headers are ignored, because the client controls them. There is no per-account lockout, so failing logins cannot lock the admin out from other addresses. The state is in memory: a restart clears it, an IP idle for 15 minutes is dropped, and at most 10,000 IPs are tracked (the least recently seen is dropped first). Behind a reverse proxy every client has the proxy's address and shares one budget, so an attacker failing logins through the proxy also blocks the real users behind it. +- **Login rate limit**: `POST /api/auth/login` allows 10 attempts per minute per client IP. The next attempt gets 429 with a `Retry-After` header (seconds) and a JSON error, and the IP is blocked for 1 minute. Each attempt while blocked, and each later block, doubles the wait, up to 15 minutes. A successful login clears the IP's count. There is no per-account lockout, so failing logins cannot lock the admin out from other addresses. The state is in memory: a restart clears it, an IP idle for 15 minutes is dropped, and at most 10,000 IPs are tracked (the least recently seen is dropped first). The IP is the TCP peer address, and `X-Forwarded-For` is ignored, unless the peer is listed in the new `ARTEX_TRUSTED_PROXIES` (comma-separated CIDRs, empty by default, passed through by `docker-compose.yml`). For a listed peer the header is read from the right and the first address that is not a trusted proxy is the client; entries the client added further left are never used. Behind a reverse proxy, set it: otherwise every client has the proxy's address and shares one budget, and an attacker failing logins through the proxy blocks the real users, admin included, for up to 15 minutes. The README nginx example now sends `X-Forwarded-For`. - **`GET /api/health` hides the version from anonymous callers**: without a valid token it answers `{"ok":true}` only. With a token (the web UI sends one for the version in its top bar) the answer is unchanged: `ok`, `service` and `version`. ## [0.5.0] - 2026-11-04 diff --git a/README.md b/README.md index 5b495e3..7f2c049 100644 --- a/README.md +++ b/README.md @@ -263,7 +263,7 @@ Optional: `ARTEX_LLM_PROVIDER` (anthropic | openai | openai-responses | ollama) ### Reverse proxy deployment (HTTPS / only exposing 443) -The frontend and API/SSE are served by the same backend port (default `:8787`), and the realtime activity stream uses the **same-origin** address by default, so **there is no need to configure `NEXT_PUBLIC_SSE_BASE`**: just expose 443 publicly and keep 8787 on the internal network. +The frontend and API/SSE are served by the same backend port (8787; the binary listens on `127.0.0.1:8787` by default, the Docker container on `:8787` published on `127.0.0.1`), and the realtime activity stream uses the **same-origin** address by default, so **there is no need to configure `NEXT_PUBLIC_SSE_BASE`**: just expose 443 publicly and keep 8787 on the internal network. SSE is a long-lived connection with continuous pushes, so the reverse proxy **must disable buffering**, otherwise the browser connects but never receives events (the activity stream spins forever). Nginx example: @@ -277,6 +277,7 @@ server { proxy_pass http://127.0.0.1:8787; proxy_set_header Host $host; proxy_set_header X-Forwarded-Proto $scheme; + proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; # SSE essentials: buffering off, long timeouts, HTTP/1.1 proxy_buffering off; @@ -288,6 +289,8 @@ server { } ``` +**Login rate limit behind a proxy**: login allows 10 attempts per minute per client IP. Behind a proxy every request comes from the proxy's address, so without further setup all users share one budget and an attacker who fails 10 logins blocks everyone, admin included, for up to 15 minutes. Set `ARTEX_TRUSTED_PROXIES` to the proxy's address as ARTEX sees it (comma-separated CIDRs, empty by default), for example `ARTEX_TRUSTED_PROXIES=127.0.0.1/32` for nginx on the same host as the binary. For a peer in that list, ARTEX reads `X-Forwarded-For` from the right and takes the first address that is not a trusted proxy; from any other peer the header is ignored. In Docker the connection does not arrive from `127.0.0.1` but from the Docker network's gateway (see `docker network inspect _default`); set the variable in `.env`. Trust only addresses that are proxies you run: a trusted peer can claim any client address. + > Only when SSE needs a different origin from the page (e.g. a dedicated subdomain) should you set `NEXT_PUBLIC_SSE_BASE` **at build time** (this variable is baked into the static bundle during `next build`; setting it at container runtime has no effect). --- diff --git a/cmd/artex/main.go b/cmd/artex/main.go index 602c934..f2f0b6b 100644 --- a/cmd/artex/main.go +++ b/cmd/artex/main.go @@ -108,7 +108,16 @@ func run() int { keyDir = abs } log.Printf("[config] JWT key directory: %s", keyDir) - srv := server.New(ctx, mgr, skillDir, *dataDir, keyDir, server.WithSecretScrub()) + // Proxies whose X-Forwarded-For the login rate limit reads; empty trusts none. + trusted, err := server.ParseTrustedProxies(os.Getenv("ARTEX_TRUSTED_PROXIES")) + if err != nil { + log.Printf("[config] ARTEX_TRUSTED_PROXIES: %v", err) + return 1 + } + if len(trusted) > 0 { + log.Printf("[config] trusted proxies: %v", trusted) + } + srv := server.New(ctx, mgr, skillDir, *dataDir, keyDir, server.WithSecretScrub(), server.WithTrustedProxies(trusted)) httpSrv := &http.Server{ Addr: *addr, Handler: srv.Handler(), diff --git a/docker-compose.yml b/docker-compose.yml index 682cb85..3870ea7 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -35,6 +35,7 @@ services: ARTEX_LLM_BASE_URL: ${ARTEX_LLM_BASE_URL:-} ARTEX_LLM_PROXY: ${ARTEX_LLM_PROXY:-} ARTEX_JWT_KEY_DIR: /app/keys + ARTEX_TRUSTED_PROXIES: ${ARTEX_TRUSTED_PROXIES:-} ports: - "${ARTEX_BIND_ADDR:-127.0.0.1}:8787:8787" - "127.0.0.1:8788:8788" diff --git a/reset-password.sh b/reset-password.sh index 0e4fdb1..6a3e184 100755 --- a/reset-password.sh +++ b/reset-password.sh @@ -14,7 +14,7 @@ # # Usage examples: # ./reset-password.sh # local, auto-reads config.json/env, prompts for the new password -# ./reset-password.sh -p 'NewPass!' # local, new password given directly +# ./reset-password.sh -p 'New-Passphrase!' # local, new password given directly (at least 12 characters) # ./reset-password.sh --dsn postgres://u:p@h:5432/artex # ./reset-password.sh -H 127.0.0.1 -P 5433 -U autopentest -W pass -d artex # ./reset-password.sh -m docker # docker deployment (reads POSTGRES_* from .env) @@ -130,6 +130,10 @@ if [[ -z "$NEWPASS" ]]; then [[ "$NEWPASS" == "$NEWPASS2" ]] || die "the two entries do not match" fi [[ -n "$NEWPASS" ]] || die "password cannot be empty" +# Same minimum as the server (minPasswordRunes in server/auth.go), counted in +# characters like the server: UTF-8 bytes minus continuation bytes, whatever the locale. +NEWPASS_CHARS=$(printf '%s' "$NEWPASS" | LC_ALL=C tr -d '\200-\277' | wc -c) +(( NEWPASS_CHARS >= 12 )) || die "password must be at least 12 characters" # Hand the password to psql via environment variable (read with \getenv, never in argv/ps) export ARTEX_RESET_NEWPASS="$NEWPASS" diff --git a/server/auth.go b/server/auth.go index 4b646a9..a932332 100644 --- a/server/auth.go +++ b/server/auth.go @@ -380,10 +380,10 @@ func (s *Server) authLogin(w http.ResponseWriter, r *http.Request) { // login is authLogin past the data-source check; getSetting is pg.GetSetting, // a parameter so tests can run it without PostgreSQL. Every attempt counts -// against the caller's IP (s.loginLimit) and a successful one clears it. There -// is no per-account lockout and no password length check here. +// against the caller's IP (s.loginLimit, keyed by clientIP) and a successful +// one clears it. There is no per-account lockout and no password length check here. func (s *Server) login(w http.ResponseWriter, r *http.Request, getSetting func(string) (string, bool, error)) { - ip := peerIP(r) + ip := clientIP(r, s.trustedProxies) if wait := s.loginLimit.allow(ip); wait > 0 { secs := int((wait + time.Second - 1) / time.Second) w.Header().Set("Retry-After", strconv.Itoa(secs)) diff --git a/server/login_limit.go b/server/login_limit.go index dc65fdc..51dec5a 100644 --- a/server/login_limit.go +++ b/server/login_limit.go @@ -1,8 +1,11 @@ package server import ( + "fmt" "net" "net/http" + "net/netip" + "strings" "sync" "time" ) @@ -114,9 +117,7 @@ func (l *loginLimiter) evictOldest() { delete(l.entries, oldest) } -// peerIP is the host part of the TCP peer address. X-Forwarded-For and similar -// headers are ignored on purpose: the client sets them, so trusting them would -// give an attacker a fresh bucket per request. +// peerIP is the host part of the TCP peer address. func peerIP(r *http.Request) string { host, _, err := net.SplitHostPort(r.RemoteAddr) if err != nil { @@ -124,3 +125,67 @@ func peerIP(r *http.Request) string { } return host } + +// clientIP is the address login attempts are counted against. It is the TCP +// peer unless the peer is in trusted: then X-Forwarded-For is read from the +// right, past trusted hops, and the first untrusted hop is the client. Entries +// left of that hop come from the client itself and are never used, so a client +// cannot pick its own bucket. With trusted empty the header is ignored. +func clientIP(r *http.Request, trusted []netip.Prefix) string { + peer := peerIP(r) + addr, err := netip.ParseAddr(peer) + if err != nil || !isTrusted(addr, trusted) { + return peer + } + hops := strings.Split(strings.Join(r.Header.Values("X-Forwarded-For"), ","), ",") + for i := len(hops) - 1; i >= 0; i-- { + hop, err := netip.ParseAddr(strings.TrimSpace(hops[i])) + if err != nil { + break // keep the last address a trusted proxy reported + } + addr = hop + if !isTrusted(addr, trusted) { + break + } + } + return addr.Unmap().WithZone("").String() +} + +func isTrusted(a netip.Addr, trusted []netip.Prefix) bool { + a = a.Unmap().WithZone("") + for _, p := range trusted { + if p.Contains(a) { + return true + } + } + return false +} + +// ParseTrustedProxies parses ARTEX_TRUSTED_PROXIES: comma-separated CIDRs (a +// bare address means that one host). Empty means no trusted proxy. +func ParseTrustedProxies(s string) ([]netip.Prefix, error) { + var out []netip.Prefix + for _, f := range strings.Split(s, ",") { + f = strings.TrimSpace(f) + if f == "" { + continue + } + if p, err := netip.ParsePrefix(f); err == nil { + out = append(out, p.Masked()) + continue + } + a, err := netip.ParseAddr(f) + if err != nil || a.Zone() != "" { + return nil, fmt.Errorf("%q is not a CIDR or an IP address", f) + } + a = a.Unmap() + out = append(out, netip.PrefixFrom(a, a.BitLen())) + } + return out, nil +} + +// WithTrustedProxies sets the proxies whose X-Forwarded-For the login rate +// limit reads (see clientIP). +func WithTrustedProxies(p []netip.Prefix) Option { + return func(s *Server) { s.trustedProxies = p } +} diff --git a/server/login_limit_test.go b/server/login_limit_test.go index 6f5b285..255b64c 100644 --- a/server/login_limit_test.go +++ b/server/login_limit_test.go @@ -4,6 +4,7 @@ import ( "encoding/json" "fmt" "net/http/httptest" + "net/netip" "strings" "testing" "time" @@ -142,6 +143,94 @@ func TestLoginRateLimitIgnoresForwardedFor(t *testing.T) { } } +func mustTrusted(t *testing.T, s string) []netip.Prefix { + t.Helper() + p, err := ParseTrustedProxies(s) + if err != nil { + t.Fatal(err) + } + return p +} + +func TestParseTrustedProxies(t *testing.T) { + if p, err := ParseTrustedProxies(" "); err != nil || len(p) != 0 { + t.Fatalf("empty: %v, %v; want no proxies", p, err) + } + p, err := ParseTrustedProxies(" 127.0.0.1/32, 10.1.2.3/8 ,::1") + if err != nil { + t.Fatal(err) + } + if got := fmt.Sprint(p); got != "[127.0.0.1/32 10.0.0.0/8 ::1/128]" { + t.Fatalf("got %s", got) + } + for _, bad := range []string{"proxy.local", "10.0.0.0/33", "127.0.0.1,nope"} { + if _, err := ParseTrustedProxies(bad); err == nil { + t.Fatalf("%q: want an error", bad) + } + } +} + +func TestClientIP(t *testing.T) { + trusted := mustTrusted(t, "127.0.0.1/32,10.0.0.0/8") + for _, tc := range []struct { + name, remote string + xff []string + trusted []netip.Prefix + want string + }{ + {"direct client, no proxies configured", "198.51.100.9:5000", nil, nil, "198.51.100.9"}, + {"direct client, proxies configured", "198.51.100.9:5000", nil, trusted, "198.51.100.9"}, + {"spoofed header, no proxies configured", "198.51.100.9:5000", []string{"203.0.113.1"}, nil, "198.51.100.9"}, + {"spoofed header from an untrusted peer", "198.51.100.9:5000", []string{"203.0.113.1"}, trusted, "198.51.100.9"}, + {"spoofed header claiming a trusted address", "198.51.100.9:5000", []string{"127.0.0.1"}, trusted, "198.51.100.9"}, + {"one trusted proxy", "127.0.0.1:5000", []string{"198.51.100.7"}, trusted, "198.51.100.7"}, + {"client-supplied entries left of the real hop", "127.0.0.1:5000", []string{"203.0.113.1, 10.0.0.9, 198.51.100.7"}, trusted, "198.51.100.7"}, + {"chain of trusted proxies", "127.0.0.1:5000", []string{"203.0.113.1, 198.51.100.7, 10.0.0.5"}, trusted, "198.51.100.7"}, + {"chain split over header lines", "127.0.0.1:5000", []string{"203.0.113.1", "198.51.100.7, 10.0.0.5"}, trusted, "198.51.100.7"}, + {"trusted proxy without the header", "127.0.0.1:5000", nil, trusted, "127.0.0.1"}, + {"unparsable hop stops the walk", "127.0.0.1:5000", []string{"198.51.100.7, garbage, 10.0.0.5"}, trusted, "10.0.0.5"}, + {"IPv6 client behind the proxy", "127.0.0.1:5000", []string{"2001:db8::7"}, trusted, "2001:db8::7"}, + } { + t.Run(tc.name, func(t *testing.T) { + r := httptest.NewRequest("GET", "/", nil) + r.RemoteAddr = tc.remote + for _, v := range tc.xff { + r.Header.Add("X-Forwarded-For", v) + } + if got := clientIP(r, tc.trusted); got != tc.want { + t.Fatalf("clientIP=%s want %s", got, tc.want) + } + }) + } +} + +// Behind a trusted reverse proxy every request arrives from the proxy's address; +// the limit must still apply per real client. +func TestLoginRateLimitBehindTrustedProxy(t *testing.T) { + clk := newFakeClock() + s, post := loginServer(t, clk) + s.trustedProxies = mustTrusted(t, "127.0.0.1/32") + const proxy = "127.0.0.1:50000" + via := func(xff string) map[string]string { return map[string]string{"X-Forwarded-For": xff} } + + for i := 0; i < loginMaxAttempts; i++ { + if w := post(proxy, "wrong-password", via("198.51.100.20")); w.Code != 401 { + t.Fatalf("attempt %d: status=%d want 401", i+1, w.Code) + } + } + if w := post(proxy, oldPassword, via("198.51.100.20")); w.Code != 429 { + t.Fatalf("attacker: status=%d want 429", w.Code) + } + // What nginx's $proxy_add_x_forwarded_for sends when the attacker adds its own header. + if w := post(proxy, oldPassword, via("203.0.113.50, 198.51.100.20")); w.Code != 429 { + t.Fatalf("attacker with a forged entry: status=%d want 429", w.Code) + } + // Another user behind the same proxy is not affected. + if w := post(proxy, oldPassword, via("198.51.100.21")); w.Code != 200 { + t.Fatalf("other user via the proxy: status=%d want 200", w.Code) + } +} + // The 12-character minimum applies to new passwords only. A hash stored under the // old 8-character minimum must still log in. func TestEightCharPasswordLogsInButIsRejectedAsNew(t *testing.T) { diff --git a/server/server.go b/server/server.go index 6f69ffb..48ce4a0 100644 --- a/server/server.go +++ b/server/server.go @@ -9,6 +9,7 @@ import ( "log" "net" "net/http" + "net/netip" "net/url" "os" "path/filepath" @@ -51,7 +52,8 @@ type Server struct { setupMu sync.Mutex setupToken string // one-time token POST /api/auth/init requires; empty when none is active - loginLimit *loginLimiter // per-IP attempt limit on POST /api/auth/login + loginLimit *loginLimiter // per-IP attempt limit on POST /api/auth/login + trustedProxies []netip.Prefix // ARTEX_TRUSTED_PROXIES; see clientIP // concMu serializes concurrency-cap decisions (admission + reconcile) so a // scheduler tick and an HTTP settings change / task creation can't both count