[PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Kamesh Jayachandran wrote:
> Reasonable fix for this on the server side is to apply SSL_OP_NO_TICKET 
> patch and enable SSLSessionCache.

There is actually another reason why disabling TLS session tickets makes
sense at the present time: with OpenSSL's current stable version
(0.9.8k), session tickets only work properly for the first/default
vhost. For all other vhosts, mod_ssl will fail to decrypt a
previously-generated ticket, due to the order in which OpenSSL currently
deals with the SNI and ticket extensions (and their callbacks). The
consequence is that with 2.2.x and an SNI configuration, session caching
for clients supporting TLS tickets is not working for all but the first
vhost.

The attached patch (for trunk, plus a backport for 2.2.x) includes two
proposed changes:

1) When configuring a new SSL context (in
ssl_engine_init.c:ssl_init_ctx_tls_extensions), it disables session
ticket support if a server-side session cache is configured. Enabling
both session tickets and a cache for stateful resumption at the same
time doesn't make that much sense anyway, IMO. This change will also
solve the issue with OpenSSL clients (as reported by Kamesh), provided
that a server-side cache is configured.

2) In the SNI callback, it adjusts OpenSSL's session id context - which
makes sure that the session can be properly resumed. (With the current
mod_ssl code, this context is always tied to the first vhost, possibly
resulting in incorrect resumption behavior.)

The first change might later be revised, depending on how future OpenSSL
versions deal with the combination of SNI + session tickets (work is
underway in this area). But given the fact that OpenSSL versions between
0.9.8f and 0.9.8k will stay around for quite some time, I consider this
the appropriate fix for the time being (later on, it could be #if'd
based on OPENSSL_VERSION_NUMBER, or maybe even made configurable through
an additional directive).

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kamesh Jayachandran <ka...@collab.net>]

Thanks Kaspar.

I tested your patch against 2.2 branch on to my apache-2.2.12 and found 
it to be working.

My observations:
Without SSLSessionCache it failed as usual after a long time.

With SSLSessionCache it succeeded.

Looking forward for this patch to get committed.

Thanks to all involved in this bug fix/analysis.

With regards
Kamesh Jayachandran
On 11/04/2009 03:05 PM, Kaspar Brand wrote:
> Kamesh Jayachandran wrote:
>    
>> Reasonable fix for this on the server side is to apply SSL_OP_NO_TICKET
>> patch and enable SSLSessionCache.
>>      
> There is actually another reason why disabling TLS session tickets makes
> sense at the present time: with OpenSSL's current stable version
> (0.9.8k), session tickets only work properly for the first/default
> vhost. For all other vhosts, mod_ssl will fail to decrypt a
> previously-generated ticket, due to the order in which OpenSSL currently
> deals with the SNI and ticket extensions (and their callbacks). The
> consequence is that with 2.2.x and an SNI configuration, session caching
> for clients supporting TLS tickets is not working for all but the first
> vhost.
>
> The attached patch (for trunk, plus a backport for 2.2.x) includes two
> proposed changes:
>
> 1) When configuring a new SSL context (in
> ssl_engine_init.c:ssl_init_ctx_tls_extensions), it disables session
> ticket support if a server-side session cache is configured. Enabling
> both session tickets and a cache for stateful resumption at the same
> time doesn't make that much sense anyway, IMO. This change will also
> solve the issue with OpenSSL clients (as reported by Kamesh), provided
> that a server-side cache is configured.
>
> 2) In the SNI callback, it adjusts OpenSSL's session id context - which
> makes sure that the session can be properly resumed. (With the current
> mod_ssl code, this context is always tied to the first vhost, possibly
> resulting in incorrect resumption behavior.)
>
> The first change might later be revised, depending on how future OpenSSL
> versions deal with the combination of SNI + session tickets (work is
> underway in this area). But given the fact that OpenSSL versions between
> 0.9.8f and 0.9.8k will stay around for quite some time, I consider this
> the appropriate fix for the time being (later on, it could be #if'd
> based on OPENSSL_VERSION_NUMBER, or maybe even made configurable through
> an additional directive).
>
> Kaspar
>    

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Dr Stephen Henson <sh...@oss-institute.org>]

Kaspar Brand wrote:
> Kamesh Jayachandran wrote:
>> Reasonable fix for this on the server side is to apply SSL_OP_NO_TICKET 
>> patch and enable SSLSessionCache.
> 
> There is actually another reason why disabling TLS session tickets makes
> sense at the present time: with OpenSSL's current stable version
> (0.9.8k), session tickets only work properly for the first/default
> vhost. For all other vhosts, mod_ssl will fail to decrypt a
> previously-generated ticket, due to the order in which OpenSSL currently
> deals with the SNI and ticket extensions (and their callbacks). The
> consequence is that with 2.2.x and an SNI configuration, session caching
> for clients supporting TLS tickets is not working for all but the first
> vhost.
> 

