You are viewing a plain text version of this content. The canonical link for it is here.
Posted to cvs@httpd.apache.org by pq...@apache.org on 2007/06/08 04:48:05 UTC
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
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/CHANGES
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/CHANGES?view=diff&rev=545379&r1=545378&r2=545379
==============================================================================
--- httpd/httpd/trunk/CHANGES [utf-8] (original)
+++ httpd/httpd/trunk/CHANGES [utf-8] Thu Jun 7 19:48:04 2007
@@ -2,6 +2,8 @@
Changes with Apache 2.3.0
[Remove entries to the current 2.0 and 2.2 section below, when backported]
+ *) mod_ssl: Add support for caching SSL Sessions in memcached. [Paul Querna]
+
*) SECURITY: CVE-2007-1862 (cve.mitre.org)
mod_mem_cache: Copy headers into longer lived storage; header names and
values could previously point to cleaned up storage
Modified: httpd/httpd/trunk/modules/ssl/config.m4
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/config.m4?view=diff&rev=545379&r1=545378&r2=545379
==============================================================================
--- httpd/httpd/trunk/modules/ssl/config.m4 (original)
+++ httpd/httpd/trunk/modules/ssl/config.m4 Thu Jun 7 19:48:04 2007
@@ -86,6 +86,56 @@
fi
])
+
+
+AC_DEFUN([CHECK_SSL_MEMCACHE], [
+ AC_MSG_CHECKING(for ssl session caching in memcache)
+ ap_ssltk_mc="no"
+ tmp_nomessage=""
+ tmp_forced="no"
+ AC_ARG_ENABLE(ssl-memcache,
+ APACHE_HELP_STRING(--enable-ssl-memcache,Select memcache support in mod_ssl),
+ ap_ssltk_mc="$enableval"
+ tmp_nomessage=""
+ tmp_forced="yes"
+ if test "x$ap_ssltk_mc" = "x"; then
+ ap_ssltk_mc="yes"
+ dnl our "error"s become "tests revealed that..."
+ tmp_forced="no"
+ fi
+ if test "$ap_ssltk_mc" != "yes" -a "$ap_ssltk_mc" != "no"; then
+ tmp_nomessage="--enable-ssl-cache-memcache had illegal syntax - disabling"
+ ap_ssltk_mc="no"
+ fi)
+ if test "$tmp_forced" = "no"; then
+ AC_MSG_RESULT($ap_ssltk_mc (default))
+ else
+ AC_MSG_RESULT($ap_ssltk_mc (specified))
+ fi
+ if test "$tmp_forced" = "yes" -a "x$ap_ssltk_mc" = "xno" -a "x$tmp_nomessage" != "x"; then
+ AC_MSG_ERROR(ssl memcache support failed: $tmp_nomessage)
+ fi
+ if test "$ap_ssltk_mc" = "yes"; then
+ save_cpp=$CPPFLAGS
+ CPPFLAGS="$CPPFLAGS -I$APR_INCLUDEDIR -I$APU_INCLUDEDIR"
+ AC_CHECK_HEADER(
+ [apr_memcache.h],
+ [],
+ [tmp_nomessage="can't include apr_memcache headers"
+ ap_ssltk_mc="no"])
+
+ CPPFLAGS=$save_cpp
+
+ if test "$tmp_forced" = "yes" -a "x$ap_ssltk_mc" = "xno"; then
+ AC_MSG_ERROR(ssl memcache support failed: $tmp_nomessage)
+ fi
+ fi
+ if test "$ap_ssltk_mc" = "yes"; then
+ AC_DEFINE(HAVE_SSL_CACHE_MEMCACHE, 1, [Define if ssl-memcache support is enabled])
+ fi
+])
+
+
dnl # start of module specific part
APACHE_MODPATH_INIT(ssl)
@@ -110,6 +160,7 @@
ssl_scache_dbm.lo dnl
ssl_scache_shmcb.lo dnl
ssl_scache_dc.lo dnl
+ssl_scache_memcache.lo dnl
ssl_util.lo dnl
ssl_util_ssl.lo dnl
"
@@ -118,6 +169,7 @@
APACHE_CHECK_SSL_TOOLKIT
APR_SETVAR(MOD_SSL_LDADD, [\$(SSL_LIBS)])
CHECK_DISTCACHE
+ CHECK_SSL_MEMCACHE
if test "x$enable_ssl" = "xshared"; then
# The only symbol which needs to be exported is the module
# structure, so ask libtool to hide everything else:
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";
+#endif
+ }
else {
return "SSLSessionCache: Invalid argument";
}
Modified: httpd/httpd/trunk/modules/ssl/ssl_private.h
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_private.h?view=diff&rev=545379&r1=545378&r2=545379
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_private.h (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_private.h Thu Jun 7 19:48:04 2007
@@ -175,6 +175,7 @@
#endif
#endif
+
/**
* Define the certificate algorithm types
*/
@@ -277,6 +278,7 @@
SSL_SCMODE_DBM = 1,
SSL_SCMODE_SHMCB = 3,
SSL_SCMODE_DC = 4,
+ SSL_SCMODE_MC = 5,
SSL_SCMODE_NONE_NOT_NULL = 5
} ssl_scmode_t;
@@ -598,6 +600,15 @@
SSL_SESSION *ssl_scache_dc_retrieve(server_rec *, UCHAR *, int);
void ssl_scache_dc_remove(server_rec *, UCHAR *, int);
void ssl_scache_dc_status(request_rec *r, int flags, apr_pool_t *pool);
+
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+void ssl_scache_mc_init(server_rec *, apr_pool_t *);
+void ssl_scache_mc_kill(server_rec *);
+BOOL ssl_scache_mc_store(server_rec *, UCHAR *, int, time_t, SSL_SESSION *);
+SSL_SESSION *ssl_scache_mc_retrieve(server_rec *, UCHAR *, int);
+void ssl_scache_mc_remove(server_rec *, UCHAR *, int);
+void ssl_scache_mc_status(request_rec *r, int flags, apr_pool_t *pool);
+#endif
/** Proxy Support */
int ssl_proxy_enable(conn_rec *c);
Modified: httpd/httpd/trunk/modules/ssl/ssl_scache.c
URL: http://svn.apache.org/viewvc/httpd/httpd/trunk/modules/ssl/ssl_scache.c?view=diff&rev=545379&r1=545378&r2=545379
==============================================================================
--- httpd/httpd/trunk/modules/ssl/ssl_scache.c (original)
+++ httpd/httpd/trunk/modules/ssl/ssl_scache.c Thu Jun 7 19:48:04 2007
@@ -59,6 +59,10 @@
else if (mc->nSessionCacheMode == SSL_SCMODE_DC)
ssl_scache_dc_init(s, p);
#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (mc->nSessionCacheMode == SSL_SCMODE_MC)
+ ssl_scache_mc_init(s, p);
+#endif
else if (mc->nSessionCacheMode == SSL_SCMODE_SHMCB) {
void *data;
const char *userdata_key = "ssl_scache_init";
@@ -85,6 +89,10 @@
else if (mc->nSessionCacheMode == SSL_SCMODE_DC)
ssl_scache_dc_kill(s);
#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (mc->nSessionCacheMode == SSL_SCMODE_MC)
+ ssl_scache_mc_kill(s);
+#endif
return;
}
@@ -101,6 +109,10 @@
else if (mc->nSessionCacheMode == SSL_SCMODE_DC)
rv = ssl_scache_dc_store(s, id, idlen, expiry, sess);
#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (mc->nSessionCacheMode == SSL_SCMODE_MC)
+ rv = ssl_scache_mc_store(s, id, idlen, expiry, sess);
+#endif
return rv;
}
@@ -117,6 +129,10 @@
else if (mc->nSessionCacheMode == SSL_SCMODE_DC)
sess = ssl_scache_dc_retrieve(s, id, idlen);
#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (mc->nSessionCacheMode == SSL_SCMODE_MC)
+ sess = ssl_scache_mc_retrieve(s, id, idlen);
+#endif
return sess;
}
@@ -132,6 +148,10 @@
else if (mc->nSessionCacheMode == SSL_SCMODE_DC)
ssl_scache_dc_remove(s, id, idlen);
#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (mc->nSessionCacheMode == SSL_SCMODE_MC)
+ ssl_scache_mc_remove(s, id, idlen);
+#endif
return;
}
@@ -161,6 +181,10 @@
#ifdef HAVE_DISTCACHE
else if (sc->mc->nSessionCacheMode == SSL_SCMODE_DC)
ssl_scache_dc_status(r, flags, r->pool);
+#endif
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+ else if (sc->mc->nSessionCacheMode == SSL_SCMODE_MC)
+ ssl_scache_mc_status(r, flags, r->pool);
#endif
ap_rputs("</td></tr>\n", r);
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 @@
+/* Licensed to the Apache Software Foundation (ASF) under one or more
+* contributor license agreements. See the NOTICE file distributed with
+* this work for additional information regarding copyright ownership.
+* The ASF licenses this file to You under the Apache License, Version 2.0
+* (the "License"); you may not use this file except in compliance with
+* the License. You may obtain a copy of the License at
+*
+* http://www.apache.org/licenses/LICENSE-2.0
+*
+* Unless required by applicable law or agreed to in writing, software
+* distributed under the License is distributed on an "AS IS" BASIS,
+* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+* See the License for the specific language governing permissions and
+* limitations under the License.
+*/
+
+
+/* _ _
+ * _ __ ___ ___ __| | ___ ___| | mod_ssl
+ * | '_ ` _ \ / _ \ / _` | / __/ __| | Apache Interface to OpenSSL
+ * | | | | | | (_) | (_| | \__ \__ \ |
+ * |_| |_| |_|\___/ \__,_|___|___/___/_|
+ * |_____|
+ * ssl_scache_memcache.c
+ * Distributed Session Cache on top of memcached
+ */
+
+#include "ssl_private.h"
+
+#ifdef HAVE_SSL_CACHE_MEMCACHE
+
+#include "apr_memcache.h"
+#include "ap_mpm.h"
+
+/*
+ * SSL Session Caching using memcached as a backend.
+ */
+
+/*
+**
+** High-Level "handlers" as per ssl_scache.c
+**
+*/
+
+
+/* The underlying apr_memcache system is thread safe.. */
+static apr_memcache_t *memctxt;
+
+#define MC_TAG "mod_ssl:"
+#define MC_TAG_LEN \
+ (sizeof(MC_TAG))
+
+#define MC_KEY_LEN 254
+
+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 */
+ }
+
+ /* Should Max Conns be (thread_limit / nservers) ? */
+ rv = apr_memcache_server_create(p,
+ host_str, port,
+ 0,
+ 1,
+ thread_limit,
+ 600,
+ &st);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
+ "SSLSessionCache: Failed to Create Server: %s:%d",
+ host_str, port);
+ ssl_die();
+ }
+
+ rv = apr_memcache_add_server(memctxt, st);
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_CRIT, rv, s,
+ "SSLSessionCache: Failed to Add Server: %s:%d",
+ host_str, port);
+ ssl_die();
+ }
+
+ split = apr_strtok(NULL,",", &tok);
+ }
+
+ return;
+}
+
+void ssl_scache_mc_kill(server_rec *s)
+{
+
+}
+
+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++) {
+ apr_snprintf(cp, strsize - (cp-str), "%02X", id[n]);
+ 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,
+ (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"); */
+
+ return pSession;
+}
+
+void ssl_scache_mc_remove(server_rec *s, UCHAR *id, int idlen)
+{
+ 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;
+ }
+
+ rv = apr_memcache_delete(memctxt, strkey, 0);
+
+ if (rv != APR_SUCCESS) {
+ ap_log_error(APLOG_MARK, APLOG_DEBUG, rv, s,
+ "scache_mc: error deleting key '%s' ",
+ strkey);
+ return;
+ }
+}
+
+void ssl_scache_mc_status(request_rec *r, int flags, apr_pool_t *pool)
+{
+ /* SSLModConfigRec *mc = myModConfig(r->server); */
+ /* TODO: Make a mod_status handler. meh. */
+}
+
+#endif
Propchange: httpd/httpd/trunk/modules/ssl/ssl_scache_memcache.c
------------------------------------------------------------------------------
svn:eol-style = native
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
Posted by Paul Querna <ch...@force-elite.com>.
Ruediger Pluem wrote:
>> +#else
>> + return "SSLSessionCache: distcache support disabled";
>
> Nitpicking: Should memcache instead of distcache.
Fixed in r545538.
>> 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 @@
....
>> + 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?
>
Fixed in r546707.
>> + /* 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?
I've made them over-ridable compile time #defines for now -- making them
accessible from the config file with a reasonable syntax seems like a
painful challenge right now. (SSLSessionCache atm can only be configured
on a signle command/line atm).
>> +
>> +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?
Yup. Changed in r546708.
....
>> +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?
I've re-worked pool allocations for both memcache and DBM backends, in
r545608 and r545610, but there are still some places in the DBM cache
where allocate from the configuration pool; However we should be good
now in the memcache cache backend.
....
>> + /* ap_log_error(APLOG_MARK, APLOG_DEBUG, 0, s,
>> + "scache_mc: 'get_session' HIT"); */
>
> Why the comments above?
Useful for development/debugging, but they would be be hit 100% if they
weren't commented out....
Thank You for reviewing,
-Paul
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
Posted by Paul Querna <ch...@force-elite.com>.
Ruediger Pluem wrote:
>
> On 06/08/2007 04:48 AM, pquerna@apache.org wrote:
>> Author: pquerna
>> Date: Thu Jun 7 19:48:04 2007
>> New Revision: 545379
....
>> +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?
No, its not the correct pool to use -- but this is the pool that the DBM
backend uses.... So, ugh. Looks like the dbm backend also leaks :-)
I think the right fix is to change ssl_callback_GetSessionCacheEntry in
ssl_engine_kernel.c, and pass down either the conn_rec* or the conn's
pool. This of course means changing the prototype on
ssl_scache_retrieve and everything it calls... Thoughts?
Should likely do the same with
ssl_callback_NewSessionCacheEntry/ssl_scache_store....
-Paul
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
Posted by Ruediger Pluem <rp...@apache.org>.
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