Not at my machine to really check the runtime behaviour. Can do that tomorrow. My thoughts so far:
- the return code was not checked intentionally. Once the 101 intermediate response is sent, there is nothing core can do on the connection. It has been switched to a protocol unknown to core. Failure in the protocol handler won't make it HTTP/1 again. If we do not close the connection afterwards, but try to read the next h1 request, we just expose ourselves to someone inserting payload that tricks us. Think about CORS restrictions and other stuff. - I think its the protocol handlers job to deal with any request body. core does not need to do anything here. For h2 upgrade, for example, the switch is only offered for requests without body. That exposes more predictable behaviour to clients than ignoring the upgrade on some arbitrary number of bytes. Other protocols might be fine with bodies, h2 is not. This was a consensus on the http wg mailing list some months ago. - moving things to post read sounds tempting, however I'm not sure if we want to upgrade on non-authed request or not, for example. I am not sure what else we do in post read, maybe someone else has an opinion about that. It certainly looks nicer in the OPTIONS * case. //stefan > Am 07.12.2015 um 19:02 schrieb William A Rowe Jr <wr...@rowe-clan.net>: > > Here's my thoughts to address this rather quickly for 2.4.18 to keep > ourselves on track; simply move the core protocol handling phase and > ap_switch_protocol action into the appropriate request processing phase, > address the fail-cases of the ap_switch_protocol exceptions that have > been overlooked so that users can start leveraging the API, and just fix > the mod_ssl OPTIONS * advertisement as presented on trunk (I am > updating STATUS in a moment for that patch). > > The actual fix to incorporate mod_ssl TLS upgrade to work within the > Protocol API can wait, so we don't further disrupt the overdue fixes > for the mod_http2 feature for our users. Although I would -like- them > to coexist nicely, I don't believe that must happen until 2.4.19. If the > SSLOptions optional works within one vhost, and Protocol h2c works > on another vhost, I think we are in good shape for the experimental > new feature release. > > WDTY? > > >> On Mon, Dec 7, 2015 at 10:38 AM, William A Rowe Jr <wr...@rowe-clan.net> >> wrote: >> Hi folks, sorry for the late interruption after we have already shipped >> 2.4.16, but there seems to be an issue that merits revisiting before the >> 2.4.16 API schema is widely adopted. >> >> We seem to have misplaced the upgrade handler in the wrong hook. This is >> easily shown by the fact that we had two different upgrade paths between >> mod_ssl TLS upgrade, and h2c upgrade paths, and a crufty patch to solve the >> OPTIONS * request exception which isn't an exception at all per RFC7230. >> >> In mod_ssl connection upgrade we once emitted the TLS upgrade header in >> fixups. Fixups are bypassed by OPTIONS *. My trunk patch moved this >> mod_ssl TLS upgrade handling to post read req, leaving OPTIONS * with its >> simple behavior. >> >> In h2c this exists in an upgrade handler, and OPTIONS * was hacked to invoke >> this function. Problem, it is a transport (hop by hop) detail and should >> not be presented in the request, but rather in the connection hooks. >> Moreover, none of the other *request* processing phases appear valid prior >> to conducting the h2c upgrade, and some appear that they can be harmful to >> the admin attempting to configure Protocol h2c. >> >> The relevant specs are clear, it is a hop-by-hop connection transport offer >> carried over the request headers. It is not generally subject to auth >> processing, certainly not resource processing. The underlying request must >> still be satisfied. The current Protocols logic doesn't appear to do this >> because it was based off of tangential specs such as h2c that don't override >> 2616bis. If we consider that a "method" has a "handler", and transfer >> "headers" are "filtered" it becomes more obvious. This is not the UPGRADE >> method. >> >> https://tools.ietf.org/html/rfc7230#section-6.7 makes things more >> interesting, it calls out that 101-continue and the request body read >> precedes the 101-switching protocols. Not sure who decided that would be a >> good idea, sigh... but mod_ssl TLS upgrade has these reversed for several >> good reasons including the intent to encrypt the request body if present and >> simple economics of processing. >> >> I think that handling upgrade advertisement and alerting must be in post >> read req, bypassing all request hooks until the 101-continue is presented, >> any small request body read and set aside for the http input brigade, and >> 101-switching protocols is presented. This allows the request to still be >> processed for tls-style upgrades, or discarded for relevant protocols. >> >> The beginning of a patch is attached, I note that the ap_switch_protocol >> return value was never properly interpreted and my patch doesn't begin to >> catch all of the exceptional cases. Because 101-switching protocols has >> already been sent, if the call fails to perform an upgrade, it seems we need >> to respond with a generic 400 switch failed unless the registered protocol >> switch handler is going to ap_die with a different sort of specific error >> (e.g. "A server MUST NOT switch protocols unless the received message >> semantics can be honored by the new protocol; an OPTIONS request can be >> honored by any protocol.") >> >> Stefan and other protocol gurus, please have a look at the proposed patch, >> thanks. >> >> Bill >> >