The current OpenSSL (unreleased) stable code uses ticket keys from the initial
ctx and not the current one. This makes session resumption with tickets and SNI
work again because they all use the same keys.

The equivalent can be done with previous versions of OpenSSL by generating the
three ticket related keys and initializing the same ones in all SSL_CTX
structures. The function macro SSL_CTX_set_tlsext_ticket_keys can be used to do
this.

> 
> 1) When configuring a new SSL context (in
> ssl_engine_init.c:ssl_init_ctx_tls_extensions), it disables session
> ticket support if a server-side session cache is configured. Enabling
> both session tickets and a cache for stateful resumption at the same
> time doesn't make that much sense anyway, IMO. This change will also
> solve the issue with OpenSSL clients (as reported by Kamesh), provided
> that a server-side cache is configured.
> 

I suppose if some clients support tickets and others do not then enabling both
makes sense. You'd get improved performance for the equivalent cache size
because ticket supporting clients would do their own caching and non-ticket
clients would use normal stateful session resumption. Though as you note older
versions of OpenSSL will be in use for quite a while after 0.9.8l is released.

Note this should all be fixed in current unreleased OpenSSL (which will be
0.9.8l) but it needs client side as well as server side changes.

Steve.
-- 
Dr Stephen N. Henson. Senior Technical/Cryptography Advisor,
Open Source Software Institute: www.oss-institute.org
OpenSSL Core team: www.openssl.org

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Joe Orton wrote:
> 1) why do we need a new config directive for session ticket support?  
> I'm struggling to understand why any server admin would need/want 
> control over support for session tickets.

Session tickets are a relatively new thing (RFC 4507 is from May 2006),
and I'm not sure how well current TLS implementations interoperate with
each other. For situations like these, it seems desirable to me if the
admin can control this through a directive (it's not fundamentally
different form knobs like SSLHonorCipherOrder, SSLOptions
OptRenegotiate, SSLRenegBufferSize, and SSLStrictSNIVHostCheck, IMO).

> 2) I do not think we should be hacking around OpenSSL bugs as the code 
> for SSL_CTX_set_tlsext_ticket_keys() is.  People using buggy versions of 
> OpenSSL should upgrade to versions which are not buggy, if they are 
> affected.

OpenSSL versions 0.9.8f through 0.9.8l are probably going to stay around
for quite some time, so why not do these users a favor with a few
(conditionally compiled) lines? It could just be considered a special
case of OpenSSL "feature checking", like #ifdefs for
SSL_OP_CIPHER_SERVER_PREFERENCE or HAVE_SSL_X509V3_EXT_d2i (which are
quite common in mod_ssl).

> At most here I'd do as your original patch does, to set the 
> SSL_OP_NO_TICKET flag if no session cache is enabled.  This is 
> sufficient to fix the issue in question, right?

My original patch disabled tickets only if a session cache *is*
configured (nSessionCacheMode != SSL_SCMODE_NONE), because in that case,
session tickets really make sense - there's no stateful cache, so leave
support for stateless resumption enabled.

Turning off ticket support when no cache is configured seems like a bad
idea to me - it will fix the issue for clients based on OpenSSL <=
0.9.8l, true, but at the cost of making it impossible to run mod_ssl
with stateless resumption only (which I think is a perfectly legitimate
use case).

> 3) The MD5 hash used in the session id context is just a hash.  It has 
> no cryptographic use AFAIK and MD5 is a perfectly fine hash, so I don't 
> see any need to change that.

Correct, the question is how likely a collision of two vhost_ids
(servername:port) is with MD5. Changing to SHA-1 for trunk shouldn't
hurt, however - and might obviate future questions as to why mod_ssl
(still) uses MD5 internally.

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Joe Orton <jo...@redhat.com>]

