Re: PROXY protocol support

On Thu, Mar 11, 2021 at 12:05 AM Jacob Champion <pchampion@vmware.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/