You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@httpd.apache.org by "stefan@eissing.org" <st...@eissing.org> on 2020/05/04 08:36:46 UTC
Re: svn commit: r1877345 - in /httpd/httpd/trunk/modules/ssl:
ssl_engine_config.c ssl_engine_init.c ssl_engine_pphrase.c ssl_private.h
Neat.
> Am 04.05.2020 um 10:32 schrieb jorton@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 *);
>
>
Re: svn commit: r1877345 - in /httpd/httpd/trunk/modules/ssl:
ssl_engine_config.c ssl_engine_init.c ssl_engine_pphrase.c ssl_private.h
Posted by Stefan Eissing <st...@greenbytes.de>.
> Am 06.05.2020 um 10:43 schrieb Ruediger Pluem <rp...@apache.org>:
>
>
>
> On 5/6/20 10:27 AM, Luca Toscano wrote:
>> 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!
>
> It is not only helpful for you. It is helpful for everyone, because at least
> I cannot remember all the things next time I have to look into that piece of
> code again and it eases review a lot. Plus it reduces questions back to the committer
> if I get the change directly :-).
> So a big +1 and thank you from me as well for this documentation.
At my age, I need my comments to understand what I once intended but had gone terribly wrong. ;-)
Glad to hear that someone else can make use of them!
Cheers, Stefan
Re: svn commit: r1877345 - in /httpd/httpd/trunk/modules/ssl:
ssl_engine_config.c ssl_engine_init.c ssl_engine_pphrase.c ssl_private.h
Posted by Ruediger Pluem <rp...@apache.org>.
On 5/6/20 10:27 AM, Luca Toscano wrote:
> 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!
It is not only helpful for you. It is helpful for everyone, because at least
I cannot remember all the things next time I have to look into that piece of
code again and it eases review a lot. Plus it reduces questions back to the committer
if I get the change directly :-).
So a big +1 and thank you from me as well for this documentation.
Regards
Rüdiger
Re: svn commit: r1877345 - in /httpd/httpd/trunk/modules/ssl:
ssl_engine_config.c ssl_engine_init.c ssl_engine_pphrase.c ssl_private.h
Posted by Luca Toscano <to...@gmail.com>.
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 stefan@eissing.org <st...@eissing.org> wrote:
>
> Neat.
>
> > Am 04.05.2020 um 10:32 schrieb jorton@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 *);
> >
> >
>