On Mon, Nov 09, 2009 at 07:06:16PM +0100, Kaspar Brand wrote:
> Dr Stephen Henson wrote:
> > Yes that looks better. There is an alternative technique if it is easier to find
> > a "base" SSL_CTX, you can retrieve the auto generated keys using
> > SSL_CTX_get_tlsext_ticket_keys() and then copy to the new context as above.
> 
> The loop actually iterates over all contexts, so we could just remember
> the keys of the first SSL-enabled vhost, we don't have to find the
> "base" context. I.e., simply replace
> 
>   RAND_bytes(tlsext_tick_keys, tick_keys_len);
> 
> by
> 
>   SSL_CTX_get_tlsext_ticket_keys(sc->server->ssl_ctx,
>                                  tlsext_tick_keys, tick_keys_len);
> 
> I prefer the former, however, because 1) it's shorter, 2) RAND_bytes are
> cheap (aren't they? ;-) and 3) ... it would actually need another
> workaround, for OpenSSL < 0.9.8l, as I realized in the meantime: I
> should have compiled against 0.9.8k for my tests, not 0_9_8-stable -
> because this way I missed the TLXEXT_TICKET_KEYS typo :-/ In the
> attached patches (v4), I've therefore added a workaround for
> SSL_CTRL_SET_TLXEXT_TICKET_KEYS.
> 
> And back to the question whether ap_md5_binary should be used or not, I
> have now switched to apr_sha1 for the trunk version - maybe that's an
> acceptable compromise (use SHA-1 for trunk, stay with MD5 in 2.2.x, for
> backward compatibility)?

Thanks for working on this - sorry that I didn't have time to look into 
this last week.  A few questions:

1) why do we need a new config directive for session ticket support?  
I'm struggling to understand why any server admin would need/want 
control over support for session tickets.

2) I do not think we should be hacking around OpenSSL bugs as the code 
for SSL_CTX_set_tlsext_ticket_keys() is.  People using buggy versions of 
OpenSSL should upgrade to versions which are not buggy, if they are 
affected.  At most here I'd do as your original patch does, to set the 
SSL_OP_NO_TICKET flag if no session cache is enabled.  This is 
sufficient to fix the issue in question, right?

3) The MD5 hash used in the session id context is just a hash.  It has 
no cryptographic use AFAIK and MD5 is a perfectly fine hash, so I don't 
see any need to change that.

I'll commit the session-id-ctx change, that looks good.

Regards, Joe

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Dr Stephen Henson wrote:
> Yes that looks better. There is an alternative technique if it is easier to find
> a "base" SSL_CTX, you can retrieve the auto generated keys using
> SSL_CTX_get_tlsext_ticket_keys() and then copy to the new context as above.

The loop actually iterates over all contexts, so we could just remember
the keys of the first SSL-enabled vhost, we don't have to find the
"base" context. I.e., simply replace

  RAND_bytes(tlsext_tick_keys, tick_keys_len);

by

  SSL_CTX_get_tlsext_ticket_keys(sc->server->ssl_ctx,
                                 tlsext_tick_keys, tick_keys_len);

I prefer the former, however, because 1) it's shorter, 2) RAND_bytes are
cheap (aren't they? ;-) and 3) ... it would actually need another
workaround, for OpenSSL < 0.9.8l, as I realized in the meantime: I
should have compiled against 0.9.8k for my tests, not 0_9_8-stable -
because this way I missed the TLXEXT_TICKET_KEYS typo :-/ In the
attached patches (v4), I've therefore added a workaround for
SSL_CTRL_SET_TLXEXT_TICKET_KEYS.

And back to the question whether ap_md5_binary should be used or not, I
have now switched to apr_sha1 for the trunk version - maybe that's an
acceptable compromise (use SHA-1 for trunk, stay with MD5 in 2.2.x, for
backward compatibility)?

Could one of the httpd committers take over and make a decision,
therefore...? Help with getting this into the tree would be much
appreciated - thanks!

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Dr Stephen Henson <sh...@oss-institute.org>]

Kaspar Brand wrote:
> Dr Stephen Henson wrote:
>> A few comments about that:
> 
> Thanks for the review!
> 
>> These are cryptographic keys (or at least the HMAC and AES keys are) so you
>> should use RAND_bytes(), not RAND_pseudo_bytes().
> 
> Ok - when looking at ssl_lib.c:SSL_CTX_new(), I didn't realize that
> RAND_pseudo_bytes() is only used for tlsext_tick_key_name. Changed
> accordingly.
> 
>> Don't dereference the structures directly as at some point the sizes might
>> change, the structure made opaque or a different mechanism used for storing keys
>> (e.g. HSM support).
> 
> I was looking at a way to determine the size at compile time, but if you
> think that's an unsafe method (note: it's only expected to work for
> 0.9.8f through 0.9.8l), then let's change it.
> 

These things have a habit of persisting far longer than their expected lifetime ;-)

>> The approved way is to call:
>>
>> SSL_CTX_set_tlsext_ticket_keys(sc->server->ssl_ctx, NULL, -1)
>>
>> which will return the combined length of all keys.
> 
> Did that - does v3 of the patch (attached) look better? Is it ok to use
> apr_palloc here?
> 

