fix: Require LK_UPSTREAM in the proxy image, and stop leaking upstream credentials - #18
Merged
Merged
Conversation
The proxy image inherited an LK_UPSTREAM default of http://127.0.0.1:8080/v1. That is this container's own loopback, and the image contains no inference engine, so the default could never work there. Forgetting the variable produced a container that started cleanly and then failed every request, and nothing logged the upstream, so there was no signal pointing at the cause. The proxy stage now sets LK_REQUIRE_UPSTREAM, and startup exits when the upstream came from the default rather than from a flag or the environment. Passing the loopback URL explicitly still works, which is what --network host needs. The model-bundled images are unaffected because their entrypoint passes --upstream directly. Also logs the resolved upstream and its source, and names the unreachable upstream in the /health 503 body. Both redact any userinfo in the URL, since an operator can put credentials there and neither a log line nor an unauthenticated response should carry them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The proxy redacted userinfo only on /health and the startup log, and only when it was in the userinfo. Three paths still leaked: - A failed upstream call returned Go's transport error to the client. Go redacts a password there but not a username, so a key placed in the username reached the caller. This affected every proxied route. - A malformed credential URL reached the log through url.Parse's error, which echoes the raw input, so log.Fatal printed the password. - Credentials in a query string, and the opaque http:user:pass@host form, were not stripped by RedactUpstream at all. RedactUpstream now reduces a URL to scheme, host and path, dropping userinfo, query and fragment, and returns a constant for a hostless or unparseable URL. The upstream-failure responses report the redacted upstream rather than the raw error, and server.New no longer wraps the parse error. An explicit empty upstream is now rejected rather than silently replaced with the loopback default. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
GET /v1beta/models and /v1beta/models/{id} reach upstream through
fetchUpstreamJSON, whose error wraps the URL, and both interpolated it raw
into the client response. They now report the redacted upstream like the other
four routes, and the leak test covers all of them.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #14.
Summary
:proxynow requiresLK_UPSTREAMand exits if it is unset. It defaulted tohttp://127.0.0.1:8080/v1, this container's own loopback, which can never hold a model server in an image with no inference engine. Forgetting the variable produced a container that started clean and then failed every request against an address that could not work. TheproxyDockerfile stage sets an internalLK_REQUIRE_UPSTREAMmarker; startup exits when the upstream came from the default rather than a flag or the environment. Setting the loopback explicitly still works, which is what--network hostneeds.--upstreamdirectly, so the source is a flag and they never see the marker. Verified: rebuilt bundled image has the marker unset, sourceflag, healthy in 28s./health503 body names the upstream it could not reach. Nothing previously reported the upstream, so a misconfiguration gave no signal at all.Credential hardening (found by the three required reviews)
The reviews surfaced that an operator can put credentials in the upstream URL (
http://user:pass@host, or?api_key=...), and the URL was reaching clients and logs in several places. All are now closed:log.Fatal, becauseurl.Parse's error echoes the raw input.server.Newno longer wraps it.RedactUpstreamwas incomplete. It now reduces any URL to scheme, host and path, dropping userinfo, query and fragment, and returns a constant for a hostless or unparseable URL (including the opaquehttp:user:pass@hostform).--upstream ""silently reverted to the loopback default. It is now rejected.Deliberate, non-blocking disclosure
The
/healthbody and the upstream-failure error messages name the upstream host and port (credentials stripped). Anyone who can reach the port learns the internal upstream hostname on failure. This is consistent with:proxyalready authenticating none of its callers by design, is documented in the README's security section, and buys real diagnosability. If you would rather not disclose the host at all, say so and I will gate it.Test Plan
make lintclean, all unit tests pass.RedactUpstreamagainst userinfo, password-only userinfo, query, fragment, opaque, scheme-relative and unparseable inputs.TestUpstreamErrorResponsesDoNotLeakCredentialsdrives all eight failing routes against a key-as-username upstream and asserts the key never appears in the 502 body. It fails if the fix is reverted./healthcredential test asserts no password in the body.:proxycontainer pointed at a realllama-serverround-trips OpenAI, Gemini, Anthropic, Gemini streaming (SSE), and Gemini count-tokens (the llama.cpp/tokenizepath). First time the proxy has been exercised against a real engine rather than a stubbed upstream.:proxyexits non-zero whenLK_UPSTREAMis unset, starts when set, starts when the loopback is set explicitly; no credential substring in the startup log, any failing-route body, or/healthwhen the URL carries one.🤖 Generated with Claude Code