Re: PROXY protocol support

On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion <pchampion(at)vmware(dot)com> wrote:
>
> On Tue, 2021-03-09 at 11:25 +0100, Magnus Hagander wrote:
> > I've also added some trivial tests (man that took an ungodly amount of
> > fighting perl -- it's clearly been a long time since I used perl
> > properly).
>
> Yeah. The tests I'm writing for this and NSS have been the same way;
> it's a real problem. I'm basically writing supplemental tests in Python
> as the "daily driver", then trying to port whatever is easiest (not
> much) into Perl, when I get time.
>
> == More Notes ==
>
> Some additional spec-compliance stuff:
>
> > /* Lower 4 bits hold type of connection */
> > if (proxyheader.fam == 0)
> > {
> > /* LOCAL connection, so we ignore the address included */
> > }
>
> (fam == 0) is the UNSPEC case, which isn't necessarily LOCAL. We have
> to do something different for the LOCAL case:

Oh ugh. yeah, and the comment is wrong too -- it got the "command"
confused with "connection family". Too many copy/paste I think.

> > - \x0 : LOCAL : [...] The receiver must accept this connection as
> > valid and must use the real connection endpoints and discard the
> > protocol block including the family which is ignored.
>
> So we should ignore the entire "protocol block" (by which I believe
> they mean the protocol-and-address-family byte) in the case of LOCAL,
> and just accept it with the original address info intact. That seems to
> match the sample code in the back of the spec. The current behavior in
> the patch will apply the PROXY behavior incorrectly if the sender sends
> a LOCAL header with something other than UNSPEC -- which is strange
> behavior but not explicitly prohibited as far as I can see.

Yeah, I think we do the right thing in the "right usecase".

> We also need to reject all connections that aren't either LOCAL or
> PROXY commands:

Indeed.

> > - other values are unassigned and must not be emitted by senders.
> > Receivers must drop connections presenting unexpected values here.
>
> ...and naturally it'd be Nice (tm) if the tests covered those corner
> cases.

I think that's covered in the attached update.

> Over on the struct side:
>
> > + struct
> > + { /* for TCP/UDP over IPv4, len = 12 */
> > + uint32 src_addr;
> > + uint32 dst_addr;
> > + uint16 src_port;
> > + uint16 dst_port;
> > + } ip4;
> > ... snip ...
> > + /* TCPv4 */
> > + if (proxyaddrlen < 12)
> > + {
>
> Given the importance of these hardcoded lengths matching reality, is it
> possible to add some static assertions to make sure that sizeof(<ipv4
> block>) == 12 and so on? That would also save any poor souls who are
> using compilers with nonstandard struct-packing behavior.

Yeah, probably makes sense. Added.

--
Magnus Hagander
Me: https://www.hagander.net/
Work: https://www.redpill-linpro.com/