Repository navigation
fix(dashboard): raise the console nginx header buffers for large OIDC tokens - #4376
Conversation
… tokens The nginx config baked into the console image kept the nginx defaults for client_header_buffer_size (1k) and large_client_header_buffers (4 8k). An OIDC access token carrying many group claims arrives as a single Cookie header of several kilobytes, which no longer fits into one buffer, so nginx answered 400 "request header or cookie too large" before the request reached the /apis, /api or /k8s/ upstream. Raise both to 16k, at the top level of that config rather than inside the server block: a header field read before the Host header has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one. Measured against the pinned nginx base image, a 12 KB cookie gets 400 on the old config and 200 on the new one. The ceiling moves rather than disappears: a 20 KB cookie is still rejected. This is the console hop alone. The platform ingress-nginx controller in front of it runs on the same defaults, so where the dashboard is published through an Ingress a token large enough to exceed them is still rejected at the edge; where it is published through the gateway, whose edge has no per-header ceiling of this kind, the console was the hop returning the 400. Raising the controller is a separate change in a separate package. The accompanying check extracts the config from the Containerfile heredoc and fails if either directive is missing, below 16k, or no longer ahead of the first server block: the config lives inside an image build, where no chart template and no helm-unittest case can reach it. Assisted-by: LLM Signed-off-by: Aleksei Sviridkin <[email protected]>
github-actions
Bot
added
size/M
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: cozystack/cozystack/.coderabbit.yaml Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan provides up to 8 included reviews per hour; 7 remain after this review. 📝 WalkthroughWalkthroughThe console image now configures larger nginx client header buffers in its generated default configuration. A Bats test extracts that configuration and verifies buffer sizes and placement before the first ChangesConsole nginx header buffers
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to The console image now supports larger OIDC headers, with no concrete production risk established by the available evidence. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
IvanHunters
left a comment
IvanHunters
left a comment
There was a problem hiding this comment.
Verdict
LGTM with non-blocking notes
I reproduced the bug and the fix against the pinned base image: on main a 12 KB cookie gets 400, on this branch it gets 200, and the gatekeeper-shaped request the PR body describes (kc-access + Authorization + X-Forwarded-Access-Token, 12 KB each) goes 400 to 200 as well. Two notes, neither blocking.
Findings
- [MINOR]
hack/console-nginx-header-buffers.bats:61, the check pins the buffer size but not the buffer count
- [NIT]
packages/system/dashboard/images/console/Containerfile:46,client_header_buffer_size 16kis an optimisation, not part of the fix
Still open elsewhere
Where the dashboard is published through an Ingress, the edge controller still runs on the stock 8k, so a large enough token is rejected a hop earlier. Nothing in the tree raises it: grep -rn 'large_client_header_buffers\|large-client-header-buffers' packages/ hack/ matches only this PR's two files and opencost's own frontend. The commit message says so; the release note alone reads as the whole fix. That is #2853 and out of scope here.
Worth knowing
nginx -t on this config fails with [emerg] host not found in upstream "kubernetes.default.svc" unless the name resolves, so wiring a syntax check into the dashboard package's test target needs --add-host kubernetes.default.svc:127.0.0.1 (or a resolver plus a variable in proxy_pass). Worth doing: nothing parses this config before it ships, and a missing semicolon would satisfy the new check and CrashLoop the console.
|
|
||
| # Third field, because the directive is `large_client_header_buffers <number> <size>` | ||
| # and it is the per-buffer size that bounds one Cookie header. | ||
| large_buffer=$(printf '%s\n' "$conf" | awk '$1 == "large_client_header_buffers" { sub(/;$/, "", $3); print $3 }') |
There was a problem hiding this comment.
[MINOR] the check pins the buffer size but not the buffer count
awk '$1 == "large_client_header_buffers" { ... print $3 }' reads the third field, so the count in large_client_header_buffers <count> <size> is never asserted. A later rewrite can drop it to 1 and the suite stays green while the console answers the exact 400 this PR exists to remove.
$ sed -i 's/large_client_header_buffers 4 16k;/large_client_header_buffers 1 16k;/' \
packages/system/dashboard/images/console/Containerfile
$ hack/cozytest.sh hack/console-nginx-header-buffers.bats
✅ Test OK: console nginx config reads client request headers into buffers larger than the nginx defaults
$ git checkout -- packages/system/dashboard/images/console/Containerfile
What that green config does to the request the PR body describes, the gatekeeper forwarding kc-access, Authorization and X-Forwarded-Access-Token as three fields of 12 KB. Extract the heredoc to head.conf, make count1.conf from it with the count at 1 and base.conf with both directives dropped, then run each in turn:
$ T=$(python3 -c "print('A'*12000)")
$ probe() { # one config per invocation
cid=$(docker run -d --rm --add-host kubernetes.default.svc:127.0.0.1 -p 18098:8080 \
-v "$PWD/$1":/etc/nginx/conf.d/default.conf:ro \
nginxinc/nginx-unprivileged:1.29-alpine@sha256:0c79d56aee561a1d81c63f00eee5fb5fe29279560cdc55e91425133104c7fbe6)
sleep 2
curl -s -o /dev/null -w "$1 %{http_code}\n" -H "Cookie: kc-access=$T" \
-H "Authorization: Bearer $T" -H "X-Forwarded-Access-Token: $T" \
http://127.0.0.1:18098/healthcheck
docker kill "$cid" >/dev/null
}
$ probe head.conf; probe count1.conf; probe base.conf
head.conf 200
count1.conf 400
base.conf 400
Sweeping the count directly, fields of 15 KB so none of them hits the per-field ceiling: 4 16k carries five and rejects the sixth, 1 16k rejects the third. So the count bounds the aggregate, which is the dimension this deployment actually loads three fields onto. Asserting $2 >= 4 alongside the existing $3 >= 16k closes it.
| # than inside the server block: a field read before the Host header has selected | ||
| # the virtual server is measured against the default server's buffers, so the | ||
| # same lines inside a server block cover this one only while it stays default. | ||
| client_header_buffer_size 16k; |
There was a problem hiding this comment.
[NIT] client_header_buffer_size 16k is an optimisation, not part of the fix
large_client_header_buffers alone carries it. With this line removed and client_header_buffer_size left at the nginx default of 1k, both the 12 KB cookie and the three-field gatekeeper-shaped request are accepted:
$ grep -v '^client_header_buffer_size' head.conf > largeonly.conf
$ docker run -d --rm --add-host kubernetes.default.svc:127.0.0.1 -p 18093:8080 \
-v $PWD/largeonly.conf:/etc/nginx/conf.d/default.conf:ro \
nginxinc/nginx-unprivileged:1.29-alpine@sha256:0c79d56aee561a1d81c63f00eee5fb5fe29279560cdc55e91425133104c7fbe6
$ curl -s -o /dev/null -w '%{http_code}' \
-H "Cookie: kc-access=$(python3 -c "print('A'*12000)")" http://127.0.0.1:18093/healthcheck
200
The PR body is right that it saves the escalation to a large buffer. The cost is the other half: this buffer is taken for every connection, the large ones only on demand, in a pod capped at memory: 256Mi (packages/system/dashboard/templates/console.yaml:68). Keep it if you want the escalation gone, but the comment above it names only the benefit, and the check at hack/console-nginx-header-buffers.bats:57 now pins an optimisation at the same severity as the fix.
2 participants
What this PR does
A user whose OIDC access token carries many group claims cannot use the console: nginx answers
400 request header or cookie too largebefore the request reaches the apiserver proxy. The config baked into the console image kept the nginx defaults, 1k forclient_header_buffer_sizeand four buffers of 8k forlarge_client_header_buffers, while the token arrives as one Cookie header several kilobytes long. A header field has to fit into a single buffer, so the per-buffer size is what decides this. Both directives are now 16k, at the top level of that config rather than inside the server block: a header field read before theHostheader has selected the virtual server is measured against the default server's buffers, so the same lines inside a server block cover that server only while it stays the default one.Measured in the pinned nginx base image: a 12 KB cookie gets 400 on the old config and 200 on the new one, a 20 KB one still gets 400.
nginx -taccepts the result.This hop sees more than the browser sent. The gatekeeper in front of it runs with
--pass-access-token=trueand--pass-authorization-header=true, so the request arrives carrying thekc-accesscookie, anAuthorization: Bearerheader andX-Forwarded-Access-Token, three separately bounded header fields where the edge saw one.This is the console image only. The platform ingress-nginx controller also runs on the defaults, which is #2853. Worth reading that trace again before fixing it there: the controller logged
upstream_status 400, so it forwarded the request and the 400 came back from behind the gatekeeper, which is this layer.The check in
hack/console-nginx-header-buffers.batsreads the config out of the Containerfile heredoc and fails when either directive is missing, below 16k, or no longer ahead of the first server block. That config lives inside an image build, so no chart template and no helm-unittest case can reach it.make unit-testspicks the file up on its own.No image reference changes here. CI stamps the console digest.
Closes #3113
Screenshots
No UI change. This is the nginx config in front of the SPA.
Downstream repositories
The diff is the console image's nginx config plus one new
hack/*.batsfile.cozystack-uiis archived and the console lives in this repo, so a console change belongs in this PR. Nothing underhack/moved or was renamed, and no make target changed behaviour.Release note
Summary by CodeRabbit
Bug Fixes
Tests