From c74bf2f82190e850782bf0d1ac16613d5c8266a6 Mon Sep 17 00:00:00 2001 From: Yann Ylavic Date: Tue, 11 Oct 2022 09:53:04 +0000 Subject: [PATCH] mod_proxy: Ignore (and warn about) enablereuse=on for ProxyPassMatch when some dollar substitution (backreference) happens in the hostname or port part of the URL. Address or connection reuse can't work when the autority part of the URL is dynamic (single origin server[:port] handled/assumed in the reslist). Detect such cases and unset worker->s->is_address_reusable to disable reuse regardless of enablereuse/disablereuse. * modules/proxy/proxy_util.c(ap_proxy_define_worker_ex): Lookup for $n substitution in the hostname[:port] when parsing the URL and if present, set worker->->is_address_reusable=0 / worker->s->disablereuse=1. * modules/proxy/proxy_util.c(ap_proxy_initialize_worker): Don't overwrite worker->s->is_address_reusable from enablereuse/disablereuse parameters, and set both consistently. * docs/manual/mod/mod_proxy.xml: Add ProxyPassMatch compatibility note about key=value parameters handled with $n substitutions since 2.4.47. Document the specificities of enablereuse/disablereuse w.r.t. $n subsitutions in the different part of the URL. Axe the note about unparsable URLs when the $n substitution happens in the port, this has been addressed in 2.4.47 too (and works now). git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1904513 13f79535-47bb-0310-9956-ffa450edef68 --- changes-entries/enablereuse.txt | 3 ++ docs/manual/mod/mod_proxy.xml | 32 ++++++++------- modules/proxy/proxy_util.c | 69 ++++++++++++++++++++++++--------- 3 files changed, 71 insertions(+), 33 deletions(-) create mode 100644 changes-entries/enablereuse.txt diff --git a/changes-entries/enablereuse.txt b/changes-entries/enablereuse.txt new file mode 100644 index 0000000000..b8e1a3f617 --- /dev/null +++ b/changes-entries/enablereuse.txt @@ -0,0 +1,3 @@ + *) mod_proxy: Ignore (and warn about) enablereuse=on for ProxyPassMatch when + some dollar substitution (backreference) happens in the hostname or port + part of the URL. [Yann Ylavic] diff --git a/docs/manual/mod/mod_proxy.xml b/docs/manual/mod/mod_proxy.xml index a440f99ccc..811c242d3f 100644 --- a/docs/manual/mod/mod_proxy.xml +++ b/docs/manual/mod/mod_proxy.xml @@ -1511,6 +1511,9 @@ ProxyPassReverse "/mirror/foo/" "https://backend.example.com/" server configvirtual host directory +Since 2.4.47 the key=value Parameters are honored +when the url parameter contains backreference(s) (see note below). +

This directive is equivalent to ProxyPass @@ -1532,20 +1535,7 @@ ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com/$1"

will cause a local request for http://example.com/foo/bar.gif to be internally converted into a proxy request to http://backend.example.com/foo/bar.gif.

- Note -

The URL argument must be parsable as a URL before regexp - substitutions (as well as after). This limits the matches you can use. - For instance, if we had used

- -ProxyPassMatch "^(/.*\.gif)$" "http://backend.example.com:8000$1" - -

in our previous example, it would fail with a syntax error - at server startup. This is a bug (PR 46665 in the ASF bugzilla), - and the workaround is to reformulate the match:

- -ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com:8000/$1" - - +

The ! directive is useful in situations where you don't want to reverse-proxy a subdirectory.

@@ -1564,6 +1554,20 @@ ProxyPassMatch "^/(.*\.gif)$" "http://backend.example.com:8000/$1" expression, the original URL will be appended to the URL parameter.

+ + <var><code>key=value</code> Parameters versus <var>url</var> with backreference(s) +

Since Apache HTTP Server 2.4.47, the key=value Parameters + are no longer ignored in a ProxyPassMatch using + an url with backreference(s). However to keep the existing + behavior regarding reuse/keepalive of backend connections (which were + never reused before for these URLs), the parameter enablereuse + (or disablereuse) default to off (resp. on) + in this case. Setting enablereuse=on explicitely allows to + reuse connections unless some backreference(s) belong in + the authority part (hostname and/or port) of the url + (this condition is enforced since Apache HTTP Server 2.4.55, and produces + a warning at startup because these URLs are not reusable per se).

