On Mon, Nov 4, 2019 at 4:16 PM Ruediger Pluem <rpl...@apache.org> wrote:
>
> On 11/03/2019 04:48 PM, yla...@apache.org wrote:
> >
> > + /* XXX: what's the point of "NONE"? We probably should _always_ check
> > + * that the client wants an Upgrade..
> > + */
>
> NONE was used to upgrade the client whether it requested it or no idea what
> the purpose was.
> Maybe Jean-Frederic who introduced it in r1792092 can explain.
I asked Jean-Frédéric in another thread, let's wait.
I think it was not the correct way to address the issue reported to
JBoss, and wish we are not stuck with "NONE" compatibility (i.e. no
user..).
>
>
> > + upgrade_method = *worker->s->upgrade ? worker->s->upgrade :
> > "WebSocket";
> > if (ap_cstr_casecmp(upgrade_method, "NONE") != 0) {
> > - const char *upgrade;
> > upgrade = apr_table_get(r->headers_in, "Upgrade");
> > if (!upgrade || (ap_cstr_casecmp(upgrade, upgrade_method) != 0 &&
> > - ap_cstr_casecmp(upgrade_method, "ANY") !=0)) {
> > - ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(02900)
> > - "declining URL %s (not %s, Upgrade: header is
> > %s)",
> > - url, upgrade_method, upgrade ? upgrade :
> > "missing");
> > - return DECLINED;
> > + ap_cstr_casecmp(upgrade_method, "ANY") != 0)) {
> > + ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02900)
> > + "require upgrade for URL %s "
> > + "(Upgrade header is %s, expecting %s)",
> > + url, upgrade ? upgrade : "missing",
> > upgrade_method);
> > + apr_table_setn(r->err_headers_out, "Connection", "Upgrade");
> > + apr_table_setn(r->err_headers_out, "Upgrade", upgrade_method);
> > + return HTTP_UPGRADE_REQUIRED;
>
> Why don't we fallback any longer to normal HTTP handling if no upgrade
> handler was sent?
> This was itruduced by you in r1674632
Actually we never fallback despite r1674632 or even later r1776290 by
Eric, because previous proxy_trans hook already assigned "ws(s):"
scheme to r->filename. So at this point HTTP_UPGRADE_REQUIRED is the
best we can do IMHO.
I'm about to commit something like the attached patch to make this
work, but wouldn't want to handle "NONE" there nor in a future
mod_proxy_http change to handle Upgrade..
Regards,
Yann.
Index: modules/proxy/mod_proxy.h
===================================================================
--- modules/proxy/mod_proxy.h (revision 1869358)
+++ modules/proxy/mod_proxy.h (working copy)
@@ -623,6 +623,8 @@ APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, schem
(request_rec *r, proxy_worker *worker,
proxy_server_conf *conf, char *url,
const char *proxyhost, apr_port_t proxyport))
+APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, check_trans,
+ (request_rec *r, const char *url))
APR_DECLARE_EXTERNAL_HOOK(proxy, PROXY, int, canon_handler,
(request_rec *r, char *url))
Index: modules/proxy/mod_proxy.c
===================================================================
--- modules/proxy/mod_proxy.c (revision 1869358)
+++ modules/proxy/mod_proxy.c (working copy)
@@ -764,6 +764,15 @@ PROXY_DECLARE(int) ap_proxy_trans_match(request_re
}
if (found) {
+ /* A proxy module is assigned this URL, check whether it's interested
+ * in the request itself (e.g. proxy_wstunnel cares about Upgrade
+ * requests only, and could hand over to proxy_http otherwise).
+ */
+ int rc = proxy_run_check_trans(r, found + 6);
+ if (rc != OK && rc != DECLINED) {
+ return DONE;
+ }
+
r->filename = found;
r->handler = "proxy-server";
r->proxyreq = PROXYREQ_REVERSE;
@@ -3120,6 +3129,7 @@ APR_HOOK_STRUCT(
APR_HOOK_LINK(pre_request)
APR_HOOK_LINK(post_request)
APR_HOOK_LINK(request_status)
+ APR_HOOK_LINK(check_trans)
)
APR_IMPLEMENT_EXTERNAL_HOOK_RUN_FIRST(proxy, PROXY, int, scheme_handler,
@@ -3128,6 +3138,9 @@ APR_IMPLEMENT_EXTERNAL_HOOK_RUN_FIRST(proxy, PROXY
char *url, const char *proxyhost,
apr_port_t proxyport),(r,worker,conf,
url,proxyhost,proxyport),DECLINED)
+APR_IMPLEMENT_EXTERNAL_HOOK_RUN_FIRST(proxy, PROXY, int, check_trans,
+ (request_rec *r, const char *url),
+ (r, url), DECLINED)
APR_IMPLEMENT_EXTERNAL_HOOK_RUN_FIRST(proxy, PROXY, int, canon_handler,
(request_rec *r, char *url),(r,
url),DECLINED)
Index: modules/proxy/mod_proxy_wstunnel.c
===================================================================
--- modules/proxy/mod_proxy_wstunnel.c (revision 1869358)
+++ modules/proxy/mod_proxy_wstunnel.c (working copy)
@@ -93,6 +93,21 @@ static void proxy_wstunnel_callback(void *b)
}
+static int proxy_wstunnel_check_trans(request_rec *r, const char *url)
+{
+ if (ap_cstr_casecmpn(url, "ws:", 3) != 0
+ && ap_cstr_casecmpn(url, "wss:", 4) != 0) {
+ return DECLINED;
+ }
+
+ if (!apr_table_get(r->headers_in, "Upgrade")) {
+ /* No Upgrade, let mod_proxy_http handle it */
+ return HTTP_UPGRADE_REQUIRED;
+ }
+
+ return OK;
+}
+
/*
* Canonicalise http-like URLs.
* scheme is the scheme for the URL
@@ -414,6 +429,7 @@ static void ap_proxy_http_register_hook(apr_pool_t
{
static const char * const aszSucc[] = { "mod_proxy_http.c", NULL};
proxy_hook_scheme_handler(proxy_wstunnel_handler, NULL, aszSucc, APR_HOOK_FIRST);
+ proxy_hook_check_trans(proxy_wstunnel_check_trans, NULL, aszSucc, APR_HOOK_FIRST);
proxy_hook_canon_handler(proxy_wstunnel_canon, NULL, aszSucc, APR_HOOK_FIRST);
}
