Add PROXY protocol support

[blacktear23]

Add PROXY protocol V1 and V2 support.

usage: tidb-server --proxy-protocol-networks "*" --proxy-protocol-header-timeout 5

Add --proxy-protocol-networks command parameter for PROXY protocol enable or disable. If you want to limit HAProxy server IP range, you can set --proxy-protocol-networks parameter to a CIDRs and split by ",". For example:

tidb-server --proxy-protocol-networks "192.168.1.0/24,192.168.2.0/24"

For more information about PROXY protocol please refer https://www.haproxy.org/download/1.8/doc/proxy-protocol.txt

[CLAassistant]

All committers have signed the CLA.

[ngaut]

Thank you for your contribution. We don't have any plans for proxy protocol currently. Let's discuss it after the 1.0GA version.

[tiancaiamao]

Could you revert unrelated changes and make this PR reviewable? Maybe merge master would make the diff information clean? @blacktear23

[blacktear23]

I think I should open another PR.

[breezewish]

At the code level it looks good to me. I will setup HAProxy to have a try with this PR when I'm free.

[blacktear23]

@breeswish here's a HAproxy configuration for test, hope it will help you to do the test.

global
    log /dev/log local0
    maxconn 65536
    ulimit-n 131086
    quiet
    pidfile /var/run/haproxy.pid

defaults
    option httplog
    option redispatch
    option nolinger
    option dontlognull
    retries 3
    contimeout 5000
    clitimeout 50000
    srvtimeout 50000
    log 127.0.0.1 local3

listen web
    bind :3307
    mode tcp
    balance roundrobin
    server tidb1 127.0.0.1:4000 send-proxy weight 1

[breezewish]

LGTM

[breezewish]

@tiancaiamao Is it better to move "parse PROXY header" into newConn so that:

• Always parse proxy first
• Able to know real client address in newConn

[blacktear23]

@breeswish before I submit this PR I really do the test as you mentioned, but it will not work.

Below is how mysql client communicate with mysql server:

1. Client start TCP connection
2. Server send handshake "hello" message
3. Client check handshake "hello" message and send response ...

That mean Server should send some data before receive any data. Let's introduce HAProxy, HAProxy will not send any PROXY protocol header before client send any data. So if we move parse protocol proxy header code on newConn function before we call handshake function, we may run into dead-lock:

• Server Wait Client To Send Data
• Client Wait Server To Send Handshake Hello

So, that's why I move parse proxy protocol header code at first line of readHandshakeResponse function.

[breezewish]

@blacktear23

I've just tested with HAProxy and HAProxy sent PROXY header first even if the server did not send anything (and was waiting for PROXY header). The dead lock only happens when we directly use mysql-client to connect to the server. I think this kind of dead lock is acceptable since it should not happen with correct configurations.

In addition, it seems that the specification stipulates that the server need to parse header first before anything else:

The receiver MUST NOT start processing the connection before it receives a complete and valid PROXY protocol header. This is particularly important for protocols where the receiver is expected to speak first....

I have modified your PR to prevent server from sending anything before receiving PROXY protocol header. However timeout is not implemented. Since it is not very important, I could expect it to be implemented in the future.

[blacktear23]

@breeswish I test your post it works. Maybe I make a mistake.

[breezewish]

Although current changes lose the feature "close connection as soon as possible with an incorrect protocol", it sticks to the specification better. According to my test with HAProxy (1.7.8 2017/07/07) @ MacOS, both solutions work as expected. I would like to use the current implementation if no HAProxy compatibility issues are found.

@blacktear23 Nice work! LGTM Would you like to open PRs to update our docs and docs-cn about the newly added command line option?

@tiancaiamao PTAL.

[blacktear23]

@breeswish I think we can add a read timeout on read proxy protocol header. And we should add a timeout command line parameter for read timeout. For default value as I think 5 ~ 10 seconds should be better.

[breezewish]

@blacktear23 I think so.

[ngaut]

