> If I understand correctly, the current code is relying on
> ssl_hook_Access to perform a *second* handshake which will ensure that
> the named vhost's access control configuration is enforced, if
> necessary?
No, that's probably due to a misunderstanding. In its current form, the
patch will set the correct SSLVerify parameters for the named vhost very
early in the initial TLS handshake (in ssl_find_vhost, which is called
by ap_vhost_iterate_given_conn). If "SSLVerifyClient require" is
configured at the vhost level and there's no certificate from the peer,
then the initial handshake will already fail (as you would expect).
A second handshake (triggered by ssl_hook_Access) only takes place if
there are per-directory specific settings (SSLVerifyClient, but also
SSLCipher) which differ from the per-server setting. Maybe the following
- somewhat artificial - pseudo config helps to illustrate the difference:
<VirtualHost *:443>
# Server 1
SSLVerifyClient optional_no_ca
</VirtualHost>
<VirtualHost *:443>
# Server 2
SSLVerifyClient require
</VirtualHost>
<VirtualHost *:443>
# Server 3
<Location />
SSLVerifyClient require
</Location>
</VirtualHost>
For server 2, there will only be one handshake, and it will fail right
after the CertificateRequest if the peer doesn't supply a valid client
cert. For server 3 (only shown for illustration purposes, doesn't make
much sense in a real-word config), a second handshake is triggered by
ssl_hook_Access, as the SSLVerifyClient parameter is a per-directory
setting in this case (there's an implied "SSLVerifyClient none" default
setting at the vhost level, which is used during the initial handshake).
> it should be done in the initial handshake,
> since it theoretically can - and requiring a second handshake invokes
> the spectre of PR 12355 and associated problems.
Absolutely agreed. That's exactly what the SSL_set_verify call in
ssl_find_vhost is for (which I added after your previous comments, this
was indeed a problem with the old version of the patch).
I hope this helps to clear up some of the confusion - let me know if you
still see issues with the current implementation.
Thanks,
Kaspar
