Use Forwarded, X-Forwarded-Scheme and X-Forwarded-Host for better scheme and host resolution #1881

evertlammerts · 2017-05-11T20:17:46Z

What do these changes do?

They enhance the resolution mechanism for scheme and host in aiohttp.web_request.Baserequest.

The scheme is resolved based on:

the currently used transport
the secure_proxy_ssl_header argument of BaseRequest.__init__
the Forwarded header
the X-Forwarded-Proto header

in that order.

The host is resolved based on:

the Forwarded header
the X-Forwarded-Host header
the Host header

in that order.

Are there changes in behavior for the user?

This PR does not include changes in the contract.

Related issue number

#1134 Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header.

Checklist

I think the code is well written
Unit tests for the changes exist
Documentation reflects the changes
If you provide code modification, please add yourself to CONTRIBUTORS.txt
- The format is <Name> <Surname>.
- Please keep alphabetical order, the file is sorted by names.
Add a new entry to CHANGES.rst
- Choose any open position to avoid merge conflicts with other PRs.
- Add a link to the issue you are fixing (if any) using #issue_number format at the end of changelog message. Use Pull Request number if there are no issues for PR or PR covers the issue only partially.

AraHaan · 2017-05-14T01:23:35Z

CHANGES.rst

@@ -41,6 +41,8 @@ Changes

 - Do not unquote `+` in match_info values #1816

+- Use Forwarded, X-Forwarded-Scheme and X-Forwarded-Host for better scheme and host resolution. #1134


PEP8 here. Please split this to 2 lines or more of up to 79 characters per line. For more information on PEP8 see the www.python.org page on PEP8.

asvetlov · 2017-05-14T13:22:53Z

Implementation looks good on fist glaze.
Let me reread RFC before merging.
I'll add required deprecation records after merging if @evertlammerts has troubles with it.

evertlammerts · 2017-05-14T18:55:36Z

does c2848ed cover the deprecation of secure_proxy_ssl_header?

AraHaan

Seems alright for now.

evertlammerts · 2017-05-19T21:47:45Z

Ping @asvetlov

asvetlov · 2017-05-19T23:37:45Z

Sorry, I'm on US PyCon right now and quite busy.

asvetlov · 2017-05-20T05:45:25Z

https://tools.ietf.org/html/rfc7239 says that Forwarded header could be present several times but the very first occurrence points to host which initiates the request.
@evertlammerts could you add an explicit test for it?

asvetlov · 2017-05-20T05:53:29Z

aiohttp/web_request.py

