Thanks Jeff and Yann for your reviews. Fixed all items as suggested,
except for these ones:
> The various calls to ssl_server_import_cert() in ssl_init_server_certs()
> need different rc checking than before. (Now ssl_server_import_cert() can
> return a fatal error instead of just a boolean.)
>
> (same for ssl_server_import_key())
Do you suggest that we should make these checks more strict? The current
code is just checking if at least one certificate/key was configured
successfully. My change so far was the following:
- if (!(have_rsa || have_dsa
+ if ((have_rsa != APR_SUCCESS) && (have_dsa != APR_SUCCESS)
#ifdef HAVE_ECC
- || have_ecc
+ && (have_ecc != APR_SUCCESS)
#endif
-)) {
+) {
ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO(01910)
"Oops, no " KEYTYPES " server certificate found "
"for '%s:%d'?!", s->server_hostname, s->port);
- ssl_die(s);
+ return ssl_die(s);
(I have simply rewritten the condition with De Morgan's law)
I'm fine with extending these checks (i.e., fail if any of the
ssl_server_import_cert or ssl_server_import_key calls fails), but this
can result in refusing to load existing configs.
> It looks like some errors in the proxy config that previously were ignored
> now cause startup failures... (shrug)
There were three places in ssl_init_proxy_certs where I mistakenly
replaced the previous "return" with "return APR_EGENERAL" instead of
"return APR_SUCCESS". Thanks for testing!
Committed with r1544774 (with some additional pkcs7 adjustments),
incremental diff attached for reference. Passes t/ssl/*.t from the test
framework for me, but additional testing is of course welcome.
Kaspar
diff -u ssl_engine_init.c ssl_engine_init.c
--- ssl_engine_init.c (working copy)
+++ ssl_engine_init.c (working copy)
@@ -605,7 +605,7 @@
static apr_status_t ssl_init_ctx_verify(server_rec *s,
apr_pool_t *p,
apr_pool_t *ptemp,
- modssl_ctx_t *mctx)
+ modssl_ctx_t *mctx)
{
SSL_CTX *ctx = mctx->ssl_ctx;
@@ -780,14 +780,15 @@
return APR_SUCCESS;
}
-static void ssl_init_ctx_pkcs7_cert_chain(server_rec *s, modssl_ctx_t *mctx)
+static apr_status_t ssl_init_ctx_pkcs7_cert_chain(server_rec *s,
+ modssl_ctx_t *mctx)
{
STACK_OF(X509) *certs = ssl_read_pkcs7(s, mctx->pkcs7);
int n;
STACK_OF(X509) *extra_certs = NULL;
if (!certs)
- return;
+ return APR_EGENERAL;
#ifdef OPENSSL_NO_SSL_INTERN
SSL_CTX_get_extra_chain_certs(mctx->ssl_ctx, &extra_certs);
@@ -798,6 +799,8 @@
if (!extra_certs)
for (n = 1; n < sk_X509_num(certs); ++n)
SSL_CTX_add_extra_chain_cert(mctx->ssl_ctx, sk_X509_value(certs,
n));
+
+ return APR_SUCCESS;
}
static apr_status_t ssl_init_ctx_cert_chain(server_rec *s,
@@ -810,8 +813,7 @@
const char *chain = mctx->cert_chain;
if (mctx->pkcs7) {
- ssl_init_ctx_pkcs7_cert_chain(s, mctx);
- return APR_SUCCESS;
+ return ssl_init_ctx_pkcs7_cert_chain(s, mctx);
}
/*
@@ -863,7 +865,9 @@
{
apr_status_t rv;
- ssl_init_ctx_protocol(s, p, ptemp, mctx);
+ if ((rv = ssl_init_ctx_protocol(s, p, ptemp, mctx)) != APR_SUCCESS) {
+ return rv;
+ }
ssl_init_ctx_session_cache(s, p, ptemp, mctx);
@@ -1229,7 +1233,7 @@
ssl_callback_proxy_cert);
if (!(pkp->cert_file || pkp->cert_path)) {
- return APR_EGENERAL;
+ return APR_SUCCESS;
}
sk = sk_X509_INFO_new_null();
@@ -1246,7 +1250,7 @@
sk_X509_INFO_free(sk);
ap_log_error(APLOG_MARK, APLOG_WARNING, 0, s, APLOGNO(02206)
"no client certs found for SSL proxy");
- return APR_EGENERAL;
+ return APR_SUCCESS;
}
/* Check that all client certs have got certificates and private
@@ -1279,7 +1283,7 @@
if (!pkp->ca_cert_file || !store) {
- return APR_EGENERAL;
+ return APR_SUCCESS;
}
/* If SSLProxyMachineCertificateChainFile is configured, load all
@@ -1363,7 +1367,9 @@
{
apr_status_t rv;
- ssl_init_ctx(s, p, ptemp, sc->proxy);
+ if ((rv = ssl_init_ctx(s, p, ptemp, sc->proxy)) != APR_SUCCESS) {
+ return rv;
+ }
if ((rv = ssl_init_proxy_certs(s, p, ptemp, sc->proxy)) != APR_SUCCESS) {
return rv;
@@ -1379,16 +1385,22 @@
{
apr_status_t rv;
- ssl_init_server_check(s, p, ptemp, sc->server);
+ if ((rv = ssl_init_server_check(s, p, ptemp, sc->server)) != APR_SUCCESS) {
+ return rv;
+ }
- ssl_init_ctx(s, p, ptemp, sc->server);
+ if ((rv = ssl_init_ctx(s, p, ptemp, sc->server)) != APR_SUCCESS) {
+ return rv;
+ }
if ((rv = ssl_init_server_certs(s, p, ptemp, sc->server)) != APR_SUCCESS) {
return rv;
}
#ifdef HAVE_TLS_SESSION_TICKETS
- ssl_init_ticket_key(s, p, ptemp, sc->server);
+ if ((rv = ssl_init_ticket_key(s, p, ptemp, sc->server)) != APR_SUCCESS) {
+ return rv;
+ }
#endif
return APR_SUCCESS;
@@ -1608,6 +1620,7 @@
ap_log_error(APLOG_MARK, APLOG_EMERG, rv, s, APLOGNO(02211)
"Failed to open Certificate Path `%s'",
ca_path);
+ sk_X509_NAME_pop_free(ca_list, X509_NAME_free);
return NULL;
}
diff -u ssl_engine_pphrase.c ssl_engine_pphrase.c
--- ssl_engine_pphrase.c (working copy)
+++ ssl_engine_pphrase.c (working copy)
@@ -211,11 +211,12 @@
|| sc->server->pkcs7); i++) {
const char *key_id;
int using_cache = 0;
- STACK_OF(X509) *certs;
-
- if (sc->server->pkcs7 &&
- (certs = ssl_read_pkcs7(pServ, sc->server->pkcs7))) {
+ if (sc->server->pkcs7) {
+ STACK_OF(X509) *certs = ssl_read_pkcs7(pServ,
+ sc->server->pkcs7);
+ if (!certs)
+ return APR_EGENERAL;
pX509Cert = sk_X509_value(certs, 0);
i = SSL_AIDX_MAX;
} else {
diff -u ssl_util.c ssl_util.c
--- ssl_util.c (working copy)
+++ ssl_util.c (working copy)
@@ -290,6 +290,7 @@
}
p7 = PEM_read_PKCS7(f, NULL, NULL, NULL);
+ fclose(f);
if (!p7) {
ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO(02274)
"Can't read PKCS7 object %s", pkcs7);
@@ -322,8 +323,6 @@
return NULL;
}
- fclose(f);
-
return certs;
}
