Skip to content

proxy: unsupported upgrade fallback ignores backend headers #3456

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
AlexanderYastrebov opened this issue Mar 24, 2025 · 3 comments
Open

Comments

@AlexanderYastrebov
Copy link
Member

AlexanderYastrebov commented Mar 24, 2025

Whenever Skipper sees connection upgrade headers it assumes connection will be upgraded and when status is not 101 it tries to serve the backend response to the client.
In doing so it completely ignores backend response headers

skipper/proxy/upgrade.go

Lines 125 to 134 in c3b1657

if resp.StatusCode != http.StatusSwitchingProtocols {
log.Debugf("Got invalid status code from backend: %d", resp.StatusCode)
w.WriteHeader(resp.StatusCode)
_, err := io.Copy(w, resp.Body)
if err != nil {
log.Errorf("Error writing body to client: %s", err)
return
}
return
}

which results in Content-Type: text/plain; charset=utf-8, missing backend headers and Skipper branding

skipper/proxy/proxy.go

Lines 929 to 934 in c3b1657

// addBranding overwrites any existing `X-Powered-By` or `Server` header from headerMap
func addBranding(headerMap http.Header) {
if headerMap.Get("Server") == "" {
headerMap.Set("Server", "Skipper")
}
}

Tentatively this code path should reuse proxy code to serve a backend response.

See related HTTP/1.1 TLS Upgrade (RFC-2817) should not be default.

@AlexanderYastrebov
Copy link
Member Author

Handling of non 101 status was added by #921 and changed to send the body by #1291

@AlexanderYastrebov
Copy link
Member Author

AlexanderYastrebov commented Mar 24, 2025

As a workaround for https://issues.apache.org/jira/browse/HTTPCLIENT-2344 one may add dropRequestHeader("Connection") -> dropRequestHeader("Upgrade") filters.

It would be handy to drop a specific Connection: Upgrade and e.g. Upgrade: TLS/1.0 values only so I've created #3457

@AlexanderYastrebov AlexanderYastrebov changed the title upgrade: failed/unsupported upgrade ignores backend headers proxy: unsupported upgrade fallback ignores backend headers Mar 24, 2025
@szuecs
Copy link
Member

szuecs commented Mar 25, 2025

Interesting issue!

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

No branches or pull requests

2 participants