On Fri, Dec 21, 2007 at 01:16:21PM -0000, [EMAIL PROTECTED] 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
