On 04/21/2009 07:10 AM, Kaspar Brand wrote: > Ruediger Pluem wrote: >> Looks good. Some minor nitpicks:
> >> 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. > > Here I'm not sure I'm really following you / understanding your point. > The case I was primarily thinking of is something like > > <VirtualHost foo.example.com:443> > SSLVerifyClient none # (the default, anyway) > </VirtualHost> > > <VirtualHost bar.example.com:443> > SSLVerifyClient optional > SSLCACertificateFile bar-clientauth-bundle.pem > </VirtualHost> > > In this situation, if a non-SNI client requests content from > bar.example.com, the "renegotiate" variable will get set, but since > "verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT" is not true, we will > currently let it proceed. Are you proposing to return HTTP_FORBIDDEN > immediately after the MODSSL_CFG_CA_NE checks fail, instead (i.e., even > if SSLVerifyClient is optional)? My idea when writing that code was that Yes, this is my proposal. The configuration you mentioned above is a good example for a non working configuration with name based virtual Hosts and SSL *without* SNI. As we have no SSLCACertificateFile set in the the default host we effectively have no CA against which we can check client certs so we will never have a successful client verification here. While the above does no real security harm (it just doesn't work as expected) the next configuration *can* do security harm: <VirtualHost foo.example.com:443> SSLVerifyClient optional SSLCACertificateFile foo-clientauth-bundle.pem </VirtualHost> <VirtualHost bar.example.com:443> SSLVerifyClient optional SSLCACertificateFile bar-clientauth-bundle.pem </VirtualHost> This would cause client certificates signed by foo-clientauth-bundle accepted by the virtual host bar.example.com. Even if SSLVerifyClient is optional later processing steps such as RewriteRules or applications may evaluate the environment variables set by SSL and think that a successful authentication took place where in fact it has not. Or think about adding SSLOptions +FakeBasicAuth to the configuration of the virtual host bar.example.com > unless SSLVerifyClient is set to "require", we should not stop non-SNI > clients here - the evaluation of a possible SSLRequire configuration > directive at the end of ssl_hook_Access can still return HTTP_FORBIDDEN, > if really needed (OTOH, why exactly would the admin choose "optional", > then?). But maybe I'm simply missing your real point. The Fakeauth setting above is a perfect example why an admin might set optional: Either the user has a cert or it has to authenticate via username and password. Another one is: SSLVerifyClient require causes a Forbidden. While you can customize an error page for that you might want to have a specific one for this very special failure e.g. with links on it who to contact for a client cert. This can be done very nicely with rewriterules and the environment variable SSL_CLIENT_VERIFY. > What I had to do anyway, however, is setting r->connection->aborted > before returning HTTP_FORBIDDEN... otherwise we run into a problem with > keep-alive requests, as additional testing has shown: if we don't drop > the connection at the same time (by setting r->connection->aborted), > then verify_old keeps its value from the previous request, and > "renegotiate" may therefore not be set again when processing the next > request (if verify == verify_old). Using r->connection->aborted for > closing the connection immediately is also used in code further down in > ssl_hook_Access (when a renegotiation doesn't have the expected outcome) > - i.e., we're not introducing new behavior by using this "technique". It took me some time, but I think I got it. So we either need to end the keepalive request or we need to restore the old verify setting via modssl_set_verify(ssl, verify_old, ssl_callback_SSLVerify); correct? Dropping the connection is not so efficient but might be more secure. Anyway the solution is bad. This is not your fault as the existing code is bad here as well :-). r->connection->aborted should *only* be set if the client did a network disconnect *never* if the server decides to shutdown the connection. In this case we should set r->connection->keepalive = AP_CONN_CLOSE But as said this IMHO needs fixing in the code further down below as well. Further comments might follow when I have time to review v8. Stay tuned :-). Regards Rüdiger
