On 04/15/2009 10:31 AM, Kaspar Brand wrote:
>
> Correct, again. Changed accordingly in v7 of the patch, which I'm
> attaching (together with an interdiff from v6). I have tested this
> version with quite many different vhosts, per-vhost and per-dir
> settings, and with both SNI and non-SNI clients... but of course
> it's still possible that I missed something, so your review
> and additional testing is again very valuable and welcome.
Looks good. Some minor nitpicks:
1.
+ if ((r->server != sslconn->server)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
Reviewing the code again I don't think we need to have
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
this condition at all. Lets look at the cases (always Apache *with* SNI support)
a. SNI enabled client, supplied SNI hostname, HTTP hostname header different
from SNI hostname.
b. SNI enabled client, supplied SNI hostname, HTTP hostname header the same as
SNI hostname.
c. Non SNI enabled client.
a. This results in a Bad Request in the Post Read Request hook
(ssl_hook_ReadReq). So
we do not get here anymore.
b. In this case r->server == sslconn->server so we don't reach this condition.
c. In this case !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name)) is always
true.
2. The whole
+ if ((r->server != sslconn->server)
+ && renegotiate
+ && (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT)
+#ifndef OPENSSL_NO_TLSEXT
+ && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
+#endif
+ )
can move inside the if block above (
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
)
because only if get inside this block it can happen that
verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT
gets true. So we safe some checks in the case that
((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
(sc->server->auth.verify_mode != SSL_CVERIFY_UNSET))
is not true that will always fail and thus waste cycles even more
so as there are many SSL configurations outside that do not use
client certs at all.
This also means we can move the initialization of verify back
into the if block.
Furthermore I think that we need to check for CA list change in any case
that we need to renegotiate as even if we do not fail on SSL level due
to SSL_VERIFY_FAIL_IF_NO_PEER_CERT there could be other conditions later
on in the configuration (sslrequire / rewriterules) that check if we had been
successful with our check on SSL level to e.g. provide a nice error page
if we were not. So if we have changed the CA list we might signal success
to these downstream configuration options although there wasn't one because
we used the wrong CA list.
3. I created a var handshakeserver to avoid the dereferencing of sslconn->server
over and over again but this might be only a minor issue.
Regards
Rüdiger