Have you tested the proxy protocol handler with go-fuzz? You may need to test it in case of some safe scanner sending random bytes which may crash the protocol handler.

[blacktear23]

@ngaut shall I add xxx_fuzz.go in code tree? I don't see any go-fuzz related code in current code tree.

[ngaut]

Sure.

[choleraehyq]

@blacktear23 I've used go-fuzz to test our parser, but I haven't push them into our code tree.

[blacktear23]

@choleraehyq can you give me some reference? I want to check if my code is correct.

[choleraehyq]

@blacktear23 You can see the examples in go-fuzz, it's quite easy.

[blacktear23]

@choleraehyq one thing is not clear is corpus. Is that mean I should generate (or copy from examlpes) by my self? Or can I understand it as a test case?

[choleraehyq]

@blacktear23 http://tonybai.com/2015/12/08/go-fuzz-intro/

[blacktear23]

@choleraehyq how long does go-fuzz running is means good?

2017/08/09 15:14:04 slaves: 4, corpus: 12 (54m16s ago), crashers: 0, restarts: 1/9993, execs: 81888815 (25135/sec), cover: 31, uptime: 54m18s
2017/08/09 15:14:07 slaves: 4, corpus: 12 (54m19s ago), crashers: 0, restarts: 1/9992, execs: 81971889 (25137/sec), cover: 31, uptime: 54m21s
2017/08/09 15:14:10 slaves: 4, corpus: 12 (54m22s ago), crashers: 0, restarts: 1/9992, execs: 82051684 (25138/sec), cover: 31, uptime: 54m24s
2017/08/09 15:14:13 slaves: 4, corpus: 12 (54m25s ago), crashers: 0, restarts: 1/9993, execs: 82136451 (25141/sec), cover: 31, uptime: 54m27s
2017/08/09 15:14:16 slaves: 4, corpus: 12 (54m28s ago), crashers: 0, restarts: 1/9993, execs: 82220426 (25144/sec), cover: 31, uptime: 54m30s
2017/08/09 15:14:19 slaves: 4, corpus: 12 (54m31s ago), crashers: 0, restarts: 1/9994, execs: 82302406 (25146/sec), cover: 31, uptime: 54m33s
2017/08/09 15:14:22 slaves: 4, corpus: 12 (54m34s ago), crashers: 0, restarts: 1/9993, execs: 82387280 (25148/sec), cover: 31, uptime: 54m36s
2017/08/09 15:14:25 slaves: 4, corpus: 12 (54m37s ago), crashers: 0, restarts: 1/9994, execs: 82473144 (25152/sec), cover: 31, uptime: 54m39s
2017/08/09 15:14:28 slaves: 4, corpus: 12 (54m40s ago), crashers: 0, restarts: 1/9994, execs: 82556693 (25154/sec), cover: 31, uptime: 54m42s
2017/08/09 15:14:31 slaves: 4, corpus: 12 (54m43s ago), crashers: 0, restarts: 1/9993, execs: 82639290 (25156/sec), cover: 31, uptime: 54m45s
2017/08/09 15:14:34 slaves: 4, corpus: 12 (54m46s ago), crashers: 0, restarts: 1/9994, execs: 82722851 (25159/sec), cover: 31, uptime: 54m48s
2017/08/09 15:14:37 slaves: 4, corpus: 12 (54m49s ago), crashers: 0, restarts: 1/9994, execs: 82802565 (25160/sec), cover: 31, uptime: 54m51s
2017/08/09 15:14:40 slaves: 4, corpus: 12 (54m52s ago), crashers: 0, restarts: 1/9993, execs: 82886556 (25163/sec), cover: 31, uptime: 54m54s
2017/08/09 15:14:43 slaves: 4, corpus: 12 (54m55s ago), crashers: 0, restarts: 1/9994, execs: 82970650 (25165/sec), cover: 31, uptime: 54m57s
2017/08/09 15:14:46 slaves: 4, corpus: 12 (54m58s ago), crashers: 0, restarts: 1/9993, execs: 83047610 (25166/sec), cover: 31, uptime: 55m0s
^C2017/08/09 15:14:47 shutting down...

