front: compare forward-auth peers as addresses, not strings (v0.2.90)

The Digesta host added both 10.89.0.178 and ::ffff:10.89.0.178 to
trusted_proxies and host->8777 still refused. The ::ffff: hunch was right: the
internal nginx listens on 8776 AND [::]:8776, so a connection landing on the v6
socket makes $remote_addr -- and therefore X-Forwarded-For, and therefore the
peer -- the IPv4-mapped form, which a string membership test rejects even when
the plain address is listed. Nothing in the configuration explains that refusal.

Peers are now compared as addresses: a mapped v6 peer matches a plain v4 entry
and the reverse, and CIDR entries work, so a container network can be trusted as
10.89.0.0/24 instead of pinning one address in the quadlet. A non-address entry
(a socket path) is still compared literally, and a v6 peer never matches a v4
network.

Their broadened list could not have taken effect anyway -- it went into the
config.json the app is not reading, which is why `enabled` there did nothing and
the env var was needed. So that test did not rule the peer check out; the list in
force was still the default pair.

The rest of the report is answered in the doc rather than in code, because there
is no bug behind it: `Server: nginx` appears on PROXIED responses too unless
proxy_pass_header Server is set, and the missing `GET /admin` access-log line is
_PollNoiseFilter, which drops reads of /admin, /static, /login and /logout from
the front's access log while leaving /healthz alone -- exactly the asymmetry that
looked like nginx short-circuiting by source. A 302 to /coderai/login with no
Set-Cookie is what the front returns when forward-auth declines.
Co-Authored-By: 's avatarClaude Opus 5 <noreply@anthropic.com>
parent 3cbea24f
...@@ -16,7 +16,7 @@ ...@@ -16,7 +16,7 @@
# Canonical product version for CoderAI — single source of truth. Both the API # Canonical product version for CoderAI — single source of truth. Both the API
# metadata and the admin web UI read from here. # metadata and the admin web UI read from here.
__version__ = "0.2.89" __version__ = "0.2.90"
__tagline__ = "Complete orchestration, distribution and escalation of remotizable advanced inference." __tagline__ = "Complete orchestration, distribution and escalation of remotizable advanced inference."
# Configure the CUDA caching allocator BEFORE torch is imported anywhere. # Configure the CUDA caching allocator BEFORE torch is imported anywhere.
......
...@@ -118,6 +118,50 @@ def register_ui_pages(app: FastAPI, config_dir) -> bool: ...@@ -118,6 +118,50 @@ def register_ui_pages(app: FastAPI, config_dir) -> bool:
return [t for t in (x.strip() for x in raw.replace(",", " ").split()) if t] return [t for t in (x.strip() for x in raw.replace(",", " ").split()) if t]
return [str(t).strip() for t in (cfg.trusted_proxies or []) if str(t).strip()] return [str(t).strip() for t in (cfg.trusted_proxies or []) if str(t).strip()]
def _fa_peer_allowed(peer: str, trusted: list) -> bool:
"""Is this peer one of the trusted proxies?
Compared as ADDRESSES, not strings. The internal nginx listens on both
`8776` and `[::]:8776`, so a connection that lands on the v6 socket makes
$remote_addr -- and therefore X-Forwarded-For, and therefore the peer --
the IPv4-mapped form `::ffff:10.89.0.178`. A string membership test
rejects that even when the operator has listed `10.89.0.178`, which is a
refusal nothing in the configuration explains. CIDR entries work too, so
a container network can be trusted as `10.89.0.0/24` rather than by
pinning one address.
Anything that is not an address or a network (a unix socket path, say) is
still compared literally, so an exotic entry keeps working.
"""
import ipaddress
if not peer:
return False
if peer in trusted: # the common case, and the cheapest
return True
try:
ip = ipaddress.ip_address(peer)
except ValueError:
return False
mapped = getattr(ip, "ipv4_mapped", None)
candidates = [ip] + ([mapped] if mapped else [])
for entry in trusted:
if "/" in entry:
try:
net = ipaddress.ip_network(entry, strict=False)
except ValueError:
continue
if any(c in net for c in candidates if c.version == net.version):
return True
continue
try:
want = ipaddress.ip_address(entry)
except ValueError:
continue
wmapped = getattr(want, "ipv4_mapped", None)
if any(c == w for c in candidates for w in ([want] + ([wmapped] if wmapped else []))):
return True
return False
def _fa_reject(peer: str, reason: str) -> bool: def _fa_reject(peer: str, reason: str) -> bool:
"""Say once, per peer and reason, why a forward-auth attempt was refused.""" """Say once, per peer and reason, why a forward-auth attempt was refused."""
import time import time
...@@ -147,7 +191,7 @@ def register_ui_pages(app: FastAPI, config_dir) -> bool: ...@@ -147,7 +191,7 @@ def register_ui_pages(app: FastAPI, config_dir) -> bool:
import hmac import hmac
peer = (request.client.host if request.client else "") or "" peer = (request.client.host if request.client else "") or ""
trusted = _fa_peer_allowlist(cfg) trusted = _fa_peer_allowlist(cfg)
if peer not in trusted: if not _fa_peer_allowed(peer, trusted):
return _fa_reject(peer, f"peer is not in trusted_proxies {trusted}") return _fa_reject(peer, f"peer is not in trusted_proxies {trusted}")
sent = request.headers.get(cfg.shared_secret_header.lower(), "") or "" sent = request.headers.get(cfg.shared_secret_header.lower(), "") or ""
if not sent: if not sent:
......
...@@ -214,6 +214,46 @@ host→8777 mints would lock everyone out (no coderai passwords; SSO would be th ...@@ -214,6 +214,46 @@ host→8777 mints would lock everyone out (no coderai passwords; SSO would be th
The Digesta side is ready — `GET /api/authz/coderai` returns `204` + `X-User` for a Digesta The Digesta side is ready — `GET /api/authz/coderai` returns `204` + `X-User` for a Digesta
admin, `401` otherwise, for nginx `auth_request`. admin, `401` otherwise, for nginx `auth_request`.
### Update — 0.2.87, sharper repro (the bug is BEFORE the front, on the published-port path)
Two things ruled out, one thing pinned down:
- **Not a `trusted_proxies` string mismatch.** Broadened it to
`["127.0.0.1","::1","::ffff:127.0.0.1","10.89.0.178","::ffff:10.89.0.178","10.89.0.1","::ffff:10.89.0.1"]`
(covering IPv4-mapped-IPv6 and gateway forms) and restarted — host→8777 `/admin` **still** `302`s.
- **The `/admin` request never reaches the front on the published-port path.** For host→`127.0.0.1:8777/admin`
the response is `302 → /coderai/login` with **`Server: nginx`**, and the **front's uvicorn access log shows
no `/admin` line at all** (while it logs `/healthz` 200 from the same host→8777 path). So the **internal
nginx returns the 302 itself**, without proxying `/admin` to the front.
- **Yet the same `/admin` DOES reach the front and mint from inside the container.** `curl …
http://127.0.0.1:8776/admin` (internal nginx, source `127.0.0.1`) with the same secret + `X-Forwarded-*`
headers → `200 + Set-Cookie`. And `http://127.0.0.1:18776/admin` (front direct) likewise mints.
So the internal nginx treats `/admin` differently **by source**: proxied to the front from loopback
(`127.0.0.1`), but short-circuited to a `/coderai/login` 302 from the rootless published-port source
(`10.89.0.178`). `/healthz` is proxied to the front from both sources. The nginx.conf we can read
(`/etc/nginx/nginx.conf`: `map` blocks for `X-Forwarded-*`, sub-app `location`s, `location / { proxy_pass
http://coderai; }`) has **no `/admin` block and no source/`allow`/`deny`/`auth_request`/`geo` rule** — so the
gate is somewhere we can't see (an included conf, a baked front/nginx behaviour, or the front proxy upstream
failing for `/admin` from non-loopback and mapping to a login redirect). **This is the thing to fix**: `/admin`
must reach the front over the published-port path exactly as `/healthz` does. A quick repro on any rootless
podman host: publish the full image's `8776` on a loopback port and `curl host:PORT/admin` with the secret +
`X-Forwarded-User`/`-Groups` headers — it 302s to `/coderai/login` and never hits the front, whereas the same
from inside the container mints.
We also tried to **bypass the internal nginx** by publishing the front (`18776`) on its own loopback host
port and pointing at it directly — but the **front binds `127.0.0.1:18776` *inside the container*** (loopback
only), so a published port to `18776` reaches the container's eth0 where nothing listens (connection refused).
The internal nginx on `8776` is the **only** thing reachable from the host, so the `/admin` short-circuit
cannot be worked around at the proxy/ops layer — **the fix has to be in the image** (internal nginx and/or the
front), making `/admin` reach the front over the published-port path the same way `/healthz` already does.
Until this is fixed, the host nginx on prod stays on **basic-auth** (not swapped to `auth_request`), to avoid
locking everyone out of the GUI via the one path nginx uses. Everything else (Digesta `/api/authz/coderai`
endpoint, the quadlet env + config, the pinned container IP) is ready for the swap the moment host→8777 `/admin`
mints.
--- ---
## As shipped (0.2.85) ## As shipped (0.2.85)
...@@ -366,3 +406,132 @@ test with. ...@@ -366,3 +406,132 @@ test with.
mints**, and you no longer have to choose: with `keep_local_login: true` the mints**, and you no longer have to choose: with `keep_local_login: true` the
password form stays as a loopback fallback, so a broken SSO cannot lock you out. password form stays as a loopback fallback, so a broken SSO cannot lock you out.
Verify the mint through the real path first, then swap. Verify the mint through the real path first, then swap.
---
## Answer to the 0.2.87 follow-up: the 302 IS the front refusing
The sharper repro is good work, but both pillars of "the bug is before the front"
have innocent explanations, and the test that was meant to rule out
`trusted_proxies` could not have.
### `Server: nginx` does not mean nginx wrote the response
nginx sets its own `Server` header on **proxied** responses too, unless you add
`proxy_pass_header Server`. So that header is present whether the 302 came from
nginx or was passed through from the front. It carries no information here.
### The missing `/admin` access-log line is a log filter, not a missing request
`codai/frontproxy/app.py` has `_PollNoiseFilter`, installed on the front's
uvicorn **access** handler unless `--debug-web`:
```python
_READ = ("GET", "HEAD", "OPTIONS")
_WEB_PREFIXES = ("/admin", "/static", "/login", "/logout")
```
A `GET /admin` is **dropped from the access log, from every source**. `/healthz`
is not in that list, which is exactly the asymmetry that was observed:
| request | access log |
|---|---|
| `GET /admin` | **dropped** |
| `GET /healthz` | logged |
| `GET /coderai/admin` | logged (the prefix is not stripped yet) |
So "no `/admin` line" is the expected output for a request the front *did*
handle — the in-container test that minted was not logged either. Re-run any of
this with `--debug-web` and the lines appear. There is no nginx rule to find,
which is also why none was found: `location /` proxies `/admin` to the front
from both sources, as `/healthz` demonstrates.
A `302 → /coderai/login` with no `Set-Cookie` is precisely what the front
returns when forward-auth declines: the middleware calls `_fa_login`, gets
nothing, and falls through to the normal "no session" redirect.
### Why broadening `trusted_proxies` changed nothing
It was broadened in `config.json` — the file we established the app is **not
reading**, which is why `enabled: true` there did nothing and
`CODERAI_ADMIN_FORWARD_AUTH=1` was needed. Editing an unread file cannot change
behaviour, so that test does not exonerate the peer check. The list actually in
force was still the default `["127.0.0.1", "::1"]`: loopback in, container
address out. One cause, all three symptoms.
### And a second, independent bug the `::ffff:` instinct was right about
Adding `::ffff:10.89.0.178` was the correct hunch. The internal nginx listens on
**both** `8776` and `[::]:8776`, so a connection landing on the v6 socket makes
`$remote_addr` — and therefore `X-Forwarded-For`, and therefore the peer — the
IPv4-mapped form. The check compared **strings**, so `::ffff:10.89.0.178` was
refused even with `10.89.0.178` listed, and nothing in the configuration
explained the refusal.
Fixed in **0.2.90**: peers are compared as addresses. A mapped v6 peer matches
the plain v4 entry and vice versa, and **CIDR entries work**, so a container
network is trusted as `10.89.0.0/24` instead of pinning one address with
`Network=digesta-net:ip=…`. A non-address entry is still compared literally.
## Do this, in this order
**1. Upgrade.** `production` carries 0.2.90.
```bash
CODERAI_UPGRADE_REF=production coderai-docker --podman --upgrade \
ghcr.io/nextime/coderai:latest
systemctl --user restart coderai.service
podman exec coderai grep -m1 __version__ /opt/coderai/app/codai/__init__.py
```
Or pull the signed image — `ghcr.io/nextime/coderai:0.2.88` is published and
`cosign verify`s; 0.2.89/0.2.90 are reachable by `--upgrade`.
**2. Put the peer list where it is read, by env, so the config path stops
mattering** — in the quadlet, beside the two variables already there:
```ini
Environment=CODERAI_ADMIN_FORWARD_AUTH=1
Environment=CODERAI_ADMIN_FORWARD_AUTH_SECRET=…
Environment=CODERAI_ADMIN_FORWARD_AUTH_TRUSTED_PROXIES=127.0.0.1,::1,10.89.0.0/24
```
New in 0.2.88. Previously the switch and the secret could come from a unit file
but the peers could only come from `config.json` — so the feature could be
enabled by env and then reject every request from its own proxy.
**3. Then one request settles it.** 0.2.88+ says why it refused, once per peer
and reason per 60s:
```
[front] admin forward-auth refused: peer is not in trusted_proxies
['127.0.0.1', '::1'] (peer '10.89.0.178')
```
If the list in that line is the default pair, `config.json` is still unread and
step 2 is what makes it work. If it names the peer and the line is about the
secret instead, the peer check passed and the secret is the remaining problem.
Either way the next move is in the message rather than in a repro.
**4. Fix the config path anyway**, so Settings and everything else apply:
```bash
podman exec coderai sh -lc 'ls -l ~/.coderai/config.json; \
python3 -c "import json;print(json.load(open(\"$HOME/.coderai/config.json\")).get(\"admin\"))"'
```
The app resolves config from the HOME-style `$CODERAI_CONFIG_DIR/coderai`; a
file at `$CODERAI_CONFIG_DIR/config.json` is silently ignored.
**5. Swap nginx to `auth_request`** once host→8777 mints. With
`keep_local_login: true` the password form stays as a loopback fallback, so a
broken SSO cannot lock anyone out — the basic-auth caution was right while the
mint was unproven, and stops being necessary once it works.
### Not worth pursuing
- There is no `/admin` short-circuit in nginx to find.
- Publishing the front's `18776` cannot work: it binds `127.0.0.1` inside the
container by design, so the internal nginx is the only reachable entry point.
That part of the write-up is correct and the conclusion stands — the fix had to
be in the image. It is, just not in nginx.
...@@ -68,7 +68,9 @@ def test_the_config_round_trips(tmp_path): ...@@ -68,7 +68,9 @@ def test_the_config_round_trips(tmp_path):
# --------------------------------------------------------- the trust contract # --------------------------------------------------------- the trust contract
def test_both_checks_are_required_not_either(): def test_both_checks_are_required_not_either():
blk = UI.split("def _fa_trusted")[1].split("def _fa_identity")[0] blk = UI.split("def _fa_trusted")[1].split("def _fa_identity")[0]
assert "peer not in trusted" in blk # The peer is checked as an ADDRESS (mapped IPv6 and CIDR forms included),
# not by string membership -- see tests/test_forward_auth_peer_trust.py.
assert "_fa_peer_allowed(peer, trusted)" in blk
assert "compare_digest" in blk, "the secret must not be compared with ==" assert "compare_digest" in blk, "the secret must not be compared with =="
# The peer check and the secret check are separate early returns, so neither # The peer check and the secret check are separate early returns, so neither
# alone can authenticate. Each refusal goes through _fa_reject, which returns # alone can authenticate. Each refusal goes through _fa_reject, which returns
......
...@@ -162,3 +162,98 @@ def test_the_docstring_no_longer_claims_the_immediate_peer(): ...@@ -162,3 +162,98 @@ def test_the_docstring_no_longer_claims_the_immediate_peer():
blk = UI.split("def _fa_trusted(")[1].split("def _fa_identity")[0] blk = UI.split("def _fa_trusted(")[1].split("def _fa_identity")[0]
assert "immediate peer" not in blk assert "immediate peer" not in blk
assert "ProxyHeadersMiddleware" in blk, "say where the value comes from" assert "ProxyHeadersMiddleware" in blk, "say where the value comes from"
# ------------------------------------------------- the peer is an ADDRESS, not a string
# Second report from the Digesta host (0.2.87): the operator broadened
# trusted_proxies to include both `10.89.0.178` and `::ffff:10.89.0.178` and
# host->8777 still refused. The `::ffff:` instinct was right -- the internal
# nginx listens on `8776` AND `[::]:8776`, so a connection landing on the v6
# socket makes $remote_addr, X-Forwarded-For and therefore the peer the
# IPv4-mapped form -- but a plain string membership test rejects that even with
# the plain address listed, and the broadened list went into the config file the
# app was not reading anyway.
def _allowed(peer, trusted):
"""Mirror of _fa_peer_allowed, which is a closure inside register_ui_pages."""
import ipaddress
if not peer:
return False
if peer in trusted:
return True
try:
ip = ipaddress.ip_address(peer)
except ValueError:
return False
mapped = getattr(ip, "ipv4_mapped", None)
candidates = [ip] + ([mapped] if mapped else [])
for entry in trusted:
if "/" in entry:
try:
net = ipaddress.ip_network(entry, strict=False)
except ValueError:
continue
if any(c in net for c in candidates if c.version == net.version):
return True
continue
try:
want = ipaddress.ip_address(entry)
except ValueError:
continue
wmapped = getattr(want, "ipv4_mapped", None)
if any(c == w for c in candidates for w in ([want] + ([wmapped] if wmapped else []))):
return True
return False
def test_the_helper_in_the_source_matches_this_mirror():
"""If the implementation drifts from the mirror above, these tests lie."""
blk = UI.split("def _fa_peer_allowed")[1].split("def _fa_reject")[0]
for marker in ("ipv4_mapped", 'if "/" in entry:', "ip_network(entry, strict=False)",
"if peer in trusted:"):
assert marker in blk, marker
def test_a_mapped_peer_matches_the_plain_address():
"""The reported failure, in one line."""
assert _allowed("::ffff:10.89.0.178", ["127.0.0.1", "::1", "10.89.0.178"])
def test_a_plain_peer_matches_a_mapped_entry():
"""The reverse, so an operator who listed the ::ffff: form is not punished."""
assert _allowed("10.89.0.178", ["::ffff:10.89.0.178"])
def test_mapped_loopback_is_still_loopback():
assert _allowed("::ffff:127.0.0.1", ["127.0.0.1", "::1"])
def test_a_container_network_can_be_trusted_as_cidr():
"""Better than pinning one address with Network=digesta-net:ip=…"""
assert _allowed("10.89.0.178", ["10.89.0.0/24"])
assert _allowed("::ffff:10.89.0.178", ["10.89.0.0/24"])
def test_an_address_outside_the_network_is_still_refused():
assert not _allowed("10.90.0.5", ["10.89.0.0/24"])
assert not _allowed("10.89.0.5", ["10.89.0.178"])
def test_an_empty_peer_is_refused():
"""request.client is None on some transports; that is not 'trusted'."""
assert not _allowed("", ["127.0.0.1"])
def test_a_non_address_entry_is_still_compared_literally():
"""A unix socket path or hostname entry must keep working."""
assert _allowed("/run/nginx.sock", ["/run/nginx.sock"])
assert not _allowed("10.0.0.1", ["/run/nginx.sock"])
def test_a_malformed_entry_does_not_crash_or_open_the_door():
assert not _allowed("10.89.0.178", ["not-an-address", "10.89.0.0/999"])
def test_widening_is_never_implicit():
"""A v6 peer must not match a v4 network just because the bits line up."""
assert not _allowed("2001:db8::1", ["10.89.0.0/24"])
Markdown is supported
0% or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment