mod_ssl: Use retained data API for storing private keys across reloads.

Allocate SSLModConfigRec from pconf rather than the process pool.

* modules/ssl/ssl_private.h: Add modssl_retained_data_t structure and
  move private key storage here from SSLModConfigRec.  Add retained
  pointer to SSLModConfigRec.

* modules/ssl/ssl_engine_config.c (ssl_config_global_create): Take
  pool argument; allocate SSLModConfigRec from there and
  initialize mc->retained.  SSLModConfigRec no longer cached for the
  process lifetime.
  (ssl_init_Module): Sanity check that sc->mc is correct.
  (ssl_init_server_certs): Use private keys from mc->retained.

* modules/ssl/ssl_engine_pphrase.c
  (privkey_vhost_keyid): Rename from asn1_table_vhost_key and
  update to use the retained structure.
  (ssl_load_encrypted_pkey): Update for above.

* modules/ssl/ssl_engine_init.c (ssl_init_Module): Remove
  (apparently) redundant call to ssl_config_global_create and
  add debug asserts to validate that is safe.

Github: closes #119


git-svn-id: https://svn.apache.org/repos/asf/httpd/httpd/trunk@1877345 13f79535-47bb-0310-9956-ffa450edef68

modules/ssl/ssl_engine_config.c

@@ -40,17 +40,17 @@
 **  _________________________________________________________________
 */
 
-#define SSL_MOD_CONFIG_KEY "ssl_module"
 
-SSLModConfigRec *ssl_config_global_create(server_rec *s)
+static SSLModConfigRec *ssl_config_global_create(apr_pool_t *pool, server_rec *s)
 {
-    apr_pool_t *pool = s->process->pool;
     SSLModConfigRec *mc;
-    void *vmc;
 
-    apr_pool_userdata_get(&vmc, SSL_MOD_CONFIG_KEY, pool);
+    if (ap_server_conf && s != ap_server_conf) {
-    if (vmc) {
+        SSLSrvConfigRec *sc = mySrvConfig(ap_server_conf);
-        return vmc; /* reused for lifetime of the server */
+
+        AP_DEBUG_ASSERT(sc->mc);
+
+        return sc->mc;
     }
 
     /*
@@ -68,8 +68,6 @@ SSLModConfigRec *ssl_config_global_create(server_rec *s)
     mc->pMutex                 = NULL;
     mc->aRandSeed              = apr_array_make(pool, 4,
                                                 sizeof(ssl_randseed_t));
-    mc->tVHostKeys             = apr_hash_make(pool);
-    mc->tPrivateKey            = apr_hash_make(pool);
 #if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
     mc->szCryptoDevice         = NULL;
 #endif
@@ -86,9 +84,15 @@ SSLModConfigRec *ssl_config_global_create(server_rec *s)
     mc->fips = UNSET;
 #endif
 
-    apr_pool_userdata_set(mc, SSL_MOD_CONFIG_KEY,
+    mc->retained = ap_retained_data_get(MODSSL_RETAINED_KEY);
-                          apr_pool_cleanup_null,
+    if (!mc->retained) {
-                          pool);
+        /* Allocate the retained data; the hash table is allocated out
+         * of the process pool. */
+        mc->retained = ap_retained_data_create(MODSSL_RETAINED_KEY,
+                                               sizeof *mc->retained);
+        mc->retained->privkeys = apr_hash_make(s->process->pool);
+        mc->retained->key_ids = apr_hash_make(s->process->pool);
+    }
 
     return mc;
 }
@@ -248,7 +252,7 @@ void *ssl_config_server_create(apr_pool_t *p, server_rec *s)
 {
     SSLSrvConfigRec *sc = ssl_config_server_new(p);
 
-    sc->mc = ssl_config_global_create(s);
+    sc->mc = ssl_config_global_create(p, s);
 
     return sc;
 }

modules/ssl/ssl_engine_init.c

@@ -226,6 +226,8 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_pool_t *plog,
     apr_status_t rv;
     apr_array_header_t *pphrases;
 