command line is:

go-fuzz -bin=server-fuzz.zip -workdir=workdir

[choleraehyq]

Crasher = 0 means no panic. As I know, go-fuzz will mutate its input and try again and again, there is no running time limits or something.

[blacktear23]

@choleraehyq no crash running about 1 hours means acceptable?

[tiancaiamao]

LGTM @breeswish @choleraehyq

[choleraehyq]

I think fuzz code should not be merged in our code tree, because fuzz test can not be integrated in CI. Other projects tested by go-fuzz don't merge fuzz code either, like go compiler and std itself.

[blacktear23]

@choleraehyq shall I delete fuzz.go from source tree?

[choleraehyq]

Yeah, just fuzz test at your local machine.

[breezewish]

Is it really necessary to implement our own buffered IO? Since it is not very obvious to find out that the code is right. Shall we just read 108 bytes once? The protocol itself permits such implementation.

Spec:

Both formats are designed to fit in the smallest TCP segment that any TCP/IP host is required to support (576 - 40 = 536 bytes). This ensures that the whole header will always be delivered at once when the socket buffers are still empty at the beginning of a connection. The sender must always ensure that the header is sent at once, so that the transport layer maintains atomicity along the path to the receiver. The receiver may be tolerant to partial headers or may simply drop the connection when receiving a partial header.

[blacktear23]

@breeswish It's not a buffer IO. The buffer just handle it in first read and after first read it will just proxy net.Conn.Read directly. And in my opinion this should be a little bit faster than wrap a buffered IO. Due to mysql's client logic (wait server handshake before send bytes to server) we can assume that in most scenario the first packet that tidb server received is PROXY protocol header that HAProxy sent, and buffer shall be nil. So the code for buffer process almost not run. And for PROXY protocol spec, the header decoder is focus on first packet. If first packet (first Read call) is an invalid header format it will return an error and drop this connection.

[breezewish]

@blacktear23 Thanks for your explanation. but I missed some of your dialogues previously so that still I cannot understand the purpose of the function proxyProtocolConn.Read(). Could you briefly introduce it to me? Mainly, why do we need this function (& wrap connection). If we don't do this, what issue will we meet. Thanks a lot!

[blacktear23]

@breeswish My first implements is Read 1 byte one by one until got "\r\n". And this implements is the easy way to make sure header decoder not read more byte after "\r\n". But the problem is this method's performance is not good (as you know a Read call may cause a goroutine's context switch). So I should find a way to reduce Read call times. The first idea about reduce Read call is create proxyProtocolConn to wrap net.Conn, so we can safely read first packet bytes by one Read call, and parse PROXY protocol header bytes. As we know the PROXY protocol v1 header length is variable, so we cannot read exactly bytes, and the first Read may contain some bytes after header, so I introduce a buffer to store the bytes after header's "\r\n". Then I should proxy Conn.Read and make sure next Read call can read the rest bytes after PROXY protocol header. And I also use a flag to track the buffer is already read. So after the buffer is fully read, the Read function will just call conn.Read directly.

[breezewish]

@blacktear23 Thanks a lot! It makes sense to me now. But I would like to know whether there are cases that we may read extra data? Since in MySQL protocol the client should be waiting server's hello first.

[blacktear23]

@breeswish mysql-client is work as I mentioned, but other client library is not tested.

[breezewish]

@blacktear23 Ok, let's keep this since it may be useful in the future.

[breezewish]

Does wrapping conn compatible with this PR?

https://github.com/pingcap/tidb/pull/3716/files#diff-dc417f5322100e2e83a8818c70a25878R863

[blacktear23]

@breeswish As I read the code you mentioned, proxyProtocolConn will not break those commit. You can treat proxyProtocolConn as net.Conn.

[breezewish]

