net: Implement new SSL session cache for OpenSSL sockets.

net: Implement new SSL session cache for OpenSSL sockets.

This patch reworks the SSL session caching scheme for OpenSSL
client sockets, as an attempt to get rid of the mysterious crashes
described at http://crbug.com/298606

- Move the internal SSLSessionCache class to its own source file,
  while renaming it as SSLSessionCacheOpenSSL.

- Change the session caching logic to:

  - Completely disable OpenSSL's builtin cache.
  - Implement Session ID generation (and uniqueness check).
  - Implement eviction and expiration detection.

- Add a unit test for SSLSessionCacheOpenSSL.

- Modify SSLClientSocketOpenSSL implementation to use the new cache.

BUG=298606
R=rsleevi@chromium.org,agl@chromium.org,wtc@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=238398

[agl, 2013-11-27 17:29:47]

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:33: static
base::LazyInstance<SSLContextExIndex>::Leaky s_instance =
Other instances of LazyInstance in the codebase are at the toplevel. I worry
that this is racy since we disable thread-safe function statics in Chromium.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:69: // as sstd::hash<std::string>
without the extra string copy. See
typo: sstd

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:70: // base/containers/hash_tables.h.
We don't depend on this matching the std::string hash though, right?

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:135: // context |ctx|, after this
OpenSSL will call back during SSL connection
I think ", after this" might read better as ". After this"

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:172: // Retrieve the cache key from
|ssl| and lookup for a corresponding
s/lookup/look/

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:294: RemoveSessionLocked(session);
Is it acceptable to continue using |it| here when |RemoveSessionLocked| will
have erased it?

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:331: // |*id_len| is, o}n input, the
size of the desired ID. It will be 16 for
typo: o}n

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:332: // OpenSSLv2, and 32 for anything
else. OpenSSL allows an implementation
OpenSSLv2? Do you mean SSLv2?

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:338: static int
GenerateSessionIdStatic(const SSL* ssl,
This is only used by servers, no?

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:361: it = key_index_.find(cache_key);
isn't the iterator returned from insert()?

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:397: SSL_SESSION* session = *it++;
why postincrement |it| here?

[digit1, 2013-11-27 18:12:45]

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:33: static
base::LazyInstance<SSLContextExIndex>::Leaky s_instance =
On 2013/11/27 17:29:47, agl wrote:
> Other instances of LazyInstance in the codebase are at the toplevel. I worry
> that this is racy since we disable thread-safe function statics in Chromium. 

That's what the comment on line 32 addresses, i.e. there is no compiler-induced
lazy initialization of s_instance here. I've moved it to the top-level though.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:69: // as sstd::hash<std::string>
without the extra string copy. See
Oh snap, thanks, done.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:70: // base/containers/hash_tables.h.
Absolutely not, I just wanted something relatively simple and without surprises.
An alternative would be to use the same algorithm as OpenSSL, which uses only
the first 3 32-bit words of the ID buffer. Let me know if you prefer something
else.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:135: // context |ctx|, after this
OpenSSL will call back during SSL connection
On 2013/11/27 17:29:47, agl wrote:
> I think ", after this" might read better as ". After this"

Done.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:172: // Retrieve the cache key from
|ssl| and lookup for a corresponding
On 2013/11/27 17:29:47, agl wrote:
> s/lookup/look/

Done.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:294: RemoveSessionLocked(session);
it was incremented on line 286 so doesn't point on the removed item anymore.
According to http://www.cplusplus.com/reference/list/list/erase/

  Iterators, pointers and references referring to elements removed by the
  function are invalidated. All other iterators, pointers and references keep
  their validity.

Which is typically true for ordered containers.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:331: // |*id_len| is, o}n input, the
size of the desired ID. It will be 16 for
On 2013/11/27 17:29:47, agl wrote:
> typo: o}n