+        forwarded = self._message.headers.get(hdrs.FORWARDED)
+        if forwarded is not None:
+            parsed = re.findall(
+                r'^by=([^;]*); +for=([^;]*); +host=([^;]*); +proto=(https?)$',


I suspect the parser should be more complicated.

The header field value can be defined in ABNF syntax as: Forwarded = 1#forwarded-element forwarded-element = [ forwarded-pair ] *( ";" [ forwarded-pair ] ) forwarded-pair = token "=" value value = token / quoted-string token = <Defined in [RFC7230], Section 3.2.6> quoted-string = <Defined in [RFC7230], Section 3.2.6>

It means the forwarded-pair elements could be in random order: by=a; host=b.com and host=b.com, by=a are both valid combinations.
Also value could be optionally quoted, aiohttp shoud unquote it automatically.
pair-name is case-insensitive: all 'Host', 'host', and 'HOst` are valid names:

Forwarded: for="_gazonk" Forwarded: For="[2001:db8:cafe::17]:4711" Forwarded: for=192.0.2.60;proto=http;by=203.0.113.43 Forwarded: for=192.0.2.43, for=198.51.100.17 Note that as ":" and "[]" are not valid characters in "token", IPv6 addresses are written as "quoted-string".

Please note: the lase example contains two for pairs, it's not supported by your regexp now.

evertlammerts · 2017-05-20T11:58:13Z

😊 I should have been way more thorough. Sorry about this, I will go fix. Thanks for looking into it!

…

On Sat, May 20, 2017 at 7:59 AM, Andrew Svetlov ***@***.***> wrote: ***@***.**** requested changes on this pull request. ------------------------------ In aiohttp/web_request.py <#1881 (comment)>: > @@ -151,15 +151,30 @@ def secure(self): return self.url.scheme == 'https' @reify + def _forwarded(self): + forwarded = self._message.headers.get(hdrs.FORWARDED) + if forwarded is not None: + parsed = re.findall( + r'^by=([^;]*); +for=([^;]*); +host=([^;]*); +proto=(https?)$', I suspect the parser should be more complicated. The header field value can be defined in ABNF syntax as: Forwarded = 1#forwarded-element forwarded-element = [ forwarded-pair ] *( ";" [ forwarded-pair ] ) forwarded-pair = token "=" value value = token / quoted-string token = <Defined in [RFC7230], Section 3.2.6> quoted-string = <Defined in [RFC7230], Section 3.2.6> It means the forwarded-pair elements could be in random order: by=a; host= b.com and host=b.com, by=a are both valid combinations. Also value could be optionally quoted, aiohttp shoud unquote it automatically. pair-name is case-insensitive: all 'Host', 'host', and 'HOst` are valid names: Forwarded: for="_gazonk" Forwarded: For="[2001:db8:cafe::17]:4711" Forwarded: for=192.0.2.60;proto=http;by=203.0.113.43 Forwarded: for=192.0.2.43, for=198.51.100.17 Note that as ":" and "[]" are not valid characters in "token", IPv6 addresses are written as "quoted-string". ------------------------------ In aiohttp/web_request.py <#1881 (comment)>: > @@ -151,15 +151,30 @@ def secure(self): return self.url.scheme == 'https' @reify + def _forwarded(self): + forwarded = self._message.headers.get(hdrs.FORWARDED) + if forwarded is not None: + parsed = re.findall( + r'^by=([^;]*); +for=([^;]*); +host=([^;]*); +proto=(https?)$', Please note: the lase example contains two for pairs, it's not supported by your regexp now. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1881 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACjx1nCRcX1AMTWrK0Ev9NY-YbwoSrxfks5r7oFTgaJpZM4NYgE5> .

evertlammerts · 2017-05-20T21:55:08Z

Alright, this actually adds a public forwarded property to BaseRequest. It makes an effort to parse Forwarded headers as specified by the RFC:

It adds all parameters (by, for, host, proto) in the order it finds them; starting at the topmost / first added 'Forwarded' header, at the leftmost / first-added parameter.
It checks that the value has valid syntax in general as specified in section 4: either a 'token' or a 'quoted-string'.
It un-escapes found escape sequences.
It does NOT validate 'by' and 'for' contents as specified in section 6.
It does NOT validate 'host' contents (Host ABNF).
It does NOT validate 'proto' contents for valid URI scheme names.

Don't you think the place for parsing the Forwarded header is rather a middleware? I'm in doubt myself now.

Otherwise, what's a good place for the regex constants? Would be nice to compile the regex only once, but module level doesn't do much for esthetics.

asvetlov

I love .req.forwarded property.

No, we don't need a middleware for it.

Global variables for regexps are pretty fine. If you don't like this approach you could put them ad class variables into BaseRequest

asvetlov · 2017-05-21T06:50:15Z

aiohttp/web_request.py

+        if hdrs.FORWARDED in self._message.headers:
+            for forwarded_elm in self._message.headers.getall(hdrs.FORWARDED):
+                if len(forwarded_elm):
+                    forwarded_pairs = tuple(


Don't create a tuple but make a generator comprehension. It saves both time and memory.

Please support Forwarded: by=first, by=second form, it's equal to

Forwarded: by=first Forwarded: by=second

This is supported by the regexes and I've included tests for it

asvetlov · 2017-05-21T06:53:34Z

aiohttp/web_request.py

+        params = {'by': [], 'for': [], 'host': [], 'proto': []}
+        if hdrs.FORWARDED in self._message.headers:
+            for forwarded_elm in self._message.headers.getall(hdrs.FORWARDED):
+                if len(forwarded_elm):


Drop this check, iteration over empty forwarded_elm is pretty fine.

asvetlov · 2017-05-21T06:56:33Z

aiohttp/web_request.py

+        proto=tuple(...), ) 
+        """
+        params = {'by': [], 'for': [], 'host': [], 'proto': []}
+        if hdrs.FORWARDED in self._message.headers:


Drop the check, in is not free.
Use for forwarded_elm in self._message.headers.getall(hdrs.FORWARDED, []) instead

asvetlov · 2017-05-21T07:02:13Z

aiohttp/web_request.py

+    r'[bB][yY]|[fF][oO][rR]|[hH][oO][sS][tT]|[pP][rR][oO][tT][oO]')
+
+_FORWARDED_PAIR = (
+    r'^ *({forwarded_params})=({token}|{quoted_string}) *$'.format(


IIRC spaces around = are allowed.

I'm not sure but as far as I can see that's not the case. Section 4 only says forwarded-pair = token "=" value and section 7 recalls Note that an HTTP list allows white spaces to occur between the identifiers, and the list may be split over multiple header fields.

IMHO it's rare case but still allowed

asvetlov · 2017-05-21T07:03:44Z

aiohttp/web_request.py

+                            continue
+                        param = forwarded_pair[0][0].lower()
+                        value = forwarded_pair[0][1]
+                        if len(value) and value[0] == '"':


Add a check for value[-1] == '"'

If empty value allowed? I mean len(value) == 0.
Also if value: gives the same result as if len(value) but works a bit faster.

There's no need to check for a closing quote: the regex validates quoted-string so any value starting with " must also have a closing ".

A token may be empty (but note again that I don't check the syntax in section 6)

asvetlov · 2017-05-21T07:13:00Z

aiohttp/web_request.py

+        Returns a dict(by=tuple(...), for=tuple(...), host=tuple(...),
+        proto=tuple(...), ) 
+        """
+        params = {'by': [], 'for': [], 'host': [], 'proto': []}


I believe the property's type should be MultyDict instead of dict of lists.
It preserves all information about proxies in right order but original host still could be checked as req.forwarded.get('host')

asvetlov · 2017-05-21T07:13:45Z

aiohttp/web_request.py

+                            value = _QUOTED_PAIR_REPLACE_RE.sub(
+                                r'\1', value[1:-1])
+                        params[param].append(value)
+        return params


return MultiDictProxy(params) -- it's an immutable view.

asvetlov · 2017-05-21T07:25:55Z

aiohttp/web_request.py

+        Returns a dict(by=tuple(...), for=tuple(...), host=tuple(...),
+        proto=tuple(...), )
+        """
+        params = {'by': [], 'for': [], 'host': [], 'proto': []}


Use MultiDict here instead of dict of pairs. It preserves all information about proxies.

asvetlov · 2017-05-21T07:27:30Z

aiohttp/web_request.py

+        """
+        params = {'by': [], 'for': [], 'host': [], 'proto': []}
+        if hdrs.FORWARDED in self._message.headers:
+            for forwarded_elm in self._message.headers.getall(hdrs.FORWARDED):


Drop previous line, in is not free but getall(hdrs.FORWARDED, ()) does the check using single call to data structure.

* Fix #1839: python 3.7 regression for regexp escaping * Fix import sort order

evertlammerts · 2017-05-21T13:23:08Z

I forgot Python < 3.5 doesn't like an arbitrary number of unpackings (*iterable). Just to be certain, is Python >= 3.4.2 still the requirement?

asvetlov · 2017-05-21T16:33:05Z

Yes, we sill support Python 3.4.2+
Will drop it soon but not right now.
It will be trivial but huge change -- replace all yield from with await :)

