Add support for PROXY protocol

[Sir-Photch]

Suggested in #28. Right now, receiving v1 works, but the information is just dumped into the command line. Should I put the proxy-protocol information into struct client?

[omar-polo]

Hello,

thanks for the the effort and sorry for the delay!

I haven't tried the diff yet, but it looks fine to me. I have a few nits before we can merge this, but first I'd ask you if it would be possible amend the indentation changes in server.c in the first commit. It makes the diff harder to review.

Thanks!

[Sir-Photch]

but first I'd ask you if it would be possible amend the indentation changes in server.c in the first commit.

Yes, sorry. Apparently I told my editor to auto format at some point; I will take care of that.

We should probably add some unit-tests for parsing. And what do you think about fuzzing? Clang supports this AFAIK, but I am not sure about gcc. Also, is this parsing happening in a process that drops its privileges? (It was (is) a little daunting to write network facing C code :b)

If you want to test the behaviour with nginx in front, you can refer to this snippet:

stream {
        server {
                listen 1966;
                proxy_pass 127.0.0.1:1965;

                proxy_protocol on;
        }
        server {
                listen 1967;
                proxy_pass [::1]:1965;

                proxy_protocol on;
        }
}

this will proxy requests to port 1966 as IPv4 and to 1967 as IPv6, while passing it back to gmid.

[omar-polo]

Yes, sorry. Apparently I told my editor to auto format at some point; I will take care of that.

no problem.

We should probably add some unit-tests for parsing.

Yep, I'd really like some tests for this. We have both unit tests (although only used for the IRI parsing code) and runtime tests. If we had support for the proxy protocol also for, ehm, proxying connections it would be easy to do.

But no rush for this, we'll get to it after the diff is in a state where it can be merged :)

And what do you think about fuzzing? Clang supports this AFAIK, but I am not sure about gcc.

I wouldn't mind having something for fuzzying. Maybe this is the chance I needed to look more into it. AFAIK the compiler doesn't really matter, but you can use stuff like asan and ubsan too which may depend on the compiler/platform.

Also, is this parsing happening in a process that drops its privileges? (It was (is) a little daunting to write network facing C code :b)

Yes, the connection handling runs in a process that has dropped privileges, chrooted and, depending on the system, sandboxed itself.

If you want to test the behaviour with nginx in front, you can refer to this snippet:

Thanks! It'll take me a bit to be able to actually test this, but many thanks for the nginx snippet!

[Sir-Photch]

Hi, I've cleaned up the whitespace changes. My actual changes should still be there. :) make regress runs fine indicating that non-proxy-protocol connections are fine. (But there are no test for the protocol yet, of course.)

[Sir-Photch]

I rewrote the parsing according to your suggestions. Test are obviously failing now since proxy-protocol is expected now, what's missing is some kind of configuration for server_accept() to set c->buf.should_buffer accordingly. That doesn't change during runtime, so maybe via a global variable?

[omar-polo]

To be fair it's a bit lame the way I've added the proxy-v1 keyword. I'll refactor a bit later how listen is processed so it's tidier.

After the pending thing will be fixed I believe we can merge this! I hope you won't take it bad, but I'll probably rebase it against the master (I prefer to have a linear history and avoid merges), squash some commits, and pass proxy-proto.c thru knf, mostly for indentation, for consistency with the rest of the code.

Thanks! :)

[Sir-Photch]

I hope you won't take it bad, but I'll probably rebase it against the master (I prefer to have a linear history and avoid merges), squash some commits, and pass proxy-proto.c thru knf, mostly for indentation, for consistency with the rest of the code.

No problem, this is your codebase after all, and I admit that my commit-messages are not necessarily very meaningful. (Also some of them were drafts, so no reason not to squash.)

Thanks for pointing me to knf though, it's very interesting how OpenBSD folk do things :)

[omar-polo]

I"ve merged it manually, so unfortunately on github it will show as 'closed' instead of merged :(

Thank you for working on this and enduring my requests :)

There are two follow-up commits that I'd like to point out since they may be of interest for you and that I noticed only after starting to playing with this.

• https://github.com/omar-polo/gmid/commit/acbe7a53131d0ba42bf919ea7a11dc7d1b60fb9c
• https://github.com/omar-polo/gmid/commit/2a090b866d78d8e9a06a1e49e6b48cd4b5a488d0

[omar-polo]

Just FYI I've started to fuzz some parts, the proxy protocol and the IRI parser. See regress/fuzz/README.md if you're interested :)