On Mon, Feb 24, 2014 at 10:05 AM, Ruediger Pluem <rpl...@apache.org> 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.
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).
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?
>
>
> >
> > 1.2 => 1.b
> > It seems that the client-provided SNI is the requested backend (not
> > the proxy), although I may be wrong.
>
> Keep in mind that 1.e is only the current state on trunk and SNI checking
> IMHO makes no sense in the 1.2 case
> as it cannot be used for virtual host selection on our side. This information
> is for the proxied side.
I was thinking of some admin wanting to enforce "don't forward request
with invalid SNI", but I agree it may be screwy.
>
> >
> > 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
:
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
"proxy-request-hostname" note), computed once (according to
ProxyPreserveHost on/off/canon?), and used everywhere (header, SNI,
CheckPeerCN/Name...).
>
> >
> > 3 => the proxy-provided SNI (or wildcard match)
> > Same as currently, but now this is really what was requested, whatever
> > ProxyPreserveHost is.
> > The SubjectAltName(s) should be used too.
>
> ProxyCheckPeerName should do this already.
Thanks, I missed that directive at the time (Pavel talked about it in
the meantime too, so I had a look).
>
>
> >
> > 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;
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)
@@ -1405,6 +1405,7 @@ static void socket_cleanup(proxy_conn_rec *conn)
{
conn->sock = NULL;
conn->connection = NULL;
+ conn->ssl_hostname = NULL;
apr_pool_clear(conn->scpool);
}
@@ -2284,6 +2285,7 @@ ap_proxy_determine_connection(apr_pool_t *p, reque
conn->pool);
}
socket_cleanup(conn);
+ conn->close = 0;
}
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);
+ }
+ 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;
Regards,
Yann.