I tested with your code using the following script. The login request encoded in the script is username=root (no password). I found out that in fact your code does not work with clients that send LOGIN_REQUEST before receiving SERVER_GREETING:

import socket

def hexOutput(bytes):
    hex = ['{:02x}'.format(x) for x in bytes]
    for i in range(0, len(hex), 16):
        print(' '.join(hex[i:i+16]))
    print('')

def checkResponseOk(bytes):
    packet_len = int.from_bytes(bytes[0:3], byteorder='little')
    packet_body = bytes[4:]
    is_ok = packet_body[0] == 0x00
    if not is_ok:
        assert packet_body[0] == 0xff
        err_code = int.from_bytes(packet_body[1:3], byteorder='little')
        err_body = packet_body[10:].decode()
        print("ERROR {0}: {1}".format(err_code, err_body))
    else:
        print("OK!")

s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('127.0.0.1', 4000))

enable_proxy = True
send_login_request_first = True

login_request = b"\x90\x00\x00\x01\x05\xa6\xff\x01\x00\x00\x00\x01\x21\x00\x00\x00" \
b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
b"\x00\x00\x00\x00\x72\x6f\x6f\x74\x00\x00\x69\x03\x5f\x6f\x73\x08" \
b"\x6f\x73\x78\x31\x30\x2e\x31\x32\x0c\x5f\x63\x6c\x69\x65\x6e\x74" \
b"\x5f\x6e\x61\x6d\x65\x08\x6c\x69\x62\x6d\x79\x73\x71\x6c\x04\x5f" \
b"\x70\x69\x64\x05\x33\x39\x38\x33\x38\x0f\x5f\x63\x6c\x69\x65\x6e" \
b"\x74\x5f\x76\x65\x72\x73\x69\x6f\x6e\x06\x35\x2e\x37\x2e\x31\x39" \
b"\x09\x5f\x70\x6c\x61\x74\x66\x6f\x72\x6d\x06\x78\x38\x36\x5f\x36" \
b"\x34\x0c\x70\x72\x6f\x67\x72\x61\x6d\x5f\x6e\x61\x6d\x65\x05\x6d" \
b"\x79\x73\x71\x6c"

proxy_request = b"PROXY UNKNOWN\r\n"

if send_login_request_first:
    if enable_proxy:
        s.send(proxy_request + login_request)
    else:
        s.send(login_request)
    server_greeting = s.recv(4096)
else:
    if enable_proxy:
        s.send(proxy_request)
    server_greeting = s.recv(4096)
    s.send(login_request)

server_response = s.recv(4096)
s.close()

print("Server greeting:\n")
hexOutput(server_greeting)

print("Server response:\n")
hexOutput(server_response)
checkResponseOk(server_response)

• When enable_proxy = True and send_login_request_first = True, PROXY + LOGIN_REQUEST will be sent together in the first TCP packet. (FAIL)
• When enable_proxy = True and send_login_request_first = False, PROXY packet will be sent first, then it will expect a SERVER_GREETING. After that a LOGIN_REQUEST will be sent. (SUCCESS)
• enable_proxy = False, send_login_request_first = True/False: SUCCESS

This problem may relate to the function newPacketIO. You can take a look.

Finally, I doubt the existence of such client that sends LOGIN_REQUEST before SERVER_GREETING, since it violates the MySQL protocol and is obviously not able to do authentication (since authentication salt is in SERVER_GREETING).

[blacktear23]

@breeswish thanks for you test! I found this is a big bug here. :)

[sre-bot]

Hi contributor, thanks for your PR.

This patch needs to be approved by someone of admins. They should reply with "/ok-to-test" to accept this PR for running test automatically.

[iamxy]

/ok-to-test

[iamxy]

/run-all-test

[breezewish]

TLS/SSL functionality is merged, please resolve conflicts and tests whether your implementation works with it.

