Re: [PATCH 55593] Add SSLServerInfoFile directive

2013-10-13 Thread Kaspar Brand
On 13.10.2013 00:43, Trevor Perrin wrote:
 On Thu, Oct 10, 2013 at 4:44 PM, Dr Stephen Henson
 I *think* you then have to delve into ssl_pphrase_Handle() [note the comment 
 on
 the way in] and somehow link the ServerInfo index with something you can use 
 to
 recognise it later. The algorithm type 'at' might be usable or perhaps turn 
 the
 algorithm type into one of the SSL_AIDX_ALGORITHM values?
 
 I don't see a direct way to map ssl_algo_t to the SSL_AIX_* that's
 needed later.  I suppose something could be kludged out of
 ssl_util_algotypestr() and ssl_asn1_keystr().
 
 But maybe the easiest way to handle this is to create another hash
 table like tPublicCert (e.g. tServerInfoFile or tSSLConfCmd).
 
 This table could be populated in ssl_pphrase_Handle at the same time
 that the tPublicCert table is populated, and read in
 ssl_server_import_certs()?

Please not... as the comment in ssl_private.h already says, This should
really be fixed using a smaller structure.

As a proof of concept (or proof of my theory, if you like), I'm
attaching a patch which completely does without the whole
ssl_pphrase_Handle dance (with the limitation of not supporting
encrypted key files, currently).

 This would be easy to do as a directive, since only a ServerInfoFile
 string would be stored in the hash table, and no OpenSSL changes are
 needed.
 
 As an SSL_CONF_CMD, there's more work:
  - Add some indicator to distinguish per-cert vs global commands (?)
  - Serialize/deserialize SSL_CONF_CMD name/value lists into the hashtable
  - OpenSSL work:
- Implement SSL_CONF_CMD for ServerInfoFile
- Implement SSL_CONF_cmd_type(...) for relative path handling

Provided that OpenSSL adds support for KeyFile and CertificateFile to
SSL_CONF, you could simply replace the
SSL_CTX_use_certificate_chain_file()/SSL_CTX_use_PrivateKey_file() calls
with a replay of the whole SSL_CONF_CMD stanza, including ServerInfoFile.

 It seems like you guys are contemplating a larger redesign of cert/key
 handling based around SSL_CONF_CMD.
 
 Perhaps I could just do a directive for now, and let all this be swept
 into a big redesign later?

It depends on what your goal is. If it's a patch for your own needs,
then that's fine, but I'm clearly not in support of adding this to the
mod_ssl tree (not to trunk, but even less as a backport to 2.4.x).

Kaspar
Index: ssl_engine_init.c
===
--- ssl_engine_init.c   (revision 1531623)
+++ ssl_engine_init.c   (working copy)
@@ -185,6 +185,7 @@
 }
 #endif
 
+#if 0
 /*
  * read server private keys/public certs into memory.
  * decrypting any encrypted keys via configured SSLPassPhraseDialogs
@@ -192,6 +193,7 @@
  * restarts, in which case they'll live inside s-process-pool.
  */
 ssl_pphrase_Handle(base_server, ptemp);
+#endif
 
 /*
  * initialize the mutex handling
@@ -835,7 +837,9 @@
 
 if (mctx-pks) {
 /* XXX: proxy support? */
+#if 0
 ssl_init_ctx_cert_chain(s, p, ptemp, mctx);
+#endif
 #ifdef HAVE_TLSEXT
 ssl_init_ctx_tls_extensions(s, p, ptemp, mctx);
 #endif
@@ -1019,6 +1023,7 @@
 int have_ecc;
 #endif
 
+#if 0
 rsa_id = ssl_asn1_table_keyfmt(ptemp, vhost_id, SSL_AIDX_RSA);
 dsa_id = ssl_asn1_table_keyfmt(ptemp, vhost_id, SSL_AIDX_DSA);
 #ifdef HAVE_ECC
@@ -1061,6 +1066,36 @@
 Oops, no  KEYTYPES  server private key found?!);
 ssl_die(s);
 }
