httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Yann Ylavic <ylavic....@gmail.com>
Subject Re: Behavior of Host: vs. SNI Hostname in proxy CONNECT requests
Date Thu, 27 Feb 2014 14:08:46 GMT
On Wed, Feb 26, 2014 at 9:45 PM, Ruediger Pluem <rpluem@apache.org> wrote:

>
>
> Yann Ylavic wrote:
> > On Mon, Feb 24, 2014 at 10:05 AM, Ruediger Pluem <rpluem@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.
>

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.

Mime
View raw message