Added support for proxy protocol v2. #2912
Conversation
|
Introduced by accident, will fix up later today.
…On Fri, Jan 27, 2023, at 00:33, Benoit Chesneau wrote:
***@***.**** commented on this pull request.
In gunicorn/http/message.py
<#2912 (comment)>:
>
from gunicorn.http.body import ChunkedReader, LengthReader, EOFReader, Body
from gunicorn.http.errors import (
InvalidHeader, InvalidHeaderName, NoMoreData,
InvalidRequestLine, InvalidRequestMethod, InvalidHTTPVersion,
LimitRequestLine, LimitRequestHeaders,
)
-from gunicorn.http.errors import InvalidProxyLine, ForbiddenProxyRequest
-from gunicorn.http.errors import InvalidSchemeHeaders
+from gunicorn.http.errors import InvalidProxyHeader, InvalidProxyLine
why this change?
—
Reply to this email directly, view it on GitHub
<#2912 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAT5CYW2LSNEINFOKYW7VDWUMCV3ANCNFSM6AAAAAATO7BUDE>.
You are receiving this because you authored the thread.Message ID:
***@***.***>
|
|
Very cool to have this feature! |
| """\ | ||
| Detect, check and parse proxy protocol. | ||
|
|
||
| :raises: ForbiddenProxyRequest, InvalidProxyLine. | ||
| :return: True for proxy protocol line else False | ||
| """ | ||
| if not self.cfg.proxy_protocol: | ||
| return False | ||
| return |
There was a problem hiding this comment.
A docstring above still says "... else False".
And why did you decide change False to None?
There was a problem hiding this comment.
It is no longer needed. Previously the code checked the return value based on this and then changed behavior. Now the proxy protocol parsing is fully "transparent" and leaves the streams in a usable state for follow up. Will adjust the comment in a follow up commit.
gunicorn/http/message.py
Outdated
| if ver_cmd & 0xF == 0x00: | ||
| return # LOCAL command, do not change source addr | ||
|
|
||
| if fam == 0x11: # TCPv4 |
There was a problem hiding this comment.
How about to make constants with meaningful names for 0x11 and so on instead comments? Or enum.IntFlag?
There was a problem hiding this comment.
Sounds like a plan, will see what I can come up with.
| s_addr, d_addr, s_port, d_port = struct.unpack(fmt, body) | ||
| except struct.error as e: | ||
| raise InvalidProxyHeader("cannot unpack %r: %s" % (body, e)) | ||
| s_addr = str(ip_class(s_addr)) |
There was a problem hiding this comment.
Why is it needed? - converting str -> ipaddress -> str. Why just not use str representation as is?
If it is needed for validation then where is the error handling?
There was a problem hiding this comment.
s_addr is never really a string but either an 4-byte integer (ipv4 case) or a 16-byte integer (ipv6 case). Those are "narrow" binary representations of the addresses and not directly usable. So this code is turning the address into something human readable.
As for validation errors: How much do we want to trust the trusted proxies to send the correct data? :D
|
Hi @sirkonst and @benoitc -- sorry for the late reply; I have lost track here. I'll update and address the comments today. In the meantime it would be great if you could review #2913, due to the bugs in |
|
I think I have addressed all comments, as far as I can see the linting errors are coming from other files. Let me know what you think. |
This PR adds support for proxy protocol v2 (v1 keeps working as well): https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt
Additionally I did rewrite the
BytesIOusage tobytearray, the code becomes simpler imo because we can just keep a buffer around and resize it as needed.