On Thu, Jun 30, 2016 at 5:26 PM, Yann Ylavic <[email protected]> wrote:
> On Thu, Jun 30, 2016 at 5:05 PM, Ruediger Pluem <[email protected]> wrote:
>>
>> Is there a reson why we use ssl_callback_SSLVerify instead of NULL like we
>> do in asimilar situation below?
>> IMHO we do not want to change the callback here to whatever it may set.
>> I agree that in practice there won't be any difference right now, since we
>> only have one callback.
>
> I agree that if/when we have multiple callback possibilities, we
> should set NULL here, but also above where we force the new mode.
Also note that we could avoid this SSL_set_verify() dance in
ssl_hook_Access() with something like the attached patch, which moves
it just before the actual renegotiation.
The new AP_CONN_CLOSE are to help core HTTP with connections we know
are not unrecoverable.
>
> Regards,
> Yann.
Index: modules/ssl/ssl_engine_kernel.c
===================================================================
--- modules/ssl/ssl_engine_kernel.c (revision 1750805)
+++ modules/ssl/ssl_engine_kernel.c (working copy)
@@ -682,11 +682,12 @@ int ssl_hook_Access(request_rec *r)
* verification but at least skip the I/O-intensive renegotiation
* handshake.
*/
+ verify = SSL_get_verify_mode(ssl);
if ((dc->nVerifyClient != SSL_CVERIFY_UNSET) ||
(sc->server->auth.verify_mode != SSL_CVERIFY_UNSET)) {
/* remember old state */
- verify_old = SSL_get_verify_mode(ssl);
+ verify_old = verify;
/* configure new state */
verify = SSL_VERIFY_NONE;
@@ -703,12 +704,6 @@ int ssl_hook_Access(request_rec *r)
verify |= SSL_VERIFY_PEER;
}
- /* TODO: this seems premature since we do not know if there
- * are any changes required.
- */
- SSL_set_verify(ssl, verify, ssl_callback_SSLVerify);
- SSL_set_verify_result(ssl, X509_V_OK);
-
/* determine whether we've to force a renegotiation */
if (!renegotiate && verify != verify_old) {
if (((verify_old == SSL_VERIFY_NONE) &&
@@ -727,7 +722,6 @@ int ssl_hook_Access(request_rec *r)
* on this connection.
*/
apr_table_setn(r->notes, "ssl-renegotiate-forbidden", "verify-client");
- SSL_set_verify(ssl, verify_old, ssl_callback_SSLVerify);
return HTTP_FORBIDDEN;
}
/* optimization */
@@ -802,7 +796,6 @@ int ssl_hook_Access(request_rec *r)
"'require' and VirtualHost-specific CA certificate "
"list is only available to clients with TLS server "
"name indication (SNI) support");
- SSL_set_verify(ssl, verify_old, NULL);
return HTTP_FORBIDDEN;
} else
/* let it pass, possibly with an "incorrect" peer cert,
@@ -850,6 +843,7 @@ int ssl_hook_Access(request_rec *r)
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02257)
"could not buffer message body to allow "
"SSL renegotiation to proceed");
+ r->connection->keepalive = AP_CONN_CLOSE;
return rv;
}
}
@@ -872,6 +866,9 @@ int ssl_hook_Access(request_rec *r)
ap_log_rerror(APLOG_MARK, APLOG_INFO, 0, r, APLOGNO(02221)
"Requesting connection re-negotiation");
+ SSL_set_verify(ssl, verify, NULL);
+ SSL_set_verify_result(ssl, X509_V_OK);
+
if (renegotiate_quick) {
STACK_OF(X509) *cert_stack;
@@ -898,6 +895,7 @@ int ssl_hook_Access(request_rec *r)
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02222)
"Cannot find peer certificate chain");
+ r->connection->keepalive = AP_CONN_CLOSE;
return HTTP_FORBIDDEN;
}
@@ -907,6 +905,7 @@ int ssl_hook_Access(request_rec *r)
ap_log_rerror(APLOG_MARK, APLOG_ERR, 0, r, APLOGNO(02223)
"Cannot find certificate storage");
+ r->connection->keepalive = AP_CONN_CLOSE;
return HTTP_FORBIDDEN;
}
@@ -2462,7 +2461,7 @@ int ssl_callback_SRPServerParams(SSL *ssl, int *ad
#if OPENSSL_VERSION_NUMBER >= 0x10100000L
SRP_user_pwd_free(u);
#endif
- SSL_set_verify(ssl, SSL_VERIFY_NONE, ssl_callback_SSLVerify);
+ SSL_set_verify(ssl, SSL_VERIFY_NONE, NULL);
return SSL_ERROR_NONE;
}
