Hi,

On Tue, Nov 01, 2022 at 05:14:02PM +0300, Maxim Dounin wrote:
> Hello!
>
> On Mon, Oct 31, 2022 at 04:07:00PM +0400, Roman Arutyunyan wrote:
>
> > While testing the feature, it became clear that 107 bytes as maximum PROXY
> > protocol header size may not be enough. This limit came from v1 and stayed
> > unchanged when v2 was added. With this limit, there are only 79 bytes left for
> > TLVs in case of IPv4 and 55 bytes in case of IPv6.
> >
> > Attached is a patch that increases buffer size up to 65K while reading PROXY
> > protocl header. Writing is not changed since only v1 is supported.
>
> [...]
>
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1667216033 -14400
> > # Mon Oct 31 15:33:53 2022 +0400
> > # Node ID 8c99314f90eccc2ad5aaf4b3de5368e964c4ffe0
> > # Parent 81b4326daac70d6de70abbc3fe36d4f6e3da54a2
> > Increased maximum read PROXY protocol header size.
> >
> > Maximum size for reading the PROXY protocol header is increased to 65536 to
> > accommodate a bigger number of TLVs, which are supported since cca4c8a715de.
> >
> > Maximum size for writing the PROXY protocol header is not changed since only
> > version 1 is currently supported.
> >
> > diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
> > --- a/src/core/ngx_proxy_protocol.c
> > +++ b/src/core/ngx_proxy_protocol.c
> > @@ -281,7 +281,7 @@ ngx_proxy_protocol_write(ngx_connection_
> > {
> > ngx_uint_t port, lport;
> >
> > - if (last - buf < NGX_PROXY_PROTOCOL_MAX_HEADER) {
> > + if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
> > return NULL;
> > }
> >
> > diff --git a/src/core/ngx_proxy_protocol.h b/src/core/ngx_proxy_protocol.h
> > --- a/src/core/ngx_proxy_protocol.h
> > +++ b/src/core/ngx_proxy_protocol.h
> > @@ -13,7 +13,8 @@
> > #include <ngx_core.h>
> >
> >
> > -#define NGX_PROXY_PROTOCOL_MAX_HEADER 107
> > +#define NGX_PROXY_PROTOCOL_V1_MAX_HEADER 107
> > +#define NGX_PROXY_PROTOCOL_MAX_HEADER 65536
> >
> >
> > struct ngx_proxy_protocol_s {
> > diff --git a/src/http/ngx_http_request.c b/src/http/ngx_http_request.c
> > --- a/src/http/ngx_http_request.c
> > +++ b/src/http/ngx_http_request.c
> > @@ -641,7 +641,7 @@ ngx_http_alloc_request(ngx_connection_t
> > static void
> > ngx_http_ssl_handshake(ngx_event_t *rev)
> > {
> > - u_char *p, buf[NGX_PROXY_PROTOCOL_MAX_HEADER + 1];
> > + u_char *p;
> > size_t size;
> > ssize_t n;
> > ngx_err_t err;
> > @@ -651,6 +651,7 @@ ngx_http_ssl_handshake(ngx_event_t *rev)
> > ngx_http_ssl_srv_conf_t *sscf;
> > ngx_http_core_loc_conf_t *clcf;
> > ngx_http_core_srv_conf_t *cscf;
> > + static u_char buf[NGX_PROXY_PROTOCOL_MAX_HEADER + 1];
> >
>
> I'm somewhat sceptical about the idea of allocation of up to 3
> global 64k buffers, especially given that the protocol requires
> atomic sending of the header, which means it cannot reliably
> exceed 1500 bytes in most configurations.
>
> Further, the maximum size of the proxy protocol header in non-SSL
> case is still limited by client_header_buffer_size, which is 1024
> bytes by default. Assuming there will be headers which does not
> fit into 1k buffer, it is not clear why these will magically work
> in case of SSL, but will require client_header_buffer_size tuning
> otherwise.

It will require tuning in some cases and work automatically in others.
Currently it only works where tuning is available.

> Might be some dynamically allocated buffer, sized after
> client_header_buffer_size, like in non-SSL case, would be a better
> idea?

We read PROXY protocol in mail and stream as well, which would require
similar settings there.

> (It also might make sense to avoid assuming atomicity, which
> clearly cannot be guaranteed if the header is larger than MTU.
> This will also require dynamically allocated per-connection
> buffers.)

Yes, dynamic per-connection buffers is what I've been trying to avoid.

> Alternatively, we can consider using some larger-than-now on-stack
> buffers, something like 4k should be acceptable given we use such
> buffers in other places anyway. It should be enough for most
> proxy protocol headers.

I like this alternative. This was in fact my first thought, but the actual
number was (and still is) hard to pick. I suggest that we increase the
on-stack buffer for reading and leave the writing buffer size as is for now.
The writing part will need refactoring anyway when we add ppv2 support.

--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1667382376 -14400
# Wed Nov 02 13:46:16 2022 +0400
# Node ID dc5f16e6a243c15f58e2c6a62f7a83f536729174
# Parent 81b4326daac70d6de70abbc3fe36d4f6e3da54a2
Increased maximum read PROXY protocol header size.

Maximum size for reading the PROXY protocol header is increased to 4096 to
accommodate a bigger number of TLVs, which are supported since cca4c8a715de.

Maximum size for writing the PROXY protocol header is not changed since only
version 1 is currently supported.

diff --git a/src/core/ngx_proxy_protocol.c b/src/core/ngx_proxy_protocol.c
--- a/src/core/ngx_proxy_protocol.c
+++ b/src/core/ngx_proxy_protocol.c
@@ -281,7 +281,7 @@ ngx_proxy_protocol_write(ngx_connection_
{
ngx_uint_t port, lport;

- if (last - buf < NGX_PROXY_PROTOCOL_MAX_HEADER) {
+ if (last - buf < NGX_PROXY_PROTOCOL_V1_MAX_HEADER) {
return NULL;
}

diff --git a/src/core/ngx_proxy_protocol.h b/src/core/ngx_proxy_protocol.h
--- a/src/core/ngx_proxy_protocol.h
+++ b/src/core/ngx_proxy_protocol.h
@@ -13,7 +13,8 @@
#include <ngx_core.h>


-#define NGX_PROXY_PROTOCOL_MAX_HEADER 107
+#define NGX_PROXY_PROTOCOL_V1_MAX_HEADER 107
+#define NGX_PROXY_PROTOCOL_MAX_HEADER 4096


struct ngx_proxy_protocol_s {
diff --git a/src/mail/ngx_mail_proxy_module.c b/src/mail/ngx_mail_proxy_module.c
--- a/src/mail/ngx_mail_proxy_module.c
+++ b/src/mail/ngx_mail_proxy_module.c
@@ -890,7 +890,7 @@ ngx_mail_proxy_send_proxy_protocol(ngx_m
u_char *p;
ssize_t n, size;
ngx_connection_t *c;
- u_char buf[NGX_PROXY_PROTOCOL_MAX_HEADER];
+ u_char buf[NGX_PROXY_PROTOCOL_V1_MAX_HEADER];

s->connection->log->action = "sending PROXY protocol header to upstream";

@@ -898,7 +898,7 @@ ngx_mail_proxy_send_proxy_protocol(ngx_m
"mail proxy send PROXY protocol header");

p = ngx_proxy_protocol_write(s->connection, buf,
- buf + NGX_PROXY_PROTOCOL_MAX_HEADER);
+ buf + NGX_PROXY_PROTOCOL_V1_MAX_HEADER);
if (p == NULL) {
ngx_mail_proxy_internal_server_error(s);
return NGX_ERROR;
diff --git a/src/stream/ngx_stream_proxy_module.c b/src/stream/ngx_stream_proxy_module.c
--- a/src/stream/ngx_stream_proxy_module.c
+++ b/src/stream/ngx_stream_proxy_module.c
@@ -894,7 +894,7 @@ ngx_stream_proxy_init_upstream(ngx_strea
return;
}

- p = ngx_pnalloc(c->pool, NGX_PROXY_PROTOCOL_MAX_HEADER);
+ p = ngx_pnalloc(c->pool, NGX_PROXY_PROTOCOL_V1_MAX_HEADER);
if (p == NULL) {
ngx_stream_proxy_finalize(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
return;
@@ -902,7 +902,8 @@ ngx_stream_proxy_init_upstream(ngx_strea

cl->buf->pos = p;

- p = ngx_proxy_protocol_write(c, p, p + NGX_PROXY_PROTOCOL_MAX_HEADER);
+ p = ngx_proxy_protocol_write(c, p,
+ p + NGX_PROXY_PROTOCOL_V1_MAX_HEADER);
if (p == NULL) {
ngx_stream_proxy_finalize(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
return;
@@ -946,14 +947,15 @@ ngx_stream_proxy_send_proxy_protocol(ngx
ngx_connection_t *c, *pc;
ngx_stream_upstream_t *u;
ngx_stream_proxy_srv_conf_t *pscf;
- u_char buf[NGX_PROXY_PROTOCOL_MAX_HEADER];
+ u_char buf[NGX_PROXY_PROTOCOL_V1_MAX_HEADER];

c = s->connection;

ngx_log_debug0(NGX_LOG_DEBUG_STREAM, c->log, 0,
"stream proxy send PROXY protocol header");

- p = ngx_proxy_protocol_write(c, buf, buf + NGX_PROXY_PROTOCOL_MAX_HEADER);
+ p = ngx_proxy_protocol_write(c, buf,
+ buf + NGX_PROXY_PROTOCOL_V1_MAX_HEADER);
if (p == NULL) {
ngx_stream_proxy_finalize(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
return NGX_ERROR;
_______________________________________________
nginx-devel mailing list -- nginx-devel@nginx.org
To unsubscribe send an email to nginx-devel-leave@nginx.org