cc @neild

It looks like the intent of the code is, if X-Forwarded-Proto is not present, ...

I think the intent is to check that it is not explicitly set to nil in which case it would not be updated.

Why do you think proxy should pass per-existing value?

It looks like the intent of the code is, if X-Forwarded-Proto is not present, ...

I think the intent is to check that it is not explicitly set to nil in which case it would not be updated.

Why do you think proxy should pass per-existing value?

It's not a standard HTTP header, so I don't know if I can provide an authoritative reference here, but MDN says: "The X-Forwarded-Proto (XFP) header is a de-facto standard header for identifying the protocol (HTTP or HTTPS) that a client used to connect to your proxy or load balancer." If X-Forwarded-Proto is already present, then presumably there is another proxy in front of httputil.ReverseProxy that set that header, and has better information about the protocol the client used. So I think we would want to respect that.

@neild discussed this bug in the release weekly meeting, any updates? Thanks

This was added by https://go.dev/cl/407414. The intent is to set X-Forwarded-Proto based on the inbound request that we're forwarding. If an inbound request has an X-Forwarded-Proto header, we have no way to tell whether it should be trusted or not. Setting a header that says we received a request over HTTPS when it actually arrived over HTTP seems wrong.

Unfortunately, the ReverseProxy.Director API doesn't give us any way to both set X-Forwarded-Proto by default and to allow the user to override that decision.

The new ReverseProxy.Rewrite API being added in Go 1.20 makes this simple; a Rewrite func can choose to preserve the X-Forwarded-Proto from the inbound request, overwrite it, or whatever.

The whole point of adding Rewrite is that it's impossible to satisfy everyone with the Director API. For example, whatever we do with X-Forwarded-Proto is going to not be what some people want.

Let's revert the change to set X-Forwarded-Host and X-Forwarded-Proto automatically. DIrector can stay frozen as it is was as of 1.18. Aside from security fixes, future improvements can be limited to Rewrite, which is a better base for adding new features without breaking existing users.

Change https://go.dev/cl/457595 mentions this issue: net/http/httputil: don't add X-Forwarded-{Host,Proto} after invoking Director funcs

It sounds like the existing semantics will be preserved, and improvements will be limited to Rewrite. This sounds good to me. I just wanted to chime in on part of the comment below to provide some context from our use case for posterity's sake.

This was added by https://go.dev/cl/407414. The intent is to set X-Forwarded-Proto based on the inbound request that we're forwarding. If an inbound request has an X-Forwarded-Proto header, we have no way to tell whether it should be trusted or not. Setting a header that says we received a request over HTTPS when it actually arrived over HTTP seems wrong.

In our use case, a web client (i.e. browser) talks to a cloud provider frontend (e.g. Google Cloud Load Balancer) which sets X-Forwarded-Proto according to the connection used by the web client. GCLB and Cloudflare both document using X-Forwarded-Proto in this manner, and I suspect others do as well.

While it's generally advisable that the cloud provider frontend communicates with its backend over a secure connection, this connection is distinct from the web client <=> cloud frontend connection, which the cloud frontend communicates the protocol used via X-Forwarded-Proto to its backend.

In our use case, we sometimes will insert yet another reverse proxy between the cloud frontend proxy and our app servers. This reverse proxy uses Go's ReverseProxy. Arguably, we could/should define a separate header to communicate the protocol used for the original web client <=> cloud provider frontend connection, but we wrote it as if it were a transparent proxy. That said, MDN indicates that X-Forwarded-Proto should preserve the original protocol between the client and the load balancer:

"The X-Forwarded-Proto (XFP) header is a de-facto standard header for identifying the protocol (HTTP or HTTPS) that a client used to connect to your proxy or load balancer. Your server access logs contain the protocol used between the server and the load balancer, but not the protocol used between the client and the load balancer. To determine the protocol used between the client and the load balancer, the X-Forwarded-Proto request header can be used."

and I suspect others do as well.

AWS ALB does the same https://docs.aws.amazon.com/elasticloadbalancing/latest/application/x-forwarded-headers.html#x-forwarded-proto (text looks identical to MDN docs)