Re: Allow SSLProxy* config in context?

[Yann Ylavic]

On Tue, Apr 19, 2016 at 9:36 PM, Yann Ylavic  wrote:
>
> What changed is:
> 1. SSLProxy* directives are now per directory (restricted to
> Server/VirtualHost and ), so all the internal struct members
> have been move from SSLSrvConfigRec to SSLDirConfigRec;
> 2. The merge happens from main server to VirtualHosts (if any) to
>  sections (if any), as usual;
> 3. The proxies SSL_CTX are still created once at startup, in the
> post_config hook, by retrieving all the dc->proxy and initializing
> them (the one associated with the server_rec and the ones of the
>  sections, with the help of a new mod_proxy optional function:
> ap_proxy_get_sections_configs());
> 4. At runtime, the new ssl_proxy_{enable,disable}_ex() optional
> functions are used by mod_proxy to indicate the per dir (proxy
> worker's) configuration to mod_ssl.

I did even more testing with this new (attached) patch and it works for me.

>
> Feedbacks and more testing welcome :)

Let me known if it's suitable for trunk...
Index: include/http_config.h
===
--- include/http_config.h	(revision 1740086)
+++ include/http_config.h	(working copy)
@@ -923,9 +923,10 @@ AP_DECLARE(const char *) ap_check_cmd_context(cmd_
 #define  NOT_IN_LOCATION0x08 /**< Forbidden in Location */
 #define  NOT_IN_FILES   0x10 /**< Forbidden in Files or If*/
 #define  NOT_IN_HTACCESS0x20 /**< Forbidden in .htaccess files */
-/** Forbidden in Directory/Location/FilesIf*/
-#define  NOT_IN_DIR_LOC_FILE(NOT_IN_DIRECTORY|NOT_IN_LOCATION|NOT_IN_FILES)
-/** Forbidden in VirtualHost/Limit/Directory/Location/Files/If */
+#define  NOT_IN_PROXY   0x40 /**< Forbidden in Proxy */
+/** Forbidden in Directory/Location/FilesIfProxy*/
+#define  NOT_IN_DIR_LOC_FILE(NOT_IN_DIRECTORY|NOT_IN_LOCATION|NOT_IN_FILES|NOT_IN_PROXY)
+/** Forbidden in VirtualHost/Limit/Directory/Location/Files/IfProxy*/
 #define  GLOBAL_ONLY(NOT_IN_VIRTUALHOST|NOT_IN_LIMIT|NOT_IN_DIR_LOC_FILE)
 
 /** @} */
Index: modules/http2/h2_h2.c
===
--- modules/http2/h2_h2.c	(revision 1740086)
+++ modules/http2/h2_h2.c	(working copy)
@@ -56,7 +56,6 @@ const char *H2_MAGIC_TOKEN = "PRI * HTTP/2.0\r\n\r
 /***
  * The optional mod_ssl functions we need. 
  */
-static APR_OPTIONAL_FN_TYPE(ssl_engine_disable) *opt_ssl_engine_disable;
 static APR_OPTIONAL_FN_TYPE(ssl_is_https) *opt_ssl_is_https;
 static APR_OPTIONAL_FN_TYPE(ssl_var_lookup) *opt_ssl_var_lookup;
 
@@ -441,7 +440,6 @@ apr_status_t h2_h2_init(apr_pool_t *pool, server_r
 {
 (void)pool;
 ap_log_error(APLOG_MARK, APLOG_TRACE1, 0, s, "h2_h2, child_init");
-opt_ssl_engine_disable = APR_RETRIEVE_OPTIONAL_FN(ssl_engine_disable);
 opt_ssl_is_https = APR_RETRIEVE_OPTIONAL_FN(ssl_is_https);
 opt_ssl_var_lookup = APR_RETRIEVE_OPTIONAL_FN(ssl_var_lookup);
 
Index: modules/http2/mod_proxy_http2.c
===
--- modules/http2/mod_proxy_http2.c	(revision 1740086)
+++ modules/http2/mod_proxy_http2.c	(working copy)
@@ -563,9 +563,9 @@ run_connect:
   "setup new connection: is_ssl=%d %s %s %s", 
   ctx->p_conn->is_ssl, ctx->p_conn->ssl_hostname, 
   locurl, ctx->p_conn->hostname);
-if ((status = ap_proxy_connection_create(ctx->proxy_func, ctx->p_conn,
- ctx->owner, 
- ctx->server)) != OK) {
+status = ap_proxy_connection_create_ex(ctx->proxy_func,
+   ctx->p_conn, ctx->rbase);
+if (status != OK) {
 goto cleanup;
 }
 
Index: modules/proxy/mod_proxy.c
===
--- modules/proxy/mod_proxy.c	(revision 1740086)
+++ modules/proxy/mod_proxy.c	(working copy)
@@ -26,6 +26,10 @@
 #else
 APR_DECLARE_OPTIONAL_FN(int, ssl_proxy_enable, (conn_rec *));
 APR_DECLARE_OPTIONAL_FN(int, ssl_engine_disable, (conn_rec *));
+APR_DECLARE_OPTIONAL_FN(int, ssl_proxy_enable_ex,
+(conn_rec *, ap_conf_vector_t *));
+APR_DECLARE_OPTIONAL_FN(int, ssl_proxy_disable_ex,
+(conn_rec *, ap_conf_vector_t *));
 APR_DECLARE_OPTIONAL_FN(int, ssl_is_https, (conn_rec *));
 APR_DECLARE_OPTIONAL_FN(char *, ssl_var_lookup,
 (apr_pool_t *, server_rec *,
@@ -2313,6 +2317,9 @@ static const char *add_member(cmd_parms *cmd, void
  "Sharing worker '%s' instead of creating new worker '%s'",
  ap_proxy_worker_name(cmd->pool, worker), name);
 }
+if (!worker->section_config) {
+worker->section_config = balancer->section_config;
+}
 
 arr = 

Re: svn commit: r1740084 - in /httpd/httpd/trunk/modules/http2: h2_bucket_beam.c h2_util.c h2_util.h

[Christophe JAILLET]

Le 20/04/2016 11:59, ic...@apache.org a écrit :

Author: icing
Date: Wed Apr 20 09:59:15 2016
New Revision: 1740084

URL: http://svn.apache.org/viewvc?rev=1740084=rev
Log:
compilation fixes for VC

Modified:
 httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
 httpd/httpd/trunk/modules/http2/h2_util.c
 httpd/httpd/trunk/modules/http2/h2_util.h

Modified: httpd/httpd/trunk/modules/http2/h2_bucket_beam.c
URL: 
http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/http2/h2_bucket_beam.c?rev=1740084=1740083=1740084=diff
==
--- httpd/httpd/trunk/modules/http2/h2_bucket_beam.c (original)
+++ httpd/httpd/trunk/modules/http2/h2_bucket_beam.c Wed Apr 20 09:59:15 2016
[...]
@@ -287,7 +287,7 @@ static apr_status_t beam_cleanup(void *d
  h2_bucket_beam *beam = data;
  
  if (beam->live_beam_buckets) {

-ap_log_perror(APLOG_MARK, APLOG_WARNING, 0, beam->life_pool, 
APLOGNO(03383)
+ap_log_perror(APLOG_MARK, APLOG_WARNING, 0, beam->life_pool,
"h2_beam(%d-%s) cleanup with live %d buckets",
beam->id, beam->tag, (int)beam->live_beam_buckets);
  }

Intentional?

CJ

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Roy T. Fielding]

> On Apr 20, 2016, at 4:29 AM, Stefan Eissing  
> wrote:
> 
>> 
>> Am 20.04.2016 um 13:16 schrieb Yann Ylavic :
>> 
>> On Wed, Apr 20, 2016 at 1:09 PM, Yann Ylavic  wrote:
>>> On Wed, Apr 20, 2016 at 11:25 AM, Stefan Eissing
>>>  wrote:
 Done in r1740075.
 
 I was thinking of a nicer solution, but that involved inventing new hooks 
 which seems not worth it.
 
 Since this area of protocol negotiation has already been talked about in 
 regard to TLS upgrades
 and websockets, I do not want to invest in the current way of handling 
 this too much time.
>>> 
>>> I really don't see why we couldn't upgrade to h2 from "http:" (not
>>> "https:" since ALPN did not take place already, or would have done
>>> it).
>>> ISTM that "Upgrade: h2" could be valid in response to a (plain) HTTP/1
>>> request, and the client could upgrade from there...
>> 
>> More on this and Michael's quote of RFC 7540 ("A server MUST ignore an
>> "h2" token...").
>> An HTTP/2 server must indeed ignore the inner HTTP/1 request's
>> "Upgrade: h2" header since it's RFC states it, but and HTTP/1 server
>> (AFAICT) is not concerned by this RFC, and should not...
> 
> Totally agree. And, although untested, in principle we would upgrade
> such a request, as our protocol negotiation framework is somewhat agnostic
> to which RFC can leak water farther than others.

I think we need to be clear when the RFC applies and when it does not.

For example, ANY requirement in the RFC about TLS must be ignored by our
code (but not our default config) because we intend to implement h2c over
anything within some environments.  The RFC was written for the sole
use case of browsers making requests over the open Internet and we should
expect to deviate where its requirements are clearly for that context
rather than for technical reasons.

In any case, RFC7540 does allow an "Upgrade: h2" to be sent by the server
when the connection is already TLS; the fact that it actually means
"Upgrade: HTTP/2.0, TLS" can be handled as we see fit.  There is no
need to be pedantic because we are doing HTTP/1.1 at that point, not h2.

If there is a problem with sending h2 or h2c in a non-TLS response, then
we can send

   Upgrade: HTTP/2.0

instead (with or without the TLS, depending on how we are configured).
It says the same thing as h2c (or h2), as far as HTTP/1.x is concerned.

If that is still a problem with NodeJS, then we should have a conditional
workaround that is limited to the NodeJS client string (including its version);
we cannot allow broken clients to define the protocol in the long term.

Cheers,

Roy

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Stefan Eissing]

Yes, you are correct. Had not covered that test in my test suite...

Fixed in r1740119.

Thanks!

-Stefan

> Am 20.04.2016 um 13:24 schrieb Michael Kaufmann :
> 
>> Done in r1740075.
>> 
> 
> I think that commit introduced a small bug, because the "for" loop is left 
> when "h2" is seen and "report_all" is false. There may be other protocols 
> that are more preferred than the current one.
> 
> 
> Suggested change:
> 
> Index: server/protocol.c
> ===
> --- server/protocol.c (revision 1740112)
> +++ server/protocol.c (working copy)
> @@ -2017,15 +2017,18 @@
>  * existing. (TODO: maybe 426 and Upgrade then?) */
> upgrades = apr_array_make(pool, conf->protocols->nelts + 1,
>   sizeof(char *));
> for (i = 0; i < conf->protocols->nelts; i++) {
> const char *p = APR_ARRAY_IDX(conf->protocols, i, char *);
> /* special quirk for HTTP/2 which does not allow 'h2' to
>  * be part of an Upgrade: header */
> -if (strcmp(existing, p) && strcmp("h2", p)) {
> +if (!strcmp("h2", p)) {
> +continue;
> +}
> +if (strcmp(existing, p)) {
> /* not the one we have and possible, add in this order */
> APR_ARRAY_PUSH(upgrades, const char*) = p;
> }
> else if (!report_all) {
> break;
> }
> }
> 
> 
> Regards,
> Michael
> 

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Stefan Eissing]

> Am 20.04.2016 um 13:16 schrieb Yann Ylavic :
> 
> On Wed, Apr 20, 2016 at 1:09 PM, Yann Ylavic  wrote:
>> On Wed, Apr 20, 2016 at 11:25 AM, Stefan Eissing
>>  wrote:
>>> Done in r1740075.
>>> 
>>> I was thinking of a nicer solution, but that involved inventing new hooks 
>>> which seems not worth it.
>>> 
>>> Since this area of protocol negotiation has already been talked about in 
>>> regard to TLS upgrades
>>> and websockets, I do not want to invest in the current way of handling this 
>>> too much time.
>> 
>> I really don't see why we couldn't upgrade to h2 from "http:" (not
>> "https:" since ALPN did not take place already, or would have done
>> it).
>> ISTM that "Upgrade: h2" could be valid in response to a (plain) HTTP/1
>> request, and the client could upgrade from there...
> 
> More on this and Michael's quote of RFC 7540 ("A server MUST ignore an
> "h2" token...").
> An HTTP/2 server must indeed ignore the inner HTTP/1 request's
> "Upgrade: h2" header since it's RFC states it, but and HTTP/1 server
> (AFAICT) is not concerned by this RFC, and should not...

Totally agree. And, although untested, in principle we would upgrade
such a request, as our protocol negotiation framework is somewhat agnostic
to which RFC can leak water farther than others.

This whole PR came about because NodeJS HTTP client misunderstood the whole
Upgrade: header handling of HTTP/1.1. They though that when the client 
*receives*
as response header Upgrade: that they *needed* to change the protocol. 
With clever carefulness they decided to better not ignore it and fail any 
response that carries an  Upgrade:, irregardless of field value.

This has been fixed now, AFAIK. But, as with all software, a lot of 
installations
out there still have it and thus fail when talking to a new Apache. Since RFC 
7230 
clearly allows Upgrade: in the response, it was argued hat at least "Upgrade: 
h2"
is outside any spec and should go away.

If our release reaches potential users faster than the nodejs update, I do not 
know...

This is the story as I see it. Lots of good intentions from all sides, no real
leap forward for mankind in this protocol layering mix.

-Stefan

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Michael Kaufmann]

Done in r1740075.



I think that commit introduced a small bug, because the "for" loop is  
left when "h2" is seen and "report_all" is false. There may be other  
protocols that are more preferred than the current one.



Suggested change:

Index: server/protocol.c
===
--- server/protocol.c   (revision 1740112)
+++ server/protocol.c   (working copy)
@@ -2017,15 +2017,18 @@
  * existing. (TODO: maybe 426 and Upgrade then?) */
 upgrades = apr_array_make(pool, conf->protocols->nelts + 1,
   sizeof(char *));
 for (i = 0; i < conf->protocols->nelts; i++) {
 const char *p = APR_ARRAY_IDX(conf->protocols, i, char *);
 /* special quirk for HTTP/2 which does not allow 'h2' to
  * be part of an Upgrade: header */
-if (strcmp(existing, p) && strcmp("h2", p)) {
+if (!strcmp("h2", p)) {
+continue;
+}
+if (strcmp(existing, p)) {
 /* not the one we have and possible, add in this order */
 APR_ARRAY_PUSH(upgrades, const char*) = p;
 }
 else if (!report_all) {
 break;
 }
 }


Regards,
Michael

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Yann Ylavic]

On Wed, Apr 20, 2016 at 1:09 PM, Yann Ylavic  wrote:
> On Wed, Apr 20, 2016 at 11:25 AM, Stefan Eissing
>  wrote:
>> Done in r1740075.
>>
>> I was thinking of a nicer solution, but that involved inventing new hooks 
>> which seems not worth it.
>>
>> Since this area of protocol negotiation has already been talked about in 
>> regard to TLS upgrades
>> and websockets, I do not want to invest in the current way of handling this 
>> too much time.
>
> I really don't see why we couldn't upgrade to h2 from "http:" (not
> "https:" since ALPN did not take place already, or would have done
> it).
> ISTM that "Upgrade: h2" could be valid in response to a (plain) HTTP/1
> request, and the client could upgrade from there...

More on this and Michael's quote of RFC 7540 ("A server MUST ignore an
"h2" token...").
An HTTP/2 server must indeed ignore the inner HTTP/1 request's
"Upgrade: h2" header since it's RFC states it, but and HTTP/1 server
(AFAICT) is not concerned by this RFC, and should not...

>
> Regards,
> Yann.
>
> PS: feel free to ignore this message since that's already ruled :)

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Yann Ylavic]

On Wed, Apr 20, 2016 at 11:25 AM, Stefan Eissing
 wrote:
> Done in r1740075.
>
> I was thinking of a nicer solution, but that involved inventing new hooks 
> which seems not worth it.
>
> Since this area of protocol negotiation has already been talked about in 
> regard to TLS upgrades
> and websockets, I do not want to invest in the current way of handling this 
> too much time.

I really don't see why we couldn't upgrade to h2 from "http:" (not
"https:" since ALPN did not take place already, or would have done
it).
ISTM that "Upgrade: h2" could be valid in response to a (plain) HTTP/1
request, and the client could upgrade from there...

Regards,
Yann.

PS: feel free to ignore this message since that's already ruled :)

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Michael Kaufmann]

Zitat von Stefan Eissing :


Done in r1740075.

I was thinking of a nicer solution, but that involved inventing new  
hooks which seems not worth it.


Since this area of protocol negotiation has already been talked  
about in regard to TLS upgrades and websockets, I do not want to  
invest in the current way of handling this too much time.


-Stefan


Thank you Stefan. Also thanks to everyone who took part in the  
discussion. This topic is way more complex than I thought.


Regards,
Michael

Re: "Upgrade: h2" header for HTTP/1.1 via TLS (Bug 59311)

[Stefan Eissing]

Done in r1740075.

I was thinking of a nicer solution, but that involved inventing new hooks which 
seems not worth it.

Since this area of protocol negotiation has already been talked about in regard 
to TLS upgrades and websockets, I do not want to invest in the current way of 
handling this too much time.

-Stefan

> Am 20.04.2016 um 05:35 schrieb William A Rowe Jr :
> 
> On Tue, Apr 19, 2016 at 10:59 AM, Stefan Eissing 
>  wrote:
> 
> > Am 19.04.2016 um 17:47 schrieb William A Rowe Jr :
> >
> > I agree with your analysis, "h2" is not an upgrade candidate.
> >
> > "h2c" is an upgrade candidate.
> >
> > This isn't even an HTTP/2 issue (unless the working group reverses 
> > themselves
> > on accepting Upgrade: h2 protocol switching), until we accept Upgrade: h2 we
> > should be dropping h2 from the server Upgrade: response header.  But do let
> > us know what the wg feedback is.
> 
> While I do not feel strongly about this feature, I'd like to add that the 
> "Upgrade: h2" is sent out as that very mechanism is available to a client. 
> And I do not feel strongly because I do not know of a client that might be 
> able to use it. It is just the result of a sane software architecture that 
> this has become visible.
> 
> We would probably not be talking about this if some Javascript client 
> implementation had not consciously decided to freak out on *any* Upgrade: 
> header from the server.
> 
> If http-wg thinks that it should not be visible, I'll add the extra 'if' to 
> our implementation.
> 
> Skimming the responses, they just keep getting more and more amusing, and
> shining a candle to the absurdity of keeping this non-sequitur request 
> response. 
> 
> Could you go ahead and add that conditional?
> 
> We still haven't determined if the "reply Upgrade: once, then pretend we 
> didn't"
> is valid HTTP/1.1, which I read that it is not.  Need to come back to that 
> topic.
> 
> Cheers,
> 
> Bill