By the way, in TLS/SSL PR, we extracted the buffered reader from packetIO into Conn so that the buffered reader can be used outside packetIO. I guess proxyProtocolConn in your PR can be dropped now. You can just do buffered read from Conn.bufReadConn. If there are remaining bytes after reading the PROXY protocol, it will be read by the packetIO since they share the same buffered reader.

[blacktear23]

@breeswish I already test proxy-protocol with SSL, it works in my server. For use bufferedReadConn instead proxyProtocolConn I will dive into it.

[breezewish]

@blacktear23 Ok

[blacktear23]

@breeswish I found bufio.Reader is not fit for proxyProtocolConn, Peek method will call Read until buffer full or peek size reached, and this will cause EOF on unit test and also block connection on tidb-server. Science proxyProtocolConn is working well and bufferedReadConn cannot fit proxyProtocolConn's require I think keep current proxyProtocolConn will be a good decision.

[blacktear23]

@breeswish below python script is modified by your script for PROXY protocol v2 test. Hope it could help.

import socket

def hexOutput(bytes):
    hex = ['{:02x}'.format(x) for x in bytes]
    for i in range(0, len(hex), 16):
        print(' '.join(hex[i:i+16]))
    print('')

def checkResponseOk(bytes):
    packet_len = int.from_bytes(bytes[0:3], byteorder='little')
    packet_body = bytes[4:]
    is_ok = packet_body[0] == 0x00
    if not is_ok:
        assert packet_body[0] == 0xff
        err_code = int.from_bytes(packet_body[1:3], byteorder='little')
        err_body = packet_body[10:].decode()
        print("ERROR {0}: {1}".format(err_code, err_body))
    else:
        print("OK!")

s = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
s.connect(('127.0.0.1', 4000))

enable_proxy = True
send_login_request_first = True

login_request = b"\x90\x00\x00\x01\x05\xa6\xff\x01\x00\x00\x00\x01\x21\x00\x00\x00" \
b"\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00\x00" \
b"\x00\x00\x00\x00\x72\x6f\x6f\x74\x00\x00\x69\x03\x5f\x6f\x73\x08" \
b"\x6f\x73\x78\x31\x30\x2e\x31\x32\x0c\x5f\x63\x6c\x69\x65\x6e\x74" \
b"\x5f\x6e\x61\x6d\x65\x08\x6c\x69\x62\x6d\x79\x73\x71\x6c\x04\x5f" \
b"\x70\x69\x64\x05\x33\x39\x38\x33\x38\x0f\x5f\x63\x6c\x69\x65\x6e" \
b"\x74\x5f\x76\x65\x72\x73\x69\x6f\x6e\x06\x35\x2e\x37\x2e\x31\x39" \
b"\x09\x5f\x70\x6c\x61\x74\x66\x6f\x72\x6d\x06\x78\x38\x36\x5f\x36" \
b"\x34\x0c\x70\x72\x6f\x67\x72\x61\x6d\x5f\x6e\x61\x6d\x65\x05\x6d" \
b"\x79\x73\x71\x6c"

proxy_request = b"\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A\x20\x00\00\00"

if send_login_request_first:
    if enable_proxy:
        s.send(proxy_request + login_request)
    else:
        s.send(login_request)
    server_greeting = s.recv(4096)
else:
    if enable_proxy:
        s.send(proxy_request)
    server_greeting = s.recv(4096)
    s.send(login_request)

server_response = s.recv(4096)
s.close()

print("Server greeting:\n")
hexOutput(server_greeting)

print("Server response:\n")
hexOutput(server_response)
checkResponseOk(server_response)

[blacktear23]

@breeswish I create a PR (https://github.com/blacktear23/tidb/pull/1) for proxyprotocol branch for refactor proxyprotocol using bufferedReadConn. If it seems OK, I will merge this PR.

[coocood]

@blacktear23 Have you considered using this library https://github.com/armon/go-proxyproto

It can wrap a listener, and accept both proxy and direct connection, only that it doesn't support V2.

I think this approach is cleaner, we don't need to import proxy in server package, just pass listener as an argument to NewServer.