[PATCH] Add proxy_protocol option to mail listener

[Maxim Dounin]

Hello!

On Thu, Jul 20, 2017 at 09:52:08AM +0200, Kees Bos wrote:

> On di, 2017-07-18 at 18:02 +0300, Maxim Dounin wrote:
> > Hello!
> > 
> > On Tue, Jul 18, 2017 at 03:13:21PM +0200, Kees Bos wrote:
> > 
> > > 
> > > Some inline stuff just to be sure I do understand what you mean.
> > > 
> > > On di, 2017-07-18 at 15:56 +0300, Maxim Dounin wrote:
> > > > 
> > > > Hello!
> > > > 
> > > > On Tue, Jul 18, 2017 at 12:06:09PM +0200, Kees Bos wrote:
> > > > 
> > > > > 
> > > > > 
> > > > > # HG changeset patch
> > > > > # User Kees Bos <cornelis.bos at gmail.com>
> > > > > # Date 1500371531 0
> > > > > #      Tue Jul 18 09:52:11 2017 +0000
> > > > > # Node ID 8dd6050ca6858d9bea139067611ca5c69cfe8f18
> > > > > # Parent  e3723f2a11b7ec1c196d59c331739bc21d9d9afd
> > > > > Add proxy_protocol option to mail listener
> > > > > 
> > > > > Add support for the mail handlers. This enables the use of an
> > > > > upstream
> > > > > loadbalancer/proxy (like haproxy) that connects with the proxy
> > > > > protocol.
> > > > > 
> > > > > The original ip (as exposed with the proxy protocol) will be
> > > > > used
> > > > > as
> > > > > parameter for the 'Client-IP' in the authentication call and as
> > > > > address
> > > > > in the XCLIENT call.
> > > > > 
> > > > > Optionally (if set), the real ips from the client that are
> > > > > using
> > > > > the
> > > > > proxy protocol can be restricted with "set_real_ip_from".
> > > > This approach looks unsafe and counter-intuitive.
> > > > 
> > > > Instead, address should be changed if and only if there is 
> > > > set_real_ip_from and it lists a particular client address, much 
> > > > like it is done in http and stream modules.
> > > So, "set_real_ip_from" is required as soon as "proxy_protocol" is
> > > used
> > > in the listen directive.
> > > 
> > > Correct?
> > > 
> > > 
> > > > 
> > > > 
> > > > > 
> > > > > 
> > > > > 
> > > > > Example config:
> > > > > mail {
> > > > >     server_name mail.example.com;
> > > > >     auth_http   localhost:9000/;
> > > > > 
> > > > >     server {
> > > > >         listen 143 proxy_protocol;
> > > > >         protocol imap;
> > > > >     }
> > > > That is, only parsing of PROXY protocol header should happen
> > > > here.
> > > And the connection will be closed since "set_real_ip_from" is
> > > missing.
> > > 
> > > Correct?
> > > 
> > No.
> > 
> > Try looking at http and/or stream modules: "listen ...  
> > proxy_protocol" means that nginx will accept PROXY protocol 
> > header, and will make its contents available via the 
> > $proxy_protocol_addr and $proxy_protocol_port variables.
> > 
> > When "set_real_ip_from ...; real_ip_header proxy_protocol;" is 
> > additionally used, the address obtained from the PROXY protocol 
> > header will be used as a client address.
> > 
> > > 
> > > > 
> > > > > 
> > > > >     server {
> > > > >         listen 25 proxy_protocol;
> > > > >         protocol smtp;
> > > > >         set_real_ip_from 127.0.0.0/8;
> > > > >         set_real_ip_from ::/128;
> > > > And here we can change client's address if a connection was from 
> > > > listed addresses.
> > > > 
> > > > We may also consider sending the information from the header in 
> > > > separate auth_http headers (something like Proxy-Protocol-IP, 
> > > > Proxy-Protocol-Port?) regardless of set_real_ip_from.  But
> > > > clearly 
> > > > this should be a separate header from Client-IP to make it 
> > > > possible for auth_http script to decide if this information
> > > > should 
> > > > be trusted or not.
> > > Would an additional Client-Real-IP and Client-Real-Port be better?
> > I don't think so.
> > 
> > The word "Real" is misleading.  We don't know if it's real or not, 
> > it is up to the script to decide if the address should be trusted 
> > to use PROXY protocol.
> > 
> > Additionally, it doesn't describe the source of the information, 
> > so it is a) not clear how Client-IP is different from 
> > Client-Real-IP, and b) if a different source will be introduced 
> > (for example, XCLIENT), we will have to invent another way to name 
> > things.
> > 
> > The Proxy-Protocol-IP as proposed above is an attempt to provide 
> > something similar to $proxy_protocol_addr and Client-IP at the 
> > same time.
> > 
> > (Given that we currently don't provide Client-Port in auth_http, 
> > Proxy-Protocol-Port probably is a bad idea.)
> 
> 
> Maybe it would be a bit future proof (in case some other mangling
> protocols will be invented) to use (iff proxy-protocol ip address is
> set) something like:
> 
> Proxy-IP: <connection ip address>
> Original-IP: <proxy-protocol ip address>

I don't see how it resolves the same disadvanteges as outline for 
the Client-Real-IP header.  Much like in the Client-Real-IP case,

- "Original" is misleading, and 

- it will conflict if we'll have an IP address from a different 
  source (for example, the XCLIENT SMTP command).

Additionally, I don't see reasons to introduce Proxy-IP instead of 
currently used Client-IP.  It looks unneeded (we already have 
Client-IP for the very same data, no?) and also confusing (it can 
be easily interpreted as $proxy_protocol_addr instead, at least 
this is what I initially tought when reading your messages).

-- 
Maxim Dounin
http://nginx.org/