Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Hello! On Thu, Nov 10, 2016 at 01:06:54AM +0100, Bjørnar Ness wrote: [...] > > Current question is: > > > > What "listen ... proxy_protocol" should mean in case of mail. In > > other modules, it just means that PROXY protocol header is parsed > > and appropriate variables are available for use. It would be good > > to have similar meaning in mail, but there are realip module and > > no variables in mail. > > But Auth module can get the variables passed via headers, which is > certainly a usecase, also, to be able to send same proxy protocol header out > as you get in, the proxy-proxy-protocol scenario, it needs to be > stored somewhere. > This will work seemlessly on both mail, http and stream when proxy-protocol is > enabled in both listen and outgoing, think of it as a "transparent > smart-proxy" :) It looks like then only use case you have in mind is nginx between some frontend which adds a PROXY protocol header and a backend which is able to accept such a header. Certainly this is not the only real use case, but just one of multiple possible ones. Other use cases include: - nginx behind some balancer which adds PROXY protocol, and a backend which doesn't understand PROXY protocol behind it; - nginx in front of a backend which understands PROXY protocol, and nothing in front of nginx. Also, every time I see the word "smart" I start thinking about security problems introduced along the way. > > In the stream module similar problem was resolved by not > > introducing "listen ... proxy_protocol" till variables support was > > added, and by adding realip module at the same time. May be there > > are better options. > > > > I certainly dislike what is currently suggested, that is, just > > passing an address provided via PROXY protocol to backends via > > XCLIENT. > > > > Introducing PROXY protocol to backends instead of XCLIENT looks > > as a separate thing. > > I think adding more support for XCLIENT these days is not needed, as the > software in common use today supports proxy protocol native. This doesn't seem to take into account the fact that PROXY protocol can only pass addresses, while XCLIENT is able to provide various other information like a client login, hostname and so on. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
2016-11-10 0:52 GMT+01:00 Maxim Dounin : > Hello! > > On Wed, Nov 09, 2016 at 08:52:14PM +0100, Bjørnar Ness wrote: > >> On Nov 9, 2016 19:20, "Maxim Dounin" wrote: > > The implementation of PROXY protocol in nginx basically mimics > what is available via X-Forwarded-For in http. > > Extending it with destination data may simplify some use cases. > But it will also complicate things and will make other use cases > less intuitive. It is also more or less clear that destination > can be identified using a separate destination within nginx > itself.n It is true that the way proxy protocol support in nginx is currently implemented, it discards half of the information available, and only gives us access to the src part. I think this is strange "halfway" implementation anyway. > As previously said, I'm not convinced this should be merged, as > suggested changes have obvious downsides in common cases and only > beneficial for very specific use cases. When one uses proxy-protocol, one allways has something in front that generates it. Using multiple configurations (can be _lots_) and having to do configuration management/reloads/whatever, seems like going backwards. The overhead of this impl if proxy protocol is not used in listed is just a couple ptr's, and can for sure be reduced at the cost of more complex code. >> > If you have some ideas on how to properly support PROXY protocol >> > in mail - please comment on the relevant patch. >> >> Reason for mentioning it here, is the mail proxy-protocol patch has the >> functionality mentioned here as a dependency. Both exim and dovecot >> understands proxy-protocol, and I want to pass it on after the auth is done, >> hence proxy-proxy-protocol. > > Current question is: > > What "listen ... proxy_protocol" should mean in case of mail. In > other modules, it just means that PROXY protocol header is parsed > and appropriate variables are available for use. It would be good > to have similar meaning in mail, but there are realip module and > no variables in mail. But Auth module can get the variables passed via headers, which is certainly a usecase, also, to be able to send same proxy protocol header out as you get in, the proxy-proxy-protocol scenario, it needs to be stored somewhere. This will work seemlessly on both mail, http and stream when proxy-protocol is enabled in both listen and outgoing, think of it as a "transparent smart-proxy" :) > In the stream module similar problem was resolved by not > introducing "listen ... proxy_protocol" till variables support was > added, and by adding realip module at the same time. May be there > are better options. > > I certainly dislike what is currently suggested, that is, just > passing an address provided via PROXY protocol to backends via > XCLIENT. > > Introducing PROXY protocol to backends instead of XCLIENT looks > as a separate thing. I think adding more support for XCLIENT these days is not needed, as the software in common use today supports proxy protocol native. -- Bj(/)rnar ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Hello! On Wed, Nov 09, 2016 at 08:52:14PM +0100, Bjørnar Ness wrote: > On Nov 9, 2016 19:20, "Maxim Dounin" wrote: > > > > Hello! > > > > On Thu, Nov 03, 2016 at 08:37:04PM +0100, Bjørnar Ness wrote: > > > > > Maxim: what needs change to get this merged? Followup will be mail pp > > > support, which I saw a patch for today from somone else. > > > > If this was a question to me, then the answer is: > > > > I'm not convinced this should be merged. For the use case of > > PROXY protocol bypass a more logical solution would be to avoid > > removing the PROXY protocol header at all. If one needs to bypass > > it and extract the information at the same time, something similar > > to ssl preread as recently implemented in the stream module might > > be a better solution. > > The usecase for getting access to the dst variables is that we have the: > > external LB -> nginx -> foo > > setup as mentioned earlier, and we want to take decisions based on the > destination inside nginx, this may for example be a multi-brand setup, where > the original destination address is a part of the query key to a database. > This will allow us to just do database changes, and nginx will immediately > be able to "see" for what destination this request was for. > > The separate, but related usecase is the "proxy-proxy-protocol" usecase, > where the upstream also needs the original ip/port src/destination data for > logging or decisionmaking. > > For this reason, I think it is practical to store the proxy protocol header as > a whole with ptr's in the connection struct, but I am fine with changing this > to inet_addr for example, will be more loc but a little less memory use. The implementation of PROXY protocol in nginx basically mimics what is available via X-Forwarded-For in http. Extending it with destination data may simplify some use cases. But it will also complicate things and will make other use cases less intuitive. It is also more or less clear that destination can be identified using a separate destination within nginx itself. As previously said, I'm not convinced this should be merged, as suggested changes have obvious downsides in common cases and only beneficial for very specific use cases. > > If you have some ideas on how to properly support PROXY protocol > > in mail - please comment on the relevant patch. > > Reason for mentioning it here, is the mail proxy-protocol patch has the > functionality mentioned here as a dependency. Both exim and dovecot > understands proxy-protocol, and I want to pass it on after the auth is done, > hence proxy-proxy-protocol. Current question is: What "listen ... proxy_protocol" should mean in case of mail. In other modules, it just means that PROXY protocol header is parsed and appropriate variables are available for use. It would be good to have similar meaning in mail, but there are realip module and no variables in mail. In the stream module similar problem was resolved by not introducing "listen ... proxy_protocol" till variables support was added, and by adding realip module at the same time. May be there are better options. I certainly dislike what is currently suggested, that is, just passing an address provided via PROXY protocol to backends via XCLIENT. Introducing PROXY protocol to backends instead of XCLIENT looks as a separate thing. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
On Nov 9, 2016 19:20, "Maxim Dounin" wrote: > > Hello! > > On Thu, Nov 03, 2016 at 08:37:04PM +0100, Bjørnar Ness wrote: > > > Maxim: what needs change to get this merged? Followup will be mail pp > > support, which I saw a patch for today from somone else. > > If this was a question to me, then the answer is: > > I'm not convinced this should be merged. For the use case of > PROXY protocol bypass a more logical solution would be to avoid > removing the PROXY protocol header at all. If one needs to bypass > it and extract the information at the same time, something similar > to ssl preread as recently implemented in the stream module might > be a better solution. The usecase for getting access to the dst variables is that we have the: external LB -> nginx -> foo setup as mentioned earlier, and we want to take decisions based on the destination inside nginx, this may for example be a multi-brand setup, where the original destination address is a part of the query key to a database. This will allow us to just do database changes, and nginx will immediately be able to "see" for what destination this request was for. The separate, but related usecase is the "proxy-proxy-protocol" usecase, where the upstream also needs the original ip/port src/destination data for logging or decisionmaking. For this reason, I think it is practical to store the proxy protocol header as a whole with ptr's in the connection struct, but I am fine with changing this to inet_addr for example, will be more loc but a little less memory use. > If you have some ideas on how to properly support PROXY protocol > in mail - please comment on the relevant patch. Reason for mentioning it here, is the mail proxy-protocol patch has the functionality mentioned here as a dependency. Both exim and dovecot understands proxy-protocol, and I want to pass it on after the auth is done, hence proxy-proxy-protocol. Regards, -- Bjørnar ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Hello! On Thu, Nov 03, 2016 at 08:37:04PM +0100, Bjørnar Ness wrote: > Maxim: what needs change to get this merged? Followup will be mail pp > support, which I saw a patch for today from somone else. If this was a question to me, then the answer is: I'm not convinced this should be merged. For the use case of PROXY protocol bypass a more logical solution would be to avoid removing the PROXY protocol header at all. If one needs to bypass it and extract the information at the same time, something similar to ssl preread as recently implemented in the stream module might be a better solution. If you have some ideas on how to properly support PROXY protocol in mail - please comment on the relevant patch. -- Maxim Dounin http://nginx.org/ ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Maxim: what needs change to get this merged? Followup will be mail pp support, which I saw a patch for today from somone else. ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
2016-09-20 17:18 GMT+02:00 Dmitry Volyntsev : > On 20.09.2016 15:50, Bjørnar Ness wrote: >> [ ... ] >> Also, I want access to the _dst part of the proxy protocol to make >> decisions based on the original destination address, which is >> currently unavailable. > > Why do you need to know the destination address? could you elaborate > what do you want to know on the "(something_with_proxy_protocol_support)" > side? $proxy_protocol_dst_addr is the original destination of the request. This is useful to be able to provide dynamic responses based on destination address, or in cases where the lookup key for a decision is a function on the original destination address. Say lookup-key in for example a database is $proxy_protocol_dst_addr/$http_host > If you have a fixed set of LBs you could configure a separate listen > in nginx configuration per each LB. This is true, but requires duplicated configurations and configuration generation/ reloads for a scaleup/down -- Bj(/)rnar ___ nginx-devel mailing list nginx-devel@nginx.org http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
On 20.09.2016 15:50, Bjørnar Ness wrote:
2016-09-20 13:16 GMT+02:00 Dmitry Volyntsev :
Could you please clarify what a problem are you trying to solve? Any real
world scenario?
Hello, Dmitry, thanks for responding.
The first problem I am trying to solve is the case where we have:
LB -> nginx_proxy -> (something_with_proxy_protocol_support)
Here, if proxy_protocol is enabled on both listen and outgoing, it
is logical to assume the user wants the incoming header passed
on directly.
Transparent proxy protocol bypass sounds like a valid use case.
Also, I want access to the _dst part of the proxy protocol to make
decisions based on the original destination address, which is
currently unavailable.
Why do you need to know the destination address? could you elaborate
what do you want to know on the
"(something_with_proxy_protocol_support)" side?
If you have a fixed set of LBs you could configure a separate listen
in nginx configuration per each LB.
I chose to expose both $proxy_protocol_(src|dst)_(addr|port) and keep
backwards compability with the original $proxy_protocol_(addr|port),
which I suggest removing since its naming is confusing wrt src/dst.
Storing these as ngx_str_t can definately be discussed, atleast if
you plan/want proxyprotocol v2 support. In that case it would
perhaps make sense to use something like:
struct proxy_protocol_data {
int socket_type;
struct sockaddr src;
struct sockaddr dst;
}
and in ngx_connection_t
struct proxy_protocol_data *proxy_protocol;
What do you think? I feel storing as strings makes sense until the need
for v2 arises, also, it makes the proxy-proxy-protocol support simple.
http://nginx.org/en/docs/contributing_changes.html
Try to make it clear why the suggested change is needed, and provide a use
case, if possible.
Thanks, I will keep to this standard in future updates.
--
Bj(/)rnar
___
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] proxy-protocol dst variables and proxy-proxy-protocol
2016-09-20 13:16 GMT+02:00 Dmitry Volyntsev :
> Could you please clarify what a problem are you trying to solve? Any real
> world scenario?
Hello, Dmitry, thanks for responding.
The first problem I am trying to solve is the case where we have:
LB -> nginx_proxy -> (something_with_proxy_protocol_support)
Here, if proxy_protocol is enabled on both listen and outgoing, it
is logical to assume the user wants the incoming header passed
on directly.
Also, I want access to the _dst part of the proxy protocol to make
decisions based on the original destination address, which is
currently unavailable.
I chose to expose both $proxy_protocol_(src|dst)_(addr|port) and keep
backwards compability with the original $proxy_protocol_(addr|port),
which I suggest removing since its naming is confusing wrt src/dst.
Storing these as ngx_str_t can definately be discussed, atleast if
you plan/want proxyprotocol v2 support. In that case it would
perhaps make sense to use something like:
struct proxy_protocol_data {
int socket_type;
struct sockaddr src;
struct sockaddr dst;
}
and in ngx_connection_t
struct proxy_protocol_data *proxy_protocol;
What do you think? I feel storing as strings makes sense until the need
for v2 arises, also, it makes the proxy-proxy-protocol support simple.
> http://nginx.org/en/docs/contributing_changes.html
>> Try to make it clear why the suggested change is needed, and provide a use
>> case, if possible.
Thanks, I will keep to this standard in future updates.
--
Bj(/)rnar
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Could you please clarify what a problem are you trying to solve? Any
real world scenario?
http://nginx.org/en/docs/contributing_changes.html
> Try to make it clear why the suggested change is needed, and provide
a use case, if possible.
On 18.09.2016 15:11, Bjørnar Ness wrote:
Introduce proxy_protocol_src/dst_addr/port and store proxy_protocol
header in ngx_connection struct. This enables proxy-proxy-protocol
in stream module if proxy_protocol is enabled on both listen and
outgoing, which should be the logical default.
Work in progress if/when this patch is accepted is proxy protocol
support for the mail module.
---
src/core/ngx_connection.h | 7 +-
src/core/ngx_proxy_protocol.c | 111 --
src/http/modules/ngx_http_realip_module.c | 6 +-
src/http/ngx_http_variables.c | 60
src/stream/ngx_stream_realip_module.c | 10 +--
src/stream/ngx_stream_variables.c | 60
6 files changed, 132 insertions(+), 122 deletions(-)
diff --git a/src/core/ngx_connection.h b/src/core/ngx_connection.h
index e484c81..c6e2337 100644
--- a/src/core/ngx_connection.h
+++ b/src/core/ngx_connection.h
@@ -148,8 +148,11 @@ struct ngx_connection_s {
socklen_t socklen;
ngx_str_t addr_text;
-ngx_str_t proxy_protocol_addr;
-in_port_t proxy_protocol_port;
+ngx_str_t proxy_protocol;
+ngx_str_t proxy_protocol_src_addr;
+ngx_str_t proxy_protocol_src_port;
+ngx_str_t proxy_protocol_dst_addr;
+ngx_str_t proxy_protocol_dst_port;
#if (NGX_SSL)
ngx_ssl_connection_t *ssl;
diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
index 523ec35..04dae8b 100644
--- a/src/core/ngx_proxy_protocol.c
+++ b/src/core/ngx_proxy_protocol.c
@@ -13,8 +13,9 @@ u_char *
ngx_proxy_protocol_read(ngx_connection_t *c, u_char *buf, u_char *last)
{
size_t len;
-u_char ch, *p, *addr, *port;
-ngx_int_t n;
+ngx_uint_t i, n;
+u_char ch, *p, *pp;
+ngx_str_t addr[2], port[2];
p = buf;
len = last - buf;
@@ -40,77 +41,79 @@ ngx_proxy_protocol_read(ngx_connection_t *c, u_char *buf,
u_char *last)
}
p += 5;
-addr = p;
-for ( ;; ) {
-if (p == last) {
-goto invalid;
-}
+for (i=0;i<2;i++) {
+ addr[i].data = p;
-ch = *p++;
+for ( ;; ) {
+if (p == last) {
+goto invalid;
+}
-if (ch == ' ') {
-break;
-}
+ch = *p++;
-if (ch != ':' && ch != '.'
-&& (ch < 'a' || ch > 'f')
-&& (ch < 'A' || ch > 'F')
-&& (ch < '0' || ch > '9'))
-{
-goto invalid;
-}
-}
+if (ch == ' ') {
+break;
+}
-len = p - addr - 1;
-c->proxy_protocol_addr.data = ngx_pnalloc(c->pool, len);
+if (ch != ':' && ch != '.'
+&& (ch < 'a' || ch > 'f')
+&& (ch < 'A' || ch > 'F')
+&& (ch < '0' || ch > '9'))
+{
+goto invalid;
+}
+}
-if (c->proxy_protocol_addr.data == NULL) {
-return NULL;
+addr[i].len = p - addr[i].data - 1;
}
-ngx_memcpy(c->proxy_protocol_addr.data, addr, len);
-c->proxy_protocol_addr.len = len;
+for (i=0;i<2;i++) {
+port[i].data = p;
-for ( ;; ) {
-if (p == last) {
-goto invalid;
-}
+for ( ;; ) {
+if (p == last) {
+goto invalid;
+}
-if (*p++ == ' ') {
-break;
+if ((*p++ == ' ' && i == 0) || (i == 1 && p != last && *p == CR)) {
+break;
+}
}
-}
-port = p;
+port[i].len = p - port[i].data - (1-i);
+n = ngx_atoi(port[i].data, port[i].len);
-for ( ;; ) {
-if (p == last) {
+if (n < 1 || n > 65535) {
goto invalid;
}
-
-if (*p++ == ' ') {
-break;
-}
-}
-
-len = p - port - 1;
-
-n = ngx_atoi(port, len);
-
-if (n < 0 || n > 65535) {
-goto invalid;
}
-c->proxy_protocol_port = (in_port_t) n;
-
-ngx_log_debug2(NGX_LOG_DEBUG_CORE, c->log, 0,
- "PROXY protocol address: %V %i", &c->proxy_protocol_addr,
n);
-
skip:
for ( /* void */ ; p < last - 1; p++) {
if (p[0] == CR && p[1] == LF) {
+len = p - buf - 6;
+c->proxy_protocol.data = ngx_pnalloc(c->pool, len);
+ngx_memcpy(c->proxy_protocol.data, buf + 6, len);
+c->proxy_protocol.len = len;
+
+if (i) { /* not UNKNOWN */
+pp = c->proxy_protocol.data;
+c->proxy_protocol_src_addr.data = pp + (addr[0].data
Re: [PATCH] proxy-protocol dst variables and proxy-proxy-protocol
Came to my attention that i can be used uninitialized ((thanks, Johnny), so:
diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
index 04dae8b..21c710a 100644
--- a/src/core/ngx_proxy_protocol.c
+++ b/src/core/ngx_proxy_protocol.c
@@ -13,7 +13,7 @@ u_char *
ngx_proxy_protocol_read(ngx_connection_t *c, u_char *buf, u_char *last)
{
size_t len;
-ngx_uint_t i, n;
+ngx_uint_t i=0, n;
u_char ch, *p, *pp;
ngx_str_t addr[2], port[2];
___
nginx-devel mailing list
nginx-devel@nginx.org
http://mailman.nginx.org/mailman/listinfo/nginx-devel
