You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by Ruediger Pluem <rp...@apache.org> on 2020/05/13 06:37:28 UTC
Re: svn commit: r1877646 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c
modules/proxy/mod_proxy_wstunnel.c modules/proxy/proxy_util.c
On 5/12/20 2:20 PM, ylavic@apache.org wrote:
> Author: ylavic
> Date: Tue May 12 12:20:57 2020
> New Revision: 1877646
>
> URL: http://svn.apache.org/viewvc?rev=1877646&view=rev
> Log:
> mod_proxy_http: handle Upgrade requests and upgraded protocol forwarding.
>
> If the request Upgrade header matches the worker upgrade= parameter and
> the backend switches the protocol, do the tunneling in mod_proxy_http.
> This allows to keep the protocol to HTTP until the backend really
> switches the protocol, and apply usual output filters.
>
> When configured to forward Upgrade mechanism, we want the backend to be
> able to announce its Upgrade protocol to the client (e.g. with 426
> Upgrade Required response) and thus forward back the Upgrade header that
> matches the one(s) configured in the worker upgrade= parameter.
>
> modules/proxy/mod_proxy.h:
> modules/proxy/proxy_util.c:
> ap_proxy_worker_can_upgrade(): added helper to determine whether a
> proxy worker is configured to forward an Upgrade protocol.
>
> include/ap_mmn.h:
> Bump MMN minor for ap_proxy_worker_can_upgrade().
>
> modules/proxy/mod_proxy.c:
> set_worker_param(): handle worker parameter upgrade=ANY as upgrade=*
> (should the "any" protocol scheme be something some day..).
>
> modules/proxy/mod_proxy_wstunnel.c:
> proxy_wstunnel_handler(): use ap_proxy_worker_can_upgrade() to match
> the Upgrade header. Axe handling of upgrade=NONE, it makes no sense to
> Upgrade a connection if the client did not ask for it, nor to configure
> mod_proxy_wstunnel to use a worker with upgrade=NONE by the way.
>
> modules/proxy/mod_proxy_http.c:
> proxy_http_req_t: add fields force10 (force HTTP/1.0) and upgrade (value
> of the Upgrade header sent by the client if it matches the configuration,
> NULL otherwise).
> proxy_http_handler(): use ap_proxy_worker_can_upgrade() to determine
> whether the request is electable for end to end protocol upgrading and set
> req->upgrade accordingly.
> terminate_headers(): handle Connection and Upgrade headers to send to the
> backend, according to req->force10 and req->upgrade set before.
> ap_proxy_http_prefetch(): use req->force10 and terminate_headers().
> send_continue_body(): added helper to send the body retained for end to
> end 100-continue handling.
> ap_proxy_http_process_response(): use ap_proxy_worker_can_upgrade() to
> match the response Upgrade header and forward it back if it matches the
> configured one(s). That is for 101 Switching Protocol obviously but also
> any other status code which is not overidden, at the backend wish. If the
> protocol is switching, create a proxy tunnel and run it, using the minimal
> timeout from the client or backend connection.
>
> Github: closes #125
>
>
> Modified:
> httpd/httpd/trunk/CHANGES
> httpd/httpd/trunk/docs/log-message-tags/next-number
> httpd/httpd/trunk/include/ap_mmn.h
> httpd/httpd/trunk/modules/proxy/mod_proxy.c
> httpd/httpd/trunk/modules/proxy/mod_proxy.h
> httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> httpd/httpd/trunk/modules/proxy/mod_proxy_wstunnel.c
> httpd/httpd/trunk/modules/proxy/proxy_util.c
>
> Modified: httpd/httpd/trunk/modules/proxy/mod_proxy_http.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/proxy/mod_proxy_http.c?rev=1877646&r1=1877645&r2=1877646&view=diff
> ==============================================================================
> --- httpd/httpd/trunk/modules/proxy/mod_proxy_http.c (original)
> +++ httpd/httpd/trunk/modules/proxy/mod_proxy_http.c Tue May 12 12:20:57 2020
> @@ -1509,6 +1564,7 @@ int ap_proxy_http_process_response(proxy
> ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> "HTTP: received interim %d response", r->status);
> if (!policy
> + || upgrade
Shouldn't that be (upgrade && proxy_status == HTTP_SWITCHING_PROTOCOLS) in case someone sends a upgrade header
without status HTTP_SWITCHING_PROTOCOLS?
Furthermore don't we need to add in back the upgrade header and the Connection: upgrade before we send the interim
response with HTTP_SWITCHING_PROTOCOLS?
> || (!strcasecmp(policy, "RFC")
> && (proxy_status != HTTP_CONTINUE
> || (req->expecting_100 = 1)))) {
> @@ -1615,6 +1649,67 @@ int ap_proxy_http_process_response(proxy
> do_100_continue = 0;
> }
>
> + if (proxy_status == HTTP_SWITCHING_PROTOCOLS) {
> + apr_status_t rv;
> + proxy_tunnel_rec *tunnel;
> + apr_interval_time_t client_timeout = -1,
> + backend_timeout = -1;
> +
> + /* If we didn't send the full body yet, do it now */
> + if (do_100_continue) {
> + req->expecting_100 = 0;
> + status = send_continue_body(req);
Can we really have a body that we held back in case the backend sends HTTP_SWITCHING_PROTOCOLS?
IMHO if we have a body held back we sent an Expect 100-Continue header with our request to the backend
and the backend should have replied with 100-Continue and not with HTTP_SWITCHING_PROTOCOLS
> + if (status != OK) {
> + return status;
> + }
> + }
> +
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(10239)
> + "HTTP: tunneling protocol %s", upgrade);
> +
> + rv = ap_proxy_tunnel_create(&tunnel, r, origin, "HTTP");
> + if (rv != APR_SUCCESS) {
> + ap_log_rerror(APLOG_MARK, APLOG_ERR, rv, r, APLOGNO(10240)
> + "can't create tunnel for %s", upgrade);
> + return HTTP_INTERNAL_SERVER_ERROR;
> + }
> +
> + /* Set timeout to the lowest configured for client or backend */
> + apr_socket_timeout_get(backend->sock, &backend_timeout);
> + apr_socket_timeout_get(ap_get_conn_socket(c), &client_timeout);
> + if (backend_timeout >= 0 && backend_timeout < client_timeout) {
> + tunnel->timeout = backend_timeout;
> + }
> + else {
> + tunnel->timeout = client_timeout;
> + }
> +
> + /* Bidirectional non-HTTP stream will confuse mod_reqtimeoout, we
> + * use a single idle timeout from now on.
> + */
> + ap_remove_input_filter_byhandle(c->input_filters, "reqtimeout");
> +
> + /* Let proxy tunnel forward everything */
> + status = ap_proxy_tunnel_run(tunnel);
> + if (ap_is_HTTP_ERROR(status)) {
> + /* Tunnel always return HTTP_GATEWAY_TIME_OUT on timeout,
> + * but we can differentiate between client and backend here.
> + */
> + if (status == HTTP_GATEWAY_TIME_OUT
> + && tunnel->timeout == client_timeout) {
> + status = HTTP_REQUEST_TIME_OUT;
> + }
> + }
> + else {
> + /* Update r->status for custom log */
> + status = HTTP_SWITCHING_PROTOCOLS;
> + }
> + r->status = status;
> +
> + /* We are done with both connections */
> + return DONE;
> + }
> +
> if (interim_response) {
> /* Already forwarded above, read next response */
> continue;
> @@ -1666,6 +1761,12 @@ int ap_proxy_http_process_response(proxy
> return proxy_status;
> }
>
> + /* Forward back Upgrade header if it matches the configured one(s). */
> + if (upgrade && ap_proxy_worker_can_upgrade(p, worker, upgrade)) {
> + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> + apr_table_setn(r->headers_out, "Upgrade", apr_pstrdup(p, upgrade));
> + }
> +
Hmm. When do we hit this? When the backend sends HTTP_UPGRADE_REQUIRED? And if yes shouldn't we only care in this case?
> r->sent_bodyct = 1;
> /*
> * Is it an HTTP/0.9 response or did we maybe preread the 1st line of
Regards
Rüdiger
Re: svn commit: r1877646 - in /httpd/httpd/trunk: CHANGES
docs/log-message-tags/next-number include/ap_mmn.h modules/proxy/mod_proxy.c
modules/proxy/mod_proxy.h modules/proxy/mod_proxy_http.c modules/proxy/mod_proxy_wstunnel.c
modules/proxy/proxy_util.c
Posted by Yann Ylavic <yl...@gmail.com>.
On Wed, May 13, 2020 at 8:37 AM Ruediger Pluem <rp...@apache.org> wrote:
>
> On 5/12/20 2:20 PM, ylavic@apache.org wrote:
>
> > @@ -1509,6 +1564,7 @@ int ap_proxy_http_process_response(proxy
> > ap_log_rerror(APLOG_MARK, APLOG_TRACE2, 0, r,
> > "HTTP: received interim %d response", r->status);
> > if (!policy
> > + || upgrade
>
> Shouldn't that be (upgrade && proxy_status == HTTP_SWITCHING_PROTOCOLS) in case someone sends a upgrade header
> without status HTTP_SWITCHING_PROTOCOLS?
Yes, "upgrade" was first set only for HTTP_SWITCHING_PROTOCOLS so this
test was enough, but then a later commit (on github) allowed it to be
forwarded for other status codes (as you caught below).
Fixed in r1877695.
> Furthermore don't we need to add in back the upgrade header and the Connection: upgrade before we send the interim
> response with HTTP_SWITCHING_PROTOCOLS?
Of course, fixed in r1877695 too.
> >
> > + /* Forward back Upgrade header if it matches the configured one(s). */
> > + if (upgrade && ap_proxy_worker_can_upgrade(p, worker, upgrade)) {
> > + apr_table_setn(r->headers_out, "Connection", "Upgrade");
> > + apr_table_setn(r->headers_out, "Upgrade", apr_pstrdup(p, upgrade));
> > + }
> > +
>
> Hmm. When do we hit this? When the backend sends HTTP_UPGRADE_REQUIRED? And if yes shouldn't we only care in this case?
Yes, HTTP_UPGRADE_REQUIRED or any status actually (unless
ProxyErrorOverride takes place).
We want to allow Upgrade negotiation between the client and the
backend (or the other way around), and I'm not sure that
HTTP_UPGRADE_REQUIRED is the only way to do that, so provided the
upgrade= configuration includes the protocol I think we should forward
it.
Regards;
Yann.