Hi,

On Wed, Feb 28, 2024 at 02:15:40PM +0400, Sergey Kandaurov wrote:
> On Wed, Feb 21, 2024 at 05:37:51PM +0400, Roman Arutyunyan wrote:
> > Hi,
> >
> > On Tue, Feb 13, 2024 at 02:46:35PM +0400, Sergey Kandaurov wrote:
> > >
> > > > On 10 Nov 2023, at 14:07, Roman Arutyunyan <arut@nginx.com> wrote:
> > > >
> > > > # HG changeset patch
> > > > # User Roman Arutyunyan <arut@nginx.com>
> > > > # Date 1699543504 -14400
> > > > # Thu Nov 09 19:25:04 2023 +0400
> > > > # Node ID 3cab85fe55272835674b7f1c296796955256d019
> > > > # Parent 1d3464283405a4d8ac54caae9bf1815c723f04c5
> > > > Stream: ngx_stream_pass_module.
> > > >
> > > > The module allows to pass connections from Stream to other modules such as HTTP
> > > > or Mail, as well as back to Stream. Previously, this was only possible with
> > > > proxying. Connections with preread buffer read out from socket cannot be
> > > > passed.
> > > >
> > > > The module allows to terminate SSL selectively based on SNI.
> > > >
> > > > stream {
> > > > server {
> > > > listen 8000 default_server;
> > > > ssl_preread on;
> > > > ...
> > > > }
> > > >
> > > > server {
> > > > listen 8000;
> > > > server_name foo.example.com;
> > > > pass 8001; # to HTTP
> > > > }
> > > >
> > > > server {
> > > > listen 8000;
> > > > server_name bar.example.com;
> > > > ...
> > > > }
> > > > }
> > > >
> > > > http {
> > > > server {
> > > > listen 8001 ssl;
> > > > ...
> > > >
> > > > location / {
> > > > root html;
> > > > }
> > > > }
> > > > }
> > > >
> > > > diff --git a/auto/modules b/auto/modules
> > > > --- a/auto/modules
> > > > +++ b/auto/modules
> > > > @@ -1166,6 +1166,16 @@ if [ $STREAM != NO ]; then
> > > > . auto/module
> > > > fi
> > > >
> > > > + if [ $STREAM_PASS = YES ]; then
> > > > + ngx_module_name=ngx_stream_pass_module
> > > > + ngx_module_deps=
> > > > + ngx_module_srcs=src/stream/ngx_stream_pass_module.c
> > > > + ngx_module_libs=
> > > > + ngx_module_link=$STREAM_PASS
> > > > +
> > > > + . auto/module
> > > > + fi
> > > > +
> > > > if [ $STREAM_SET = YES ]; then
> > > > ngx_module_name=ngx_stream_set_module
> > > > ngx_module_deps=
> > > > diff --git a/auto/options b/auto/options
> > > > --- a/auto/options
> > > > +++ b/auto/options
> > > > @@ -127,6 +127,7 @@ STREAM_GEOIP=NO
> > > > STREAM_MAP=YES
> > > > STREAM_SPLIT_CLIENTS=YES
> > > > STREAM_RETURN=YES
> > > > +STREAM_PASS=YES
> > > > STREAM_SET=YES
> > > > STREAM_UPSTREAM_HASH=YES
> > > > STREAM_UPSTREAM_LEAST_CONN=YES
> > > > @@ -337,6 +338,7 @@ use the \"--with-mail_ssl_module\" optio
> > > > --without-stream_split_clients_module)
> > > > STREAM_SPLIT_CLIENTS=NO ;;
> > > > --without-stream_return_module) STREAM_RETURN=NO ;;
> > > > + --without-stream_pass_module) STREAM_PASS=NO ;;
> > > > --without-stream_set_module) STREAM_SET=NO ;;
> > > > --without-stream_upstream_hash_module)
> > > > STREAM_UPSTREAM_HASH=NO ;;
> > > > @@ -556,6 +558,7 @@ cat << END
> > > > --without-stream_split_clients_module
> > > > disable ngx_stream_split_clients_module
> > > > --without-stream_return_module disable ngx_stream_return_module
> > > > + --without-stream_pass_module disable ngx_stream_pass_module
> > > > --without-stream_set_module disable ngx_stream_set_module
> > > > --without-stream_upstream_hash_module
> > > > disable ngx_stream_upstream_hash_module
> > > > diff --git a/src/stream/ngx_stream_pass_module.c b/src/stream/ngx_stream_pass_module.c
> > > > new file mode 100644
> > > > --- /dev/null
> > > > +++ b/src/stream/ngx_stream_pass_module.c
> > > > @@ -0,0 +1,245 @@
> > > > +
> > > > +/*
> > > > + * Copyright (C) Roman Arutyunyan
> > > > + * Copyright (C) Nginx, Inc.
> > > > + */
> > > > +
> > > > +
> > > > +#include <ngx_config.h>
> > > > +#include <ngx_core.h>
> > > > +#include <ngx_stream.h>
> > > > +
> > > > +
> > > > +typedef struct {
> > > > + ngx_addr_t *addr;
> > > > + ngx_stream_complex_value_t *addr_value;
> > > > +} ngx_stream_pass_srv_conf_t;
> > > > +
> > > > +
> > > > +static void ngx_stream_pass_handler(ngx_stream_session_t *s);
> > > > +static void *ngx_stream_pass_create_srv_conf(ngx_conf_t *cf);
> > > > +static char *ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
> > > > +
> > > > +
> > > > +static ngx_command_t ngx_stream_pass_commands[] = {
> > > > +
> > > > + { ngx_string("pass"),
> > > > + NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
> > > > + ngx_stream_pass,
> > > > + NGX_STREAM_SRV_CONF_OFFSET,
> > > > + 0,
> > > > + NULL },
> > > > +
> > > > + ngx_null_command
> > > > +};
> > > > +
> > > > +
> > > > +static ngx_stream_module_t ngx_stream_pass_module_ctx = {
> > > > + NULL, /* preconfiguration */
> > > > + NULL, /* postconfiguration */
> > > > +
> > > > + NULL, /* create main configuration */
> > > > + NULL, /* init main configuration */
> > > > +
> > > > + ngx_stream_pass_create_srv_conf, /* create server configuration */
> > > > + NULL /* merge server configuration */
> > > > +};
> > > > +
> > > > +
> > > > +ngx_module_t ngx_stream_pass_module = {
> > > > + NGX_MODULE_V1,
> > > > + &ngx_stream_pass_module_ctx, /* module conaddr */
> > > > + ngx_stream_pass_commands, /* module directives */
> > > > + NGX_STREAM_MODULE, /* module type */
> > > > + NULL, /* init master */
> > > > + NULL, /* init module */
> > > > + NULL, /* init process */
> > > > + NULL, /* init thread */
> > > > + NULL, /* exit thread */
> > > > + NULL, /* exit process */
> > > > + NULL, /* exit master */
> > > > + NGX_MODULE_V1_PADDING
> > > > +};
> > > > +
> > > > +
> > > > +static void
> > > > +ngx_stream_pass_handler(ngx_stream_session_t *s)
> > > > +{
> > > > + ngx_url_t u;
> > > > + ngx_str_t url;
> > > > + ngx_addr_t *addr;
> > > > + ngx_uint_t i;
> > > > + ngx_listening_t *ls;
> > > > + ngx_connection_t *c;
> > > > + ngx_stream_pass_srv_conf_t *pscf;
> > > > +
> > > > + c = s->connection;
> > > > +
> > > > + c->log->action = "passing connection to another module";
> > > > +
> > > > + if (c->buffer && c->buffer->pos != c->buffer->last) {
> > > > + ngx_log_error(NGX_LOG_ERR, s->connection->log, 0,
> > > > + "cannot pass connection with preread data");
> > > > + goto failed;
> > > > + }
> > > > +
> > > > + pscf = ngx_stream_get_module_srv_conf(s, ngx_stream_pass_module);
> > > > +
> > > > + addr = pscf->addr;
> > > > +
> > > > + if (addr == NULL) {
> > > > + if (ngx_stream_complex_value(s, pscf->addr_value, &url) != NGX_OK) {
> > > > + goto failed;
> > > > + }
> > > > +
> > > > + ngx_memzero(&u, sizeof(ngx_url_t));
> > > > +
> > > > + u.url = url;
> > > > + u.listen = 1;
> > > > + u.no_resolve = 1;
> > > > +
> > > > + if (ngx_parse_url(s->connection->pool, &u) != NGX_OK) {
> > > > + if (u.err) {
> > > > + ngx_log_error(NGX_LOG_ERR, s->connection->log, 0,
> > > > + "%s in pass \"%V\"", u.err, &u.url);
> > > > + }
> > > > +
> > > > + goto failed;
> > > > + }
> > > > +
> > > > + if (u.naddrs == 0) {
> > > > + ngx_log_error(NGX_LOG_ERR, s->connection->log, 0,
> > > > + "no addresses in pass \"%V\"", &u.url);
> > > > + goto failed;
> > > > + }
> > > > +
> > > > + addr = &u.addrs[0];
> > > > + }
> > > > +
> > > > + ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0,
> > > > + "stream pass addr: \"%V\"", &addr->name);
> > > > +
> > > > + ls = ngx_cycle->listening.elts;
> > > > +
> > > > + for (i = 0; i < ngx_cycle->listening.nelts; i++) {
> > > > + if (ngx_cmp_sockaddr(ls[i].sockaddr, ls[i].socklen,
> > > > + addr->sockaddr, addr->socklen, 1)
> > > > + == NGX_OK)
> > > > + {
> > > > + c->listening = &ls[i];
> > >
> > > The address configuration (addr_conf) is stored depending on the
> > > protocol family of the listening socket, it's different for AF_INET6.
> > > So, if the protocol family is switched when passing a connection,
> > > it may happen that c->local_sockaddr->sa_family will keep a wrong
> > > value, the listen handler will dereference addr_conf incorrectly.
> > >
> > > Consider the following example:
> > >
> > > server {
> > > listen 127.0.0.1:8081;
> > > pass [::1]:8091;
> > > }
> > >
> > > server {
> > > listen [::1]:8091;
> > > ...
> > > }
> > >
> > > When ls->handler is invoked, c->local_sockaddr is kept inherited
> > > from the originally accepted connection, which is of AF_INET.
> > > To fix this, c->local_sockaddr and c->local_socklen should be
> > > updated according to the new listen socket configuration.
> >
> > Sure, thanks.
> >
> > > OTOH, c->sockaddr / c->socklen should be kept intact.
> > > Note that this makes possible cross protocol family
> > > configurations in e.g. realip and access modules;
> > > from now on this will have to be taken into account.
> >
> > This is already possible with proxy_protocol+realip and is known to cause minor
> > issues with third-party code that's too pedantic about families.
> >
> > Also I've just sent an updated patch which fixes PROXY protocol headers
> > generated for mixed family addresses.
> >
> > > > +
> > > > + c->data = NULL;
> > > > + c->buffer = NULL;
> > > > +
> > > > + *c->log = c->listening->log;
> > > > + c->log->handler = NULL;
> > > > + c->log->data = NULL;
> > > > +
> > > > + c->listening->handler(c);
> > > > +
> > > > + return;
> > > > + }
> > > > + }
> > > > +
> > > > + ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > > > + "listen not found for \"%V\"", &addr->name);
> > > > +
> > > > + ngx_stream_finalize_session(s, NGX_STREAM_OK);
> > > > +
> > > > + return;
> > > > +
> > > > +failed:
> > > > +
> > > > + ngx_stream_finalize_session(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
> > > > +}
> > > > +
> > > > +
> > > > +static void *
> > > > +ngx_stream_pass_create_srv_conf(ngx_conf_t *cf)
> > > > +{
> > > > + ngx_stream_pass_srv_conf_t *conf;
> > > > +
> > > > + conf = ngx_pcalloc(cf->pool, sizeof(ngx_stream_pass_srv_conf_t));
> > > > + if (conf == NULL) {
> > > > + return NULL;
> > > > + }
> > > > +
> > > > + /*
> > > > + * set by ngx_pcalloc():
> > > > + *
> > > > + * conf->addr = NULL;
> > > > + * conf->addr_value = NULL;
> > > > + */
> > > > +
> > > > + return conf;
> > > > +}
> > > > +
> > > > +
> > > > +static char *
> > > > +ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> > > > +{
> > > > + ngx_stream_pass_srv_conf_t *pscf = conf;
> > > > +
> > > > + ngx_url_t u;
> > > > + ngx_str_t *value, *url;
> > > > + ngx_stream_complex_value_t cv;
> > > > + ngx_stream_core_srv_conf_t *cscf;
> > > > + ngx_stream_compile_complex_value_t ccv;
> > > > +
> > > > + if (pscf->addr || pscf->addr_value) {
> > > > + return "is duplicate";
> > > > + }
> > > > +
> > > > + cscf = ngx_stream_conf_get_module_srv_conf(cf, ngx_stream_core_module);
> > > > +
> > > > + cscf->handler = ngx_stream_pass_handler;
> > > > +
> > > > + value = cf->args->elts;
> > > > +
> > > > + url = &value[1];
> > > > +
> > > > + ngx_memzero(&ccv, sizeof(ngx_stream_compile_complex_value_t));
> > > > +
> > > > + ccv.cf = cf;
> > > > + ccv.value = url;
> > > > + ccv.complex_value = &cv;
> > > > +
> > > > + if (ngx_stream_compile_complex_value(&ccv) != NGX_OK) {
> > > > + return NGX_CONF_ERROR;
> > > > + }
> > > > +
> > > > + if (cv.lengths) {
> > > > + pscf->addr_value = ngx_palloc(cf->pool,
> > > > + sizeof(ngx_stream_complex_value_t));
> > > > + if (pscf->addr_value == NULL) {
> > > > + return NGX_CONF_ERROR;
> > > > + }
> > > > +
> > > > + *pscf->addr_value = cv;
> > > > +
> > > > + return NGX_CONF_OK;
> > > > + }
> > > > +
> > > > + ngx_memzero(&u, sizeof(ngx_url_t));
> > > > +
> > > > + u.url = *url;
> > > > + u.listen = 1;
> > > > +
> > > > + if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
> > > > + if (u.err) {
> > > > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > > > + "%s in \"%V\" of the \"pass\" directive",
> > > > + u.err, &u.url);
> > > > + }
> > > > +
> > > > + return NGX_CONF_ERROR;
> > > > + }
> > > > +
> > > > + if (u.naddrs == 0) {
> > > > + return "has no addresses";
> > > > + }
> > > > +
> > > > + pscf->addr = &u.addrs[0];
> > > > +
> > > > + return NGX_CONF_OK;
> > > > +}
> >
> > Attached is an improved version with the following changes:
> >
> > - Removed 'listen = 1' flag when parsing "pass" parameter.
> > Now it's treated like "proxy_pass" parameter.
> > - Listen match reworked to be able to match wildcards.
> > - Local_sockaddr is copied to the connection after match.
> > - Fixes in log action, log messages, commit log etc.
> >
> > --
> > Roman Arutyunyan
>
> > # HG changeset patch
> > # User Roman Arutyunyan <arut@nginx.com>
> > # Date 1708522562 -14400
> > # Wed Feb 21 17:36:02 2024 +0400
> > # Node ID 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
> > # Parent 4eb76c257fd07a69fc9e9386e845edcc9e2b1b08
> > Stream: ngx_stream_pass_module.
> >
> > The module allows to pass connections from Stream to other modules such as HTTP
> > or Mail, as well as back to Stream. Previously, this was only possible with
> > proxying. Connections with preread buffer read out from socket cannot be
> > passed.
> >
> > The module allows selective SSL termination based on SNI.
> >
> > stream {
> > server {
> > listen 8000 default_server;
> > ssl_preread on;
> > ...
> > }
> >
> > server {
> > listen 8000;
> > server_name foo.example.com;
> > pass 127.0.0.1:8001; # to HTTP
> > }
> >
> > server {
> > listen 8000;
> > server_name bar.example.com;
> > ...
> > }
> > }
> >
> > http {
> > server {
> > listen 8001 ssl;
> > ...
> >
> > location / {
> > root html;
> > }
> > }
> > }
> >
> > diff --git a/auto/modules b/auto/modules
> > --- a/auto/modules
> > +++ b/auto/modules
> > @@ -1166,6 +1166,16 @@ if [ $STREAM != NO ]; then
> > . auto/module
> > fi
> >
> > + if [ $STREAM_PASS = YES ]; then
> > + ngx_module_name=ngx_stream_pass_module
> > + ngx_module_deps=
> > + ngx_module_srcs=src/stream/ngx_stream_pass_module.c
> > + ngx_module_libs=
> > + ngx_module_link=$STREAM_PASS
> > +
> > + . auto/module
> > + fi
> > +
> > if [ $STREAM_SET = YES ]; then
> > ngx_module_name=ngx_stream_set_module
> > ngx_module_deps=
> > diff --git a/auto/options b/auto/options
> > --- a/auto/options
> > +++ b/auto/options
> > @@ -127,6 +127,7 @@ STREAM_GEOIP=NO
> > STREAM_MAP=YES
> > STREAM_SPLIT_CLIENTS=YES
> > STREAM_RETURN=YES
> > +STREAM_PASS=YES
> > STREAM_SET=YES
> > STREAM_UPSTREAM_HASH=YES
> > STREAM_UPSTREAM_LEAST_CONN=YES
> > @@ -337,6 +338,7 @@ use the \"--with-mail_ssl_module\" optio
> > --without-stream_split_clients_module)
> > STREAM_SPLIT_CLIENTS=NO ;;
> > --without-stream_return_module) STREAM_RETURN=NO ;;
> > + --without-stream_pass_module) STREAM_PASS=NO ;;
> > --without-stream_set_module) STREAM_SET=NO ;;
> > --without-stream_upstream_hash_module)
> > STREAM_UPSTREAM_HASH=NO ;;
> > @@ -556,6 +558,7 @@ cat << END
> > --without-stream_split_clients_module
> > disable ngx_stream_split_clients_module
> > --without-stream_return_module disable ngx_stream_return_module
> > + --without-stream_pass_module disable ngx_stream_pass_module
> > --without-stream_set_module disable ngx_stream_set_module
> > --without-stream_upstream_hash_module
> > disable ngx_stream_upstream_hash_module
> > diff --git a/src/stream/ngx_stream_pass_module.c b/src/stream/ngx_stream_pass_module.c
> > new file mode 100644
> > --- /dev/null
> > +++ b/src/stream/ngx_stream_pass_module.c
> > @@ -0,0 +1,272 @@
> > +
> > +/*
> > + * Copyright (C) Roman Arutyunyan
> > + * Copyright (C) Nginx, Inc.
> > + */
> > +
> > +
> > +#include <ngx_config.h>
> > +#include <ngx_core.h>
> > +#include <ngx_stream.h>
> > +
> > +
> > +typedef struct {
> > + ngx_addr_t *addr;
> > + ngx_stream_complex_value_t *addr_value;
> > +} ngx_stream_pass_srv_conf_t;
> > +
> > +
> > +static void ngx_stream_pass_handler(ngx_stream_session_t *s);
> > +static ngx_int_t ngx_stream_pass_match(ngx_listening_t *ls, ngx_addr_t *addr);
> > +static void *ngx_stream_pass_create_srv_conf(ngx_conf_t *cf);
> > +static char *ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf);
> > +
> > +
> > +static ngx_command_t ngx_stream_pass_commands[] = {
> > +
> > + { ngx_string("pass"),
> > + NGX_STREAM_SRV_CONF|NGX_CONF_TAKE1,
> > + ngx_stream_pass,
> > + NGX_STREAM_SRV_CONF_OFFSET,
> > + 0,
> > + NULL },
> > +
> > + ngx_null_command
> > +};
> > +
> > +
> > +static ngx_stream_module_t ngx_stream_pass_module_ctx = {
> > + NULL, /* preconfiguration */
> > + NULL, /* postconfiguration */
> > +
> > + NULL, /* create main configuration */
> > + NULL, /* init main configuration */
> > +
> > + ngx_stream_pass_create_srv_conf, /* create server configuration */
> > + NULL /* merge server configuration */
> > +};
> > +
> > +
> > +ngx_module_t ngx_stream_pass_module = {
> > + NGX_MODULE_V1,
> > + &ngx_stream_pass_module_ctx, /* module context */
> > + ngx_stream_pass_commands, /* module directives */
> > + NGX_STREAM_MODULE, /* module type */
> > + NULL, /* init master */
> > + NULL, /* init module */
> > + NULL, /* init process */
> > + NULL, /* init thread */
> > + NULL, /* exit thread */
> > + NULL, /* exit process */
> > + NULL, /* exit master */
> > + NGX_MODULE_V1_PADDING
> > +};
> > +
> > +
> > +static void
> > +ngx_stream_pass_handler(ngx_stream_session_t *s)
> > +{
> > + ngx_url_t u;
> > + ngx_str_t url;
> > + ngx_addr_t *addr;
> > + ngx_uint_t i;
> > + ngx_listening_t *ls;
> > + struct sockaddr *sa;
> > + ngx_connection_t *c;
> > + ngx_stream_pass_srv_conf_t *pscf;
> > +
> > + c = s->connection;
> > +
> > + c->log->action = "passing connection to port";
> > +
> > + if (c->buffer && c->buffer->pos != c->buffer->last) {
> > + ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > + "cannot pass connection with preread data");
> > + goto failed;
> > + }
> > +
> > + pscf = ngx_stream_get_module_srv_conf(s, ngx_stream_pass_module);
> > +
> > + addr = pscf->addr;
> > +
> > + if (addr == NULL) {
> > + if (ngx_stream_complex_value(s, pscf->addr_value, &url) != NGX_OK) {
> > + goto failed;
> > + }
> > +
> > + ngx_memzero(&u, sizeof(ngx_url_t));
> > +
> > + u.url = url;
> > + u.no_resolve = 1;
>
> This makes configurations with variables of limited use.

The functionality is indeed different for static and dynamic addresses.
Currently it's the best we can do without introducing more complexity.
We can add dynamic resolving here like in the upstream module and eliminate the
confusion. However a better way to eliminate it is to disable resolve
completely for both static and dynamic configurations. It seems to me,
resolving names in the pass module makes little sense. All addresses are
local anyway.

> > + if (ngx_parse_url(c->pool, &u) != NGX_OK) {
> > + if (u.err) {
> > + ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > + "%s in pass \"%V\"", u.err, &u.url);
> > + }
> > +
> > + goto failed;
> > + }
> > +
> > + if (u.naddrs == 0) {
> > + ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > + "no addresses in pass \"%V\"", &u.url);
> > + goto failed;
> > + }
> > +
> > + addr = &u.addrs[0];
> > + }
> > +
> > + ngx_log_debug1(NGX_LOG_DEBUG_STREAM, c->log, 0,
> > + "stream pass addr: \"%V\"", &addr->name);
> > +
> > + ls = ngx_cycle->listening.elts;
> > +
> > + for (i = 0; i < ngx_cycle->listening.nelts; i++) {
> > +
> > + if (ngx_stream_pass_match(&ls[i], addr) != NGX_OK) {
> > + continue;
> > + }
> > +
> > + c->listening = &ls[i];
> > +
> > + c->data = NULL;
> > + c->buffer = NULL;
> > +
> > + *c->log = c->listening->log;
> > + c->log->handler = NULL;
> > + c->log->data = NULL;
> > +
> > + sa = ngx_palloc(c->pool, addr->socklen);
> > + if (sa == NULL) {
> > + goto failed;
> > + }
>
> Is there a reason to (re-)allocate memory for c->local_sockaddr ?
>
> Either way, "addr" is stored in some pool, allocated in ngx_parse_url()
> through ngx_inet_add_addr(). It should be safe to reference it there.

Sure, removed the allocation.

> > + ngx_memcpy(sa, addr->sockaddr, addr->socklen);
> > + c->local_sockaddr = sa;
> > + c->local_socklen = addr->socklen;
> > +
> > + c->listening->handler(c);
> > +
> > + return;
> > + }
> > +
> > + ngx_log_error(NGX_LOG_ERR, c->log, 0,
> > + "port not found for \"%V\"", &addr->name);
> > +
> > + ngx_stream_finalize_session(s, NGX_STREAM_OK);
> > +
> > + return;
> > +
> > +failed:
> > +
> > + ngx_stream_finalize_session(s, NGX_STREAM_INTERNAL_SERVER_ERROR);
> > +}
> > +
> > +
> > +static ngx_int_t
> > +ngx_stream_pass_match(ngx_listening_t *ls, ngx_addr_t *addr)
> > +{
> > + if (!ls->wildcard) {
> > + return ngx_cmp_sockaddr(ls->sockaddr, ls->socklen,
> > + addr->sockaddr, addr->socklen, 1);
> > + }
> > +
> > + if (ls->sockaddr->sa_family == addr->sockaddr->sa_family
> > + && ngx_inet_get_port(ls->sockaddr) == ngx_inet_get_port(addr->sockaddr))
> > + {
> > + return NGX_OK;
> > + }
> > +
> > + return NGX_DECLINED;
> > +}
> > +
> > +
> > +static void *
> > +ngx_stream_pass_create_srv_conf(ngx_conf_t *cf)
> > +{
> > + ngx_stream_pass_srv_conf_t *conf;
> > +
> > + conf = ngx_pcalloc(cf->pool, sizeof(ngx_stream_pass_srv_conf_t));
> > + if (conf == NULL) {
> > + return NULL;
> > + }
> > +
> > + /*
> > + * set by ngx_pcalloc():
> > + *
> > + * conf->addr = NULL;
> > + * conf->addr_value = NULL;
> > + */
> > +
> > + return conf;
> > +}
> > +
> > +
> > +static char *
> > +ngx_stream_pass(ngx_conf_t *cf, ngx_command_t *cmd, void *conf)
> > +{
> > + ngx_stream_pass_srv_conf_t *pscf = conf;
> > +
> > + ngx_url_t u;
> > + ngx_str_t *value, *url;
> > + ngx_stream_complex_value_t cv;
> > + ngx_stream_core_srv_conf_t *cscf;
> > + ngx_stream_compile_complex_value_t ccv;
> > +
> > + if (pscf->addr || pscf->addr_value) {
> > + return "is duplicate";
> > + }
> > +
> > + cscf = ngx_stream_conf_get_module_srv_conf(cf, ngx_stream_core_module);
> > +
> > + cscf->handler = ngx_stream_pass_handler;
> > +
> > + value = cf->args->elts;
> > +
> > + url = &value[1];
> > +
> > + ngx_memzero(&ccv, sizeof(ngx_stream_compile_complex_value_t));
> > +
> > + ccv.cf = cf;
> > + ccv.value = url;
> > + ccv.complex_value = &cv;
> > +
> > + if (ngx_stream_compile_complex_value(&ccv) != NGX_OK) {
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + if (cv.lengths) {
> > + pscf->addr_value = ngx_palloc(cf->pool,
> > + sizeof(ngx_stream_complex_value_t));
> > + if (pscf->addr_value == NULL) {
> > + return NGX_CONF_ERROR;
> > + }
> > +
> > + *pscf->addr_value = cv;
> > +
> > + return NGX_CONF_OK;
> > + }
> > +
> > + ngx_memzero(&u, sizeof(ngx_url_t));
> > +
> > + u.url = *url;
> > +
> > + if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
> > + if (u.err) {
> > + ngx_conf_log_error(NGX_LOG_EMERG, cf, 0,
> > + "%s in \"%V\" of the \"pass\" directive",
> > + u.err, &u.url);
> > + }
> > +
> > + return NGX_CONF_ERROR;
> > + }
>
> Although you've changed the commit example from "pass 8081" to
> "pass 127.0.0.1:8001", the former syntax is still allowed.
>
> This may be misleading: with the current code, unlike "u.listen = 1",
> this means "8081" will be tested as an address (without a port)
> written in a decimal format, as finally resolved by getaddrinfo(3).
> So, using "pass 8081" corresponds to 0x1f91, or "0.0.31.145".
> Further, since it has no port, it will never match listen addresses.

Similarly we can do this in the http proxy module: "proxy_pass http://8081".
Of course there's a default port there, but it does not change the fact
that listen-like syntax is allowed in proxy_pass, and produces misleading
results as well.

> I'd check and forbid this explicitly:
>
> if (u.no_port) {
> return "has no port";
> }

Makes sense, thanks.

> > +
> > + if (u.naddrs == 0) {
> > + return "has no addresses";
> > + }
>
> It seems that this check can never happen if neither "u.no_resolve"
> nor "u.listen" set, such as in here.
> In the worst case, when the address couldn't be parsed as a literal,
> and ngx_parse_url() falls back to ngx_inet_resolve_host() to resolve
> as a name, which is either NXDOMAIN or has none of A/AAAA records,
> then ngx_parse_url() will return an error with the "host not found"
> diagnostics. It looks like another left-over from "u.listen = 1".

If we add "u.no_resolve = 1", as discussed above, this condition will make
sense again.

> > + pscf->addr = &u.addrs[0];
> > +
> > + return NGX_CONF_OK;
> > +}
> _______________________________________________
> nginx-devel mailing list
> nginx-devel@nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