evertlammerts · 2017-05-21T20:08:39Z

I seem to have missed a couple of things:

field values are separated by comma: my implementation rather assumes that each header has a single field value in which each parameter may occur multiple times (i.e. I allow host=abc; host=def; which is explicitly not allowed and should be host=abc, host=def)
MultiDict maintains order between values, so I can just use md.add()

Regarding OWS around =, I can't find examples where that is allowed. Actually, I think it isn't allowed around ; either, or at least the ABNF doesn't include OWS or RWS. (Another bug in my regexes.) The only place where it seems to be allowed is the field-value separator (,).

I looked for examples of whitespace around = in other places as well. I can only think of the Content-* (charset=...) and Accept(q=...) headers as ones that also accept key-value pairs and RFC 7231 explicitly doesn't allow whitespace. Why do you think it is allowed? Maybe something to do with clients not being compliant?

I'll do my best to add some more commits later this week. When are you planning the next release, @asvetlov?

asvetlov · 2017-05-21T21:56:01Z

Ok, I'm convinced regarding to spaces.
But commas and proper multidict usage (md.add(k, v)) should be fixed.
There is no scheduled date for the release.
I love to have this issue in next version and could wait for you for a while if @fafhrd91 agrees with me.

…r instead of tchar), changed return type of forwarded

fafhrd91 · 2017-05-22T15:08:07Z

@asvetlov it is your call. I am busy with different project in any case

evertlammerts · 2017-05-22T15:22:23Z

d1ba86b fixes the last issues, as far as I can see.

It is now possible to provide multiple field-values (comma separated list) in a single Forwarded header. See the tests / code in d1ba86b for the supported semantics.
I've fixed the unpacking issue. The patch should be compatible with Python 3.4 now.
I've fixed an issue with the quoted-pairs regex, for quoted-strings (ie host="something \"escaped\"" now works correctly)

As far as the return type goes, I've changed that to a tuple containing MappingProxies. The semantics of the Forwarded header are so that every proxy may add a Forwarding field-value; by, for, host and proto in any combination, but not multiple times per field-value. When a proxy adds a new Forwarding field-value, it either appends it to an already existing Forwarding header (after a comma) or it adds a new Forwarding header at the bottom. I think it's useful to maintain the information about the order in which proxies added something. So now the result will look something like:

req.forward = (
    MappingProxy({'by': ..., 'for': ...}),  # added by proxy 1
    MappingProxy({'host': '...', 'for': ..., 'proto': ...}),  # added by proxy 2
    # ... etc
)

