Hello! On Tue, Sep 27, 2022 at 01:41:25PM +0400, Roman Arutyunyan wrote:
[...] > # HG changeset patch > # User Roman Arutyunyan <a...@nginx.com> > # Date 1664263604 -14400 > # Tue Sep 27 11:26:44 2022 +0400 > # Node ID 38940ff7246574aa19a19c76b072073c34f191be > # Parent ba5cf8f73a2d0a3615565bf9545f3d65216a0530 > PROXY protocol v2 TLV variables. > > The variables have prefix $proxy_protocol_tlv_ and are accessible by name > and by type. Examples are: $proxy_protocol_tlv_0x01, > $proxy_protocol_tlv_alpn. > > 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 > @@ -15,6 +15,12 @@ > > #define ngx_proxy_protocol_parse_uint16(p) ((p)[0] << 8 | (p)[1]) > > +#define ngx_proxy_protocol_parse_uint32(p) > \ > + ( ((uint32_t) (p)[0] << 24) > \ > + + ( (p)[1] << 16) > \ > + + ( (p)[2] << 8) > \ > + + ( (p)[3]) ) > + > > typedef struct { > u_char signature[12]; > @@ -40,6 +46,24 @@ typedef struct { > } ngx_proxy_protocol_inet6_addrs_t; > > > +typedef struct { > + u_char type; > + u_char len[2]; > +} ngx_proxy_protocol_tlv_t; > + > + > +typedef struct { > + u_char client; > + u_char verify[4]; > +} ngx_proxy_protocol_tlv_ssl_t; > + > + > +typedef struct { > + ngx_str_t name; > + ngx_uint_t type; > +} ngx_proxy_protocol_tlv_entry_t; > + > + > static u_char *ngx_proxy_protocol_read_addr(ngx_connection_t *c, u_char *p, > u_char *last, ngx_str_t *addr); > static u_char *ngx_proxy_protocol_read_port(u_char *p, u_char *last, > @@ -48,6 +72,26 @@ static u_char *ngx_proxy_protocol_v2_rea > u_char *last); > > > +static ngx_proxy_protocol_tlv_entry_t ngx_proxy_protocol_tlv_entries[] = { > + { ngx_string("alpn"), 0x01 }, > + { ngx_string("authority"), 0x02 }, > + { ngx_string("unique_id"), 0x05 }, > + { ngx_string("ssl"), 0x20 }, > + { ngx_string("netns"), 0x30 }, > + { ngx_null_string, 0x00 } > +}; > + > + > +static ngx_proxy_protocol_tlv_entry_t ngx_proxy_protocol_tlv_ssl_entries[] > = { > + { ngx_string("version"), 0x21 }, > + { ngx_string("cn"), 0x22 }, > + { ngx_string("cipher"), 0x23 }, > + { ngx_string("sig_alg"), 0x24 }, > + { ngx_string("key_alg"), 0x25 }, > + { ngx_null_string, 0x00 } > +}; > + > + > u_char * > ngx_proxy_protocol_read(ngx_connection_t *c, u_char *buf, u_char *last) > { > @@ -412,11 +456,145 @@ ngx_proxy_protocol_v2_read(ngx_connectio > &pp->src_addr, pp->src_port, &pp->dst_addr, pp->dst_port); > > if (buf < end) { > - ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0, > - "PROXY protocol v2 %z bytes of tlv ignored", end - > buf); > + pp->tlvs.data = ngx_pnalloc(c->pool, end - buf); > + if (pp->tlvs.data == NULL) { > + return NULL; > + } > + > + ngx_memcpy(pp->tlvs.data, buf, end - buf); > + pp->tlvs.len = end - buf; > } > > c->proxy_protocol = pp; > > return end; > } > + > + > +ngx_int_t > +ngx_proxy_protocol_lookup_tlv(ngx_connection_t *c, ngx_str_t *tlvs, > + ngx_uint_t type, ngx_str_t *value) This probably can be made static and moved after ngx_proxy_protocol_get_tlv(). > +{ > + u_char *p; > + size_t n, len; > + ngx_proxy_protocol_tlv_t *tlv; > + > + ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0, > + "PROXY protocol v2 lookup tlv:%02xi", type); > + > + p = tlvs->data; > + n = tlvs->len; > + > + while (n) { > + if (n < sizeof(ngx_proxy_protocol_tlv_t)) { > + ngx_log_error(NGX_LOG_ERR, c->log, 0, "broken PROXY protocol > TLV"); > + return NGX_ERROR; > + } > + > + tlv = (ngx_proxy_protocol_tlv_t *) p; > + len = ngx_proxy_protocol_parse_uint16(tlv->len); > + > + p += sizeof(ngx_proxy_protocol_tlv_t); > + n -= sizeof(ngx_proxy_protocol_tlv_t); > + > + if (n < len) { > + ngx_log_error(NGX_LOG_ERR, c->log, 0, "broken PROXY protocol > TLV"); > + return NGX_ERROR; > + } > + > + ngx_log_debug2(NGX_LOG_DEBUG_CORE, c->log, 0, > + "PROXY protocol v2 tlv:0x%02xd len:%uz", tlv->type, > len); I tend to think this is going to be too chatty on real load with multiple TLVs, and probably should be removed or #if 0'ed. > + > + if (tlv->type == type) { > + value->data = p; > + value->len = len; > + return NGX_OK; > + } > + > + p += len; > + n -= len; > + } > + > + return NGX_DECLINED; > +} > + > + > +ngx_int_t > +ngx_proxy_protocol_get_tlv(ngx_connection_t *c, ngx_str_t *name, > + ngx_str_t *value) > +{ > + u_char *p; > + size_t n; > + uint32_t verify; > + ngx_str_t ssl, *tlvs; > + ngx_int_t rc, type; > + ngx_proxy_protocol_tlv_ssl_t *tlv_ssl; > + ngx_proxy_protocol_tlv_entry_t *te; > + > + if (c->proxy_protocol == NULL) { > + return NGX_DECLINED; > + } > + > + ngx_log_debug1(NGX_LOG_DEBUG_CORE, c->log, 0, > + "PROXY protocol v2 get tlv \"%V\"", name); > + > + te = ngx_proxy_protocol_tlv_entries; > + tlvs = &c->proxy_protocol->tlvs; > + > + p = name->data; > + n = name->len; > + > + if (n >= 4 && p[0] == 's' && p[1] == 's' && p[2] == 'l' && p[3] == '_') { > + > + rc = ngx_proxy_protocol_lookup_tlv(c, tlvs, 0x20, &ssl); > + if (rc != NGX_OK) { > + return rc; > + } > + > + if (ssl.len < sizeof(ngx_proxy_protocol_tlv_ssl_t)) { > + return NGX_ERROR; > + } > + > + p += 4; > + n -= 4; > + > + if (n == 6 && ngx_strncmp(p, "verify", 6) == 0) { > + > + tlv_ssl = (ngx_proxy_protocol_tlv_ssl_t *) ssl.data; > + verify = ngx_proxy_protocol_parse_uint32(tlv_ssl->verify); > + > + value->data = ngx_pnalloc(c->pool, NGX_INT32_LEN); > + if (value->data == NULL) { > + return NGX_ERROR; > + } > + > + value->len = ngx_sprintf(value->data, "%uD", verify) > + - value->data; > + return NGX_OK; > + } > + > + ssl.data += sizeof(ngx_proxy_protocol_tlv_ssl_t); > + ssl.len -= sizeof(ngx_proxy_protocol_tlv_ssl_t); > + > + te = ngx_proxy_protocol_tlv_ssl_entries; > + tlvs = &ssl; > + } > + > + if (n >= 2 && p[0] == '0' && p[1] == 'x') { > + > + type = ngx_hextoi(p + 2, n - 2); > + if (type == NGX_ERROR) { > + return NGX_ERROR; This probably needs some error message. > + } > + > + return ngx_proxy_protocol_lookup_tlv(c, tlvs, type, value); > + } > + > + for ( /* void */ ; te->type; te++) { > + if (te->name.len == n && ngx_strncmp(te->name.data, p, n) == 0) { > + return ngx_proxy_protocol_lookup_tlv(c, tlvs, te->type, value); > + } > + } > + > + return NGX_DECLINED; Invalid/unknown names will silently result in empty variables. I tend to think this is going to be a problem, especially if we'll introduce additional names at some point. Some error instead might be a good idea. > +} > 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 > @@ -21,6 +21,7 @@ struct ngx_proxy_protocol_s { > ngx_str_t dst_addr; > in_port_t src_port; > in_port_t dst_port; > + ngx_str_t tlvs; > }; > > > @@ -28,6 +29,10 @@ u_char *ngx_proxy_protocol_read(ngx_conn > u_char *last); > u_char *ngx_proxy_protocol_write(ngx_connection_t *c, u_char *buf, > u_char *last); > +ngx_int_t ngx_proxy_protocol_lookup_tlv(ngx_connection_t *c, ngx_str_t *tlvs, > + ngx_uint_t type, ngx_str_t *value); > +ngx_int_t ngx_proxy_protocol_get_tlv(ngx_connection_t *c, ngx_str_t *name, > + ngx_str_t *value); > > > #endif /* _NGX_PROXY_PROTOCOL_H_INCLUDED_ */ > diff --git a/src/http/ngx_http_variables.c b/src/http/ngx_http_variables.c > --- a/src/http/ngx_http_variables.c > +++ b/src/http/ngx_http_variables.c > @@ -61,6 +61,8 @@ static ngx_int_t ngx_http_variable_proxy > ngx_http_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_http_variable_proxy_protocol_port(ngx_http_request_t *r, > ngx_http_variable_value_t *v, uintptr_t data); > +static ngx_int_t ngx_http_variable_proxy_protocol_tlv(ngx_http_request_t *r, > + ngx_http_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_http_variable_server_addr(ngx_http_request_t *r, > ngx_http_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_http_variable_server_port(ngx_http_request_t *r, > @@ -214,6 +216,10 @@ static ngx_http_variable_t ngx_http_cor > ngx_http_variable_proxy_protocol_port, > offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 }, > > + { ngx_string("proxy_protocol_tlv_"), NULL, > + ngx_http_variable_proxy_protocol_tlv, > + 0, NGX_HTTP_VAR_PREFIX, 0 }, > + > { ngx_string("server_addr"), NULL, ngx_http_variable_server_addr, 0, 0, > 0 }, > > { ngx_string("server_port"), NULL, ngx_http_variable_server_port, 0, 0, > 0 }, > @@ -1387,6 +1393,39 @@ ngx_http_variable_proxy_protocol_port(ng > > > static ngx_int_t > +ngx_http_variable_proxy_protocol_tlv(ngx_http_request_t *r, > + ngx_http_variable_value_t *v, uintptr_t data) > +{ > + ngx_str_t *name = (ngx_str_t *) data; > + > + ngx_int_t rc; > + ngx_str_t tlv, value; > + > + tlv.len = name->len - (sizeof("proxy_protocol_tlv_") - 1); > + tlv.data = name->data + sizeof("proxy_protocol_tlv_") - 1; > + > + rc = ngx_proxy_protocol_get_tlv(r->connection, &tlv, &value); > + > + if (rc == NGX_ERROR) { > + return NGX_ERROR; > + } > + > + if (rc == NGX_DECLINED) { > + v->not_found = 1; > + return NGX_OK; > + } > + > + v->len = value.len; > + v->valid = 1; > + v->no_cacheable = 0; > + v->not_found = 0; > + v->data = value.data; > + > + return NGX_OK; > +} > + > + > +static ngx_int_t > ngx_http_variable_server_addr(ngx_http_request_t *r, > ngx_http_variable_value_t *v, uintptr_t data) > { > diff --git a/src/stream/ngx_stream_variables.c > b/src/stream/ngx_stream_variables.c > --- a/src/stream/ngx_stream_variables.c > +++ b/src/stream/ngx_stream_variables.c > @@ -23,6 +23,8 @@ static ngx_int_t ngx_stream_variable_pro > ngx_stream_session_t *s, ngx_stream_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_stream_variable_proxy_protocol_port( > ngx_stream_session_t *s, ngx_stream_variable_value_t *v, uintptr_t data); > +static ngx_int_t ngx_stream_variable_proxy_protocol_tlv( > + ngx_stream_session_t *s, ngx_stream_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_stream_variable_server_addr(ngx_stream_session_t *s, > ngx_stream_variable_value_t *v, uintptr_t data); > static ngx_int_t ngx_stream_variable_server_port(ngx_stream_session_t *s, > @@ -79,6 +81,10 @@ static ngx_stream_variable_t ngx_stream > ngx_stream_variable_proxy_protocol_port, > offsetof(ngx_proxy_protocol_t, dst_port), 0, 0 }, > > + { ngx_string("proxy_protocol_tlv_"), NULL, > + ngx_stream_variable_proxy_protocol_tlv, > + 0, NGX_STREAM_VAR_PREFIX, 0 }, > + > { ngx_string("server_addr"), NULL, > ngx_stream_variable_server_addr, 0, 0, 0 }, > > @@ -622,6 +628,39 @@ ngx_stream_variable_proxy_protocol_port( > > > static ngx_int_t > +ngx_stream_variable_proxy_protocol_tlv(ngx_stream_session_t *s, > + ngx_stream_variable_value_t *v, uintptr_t data) > +{ > + ngx_str_t *name = (ngx_str_t *) data; > + > + ngx_int_t rc; > + ngx_str_t tlv, value; > + > + tlv.len = name->len - (sizeof("proxy_protocol_tlv_") - 1); > + tlv.data = name->data + sizeof("proxy_protocol_tlv_") - 1; > + > + rc = ngx_proxy_protocol_get_tlv(s->connection, &tlv, &value); > + > + if (rc == NGX_ERROR) { > + return NGX_ERROR; > + } > + > + if (rc == NGX_DECLINED) { > + v->not_found = 1; > + return NGX_OK; > + } > + > + v->len = value.len; > + v->valid = 1; > + v->no_cacheable = 0; > + v->not_found = 0; > + v->data = value.data; > + > + return NGX_OK; > +} > + > + > +static ngx_int_t > ngx_stream_variable_server_addr(ngx_stream_session_t *s, > ngx_stream_variable_value_t *v, uintptr_t data) > { Otherwise looks good. > # HG changeset patch > # User Roman Arutyunyan <a...@nginx.com> > # Date 1664263876 -14400 > # Tue Sep 27 11:31:16 2022 +0400 > # Node ID 615268a957ab930dc4be49fe5f6f88cd7e377f12 > # Parent 38940ff7246574aa19a19c76b072073c34f191be > Added type cast to ngx_proxy_protocol_parse_uint16(). > > The cast is added to make ngx_proxy_protocol_parse_uint16() similar to > ngx_proxy_protocol_parse_uint32(). > > 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 > @@ -13,7 +13,9 @@ > #define NGX_PROXY_PROTOCOL_AF_INET6 2 > > > -#define ngx_proxy_protocol_parse_uint16(p) ((p)[0] << 8 | (p)[1]) > +#define ngx_proxy_protocol_parse_uint16(p) > \ > + ( ((uint16_t) (p)[0] << 8) > \ > + + ( (p)[1]) ) > > #define ngx_proxy_protocol_parse_uint32(p) > \ > ( ((uint32_t) (p)[0] << 24) > \ Looks good. > # HG changeset patch > # User Roman Arutyunyan <a...@nginx.com> > # Date 1664200613 -14400 > # Mon Sep 26 17:56:53 2022 +0400 > # Node ID ff87ed4999b49433a9abecaaf2e574cbfa502961 > # Parent 95ba1e704b7b29c39447135e18ed003ecd305924 > Tests: client PROXY protocol v2 TLV variables. > > diff --git a/proxy_protocol2.t b/proxy_protocol2.t > --- a/proxy_protocol2.t > +++ b/proxy_protocol2.t > @@ -24,7 +24,7 @@ select STDOUT; $| = 1; > > my $t = Test::Nginx->new()->has(qw/http access realip/); > > -$t->write_file_expand('nginx.conf', <<'EOF')->plan(23); > +$t->write_file_expand('nginx.conf', <<'EOF')->plan(26); > > %%TEST_GLOBALS%% > > @@ -45,6 +45,8 @@ http { > set_real_ip_from 127.0.0.1/32; > add_header X-IP $remote_addr!$remote_port; > add_header X-PP $proxy_protocol_addr!$proxy_protocol_port; > + add_header X-TL > $proxy_protocol_tlv_0x3-$proxy_protocol_tlv_0x0000ae-$proxy_protocol_tlv_0x0f; > + add_header X-NT > $proxy_protocol_tlv_unique_id-$proxy_protocol_tlv_ssl_cn-$proxy_protocol_tlv_ssl_0x22-$proxy_protocol_tlv_ssl_verify; > > location /pp { > real_ip_header proxy_protocol; > @@ -76,7 +78,11 @@ my $p = pack("N3C", 0x0D0A0D0A, 0x000D0A > my $tcp4 = $p . pack("CnN2n2", 0x11, 12, 0xc0000201, 0xc0000202, 123, 5678); > my $tcp6 = $p . pack("CnNx8NNx8Nn2", 0x21, 36, > 0x20010db8, 0x00000001, 0x20010db8, 0x00000002, 123, 5678); > -my $tlv = $p . pack("CnN2n2x9", 0x11, 21, 0xc0000201, 0xc0000202, 123, 5678); > +my $tlv = $p . pack("CnN2n2N3", 0x11, 24, 0xc0000201, 0xc0000202, 123, 5678, > + 0x03000141, 0xAE000531, 0x32333435); > +my $tlv2 = $p . pack("CnN2n2N7", 0x11, 40, 0xc0000201, 0xc0000202, 123, 5678, > + 0x05000555, 0x4E495151, > + 0x20001100, 0xdeadbeef, 0x22000966, 0x6f6f2e62, 0x61727272); > my $unk1 = $p . pack("Cxx", 0x01); > my $unk2 = $p . pack("CnC4", 0x41, 4, 1, 2, 3, 4); > my $r; > @@ -97,6 +103,11 @@ unlike($r, qr/X-IP: (2001:DB8::1|[^!]+!1 > like($r, qr/SEE-THIS/, 'tlv request'); > like($r, qr/X-PP: 192.0.2.1!123\x0d/, 'tlv proxy'); > unlike($r, qr/X-IP: (192.0.2.1|[^!]+!123\x0d)/, 'tlv client'); > +like($r, qr/X-TL: A-12345-\x0d/, 'tlv raw variables'); > +like($r, qr/X-NT: ---\x0d/, 'tlv missing variables'); > + > +$r = pp_get('/t1', $tlv2); > +like($r, qr/X-NT: UNIQQ-foo.barrr-foo.barrr-3735928559\x0d/, 'tlv named > variables'); > > $r = pp_get('/t1', $unk1); > like($r, qr/SEE-THIS/, 'unknown request 1'); This is going to fail without the patch and needs either try_run() or a separate test file with try_run(). A separate test file might be better, since it'll preserve relevant test coverage for stable branch. -- Maxim Dounin http://mdounin.ru/ _______________________________________________ nginx-devel mailing list -- nginx-devel@nginx.org To unsubscribe send an email to nginx-devel-le...@nginx.org