Skip to content
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

reverse_proxy leads to duplicate Server headers #6275

Closed
andersk opened this issue Apr 26, 2024 · 6 comments
Closed

reverse_proxy leads to duplicate Server headers #6275

andersk opened this issue Apr 26, 2024 · 6 comments
Labels
discussion 💬 The right solution needs to be found

Comments

@andersk
Copy link

andersk commented Apr 26, 2024

When using Caddy’s reverse_proxy in front of a server that sets a Server header, Caddy prepends its own Server header rather than replacing it, leading to duplicate Server headers. This violates RFC 9110 §5.3:

This means that, aside from the well-known exception noted below, a sender MUST NOT generate multiple field lines with the same name in a message (whether in the headers or trailers) or append a field line when a field line of the same name already exists in the message, unless that field's definition allows multiple field line values to be recombined as a comma-separated list (i.e., at least one alternative of the field's definition allows a comma-separated list, such as an ABNF rule of #(values) defined in Section 5.6.1).

as Server is not defined as comma-separated.

Caddyfile for self-contained reproduction:

http://localhost:1234 {
	reverse_proxy http://localhost:1235
}

http://localhost:1235 {
	reverse_proxy http://localhost:1236
}

http://localhost:1236 {
	header Server "MyServer"
	respond "Hello"
}
# caddy version
v2.7.6 h1:w0NymbG2m9PcvKWsrXO6EEkY9Ru4FJK8uQbYcev1p3A=
# caddy run
2024/04/26 20:13:00.317	INFO	using adjacent Caddyfile
2024/04/26 20:13:00.318	INFO	admin	admin endpoint started	{"address": "localhost:2019", "enforce_origin": false, "origins": ["//localhost:2019", "//[::1]:2019", "//127.0.0.1:2019"]}
2024/04/26 20:13:00.318	INFO	tls.cache.maintenance	started background certificate maintenance	{"cache": "0xc0000b6000"}
2024/04/26 20:13:00.319	INFO	http.log	server running	{"name": "srv0", "protocols": ["h1", "h2", "h3"]}
2024/04/26 20:13:00.319	INFO	http.log	server running	{"name": "srv1", "protocols": ["h1", "h2", "h3"]}
2024/04/26 20:13:00.319	INFO	http.log	server running	{"name": "srv2", "protocols": ["h1", "h2", "h3"]}
2024/04/26 20:13:00.319	INFO	autosaved config (load with --resume flag)	{"file": "/root/.config/caddy/autosave.json"}
2024/04/26 20:13:00.319	INFO	serving initial configuration
2024/04/26 20:13:00.365	WARN	tls	storage cleaning happened too recently; skipping for now	{"storage": "FileStorage:/root/.local/share/caddy", "instance": "29a71809-9042-40ac-854b-3b22247b7194", "try_again": "2024/04/27 20:13:00.365", "try_again_in": 86399.999998755}
2024/04/26 20:13:00.365	INFO	tls	finished cleaning storage units
^Z
[1]+  Stopped                 caddy run
# bg
[1]+ caddy run &
# curl -i http://localhost:1234
HTTP/1.1 200 OK
Content-Length: 5
Content-Type: text/plain; charset=utf-8
Date: Fri, 26 Apr 2024 20:13:10 GMT
Server: Caddy
Server: Caddy
Server: MyServer

Hello
@francislavoie
Copy link
Member

This is working as intended. This behaviour makes it a really useful debugging tool to determine how the request was routed. If you want to drop the Server header, you can just do header -Server.

@francislavoie francislavoie closed this as not planned Won't fix, can't repro, duplicate, stale Apr 26, 2024
@mholt
Copy link
Member

mholt commented Apr 26, 2024

That was initially intentional, but RFC 9110 didn't exist at the time either.

We can probably update our behavior now that RFC 9110 clearly states that origin servers generate this header, not proxies.

It is extremely useful for simple debugging/tracing, but that can also be done with something like header +Proxy-Server Caddy or something.

@mholt
Copy link
Member

mholt commented Apr 26, 2024

Oh, Francis beat me to it. (Dang GitHub not telling me there's a reply while I'm typing.) I'm going to reopen this until we fix it.

@mholt mholt reopened this Apr 26, 2024
@mholt mholt added the bug 🐞 Something isn't working label Apr 26, 2024
@francislavoie
Copy link
Member

We can probably update our behavior now that RFC 9110 clearly states that origin servers generate this header, not proxies.

Nginx's opinion is that a reverse proxy (not "proxy", as in forward) is an origin, and therefore generates its own Server header. In fact, Nginx's default behaviour is to ignore upstream's Server header and only write its own, unless you add a proxy_pass config line for it.

@mholt
Copy link
Member

mholt commented Apr 26, 2024

That's true, but also antiquated. RFC 9110 conveniently defines origin servers (as opposed to intermediates) for us: https://www.rfc-editor.org/rfc/rfc9110.html#name-origin-server

@mholt
Copy link
Member

mholt commented Apr 27, 2024

Now, I'm one to be quick to want to comply with the standard. So is Francis. So Francis and I discussed this in Slack, and I actually find his arguments quite compelling.

  • We are already conforming with the de-facto standard behaviour. This is true, nginx has done this for years and years, and is what a lot of the Web expects, even though the latest standard potentially says otherwise. (It does leave some room for interpretation, read on.)

  • A reverse proxy is an origin. The same RFC 9110 says in Section 3.7:

    A "gateway" (a.k.a. "reverse proxy") is an intermediary that acts as an origin server for the outbound connection but translates received requests and forwards them inbound to another server or servers.

    I was under the impression that a proxy is just a relay/intermediate, but the spec actually says reverse proxies act as origins, thus it follows that they should be allowed to set a Server header. (I can see how a forward proxy would NOT be an origin. But a reverse proxy? It is an authority to respond for its domain.)

  • What is the actual problem? We are not sure what problem this is causing (as it is not new behavior, by a long shot) and what changing this would really fix. But we do know that changing this would actually cause us some problems... next point:

  • The current behaviour is the most useful, and can be opted-out very easily. As Francis said to me, "The amount of times I've been able to successfully debug things on the forums because of Server: Caddy appearing twice... very very useful. If we hid it by default when proxying then we have no way to prove that the request did get handled by Caddy, especially when people have their DNS wrong and they think they're hitting Caddy but they're not." Having this current behaviour be the default is highly advantageous to our helpers.

  • The RFC strictness on multiple header fields is intended for other use cases. I believe, IIRC, that our Server header is actually ordered appropriately, for one thing. So that part is still in compliance with the RFC regarding order mattering. While it does seem to suggest that only list-formatted fields can appear multiple times, the Server field is kind of a list, it's just space-separated instead of comma-separated, purely for compatibility reasons I suppose. But the semantics are similar.

While I definitely don't want to cause breakage, I can see both sides of the argument here. The RFC does seem to say that there should only be 1 Server header, but distinguishing comma-separated lists from space-separated lists seems silly in this context. It also seems that a reverse proxy is, in itself, an origin, even before considering the de-facto standard behavior today. The presence of our Server header is also extremely useful in helping people.

Is the current behavior causing some sort of problem?

@mholt mholt closed this as not planned Won't fix, can't repro, duplicate, stale Apr 30, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
discussion 💬 The right solution needs to be found
Projects
None yet
Development

No branches or pull requests

3 participants