httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Kaspar Brand <httpd-dev.2...@velox.ch>
Subject Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGES modules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.c modules/ssl/ssl_toolkit_compat.h
Date Wed, 02 Jan 2008 16:36:22 GMT
Joe, many thanks for your review and valuable comments, first off! As
the person who contributed to the original SNI patch (authored by Peter
Sylvester/EdelWeb), I'll try to address your questions to the best of my
knowledge/ability.

An updated version of the patch (based on Guenter's latest version), is
attached to this message, in two versions: first against r606189 (the
last w/o any SNI related code) and second, against the current trunk
version. I'm grateful for comments, hints about flaws in my reasoning /
incorrect assumptions / omissions, or even objections, if there are.

> My main comment: it's not obvious to me how this actually does what it 
> needs to.  The issue with getting true SNI support is that the hostname 
> sent in the ClientHello must cause a change to the selection of 
> r->server early enough to ensure that any security policy specified in 
> the vhost configuration is applied correctly (e.g. SSLVerifyClient) - 
> not merely selection of the correct SSL_CTX (and hence server cert).

That's correct. However, choosing the appropriate r->server is mainly
handled by ap_read_request() in protocol.c, fortunately: specifically,
ap_read_request() calls ap_update_vhost_from_headers() to select the
right virtual host.

> I can't see how this patch achieves that.  Any clues?

If the request includes a Host: header (which results in r->hostname
being set), then everything is fine as soon as the headers have been
read (i.e. before ssl_hook_ReadReq is called, and of course before
ssl_hook_Access checks for directory-specific access restrictions). We
"only" need to make sure that the correct SSL context (and hence the
right server cert) is selected while the TLS handshake takes place.

> Has a configuration
> with an SSLVerifyClient specified in the named vhost been tested?

Yes, and one specific configuration actually made me tweak the code in
the servername callback further: when modifying the SSL connection,
OpenSSL's SSL_set_SSL_CTX() will only adjust the server cert from the
context, but not additional settings like verify_mode and the verify
callback. These are relevant when SSLVerifyClient is configured at the
*per-server* context (i.e. at the vhost level), and the previous version
of the patch failed to enforce such a configuration, at least in cases
where SSLVerifyClient for the first (=default) VirtualHost was different
than for any subsequent VirtualHosts.

Per-directory SSLVerifyClient settings don't suffer from this problem
(as the correct vhost is selected before ssl_hook_Access is called),
these are enforced by triggering a renegotiation, if needed.

When speaking of renegotiating (but not related to SSLVerifyClient
specifically), note that I removed this code from ssl_hook_Access:

> -     * We will force a renegotiation if we switch to another virtualhost.
> -     */
> -    if (r->hostname && !SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
{
> -        if (ssl_set_vhost_ctx(ssl, r->hostname) && ctx != SSL_get_SSL_CTX(ssl))
> -            renegotiate = TRUE;
> -    }

This applied to connections from clients which did not send an SNI
extension; renegotiating here doesn't make much sense to me since these
clients already received a (possibly) wrong certificate, so the mismatch
has already happened.

On the other hand, I've added some code to ssl_hook_ReadReq which deals
with the case where the client supplies an SNI extension but doesn't
send a Host: header (which is probably rare, but anyway):

> +    if (!r->hostname &&
> +        (servername = SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
> +        /* Use the SNI extension as the hostname if no Host: header was sent */
> +        r->hostname = apr_pstrdup(r->pool, servername);
> +        ap_update_vhost_from_headers(r);
> +    }

In reply to your further comments (unless addressed by Guenter already):

> 3) Calling an env var "SSL_TLS_..." is redundant; why is an extra env 
> var necessary here anyway?

It adds a couple of useful options when dealing with SSL requests:

* in a CGI application, you can programmatically determine if the client
  supplied an SNI extension (and possibly redirect the user, or point
  him to further information)

* you can log this information (LogFormat and "%{SSL_TLS_SNI}e" as the
  parameter), which allows to gather statistics about the percentage
  of SNI capable clients your site has, e.g.

* you can use this information in mod_rewrite rules, e.g. for
  rewriting requests based on the absence of an SNI extension (such as
  'RewriteCond %{SSL:SSL_TLS_SNI} =""' - note that I had to add support
  for this in ssl_engine_vars.c, this didn't work so far)

I agree that "SSL_TLS_" is somewhat redundant, but on the other hand
"SSL_" seems to be the proper prefix for all mod_ssl related variables,
and using "SSL_SNI" only might lead to the conclusion that SNI is also
relates to pure SSLv2/v3 connections.

> 4) Why is it desirable to send a fatal TLS alert if no vhost is found 
> matching the hostname in the clienthello?

I've changed that to SSL_TLSEXT_ERR_ALERT_WARNING, which also makes it
less of a dead end for NSS based clients e.g. (such as Firefox).

> This warning has no less weight given the presence of SNI support in the 
> version of OpenSSL against which mod_ssl is built.  Many (probably most) 
> deployed clients do not support SNI and hence cannot support name-based 
> vhosting with SSL.  I don't think this should be omitted, merely 
> reworded.

Right, the previous approach was too crude. I'm adapting the wording of
these warnings based on the presence of support for the SNI extension.

Additional changes, compared to the latest version committed to the
trunk by Guenter:

* renamed most of the functions, to make them more consistent
  with the general style for mod_ssl function names

* moved the callback function from ssl_engine_init.c to
  ssl_engine_kernel.c (where all the other OpenSSL callbacks live)

* merged the (intermediate) ssl_set_vhost_ctx function into
  ssl_callback_ServerNameIndication (was only needed by the code in
  ssl_hook_Access, which is now gone)

* some style improvements (last part of ssl_find_vhost, notably)

* in ssl_callback_ServerNameIndication, add some diagnostics at
  APLOG_DEBUG level

Thanks again,
Kaspar


Mime
View raw message