+    AP_DEBUG_ASSERT(mc);
+
     if (SSLeay() < MODSSL_LIBRARY_VERSION) {
         ap_log_error(APLOG_MARK, APLOG_WARNING, 0, base_server, APLOGNO(01882)
                      "Init: this version of mod_ssl was compiled against "
@@ -250,7 +252,6 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_pool_t *plog,
     /*
      * Any init round fixes the global config
      */
-    ssl_config_global_create(base_server); /* just to avoid problems */
     ssl_config_global_fix(mc);
 
     /*
@@ -260,6 +261,8 @@ apr_status_t ssl_init_Module(apr_pool_t *p, apr_pool_t *plog,
     for (s = base_server; s; s = s->next) {
         sc = mySrvConfig(s);
 
+        AP_DEBUG_ASSERT(sc->mc == mc);
+
         if (sc->server) {
             sc->server->sc = sc;
         }
@@ -1441,7 +1444,7 @@ static apr_status_t ssl_init_server_certs(server_rec *s,
             /* perhaps it's an encrypted private key, so try again */
             ssl_load_encrypted_pkey(s, ptemp, i, keyfile, &pphrases);
 
-            if (!(asn1 = ssl_asn1_table_get(mc->tPrivateKey, key_id)) ||
+            if (!(asn1 = ssl_asn1_table_get(mc->retained->privkeys, key_id)) ||
                 !(ptr = asn1->cpData) ||
                 !(pkey = d2i_AutoPrivateKey(NULL, &ptr, asn1->nData)) ||
                 (SSL_CTX_use_PrivateKey(mctx->ssl_ctx, pkey) < 1)) {

modules/ssl/ssl_engine_pphrase.c

@@ -71,38 +71,25 @@ static apr_status_t exists_and_readable(const char *fname, apr_pool_t *pool,
     return APR_SUCCESS;
 }
 
-/*
+/* Returns the vhost-key-id which is the index into the
- * reuse vhost keys for asn1 tables where keys are allocated out
+ * mc->retained->privkeys hash table.  The returned string is
- * of s->process->pool to prevent "leaking" each time we format
+ * allocated from the same pool as that hash table, to ensure it has
- * a vhost key.  since the key is stored in a table with lifetime
+ * the correct (process) lifetime of the retained data. */
- * of s->process->pool, the key needs to have the same lifetime.
+static const char *privkey_vhost_keyid(SSLModConfigRec *mc, apr_pool_t *p,
- *
- * XXX: probably seems silly to use a hash table with keys and values
- * being the same, but it is easier than doing a linear search
- * and will make it easier to remove keys if needed in the future.
- * also have the problem with apr_array_header_t that if we
- * underestimate the number of vhost keys when we apr_array_make(),
- * the array will get resized when we push past the initial number
- * of elts.  this resizing in the s->process->pool means "leaking"
- * since apr_array_push() will apr_alloc arr->nalloc * 2 elts,
- * leaving the original arr->elts to waste.
- */
-static const char *asn1_table_vhost_key(SSLModConfigRec *mc, apr_pool_t *p,
                                        const char *id, int i)
 {
     /* 'p' pool used here is cleared on restarts (or sooner) */
     char *key = apr_psprintf(p, "%s:%d", id, i);
-    void *keyptr = apr_hash_get(mc->tVHostKeys, key,
+    const char *keyptr = apr_hash_get(mc->retained->key_ids, key,
                                       APR_HASH_KEY_STRING);
 
     if (!keyptr) {
-        /* make a copy out of s->process->pool */
+        /* Make a copy in the (process) pool used for the retained data. */
-        keyptr = apr_pstrdup(mc->pPool, key);
+        keyptr = apr_pstrdup(apr_hash_pool_get(mc->retained->privkeys), key);
-        apr_hash_set(mc->tVHostKeys, keyptr,
+        apr_hash_set(mc->retained->key_ids, keyptr, APR_HASH_KEY_STRING, keyptr);
-                     APR_HASH_KEY_STRING, keyptr);
     }
 
-    return (char *)keyptr;
+    return keyptr;
 }
 
 /*  _________________________________________________________________
@@ -134,7 +121,7 @@ apr_status_t ssl_load_encrypted_pkey(server_rec *s, apr_pool_t *p, int idx,
 {
     SSLModConfigRec *mc = myModConfig(s);
     SSLSrvConfigRec *sc = mySrvConfig(s);
-    const char *key_id = asn1_table_vhost_key(mc, p, sc->vhost_id, idx);
+    const char *key_id = privkey_vhost_keyid(mc, p, sc->vhost_id, idx);
     EVP_PKEY *pPrivateKey = NULL;
     ssl_asn1_t *asn1;
     int nPassPhrase = (*pphrases)->nelts;
@@ -187,7 +174,7 @@ apr_status_t ssl_load_encrypted_pkey(server_rec *s, apr_pool_t *p, int idx,
      * are used to give a better idea as to what failed.
      */
     if (pkey_mtime) {
-        ssl_asn1_t *asn1 = ssl_asn1_table_get(mc->tPrivateKey, key_id);
+        ssl_asn1_t *asn1 = ssl_asn1_table_get(mc->retained->privkeys, key_id);
         if (asn1 && (asn1->source_mtime == pkey_mtime)) {
             ap_log_error(APLOG_MARK, APLOG_INFO, 0, s, APLOGNO(02575)
                          "Reusing existing private key from %s on restart",
@@ -345,7 +332,7 @@ apr_status_t ssl_load_encrypted_pkey(server_rec *s, apr_pool_t *p, int idx,
 
     /* Cache the private key in the global module configuration so it
      * can be used after subsequent reloads. */
-    asn1 = ssl_asn1_table_set(mc->tPrivateKey, key_id, pPrivateKey);
+    asn1 = ssl_asn1_table_set(mc->retained->privkeys, key_id, pPrivateKey);
 
     if (ppcb_arg.nPassPhraseDialogCur != 0) {
         /* remember mtime of encrypted keys */

modules/ssl/ssl_private.h

@@ -553,34 +553,37 @@ typedef struct {
     int vhost_found;          /* whether we found vhost from SNI already */
 } SSLConnRec;
 
-/* BIG FAT WARNING: SSLModConfigRec has unusual memory lifetime: it is
+/* Private keys are retained across reloads, since decryption
- * allocated out of the "process" pool and only a single such
+ * passphrases can only be entered during startup (before detaching
- * structure is created and used for the lifetime of the process.
+ * from a terminal).  This structure is stored via the ap_retained_*
- * (The process pool is s->process->pool and is stored in the .pPool
+ * API and retrieved on later module reloads.  If the structure
- * field.)  Most members of this structure are likewise allocated out
+ * changes, the key name must be changed by increasing the digit at
- * of the process pool, but notably sesscache and sesscache_context
+ * the end, to avoid an updated version of mod_ssl loading retained
- * are not.
+ * data with a different struct definition.
  *
- * The structure is treated as mostly immutable after a single config
+ * All objects used here must be allocated from the process pool
- * parse has completed; the post_config hook (ssl_init_Module) flips
+ * (s->process->pool) so they also survives restarts. */
- * the bFixed flag to true and subsequent invocations of the config
+#define MODSSL_RETAINED_KEY "mod_ssl-retained-1"
- * callbacks hence do nothing.
+
- *
+typedef struct {
- * This odd lifetime strategy is used so that encrypted private keys
+    /* A hash table of vhost key-IDs used to index the privkeys hash,
- * can be decrypted once at startup and continue to be used across
+     * for example the string "vhost.example.com:443:0".  For each
- * subsequent server reloads where the interactive password prompt is
+     * (key, value) pair the value is the same as the key, allowing
- * not possible.
+     * the keys to be retrieved on subsequent reloads rather than
+     * rellocated.  ### This optimisation seems to be of dubious
+     * value.  Allocating the vhost-key-ids from pconf and duping them
+     * when storing them in ->privkeys would be simpler. */
+    apr_hash_t *key_ids;
+
+    /* A hash table of pointers to ssl_asn1_t structures.  The
+     * structures are used to store private keys in raw DER format
+     * (serialized OpenSSL PrivateKey structures).  The table is
+     * indexed by key-IDs from the key_ids hash table. */
+    apr_hash_t *privkeys;
+
+    /* Do NOT add fields here without changing the key name, as above. */
+} modssl_retained_data_t;
 
- * It is really an ABI nightmare waiting to happen since DSOs are
- * reloaded across restarts, and nothing prevents the struct type
- * changing across such reloads, yet the cached structure will be
- * assumed to match regardless.
- *
- * This should really be fixed using a smaller structure which only
- * stores that which is absolutely necessary (the private keys, maybe
- * the random seed), and have that structure be strictly ABI-versioned
- * for safety.
- */
 typedef struct {
     pid_t           pid;
     apr_pool_t     *pPool;
@@ -589,6 +592,9 @@ typedef struct {
     /* OpenSSL SSL_SESS_CACHE_* flags: */
     long            sesscache_mode;
 
+    /* Data retained across reloads. */
+    modssl_retained_data_t *retained;
+
     /* The configured provider, and associated private data
      * structure. */
     const ap_socache_provider_t *sesscache;
@@ -596,13 +602,6 @@ typedef struct {
 
     apr_global_mutex_t   *pMutex;
     apr_array_header_t   *aRandSeed;
-    apr_hash_t     *tVHostKeys;
-
-    /* A hash table of pointers to ssl_asn1_t structures.  The structures
-     * are used to store private keys in raw DER format (serialized OpenSSL
-     * PrivateKey structures).  The table is indexed by (vhost-id,
-     * index), for example the string "vhost.example.com:443:0". */
-    apr_hash_t     *tPrivateKey;
 
 #if defined(HAVE_OPENSSL_ENGINE_H) && defined(HAVE_ENGINE_INIT)
     const char     *szCryptoDevice;
@@ -814,7 +813,6 @@ SSLSrvConfigRec *ssl_policy_lookup(apr_pool_t *pool, const char *name);
 extern module AP_MODULE_DECLARE_DATA ssl_module;
 
 /**  configuration handling   */
-SSLModConfigRec *ssl_config_global_create(server_rec *);
 void         ssl_config_global_fix(SSLModConfigRec *);
 BOOL         ssl_config_global_isfixed(SSLModConfigRec *);
 void        *ssl_config_server_create(apr_pool_t *, server_rec *);