Sanic: IPV6 address wrapped in brackets

[ahopkins]

@Tronic why are we wrapping IPV6 addresses with square brackets here?

class ConnInfo:
    ...
    def __init__(self, transport, unix=None):
        ...
        # IPv4 (ip, port) or IPv6 (ip, port, flowinfo, scopeid)
        ...
        if isinstance(addr, tuple):
            self.client = addr[0] if len(addr) == 2 else f"[{addr[0]}]"
            self.client_port = addr[1]

source

[Tronic]

Because in URLs and in Host and Forwarded headers they are wrapped.

[sjsadowski]

Also it's required as part of RFC 3986 when used in conjunction with other parts such as user:pass and :port as both use colon designators and could look like hex refs.

[ahopkins]

Well, then someone ought to tell Postgres.

update some_table set ip_addr='[::ffff:172.20.0.1]';                                                                                           
invalid input syntax for type inet: "[::ffff:172.20.0.1]"
LINE 1: update some_table set ip_addr='[::ffff:172.20...
                                                     ^

I think maybe we should make it somewhat easier to get at the raw value than: request.conn_info.sockname[0].

[sjsadowski]

The brackets are only valid as part of the address when there's a prefix or suffix as part of a URI... IE stephen:password@[::ff] or [::ff]:8080

The address itself should never contain the brackets. Think of it like this, using an f-stringINET_ADDR

f'https://{prefix}[{ipv6_addr}]:{port}'

Technically postgres is correct because you shouldn't be storing the the entire uri as an inet type, you'd do it in a char type field., same as you wouldn't store 172.20.0.1:443 in an inet column, as postgres would reject that.

Hope this helps.

[ahopkins]

Right, that makes sense. but, given that Sanic is setting these properties, it looks like Sanic is wrong, and to get the actual value from the tuple yourself is not intuitive. Either, we should add another top level property to request object, or only do the wrapping when needed. I'm particularly not even sure that Sanic should do it at all except when building an address maybe. Grabbing request.ip for DB insertion seems common enough pattern that it shouldn't appear as a gotcha.

[Tronic]

The fields of concern here are at least request.ip and request.remote_addr.

Others such as request.host and request.forwarded.for/by/host must stay bracketed because port or other identifiers may be included in them that would be ambiguous if IPv6 were not bracketed, and changing of which would also break URL formatting. request.server_name probably should stay bracketed but it can be argued that it shouldn't be. The ConnInfo structure and its uses are also a concern.

If fixed by a new top level request accessor, I suggest adding one that gives the user address, meaning the client IP either locally or via a proxy, giving either IPv4 or IPv6 without brackets, or None if a configured proxy doesn't report any remote address or if a unix socket is used without proxies. Currently there is no direct accessor for getting the user address, so this wouldn't be a breaking change, and would have additional use in avoiding hacks like request.remote_addr or request.ip to get the IP that you actually want in your server logs and database.

If implemented as described above, request.client_ip would probably be the most obvious name, as it wouldn't report unix socket names or internal IP addresses.

[ahopkins]

Yes, that is exactly what I was thinking and that strategy is what lead me to this realization this week.

[sjsadowski]

I may take a crack at this one and use a feature flag to do it, so that any code that relies on it currently isn't broken, but then we can change the default for the 21.03 quarterly release and deprecate the feature after the 21.12 LTS. Seems the most sane way to do it.

[ahopkins]

That sounds like a good plan to me. Not sure how we can, but it would be nice to add a warning. But since I don't think there's really anything callable, we'd have to probably just do it in release notes, and maybe a warning at startup.

[Tronic]

@sjsadowski Adding a new accessor won't break any existing code, assuming you are taking the path discussed a few messages above.

[stale[bot]]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. If this is incorrect, please respond with an update. Thank you for your contributions.

[ahopkins]

@sjsadowski Are you working on this?

[sjsadowski]

Picking this back up.