[serf] r1982 committed - Add API for reusing SSL sessions....

[se...@googlecode.com]

Revision: 1982
Author: chemodax
Date: Tue Jul 2 08:28:23 2013
Log: Add API for reusing SSL sessions.

* serf_bucket_types.h
(SERF_HAS_SSL_SESSION_API): New macro to detected serf API support in
compile time.
(serf_ssl_session_t, serf_ssl_session_export, serf_ssl_session_import,
serf_ssl_new_session_t, serf_ssl_new_session_callback_set,
ssl_resume_session): New.

* buckets/ssl_buckets.c
(serf_ssl_context_t): Add new_session_cb and new_session_cb_baton.
(serf_ssl_session_t): New.
(serf_ssl_new_session_callback_set, new_session, serf_ssl_session_export,
cleanup_session, serf_ssl_session_import, ssl_resume_session): New.
(ssl_init_context): Initialize new_session_cb to NULL. Enable
SSL_CTX_sess_set_new_cb callback and disable internal session management.

* test/serf_get.c
(app_baton_t): Add session_filename.
(new_ssl_session, read_ssl_session): New.
(conn_setup): Attempt to reuse existing SSL session if requested.
(print_usage: Mention new '-s' option.
(main): Parse new '-s' option.

http://code.google.com/p/serf/source/detail?r=1982

Modified:
/trunk/buckets/ssl_buckets.c
/trunk/serf_bucket_types.h
/trunk/test/serf_get.c

=======================================
--- /trunk/buckets/ssl_buckets.c Sun Apr 21 13:19:52 2013
+++ /trunk/buckets/ssl_buckets.c Tue Jul 2 08:28:23 2013
@@ -173,6 +173,9 @@
serf_ssl_server_cert_chain_cb_t server_cert_chain_callback;
void *server_cert_userdata;

+ serf_ssl_new_session_t new_session_cb;
+ void *new_session_cb_baton;
+
const char *cert_path;

X509 *cached_cert;
@@ -201,6 +204,10 @@
int depth;
};

+struct serf_ssl_session_t {
+ SSL_SESSION *session_obj;
+};
+
static void disable_compression(serf_ssl_context_t *ssl_ctx);

#if SSL_VERBOSE
@@ -1193,6 +1200,93 @@
context->server_cert_userdata = data;
}

+void serf_ssl_new_session_callback_set(
+ serf_ssl_context_t *context,
+ serf_ssl_new_session_t new_session_cb,
+ void *baton)
+{
+ context->new_session_cb = new_session_cb;
+ context->new_session_cb_baton = baton;
+}
+
+static int new_session(SSL *ssl, SSL_SESSION *sess)
+{
+ serf_ssl_context_t *ctx = SSL_get_app_data(ssl);
+
+ if (ctx->new_session_cb) {
+ serf_ssl_session_t session;
+ apr_pool_t *subpool;
+
+ session.session_obj = sess;
+ apr_pool_create(&subpool, ctx->pool);
+
+ ctx->new_session_cb(&session, ctx->new_session_cb_baton, subpool);
+
+ apr_pool_destroy(subpool);
+ }
+
+ return 0;
+}
+
+apr_status_t serf_ssl_session_export(void **data_p,
+ apr_size_t *len_p,
+ const serf_ssl_session_t *session,
+ apr_pool_t *pool)
+{
+ int sess_len;
+ void *sess_data;
+ unsigned char *p;
+
+ sess_len = i2d_SSL_SESSION(session->session_obj, NULL);
+ if (!sess_len) {
+ return APR_EGENERAL;
+ }
+
+ sess_data = apr_palloc(pool, sess_len);
+ p = sess_data;
+ sess_len = i2d_SSL_SESSION(session->session_obj, &p);
+ if (!sess_len) {
+ return APR_EGENERAL;
+ }
+
+ *data_p = sess_data;
+ *len_p = sess_len;
+ return APR_SUCCESS;
+}
+
+static apr_status_t cleanup_session(void *data)
+{
+ serf_ssl_session_t *session = data;
+
+ SSL_SESSION_free(session->session_obj);
+ session->session_obj = NULL;
+
+ return APR_SUCCESS;
+}
+
+apr_status_t serf_ssl_session_import(const serf_ssl_session_t **session_p,
+ void *data,
+ apr_size_t len,
+ apr_pool_t *pool)
+{
+ SSL_SESSION *sess;
+ serf_ssl_session_t *session;
+ const unsigned char *p = data;
+
+ sess = d2i_SSL_SESSION(NULL, &p, len);
+
+ if (!sess) {
+ return APR_EGENERAL;
+ }
+
+ session = apr_pcalloc(pool, sizeof(serf_ssl_session_t));
+ session->session_obj = sess;
+ apr_pool_cleanup_register(pool, session, cleanup_session,
cleanup_session);
+
+ *session_p = session;
+ return APR_SUCCESS;
+}
+
static serf_ssl_context_t *ssl_init_context(void)
{
serf_ssl_context_t *ssl_ctx;
@@ -1226,6 +1320,15 @@
SSL_CTX_set_verify(ssl_ctx->ctx, SSL_VERIFY_PEER,
validate_server_certificate);
SSL_CTX_set_options(ssl_ctx->ctx, SSL_OP_ALL);
+
+ ssl_ctx->new_session_cb = NULL;
+
+ /* Enable SSL callback for new sessions and disable internal session
+ handling. */
+ SSL_CTX_set_session_cache_mode(
+ ssl_ctx->ctx, SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL);
+ SSL_CTX_sess_set_new_cb(ssl_ctx->ctx, new_session);
+
/* Disable SSL compression by default. */
disable_compression(ssl_ctx);

@@ -1260,6 +1363,15 @@

return ssl_ctx;
}
+
+apr_status_t ssl_resume_session(
+ serf_ssl_context_t *ssl_ctx,
+ const serf_ssl_session_t *session,
+ apr_pool_t *pool)
+{
+ SSL_set_session(ssl_ctx->ssl, session->session_obj);
+ return APR_SUCCESS;
+}

