Thanks for your time and review - much appreciated.
> + n = sslconn->verify_depth;
> + sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
> + dc->nVerifyDepth : sc->server->auth.verify_depth;
> + if ((sslconn->verify_depth < n) ||
> + ((n == 0) && (sc->server->auth.verify_depth == 0))) {
> + renegotiate = TRUE;
> + ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
> + "Reduced client verification depth will force "
> + "renegotiation");
[...]
> I don't really understand this part of the patch. If the default host has
> a verify depth of lets say 10 and the virtual host with the correct name has 1
> then IMHO a renegotiation should happen but the code above does not seem to
> do this.
Good catch. When I was modifying the SSLVerifyDepth related code (only
for 2.2.x, at that time), I concentrated on doing "the right thing" for
persistent connections (keep-alive requests). Apparently I missed the
case you're bringing up in my tests then, but thanks to your
improvements with r757463, determining the depth of the current request
can now be done like this:
n = sslconn->verify_depth ?
sslconn->verify_depth :
(mySrvConfig(sslconn->server))->server->auth.verify_depth;
I.e., if it's a persistent connection, then verify_depth has been set
(by the previous run of ssl_hook_Access), and we use that one. If it's
the first connection, OTOH, then we have to figure out the default for
the respective vhost (where sslconn->server comes in handy).
> I don't get why the above code is only needed in the case that Openssl is SNI
> capable. IMHO this check is always needed or better regardless of SNI or not
> SNI
> only needed
>
> + if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
> + (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
>
> Otherwise a compiler warning about verify might be used uninitialized tells
> you
> that something is wrong with the code anyway.
> The only thing that needs an ifndef above is
>
> +#ifndef OPENSSL_NO_TLSEXT
> + && !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))
> +#endif
>
> which is always true in the non SNI case.
You're right, it doesn't hurt to add this check even if SNI support
isn't compiled in. When I added that code, I was mainly having the
backport to 2.2.x in mind, and therefore tried to avoid changes which
would also have an effect if SNI support wasn't compiled (probably not
completely consistent in all cases, though).
As far as the initialization of verify is concerned, that's another good
point you raise. In the httpd-2.2.x-sni.20080928.patch (from last
September), I removed the if clause around the code determining the new
verify mode, so verify was always initialized. Your change in r757373
obviated the need for always calculating the new verify mode (for SNI
clients, mostly), and when I ported the latest 2.2.x changes to trunk
again (in sni_sslverifyclient-v6.diff), I wasn't careful enough to check
all the consequences of keeping that if clause, obviously.
I propose we just initialize verify when we declare it - that's the most
straightforward solution, IMO.
> One other thing: r->connection->base_server is IMHO always wrong in the patch
> and should be replaced with sslconn->server
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.
Kaspar
Index: httpd-trunk/modules/ssl/ssl_engine_kernel.c
===================================================================
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (revision 765079)
+++ httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
@@ -186,16 +186,6 @@ int ssl_hook_ReadReq(request_rec *r)
return HTTP_BAD_REQUEST;
}
}
- else if (r->connection->vhost_lookup_data) {
- /*
- * We are using a name based configuration here, but no hostname was
- * provided via SNI. Don't allow that.
- */
- ap_log_error(APLOG_MARK, APLOG_ERR, 0, r->server,
- "No hostname was provided via SNI for a name based"
- " virtual host");
- return HTTP_FORBIDDEN;
- }
#endif
SSL_set_app_data2(ssl, r);
@@ -281,7 +271,7 @@ int ssl_hook_Access(request_rec *r)
X509_STORE_CTX cert_store_ctx;
STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL;
const SSL_CIPHER *cipher = NULL;
- int depth, verify_old, verify, n;
+ int depth, verify_old, verify = SSL_VERIFY_NONE, n;
if (ssl) {
ctx = SSL_get_SSL_CTX(ssl);
@@ -362,7 +352,7 @@ int ssl_hook_Access(request_rec *r)
* has to enable this via ``SSLOptions +OptRenegotiate''. So we do no
* implicit optimizations.
*/
- if (dc->szCipherSuite) {
+ if (dc->szCipherSuite || (r->server != sslconn->server)) {
/* remember old state */
if (dc->nOptions & SSL_OPT_OPTRENEGOTIATE) {
@@ -377,7 +367,10 @@ int ssl_hook_Access(request_rec *r)
}
/* configure new state */
- if (!modssl_set_cipher_list(ssl, dc->szCipherSuite)) {
+ if ((dc->szCipherSuite || sc->server->auth.cipher_suite) &&
+ !modssl_set_cipher_list(ssl, dc->szCipherSuite ?
+ dc->szCipherSuite :
+ sc->server->auth.cipher_suite)) {
ap_log_rerror(APLOG_MARK, APLOG_WARNING, 0, r,
"Unable to reconfigure (per-directory) "
"permitted SSL ciphers");
@@ -443,8 +436,13 @@ int ssl_hook_Access(request_rec *r)
sk_SSL_CIPHER_free(cipher_list_old);
}
- /* tracing */
if (renegotiate) {
+#ifdef SSL_OP_CIPHER_SERVER_PREFERENCE
+ if (sc->cipher_server_pref == TRUE) {
+ SSL_set_options(ssl, SSL_OP_CIPHER_SERVER_PREFERENCE);
+ }
+#endif
+ /* tracing */
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Reconfigured cipher suite will force renegotiation");
}
@@ -457,31 +455,24 @@ int ssl_hook_Access(request_rec *r)
* function and not by OpenSSL internally (and our function is aware of
* both the per-server and per-directory contexts). So we cannot ask
* OpenSSL about the currently verify depth. Instead we remember it in our
- * ap_ctx attached to the SSL* of OpenSSL. We've to force the
+ * SSLConnRec attached to the SSL* of OpenSSL. We've to force the
* renegotiation if the reconfigured/new verify depth is less than the
* currently active/remembered verify depth (because this means more
* restriction on the certificate chain).
*/
- if ((sc->server->auth.verify_depth != UNSET) &&
- (dc->nVerifyDepth == UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyDepth = sc->server->auth.verify_depth;
+ n = sslconn->verify_depth ?
+ sslconn->verify_depth :
+ (mySrvConfig(sslconn->server))->server->auth.verify_depth;
+ /* determine the new depth */
+ sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
+ dc->nVerifyDepth : sc->server->auth.verify_depth;
+ if (sslconn->verify_depth < n) {
+ renegotiate = TRUE;
+ ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
+ "Reduced client verification depth will force "
+ "renegotiation");
}
- if (dc->nVerifyDepth != UNSET) {
- /* XXX: doesnt look like sslconn->verify_depth is actually used */
- if (!(n = sslconn->verify_depth)) {
- sslconn->verify_depth = n = sc->server->auth.verify_depth;
- }
- /* determine whether a renegotiation has to be forced */
- if (dc->nVerifyDepth < n) {
- renegotiate = TRUE;
- ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
- "Reduced client verification depth will force "
- "renegotiation");
- }
- }
-
/*
* override of SSLVerifyClient
*
@@ -496,23 +487,21 @@ int ssl_hook_Access(request_rec *r)
* verification but at least skip the I/O-intensive renegotation
* handshake.
*/
- if ((sc->server->auth.verify_mode != SSL_CVERIFY_UNSET) &&
- (dc->nVerifyClient == SSL_CVERIFY_UNSET)) {
- /* apply per-vhost setting, if per-directory config is not set */
- dc->nVerifyClient = sc->server->auth.verify_mode;
- }
- if (dc->nVerifyClient != SSL_CVERIFY_UNSET) {
+ if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
/* remember old state */
verify_old = SSL_get_verify_mode(ssl);
+
/* configure new state */
- verify = SSL_VERIFY_NONE;
-
- if (dc->nVerifyClient == SSL_CVERIFY_REQUIRE) {
+ if ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_REQUIRE)) {
verify |= SSL_VERIFY_PEER_STRICT;
}
if ((dc->nVerifyClient == SSL_CVERIFY_OPTIONAL) ||
- (dc->nVerifyClient == SSL_CVERIFY_OPTIONAL_NO_CA))
+ (dc->nVerifyClient == SSL_CVERIFY_OPTIONAL_NO_CA) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_OPTIONAL) ||
+ (sc->server->auth.verify_mode == SSL_CVERIFY_OPTIONAL_NO_CA))
{
verify |= SSL_VERIFY_PEER;
}
@@ -550,6 +539,41 @@ int ssl_hook_Access(request_rec *r)
}
}
+ /* If we're handling a request for a vhost other than the default one,
+ * then we need to make sure that client authentication is properly
+ * enforced. For clients supplying an SNI extension, the peer certificate
+ * verification has happened in the handshake already. For non-SNI
requests,
+ * an additional check is needed here. If client authentication is
+ * configured as mandatory, then we can only proceed if the CA list
+ * doesn't have to be changed (OpenSSL doesn't provide an option
+ * to change the list for an existing session).
+ */
+ 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
+ ) {
+ SSLSrvConfigRec *bssc = mySrvConfig(sslconn->server);
+
+#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && \
+ strNE(sc1->server->auth.f, sc2->server->auth.f)))
+
+ if (MODSSL_CFG_CA_NE(ca_cert_file, sc, bssc) ||
+ MODSSL_CFG_CA_NE(ca_cert_path, sc, bssc)) {
+ ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r,
+ "Non-default virtual host with SSLVerify set to 'require' "
+ "and VirtualHost-specific CA certificate list is only "
+ "supported for clients with TLS server name indication "
+ "(SNI) support");
+ return HTTP_FORBIDDEN;
+ }
+ }
+
/* If a renegotiation is now required for this location, and the
* request includes a message body (and the client has not
* requested a "100 Continue" response), then the client will be
@@ -732,8 +756,10 @@ int ssl_hook_Access(request_rec *r)
/*
* Finally check for acceptable renegotiation results
*/
- if (dc->nVerifyClient != SSL_CVERIFY_NONE) {
- BOOL do_verify = (dc->nVerifyClient == SSL_CVERIFY_REQUIRE);
+ if ((dc->nVerifyClient != SSL_CVERIFY_NONE) ||
+ (sc->server->auth.verify_mode != SSL_CVERIFY_NONE)) {
+ BOOL do_verify = ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
+ (sc->server->auth.verify_mode ==
SSL_CVERIFY_REQUIRE));
if (do_verify && (SSL_get_verify_result(ssl) != X509_V_OK)) {
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r,
@@ -1195,8 +1221,8 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
SSL_get_ex_data_X509_STORE_CTX_idx());
conn_rec *conn = (conn_rec *)SSL_get_app_data(ssl);
- server_rec *s = mySrvFromConn(conn);
request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+ server_rec *s = r ? r->server : mySrvFromConn(conn);
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLDirConfigRec *dc = r ? myDirConfig(r) : NULL;
@@ -1326,7 +1352,10 @@ int ssl_callback_SSLVerify(int ok, X509_STORE_CTX
int ssl_callback_SSLVerify_CRL(int ok, X509_STORE_CTX *ctx, conn_rec *c)
{
- server_rec *s = mySrvFromConn(c);
+ SSL *ssl = X509_STORE_CTX_get_ex_data(ctx,
+
SSL_get_ex_data_X509_STORE_CTX_idx());
+ request_rec *r = (request_rec *)SSL_get_app_data2(ssl);
+ server_rec *s = r ? r->server : mySrvFromConn(c);
SSLSrvConfigRec *sc = mySrvConfig(s);
SSLConnRec *sslconn = myConnConfig(c);
modssl_ctx_t *mctx = myCtxConfig(sslconn, sc);
diff -u httpd-trunk/modules/ssl/ssl_engine_kernel.c
httpd-trunk/modules/ssl/ssl_engine_kernel.c
--- httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
+++ httpd-trunk/modules/ssl/ssl_engine_kernel.c (working copy)
@@ -271,7 +271,7 @@
X509_STORE_CTX cert_store_ctx;
STACK_OF(SSL_CIPHER) *cipher_list_old = NULL, *cipher_list = NULL;
const SSL_CIPHER *cipher = NULL;
- int depth, verify_old, verify, n;
+ int depth, verify_old, verify = SSL_VERIFY_NONE, n;
if (ssl) {
ctx = SSL_get_SSL_CTX(ssl);
@@ -352,7 +352,7 @@
* has to enable this via ``SSLOptions +OptRenegotiate''. So we do no
* implicit optimizations.
*/
- if (dc->szCipherSuite || (r->server != r->connection->base_server)) {
+ if (dc->szCipherSuite || (r->server != sslconn->server)) {
/* remember old state */
if (dc->nOptions & SSL_OPT_OPTRENEGOTIATE) {
@@ -455,16 +455,18 @@
* function and not by OpenSSL internally (and our function is aware of
* both the per-server and per-directory contexts). So we cannot ask
* OpenSSL about the currently verify depth. Instead we remember it in our
- * ap_ctx attached to the SSL* of OpenSSL. We've to force the
+ * SSLConnRec attached to the SSL* of OpenSSL. We've to force the
* renegotiation if the reconfigured/new verify depth is less than the
* currently active/remembered verify depth (because this means more
* restriction on the certificate chain).
*/
- n = sslconn->verify_depth;
+ n = sslconn->verify_depth ?
+ sslconn->verify_depth :
+ (mySrvConfig(sslconn->server))->server->auth.verify_depth;
+ /* determine the new depth */
sslconn->verify_depth = (dc->nVerifyDepth != UNSET) ?
dc->nVerifyDepth : sc->server->auth.verify_depth;
- if ((sslconn->verify_depth < n) ||
- ((n == 0) && (sc->server->auth.verify_depth == 0))) {
+ if (sslconn->verify_depth < n) {
renegotiate = TRUE;
ap_log_rerror(APLOG_MARK, APLOG_DEBUG, 0, r,
"Reduced client verification depth will force "
@@ -489,9 +491,8 @@
(sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
/* remember old state */
verify_old = SSL_get_verify_mode(ssl);
- /* configure new state */
- verify = SSL_VERIFY_NONE;
+ /* configure new state */
if ((dc->nVerifyClient == SSL_CVERIFY_REQUIRE) ||
(sc->server->auth.verify_mode == SSL_CVERIFY_REQUIRE)) {
verify |= SSL_VERIFY_PEER_STRICT;
@@ -538,26 +539,29 @@
}
}
-#ifndef OPENSSL_NO_TLSEXT
/* If we're handling a request for a vhost other than the default one,
* then we need to make sure that client authentication is properly
* enforced. For clients supplying an SNI extension, the peer certificate
* verification has happened in the handshake already. For non-SNI
requests,
* an additional check is needed here. If client authentication is
* configured as mandatory, then we can only proceed if the CA list
- * doesn't have to be changed (SSL_set_cert_store() would be required
- * for this).
+ * doesn't have to be changed (OpenSSL doesn't provide an option
+ * to change the list for an existing session).
*/
-#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
- (sc1->server->auth.f && \
- (!sc2->server->auth.f || \
- sc2->server->auth.f && strNE(sc1->server->auth.f, sc2->server->auth.f)))
+ 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
+ ) {
+ SSLSrvConfigRec *bssc = mySrvConfig(sslconn->server);
- if ((r->server != r->connection->base_server) &&
- (verify & SSL_VERIFY_FAIL_IF_NO_PEER_CERT) &&
- renegotiate &&
- !(SSL_get_servername(ssl, TLSEXT_NAMETYPE_host_name))) {
- SSLSrvConfigRec *bssc = mySrvConfig(r->connection->base_server);
+#define MODSSL_CFG_CA_NE(f, sc1, sc2) \
+ (sc1->server->auth.f && \
+ (!sc2->server->auth.f || \
+ sc2->server->auth.f && \
+ strNE(sc1->server->auth.f, sc2->server->auth.f)))
if (MODSSL_CFG_CA_NE(ca_cert_file, sc, bssc) ||
MODSSL_CFG_CA_NE(ca_cert_path, sc, bssc)) {
@@ -569,7 +573,6 @@
return HTTP_FORBIDDEN;
}
}
-#endif /* OPENSSL_NO_TLSEXT */
/* If a renegotiation is now required for this location, and the
* request includes a message body (and the client has not
