Re: ssl_die() and pool cleanup

2013-11-23 Thread Kaspar Brand 
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

Re: ssl_die() and pool cleanup

2013-11-23 Thread Jeff Trawick 
On Sat, Nov 23, 2013 at 7:24 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 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/

RE: ssl_die() and pool cleanup

2013-11-23 Thread Falco Schwarz 

 Date: Sat, 23 Nov 2013 13:24:53 +0100
 From: httpd-dev.2...@velox.ch
 To: dev@httpd.apache.org
 Subject: Re: ssl_die() and pool cleanup

 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.

Wouldn't that essentially remove the ability to configure a _default_:443 
VirtualHost with an non-existing Servername, whose only purpose is to serve a 
certificate for all other VirtualHosts?

e.g.

VirtualHost _default_:443
    ServerName nonexistant.domain
    SSLEngine On
    SSLCertificateFile   conf/ssl/www.example.com.cer
    SSLCertificateKeyFile    conf/ssl/www.example.com.key
    SSLCertificateChainFile  conf/ssl/www.example.com.ca
/VirtualHost

VirtualHost *:80 *:443
    ServerName  www.example.com

    [...]
/VirtualHost

- If one sets www.example.com as ServerName of the _default_ VirtualHost, then 
all requests are served by this vhost instead of the wildcard one.

- If mod_ssl would refuse to start the server because the _default_ vhost 
ServerName does not match a certificate, then you would have to use a wildcard 
certificate to make use of a _default_ vhost.


So, if the sanity check is skipped for the _default_ host, or there is a better 
way to set the ServerName of the _default_ host, which I don't know yet, then 
this wouldn't be affected.

Re: ssl_die() and pool cleanup

2013-11-23 Thread Eric Covener 
 So, if the sanity check is skipped for the _default_ host, or there is a 
 better way to set the ServerName of the _default_ host, which I don't know 
 yet, then this wouldn't be affected.

I don't think any behavior should be based on _default_ vs. *.

Your scenario probably works the same with the first VH as * simply
because it's the first listed NVH.

RE: ssl_die() and pool cleanup

2013-11-23 Thread Falco Schwarz 

 Date: Sat, 23 Nov 2013 08:18:14 -0500
 Subject: Re: ssl_die() and pool cleanup
 From: cove...@gmail.com
 To: dev@httpd.apache.org

 So, if the sanity check is skipped for the _default_ host, or there is a 
 better way to set the ServerName of the _default_ host, which I don't know 
 yet, then this wouldn't be affected.

 I don't think any behavior should be based on _default_ vs. *.

 Your scenario probably works the same with the first VH as * simply
 because it's the first listed NVH.

You are right, there should be no difference between _default_ vs. *.

Yet, this does not change the fact, that you have to explicitly set a 
ServerName for the first VH, different to the CN in the certificate. Otherwise 
all requests would be served by the first VH, instead of the other ones.

If mod_ssl would decline VH's with a nonmatching ServerName, then a 
configuration with a VirtualHost :80 :443 would be impossible without a 
wildcard certificate, at least to my knowledge.

Re: ssl_die() and pool cleanup

2013-11-23 Thread Kaspar Brand 
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)

Re: ssl_die() and pool cleanup

2013-11-23 Thread Jeff Trawick 
On Sat, Nov 23, 2013 at 8:51 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 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


IMO it adds some future-proofing and self explanation to return early if
(rv != APR_SUCCESS  rv != APR_NOTFOUND) instead of just checking for
equality with APR_EGENERAL.

YMMV :)

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: ssl_die() and pool cleanup

2013-11-23 Thread Kaspar Brand 
On 23.11.2013 15:03, Jeff Trawick wrote:
 IMO it adds some future-proofing and self explanation to return early if
 (rv != APR_SUCCESS  rv != APR_NOTFOUND) instead of just checking for
 equality with APR_EGENERAL.

Done with r1544812. Note that I'm working towards nukeing
ssl_server_import_{cert,key} soon
(https://people.apache.org/~kbrand/mod_ssl_pkey_2013-11-18_wip.patch),
so future proofing is less of a concern to me at that point :-)

Kaspar

Re: ssl_die() and pool cleanup