Yes that looks better. There is an alternative technique if it is easier to find
a "base" SSL_CTX, you can retrieve the auto generated keys using
SSL_CTX_get_tlsext_ticket_keys() and then copy to the new context as above.

Steve.
-- 
Dr Stephen N. Henson. Senior Technical/Cryptography Advisor,
Open Source Software Institute: www.oss-institute.org
OpenSSL Core team: www.openssl.org

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Dr Stephen Henson wrote:
> A few comments about that:

Thanks for the review!

> These are cryptographic keys (or at least the HMAC and AES keys are) so you
> should use RAND_bytes(), not RAND_pseudo_bytes().

Ok - when looking at ssl_lib.c:SSL_CTX_new(), I didn't realize that
RAND_pseudo_bytes() is only used for tlsext_tick_key_name. Changed
accordingly.

> Don't dereference the structures directly as at some point the sizes might
> change, the structure made opaque or a different mechanism used for storing keys
> (e.g. HSM support).

I was looking at a way to determine the size at compile time, but if you
think that's an unsafe method (note: it's only expected to work for
0.9.8f through 0.9.8l), then let's change it.

> The approved way is to call:
> 
> SSL_CTX_set_tlsext_ticket_keys(sc->server->ssl_ctx, NULL, -1)
> 
> which will return the combined length of all keys.

Did that - does v3 of the patch (attached) look better? Is it ok to use
apr_palloc here?

> Finally:
> 
> +            sid_ctx = ap_md5_binary(c->pool, (unsigned char*)sc->vhost_id,
> +                                    sc->vhost_id_len);
> 
> should we be using MD5 now if it can be avoided?

That's what is currently used in mod_ssl.c:ssl_init_ssl_connection() -
these two should be kept in sync. I'm not opposed to a change (at both
places), but the question is whether now is the right time for doing
that. For distcache setups e.g. I would assume that it has some unwanted
consequences if two 2.2.x releases calculate the session id context in a
different way... but I'm interested to hear others' opinions on this.

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Dr Stephen Henson <sh...@oss-institute.org>]

Kaspar Brand wrote:


+#if !defined(OPENSSL_NO_TLSEXT) && OPENSSL_VERSION_NUMBER < 0x009080d0
+#define TICK_KEYS_LEN   sizeof(((SSL_CTX *)0)->tlsext_tick_key_name) \
+                      + sizeof(((SSL_CTX *)0)->tlsext_tick_hmac_key) \
+                      + sizeof(((SSL_CTX *)0)->tlsext_tick_aes_key)
+    unsigned char tlsext_tick_keys[TICK_KEYS_LEN];
+    RAND_pseudo_bytes(tlsext_tick_keys, TICK_KEYS_LEN);
+#endif
+

A few comments about that:

These are cryptographic keys (or at least the HMAC and AES keys are) so you
should use RAND_bytes(), not RAND_pseudo_bytes().

Don't dereference the structures directly as at some point the sizes might
change, the structure made opaque or a different mechanism used for storing keys
(e.g. HSM support).

The approved way is to call:

SSL_CTX_set_tlsext_ticket_keys(sc->server->ssl_ctx, NULL, -1)

which will return the combined length of all keys.

Finally:

+            sid_ctx = ap_md5_binary(c->pool, (unsigned char*)sc->vhost_id,
+                                    sc->vhost_id_len);

should we be using MD5 now if it can be avoided?

Steve.
-- 
Dr Stephen N. Henson. Senior Technical/Cryptography Advisor,
Open Source Software Institute: www.oss-institute.org
OpenSSL Core team: www.openssl.org

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Kaspar Brand wrote:
> Does that sound reasonable? If so, I would prepare a new patch with
> SSL_CTX_set_tlsext_ticket_keys and the new config directive.

No reactions = no objections?

Would it perhaps be possible to piggyback onto Joe's reneg patch and get
this also into 2.2.15...? ;-)

Attached are updated patches for trunk and 2.2.x which:

- fix TLS session tickets for SNI configurations when OpenSSL versions
between 0.9.8f and 0.9.8l are used

- add a new (global) "SSLSessionTicketExtension" configuration directive
which allows controlling SSL_OP_NO_TICKET (defaulting to "on", i.e.
tickets are left enabled, but can be turned off if necessary)

- include the fix for the SNI callback which makes sure that the correct
session id context is set (to prevent improper session resumption).

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Ruediger Pluem wrote:
> I would like to see your comment on Steves comment regarding the usage of
> SSL_CTX_set_tlsext_ticket_keys.

