Yann Ylavic wrote:
> 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.
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.
>
> 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?
> "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).
> 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.
> }
> 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?
> + }
> + 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
Rüdiger