Diff attached.

--
Roman Arutyunyan
# HG changeset patch
# User Roman Arutyunyan <arut@nginx.com>
# Date 1709125245 -14400
# Wed Feb 28 17:00:45 2024 +0400
# Node ID f533e218c56a1d14be02c0b81409bcc12bed3562
# Parent 44da04c2d4db94ad4eefa84b299e07c5fa4a00b9
imported patch stream-pass-fix1

diff --git a/src/stream/ngx_stream_pass_module.c b/src/stream/ngx_stream_pass_module.c
--- a/src/stream/ngx_stream_pass_module.c
+++ b/src/stream/ngx_stream_pass_module.c
@@ -71,7 +71,6 @@ ngx_stream_pass_handler(ngx_stream_sessi
ngx_addr_t *addr;
ngx_uint_t i;
ngx_listening_t *ls;
- struct sockaddr *sa;
ngx_connection_t *c;
ngx_stream_pass_srv_conf_t *pscf;

@@ -114,6 +113,12 @@ ngx_stream_pass_handler(ngx_stream_sessi
goto failed;
}

+ if (u.no_port) {
+ ngx_log_error(NGX_LOG_ERR, c->log, 0,
+ "no port in pass \"%V\"", &u.url);
+ goto failed;
+ }
+
addr = &u.addrs[0];
}

@@ -137,13 +142,7 @@ ngx_stream_pass_handler(ngx_stream_sessi
c->log->handler = NULL;
c->log->data = NULL;

- sa = ngx_palloc(c->pool, addr->socklen);
- if (sa == NULL) {
- goto failed;
- }
-
- ngx_memcpy(sa, addr->sockaddr, addr->socklen);
- c->local_sockaddr = sa;
+ c->local_sockaddr = addr->sockaddr;
c->local_socklen = addr->socklen;

c->listening->handler(c);
@@ -251,6 +250,7 @@ ngx_stream_pass(ngx_conf_t *cf, ngx_comm
ngx_memzero(&u, sizeof(ngx_url_t));

u.url = *url;
+ u.no_resolve = 1;

if (ngx_parse_url(cf->pool, &u) != NGX_OK) {
if (u.err) {
@@ -266,6 +266,10 @@ ngx_stream_pass(ngx_conf_t *cf, ngx_comm
return "has no addresses";
}

+ if (u.no_port) {
+ return "has no port";
+ }
+
pscf->addr = &u.addrs[0];

return NGX_CONF_OK;
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel