On Sat, Nov 23, 2013 at 7:24 AM, Kaspar Brand <httpd-dev.2...@velox.ch>wrote:
> 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.
>
Actually I was fooled by the variable names :)
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);
#ifdef HAVE_ECC
have_ecc = ssl_server_import_key(s, mctx, ecc_id, SSL_AIDX_ECC);
#endif
have_FOO should only be a boolean, right?
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.
(I hope I'm making sense!)
> > 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
>
--
Born in Roswell... married an alien...
http://emptyhammock.com/
