Mailing List Archive

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 <stefan@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 *);
> >
> >
>

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

> Am 06.05.2020 um 10:43 schrieb Ruediger Pluem <rpluem@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