On 23.11.2013 13:56, Jeff Trawick wrote:
> Maybe I'm still missing something here, but it looks like we can still
> survive some calls to ssl_die() this logic. (I guess it doesn't make sense
> to have a configuration where that could happen, but it is confusing
> looking at the code.) Look at the various calls to ap_die() in
> ssl_server_import_key(). If that happens on the SSL_AIDX_RSA call, we'll
> call it again for SSL_AIDX_DSA. I think it is best not to continue after a
> fatal error. Also, if there is a certain type of key file and we fail
> importing it, we'll first get the fatal error message then print the
> AP01910 message then get another fatal error message.
Ah right, I overlooked that ssl_die()s are spread all over
ssl_server_import_{cert,key}. So one option would be to make
ssl_server_import_{cert,key} sort of tri-state, and then explicitly
check for APR_EGENERAL in ssl_init_server_certs and abort in this case,
like in the attached patch?
Kaspar
Index: ssl_engine_init.c
===================================================================
--- ssl_engine_init.c (revision 1544784)
+++ ssl_engine_init.c (working copy)
@@ -886,7 +886,7 @@ static apr_status_t ssl_server_import_cert(server_
X509 *cert;
if (!(asn1 = ssl_asn1_table_get(mc->tPublicCert, id))) {
- return APR_EGENERAL;
+ return APR_NOTFOUND;
}
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02232)
@@ -941,7 +941,7 @@ static apr_status_t ssl_server_import_key(server_r
pkey_type = (idx == SSL_AIDX_RSA) ? EVP_PKEY_RSA : EVP_PKEY_DSA;
if (!(asn1 = ssl_asn1_table_get(mc->tPrivateKey, id))) {
- return APR_EGENERAL;
+ return APR_NOTFOUND;
}
ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO(02236)
@@ -1057,10 +1057,19 @@ static apr_status_t ssl_init_server_certs(server_r
ecc_id = ssl_asn1_table_keyfmt(ptemp, vhost_id, SSL_AIDX_ECC);
#endif
- have_rsa = ssl_server_import_cert(s, mctx, rsa_id, SSL_AIDX_RSA);
- have_dsa = ssl_server_import_cert(s, mctx, dsa_id, SSL_AIDX_DSA);
+ if ((have_rsa = ssl_server_import_cert(s, mctx, rsa_id, SSL_AIDX_RSA))
+ == APR_EGENERAL) {
+ return have_rsa;
+ }
+ if ((have_dsa = ssl_server_import_cert(s, mctx, dsa_id, SSL_AIDX_DSA))
+ == APR_EGENERAL) {
+ return have_dsa;
+ }
#ifdef HAVE_ECC
- have_ecc = ssl_server_import_cert(s, mctx, ecc_id, SSL_AIDX_ECC);
+ if ((have_ecc = ssl_server_import_cert(s, mctx, ecc_id, SSL_AIDX_ECC))
+ == APR_EGENERAL) {
+ return have_ecc;
+ }
#endif
if ((have_rsa != APR_SUCCESS) && (have_dsa != APR_SUCCESS)
@@ -1078,10 +1087,19 @@ static apr_status_t ssl_init_server_certs(server_r
ssl_check_public_cert(s, ptemp, mctx->pks->certs[i], i);
}
- have_rsa = ssl_server_import_key(s, mctx, rsa_id, SSL_AIDX_RSA);
- have_dsa = ssl_server_import_key(s, mctx, dsa_id, SSL_AIDX_DSA);
+ if ((have_rsa = ssl_server_import_key(s, mctx, rsa_id, SSL_AIDX_RSA))
+ == APR_EGENERAL) {
+ return have_rsa;
+ }
+ if ((have_dsa = ssl_server_import_key(s, mctx, dsa_id, SSL_AIDX_DSA))
+ == APR_EGENERAL) {
+ return have_dsa;
+ }
#ifdef HAVE_ECC
- have_ecc = ssl_server_import_key(s, mctx, ecc_id, SSL_AIDX_ECC);
+ if ((have_ecc = ssl_server_import_key(s, mctx, ecc_id, SSL_AIDX_ECC))
+ == APR_EGENERAL) {
+ return have_ecc;
+ }
#endif
if ((have_rsa != APR_SUCCESS) && (have_dsa != APR_SUCCESS)
