Proxy: proxy protocol v2

[conradludgate]

Problem

We need to log the client IP, not the IP of the NLB.

Summary of changes

Parse the proxy protocol version 2 if possible

Checklist before requesting a review

• I have performed a self-review of my code.
• If it is a core feature, I have added thorough tests.
• Do we need to implement analytics? if so did you add the relevant metrics to the dashboard?
• If this PR requires public announcement, mark it with /release-notes label and add several sentences in this section.

Checklist before merging

• Do not forget to reformat commit message to not include the above checklist

[github-actions]

1624 tests run: 1551 passed, 0 failed, 73 skipped (full report)

---

_{The comment gets automatically updated with the latest test results
f4b5622 at 2023-08-31T11:16:17.856Z :recycle:}

[petuhovskiy]

Reading through the specification:

The receiver MUST be configured to only receive the protocol described in this
specification and MUST not try to guess whether the protocol header is present
or not. This means that the protocol explicitly prevents port sharing between
public and private access. Otherwise it would open a major security breach by
allowing untrusted parties to spoof their connection addresses. The receiver
SHOULD ensure proper access filtering so that only trusted proxies are allowed
to use this protocol.

It seems that deploy will be painful...

[conradludgate]

Yeah I think that is unfortunate. I didn't implement that part of the specification and it will in fact guess whether the header is there

[petuhovskiy]

1. We need to add config option to enable/disable v2 protocol. Will be useful when we need to deploy proxy in places without v2 support, such as local.
2. Found several crates with v2 implementation on the internet, for example. What do you think, shouldn't we try to use existing lib?
3. Haven't deeply looked at proxy/src/protocol2.rs yet, but IMO it requires one more pair of eyes. It is complicated and works with public traffic, so it is quite sensitive.
4. I'm not sure if guessing v2 header presence is a good idea, we need to consider possible attack vectors.
5. Someone needs to look at deploy scripts, we can't just merge this without changing helm charts / configuring NLB.

[conradludgate]

1. We need to add config option to enable/disable v2 protocol. Will be useful when we need to deploy proxy in places without v2 support, such as local.

I think I will change to this model eventually, but as you pointed out it makes migration extremely difficult. The only alternative is probably switching to a new NLB target with a new service/deployment but I don't think I want to go through that effort if it's unnecessary

2. Found several crates with v2 implementation on the internet, for [example](https://github.com/Proximyst/proxy-protocol). What do you think, shouldn't we try to use existing lib?

That crate was a direct inspiration for my implementation. I can't really understand how I should use it though. It returns an error if we haven't read enough bytes which is quite complicated to deal with correctly from the looks of it. There's no way to know how many bytes to read unless we are the ones parsing the length field of the header.

4. I'm not sure if guessing v2 header presence is a good idea, we need to consider possible attack vectors.

Guessing is quite straightforward: Does the magic 12 bytes exist in the first payload.

0x0D, 0x0A, 0x0D, 0x0A, 0x00, 0x0D, 0x0A, 0x51, 0x55, 0x49, 0x54, 0x0A

For Postgres protocol, no valid postgres message starts with 0x0D (all message tags are currently valid ascii) so this is unambiguous.
For Websockets and HTTP over HTTPS, that first TLS ClientHello message should start with 0x03XX https://www.rfc-editor.org/rfc/rfc8446#page-28 https://www.rfc-editor.org/rfc/rfc5246#appendix-E so this is unambiguous

We don't plan on using the information for anything security-related yet, only for some nice extra info in error logs. When we switch to a non-guessing implementation we can consider using the IP addresses as security monitoring info.

5. Someone needs to look at deploy scripts, we can't just merge this without changing helm charts / configuring NLB.

If we merge as is, we should be able to update the NLB after the fact

[petuhovskiy]

Ok, so it looks like hard-switch is not convenient and guessing is the only feasible option. I'll review today.

[petuhovskiy]

It's hard to test it with the real NLB, so would be nice to deploy it to staging sooner.

[conradludgate]

It's hard to test it with the real NLB, so would be nice to deploy it to staging sooner.

Yeah. I did test it locally with HAProxy using protocol v2 and it did work