On Wed, Feb 26, 2014 at 9:45 PM, Ruediger Pluem <[email protected]> wrote:
> > > Yann Ylavic wrote: > > On Mon, Feb 24, 2014 at 10:05 AM, Ruediger Pluem <[email protected]> > wrote: > >> > >> Yann Ylavic wrote: > >>> There seem to be different questions in this thread regarding SNI. > >>> Maybe we can enumerate them first to see what's going on (at least I > need to) > >>> > >>> 1. What should the client-provided SNI be checked against? > >>> 1.1. for server or proxy-reverse > >>> 1.2. for proxy-forward/CONNECT > >>> > >>> Possibilities are : > >>> 1.a. Host: header > >>> 1.b. Request-URI's hostname > >>> 1.c. ServerName/ServerAlias(es) > >>> 1.d. ap_get_server_name_for_url() > >>> 1.e. none > >>> > >>> 2. What should the proxy-provided SNI be? > >>> 2.1. when ProxyPreserveHost is off > >>> 2.2. when ProxyPreserveHost is on > >>> > >>> Possibilities are : > >>> 2.a. proxy worker's hostname > >>> 2.b. r->hostname > >>> 2.c. Host: header (being sent) > >>> > >>> 3. What should the backend's CN be checked against? > >>> > >>> 4. How should the proxy reuse SNI connections? > >>> > >>> Currently : > >>> 1.1 => 1.b if any, otherwise 1.a > >>> 1.2 => 1.e > >>> 2.1 => 2.a > >>> 2.2 => 2.b > >>> 3 => the proxy-provided SNI (or wildcard match) > >>> 4 => always > >>> > >>> > >>> I propose to use the following instead : > >>> > >>> 1.1 => 1.d > >>> So that the admin can configure UseCanonicalName to branch 1.b-a or > 1.c. > >> > >> Host header and SNI should be consistent IMHO. So I think the current > setup is correct. > > > > I tend to agree but, as you can see, people have existing/working > > configurations where they don't use vhosting by SNI (backend side), > > and/or authenticate the backend based on the requested Host (proxy > > side): they use SSL at connection level only. > > Even if they use IP/Port based virtual hosting the SNI name and supplied > host header should be consistent. > Maybe we can add a "don't use it" directive here that turns off this > check. But I am not in favor of this. > IMHO it is a bug of the client if both things are not consistent. > How about my proposal to add the new ProxyPreserveHost and SSLProxyCheckPeerCN/Name "canon" value. With "ProxyPreserveHost canon" the Host header and SNI would be both the ServerName (which is consistent, I'm not arguing to send different values here). With "SSLProxyCheckPeerCN/Name canon" the CN/AltName would be checked against the ServerName (whatever the Host/SNI sent). That way the admin can use the ServerName for "SSL at connection level", which is not as if it was a freely configurable, and IMHO is consistent. > > > > By checking (client side) and setting (proxy side) the SNI > > unconditionally (based on the given Host), it's almost as if we forbid > > access to the default vhost (on some port) when the requested Host > > does not match any of its ServerName/ServerAlias(es). > > I don't see that this is the case from the existing code. IMHO we compare > two different data pieces the > client supplied for consistency. > > > AFAICT, httpd won't block anything like that by its own, that's what I > > meant by "hot potato" and "should be denied by httpd/mod_proxy before > > forwarding" (below). > > > > Why would it be different in the SSL+SNI case if the admin knows what > > (s)he does and can't use anything than the requested Host, the > > ServerName or the worker hostname? > > > >> > > > > >> > >>> > >>> 2.1 => 2.c > >>> 2.2 => 2.c > >>> Both should use the Host: header since this is what the backend will > check. > >>> Either the Host: is legitimate and we can forward it, or it should be > >>> denied by the proxy before forwarding (that's kind of hot potato sent > >>> to the backend otherwise). > >> > >> I strongly disagree. In the 2.1 case we need to set the host header and > SNI (provided no IP) as requested > >> by the admin. This configuration is usually on purpose and takes into > account that the internal servers have > >> different DNS names and a matching certificate / virtual host setup. > >> Regarding 2.2 I think that 2.b is still the correct choice as we use a > sanitized and normalized version of the > >> host header here. > > > > Sorry I mispoke here. > > I didn't mean to use the user-agent's requested Host as SNI in both > > cases, but using the same Host as the one set by > > ap_proxy_create_hdrbrgd() (the one that will finally reach the > > backend), ie : > > > > if (dconf->preserve_host == 0) { > > if (ap_strchr_c(uri->hostname, ':')) { /* if literal IPv6 > address */ > > if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) { > > buf = apr_pstrcat(p, "Host: [", uri->hostname, "]:", > > uri->port_str, CRLF, NULL); > > } else { > > buf = apr_pstrcat(p, "Host: [", uri->hostname, "]", > CRLF, NULL); > > } > > } else { > > if (uri->port_str && uri->port != DEFAULT_HTTP_PORT) { > > buf = apr_pstrcat(p, "Host: ", uri->hostname, ":", > > uri->port_str, CRLF, NULL); > > } else { > > buf = apr_pstrcat(p, "Host: ", uri->hostname, CRLF, > NULL); > > } > > } > > } > > else { > > /* don't want to use r->hostname, as the incoming header might > have a > > * port attached > > */ > > const char* hostname = apr_table_get(r->headers_in,"Host"); > > if (!hostname) { > > hostname = r->server->server_hostname; > > ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r, APLOGNO(01092) > > "no HTTP 0.9 request (with no host line) " > > "on incoming request and preserve host set " > > "forcing hostname to be %s for uri %s", > > hostname, r->uri); > > } > > buf = apr_pstrcat(p, "Host: ", hostname, CRLF, NULL); > > } > > ap_xlate_proto_to_ascii(buf, strlen(buf)); > > e = apr_bucket_pool_create(buf, strlen(buf), p, c->bucket_alloc); > > APR_BRIGADE_INSERT_TAIL(header_brigade, e); > > > > The code above and the one from proxy_http_handler() below (which sets > > the SNI via "proxy-request-hostname") are not consistent in all cases > > It becomes consistent as > > - We do not put a port in our SNI request as neither uri->hostname nor > r->hostname have one. > - We do only compare stuff without ports below. > > > : > > > > if (is_ssl) { > > proxy_dir_conf *dconf; > > const char *ssl_hostname; > > > > /* > > * In the case of ProxyPreserveHost on use the hostname > of > > * the request if present otherwise use the one from the > > * backend request URI. > > */ > > dconf = ap_get_module_config(r->per_dir_config, > &proxy_module); > > if ((dconf->preserve_host != 0) && (r->hostname != > NULL)) { > > ssl_hostname = r->hostname; > > } > > else { > > ssl_hostname = uri->hostname; > > } > > > > apr_table_set(backend->connection->notes, > > "proxy-request-hostname", > > ssl_hostname); > > } > > > > Also, for proxied subrequests, r->hostname is not always "up to date". > > What I mean is that there should be one forwarded Host value (say the > > Can you give an example? > What I mean is that for example, with ProxyPreserveHost on, if one (module) creates a subrequest to send it to another (SSL) backend than the original one, not only will (s)he have to set subr's Host header to the corresponding value, but also subr->hostname accordingly. Of couse (s)he should know what is done, and AFAIK there's no way to do this by httpd configuration, but that's an example of r->hostname and Host header (without the port) possible inconsistency. The codes above won't do the right thing in this case. No so common I agree, but since subrequests exist... > > > "proxy-request-hostname" note), computed once (according to > > ProxyPreserveHost on/off/canon?), and used everywhere (header, SNI, > > CheckPeerCN/Name...). > > > >> > > >> > >>> > >>> 4 => compare reusable connection's hostname with 2.c (when > conn->is_ssl only) > >> > >> This could be a worthwhile idea and I have seen your patch in bugzilla. > >> For ease of commenting I would like you to post it here such that > inline comments could > >> be made (I would have some). > > > > Here it is (thanks for looking into this). > > > > Index: modules/proxy/mod_proxy.h > > =================================================================== > > --- modules/proxy/mod_proxy.h (revision 1569048) > > +++ modules/proxy/mod_proxy.h (working copy) > > @@ -242,6 +242,7 @@ typedef struct { > > apr_pool_t *pool; /* Subpool for hostname and addr data */ > > const char *uds_path; /* Unix domain socket path */ > > const char *hostname; > > + const char *ssl_hostname; > > As already pointed by Bill: Put it at the end of the struct to keep it > backportable (only minor bump required in this case). > I'll do this for the commit. > > > apr_sockaddr_t *addr; /* Preparsed remote address info */ > > apr_pool_t *scpool; /* Subpool used for socket and > > connection data */ > > apr_socket_t *sock; /* Connection socket */ > > Index: modules/proxy/mod_proxy_http.c > > =================================================================== > > --- modules/proxy/mod_proxy_http.c (revision 1569048) > > +++ modules/proxy/mod_proxy_http.c (working copy) > > @@ -1916,6 +1916,7 @@ static int proxy_http_handler(request_rec *r, prox > > ap_proxy_ssl_connection_cleanup(backend, r); > > } > > > > + > > /* > > * In the case that we are handling a reverse proxy connection and > this > > * is not a request that is coming over an already kept alive > connection > > @@ -1958,25 +1959,10 @@ static int proxy_http_handler(request_rec *r, > prox > > * requested, such that mod_ssl can check if it is > requested to do > > * so. > > */ > > - if (is_ssl) { > > - proxy_dir_conf *dconf; > > - const char *ssl_hostname; > > - > > - /* > > - * In the case of ProxyPreserveHost on use the hostname > of > > - * the request if present otherwise use the one from the > > - * backend request URI. > > - */ > > - dconf = ap_get_module_config(r->per_dir_config, > &proxy_module); > > - if ((dconf->preserve_host != 0) && (r->hostname != > NULL)) { > > - ssl_hostname = r->hostname; > > - } > > - else { > > - ssl_hostname = uri->hostname; > > - } > > - > > - apr_table_set(backend->connection->notes, > > "proxy-request-hostname", > > - ssl_hostname); > > + if (backend->ssl_hostname) { > > + apr_table_setn(backend->connection->notes, > > + "proxy-request-hostname", > > + backend->ssl_hostname); > > } > > > > /* Step Three-and-a-Half: See if the socket is still > connected (if > > Index: modules/proxy/proxy_util.c > > =================================================================== > > --- modules/proxy/proxy_util.c (revision 1569048) > > +++ modules/proxy/proxy_util.c (working copy) > > > @@ -2284,6 +2285,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque > > conn->pool); > > } > > socket_cleanup(conn); > > + conn->close = 0; > > This one makes sense, but is not related to the contents of this patch. > Please commit it separately. > No pb, that was not intentional to put it in this patch, just a remaining working diff... Done in r1572561. > > } > > if (will_reuse) { > > /* > > @@ -2345,6 +2347,35 @@ ap_proxy_determine_connection(apr_pool_t *p, reque > > return ap_proxyerror(r, HTTP_FORBIDDEN, > > "Connect to remote machine blocked"); > > } > > + /* > > + * When SSL is configured, determine the SNI for the request and > > + * save it in conn->ssl_hostname. Close any reused connection whose > > + * SNI differs. > > + */ > > + if (conn->is_ssl) { > > + proxy_dir_conf *dconf; > > + const char *ssl_hostname; > > + /* > > + * In the case of ProxyPreserveHost on use the hostname of > > + * the request if present otherwise use the one from the > > + * backend request URI. > > + */ > > + dconf = ap_get_module_config(r->per_dir_config, &proxy_module); > > + if (dconf->preserve_host) { > > + ssl_hostname = r->hostname; > > + } > > + else { > > + ssl_hostname = conn->hostname; > > + } > > + if (conn->ssl_hostname != NULL && > > + (!ssl_hostname || strcasecmp(conn->ssl_hostname, > > + ssl_hostname) != 0)) { > > + socket_cleanup(conn); > > Shouldn't we set conn->close = 0 here as well? > I don't think so, conn->close is always 0 here since the code : /* Close a possible existing socket if we are told to do so */ if (conn->close) { socket_cleanup(conn); conn->close = 0; } is run just before. This code (socket_cleanup) is only executed if conn->close was not set before determine_connection() anyway. > > > + } > > + if (conn->ssl_hostname == NULL) { > > + conn->ssl_hostname = apr_pstrdup(conn->scpool, > ssl_hostname); > > + } > > + } > > ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r, APLOGNO(00947) > > "connected %s to %s:%d", *url, conn->hostname, > conn->port); > > return OK; > > > Thanks for reviewing this, I'll commit soon. Regards, Yann.
