httpd-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ruediger Pluem <rpl...@apache.org>
Subject Re: svn commit: r545379 - in /httpd/httpd/trunk: CHANGES modules/ssl/config.m4 modules/ssl/ssl_engine_config.c modules/ssl/ssl_private.h modules/ssl/ssl_scache.c modules/ssl/ssl_scache_memcache.c
Date Fri, 08 Jun 2007 08:50:48 GMT


On 06/08/2007 04:48 AM, pquerna@apache.org wrote:
> Author: pquerna
> Date: Thu Jun  7 19:48:04 2007
> New Revision: 545379
> 
> URL: http://svn.apache.org/viewvc?view=rev&rev=545379
> Log:
> Add support for distributed caching of SSL Sessions inside memcached, using apr_memcache,
which is present in APR-Util 1.3/trunk.
> 
> This was originally written at ApacheCon US 2005 (San Diego), and was sent to the list:
> http://mail-archives.apache.org/mod_mbox/httpd-dev/200512.mbox/%3C439C6C07.9030904@force-elite.com%3E
> 
> This version is slightly cleaned up, and of course, uses the now bundled apr_memcache,
rather than an external dependency.
> 
> 
> Added:
>     httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c   (with props)
> Modified:
>     httpd/httpd/trunk/CHANGES
>     httpd/httpd/trunk/modules/ssl/config.m4
>     httpd/httpd/trunk/modules/ssl/ssl_engine_config.c
>     httpd/httpd/trunk/modules/ssl/ssl_private.h
>     httpd/httpd/trunk/modules/ssl/ssl_scache.c
> 

> 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?view=diff&rev=545379&r1=545378&r2=545379
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_engine_config.c (original)
> +++ httpd/httpd/trunk/modules/ssl/ssl_engine_config.c Thu Jun  7 19:48:04 2007
> @@ -1034,6 +1034,19 @@
>          return "SSLSessionCache: distcache support disabled";
>  #endif
>      }
> +    else if ((arglen > 3) && strcEQn(arg, "memcache:", 9)) {
> +#ifdef HAVE_SSL_CACHE_MEMCACHE
> +        mc->nSessionCacheMode      = SSL_SCMODE_MC;
> +        mc->szSessionCacheDataFile = apr_pstrdup(mc->pPool, arg+9);
> +        if (!mc->szSessionCacheDataFile) {
> +            return apr_pstrcat(cmd->pool,
> +                               "SSLSessionCache: Invalid memcache config: ",
> +                               arg+9, NULL);
> +        }
> +#else
> +        return "SSLSessionCache: distcache support disabled";

Nitpicking: Should memcache instead of distcache.


> Added: httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c
> URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c?view=auto&rev=545379
> ==============================================================================
> --- httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c (added)
> +++ httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c Thu Jun  7 19:48:04 2007
> @@ -0,0 +1,289 @@


> +
> +void ssl_scache_mc_init(server_rec *s, apr_pool_t *p)
> +{
> +    apr_status_t rv;
> +    int thread_limit = 0;
> +    int nservers = 0;
> +    char *cache_config;
> +    char *split;
> +    char *tok;
> +    SSLModConfigRec *mc = myModConfig(s);
> +
> +    ap_mpm_query(AP_MPMQ_HARD_LIMIT_THREADS, &thread_limit);
> +
> +    if (mc->szSessionCacheDataFile == NULL) {
> +        ap_log_error(APLOG_MARK, APLOG_ERR, 0, s, "SSLSessionCache required");
> +        ssl_die();
> +    }
> +
> +    /* Find all the servers in the first run to get a total count */
> +    cache_config = apr_pstrdup(p, mc->szSessionCacheDataFile);
> +    split = apr_strtok(cache_config, ",", &tok);
> +    while (split) {
> +        nservers++;
> +        split = apr_strtok(NULL,",", &tok);
> +    }
> +
> +    rv = apr_memcache_create(p, nservers, 0, &memctxt);
> +    if (rv != APR_SUCCESS) {
> +        ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
> +                     "SSLSessionCache: Failed to create Memcache Object of '%d' size.",

> +                     nservers);
> +        ssl_die();
> +    }
> +
> +    /* Now add each server to the memcache */
> +    cache_config = apr_pstrdup(p, mc->szSessionCacheDataFile);
> +    split = apr_strtok(cache_config, ",", &tok);
> +    while (split) {
> +        apr_memcache_server_t *st;
> +        char *host_str;
> +        char *scope_id;
> +        apr_port_t port;
> +
> +        rv = apr_parse_addr_port(&host_str, &scope_id, &port, split, p);
> +        if (rv != APR_SUCCESS) {
> +            ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
> +                         "SSLSessionCache: Failed to Parse Server: '%s'", split);
> +            ssl_die();
> +        }
> +
> +        if (host_str == NULL) {
> +            ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
> +                         "SSLSessionCache: Failed to Parse Server, "
> +                         "no hostname specified: '%s'", split);
> +            ssl_die();
> +        }
> +
> +        if (port == 0) {
> +            port = 11211; /* default port */

Shouldn't we use a define for the default port and refer to it instead of using it directly?

> +        }
> +
> +        /* Should Max Conns be (thread_limit / nservers) ? */
> +        rv = apr_memcache_server_create(p,
> +                                        host_str, port,
> +                                        0,
> +                                        1,
> +                                        thread_limit, 
> +                                        600,
> +                                        &st);