Done.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:332: // OpenSSLv2, and 32 for anything
else. OpenSSL allows an implementation
Oh yes, of course. Thanks. Fixed :-)

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:338: static int
GenerateSessionIdStatic(const SSL* ssl,
It's used by both clients and servers. It's also how clients generate a new
session ID, e.g. see ssl3_client_hello().

You may be thinking about ssl_get_prev_session() which is only used by servers.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:361: it = key_index_.find(cache_key);
I'm not sure this works with base::hash_map unfortunately, I'll try to check it
though. Unfortunately the implementation may vary between STLport (Android) and
libstdc++ (linux_redux).

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:397: SSL_SESSION* session = *it++;
Ah, it's not actually necessary anymore (came from older version of the code).
I've removed it.

[digit1, 2013-11-27 18:26:29]

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:361: it = key_index_.find(cache_key);
Actually, I've checked that using the version that returns a std::pair<iterator,
bool> works well in both cases, so it's been uploaded in the latest patch.
Thanks.

[agl, 2013-11-27 21:58:33]

LGTM

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:70: // base/containers/hash_tables.h.
On 2013/11/27 18:12:45, digit1 wrote:
> Absolutely not, I just wanted something relatively simple and without
surprises.
> An alternative would be to use the same algorithm as OpenSSL, which uses only
> the first 3 32-bit words of the ID buffer. Let me know if you prefer something
> else.

I think this is fine, but could you make the comment say "This happens to
compute the same value as"... Just to be clear that it doesn't depend on that
fact.

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:338: static int
GenerateSessionIdStatic(const SSL* ssl,
On 2013/11/27 18:12:45, digit1 wrote:
> It's used by both clients and servers. It's also how clients generate a new
> session ID, e.g. see ssl3_client_hello().

Ah, of course. Clients generate random, nonsense SessionIDs when using
SessionTickets.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.cc:37: // static
You're in an anonymous namespace, so // static isn't often added. Below, you
have static int GetSSLContextExIndex. Just highlighting it, pick whatever you
like :)

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:35: // Helper class used to
associate liberal std::string keys with SSL objects.
liberal?

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:50: static
base::LazyInstance<SSLKeyHelper>::Leaky s_instance =
I'll take your word that this compiles into nothing.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:73: // Called when a SSL object
is copied through SSL_dup(). This needs to copy
s/a/an/ I think.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:91: // Called to destroy the
value associated with a SSL object.
s/a/an/

[digit1, 2013-11-28 10:37:19]

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
File net/socket/ssl_session_cache_openssl.cc (right):

https://codereview.chromium.org/89623002/diff/80001/net/socket/ssl_session_ca...
net/socket/ssl_session_cache_openssl.cc:70: // base/containers/hash_tables.h.
I've done that, and added a blurb about the fact that other hashing schemes
should be fine too.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:35: // Helper class used to
associate liberal std::string keys with SSL objects.
I've changed it to 'arbitrary' which is the meaning I intended. Pardon my
French, English isn't my mother tongue :-)

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:50: static
base::LazyInstance<SSLKeyHelper>::Leaky s_instance =
I confirm it does (I disassembled the result to manually check too).

Note that even if it did not, this is a unit test class with a single thread, so
I don't think there would be an issue anyway.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:73: // Called when a SSL object
is copied through SSL_dup(). This needs to copy
On 2013/11/27 21:58:33, agl wrote:
> s/a/an/ I think.

Done.

https://codereview.chromium.org/89623002/diff/110001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl_unittest.cc:91: // Called to destroy the
value associated with a SSL object.
On 2013/11/27 21:58:33, agl wrote:
> s/a/an/

Done.

[digit1, 2013-12-02 14:00:53]

Ryan and Wan-Teh, any opinion on this CL? Thanks.

[digit1, 2013-12-03 11:02:07]

It's been 5 days, so I'm submitting this now. If you have a reason against it,
feel free to say so or revert the patch afterwards. Thanks.

[commit-bot: I haz the power, 2013-12-03 11:21:29]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/digit@chromium.org/89623002/130001

[commit-bot: I haz the power, 2013-12-03 15:13:34]

Change committed as 238398

[Ryan Sleevi, 2013-12-03 21:20:12]

Delayed LGTM, but two (minor) nits below.

https://codereview.chromium.org/89623002/diff/130001/net/socket/ssl_session_c...
File net/socket/ssl_session_cache_openssl.h (right):

https://codereview.chromium.org/89623002/diff/130001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.h:48: // SSL connections being performed in
parallel in multiple threads.
I'm a little concerned that this comment is needed. If there is this happening,
it's a bug. SSL should only ever happen on the IO thread.

https://codereview.chromium.org/89623002/diff/130001/net/socket/ssl_session_c...
net/socket/ssl_session_cache_openssl.h:123: static const size_t
kMaxExpirationChecks = 256;
This is not valid syntax; it's a GCC extension that allows this.

You should be declaring the defaults in the .cc.