If a field-value added by a proxy cannot be parsed, the code will add a MappingProxy(dict()) so that users can still see that something is there, and that it was added by a proxy at that point in the chain.

Let me know what you think, @asvetlov.

asvetlov · 2017-05-22T17:32:24Z

Awesome work @evertlammerts
Very thank you!

adamcharnock · 2017-12-17T14:22:40Z

For anyone else chasing their tail trying to figure out where this feature has gone, it has been removed and moved into middleware. See here:

#2171 (comment)

lock · 2019-10-28T11:04:31Z

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a [new issue] for related bugs.
If you feel like there's important points made in this discussion, please include those exceprts into that [new issue].
[new issue]: https://github.com/aio-libs/aiohttp/issues/new

Evert Lammerts and others added 5 commits May 10, 2017 21:25

#1134 Support X-Forwarded-* and Forwarded implicitly

b1043b4

#1134 tests

85fcdfc

added myself to contributors

be95cb4

added line to changes.rst

9554ce0

Merge branch 'master' into master

3799f85

AraHaan suggested changes May 14, 2017

View reviewed changes

evertlammerts added 2 commits May 14, 2017 08:05

Update CHANGES.rst

76b1624

Update CHANGES.rst

1f79171

asvetlov self-assigned this May 14, 2017

deprecating secure_proxy_ssl_header

c2848ed

AraHaan approved these changes May 15, 2017

View reviewed changes

asvetlov requested changes May 20, 2017

View reviewed changes

Evert Lammerts added 2 commits May 20, 2017 22:52

Merge https://github.com/aio-libs/aiohttp

6d37b83

more extensive Forwarded parsing

73d4121

flake

0cb4b22

asvetlov requested changes May 21, 2017

View reviewed changes

asvetlov and others added 4 commits May 21, 2017 00:50

make isort with new parameters

eee65a4

Fix #1839: python 3.7 regression for regexp escaping (#1908)

e5c7bfe

* Fix #1839: python 3.7 regression for regexp escaping * Fix import sort order

small changes

2b33c7d

unpacking tuple implicit

15462c2

Evert Lammerts added 11 commits May 22, 2017 16:53

#1134 Support X-Forwarded-* and Forwarded implicitly

b672b16

#1134 tests

105c9ed

added myself to contributors

145f31f

added line to changes.rst

b2a0cc5

deprecating secure_proxy_ssl_header

09cd914

more extensive Forwarded parsing

df470cd

flake

5560bab

small changes

ef0c597

unpacking tuple implicit

cd643a7

handling multiple field-values, fixed quoted-pair bug (now using vcha…

d1ba86b

…r instead of tchar), changed return type of forwarded

merge conflict

8ede7d2

asvetlov approved these changes May 22, 2017

View reviewed changes

asvetlov merged commit 5a43ce0 into aio-libs:master May 22, 2017

This was referenced May 22, 2017

Docs for forwarded attribute #1911

Merged

Support X-Forwarded-* and Forwarded implicitly, deprecate secure_proxy_ssl_header. #1134

Closed

lock bot added the outdated label Oct 28, 2019

lock bot locked as resolved and limited conversation to collaborators Oct 28, 2019

		@@ -41,6 +41,8 @@ Changes

		- Do not unquote `+` in match_info values #1816

		- Use Forwarded, X-Forwarded-Scheme and X-Forwarded-Host for better scheme and host resolution. #1134

Use Forwarded, X-Forwarded-Scheme and X-Forwarded-Host for better scheme and host resolution #1881

Use Forwarded, X-Forwarded-Scheme and X-Forwarded-Host for better scheme and host resolution #1881

Conversation

evertlammerts commented May 11, 2017

What do these changes do?

Are there changes in behavior for the user?

Related issue number

Checklist

Choose a reason for hiding this comment

asvetlov commented May 14, 2017

evertlammerts commented May 14, 2017

AraHaan left a comment

Choose a reason for hiding this comment

evertlammerts commented May 19, 2017

asvetlov commented May 19, 2017

asvetlov commented May 20, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evertlammerts commented May 20, 2017 via email

evertlammerts commented May 20, 2017

asvetlov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

evertlammerts commented May 21, 2017 • edited Loading

asvetlov commented May 21, 2017

evertlammerts commented May 21, 2017

asvetlov commented May 21, 2017

fafhrd91 commented May 22, 2017

evertlammerts commented May 22, 2017

asvetlov commented May 22, 2017

adamcharnock commented Dec 17, 2017

lock bot commented Oct 28, 2019

asvetlov commented May 20, 2017 •

edited

Loading

evertlammerts commented May 21, 2017 •

edited

Loading