On Sun, Dec 1, 2013 at 6:03 PM, Phil Pennock <pdp@???> wrote:
> On 2013-11-30 at 20:17 +0000, Exim Git Commits Mailing List wrote:
>> +proxy_required_hosts = HOSTLIST
>> +
>> +The proxy_required_hosts option will require any IP in that hostlist
>> +to use Proxy Protocol. The specification of Proxy Protocol is very
>> +strict, and if proxy negotiation fails, Exim will not allow any SMTP
>> +command other than QUIT. (See end of this section for an example.)
>> +The option is expanded when used, so it can be a hostlist as well as
>> +string of IP addresses. Since it is expanded, specifying an alternate
>> +separator is supported for ease of use with IPv6 addresses.
>
> No control variable which can be set in an acl_smtp_connect ACL? :)
What would a control variable do for us? Once it gets to the ACL
processing, the proxy negotiation has already failed. Is your
contention that limiting to only the QUIT command is short sighted?
The spec very clearly spells out that a connection must be configured
to be a proxy connection, or a regular connection, and not do any kind
of auto-detection. I followed that thought process to come to the
conclusion that for a proxy configured host, it must pass, or no smtp
commands are to be allowed. I welcome better suggestions.
>> + H=mail.example.net [1.2.3.4] P=esmtp PRX=192.168.1.2 S=433
>> +
>> +3. In the ACL's the following expansion variables are available.
>> +
>> +proxy_host The src IP of the proxy server making the connection
>> +proxy_port The src port the proxy server is using
>> +proxy_session Boolean, yes/no, the connected host is required to use
>> + Proxy Protocol.
>
> Existing variable names suggest $proxy_host_address for the first
> variable, since _host/_hosts tends to be for hostlist consumers. This
> reduces problems with people expecting to see DNS hostnames.
>
> Given $sender_host_address and $sender_host_port, that would suggest
> $proxy_host_address and $proxy_host_port.
Good point. That's easy enough to change and I will do so.
>> + - This is not advised, but is mentioned for completeness if you have
>> + a specific internal configuration that you want this: If the Exim
>> + server only has an internal IP address and no other machines in your
>> + organization will connect to it to try to send email, you may
>> + simply set the hostlist to "*", however, this will prevent local
>> + mail programs from working because that would require mail from
>> + localhost to use Proxy Protocol. Again, not advised!
>
> Does this work?
>
> proxy_required_hosts = !@[] : *
Yes it does. I'll add that to the docs as a reasonable approach to
expecting all connections to use proxy protocol but still allowing any
locally originated email.
>> +#define LX_proxy 0x88000000
>
> Is anyone working on LX2 or any other means of handling logging? You've
> just taken the last available LX slot. You were right to do so, but it
> does leave us with a problem.
> 64-bit log_extra_selector variable? Something else? Bit-field
> array manipulation such as we use for D_* debug variables?
I admit to ignorance of this logging design, but after a cursory
inspection, it seems like sticking with the current design would
require just a few changes:
1) add a log_extra2_selector variable
2) #defines that use LX2_blah and 0x90000000 codes
3) bit logic would still use 0x7fffffff to filter
4) decode_bits() would have to be enhanced to work with a third
selector, and detect difference between second selector and third
selector
5) all new code would use log_extra2_selector & LX2_blah names
It does seem easier to just make log_extra_selector a 64 bit int.
>> +setup_proxy_protocol_host()
>> +{
>> +union {
>> + struct {
>> + uschar line[108];
>> + } v1;
>> + struct {
>> + uschar sig[12];
>> + uschar ver;
>
> 12 for sig ...
>
>> +const char v2sig[13] = "\x0D\x0A\x0D\x0A\x00\x0D\x0A\x51\x55\x49\x54\x0A\x02";
>
> Do you need to use any pragmas on struct definition to ensure that it's
> always aligned, on allocation, such that the ver will immediately follow
> on from the sig? I suspect that some linters will complain.
I used the declaration and code from the spec. I didn't consider that
alignment might be off.
>> + DEBUG(D_receive) debug_printf("Detected PROXYv2 header\n");
>> + size = 16 + hdr.v2.len;
>> + if (ret < size)
>> + {
>> + DEBUG(D_receive) debug_printf("Truncated or too large PROXYv2 header\n");
>> + goto proxyfail;
>> + }
>
> So the assumption is that the sender fully constructs the buffer and
> sends it in one write(), so we always see the entire thing? I see from
> <http://haproxy.1wt.eu/download/1.5/doc/proxy-protocol.txt> that the
> sender is required to do this, but also that receivers are recommended
> to be more tolerant. This is a reasonable choice, but should be called
> out in the experimental doc as a design choice.
Will do.
>> +proxyfail:
>> +/* Don't flush any potential buffer contents. Any input should cause a
>> +synchronization failure or we just don't want to speak SMTP to them */
>> +return FALSE;
>
> Should you be coercing smtp_enforce_sync on for proxy hosts? How does
> smtp_enforce_sync being set false interact with this?
Untested, I will test. It seems like it should not be a problem
because the code merely does a PEEK at the input buffer, so it should
be treated the same as if the Proxy Protocol code weren't being
called.
>> +done:
>> +flush_input();
>
> That flushes to the end of the next \n line. Is that really what is
> wanted at this point?
It there a better way to remove everything from the input buffer until
the newline? I thought this to be the most efficient and accurate way
to do it. The process got this far, which means that the contents of
the buffer matched the very strict definition of the header and all
data had been successfully extracted. It serves no further purpose at
this point and is invalid SMTP, so it should merely get discarded.
Any differing opinions or scenarios I haven't considered?
...Todd