Having at least some of the values configurable would be an enhancement (Sorry for wanting
to have
everything perfect from the start :-))
Nevertheless shouldn't we use defines for these default values (except thread_limit of course)
and refer to them instead of using the values directly?


> +
> +static char *mc_session_id2sz(unsigned char *id, int idlen,
> +                               char *str, int strsize)
> +{
> +    char *cp;
> +    int n;
> + 
> +    cp = apr_cpystrn(str, MC_TAG, MC_TAG_LEN);
> +    for (n = 0; n < idlen && n < (MC_KEY_LEN - MC_TAG_LEN); n++) {

Hm. It seems to me that our upper limit for n is (MC_KEY_LEN - MC_TAG_LEN)/2
and not (MC_KEY_LEN - MC_TAG_LEN) since every element of id consumes 2 bytes in the
target buffer. Furthermore I would think that using (strsize - MC_TAG_LEN)/2
as a precalculated value would be even bettter.

> +        apr_snprintf(cp, strsize - (cp-str), "%02X", id[n]);

Couldn't we replace

strsize - (cp-str)

just with

2

once the loop condition is fixed?

> +        cp += 2;
> +    }
> +    *cp = '\0';
> +    return str;
> +}
> +
> +BOOL ssl_scache_mc_store(server_rec *s, UCHAR *id, int idlen,
> +                           time_t timeout, SSL_SESSION *pSession)
> +{
> +    char buf[MC_KEY_LEN];
> +    char *strkey = NULL;
> +    UCHAR ucaData[SSL_SESSION_MAX_DER];
> +    UCHAR *ucp;
> +    int nData;
> +    apr_status_t rv;
> +
> +    /* streamline session data */
> +    if ((nData = i2d_SSL_SESSION(pSession, NULL)) > sizeof(ucaData)) {
> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
> +                     "scache_mc: streamline session data size too large: %d > "
> +                     "%" APR_SIZE_T_FMT,
> +                     nData, sizeof(ucaData));
> +        return FALSE;
> +    }
> +
> +    ucp = ucaData;
> +    i2d_SSL_SESSION(pSession, &ucp);
> +
> +    strkey = mc_session_id2sz(id, idlen, buf, sizeof(buf));
> +    if(!strkey) {
> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s, "scache_mc: Key generation borked.");
> +        return FALSE;
> +    }
> +
> +    timeout -= time(NULL);
> +
> +    timeout = apr_time_sec(timeout);
> +
> +    rv = apr_memcache_set(memctxt, strkey, (char*)ucp, nData, timeout, 0);
> +
> +    if (rv != APR_SUCCESS) {
> +        ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
> +                     "scache_mc: error setting key '%s' "
> +                     "with %d bytes of data", strkey, nData);
> +        return FALSE;
> +    }
> +
> +    return TRUE;
> +}
> +
> +SSL_SESSION *ssl_scache_mc_retrieve(server_rec *s, UCHAR *id, int idlen)
> +{
> +    SSL_SESSION *pSession;
> +    MODSSL_D2I_SSL_SESSION_CONST unsigned char *pder;
> +    apr_size_t der_len;
> +    SSLModConfigRec *mc = myModConfig(s);
> +    char buf[MC_KEY_LEN];
> +    char* strkey = NULL;
> +    apr_status_t rv;
> +
> +    strkey = mc_session_id2sz(id, idlen, buf, sizeof(buf));
> +
> +    if(!strkey) {
> +        ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
> +                     "scache_mc: Key generation borked.");
> +        return NULL;
> +    }
> +
> +    rv = apr_memcache_getp(memctxt,  mc->pPool, strkey,

Is this the correct pool to use? AFAICT the data stored in *pder
will only be used locally, because d2i_SSL_SESSION allocates
new memory to create the pSession object.
mc->pPool seems to be a pool with a long lifetime to me and I fear
that we introduce a memory leak here. How about a temporary subpool
if no other pool is at hand here?

> +                           (char**)&pder, &der_len, NULL);
> +
> +    if (rv == APR_NOTFOUND) {
> +        /* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
> +                       "scache_mc: 'get_session' MISS"); */
> +        return NULL;
> +    }
> +
> +    if (rv != APR_SUCCESS) {
> +        ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> +                     "scache_mc: 'get_session' FAIL");
> +        return NULL;
> +    }
> +
> +    if (der_len > SSL_SESSION_MAX_DER) {
> +        ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> +                     "scache_mc: 'get_session' OVERFLOW");
> +        return NULL;
> +    }
> +
> +    pSession = d2i_SSL_SESSION(NULL, &pder, der_len);
> +
> +    if (!pSession) {
> +        ap_log_error(APLOG_MARK, APLOG_ERR, 0, s,
> +                     "scache_mc: 'get_session' CORRUPT");
> +        return NULL;
> +    }
> +
> +    /* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
> +        "scache_mc: 'get_session' HIT"); */

Why the comments above?


Regards

RĂ¼diger

Mime
View raw message