2013-11-23 Thread Jeff Trawick 
On Sat, Nov 23, 2013 at 10:02 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 On 23.11.2013 15:03, Jeff Trawick wrote:
  IMO it adds some future-proofing and self explanation to return early if
  (rv != APR_SUCCESS  rv != APR_NOTFOUND) instead of just checking for
  equality with APR_EGENERAL.

 Done with r1544812. Note that I'm working towards nukeing
 ssl_server_import_{cert,key} soon
 (https://people.apache.org/~kbrand/mod_ssl_pkey_2013-11-18_wip.patch),
 so future proofing is less of a concern to me at that point :-)

 Kaspar


Got it.  Thanks a bunch for taking care of this mess!

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: ssl_die() and pool cleanup

2013-11-22 Thread Jeff Trawick 
On Wed, Nov 20, 2013 at 5:19 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 On 18.11.2013 14:59, Jeff Trawick wrote:
  Has anyone looked at making ssl_die() clean up pools on the way out
  (presumably by calling some function besides exit())?  It is rather easy
 to
  end up with a bunch of stranded IPC objects while debugging your SSL
 config.

 Oh yes, a major annoyance I'm also occasionally running into.

   * XXX: The config hooks should return errors instead of calling
 exit().

 Gave it a try, see attachment. Not yet extensively tested (*), so
 perhaps incomplete. But httpd now properly cleans up for me, e.g. when a
 SIGHUPing fails, as shown in this log extract:

  [Wed Nov 20 11:02:14.304528 2013] [mpm_worker:notice] [pid 23918:tid
 3214934016] AH00298: SIGHUP received.  Attempting to restart
  [Wed Nov 20 11:02:14.544660 2013] [ssl:info] [pid 23918:tid 3214934016]
 AH02200: Loading certificate  private key of SSL-aware server 'server:443'
  [Wed Nov 20 11:02:14.545137 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 AH02241: Init: Unable to read server certificate from file /tmp/snakeoil.pem
  [Wed Nov 20 11:02:14.545240 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 SSL Library Error: error:0D06B08E:asn1 encoding
 routines:ASN1_D2I_READ_BIO:not enough data
  [Wed Nov 20 11:02:14.545278 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 AH02312: Fatal error initialising mod_ssl, exiting.
  [Wed Nov 20 11:02:14.545310 2013] [:emerg] [pid 23918:tid 3214934016]
 AH00020: Configuration Failed, exiting

 (Configuration Failed, exiting is the key here - this comes from
 main.c and will call destroy_and_exit_process() to clean up.)

 Kaspar

 (*) The changes related to ssl_read_pkcs7 in particular are fairly
 superficial, but I think we should drop that PKCS#7 stuff from mod_ssl
 anyway.


This is what I found:

The two calls to ssl_init_ctx() (engine_init) need to be checked for rv !=
APR_SUCCESS.

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())

The call to ssl_init_server_check() (engine_init) needs to be checked for
rv != APR_SUCCESS.

call to ssl_init_ctx_protocol() also needs a check
same for ssl_init_ticket_key()

It looks like some errors in the proxy config that previously were ignored
now cause startup failures...  (shrug)

Not bad for boiling the ocean :)

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

Re: ssl_die() and pool cleanup

2013-11-22 Thread Yann Ylavic 
Maybe sk_X509_NAME_pop_free(ca_list, X509_NAME_free) should be called too
before returning NULL in ssl_init_FindCAList() (so to avoid a leak in case
of failure).

Regards;
Yann.


On Fri, Nov 22, 2013 at 4:20 PM, Jeff Trawick traw...@gmail.com wrote:

 On Wed, Nov 20, 2013 at 5:19 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 On 18.11.2013 14:59, Jeff Trawick wrote:
  Has anyone looked at making ssl_die() clean up pools on the way out
  (presumably by calling some function besides exit())?  It is rather
 easy to
  end up with a bunch of stranded IPC objects while debugging your SSL
 config.

 Oh yes, a major annoyance I'm also occasionally running into.

   * XXX: The config hooks should return errors instead of calling
 exit().

 Gave it a try, see attachment. Not yet extensively tested (*), so
 perhaps incomplete. But httpd now properly cleans up for me, e.g. when a
 SIGHUPing fails, as shown in this log extract:

  [Wed Nov 20 11:02:14.304528 2013] [mpm_worker:notice] [pid 23918:tid
 3214934016] AH00298: SIGHUP received.  Attempting to restart
  [Wed Nov 20 11:02:14.544660 2013] [ssl:info] [pid 23918:tid 3214934016]
 AH02200: Loading certificate  private key of SSL-aware server 'server:443'
  [Wed Nov 20 11:02:14.545137 2013] [ssl:emerg] [pid 23918:tid
 3214934016] AH02241: Init: Unable to read server certificate from file
 /tmp/snakeoil.pem
  [Wed Nov 20 11:02:14.545240 2013] [ssl:emerg] [pid 23918:tid
 3214934016] SSL Library Error: error:0D06B08E:asn1 encoding
 routines:ASN1_D2I_READ_BIO:not enough data
  [Wed Nov 20 11:02:14.545278 2013] [ssl:emerg] [pid 23918:tid
 3214934016] AH02312: Fatal error initialising mod_ssl, exiting.
  [Wed Nov 20 11:02:14.545310 2013] [:emerg] [pid 23918:tid 3214934016]
 AH00020: Configuration Failed, exiting

 (Configuration Failed, exiting is the key here - this comes from
 main.c and will call destroy_and_exit_process() to clean up.)

 Kaspar

 (*) The changes related to ssl_read_pkcs7 in particular are fairly
 superficial, but I think we should drop that PKCS#7 stuff from mod_ssl
 anyway.


 This is what I found:

 The two calls to ssl_init_ctx() (engine_init) need to be checked for rv !=
 APR_SUCCESS.

 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())

 The call to ssl_init_server_check() (engine_init) needs to be checked for
 rv != APR_SUCCESS.

 call to ssl_init_ctx_protocol() also needs a check
 same for ssl_init_ticket_key()

 It looks like some errors in the proxy config that previously were ignored
 now cause startup failures...  (shrug)

 Not bad for boiling the ocean :)


 --
 Born in Roswell... married an alien...
 http://emptyhammock.com/

