On Wed, Feb 28, 2024 at 06:22:34PM +0400, Roman Arutyunyan wrote:
> 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,
> > >
> > > 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.

Agree.

>
> > > + 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.

Ok.

>
> > > + pscf->addr = &u.addrs[0];
> > > +
> > > + return NGX_CONF_OK;
> > > +}
>
> 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;

Looks good to me.
_______________________________________________
nginx-devel mailing list
nginx-devel@nginx.org
https://mailman.nginx.org/mailman/listinfo/nginx-devel