Re: [PATCH] Add proxy_protocol option to mail listener
Hello!
On Mon, Aug 07, 2017 at 03:23:12PM +0200, Kees Bos wrote:
> # HG changeset patch
> # User Kees Bos
> # Date 1500565189 0
> # Thu Jul 20 15:39:49 2017 +
> # Node ID 327f18e079b175b14277a23e75715f5feee34d69
> # Parent 863b862534d7ac0dbf8babf68b824de6fb0d6ef4
> Add proxy_protocol option to mail listener
Style, should be:
Mail: added proxy_protocol option to mail listener.
>
> Add support for the mail handlers. This enables the use of an upstream
> loadbalancer/proxy that connects with the proxy protocol. Examples of
> this are haproxy or a nginx stream handler that uses then proxy protocol
> in client conections.
>
> The proxy protocol source ip address will we exposed to the auth
> handler as 'Proxy-Protocol-IP'.
Probably this should be a separate patch. That is, one patch for
"listen ... proxy_protocol" and "Proxy-Protocol-IP", and another
one for set_real_ip_from (see below).
>
> If the sender ip address matches one or more "set_real_ip_from" directives,
> the source ip address as specified in the in the proxy protocol will be
> used as 'Client-IP' in the authentication call and as address in the
> XCLIENT call.
>
> Example config:
> mail {
> server_name mail.example.com;
> auth_http localhost:9000/;
>
> server {
> listen 143 proxy_protocol;
> protocol imap;
> }
>
> server {
> listen 25 proxy_protocol;
> protocol smtp;
> set_real_ip_from 127.0.0.0/8;
> set_real_ip_from ::/128;
> }
> }
>
> In the imap config, the source address given in the proxy protocol will
> never be used as Client-IP.
>
> In the smtp config, the source address given in the proxy protocol will
> only be used as XCLIENT address when the sender address matches the
> "set_real_ip_from" settings (in this case only loopback address).
This doesn't seem to match what is expected to happen whan a real
IP is set.
Much like in http and stream modules, if set_real_ip_from includes
the client's address, then the client's address have to be
replaced with one provided via PROXY protocol. And then this
address will be used everywhere where nginx uses client's address.
It looks like the set_real_ip_from part might not be familiar to
you, so please consider preserving only the first patch, with
"listen ... proxy_protocol" and "Proxy-Protocol-IP".
[...]
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
Thanks Kees and Maxim, for the patch and the discussion. A very helpful
community!
This looks like it's working well for my use case for a tcp465/"ssl on"
mail proxy server block. My earlier issues appear to have (at least
partly) been related to STARTTLS with a tcp587/"starttls only" block
that I started my investigations with...
Once I switched over to the straight SSL block things started working a
lot better. I'll get back to my STARTTLS challenge in a little bit,
unless there's some reason you know of off-hand that it should act
differently?
The only other thing I've noticed is an inconsistency/typo in the
'set_real_ip_from' directive (as in the documentation section of the
patchset). It looks like it is actually parsed as 'set_realip_from' (no
_ in the realip part). I'm not sure which is the correct form ( :-) ),
but the feature works when I use the 'set_realip_from' form , so it does
differ from the docs...
Thanks again Kees! Wayde.
On 08/07/2017 09:25 AM, Kees Bos wrote:
> On ma, 2017-08-07 at 07:49 -0400, Wayde Nie wrote:
>> On 08/07/2017 03:44 AM, Kees Bos wrote:, and when I do set
proxy_protocol
on the listen directive I see the correct ip and port picked up and
logged in the error.log, however, then nginx stops sending the smtp
greeting... My mail client connects to my loadbalancer, the lb
connects
to nginx:587 sending the PROXY line, nginx parses and logs the PROXY
fields, then the client times out waiting for any return traffic
from
nginx... I know it's something I'm doing :-)
I'm happy to keep poking away at it, but curious mostly, if you think
the approach is sound? (ie. use $proxy_protocol_addr, set by
proxy_protocol directive and pass in to auth_http script in auth url
as
a get param?) and if an initial patch that starts by just setting
$proxy_protocol_* variables would be a useful first step in this type
of
functionality?
> Just to get the picture right (it looks to me that your downstream smtp
> server expects the proxy protocol), what's the exact flow you're trying
> to accomplish?
>>
>> Hi Kees,
>>
>> Thanks for looking!
>>
>> In my use case I have an external hardware loadbalancer that is
>> receiving end user connections on a VIP, pre-pending proxy_protocol
>> header and loadbalancing them to small pool of nginx servers running
>> as the mail proxies. Nginx is parsing the proxy_protocol header and
>> (I hope) proxying to my upstream smtp server without passing the
>> proxy_protocol header, which my upstream smtp server doesn't support
>> (as currently implemented).
>>
>> Flow like:
>>
>> 1) Client makes connection to [external-LB-VIP:587] for email
>> submission
>> 2) [external-LB-VIP:587] --> injects proxy_protocol header -->
>> load balances to set of nginx services via TCP service pool (ie:
>> lb straight tcp, no application level inspection by loadbalancer,
>> other than prepending proxy_protocol header)
>> 3) Nginx parses proxy_protocol header, logs client ip and passes
>> client IP into auth_http script, along with username and password
>> for authn/authz
>> 4) on successful return response from auth_http; nginx proxies
>> mail submission to upstream smtp server without proxy_protocol
>> header.
>>
>>
>> So, if possible, I'd like nginx to get the client ip passed to it
>> from the external hardware load balancer, log it and then use it in
>> the auth_http script, but otherwise not pass it on to the upstream
>> smtp server...
>>
>> Is this doable?
>>
>> Thanks,Wayde.
>
> Yep. Should be.
>
> I just noticed that my mail from Jul 20 is emtpy and should contain
> the latest patch. I've just sent the patch.
>
>
> In your case, the config should probably be something like:
>
> mail {
> server_name mail.example.com;
> auth_http ;
> server {
> listen 587 proxy_protocol;
> protocol smtp;
> set_real_ip_from ;
> }
> }
>
>
>
>
>
>
>
> ___
> nginx-devel mailing list
> nginx-devel@nginx.org
> http://mailman.nginx.org/mailman/listinfo/nginx-devel
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
On ma, 2017-08-07 at 07:49 -0400, Wayde Nie wrote:
> > > proxy_protocol
> > > on the listen directive I see the correct ip and port picked up
> > > and
> > > logged in the error.log, however, then nginx stops sending the
> > > smtp
> > > greeting... My mail client connects to my loadbalancer, the lb
> > > connects
> > > to nginx:587 sending the PROXY line, nginx parses and logs the
> > > PROXY
> > > fields, then the client times out waiting for any return traffic
> > > from
> > > nginx... I know it's something I'm doing :-)
> > >
> > > I'm happy to keep poking away at it, but curious mostly, if you
> > > think
> > > the approach is sound? (ie. use $proxy_protocol_addr, set by
> > > proxy_protocol directive and pass in to auth_http script in auth
> > > url
> > > as
> > > a get param?) and if an initial patch that starts by just setting
> > > $proxy_protocol_* variables would be a useful first step in this
> > > type
> > > of
> > > functionality?
> > > > Just to get the picture right (it looks to me that your
> > > > downstream smtp
> > > > server expects the proxy protocol), what's the exact flow
> > > > you're trying
> > > > to accomplish?
> > > >
>
> Hi Kees,
>
> Thanks for looking!
>
> In my use case I have an external hardware loadbalancer that is
> receiving end user connections on a VIP, pre-pending proxy_protocol
> header and loadbalancing them to small pool of nginx servers running
> as the mail proxies. Nginx is parsing the proxy_protocol header and
> (I hope) proxying to my upstream smtp server without passing the
> proxy_protocol header, which my upstream smtp server doesn't support
> (as currently implemented).
>
> Flow like:
> 1) Client makes connection to [external-LB-VIP:587] for email
> submission
> 2) [external-LB-VIP:587] --> injects proxy_protocol header --> load
> balances to set of nginx services via TCP service pool (ie: lb
> straight tcp, no application level inspection by loadbalancer, other
> than prepending proxy_protocol header)
> 3) Nginx parses proxy_protocol header, logs client ip and passes
> client IP into auth_http script, along with username and password for
> authn/authz
> 4) on successful return response from auth_http; nginx proxies mail
> submission to upstream smtp server without proxy_protocol header.
>
> So, if possible, I'd like nginx to get the client ip passed to it
> from the external hardware load balancer, log it and then use it in
> the auth_http script, but otherwise not pass it on to the upstream
> smtp server...
>
> Is this doable?
>
> Thanks,Wayde.
Yep. Should be.
I just noticed that my mail from Jul 20 is emtpy and should contain the
latest patch. I've just sent the patch.
In your case, the config should probably be something like:
mail {
server_name mail.example.com;
auth_http ;
server {
listen 587 proxy_protocol;
protocol smtp;
set_real_ip_from ;
}
}
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
On 08/07/2017 03:44 AM, Kees Bos wrote:, and when I do set >> proxy_protocol >> on the listen directive I see the correct ip and port picked up and >> logged in the error.log, however, then nginx stops sending the smtp >> greeting... My mail client connects to my loadbalancer, the lb >> connects >> to nginx:587 sending the PROXY line, nginx parses and logs the PROXY >> fields, then the client times out waiting for any return traffic >> from >> nginx... I know it's something I'm doing :-) >> >> I'm happy to keep poking away at it, but curious mostly, if you think >> the approach is sound? (ie. use $proxy_protocol_addr, set by >> proxy_protocol directive and pass in to auth_http script in auth url >> as >> a get param?) and if an initial patch that starts by just setting >> $proxy_protocol_* variables would be a useful first step in this type >> of >> functionality? >>> Just to get the picture right (it looks to me that your downstream smtp >>> server expects the proxy protocol), what's the exact flow you're trying >>> to accomplish? Hi Kees, Thanks for looking! In my use case I have an external hardware loadbalancer that is receiving end user connections on a VIP, pre-pending proxy_protocol header and loadbalancing them to small pool of nginx servers running as the mail proxies. Nginx is parsing the proxy_protocol header and (I hope) proxying to my upstream smtp server without passing the proxy_protocol header, which my upstream smtp server doesn't support (as currently implemented). Flow like: 1) Client makes connection to [external-LB-VIP:587] for email submission 2) [external-LB-VIP:587] --> injects proxy_protocol header --> load balances to set of nginx services via TCP service pool (ie: lb straight tcp, no application level inspection by loadbalancer, other than prepending proxy_protocol header) 3) Nginx parses proxy_protocol header, logs client ip and passes client IP into auth_http script, along with username and password for authn/authz 4) on successful return response from auth_http; nginx proxies mail submission to upstream smtp server without proxy_protocol header. So, if possible, I'd like nginx to get the client ip passed to it from the external hardware load balancer, log it and then use it in the auth_http script, but otherwise not pass it on to the upstream smtp server... Is this doable? Thanks,Wayde. ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
On za, 2017-08-05 at 20:44 -0400, Wayde Nie wrote: > Hi Kees, Maxim, > > I'm very interested in proxy_protocol enabled nginx mail proxy as > well. > Wondering if the feature might be more straightforward (initially?) > if, > when enabled on the listen line, it simply set the appropriate > $proxy_protocol_* variables, similar to the http servers? > > I was hoping that I could then include it as an http get param in the > url for my auth_http directive for the auth script to do a dns > blacklist > lookup and/or logging (and possibly some other anti spambot efforts). > > I'm working on it with your patch as a starting point, and I can get > it > to compile cleanly. Nginx keeps working as expected when I don't set > proxy_protocol on the listen directive, and when I do set > proxy_protocol > on the listen directive I see the correct ip and port picked up and > logged in the error.log, however, then nginx stops sending the smtp > greeting... My mail client connects to my loadbalancer, the lb > connects > to nginx:587 sending the PROXY line, nginx parses and logs the PROXY > fields, then the client times out waiting for any return traffic > from > nginx... I know it's something I'm doing :-) > > I'm happy to keep poking away at it, but curious mostly, if you think > the approach is sound? (ie. use $proxy_protocol_addr, set by > proxy_protocol directive and pass in to auth_http script in auth url > as > a get param?) and if an initial patch that starts by just setting > $proxy_protocol_* variables would be a useful first step in this type > of > functionality? > > Thanks! Wayde. Just to get the picture right (it looks to me that your downstream smtp server expects the proxy protocol), what's the exact flow you're trying to accomplish? ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
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
> > > > > # Date 1500371531 0
> > > > > # Tue Jul 18 09:52:11 2017 +
> > > > > # 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:
> Original-IP:
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
Re: [PATCH] Add proxy_protocol option to mail listener
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
> > > > # Date 1500371531 0
> > > > # Tue Jul 18 09:52:11 2017 +
> > > > # 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:
Original-IP:
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
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
> > > # Date 1500371531 0
> > > # Tue Jul 18 09:52:11 2017 +
> > > # 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.)
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
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
> > # Date 1500371531 0
> > # Tue Jul 18 09:52:11 2017 +
> > # 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?
> >
> >
> > 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?
> (There are also multiple style issues in the code. Some are
> outlined below, though I haven't focused on this as the code logic
> is to be changed anyway. Most of the comments apply to more than
> one place.)
Tnx
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
Hello!
On Tue, Jul 18, 2017 at 12:06:09PM +0200, Kees Bos wrote:
> # HG changeset patch
> # User Kees Bos
> # Date 1500371531 0
> # Tue Jul 18 09:52:11 2017 +
> # 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.
>
> 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.
>
> 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.
(There are also multiple style issues in the code. Some are
outlined below, though I haven't focused on this as the code logic
is to be changed anyway. Most of the comments apply to more than
one place.)
[...]
> --- a/src/mail/ngx_mail_auth_http_module.c Mon Jul 17 17:23:51 2017 +0300
> +++ b/src/mail/ngx_mail_auth_http_module.c Tue Jul 18 09:52:11 2017 +
> @@ -1142,6 +1142,7 @@
> ngx_mail_ssl_conf_t *sslcf;
> #endif
> ngx_mail_core_srv_conf_t *cscf;
> +ngx_str_t *client_addr;
Style: variables should be sorted from shortest type to longest.
Moreover, there are other ngx_str_t variables, so it should be
added to the already existing list instead.
> --- a/src/mail/ngx_mail_handler.c Mon Jul 17 17:23:51 2017 +0300
> +++ b/src/mail/ngx_mail_handler.c Tue Jul 18 09:52:11 2017 +
> @@ -12,13 +12,14 @@
>
>
> static void ngx_mail_init_session(ngx_connection_t *c);
> -
> +static void ngx_mail_proxy_protocol_handler(ngx_event_t *rev);
> #if (NGX_MAIL_SSL)
The whitespace change looks wrong. SSL-related functions are
listed separately for a reason.
[...]
> -ngx_log_error(NGX_LOG_INFO, c->log, 0, "*%uA client %*s connected to %V",
> - c->number, len, text, s->addr_text);
Removing a logging right after a connection is established might
be a bad idea. Instead, it might be a better option to introduce
additional logging if / when the address is changed.
> +if (s->proxy_protocol) {
> +c->log->action = "reading PROXY protocol";
> +ngx_add_timer(c->read, cscf->timeout);
> +c->read->handler = ngx_mail_proxy_protocol_handler;
> +if (ngx_handle_read_event(c->read, 0) != NGX_OK) {
> +ngx_mail_close_connection(c);
> +}
> +return;
> +}
Style: this clearly needs more empty lines. You may want to take
a look at the relevant code at the src/stream/ngx_stream_handler.c
for an example.
[...]
> +ngx_log_error(NGX_LOG_INFO, c->log, 0,
> +"*%uA client %*s (real %*s:%d) connected to %V",
> +c->number, len, text,
> +c->proxy_protocol_addr.len, c->proxy_protocol_addr.data,
> +c->proxy_protocol_port, s->addr_text);
Style: indentation is wrong. Instead, continuation lines should
be aligned to the first function argument instead.
[...]
> +static void *ngx_mail_realip_create_srv_conf(ngx_conf_t *cf);
> +static char *ngx_mail_realip_merge_srv_conf(ngx_conf_t *cf, void *parent,
> +void *child);
> +//static ngx_int_t ngx_mail_realip_init(ngx_conf_t *cf);
No C99-style comments please.
[...]
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
# HG changeset patch
# User Kees Bos
# Date 1500371531 0
# Tue Jul 18 09:52:11 2017 +
# 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".
Example config:
mail {
server_name mail.example.com;
auth_http localhost:9000/;
server {
listen 143 proxy_protocol;
protocol imap;
}
server {
listen 25 proxy_protocol;
protocol smtp;
set_real_ip_from 127.0.0.0/8;
set_real_ip_from ::/128;
}
}
diff -r e3723f2a11b7 -r 8dd6050ca685 auto/modules
--- a/auto/modules Mon Jul 17 17:23:51 2017 +0300
+++ b/auto/modules Tue Jul 18 09:52:11 2017 +
@@ -954,6 +954,12 @@
ngx_module_srcs=src/mail/ngx_mail_proxy_module.c
. auto/module
+
+ngx_module_name=ngx_mail_realip_module
+ngx_module_deps=
+ngx_module_srcs=src/mail/ngx_mail_realip_module.c
+
+. auto/module
fi
diff -r e3723f2a11b7 -r 8dd6050ca685 src/mail/ngx_mail.c
--- a/src/mail/ngx_mail.c Mon Jul 17 17:23:51 2017 +0300
+++ b/src/mail/ngx_mail.c Tue Jul 18 09:52:11 2017 +
@@ -408,6 +408,7 @@
#if (NGX_MAIL_SSL)
addrs[i].conf.ssl = addr[i].opt.ssl;
#endif
+addrs[i].conf.proxy_protocol = addr[i].opt.proxy_protocol;
len = ngx_sock_ntop(&addr[i].opt.sockaddr.sockaddr,
addr[i].opt.socklen,
buf, NGX_SOCKADDR_STRLEN, 1);
@@ -457,6 +458,7 @@
#if (NGX_MAIL_SSL)
addrs6[i].conf.ssl = addr[i].opt.ssl;
#endif
+addrs6[i].conf.proxy_protocol = addr[i].opt.proxy_protocol;
len = ngx_sock_ntop(&addr[i].opt.sockaddr.sockaddr,
addr[i].opt.socklen,
buf, NGX_SOCKADDR_STRLEN, 1);
diff -r e3723f2a11b7 -r 8dd6050ca685 src/mail/ngx_mail.h
--- a/src/mail/ngx_mail.h Mon Jul 17 17:23:51 2017 +0300
+++ b/src/mail/ngx_mail.h Tue Jul 18 09:52:11 2017 +
@@ -40,6 +40,7 @@
unsignedipv6only:1;
#endif
unsignedso_keepalive:2;
+unsignedproxy_protocol:1;
#if (NGX_HAVE_KEEPALIVE_TUNABLE)
int tcp_keepidle;
int tcp_keepintvl;
@@ -54,7 +55,8 @@
typedef struct {
ngx_mail_conf_ctx_t*ctx;
ngx_str_t addr_text;
-ngx_uint_t ssl;/* unsigned ssl:1; */
+unsignedssl:1;
+unsignedproxy_protocol:1;
} ngx_mail_addr_conf_t;
typedef struct {
@@ -204,6 +206,8 @@
unsignedesmtp:1;
unsignedauth_method:3;
unsignedauth_wait:1;
+unsignedssl:1;
+unsignedproxy_protocol:1;
ngx_str_t login;
ngx_str_t passwd;
diff -r e3723f2a11b7 -r 8dd6050ca685 src/mail/ngx_mail_auth_http_module.c
--- a/src/mail/ngx_mail_auth_http_module.c Mon Jul 17 17:23:51 2017 +0300
+++ b/src/mail/ngx_mail_auth_http_module.c Tue Jul 18 09:52:11 2017 +
@@ -1142,6 +1142,7 @@
ngx_mail_ssl_conf_t *sslcf;
#endif
ngx_mail_core_srv_conf_t *cscf;
+ngx_str_t *client_addr;
if (ngx_mail_auth_http_escape(pool, &s->login, &login) != NGX_OK) {
return NULL;
@@ -1208,6 +1209,11 @@
#endif
cscf = ngx_mail_get_module_srv_conf(s, ngx_mail_core_module);
+if (s->connection->proxy_protocol_addr.len) {
+client_addr = &s->connection->proxy_protocol_addr;
+} else {
+client_addr = &s->connection->addr_text;
+}
len = sizeof("GET ") - 1 + ahcf->uri.len + sizeof(" HTTP/1.0" CRLF) - 1
+ sizeof("Host: ") - 1 + ahcf->host_header.len + sizeof(CRLF) - 1
@@ -1221,7 +1227,7 @@
+ sizeof(CRLF) - 1
+ sizeof("Auth-Login-Attempt: ") - 1 + NGX_INT_T_LEN
+ sizeof(CRLF) - 1
- + sizeof("Client-IP: ") - 1 + s->connection->addr_text.len
+ + sizeof("Client-IP: ") - 1 + client_addr->len
+ sizeof(CRLF) - 1
+ sizeof("Client-Host: ") - 1 + s->host.len + sizeof(CRLF) - 1
+ sizeof("Auth-SMTP-Helo: ") - 1 + s->smtp_helo.len + sizeof(CRLF) -
1
@@ -1287,8 +1293,7 @@
s->login_attempt);
b->last = ngx_cpymem(b->last, "Client-IP: ", sizeof("Client-IP: ") - 1);
-b->last = ngx_copy(b->last, s->connection->addr_text.data,
- s->connection->addr_text.len);
+b->last = ngx_copy(b->last, client_addr->data, client_addr->len
Re: [PATCH] Add proxy_protocol option to mail listener
On wo, 2017-07-12 at 15:56 +0300, Maxim Dounin wrote: > On Wed, Jul 12, 2017 at 02:08:31PM +0200, Kees Bos wrote: > On di, 2017-07-11 at 18:12 +0300, Maxim Dounin wrote: > > > On Fri, Jul 07, 2017 at 03:38:02PM +0200, Kees Bos wrote: > > > 2. It unconditionally trusts all clients who can connect to the > > > port in question. This doesn't look wise. > > I'm not sure what you mean here. > > > > There's no way to verify the correctness of the proxy protocol > > (that's > > also true so for the http/stream implementation). If a proxy > > protocol > > claims to originate from 1.1.1.1:1 and that the connection was > > originally to 2.2.2.2:2 the listener has no way to know that that's > > correct (or not). > Obviously enough, you can't verify the information provided. But > you can trust or do not trust to the particular client. For > example, in the ngx_http_realip_module this is done using the > set_real_ip_from directive (http://nginx.org/r/set_real_ip_from) - > you can explicitly configure address blocks you want to allow to > set client's address based on the provided header or PROXY > protocol. Yes. That's clear. Now (I think) I understand what you mean. > > The link I've provided in the previous message contains an example > with set_real_ip_from as part of the review. > ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
Hello!
On Wed, Jul 12, 2017 at 02:08:31PM +0200, Kees Bos wrote:
> On di, 2017-07-11 at 18:12 +0300, Maxim Dounin wrote:
> > Hello!
> >
> > On Fri, Jul 07, 2017 at 03:38:02PM +0200, Kees Bos wrote:
> >
> > >
> > > # HG changeset patch
> > > # User Kees Bos
> > > # Date 1499422505 0
> > > # Fri Jul 07 10:15:05 2017 +
> > > # Node ID bc79b2baf494aabb889de1e5dbe3184ff0cb9bfa
> > > # Parent 70e65bf8dfd7a8d39aae8ac3a209d426e6947735
> > > 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.
> > >
> > > Example config:
> > > mail {
> > > server_name mail.example.com;
> > > auth_http localhost:9000/;
> > >
> > > server {
> > > listen 143 proxy_protocol;
> > > protocol imap;
> > > }
> > > }
> > I see at least the following problems in this patch:
>
> Thanks for the comments. I wasn't aware of a previous attempt (just
> started to look at nginx code etc). Apparently I'm not the only one
> interested in proxy_protocol for the mail module :-)
Well, I've provided the link when you asked about it in April in
the nginx@ mailing list:
http://mailman.nginx.org/pipermail/nginx/2017-April/053582.html
> > 1. The implementation is not complete compared to other modules
> > (http, stream): it only tries to change the address passed as
> > Client-IP in auth_http, but not the address used for logging
> > and/or for XCLIENT.
>
> I'll handle that.
>
> >
> > 2. It unconditionally trusts all clients who can connect to the
> > port in question. This doesn't look wise.
>
> I'm not sure what you mean here.
>
> There's no way to verify the correctness of the proxy protocol (that's
> also true so for the http/stream implementation). If a proxy protocol
> claims to originate from 1.1.1.1:1 and that the connection was
> originally to 2.2.2.2:2 the listener has no way to know that that's
> correct (or not).
Obviously enough, you can't verify the information provided. But
you can trust or do not trust to the particular client. For
example, in the ngx_http_realip_module this is done using the
set_real_ip_from directive (http://nginx.org/r/set_real_ip_from) -
you can explicitly configure address blocks you want to allow to
set client's address based on the provided header or PROXY
protocol.
The link I've provided in the previous message contains an example
with set_real_ip_from as part of the review.
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
On di, 2017-07-11 at 18:12 +0300, Maxim Dounin wrote:
> Hello!
>
> On Fri, Jul 07, 2017 at 03:38:02PM +0200, Kees Bos wrote:
>
> >
> > # HG changeset patch
> > # User Kees Bos
> > # Date 1499422505 0
> > # Fri Jul 07 10:15:05 2017 +
> > # Node ID bc79b2baf494aabb889de1e5dbe3184ff0cb9bfa
> > # Parent 70e65bf8dfd7a8d39aae8ac3a209d426e6947735
> > 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.
> >
> > Example config:
> > mail {
> > server_name mail.example.com;
> > auth_http localhost:9000/;
> >
> > server {
> > listen 143 proxy_protocol;
> > protocol imap;
> > }
> > }
> I see at least the following problems in this patch:
Thanks for the comments. I wasn't aware of a previous attempt (just
started to look at nginx code etc). Apparently I'm not the only one
interested in proxy_protocol for the mail module :-)
>
> 1. The implementation is not complete compared to other modules
> (http, stream): it only tries to change the address passed as
> Client-IP in auth_http, but not the address used for logging
> and/or for XCLIENT.
I'll handle that.
>
> 2. It unconditionally trusts all clients who can connect to the
> port in question. This doesn't look wise.
I'm not sure what you mean here.
There's no way to verify the correctness of the proxy protocol (that's
also true so for the http/stream implementation). If a proxy protocol
claims to originate from 1.1.1.1:1 and that the connection was
originally to 2.2.2.2:2 the listener has no way to know that that's
correct (or not).
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] Add proxy_protocol option to mail listener
Hello!
On Fri, Jul 07, 2017 at 03:38:02PM +0200, Kees Bos wrote:
> # HG changeset patch
> # User Kees Bos
> # Date 1499422505 0
> # Fri Jul 07 10:15:05 2017 +
> # Node ID bc79b2baf494aabb889de1e5dbe3184ff0cb9bfa
> # Parent 70e65bf8dfd7a8d39aae8ac3a209d426e6947735
> 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.
>
> Example config:
> mail {
> server_name mail.example.com;
> auth_http localhost:9000/;
>
> server {
> listen 143 proxy_protocol;
> protocol imap;
> }
> }
I see at least the following problems in this patch:
1. The implementation is not complete compared to other modules
(http, stream): it only tries to change the address passed as
Client-IP in auth_http, but not the address used for logging
and/or for XCLIENT.
2. It unconditionally trusts all clients who can connect to the
port in question. This doesn't look wise.
Just for the reference, here is a review of an earlier attempt to
introduce proxy_protocol support in the mail proxy:
http://mailman.nginx.org/pipermail/nginx-devel/2016-November/009084.html
[...]
> @@ -55,6 +56,7 @@
> ngx_mail_conf_ctx_t*ctx;
> ngx_str_t addr_text;
> ngx_uint_t ssl;/* unsigned ssl:1; */
> +unsignedproxy_protocol:1;
Note that "/* unsigned ssl:1; */" comment here means that if we
are going to add other bitfield options, the ssl field should be
changed to appthis should be changed to appropriate bitfield.
[...]
--
Maxim Dounin
http://nginx.org/
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
