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.