+#else
+const char *certfile, *keyfile;
+for (i = 0; (certfile = mctx-pks-cert_files[i]) != NULL; i++) {
+if ((SSL_CTX_use_certificate_chain_file(mctx-ssl_ctx, certfile)  1)) 
{
+ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO()
+ Failed to configure certificate #%d for %s, check 
%s,
+ i + 1, vhost_id, certfile);
+break;
+}
+keyfile = ((mctx-pks-key_files[i] != NULL) ?
+   mctx-pks-key_files[i] : certfile);
+if ((SSL_CTX_use_PrivateKey_file(mctx-ssl_ctx, keyfile,
+ SSL_FILETYPE_PEM)  1) ||
+(SSL_CTX_check_private_key(mctx-ssl_ctx)  1)) {
+ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, APLOGNO()
+ Failed to configure key #%d for %s, check %s,
+ i + 1, vhost_id, keyfile);
+break;
+}
+ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, APLOGNO()
+ Certificate and key #%d for %s configured from %s and 
%s,
+ i + 1, vhost_id, certfile, keyfile);
+}
+if (i  1) {
+ap_log_error(APLOG_MARK, APLOG_EMERG, 0, s, APLOGNO()
+ Failed to configure certificate and key for %s,
+ vhost_id);
+ssl_die(s);
+}
+#endif
 
 /*
  * Try to read DH parameters from the (first) SSLCertificateFile


Re: [PATCH 55593] Add SSLServerInfoFile directive

2013-10-13 Thread Kaspar Brand
On 11.10.2013 13:53, Dr Stephen Henson wrote:
 IMHO though there needs to be a way to be able to tie a directive to a
 certificate in mod_ssl anyway though. I'm surprised no one has needed to do 
 that
 before.

I'm not sure we really need this for mod_ssl, as configuring more than
one cert per vhost is probably a very rare case (the number of non-RSA
certs on public sites is extremely small - in the 2010 SSL Survey from
Qualys e.g., a few more than 100 out of 600,000 were DSA [1]). If people
deliberately want to go for something other than RSA, I would assume
that they either omit the RSA cert completely, or set up a dedicated
vhost for (EC)DSA.

 Well moving the SSL_CONF_CMD block does have some consequences. I placed it at
 (what I think is) the last possible point for a reason: so the SSL_CONF could
 reset just about anything set by Apache.

Currently, it's at the end of ssl_init_ctx_protocol(), which happens
still relatively early when looking at what ssl_init_ConfigureServer()
does (which is used to configure a specific vhost): for server mode,
ssl_init_ConfigureServer() calls ssl_init_server_ctx(), which in turn calls:

  ssl_init_server_check()
  ssl_init_ctx(), which consists of
ssl_init_ctx_protocol()
ssl_init_ctx_session_cache()
ssl_init_ctx_callbacks()
ssl_init_ctx_verify()
ssl_init_ctx_cipher_suite()
ssl_init_ctx_crl()
ssl_init_ctx_cert_chain()
  ssl_init_server_certs(), which consists of
ssl_server_import_cert(), once per type
ssl_server_import_key(), once per type
  ssl_init_ticket_key()

My PoC places the SSL_CTX_use_certificate_chain_file() and
SSL_CTX_use_PrivateKey_file() calls in ssl_init_server_certs, which
should also work fine for applying all SSL_CONF stuff with
SSL_CONF_CTX_set_ssl_ctx, I think.

 I think at least some twiddling with ssl_pphrase_Handle() would be needed
 because Apache will (I think) choke if you have no certificates configured.

Yes and no - see the PoC I attached to my previous message, where I
simply disabled the code relying on the tPublicCert and tPrivateKey
hashes. As the name of ssl_pphrase_Handle() says, it's raison d'ĂȘtre is
essentially handling encrypted private keys - and I hope that for the
future, we could strip it down to the minimum required.

Kaspar


[1] https://www.ssllabs.com/projects/ssl-survey/index.html