On Fri, Oct 7, 2022 at 9:14 PM Ruediger Pluem wrote:
>
> On 10/7/22 7:11 PM, Stefan Eissing via dev wrote:
> >
> >
> >> Am 07.10.2022 um 18:45 schrieb Yann Ylavic :
> >>
> >
> > Thanks, Yann, for the detailed explanation on how this works and that the
> > default does the right thing.
>
> +1. I missed the default of not reusing the connection in these cases but
> having the possibility to override it like the user did.
> But shouldn't we default to not reusing the connection in case of a $ inside
> the port as well?
We default to disabling connection reuse for any $ substitution
already, the compat issue is for an explicit enablereuse=on which used
to "work" because it was ignored..
How about a patch like the attached one now, which disables connection
reuse definitively (regardless of enablereuse) if there is a $
substitution in the hostname or port part of the URL?
The patch uses the existing "is_address_reusable" flag (set to false
initially in this case), and since it's not configurable by the user
we are safe from a connection reuse point of vue in any case.
>
> >
> > My guess is that such configurations are pretty rare, as their security is
> > not good in general. The only thing that comes to mind is logging a warning
> > for such cases.
>
> +1
Done in the patch too, along with some docs update regarding this use case.
Regards;
Yann.
Index: modules/proxy/proxy_util.c
===
--- modules/proxy/proxy_util.c (revision 1903576)
+++ modules/proxy/proxy_util.c (working copy)
@@ -1851,6 +1851,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
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;
/*
@@ -1879,6 +1880,12 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
* 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, "://");
@@ -1885,6 +1892,9 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
if (pos) {
pos += 3;
while (*pos && *pos != ':' && *pos != '/') {
+if (*pos == '$') {
+disable_reuse = 1;
+}
pos++;
}
if (*pos == ':') {
@@ -1904,6 +1914,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
vec[1].iov_base = (void *)path;
vec[1].iov_len = strlen(path);
ptr = apr_pstrcatv(p, vec, 2, NULL);
+disable_reuse = 1;
}
}
}
@@ -1999,7 +2010,7 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
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;
@@ -2008,7 +2019,31 @@ PROXY_DECLARE(char *) ap_proxy_define_worker_ex(ap
wshared->hash.def = ap_proxy_hashfunc(wshared->name_ex, PROXY_HASHFUNC_DEFAULT);
wshared->hash.fnv = ap_proxy_hashfunc(wshared->name_ex, 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.