That workaround does the trick, indeed - I played with it in the
meantime. Coding this in ssl_engine_init.c looks a bit awkward, but we
can limit the fix to OPENSSL_VERSION_NUMBER < 0x009080c0.

If we go for this option, however, then I propose that we also add an
SSLSessionTicketExtension directive, which defaults to "on" but allows
to turn off ticket support if desired (through SSL_OP_NO_TICKET).

Does that sound reasonable? If so, I would prepare a new patch with
SSL_CTX_set_tlsext_ticket_keys and the new config directive.

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Ruediger Pluem <rp...@apache.org>]

On 11/04/2009 05:59 PM, Kaspar Brand wrote:
> Ruediger Pluem wrote:

>>> 2) In the SNI callback, it adjusts OpenSSL's session id context - which
>>> makes sure that the session can be properly resumed. (With the current
>>> mod_ssl code, this context is always tied to the first vhost, possibly
>>> resulting in incorrect resumption behavior.)
>> Can you please elaborate in more detail why this shouldn't be done when
>> we have done renegotiations so far?
> 
> When ssl_hook_Access triggers a renegotation, it sets the session id
> context to a request-specific id, before calling SSL_renegotiate (to
> limit session reuse to this specific request). If we would overwrite the
> context during that renegotation (when an SNI extension is encountered
> and therefore the callback executed), then this coupling would get lost.

Thanks for explaining. Makes sense.

I would like to see your comment on Steves comment regarding the usage of
SSL_CTX_set_tlsext_ticket_keys.

Regards

Rüdiger

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Kaspar Brand <ht...@velox.ch>]

Ruediger Pluem wrote:
> I guess your current patch fails on trunk since myModConfig(s))->nSessionCacheMode
> is no longer present in trunk

Oops, you're right - my bad. I didn't compile trunk with that last
change applied, obviously. For trunk, it should be

    if ((myModConfig(s))->sesscache_mode != SSL_SESS_CACHE_OFF) {

instead.

>> 2) In the SNI callback, it adjusts OpenSSL's session id context - which
>> makes sure that the session can be properly resumed. (With the current
>> mod_ssl code, this context is always tied to the first vhost, possibly
>> resulting in incorrect resumption behavior.)
> 
> Can you please elaborate in more detail why this shouldn't be done when
> we have done renegotiations so far?

When ssl_hook_Access triggers a renegotation, it sets the session id
context to a request-specific id, before calling SSL_renegotiate (to
limit session reuse to this specific request). If we would overwrite the
context during that renegotation (when an SNI extension is encountered
and therefore the callback executed), then this coupling would get lost.

Kaspar

Re: [PATCH] mod_ssl: improving session caching for SNI configurations

[Ruediger Pluem <rp...@apache.org>]

On 11/04/2009 10:35 AM, Kaspar Brand wrote:
> Kamesh Jayachandran wrote:
>> Reasonable fix for this on the server side is to apply SSL_OP_NO_TICKET 
>> patch and enable SSLSessionCache.
> 
> There is actually another reason why disabling TLS session tickets makes
> sense at the present time: with OpenSSL's current stable version
> (0.9.8k), session tickets only work properly for the first/default
> vhost. For all other vhosts, mod_ssl will fail to decrypt a
> previously-generated ticket, due to the order in which OpenSSL currently
> deals with the SNI and ticket extensions (and their callbacks). The
> consequence is that with 2.2.x and an SNI configuration, session caching
> for clients supporting TLS tickets is not working for all but the first
> vhost.
> 
> The attached patch (for trunk, plus a backport for 2.2.x) includes two
> proposed changes:
> 
> 1) When configuring a new SSL context (in
> ssl_engine_init.c:ssl_init_ctx_tls_extensions), it disables session
> ticket support if a server-side session cache is configured. Enabling
> both session tickets and a cache for stateful resumption at the same
> time doesn't make that much sense anyway, IMO. This change will also
> solve the issue with OpenSSL clients (as reported by Kamesh), provided
> that a server-side cache is configured.

I guess your current patch fails on trunk since myModConfig(s))->nSessionCacheMode
is no longer present in trunk

> 
> 2) In the SNI callback, it adjusts OpenSSL's session id context - which
> makes sure that the session can be properly resumed. (With the current
> mod_ssl code, this context is always tied to the first vhost, possibly
> resulting in incorrect resumption behavior.)

Can you please elaborate in more detail why this shouldn't be done when
we have done renegotiations so far?

Regards

Rüdiger