On 06/08/2007 04:48 AM, [EMAIL PROTECTED] 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/[EMAIL > PROTECTED] > > 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
