Hello!

On Tue, Sep 27, 2022 at 01:41:25PM +0400, Roman Arutyunyan wrote:

[...]

> # HG changeset patch
> # User Roman Arutyunyan <arut@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 <arut@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 <arut@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-leave@nginx.org