Aside from the technical change (that IIUC it is really nice), I really love when the code is so well documented. Having comments in the code and in the commit message is great for whoever is in a similar situation like mine (namely some knowledge of httpd internals but still very far from Yann/Eric/Ruediger/Joe/Stefan/etc..'s level :D). So all the extra time put into documentation is really helpful!
Luca On Mon, May 4, 2020 at 10:37 AM ste...@eissing.org <ste...@eissing.org> wrote: > > Neat. > > > Am 04.05.2020 um 10:32 schrieb jor...@apache.org: > > > > Author: jorton > > Date: Mon May 4 08:32:23 2020 > > New Revision: 1877345 > > > > URL: http://svn.apache.org/viewvc?rev=1877345&view=rev > > Log: > > 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 > > > > Modified: > > httpd/httpd/trunk/modules/ssl/ssl_engine_config.c > > httpd/httpd/trunk/modules/ssl/ssl_engine_init.c > > httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c > > httpd/httpd/trunk/modules/ssl/ssl_private.h > > > > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_config.c > > URL: > > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_config.c?rev=1877345&r1=1877344&r2=1877345&view=diff > > ============================================================================== > > --- httpd/httpd/trunk/modules/ssl/ssl_engine_config.c (original) > > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_config.c Mon May 4 08:32:23 > > 2020 > > @@ -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 (vmc) { > > - return vmc; /* reused for lifetime of the server */ > > + if (ap_server_conf && s != ap_server_conf) { > > + SSLSrvConfigRec *sc = mySrvConfig(ap_server_conf); > > + > > + AP_DEBUG_ASSERT(sc->mc); > > + > > + return sc->mc; > > } > > > > /* > > @@ -68,8 +68,6 @@ SSLModConfigRec *ssl_config_global_creat > > 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_creat > > mc->fips = UNSET; > > #endif > > > > - apr_pool_userdata_set(mc, SSL_MOD_CONFIG_KEY, > > - apr_pool_cleanup_null, > > - pool); > > + mc->retained = ap_retained_data_get(MODSSL_RETAINED_KEY); > > + if (!mc->retained) { > > + /* 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_ > > { > > SSLSrvConfigRec *sc = ssl_config_server_new(p); > > > > - sc->mc = ssl_config_global_create(s); > > + sc->mc = ssl_config_global_create(p, s); > > > > return sc; > > } > > > > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_init.c > > URL: > > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_init.c?rev=1877345&r1=1877344&r2=1877345&view=diff > > ============================================================================== > > --- httpd/httpd/trunk/modules/ssl/ssl_engine_init.c (original) > > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_init.c Mon May 4 08:32:23 2020 > > @@ -226,6 +226,8 @@ apr_status_t ssl_init_Module(apr_pool_t > > 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 > > /* > > * 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 > > 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_cert > > /* 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)) { > > > > Modified: httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c > > URL: > > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c?rev=1877345&r1=1877344&r2=1877345&view=diff > > ============================================================================== > > --- httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c (original) > > +++ httpd/httpd/trunk/modules/ssl/ssl_engine_pphrase.c Mon May 4 08:32:23 > > 2020 > > @@ -71,38 +71,25 @@ static apr_status_t exists_and_readable( > > return APR_SUCCESS; > > } > > > > -/* > > - * reuse vhost keys for asn1 tables where keys are allocated out > > - * of s->process->pool to prevent "leaking" each time we format > > - * a vhost key. since the key is stored in a table with lifetime > > - * of s->process->pool, the key needs to have the same lifetime. > > - * > > - * 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) > > +/* Returns the vhost-key-id which is the index into the > > + * mc->retained->privkeys hash table. The returned string is > > + * allocated from the same pool as that hash table, to ensure it has > > + * the correct (process) lifetime of the retained data. */ > > +static const char *privkey_vhost_keyid(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, > > - APR_HASH_KEY_STRING); > > + 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 */ > > - keyptr = apr_pstrdup(mc->pPool, key); > > - apr_hash_set(mc->tVHostKeys, keyptr, > > - APR_HASH_KEY_STRING, keyptr); > > + /* Make a copy in the (process) pool used for the retained data. */ > > + keyptr = apr_pstrdup(apr_hash_pool_get(mc->retained->privkeys), > > key); > > + apr_hash_set(mc->retained->key_ids, keyptr, APR_HASH_KEY_STRING, > > keyptr); > > } > > > > - return (char *)keyptr; > > + return keyptr; > > } > > > > /* _________________________________________________________________ > > @@ -134,7 +121,7 @@ apr_status_t ssl_load_encrypted_pkey(ser > > { > > 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(ser > > * 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(ser > > > > /* 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 */ > > > > Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h > > URL: > > http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_private.h?rev=1877345&r1=1877344&r2=1877345&view=diff > > ============================================================================== > > --- httpd/httpd/trunk/modules/ssl/ssl_private.h (original) > > +++ httpd/httpd/trunk/modules/ssl/ssl_private.h Mon May 4 08:32:23 2020 > > @@ -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 > > - * allocated out of the "process" pool and only a single such > > - * structure is created and used for the lifetime of the process. > > - * (The process pool is s->process->pool and is stored in the .pPool > > - * field.) Most members of this structure are likewise allocated out > > - * of the process pool, but notably sesscache and sesscache_context > > - * are not. > > +/* Private keys are retained across reloads, since decryption > > + * passphrases can only be entered during startup (before detaching > > + * from a terminal). This structure is stored via the ap_retained_* > > + * API and retrieved on later module reloads. If the structure > > + * changes, the key name must be changed by increasing the digit at > > + * the end, to avoid an updated version of mod_ssl loading retained > > + * data with a different struct definition. > > * > > - * The structure is treated as mostly immutable after a single config > > - * parse has completed; the post_config hook (ssl_init_Module) flips > > - * the bFixed flag to true and subsequent invocations of the config > > - * callbacks hence do nothing. > > - * > > - * This odd lifetime strategy is used so that encrypted private keys > > - * can be decrypted once at startup and continue to be used across > > - * subsequent server reloads where the interactive password prompt is > > - * not possible. > > - > > - * 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. > > - */ > > + * All objects used here must be allocated from the process pool > > + * (s->process->pool) so they also survives restarts. */ > > +#define MODSSL_RETAINED_KEY "mod_ssl-retained-1" > > + > > +typedef struct { > > + /* A hash table of vhost key-IDs used to index the privkeys hash, > > + * for example the string "vhost.example.com:443:0". For each > > + * (key, value) pair the value is the same as the key, allowing > > + * 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; > > + > > 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_p > > 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 *); > > > > >
