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:
> - \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.
We also need to reject all connections that aren't either LOCAL or
PROXY commands:
> - 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.
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.
--Jacob
