httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Joe Orton <jor...@redhat.com>
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 Fri, 28 Dec 2007 22:19:15 GMT
On Fri, Dec 21, 2007 at 01:16:21PM -0000, fuankg@apache.org wrote:
> Author: fuankg
> Date: Fri Dec 21 05:16:21 2007
> New Revision: 606190
> 
> URL: http://svn.apache.org/viewvc?rev=606190&view=rev
> Log:
> Added server name indication (RFC 4366) support (PR 34607).

Commits containing changes authored by someone other than the committer 
should have a "Submitted by: " line giving appropriate credit.

This patch introduces the following warnings for a build with 
OPENSSL_NO_TLSEXT undefined.

ssl_engine_init.c:194: warning: no previous prototype for 'ssl_set_vhost_ctx'
ssl_engine_init.c: In function 'ssl_set_vhost_ctx':
ssl_engine_init.c:205: warning: implicit declaration of function 'ap_vhost_iterate_given_conn'
ssl_engine_init.c: At top level:
ssl_engine_init.c:209: warning: no previous prototype for 'ssl_servername_cb'
ssl_engine_init.c: In function 'ssl_init_CheckServers':
ssl_engine_init.c:1119: warning: unused variable 'conflict'
ssl_engine_init.c:1117: warning: unused variable 'klen'
ssl_engine_init.c:1116: warning: unused variable 'key'
ssl_engine_init.c:1115: warning: unused variable 'table'
ssl_engine_init.c:1113: warning: unused variable 'ps'
ssl_engine_kernel.c: In function 'ssl_hook_Access':
ssl_engine_kernel.c:307: warning: implicit declaration of function 'ssl_set_vhost_ctx'
ssl_engine_kernel.c: In function 'ssl_hook_Fixup':
ssl_engine_kernel.c:1110: warning: suggest parentheses around assignment used as truth value

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).  I 
can't see how this patch achieves that.  Any clues?  Has a configuration 
with an SSLVerifyClient specified in the named vhost been tested?

Further comments:

1) There are numerous code style issues.  Please ensure new code added 
meets the style guide: http://httpd.apache.org/dev/styleguide.html

2) Please don't introduce new global functions named "ssl_".  Use 
"modssl_".

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

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

Some random nit-picks below:

> --- httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_kernel.c Fri Dec 21 05:16:21 2007
> @@ -297,6 +297,19 @@
>       * the currently active one.
>       */
>  
> +#ifndef OPENSSL_NO_TLSEXT
> +    /*
> +     * We will switch to another virtualhost and to its ssl_ctx
> +     * if changed, we will force a renegotiation.
> +     */

That sentence doesn't make sense.

> +    if (r->hostname && !SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
{
> +        SSL_CTX *ctx = SSL_get_SSL_CTX(ssl);

That shadows a variable of the same name at function scope.

> +        if (ssl_set_vhost_ctx(ssl,(char *)r->hostname) &&

Why is the cast needed?

> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Fri Dec 21 05:16:21 2007
> @@ -135,6 +135,87 @@
...
> +    }
> +#endif
> +}
> +
>  static void ssl_init_ctx_protocol(server_rec *s,
>                                    apr_pool_t *p,
>                                    apr_pool_t *ptemp,
> @@ -712,6 +816,8 @@
>          /* XXX: proxy support? */
>          ssl_init_ctx_cert_chain(s, p, ptemp, mctx);
>      }
> +
> +    ssl_init_server_extensions(s, p, ptemp, mctx);
>  }

This is being called for both a server context and a proxy context, it 
is not necessary for the latter.

>  static int ssl_server_import_cert(server_rec *s,
> @@ -1038,6 +1144,7 @@
>          }
>      }
>  
> +#ifdef OPENSSL_NO_TLSEXT
>      /*
>       * Give out warnings when more than one SSL-aware virtual server uses the
>       * same IP:port. This doesn't work because mod_ssl then will always use

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.

joe

Mime
View raw message