Re: ssl_die() and pool cleanup

2013-11-20 Thread Jeff Trawick 
On Wed, Nov 20, 2013 at 5:19 AM, Kaspar Brand httpd-dev.2...@velox.chwrote:

 On 18.11.2013 14:59, Jeff Trawick wrote:
  Has anyone looked at making ssl_die() clean up pools on the way out
  (presumably by calling some function besides exit())?  It is rather easy
 to
  end up with a bunch of stranded IPC objects while debugging your SSL
 config.

 Oh yes, a major annoyance I'm also occasionally running into.

   * XXX: The config hooks should return errors instead of calling
 exit().

 Gave it a try, see attachment. Not yet extensively tested (*), so
 perhaps incomplete. But httpd now properly cleans up for me, e.g. when a
 SIGHUPing fails, as shown in this log extract:


It looks fine at first glance...  I'll try to test a few error paths soon
whether or not it has already been committed at the time.

Thanks!



  [Wed Nov 20 11:02:14.304528 2013] [mpm_worker:notice] [pid 23918:tid
 3214934016] AH00298: SIGHUP received.  Attempting to restart
  [Wed Nov 20 11:02:14.544660 2013] [ssl:info] [pid 23918:tid 3214934016]
 AH02200: Loading certificate  private key of SSL-aware server 'server:443'
  [Wed Nov 20 11:02:14.545137 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 AH02241: Init: Unable to read server certificate from file /tmp/snakeoil.pem
  [Wed Nov 20 11:02:14.545240 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 SSL Library Error: error:0D06B08E:asn1 encoding
 routines:ASN1_D2I_READ_BIO:not enough data
  [Wed Nov 20 11:02:14.545278 2013] [ssl:emerg] [pid 23918:tid 3214934016]
 AH02312: Fatal error initialising mod_ssl, exiting.
  [Wed Nov 20 11:02:14.545310 2013] [:emerg] [pid 23918:tid 3214934016]
 AH00020: Configuration Failed, exiting

 (Configuration Failed, exiting is the key here - this comes from
 main.c and will call destroy_and_exit_process() to clean up.)

 Kaspar

 (*) The changes related to ssl_read_pkcs7 in particular are fairly
 superficial, but I think we should drop that PKCS#7 stuff from mod_ssl
 anyway.




-- 
Born in Roswell... married an alien...
http://emptyhammock.com/

ssl_die() and pool cleanup

2013-11-18 Thread Jeff Trawick 
Has anyone looked at making ssl_die() clean up pools on the way out
(presumably by calling some function besides exit())?  It is rather easy to
end up with a bunch of stranded IPC objects while debugging your SSL config.

Commentary in ssl_die() suggests a great goal, but that's essentially
boiling the ocean at this point.

/*
 * This is used for fatal errors and here
 * it is common module practice to really
 * exit from the complete program.
 * XXX: The config hooks should return errors instead of calling exit().
 */
exit(1);

-- 
Born in Roswell... married an alien...
http://emptyhammock.com/