Re: svn commit: r606190 - in /httpd/httpd/trunk: CHANGESmodules/ssl/ssl_engine_init.c modules/ssl/ssl_engine_kernel.cmodules/ssl/ssl_toolkit_compat.h

[Guenter Knauf <fu...@apache.org>]

Hi Joe,
thanks for your review and your detailed comments.

> Commits containing changes authored by someone other than the committer
> should have a "Submitted by: " line giving appropriate credit.
hmm, you mean with the SVN log? Sorry, forgot there - but it appears in CHANGES.

> This patch introduces the following warnings for a build with
> OPENSSL_NO_TLSEXT undefined.
I believe I've fixed these now - sorry but I didnt test on Linux yet;
 and with the NetWare compiler its not possible to develop with all warnings enabled.
Please let me know if I dodnt catch them all.

> 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?
I must admit that I didnt think of this; 
I tested for the server certs only, and for this it works fine - the cert/name mismatch warning is gone.

> 1) There are numerous code style issues.  Please ensure new code added
> meets the style guide: http://httpd.apache.org/dev/styleguide.html
I tried now to meet the style guide, but at some places I was unsure - let me know what I missed.

> 2) Please don't introduce new global functions named "ssl_".  Use
> "modssl_".
hmmm, looking at all other functions in ssl_private.h the only two which are prefixed with "modssl_" are those two you recently introduced with OCSP support, all others are prefixed with "ssl_" only; so why you want this?

> 3) Calling an env var "SSL_TLS_..." is redundant; 
I prefixed it with "SSL_" since all mod_ssl vars are refixed with that...
> why is an extra env var necessary here anyway?
probably not - do you think I should remove it?

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

>> +    /*
>> +     * We will switch to another virtualhost and to its ssl_ctx
>> +     * if changed, we will force a renegotiation.
>> +     */

> That sentence doesn't make sense.
changed - let me know if that'S now better.

>> +    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.
removed - from what I see it was anyway obsolete: some lines above we do already set the ctx if ssl is not NULL,
and if ssl is NULL we return before we can reach this code, so I think at this place ctx should already been set, or?

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

> Why is the cast needed?
removed.

>> @@ -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.
moved up so that its only called for server now.

> 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.
hmmmm, this is where I didnt change yet - what would you propose here?

thanks again for your comments.

Guen.