Mailing-List: contact dev-help@httpd.apache.org; run by ezmlm
Precedence: bulk
Reply-To: dev@httpd.apache.org
Received-SPF: pass (nike.apache.org: local policy includes SPF record at
 spf.trusted-forwarder.org)
Date: Wed, 13 Nov 2013 15:50:47 -0600
From: "William A. Rowe Jr." <wrowe@rowe-clan.net>
To: dev@httpd.apache.org
Cc: ylavic.dev@gmail.com
Subject: Re: http_filter.c r1524770 open issue?
Message-ID: <20131113155047.10eb4760@hub>
In-Reply-To: 
 <CAKQ1sVOLYRyLeE6_gR2g=5YWVvCC_v_0UnZBr6TYPukHFK+jYQ@mail.gmail.com>
References: <20131113012515.4ccb8564@hub>
	<CAKQ1sVMTFQ-5mUKiyzUMv2GCWPYT_NjQLqPry0kVoD0qGtdGLw@mail.gmail.com>
	<CAAR2etNX6agTfbCBgn2N36SKX+w8DQ8wo7_ewuX53H_NMOh_aQ@mail.gmail.com>
	<CAKQ1sVOLYRyLeE6_gR2g=5YWVvCC_v_0UnZBr6TYPukHFK+jYQ@mail.gmail.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: quoted-printable

On Wed, 13 Nov 2013 22:19:37 +0100
Yann Ylavic <ylavic.dev@gmail.com> wrote:

> On Wed, Nov 13, 2013 at 5:16 PM, William A. Rowe Jr.
> <wmrowe@gmail.com>wrote:
>=20
> >
> > On Nov 13, 2013 8:22 AM, "Yann Ylavic" <ylavic.dev@gmail.com> wrote:
> > >
> > > On Wed, Nov 13, 2013 at 8:25 AM, William A. Rowe Jr. <
> > wrowe@rowe-clan.net> wrote:
> > >>
> > >> Looking at the (f->r->proxyreq =3D=3D PROXYREQ_RESPONSE) code path,
> > >> the comments note;
> > >>
> > >>  * http://tools.ietf.org/html/draft-ietf-httpbis-p1-messaging-23
> > >>  * Section 3.3.3.3: "If a Transfer-Encoding header field is
> > >>  * present in a response and the chunked transfer coding is not
> > >>  * the final encoding, the message body length is determined by
> > >>  * reading the connection until it is closed by the server."
> > >>
> > >> All well and good.  However, that statement makes almost no
> > >> sense if the response is not Connection: close (or http/1.0,
> > >> absent keep-alive).
> > >>
> > >>                  ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, f->r,
> > >>                  APLOGNO(01586) "Unknown Transfer-Encoding: %s;"
> > >>                                 " using read-until-close");
> > >>
> > >> Here we've unset C-L and T-E. but it makes no sense to wait if
> > >> the origin server has no immediate plan to close the connection.
> > >>
> > >> Jim, Yann, was this case thought through?  It seems premature to
> > >> commit the backport without considering that edge case.
> > >
> > >
> > > When the origin gives no C-L and no T-E, ap_http_filter() already
> > assumes a read-until-close, even if "Connection: close" is not
> > specified, as per "rfc2616 - Section 4.4 - Message Length - =C2=A75", or
> > "draft-ietf-httpbis-p1-messaging-24 - Section 3.3.3 - Message Body
> > Length - =C2=A77".
> > >
> > > IMHO, this is the same case here, if the origin gives a T-E which
> > > is not
> > ending with "chunked" (something likely shared with the client), it
> > is supposed to close the connection when done (as per
> > rfc/ietf-httpbis), and the filter has to trust that...
> >
> > Except, in -this- case, we unset the C-L returned by the origin
> > server.
> >
> This is what is required by the rfc (and draft-httpbis): when both
> C-L and T-E are received, the former must be ignored (by the reader)
> and removed (by the sender, proxy case).
>=20
> Actually r1524770 does remove the "spurious" C-L in ap_read_request
> (where, anyhow, a non-chunked T-E is not allowed from the client),
> but not in ap_http_filter where it is just ignored (read-until-close
> is used instead, but [backend->]r->headers_in is untouched).
>=20
> The one that will unset the C-L is ap_proxy_http_process_response
> (after backend->r->headers_in are merged into r->headers_out), as per
> (line 1429) :
>=20
>             /* can't have both Content-Length and Transfer-Encoding */
>             if (apr_table_get(r->headers_out, "Transfer-Encoding")
>                     && apr_table_get(r->headers_out,
> "Content-Length")) { /*
>                  * 2616 section 4.4, point 3: "if both
> Transfer-Encoding
>                  * and Content-Length are received, the latter MUST be
>                  * ignored";
>                  *
>                  * To help mitigate HTTP Splitting, unset
> Content-Length
>                  * and shut down the backend server connection
>                  * XXX: We aught to treat such a response as
> uncachable */
>                 apr_table_unset(r->headers_out, "Content-Length");
>                 ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> APLOGNO(01107) "server %s:%d returned Transfer-Encoding"
>                               " and Content-Length",
>                               backend->hostname, backend->port);
>                 backend->close =3D 1;
>             }
>=20
> But that lasts long before r1524770, and I think the core http filter
> should do the same to be RFC compliant "by itself"...
>=20
> Anyway, the origin can send a non-chunked T-E and close the
> connection when done if it is sharing a special coding with the
> client, but it cannot send both a non-chunked T-E and a C-L for the
> latter to be used, the RFC says it will not in any case.
>=20
> I don't see any compatibility issue here, if the origin has no
> immediate plan to close the connection, it should use either a T-E
> (ultimately) chunked or a C-L alone, as it would without a special
> T-E...

Yann,

you just reiterated everything I've already conceded.  You didn't
mention keep-alives once.  You used the word 'trust' before, but that
is something not easily assigned if server resources are carelessly
abused.  Please keep in mind, not all mod_proxy applications are
reverse proxies, and this exception is problematic for arbitrary
origin servers requested by the client.

I'll accept the silence as fact that this was not considered and will
go ahead and veto the backports for the time being, until we add the
appropriate guard against such origin server keepalive behavior.  I
would expect that patch to be very straightforward (trivial in simply
dropping the response if keepalive is requested) and will start looking
at it after finishing my reviews of various apr/httpd releases here.

Bill