static apr_status_t ssl_free_context(
serf_ssl_context_t *ssl_ctx)
=======================================
--- /trunk/serf_bucket_types.h Mon Jun 3 18:06:15 2013
+++ /trunk/serf_bucket_types.h Tue Jul 2 08:28:23 2013
@@ -553,6 +553,43 @@
serf_ssl_server_cert_chain_cb_t cert_chain_callback,
void *data);

+/* Define preprocessor macro for easy detection of serf capability.*/
+#define SERF_HAS_SSL_SESSION_API 1
+
+typedef struct serf_ssl_session_t serf_ssl_session_t;
+
+/* Exports @a session to continous memory block. */
+apr_status_t serf_ssl_session_export(void **data,
+ apr_size_t *len,
+ const serf_ssl_session_t *session,
+ apr_pool_t *pool);
+
+/* Restores previously saved session from continuous memory block @a data
with
+ * @a len length. */
+apr_status_t serf_ssl_session_import(const serf_ssl_session_t **session,
+ void *data,
+ apr_size_t len,
+ apr_pool_t *pool);
+
+/**
+ * Callback to notify when new SSL session is negotiated.
+ */
+typedef apr_status_t (*serf_ssl_new_session_t)(
+ serf_ssl_session_t *ssl_session,
+ void *baton,
+ apr_pool_t *pool);
+
+void serf_ssl_new_session_callback_set(
+ serf_ssl_context_t *context,
+ serf_ssl_new_session_t new_session_cb,
+ void *baton);
+
+/* Configure @a ssl_ctx to attempt resume exisiting @a ssl_session. */
+apr_status_t ssl_resume_session(
+ serf_ssl_context_t *ssl_ctx,
+ const serf_ssl_session_t *ssl_session,
+ apr_pool_t *pool);
+
/**
* Use the default root CA certificates as included with the OpenSSL
library.
*/
=======================================
--- /trunk/test/serf_get.c Wed Jun 26 04:47:21 2013
+++ /trunk/test/serf_get.c Tue Jul 2 08:28:23 2013
@@ -30,6 +30,7 @@
int using_ssl;
serf_ssl_context_t *ssl_ctx;
serf_bucket_alloc_t *bkt_alloc;
+ const char *session_filename;
} app_baton_t;

static void closed_connection(serf_connection_t *conn,
@@ -134,6 +135,67 @@
apr_pool_destroy(pool);
return APR_SUCCESS;
}
+
+static apr_status_t
+new_ssl_session(const serf_ssl_session_t *session,
+ void *baton,
+ apr_pool_t *pool)
+{
+ void *data;
+ apr_size_t len;
+ apr_status_t status;
+ apr_file_t *file;
+ app_baton_t *ctx = baton;
+
+ if (ctx->session_filename) {
+ status = serf_ssl_session_export(&data, &len, session, pool);
+
+ if (status) {
+ return status;
+ }
+
+ status = apr_file_open(&file, ctx->session_filename,
+ APR_WRITE|APR_CREATE, APR_OS_DEFAULT, pool);
+ if (status) {
+ return status;
+ }
+
+ apr_file_write_full(file, data, len, NULL);
+ apr_file_close(file);
+
+ fprintf(stderr, "Saved SSL session to '%s'\n",
ctx->session_filename);
+ }
+
+ return APR_SUCCESS;
+}
+
+static apr_status_t read_ssl_session(const serf_ssl_session_t **session,
+ const char *filename,
+ apr_pool_t *pool)
+{
+ apr_status_t status;
+ char buf[32*1014];
+ apr_file_t *file;
+ apr_size_t len;
+
+ status = apr_file_open(&file, filename, APR_READ, APR_OS_DEFAULT,
pool);
+ if (status) {
+ return status;
+ }
+
+ status = apr_file_read_full(file, buf, sizeof(buf), &len);
+
+ /* We should reach EOF. */
+ if (status == APR_EOF) {
+ status = serf_ssl_session_import(session, buf, len, pool);
+ } else {
+ status = APR_EGENERAL;
+ }
+
+ apr_file_close(file);
+
+ return status;
+}

static apr_status_t conn_setup(apr_socket_t *skt,
serf_bucket_t **input_bkt,
@@ -155,6 +217,23 @@
print_certs, NULL);
serf_ssl_set_hostname(ctx->ssl_ctx, ctx->hostinfo);

+ if (ctx->session_filename) {
+ serf_ssl_session_t *session;
+ apr_status_t status;
+
+ serf_ssl_new_session_callback_set(ctx->ssl_ctx,
new_ssl_session, ctx);
+ status = read_ssl_session(&session, ctx->session_filename,
pool);
+ if (status == APR_SUCCESS) {
+ fprintf(stderr, "Using SSL session from '%s'\n",
+ ctx->session_filename);
+ ssl_resume_session(ctx->ssl_ctx, session, pool);
+ }
+ else {
+ fprintf(stderr, "Cannot read SSL session from '%s': %d\n",
+ ctx->session_filename, status);
+ }
+ }
+
*output_bkt = serf_bucket_ssl_encrypt_create(*output_bkt,
ctx->ssl_ctx,
ctx->bkt_alloc);
}
@@ -366,6 +445,7 @@
puts("-m <method> Use the <method> HTTP Method");
puts("-f <file> Use the <file> as the request body");
puts("-p <hostname:port> Use the <host:port> as proxy server");
+ puts("-s <filename> Read and write SSL session to specified file");
}

int main(int argc, const char **argv)
@@ -402,10 +482,11 @@
method = "GET";
/* Do not print headers by default. */
print_headers = 0;
+ app_ctx.session_filename = NULL;

apr_getopt_init(&opt, pool, argc, argv);

