Skip to content

server: fix incorrect headers passed by oauth2-proxy#9468

Merged
Michaelvll merged 2 commits into
skypilot-org:masterfrom
webconn:bugfix/hop-by-hop-headers-oauth2-proxy
May 26, 2026
Merged

server: fix incorrect headers passed by oauth2-proxy#9468
Michaelvll merged 2 commits into
skypilot-org:masterfrom
webconn:bugfix/hop-by-hop-headers-oauth2-proxy

Conversation

@webconn

@webconn webconn commented Apr 28, 2026

Copy link
Copy Markdown
Contributor

SkyPilot API server passes replies from oauth2-proxy with all its headers, which may lead to confusing situations, such as having both Content-Lentgth and Transfer-Encoding headers in the same reply, which lead to protocol errors in a reverse proxy.

It's considered a good practice to ignore hop-by-hop headers in such cases.

Tested (run the relevant ones):

  • Code formatting: install pre-commit (auto-check on commit) or bash format.sh
  • Any manual or new tests for this PR (please specify below)
  • All smoke tests: /smoke-test (CI) or pytest tests/test_smoke.py (local)
  • Relevant individual tests: /smoke-test -k test_name (CI) or pytest tests/test_smoke.py::test_name (local)
  • Backward compatibility: /quicktest-core (CI) or pytest tests/smoke_tests/test_backward_compat.py (local)

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request implements the filtering of hop-by-hop headers in the OAuth2 proxy middleware to ensure proper protocol compliance when forwarding responses. The changes include defining a set of standard hop-by-hop headers and excluding them from the response headers sent to the client. Feedback was provided to use a frozenset for the header collection to ensure immutability as a module-level constant.

Comment thread sky/server/auth/oauth2_proxy.py Outdated
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
@Michaelvll Michaelvll requested review from aylei and cblmemo May 13, 2026 18:40

@aylei aylei left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @webconn !

@webconn

webconn commented May 26, 2026

Copy link
Copy Markdown
Contributor Author

@cblmemo what do you think?

@Michaelvll Michaelvll merged commit d77cdd4 into skypilot-org:master May 26, 2026
21 checks passed
@webconn webconn deleted the bugfix/hop-by-hop-headers-oauth2-proxy branch May 27, 2026 09:55
@tmathew0309

Copy link
Copy Markdown

We hit exactly this in a Helm deployment (chart 0.12.0, also reproduced on 0.12.3.post1) using native OIDC (auth.oauth.enabled) behind ingress-nginx: every /oauth2/callback response — success or error — came back as an opaque nginx 502 Bad Gateway, because the API server's relay copied Transfer-Encoding from the oauth2-proxy response alongside Content-Length, which nginx rejects as a protocol error. Other /oauth2/* endpoints (302 redirects with plain Content-Length framing) relayed fine, which made it quite painful to diagnose.

We confirmed this fix resolves it: pinning apiService.image to berkeleyskypilot/skypilot-nightly:1.0.0.dev20260527 (first nightly containing this commit) immediately fixed the callback, and browser SSO works end-to-end.

Since sky/server/auth/oauth2_proxy.py is unchanged from v0.12.0 through v0.12.3.post1, native OIDC behind any nginx-based ingress appears broken in every current release. Any chance this could be cherry-picked into a 0.12.x post release so users don't need to run nightly server images for SSO?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants