Honor X-Forwarded-Port header by miminar · Pull Request #2008 · distribution/distribution

miminar · 2016-10-17T13:07:47Z

If the explicit port is specified using this header, use it as a base for redirect urls.

As documented in article "HTTP Headers and Elastic Load Balancing" of AWS ELB docs:

The X-Forwarded-Port request header helps you identify the port that an HTTP or HTTPS load balancer uses to connect to the client.

This makes registry behave correctly when proxy strips port from the requests e.g.:

GET /openshift/token?account=unused&scope=repository%3Apjoe%2Fhello-world%3Apush%2Cpull HTTP/1.1
Host: registry.f24-ose.vm
User-Agent: docker/1.12.1 go/go1.7 git-commit/23cf638 kernel/4.7.6-1-ARCH os/linux arch/amd64 UpstreamClient(Docker-Client/1.12.1 \(linux\))
Authorization: Basic dW51c2VkOlV1WGoyTGdTWkk4TGc0azY1bXFwc3ZlT1VKdXg1NnFDMTJXMThRdUdYOG8=
Accept-Encoding: gzip
Connection: close
X-Forwarded-Host: registry.f24-ose.vm
X-Forwarded-Port: 5000
X-Forwarded-Proto: http
Forwarded: for=192.168.100.80;host=registry.f24-ose.vm;proto=http
X-Forwarded-For: 192.168.100.80

Where original request (docker daemon -> registry's proxy) looks like this:

GET /openshift/token?account=unused&scope=repository%3Apjoe%2Fhello-world%3Apush%2Cpull HTTP/1.1
Host: registry.f24-ose.vm:5000
User-Agent: docker/1.12.1 go/go1.7 git-commit/23cf638 kernel/4.7.6-1-ARCH os/linux arch/amd64 UpstreamClient(Docker-Client/1.12.1 \(linux\))
Authorization: Basic dW51c2VkOlV1WGoyTGdTWkk4TGc0azY1bXFwc3ZlT1VKdXg1NnFDMTJXMThRdUdYOG8=
Accept-Encoding: gzip
Connection: close

Without this patch, during a POST /v2/pjoe/hello-world/blobs/uploads/ HTTP/1.1 request, registry would redirect to port-less url registry.f24-ose.vm which would make docker daemon attempt to upload on port 80 and fail.

With this patch, registry will reply with correct location:

HTTP/1.1 202 Accepted
Content-Length: 0
Docker-Distribution-Api-Version: registry/2.0
Docker-Upload-Uuid: 24f37d8f-312d-45d4-b507-5a45923daf14
Location: http://registry.f24-ose.vm:5000/v2/pjoe/hello-world/blobs/uploads/24f37d8f-312d-45d4-b507-5a45923daf14?_state=g0583DtjmtCDcE3a8fSJbTwoM70F5BL15Omx058lFLx7Ik5hbWUiOiJwam9lL2hlbGxvLXdvcmxkIiwiVVVJRCI6IjI0ZjM3ZDhmLTMxMmQtNDVkNC1iNTA3LTVhNDU5MjNkYWYxNCIsIk9mZnNldCI6MCwiU3RhcnRlZEF0IjoiMjAxNi0xMC0xN1QxMjoxNzowMC42MTYwNDU2NzhaIn0%3D
...

codecov-io · 2016-10-17T13:24:40Z

Current coverage is 51.26% (diff: 94.17%)

Merging #2008 into master will decrease coverage by 9.88%

@@             master      #2008   diff @@
==========================================
  Files           125        126      +1   
  Lines         11136      11240    +104   
  Methods           0          0           
  Messages          0          0           
  Branches          0          0           
==========================================
- Hits           6809       5762   -1047   
- Misses         3443       4730   +1287   
+ Partials        884        748    -136

Powered by Codecov. Last update 8234784...eb1c254

RichardScothern · 2016-10-17T20:10:18Z

Could you rebase to get the build fix please @miminar

stevvooe · 2016-10-17T20:34:03Z

 		host = strings.TrimSpace(hosts[0])
 	}

+	forwardedPort := r.Header.Get("X-Forwarded-Port")


Isn't port a part of the host? How do these work together?

Mostly it is. These headers aren't defined by any standard, so it's hard to give a precise meaning. Nevertheless, when proxy sets the X-Forwarded-Port it is an explicit proxy's port the client sent request to.

When X-Forwarded-Host lacks port, the explicit port shall be taken from X-Forwarded-Port if set. I'm not that sure about conflicting ports in both headers but I take explicit value as decisive.

But, a port in the Host header is explicit, in that it is guaranteed to be set by a single entity, whereas X-Forward-Port may have been set by a secondary party.

stevvooe · 2016-10-17T20:36:20Z

+	forwardedComboHeader.Set("X-Forwarded-Port", " 12345 \t")
+
+	forwardedMixedHeader := make(http.Header, 2)
+	forwardedMixedHeader.Set("X-Forwarded-Host", "first.example.com:5000")


This example concerns me. We clearly have a declared port in the host header but we now trust an unbound port? How do we know the same proxy added both headers?

I don't think there's right or wrong answer to this. I find explicit value as better.

If there are two proxies, one overriding X-Forwarded-Host and the other overriding X-Forwarded-Port with conflicting ports, it is a clear misconfiguration.

@miminar Should we not honor the "complete" host, rather than overriding the port? It seems like port should only apply if the host lacks a port but I am not sure what is expected. From a security perspective, the host must include a port, but I am not sure about how these headers are supposed to work.

stevvooe · 2016-10-17T20:37:53Z

This makes registry behave correctly when proxy strips port from the requests e.g.:

Why are proxies stripping ports from requests?

miminar · 2016-10-18T09:16:37Z

Why are proxies stripping ports from requests?

I cannot speak for all proxies. The default haproxy deployed by OpenShift strips the port from host headers as it is mostly running on default ones. If deployed to run on non-default port, there's still X-Forwarded-Port to let clients do proper redirects.

I don't know if it's the right approach. Keeping the port in host headers for non-default ports would certainly work as well. Nevertheless, honoring this header on application level, won't make the app worse.

stevvooe · 2016-10-18T17:22:03Z

@miminar Ok, so it sounds like we should honor the X-Forwarded-Port header if the X-Forwarded-Host header does not specify a port. Does that sound reasonable?

Hopefully, you can fix the bug in openshift which is stripping the ports.

Also, it looks like the implementation of Forwarded in your examples is not following the rfc7230, in that it is missing a port. We probably need to add support for that header, as well.

RichardScothern · 2016-10-18T20:33:18Z

@miminar are these the docs you refer to? What about the x-forwarded-for header? (there is code in the context package for extracting this)

miminar · 2016-10-19T11:12:40Z

Ok, so it sounds like we should honor the X-Forwarded-Port header if the X-Forwarded-Host header does not specify a port. Does that sound reasonable?

It does, thanks!

Also, it looks like the implementation of Forwarded in your examples is not following the rfc7230, in that it is missing a port. We probably need to add support for that header, as well.

Do you want to follow-up on that or shall I include it in this PR?

are these the docs you refer to? What about the x-forwarded-for header? (there is code in the context package for extracting this)

Yes. I'll include the support for X-Forwarded-For as well. Hopefully tomorrow.

stevvooe · 2016-10-19T16:07:58Z

Do you want to follow-up on that or shall I include it in this PR?

That is up to you. It seems orthogonal.

Thanks for taking care of this!

miminar · 2016-10-22T13:00:35Z

Included support for standard Forwarded header. The non-standard (X-Forwarded-Proto, X-Forwarded-Host, X-Forwarded-Port) still take precedence. The port set by X-Forwarded-Port is now used only if the port is unset by (forwarded) host.

What about the x-forwarded-for header? (there is code in the context package for extracting this)

@RichardScothern I see the X-Forwarded-For header being already handled by the context. My understanding is that it should be used in logging to identify client, which is already the case. Am I missing something?

stevvooe · 2016-10-24T19:12:26Z

 	return appendValuesURL(up, values...).String()
 }
+
+// Following are states of forwarded header parser.


Put this in a separate file.

stevvooe · 2016-10-24T19:13:02Z

+//
+// States marked with '*' are terminating. Any state could transition to a
+// a failure which isn't listed among states.
+const (


Are these constants used outside of the parsing function? May just want to define them local to the function.

stevvooe · 2016-10-24T19:14:24Z

+	}
+}
+
+func newHTTPRequest(t *testing.T, u *url.URL, headers ...string) *http.Request {


Just use a map literal. No need to create another way to specify an http request.

miminar · 2016-10-25T11:38:59Z

Comments addressed.

stevvooe · 2016-10-26T00:52:59Z

LGTM

dmcgowan · 2016-10-26T20:34:00Z

Parser logic looks good, since there is no unit tests directly with the regex, I would at least like to see the IPv6 case tested, with and without a port.

miminar · 2016-10-31T09:52:00Z

@dmcgowan Added support for ipv6 addresses and tests. I haven't tried to forward ipv6 address though.

RichardScothern · 2016-11-01T20:25:11Z

@miminar can you check the test failures please

miminar · 2016-11-02T12:00:11Z

Ouch, the test passes for me with go 1.7. It looks like I need to tune it for go 1.6.

miminar · 2016-11-02T15:32:07Z

It should pass on both go16 and go17 now.

Prefer non-standard headers like X-Forwarded-Proto, X-Forwarded-Host and X-Forwarded-Port over the standard Forwarded header to maintain backwards compatibility. If a port is not specified neither in Host nor in forwarded headers but it is specified just with X-Forwarded-Port, use its value in base urls for redirects. Forwarded header is defined in rfc7239. X-Forwarded-Port is a non-standard header. Here's a description copied from "HTTP Headers and Elastic Load Balancing" of AWS ELB docs: > The X-Forwarded-Port request header helps you identify the port that > an HTTP or HTTPS load balancer uses to connect to the client. Signed-off-by: Michal Minář <miminar@redhat.com>

RichardScothern · 2016-11-02T17:23:18Z

thanks @miminar LGTM

dmcgowan · 2016-11-02T19:55:36Z

LGTM

GordonTheTurtle added the status/0-triage label Oct 17, 2016

miminar mentioned this pull request Oct 17, 2016

Unable to use registry exposed on non-default http port openshift/origin#11337

Closed

miminar changed the title ~~Honor X-Forwarded-Port header~~ [Do not merge] Honor X-Forwarded-Port header Oct 17, 2016

miminar changed the title ~~[Do not merge] Honor X-Forwarded-Port header~~ Honor X-Forwarded-Port header Oct 17, 2016

miminar mentioned this pull request Oct 17, 2016

Honor X-Forwarded-Port header openshift/origin#11391

Merged

stevvooe reviewed Oct 17, 2016

View reviewed changes

miminar force-pushed the honor-x-forwarded-port branch from bd7aa9b to ade8341 Compare October 18, 2016 08:16

miminar force-pushed the honor-x-forwarded-port branch from ade8341 to 8a7f011 Compare October 22, 2016 12:53

miminar force-pushed the honor-x-forwarded-port branch from 8a7f011 to f9767b0 Compare October 22, 2016 13:03

stevvooe reviewed Oct 24, 2016

View reviewed changes

miminar force-pushed the honor-x-forwarded-port branch 2 times, most recently from 0c34ef7 to eb1c254 Compare October 25, 2016 10:20

miminar force-pushed the honor-x-forwarded-port branch from eb1c254 to 3ebbd24 Compare October 31, 2016 09:49

miminar force-pushed the honor-x-forwarded-port branch from 3ebbd24 to c621615 Compare November 2, 2016 15:31

miminar force-pushed the honor-x-forwarded-port branch from c621615 to 1b43e1e Compare November 2, 2016 15:49

dmcgowan merged commit fbe6e8d into distribution:master Nov 2, 2016

miminar mentioned this pull request Nov 8, 2016

Router forwards host urls without ports when serving on non-default ports openshift/origin#11820

Closed

dmcgowan mentioned this pull request Nov 14, 2016

Handle X-Forwarded-Port in url builder #1457

Closed

tt mentioned this pull request Mar 17, 2017

Avoid including standard port #2214

Closed

dmcgowan mentioned this pull request Mar 20, 2017

Fix forwarded logic #2219

Merged

Uh oh!

Conversation

miminar commented Oct 17, 2016

Uh oh!

codecov-io commented Oct 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Current coverage is 51.26% (diff: 94.17%)

Uh oh!

RichardScothern commented Oct 17, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevvooe commented Oct 17, 2016

Uh oh!

miminar commented Oct 18, 2016

Uh oh!

stevvooe commented Oct 18, 2016

Uh oh!

RichardScothern commented Oct 18, 2016

Uh oh!

miminar commented Oct 19, 2016

Uh oh!

stevvooe commented Oct 19, 2016

Uh oh!

miminar commented Oct 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

miminar commented Oct 25, 2016

Uh oh!

stevvooe commented Oct 26, 2016

Uh oh!

dmcgowan commented Oct 26, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

miminar commented Oct 31, 2016

Uh oh!

RichardScothern commented Nov 1, 2016

Uh oh!

miminar commented Nov 2, 2016

Uh oh!

miminar commented Nov 2, 2016

Uh oh!

RichardScothern commented Nov 2, 2016

Uh oh!

dmcgowan commented Nov 2, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov-io commented Oct 17, 2016 •

edited

Loading

dmcgowan commented Oct 26, 2016 •

edited

Loading