- while ((status = apr_getopt(opt, "U:P:f:hHm:n:vp:x:", &opt_c,
&opt_arg)) ==
+ while ((status = apr_getopt(opt, "U:P:f:hHm:n:vp:x:s:", &opt_c,
&opt_arg)) ==
APR_SUCCESS) {

switch (opt_c) {
@@ -452,6 +533,8 @@
case 'v':
puts("Serf version: " SERF_VERSION_STRING);
exit(0);
+ case 's':
+ app_ctx.session_filename = opt_arg;
default:
break;
}

[Greg Stein]

On Tue, Jul 2, 2013 at 11:28 AM, <se...@googlecode.com> wrote:
> Revision: 1982
> Author: chemodax
> Date: Tue Jul 2 08:28:23 2013
> Log: Add API for reusing SSL sessions.
>
> * serf_bucket_types.h
> (SERF_HAS_SSL_SESSION_API): New macro to detected serf API support in
> compile time.
> (serf_ssl_session_t, serf_ssl_session_export, serf_ssl_session_import,
> serf_ssl_new_session_t, serf_ssl_new_session_callback_set,
> ssl_resume_session): New.

This last needs the serf_ prefix.

>...

> +++ /trunk/buckets/ssl_buckets.c Tue Jul 2 08:28:23 2013

>...

> +apr_status_t serf_ssl_session_export(void **data_p,
> + apr_size_t *len_p,
> + const serf_ssl_session_t *session,
> + apr_pool_t *pool)
> +{
> + int sess_len;
> + void *sess_data;
> + unsigned char *p;
> +
> + sess_len = i2d_SSL_SESSION(session->session_obj, NULL);
> + if (!sess_len) {
> + return APR_EGENERAL;
> + }
> +
> + sess_data = apr_palloc(pool, sess_len);
> + p = sess_data;
> + sess_len = i2d_SSL_SESSION(session->session_obj, &p);

This juggling of pointers isn't needed. Just declare sess_data as
unsigned char *. When you assign it to *data_p, it will be converted
properly.

>...

Cheers,
-g

[Ivan Zhakov]

On Tue, Jul 2, 2013 at 9:34 PM, Greg Stein <gst...@gmail.com> wrote:
> On Tue, Jul 2, 2013 at 11:28 AM, <se...@googlecode.com> wrote:
>> Revision: 1982
>> Author: chemodax
>> Date: Tue Jul 2 08:28:23 2013
>> Log: Add API for reusing SSL sessions.
>>
>> * serf_bucket_types.h
>> (SERF_HAS_SSL_SESSION_API): New macro to detected serf API support in
>> compile time.
>> (serf_ssl_session_t, serf_ssl_session_export, serf_ssl_session_import,
>> serf_ssl_new_session_t, serf_ssl_new_session_callback_set,
>> ssl_resume_session): New.
>
> This last needs the serf_ prefix.
>

Thanks! Fixed in r1986.

>>...
>> +++ /trunk/buckets/ssl_buckets.c Tue Jul 2 08:28:23 2013
>>...
>> +apr_status_t serf_ssl_session_export(void **data_p,
>> + apr_size_t *len_p,
>> + const serf_ssl_session_t *session,
>> + apr_pool_t *pool)
>> +{
>> + int sess_len;
>> + void *sess_data;
>> + unsigned char *p;
>> +
>> + sess_len = i2d_SSL_SESSION(session->session_obj, NULL);
>> + if (!sess_len) {
>> + return APR_EGENERAL;
>> + }
>> +
>> + sess_data = apr_palloc(pool, sess_len);
>> + p = sess_data;
>> + sess_len = i2d_SSL_SESSION(session->session_obj, &p);
>
> This juggling of pointers isn't needed. Just declare sess_data as
> unsigned char *. When you assign it to *data_p, it will be converted
> properly.
>

You're thinking in terms of proper API, but OpenSSL is different :)
i2d_SSL_SESSION() increments pointer P.



--
Ivan Zhakov
CTO | VisualSVN | http://www.visualsvn.com

[Greg Stein]

On Wed, Jul 3, 2013 at 4:17 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Tue, Jul 2, 2013 at 9:34 PM, Greg Stein <gst...@gmail.com> wrote:

>...

>> This juggling of pointers isn't needed. Just declare sess_data as
>> unsigned char *. When you assign it to *data_p, it will be converted
>> properly.
>
> You're thinking in terms of proper API, but OpenSSL is different :)
> i2d_SSL_SESSION() increments pointer P.

Seriously? Geezus. That's not mentioned in the docs.

Stupid stupid stupid...

(please add comments to the code about that problem, so nobody tries
to "fix" the code)

[Lieven Govaerts]

Hi Ivan,

On Tue, Jul 2, 2013 at 5:28 PM, <se...@googlecode.com> wrote:
> Revision: 1982
> Author: chemodax
> Date: Tue Jul 2 08:28:23 2013
> Log: Add API for reusing SSL sessions.

can you describe in detail what the goal is for this change? I find it
a lot of code with yet another difficult to use API for the
application.
I know we have talked about this at the svn hackathon, but I can't
remember the exact use case you want to improve.

What is the size of a SSL_SESSION in memory or on disk? Why would you
not keep this in memory?

Is the expected performance improvement really that much to go all the
way of saving and retrieving files from disk?

I don't like this macro. Compile-time feature checking has until now
always been done with the SERF_VERSION_AT_LEAST macro, I propose we
don't start changing that now.

Lieven

> --
> You received this message because you are subscribed to the Google Groups
> "Serf Development List" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to serf-dev+u...@googlegroups.com.
> To post to this group, send email to serf...@googlegroups.com.
> Visit this group at http://groups.google.com/group/serf-dev.
> For more options, visit https://groups.google.com/groups/opt_out.
>
>

[Ivan Zhakov]

Good point. Done in r1987.

[Greg Stein]

On Wed, Jul 3, 2013 at 5:22 AM, Lieven Govaerts
<lieven....@gmail.com> wrote:
>...

>> +/* Define preprocessor macro for easy detection of serf capability.*/
>> +#define SERF_HAS_SSL_SESSION_API 1
>

> I don't like this macro. Compile-time feature checking has until now
> always been done with the SERF_VERSION_AT_LEAST macro, I propose we
> don't start changing that now.

Very good point. I've come to hate APR's flags for features. The API
should have "everything", so a simple version test should be able to
tell you what is present.

Thanks,
-g

[Ivan Zhakov]

Removed in r1988. But what do you think about changing trunk version to 1.4?

--
Ivan Zhakov

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 2:47 PM, Lieven Govaerts
<lieven....@gmail.com> wrote:
> On Wed, Jul 3, 2013 at 11:45 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:

>> On Wed, Jul 3, 2013 at 1:22 PM, Lieven Govaerts
>> <lieven....@gmail.com> wrote:
>>> Hi Ivan,
>>>
>>> On Tue, Jul 2, 2013 at 5:28 PM, <se...@googlecode.com> wrote:
>>>> Revision: 1982
>>>> Author: chemodax
>>>> Date: Tue Jul 2 08:28:23 2013
>>>> Log: Add API for reusing SSL sessions.
>>>
>>> can you describe in detail what the goal is for this change? I find it
>>> a lot of code with yet another difficult to use API for the
>>> application.
>>> I know we have talked about this at the svn hackathon, but I can't
>>> remember the exact use case you want to improve.

>> The idea is to reuse already negotiated SSL session for other
>> connections. This save one round trip to server and reduce CPU usage.
>> See [2]. For example:
>>
>> Without session reuse:
>>> ptime serf_get https://svn.apache.org
>> 1.3 seconds
>>
>> First run with session reusing:
>>> ptime serf_get -s sess.bin https://svn.apache.org
>> 1.3 seconds
>>
>> Second run:
>>> ptime serf_get -s sess.bin https://svn.apache.org
>> 0.8 seconds
>
> On Mac OS X (with SSD) I get:
> lgo-macbp2:serftrunk lgo$ time test/serf_get https://svn.apache.org >
> /dev/null 2>&1
>
> real 0m0.231s
> user 0m0.007s
> sys 0m0.004s
> lgo-macbp2:serftrunk lgo$ time test/serf_get -s sess.bin
> https://svn.apache.org > /dev/null 2>&1
>
> real 0m0.230s
> user 0m0.007s
> sys 0m0.004s
> lgo-macbp2:serftrunk lgo$ time test/serf_get -s sess.bin
> https://svn.apache.org > /dev/null 2>&1
>
> real 0m0.119s
> user 0m0.004s
> sys 0m0.004s
>
> In percentage the speedup is even more noticeable, but in absolute
> numbers the difference on Windows is much more important, I suppose
> that is because of the slow random generator in OpenSSL on Windows.
>
It seems you're using EU mirror in your tests, while I was using US server.

Try 'serf_get https://svn.eu.apache.org' and 'serf_get
https://svn.us.apache.org'. But as you see performance is significant
even for nearby server.

>> Short-term goal is to implement in-memory SSL session storage and
>> reuse in Subversion.

>>
>>>> + ssl_ctx->new_session_cb = NULL;
>>>> +
>>>> + /* Enable SSL callback for new sessions and disable internal session
>>>> + handling. */
>>>> + SSL_CTX_set_session_cache_mode(
>>>> + ssl_ctx->ctx, SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL);
>>>> + SSL_CTX_sess_set_new_cb(ssl_ctx->ctx, new_session);
>>>
>>> What is the size of a SSL_SESSION in memory or on disk? Why would you
>>> not keep this in memory?
>>>

>> SSL_SESSION is about 2-8kb. It's up to client to choose where to store
>> it, but it should store in current process memory at least. And
>> probably on disk to reuse session by process.
>
> Ok, I assumed from reading the code that it had to be persisted to disk.
>
It's persisted to disk only in serf_get for testing.

>> Please note that before my change SSL_SESSION are never reused:
>> OpenSSL doesn't have internal mechanism for reusing client sessions
>> because it doesn't have information about hostname at BIO level [1]:
>> [[[
>> * SSL_SESS_CACHE_CLIENT
>> Client sessions are added to the session cache. As there is no
>> reliable way for the OpenSSL library to know whether a session should
>> be reused or which session to choose (due to the abstract BIO layer
>> the SSL engine does not have details about the connection), the
>> application must select the session to be reused by using the
>> ssl_set_session(3) function. This option is not activated by default.
>> ]]]
>>
>> We enable SSL_SESS_CACHE_CLIENT only to get notified that new session
>> is negotiated.
>
> Ok, so I see 4 situations:
> 1. SSL session reuse within one connection.
> I don't think this can be an issue with serf as we don't use multiple
> sessions over the same TCP connection.
>
Even for one serf connection we restart TCP connection after
keep-alive limit. So will benefit is this case also.

> 2. SSL session reuse on multiple connections to the same server within
> the same serf context.
> I assume the application reuses the same ssl context for connections
> to the same server in a serf context.
>
No, application creates separate serf_ssl_context_t for each
connection. Because serf_ssl_context_t holds *both* SSL and SSL_CTX.

> We should handle this reuse automatically for the application if
> possible. The per server cache I've added to serf_context_t for
> authn_info can be extended to also store the SSL session (ticket), if
> that's only 2-8KB we won't need to cache this to disk. We can provide
> an API to enable/disable this session caching, enabled by default.
>
Currently it's not possible because serf_ssl_context_t doesn't have
reference to serf_context_t.

> 3. SSL session reuse on multiple connections to the same server within
> multiple serf contexts in the same process.
> This is the Subversion case, which opens multiple serf contexts to
> open connections to the same server.
>
> I think we should ignore this case. Subversion should be adapted to
> use on only serf_context_t per server (your patch), and serf should be
> fixed where needed to support Subversion to maintain all its
> connections in one serf context (e.g. our work on the reset_connection
> issue).
> => If possible, the application should be adapted so that it fits in
> situation 2.
>
I disagree that we should ignore this case. Subversion diff editor
needs two ra_serf session at least and GUI application may use
multiple svn_ra_session_t in multi-threaded environment or something.

> 4. SSL session reuse on multiple connections to the same server,
> persisted over multiple processes.
> This also fits situation 3 where the application really needs to have
> multiple serf contexts per server, e.g. because it needs to use a
> different proxy server, or it's conceptually a different group of
> connections.
>
> This is what your patch implements right?
>
Persisting SSL sessions to disk is important for command line clients
like svn.exe. Currently we negotiate at least one new SSL session for
every svn process start. With TLS ticket extension SSL sessions could
be stored for long time (like weeks) because they don't require any
server side storage: it just ticket signed by server. It would be cool
if svn.exe will be able to use SSL session created by previous svn.exe
invocation. But we need improvements in OpenSSL to implement this [1].

I agree with you that moving SSL reuse logic to serf_context_t layer
would be nice. In this case we can also add optional callback to
request and store "external" SSL session from application.

Actually that was my original plan, before I found that
serf_ssl_context_t doesn't have reference to serf_context_t. That's
why I switched back to application controlled SSL session reuse.

[1] http://www.mail-archive.com/opens...@openssl.org/msg32724.html

[Ben Reser]

On Wed, Jul 3, 2013 at 4:26 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> Persisting SSL sessions to disk is important for command line clients
> like svn.exe. Currently we negotiate at least one new SSL session for
> every svn process start. With TLS ticket extension SSL sessions could
> be stored for long time (like weeks) because they don't require any
> server side storage: it just ticket signed by server. It would be cool
> if svn.exe will be able to use SSL session created by previous svn.exe
> invocation. But we need improvements in OpenSSL to implement this [1].
>
> I agree with you that moving SSL reuse logic to serf_context_t layer
> would be nice. In this case we can also add optional callback to
> request and store "external" SSL session from application.
>
> Actually that was my original plan, before I found that
> serf_ssl_context_t doesn't have reference to serf_context_t. That's
> why I switched back to application controlled SSL session reuse.
>
> [1] http://www.mail-archive.com/opens...@openssl.org/msg32724.html

Can you talk a bit about the security implications of this
functionality? Is it possible to store communications and then later
gain access to the store session giving plaintext to all the stored
communications that used the stored session? Your mail to openssl-dev
implies there's at least a certificate validation issue at current.

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 5:37 PM, Ben Reser <b...@reser.org> wrote:
> On Wed, Jul 3, 2013 at 4:26 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> Persisting SSL sessions to disk is important for command line clients
>> like svn.exe. Currently we negotiate at least one new SSL session for
>> every svn process start. With TLS ticket extension SSL sessions could
>> be stored for long time (like weeks) because they don't require any
>> server side storage: it just ticket signed by server. It would be cool
>> if svn.exe will be able to use SSL session created by previous svn.exe
>> invocation. But we need improvements in OpenSSL to implement this [1].
>>
>> I agree with you that moving SSL reuse logic to serf_context_t layer
>> would be nice. In this case we can also add optional callback to
>> request and store "external" SSL session from application.
>>
>> Actually that was my original plan, before I found that
>> serf_ssl_context_t doesn't have reference to serf_context_t. That's
>> why I switched back to application controlled SSL session reuse.
>>
>> [1] http://www.mail-archive.com/opens...@openssl.org/msg32724.html
>
> Can you talk a bit about the security implications of this
> functionality?

I assume you're asking about storing SSL session to disk.

> Is it possible to store communications and then later
> gain access to the store session giving plaintext to all the stored
> communications that used the stored session? Your mail to openssl-dev
> implies there's at least a certificate validation issue at current.
>

Good point. It seems it would be possible to decrypt stored
communication using later obtained SSL session. So most likely we
should not them on disk. It least without encrypting SSL session like
we do for stored passwords.

Btw all browsers store SSL sessions on disk. Firefox and IE have an
option to clear them explicitly. Google Chrome doesn't:
1. Google Chrome: http://code.google.com/p/chromium/issues/detail?id=90454
2. Firefox: https://bugzilla.mozilla.org/show_bug.cgi?id=285440
3. IE: Automatically clear on computer restart
http://support.microsoft.com/kb/290345

But I still think that it would be nice to store SSL session in
svn_client_t (in-memory) to reuse it between different
svn_ra_session_t instances.

[1] http://tools.ietf.org/html/rfc5077#section-5

[Lieven Govaerts]

On Wed, Jul 3, 2013 at 11:45 AM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Wed, Jul 3, 2013 at 1:22 PM, Lieven Govaerts
> <lieven....@gmail.com> wrote:

>> Hi Ivan,
>>
>> On Tue, Jul 2, 2013 at 5:28 PM, <se...@googlecode.com> wrote:
>>> Revision: 1982
>>> Author: chemodax
>>> Date: Tue Jul 2 08:28:23 2013
>>> Log: Add API for reusing SSL sessions.
>>
>> can you describe in detail what the goal is for this change? I find it
>> a lot of code with yet another difficult to use API for the
>> application.
>> I know we have talked about this at the svn hackathon, but I can't
>> remember the exact use case you want to improve.

> Short-term goal is to implement in-memory SSL session storage and
> reuse in Subversion.
>

>>> + ssl_ctx->new_session_cb = NULL;
>>> +
>>> + /* Enable SSL callback for new sessions and disable internal session
>>> + handling. */
>>> + SSL_CTX_set_session_cache_mode(
>>> + ssl_ctx->ctx, SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL);
>>> + SSL_CTX_sess_set_new_cb(ssl_ctx->ctx, new_session);
>>
>> What is the size of a SSL_SESSION in memory or on disk? Why would you
>> not keep this in memory?
>>

> SSL_SESSION is about 2-8kb. It's up to client to choose where to store
> it, but it should store in current process memory at least. And
> probably on disk to reuse session by process.

Ok, I assumed from reading the code that it had to be persisted to disk.

> Please note that before my change SSL_SESSION are never reused:
> OpenSSL doesn't have internal mechanism for reusing client sessions
> because it doesn't have information about hostname at BIO level [1]:
> [[[
> * SSL_SESS_CACHE_CLIENT
> Client sessions are added to the session cache. As there is no
> reliable way for the OpenSSL library to know whether a session should
> be reused or which session to choose (due to the abstract BIO layer
> the SSL engine does not have details about the connection), the
> application must select the session to be reused by using the
> ssl_set_session(3) function. This option is not activated by default.
> ]]]
>
> We enable SSL_SESS_CACHE_CLIENT only to get notified that new session
> is negotiated.

Ok, so I see 4 situations:
1. SSL session reuse within one connection.
I don't think this can be an issue with serf as we don't use multiple
sessions over the same TCP connection.

2. SSL session reuse on multiple connections to the same server within
the same serf context.
I assume the application reuses the same ssl context for connections
to the same server in a serf context.

We should handle this reuse automatically for the application if
possible. The per server cache I've added to serf_context_t for
authn_info can be extended to also store the SSL session (ticket), if
that's only 2-8KB we won't need to cache this to disk. We can provide
an API to enable/disable this session caching, enabled by default.

3. SSL session reuse on multiple connections to the same server within
multiple serf contexts in the same process.
This is the Subversion case, which opens multiple serf contexts to
open connections to the same server.

I think we should ignore this case. Subversion should be adapted to
use on only serf_context_t per server (your patch), and serf should be
fixed where needed to support Subversion to maintain all its
connections in one serf context (e.g. our work on the reset_connection
issue).
=> If possible, the application should be adapted so that it fits in
situation 2.

4. SSL session reuse on multiple connections to the same server,
persisted over multiple processes.
This also fits situation 3 where the application really needs to have
multiple serf contexts per server, e.g. because it needs to use a
different proxy server, or it's conceptually a different group of
connections.

This is what your patch implements right?

>> Is the expected performance improvement really that much to go all the
>> way of saving and retrieving files from disk?
>>

> SSL handshake takes two round trips and some computation. But resuming
> existing session is just one [2] So it's definitely worth to reuse SSL
> session if possible even read from disk. But short goal is store it
> only in memory. serf_get utility stores it on disk just for testing
> purpose.

>
>>>
>>> +/* Define preprocessor macro for easy detection of serf capability.*/
>>> +#define SERF_HAS_SSL_SESSION_API 1
>>> +
>>
>> I don't like this macro. Compile-time feature checking has until now
>> always been done with the SERF_VERSION_AT_LEAST macro, I propose we
>> don't start changing that now.
>>

> OK, I'll remove this macro.
>
> But problem that currently serf trunk is 2.0 and I have to test for
> SERF_VERSION_LEAST(2.0) when add code to use this to Subversion for
> example. May be we should change trunk to 1.4? Do we expect 2.0 in
> near future?

You can use SERF_VERSION_LEAST(2.0) for now and change it later to the
right version when the next version of serf releases.

Or we can switch the version of trunk. That depends a bit on our plans
for the 1.x line. Personally I'm only thinking about integration the
multiple-ssl branch and maybe adding CryptoAPI support (if someone is
willing to sponsor that work, not going to do that in my free time).
Then there's SPDY support, but that'll probably work best with the 2.0
API's.

Lieven

>
> [1] http://linux.die.net/man/3/ssl_ctx_set_session_cache_mode
> [2] http://vincent.bernat.im/en/blog/2011-ssl-session-reuse-rfc5077.html
>
> --
> Ivan Zhakov

[Ivan Zhakov]

On Wed, Jul 3, 2013 at 1:22 PM, Lieven Govaerts
<lieven....@gmail.com> wrote:

> Hi Ivan,
>
> On Tue, Jul 2, 2013 at 5:28 PM, <se...@googlecode.com> wrote:
>> Revision: 1982
>> Author: chemodax
>> Date: Tue Jul 2 08:28:23 2013
>> Log: Add API for reusing SSL sessions.
>
> can you describe in detail what the goal is for this change? I find it
> a lot of code with yet another difficult to use API for the
> application.
> I know we have talked about this at the svn hackathon, but I can't
> remember the exact use case you want to improve.

The idea is to reuse already negotiated SSL session for other
connections. This save one round trip to server and reduce CPU usage.
See [2]. For example:

Without session reuse:
> ptime serf_get https://svn.apache.org
1.3 seconds

First run with session reusing:
> ptime serf_get -s sess.bin https://svn.apache.org
1.3 seconds

Second run:
> ptime serf_get -s sess.bin https://svn.apache.org
0.8 seconds

Short-term goal is to implement in-memory SSL session storage and
reuse in Subversion.

>> + ssl_ctx->new_session_cb = NULL;
>> +
>> + /* Enable SSL callback for new sessions and disable internal session
>> + handling. */
>> + SSL_CTX_set_session_cache_mode(
>> + ssl_ctx->ctx, SSL_SESS_CACHE_CLIENT | SSL_SESS_CACHE_NO_INTERNAL);
>> + SSL_CTX_sess_set_new_cb(ssl_ctx->ctx, new_session);
>
> What is the size of a SSL_SESSION in memory or on disk? Why would you
> not keep this in memory?
>

SSL_SESSION is about 2-8kb. It's up to client to choose where to store
it, but it should store in current process memory at least. And
probably on disk to reuse session by process.

Please note that before my change SSL_SESSION are never reused:
OpenSSL doesn't have internal mechanism for reusing client sessions
because it doesn't have information about hostname at BIO level [1]:
[[[
* SSL_SESS_CACHE_CLIENT
Client sessions are added to the session cache. As there is no
reliable way for the OpenSSL library to know whether a session should
be reused or which session to choose (due to the abstract BIO layer
the SSL engine does not have details about the connection), the
application must select the session to be reused by using the
ssl_set_session(3) function. This option is not activated by default.
]]]

We enable SSL_SESS_CACHE_CLIENT only to get notified that new session
is negotiated.

> Is the expected performance improvement really that much to go all the
> way of saving and retrieving files from disk?
>

SSL handshake takes two round trips and some computation. But resuming
existing session is just one [2] So it's definitely worth to reuse SSL
session if possible even read from disk. But short goal is store it
only in memory. serf_get utility stores it on disk just for testing
purpose.

>>

>> +/* Define preprocessor macro for easy detection of serf capability.*/
>> +#define SERF_HAS_SSL_SESSION_API 1
>> +
>
> I don't like this macro. Compile-time feature checking has until now
> always been done with the SERF_VERSION_AT_LEAST macro, I propose we
> don't start changing that now.
>

OK, I'll remove this macro.

But problem that currently serf trunk is 2.0 and I have to test for
SERF_VERSION_LEAST(2.0) when add code to use this to Subversion for
example. May be we should change trunk to 1.4? Do we expect 2.0 in
near future?

[Lieven Govaerts]

Hi Ivan,


couple of notes on the change and your later replies.

On Wed, Jul 3, 2013 at 1:26 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:

[..]

> +apr_status_t serf_ssl_session_export(void **data_p,
> + apr_size_t *len_p,
> + const serf_ssl_session_t *session,
> + apr_pool_t *pool)
> +{

Why don't you use const char* or const unsigned char* for data_p? If
the application decides to write this data to disk it has to convert
this pointer to a byte array anyway.

[..]

> +apr_status_t serf_ssl_session_import(const serf_ssl_session_t **session_p,
> + void *data,
> + apr_size_t len,
> + apr_pool_t *pool)
> +{

On the multiple_ssl_impls branch I've started using a standard naming
for this type of functions:
serf_ssl_load_identity_from_file
serf_ssl_load_CA_cert_from_file
load_CA_cert_from_buffer

The idea being that the function name specifies the source of the data
to be imported. What'd you think of renaming this function to
serf_ssl_load_session_from_buffer ?

[..]

I've modified serf_get to make it open a new connection for each
message (the CONNECTION_CLOSE_HDR flag). This makes it easier to see
the effect of session caching for multiple connections within the same
context. When connecting to the US apache server and fetching 8 files,
I see an improvement from 8 to less than 5 seconds. Pretty nice.

Also, I did the same tests with the Mac OS X SSL engine, with a
typical result of 6.5 seconds. Now Secure Transport already caches the
session between connections to the same server automatically for up to
10 minutes [see 1], but doesn't provide the API's to export and import
this session.

[1] http://developer.apple.com/library/ios/#qa/qa1727/_index.html

That's my situation 2, with connection I mean here one TCP connection,
not a serf_connection_t.

>> 2. SSL session reuse on multiple connections to the same server within
>> the same serf context.
>> I assume the application reuses the same ssl context for connections
>> to the same server in a serf context.
>>
> No, application creates separate serf_ssl_context_t for each
> connection. Because serf_ssl_context_t holds *both* SSL and SSL_CTX.
>

I remember again, that's what we talked about at the hackathon. That's
indeed a problem.

>> We should handle this reuse automatically for the application if
>> possible. The per server cache I've added to serf_context_t for
>> authn_info can be extended to also store the SSL session (ticket), if
>> that's only 2-8KB we won't need to cache this to disk. We can provide
>> an API to enable/disable this session caching, enabled by default.
>>
> Currently it's not possible because serf_ssl_context_t doesn't have
> reference to serf_context_t.
>
>> 3. SSL session reuse on multiple connections to the same server within
>> multiple serf contexts in the same process.
>> This is the Subversion case, which opens multiple serf contexts to
>> open connections to the same server.
>>
>> I think we should ignore this case. Subversion should be adapted to
>> use on only serf_context_t per server (your patch), and serf should be
>> fixed where needed to support Subversion to maintain all its
>> connections in one serf context (e.g. our work on the reset_connection
>> issue).
>> => If possible, the application should be adapted so that it fits in
>> situation 2.
>>
> I disagree that we should ignore this case.

Ok, not ignore. But I think we should focus on:
a. caching sessions in the serf context automatically.
b. ensuring that applications can reuse the serf context as long as needed.

> Subversion diff editor
> needs two ra_serf session at least and GUI application may use
> multiple svn_ra_session_t in multi-threaded environment or something.

You are talking about a svn_ra_session_t, I'm talking about a
serf_context_t. There's a one-to-one mapping currently, but that
doesn't stay this way.

>> 4. SSL session reuse on multiple connections to the same server,
>> persisted over multiple processes.
>> This also fits situation 3 where the application really needs to have
>> multiple serf contexts per server, e.g. because it needs to use a
>> different proxy server, or it's conceptually a different group of
>> connections.
>>
>> This is what your patch implements right?
>>
> Persisting SSL sessions to disk is important for command line clients
> like svn.exe. Currently we negotiate at least one new SSL session for
> every svn process start. With TLS ticket extension SSL sessions could
> be stored for long time (like weeks) because they don't require any
> server side storage: it just ticket signed by server. It would be cool
> if svn.exe will be able to use SSL session created by previous svn.exe
> invocation. But we need improvements in OpenSSL to implement this [1].
>
> I agree with you that moving SSL reuse logic to serf_context_t layer
> would be nice. In this case we can also add optional callback to
> request and store "external" SSL session from application.
>
> Actually that was my original plan, before I found that
> serf_ssl_context_t doesn't have reference to serf_context_t. That's
> why I switched back to application controlled SSL session reuse.

Yep, I remember. This is the typical problem that the serf context
doesn't have access to the buckets used to implement the http(s)
protocol as they are created by the application. We have solved this
before with adding a bucket factory function which takes a serf
context or serf connection so that serf can link those two. (e.g.
serf_context_bucket_socket_create. Note: this all needs to go away in
serf 2.0 of course, but that shouldn't stop you now).

I'm thinking about:
void serf_connection_ssl_buckets_create(serf_connection_t *conn,
serf_bucket_t **encrypt_bkt, serf_bucket_t **decrypt_bkt,
serf_bucket_alloc_t *allocator);

The implementation then creates a encrypt and decrypt bucket, making
them automatically use the same serf_ssl_context_t, and then sets up
the callback for session reuse.

Lieven

[Ivan Zhakov]

On Sun, Jul 7, 2013 at 11:49 PM, Lieven Govaerts
<lieven....@gmail.com> wrote:
> Hi Ivan,
>
>

> couple of notes on the change and your later replies.
>
> On Wed, Jul 3, 2013 at 1:26 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>
> [..]
>
>> +apr_status_t serf_ssl_session_export(void **data_p,
>> + apr_size_t *len_p,
>> + const serf_ssl_session_t *session,
>> + apr_pool_t *pool)
>> +{
>
> Why don't you use const char* or const unsigned char* for data_p? If
> the application decides to write this data to disk it has to convert
> this pointer to a byte array anyway.
>

For the same reason why apr_file_read/write uses void *. For
application it just memory block, without any type. Otherwise we have
to choose use signed or unsigned type.

> [..]
>
>> +apr_status_t serf_ssl_session_import(const serf_ssl_session_t **session_p,
>> + void *data,
>> + apr_size_t len,
>> + apr_pool_t *pool)
>> +{
>
> On the multiple_ssl_impls branch I've started using a standard naming
> for this type of functions:
> serf_ssl_load_identity_from_file
> serf_ssl_load_CA_cert_from_file
> load_CA_cert_from_buffer
>
> The idea being that the function name specifies the source of the data
> to be imported. What'd you think of renaming this function to
> serf_ssl_load_session_from_buffer ?

serf_ssl_load_session_from_buffer is good name. I didn't use name
serf_ssl_load_session because existing functions used word 'load' for
read from file operation.

That what I was saying :)

> Also, I did the same tests with the Mac OS X SSL engine, with a
> typical result of 6.5 seconds. Now Secure Transport already caches the
> session between connections to the same server automatically for up to
> 10 minutes [see 1], but doesn't provide the API's to export and import
> this session.
>

Windows SSPI (SChannel) API also have internal session cache, without
option to export/import existing SSL session.

After learning OpenSSL API it seems to be not so important problem.
OpenSSL never reuse SSL session even used for same SSL_CTX, because
SSL object doesn't have host information.

>
>>> We should handle this reuse automatically for the application if
>>> possible. The per server cache I've added to serf_context_t for
>>> authn_info can be extended to also store the SSL session (ticket), if
>>> that's only 2-8KB we won't need to cache this to disk. We can provide
>>> an API to enable/disable this session caching, enabled by default.
>>>
>> Currently it's not possible because serf_ssl_context_t doesn't have
>> reference to serf_context_t.
>>
>>> 3. SSL session reuse on multiple connections to the same server within
>>> multiple serf contexts in the same process.
>>> This is the Subversion case, which opens multiple serf contexts to
>>> open connections to the same server.
>>>
>>> I think we should ignore this case. Subversion should be adapted to
>>> use on only serf_context_t per server (your patch), and serf should be
>>> fixed where needed to support Subversion to maintain all its
>>> connections in one serf context (e.g. our work on the reset_connection
>>> issue).
>>> => If possible, the application should be adapted so that it fits in
>>> situation 2.
>>>
>> I disagree that we should ignore this case.
>
> Ok, not ignore. But I think we should focus on:
> a. caching sessions in the serf context automatically.
> b. ensuring that applications can reuse the serf context as long as needed.
>

OK, I agree with your wording. I think we should reuse SSL sessions
privately in serf_context_t without application interaction given that
fact that most likely we cannot persist SSL session to disk for
security reasons [1]

Do you suggest to completely remove serf_ssl_context_t from newer API?
Or application will use serf_bucket_ssl_decrypt_context_get() to get
serf_ssl_context_t to configure callbacks?

[1] https://groups.google.com/d/msg/serf-dev/W6vNiUlMDDw/IgOufTGcEUAJ

--
Ivan Zhakov

[Lieven Govaerts]

On Mon, Jul 8, 2013 at 1:05 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
> On Sun, Jul 7, 2013 at 11:49 PM, Lieven Govaerts
> <lieven....@gmail.com> wrote:
>> Hi Ivan,
>>
>>
>> couple of notes on the change and your later replies.
>>

[..]

>>
>> I'm thinking about:
>> void serf_connection_ssl_buckets_create(serf_connection_t *conn,
>> serf_bucket_t **encrypt_bkt, serf_bucket_t **decrypt_bkt,
>> serf_bucket_alloc_t *allocator);
>>
>> The implementation then creates a encrypt and decrypt bucket, making
>> them automatically use the same serf_ssl_context_t, and then sets up
>> the callback for session reuse.
>>
> Do you suggest to completely remove serf_ssl_context_t from newer API?

I don't think we can, most of the ssl bucket API takes a
serf_ssl_context_t * parameter.

> Or application will use serf_bucket_ssl_decrypt_context_get() to get
> serf_ssl_context_t to configure callbacks?
>

Yes. Not ideal, but it should do for now.

>
> --
> Ivan Zhakov

Lieven

[Ivan Zhakov]

On Mon, Jul 8, 2013 at 3:56 PM, Lieven Govaerts

<lieven....@gmail.com> wrote:
> On Mon, Jul 8, 2013 at 1:05 PM, Ivan Zhakov <iv...@visualsvn.com> wrote:
>> On Sun, Jul 7, 2013 at 11:49 PM, Lieven Govaerts
>> <lieven....@gmail.com> wrote:
>>> Hi Ivan,
>>>
>>>
>>> couple of notes on the change and your later replies.
>>>
> [..]
>>>
>>> I'm thinking about:
>>> void serf_connection_ssl_buckets_create(serf_connection_t *conn,
>>> serf_bucket_t **encrypt_bkt, serf_bucket_t **decrypt_bkt,
>>> serf_bucket_alloc_t *allocator);
>>>
>>> The implementation then creates a encrypt and decrypt bucket, making
>>> them automatically use the same serf_ssl_context_t, and then sets up
>>> the callback for session reuse.
>>>
>> Do you suggest to completely remove serf_ssl_context_t from newer API?
>
> I don't think we can, most of the ssl bucket API takes a
> serf_ssl_context_t * parameter.
>
>> Or application will use serf_bucket_ssl_decrypt_context_get() to get
>> serf_ssl_context_t to configure callbacks?
>>
>
> Yes. Not ideal, but it should do for now.
>

May it's better to add serf_ssl_context *
serf_ssl_context_create(serf_connection_t *conn, serf_bucket_alloc_t
*allocator) and then use it for encrypt/decrypt buckets?

[Lieven Govaerts]

That will work too. But for the application that still means 3 calls to serf:
serf_ssl_context_create
serf_bucket_ssl_decrypt_create
serf_bucket_ssl_encrypt_create

whereas in my proposal these are grouped in one call:
serf_connection_ssl_buckets_create

well, two, because the app will need the context later:
serf_bucket_ssl_encrypt_context_get

This context can also be returned in the
serf_connection_ssl_buckets_create call, reducing it to one API call
again.

But you decide what you like best, you're writing the code. :)

Lieven

[Ivan Zhakov]

On Tue, Jul 9, 2013 at 6:43 PM, Lieven Govaerts

I've reverted r1982 and followups in r2130. I'm going to implement SSL
session reuse later without exposing them to library users. Thanks for
review!


--
Ivan Zhakov