+ Security Warning diff --git a/modules/proxy/proxy_util.c b/modules/proxy/proxy_util.c index 434e37c9d4..e4d9df9c48 100644 --- a/modules/proxy/proxy_util.c +++ b/modules/proxy/proxy_util.c @@ -1844,6 +1844,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, proxy_worker_shared *wshared; const char *ptr = NULL, *sockpath = NULL, *pdollars = NULL; apr_port_t port_of_scheme; + int disable_reuse = 0; apr_uri_t uri; /* @@ -1872,12 +1873,21 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, * to fail (e.g. "ProxyPassMatch ^/(a|b)(/.*)? http://host:port$2"). * So we trim all the $n from the :port and prepend them in uri.path * afterward for apr_uri_unparse() to restore the original URL below. + * If a dollar substitution is found in the hostname[:port] part of + * the URL, reusing address and connections in the same worker is not + * possible (the current implementation of active connections cache + * handles/assumes a single origin server:port per worker only), so + * we set disable_reuse here during parsing to take that into account + * in the worker settings below. */ #define IS_REF(x) (x[0] == '$' && apr_isdigit(x[1])) const char *pos = ap_strstr_c(ptr, "://"); if (pos) { pos += 3; while (*pos && *pos != ':' && *pos != '/') { + if (*pos == '$') { + disable_reuse = 1; + } pos++; } if (*pos == ':') { @@ -1897,6 +1907,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, vec[1].iov_base = (void *)path; vec[1].iov_len = strlen(path); ptr = apr_pstrcatv(p, vec, 2, NULL); + disable_reuse = 1; } } } @@ -1987,7 +1998,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, wshared->port = (uri.port) ? uri.port : port_of_scheme; wshared->flush_packets = flush_off; wshared->flush_wait = PROXY_FLUSH_WAIT; - wshared->is_address_reusable = 1; + wshared->is_address_reusable = !disable_reuse; wshared->lbfactor = 100; wshared->passes = 1; wshared->fails = 1; @@ -1996,7 +2007,31 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, wshared->hash.def = ap_proxy_hashfunc(wshared->name, PROXY_HASHFUNC_DEFAULT); wshared->hash.fnv = ap_proxy_hashfunc(wshared->name, PROXY_HASHFUNC_FNV); wshared->was_malloced = (mask & AP_PROXY_WORKER_IS_MALLOCED) != 0; - wshared->is_name_matchable = 0; + if (mask & AP_PROXY_WORKER_IS_MATCH) { + wshared->is_name_matchable = 1; + + /* Before AP_PROXY_WORKER_IS_MATCH (< 2.4.47), a regex worker with + * dollar substitution was never matched against any actual URL, thus + * the requests fell through the generic worker. Now if a ProyPassMatch + * matches, a worker (and its parameters) is always used to determine + * the properties of the connection with the origin server. So for + * instance the same "timeout=" will be enforced for all the requests + * matched by the same ProyPassMatch worker, which is an improvement + * compared to the global/vhost [Proxy]Timeout applied by the generic + * worker. Likewise, address and connection reuse is the default for + * a ProyPassMatch worker with no dollar substitution, just like a + * "normal" worker. However to avoid DNS and connection reuse compat + * issues, connection reuse is disabled by default if there is any + * substitution in the uri-path (an explicit enablereuse=on can still + * opt-in), and reuse is even disabled definitively for substitutions + * happening in the hostname[:port] (disable_reuse was set above so + * address reuse is also disabled which will prevent enablereuse=on + * to apply anyway). + */ + if (disable_reuse || ap_strchr_c(wshared->name, '$')) { + wshared->disablereuse = 1; + } + } if (sockpath) { if (PROXY_STRNCPY(wshared->uds_path, sockpath) != APR_SUCCESS) { return apr_psprintf(p, "worker uds path (%s) too long", sockpath); @@ -2016,20 +2051,6 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(apr_pool_t *p, (*worker)->balancer = balancer; (*worker)->s = wshared; - if (mask & AP_PROXY_WORKER_IS_MATCH) { - (*worker)->s->is_name_matchable = 1; - if (ap_strchr_c((*worker)->s->name, '$')) { - /* Before AP_PROXY_WORKER_IS_MATCH (< 2.4.47), a regex worker - * with dollar substitution was never matched against the actual - * URL thus the request fell through the generic worker. To avoid - * dns and connection reuse compat issues, let's disable connection - * reuse by default, it can still be overwritten by an explicit - * enablereuse=on. - */ - (*worker)->s->disablereuse = 1; - } - } - return NULL; } @@ -2115,12 +2136,22 @@ PROXY_DECLARE(apr_status_t) ap_proxy_initialize_worker(proxy_worker *worker, ser if (!worker->s->retry_set) { worker->s->retry = apr_time_from_sec(PROXY_WORKER_DEFAULT_RETRY); } - /* By default address is reusable unless DisableReuse is set */ + /* Consistently set address and connection reusabilty: when reuse + * is disabled by configuration, or when the address is known already + * to not be reusable for this worker (in any case, thus ignore/force + * DisableReuse). + */ if (worker->s->disablereuse) { worker->s->is_address_reusable = 0; } - else { - worker->s->is_address_reusable = 1; + else if (!worker->s->is_address_reusable) { + /* Explicit enablereuse=on can't work in this case, warn user. */ + if (worker->s->disablereuse_set && !worker->s->disablereuse) { + ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, APLOGNO(10400) + "enablereuse/disablereuse ignored for worker %s", + ap_proxy_worker_name(p, worker)); + } + worker->s->disablereuse = 1; } /*