[PATCH] Core: support for reading PROXY protocol v2 TLVs

Maxim Dounin mdounin at mdounin.ru
Tue Oct 11 01:20:52 UTC 2022


Hello!

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

[...]

> # HG changeset patch
> # User Roman Arutyunyan <arut at 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 at 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 at 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/



More information about the nginx-devel mailing list