Rewrite session cache in OpenSSL ports.

Rewrite session cache in OpenSSL ports.

The old session cache assumed session IDs were unique and called into BoringSSL
internals in the tests. Instead separate it into:

- The session cache proper which just implements the keyed map of SSL_SESSIONs
  with expiration checks and size limits. This can be unit tested without
  using OpenSSL internals.

- SSLClientSocketOpenSSL which hooks into SSL_CTX and SSL as appropriate and
  looks up or caches sessions.

Add additional tests in SSLClientSocket tests to ensure test coverage for the
latter.

Note: this removes the session removal logic via SSL_CTX_set_sess_remove_cb.
It was never called anyway (see https://crbug.com/466352). With that removed,
the SSL_SESSION* pointer-keyed map is unnecessary and the cache can just be
a base::MRUCache (which is what the original was based on anyway).

BUG=454044
Committed: https://crrev.com/dafe4e53058ed802fabc151e67e75ffded76fd18
Cr-Commit-Position: refs/heads/master@{#324292}

[davidben, 2015-03-12 00:03:45]

davidben@chromium.org changed reviewers:
+ rsleevi@chromium.org

[davidben, 2015-03-12 00:03:45]

I actually wrote an implementation and tests from scratch before I realized we
had a base::MRUCache. Good news is the tests pass with both versions.

[Ryan Sleevi, 2015-03-17 00:50:34]

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:2021: if (!SSL_session_reused(ssl_)) {
Why is this check needed? Isn't it a nop if it exists? Doesn't this prevent
refreshing the MRU of the session?

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:302: bool certificate_verified_;
First blush review: This strikes me as a bad thing. "marked_session_as_good_"
was meant to also consider the Finished message and not mark a session as good
that has a bad finished message.

(After reviewing .cc): Hrm. I see that was a mistaken assumption on my part. I
suppose this rename is fine.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:15: }
}  // namespace

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.h (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:17: class NET_EXPORT
SSLClientSessionCacheOpenSSL : public base::NonThreadSafe {
grumblenit: I dislike inheritance from traits classes (such as
base::NonThreadSafe), prefering instead to encapsulate traits via composition
(base::ThreadChecker)

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:20: Config();
http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization
recommends in-class member initialization, which is blessed by
https://chromium-cpp.appspot.com/

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:31: ~SSLClientSessionCacheOpenSSL();
If you have virtual methods, you should have a virtual dtor.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:33: size_t size() { return
cache_.size(); }
1) const
2) Don't inline this; you have no idea what complexity goes on with respect to
instantiating cache_ or calling .size()

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:38: SSL_SESSION* Lookup(const
std::string& cache_key);
const? non-const?

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:50: virtual base::Time Now();
Rather than inheritance for testing, why not use a base::Clock / base::TickClock
for this, passed in construction?

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:59: base::Time expiration;
base::TimeTicks, I would presume; otherwise, if the user adjusts their clock,
everything gets ruined here.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:61: using CacheEntryMap =
newline

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:182: for (size_t i = 0; i <
config.expiration_check_count - 1; i++) {
superflous braces

[davidben, 2015-03-20 22:15:28]

Patchset #7 (id:120001) has been deleted

[davidben, 2015-03-20 22:41:27]

Apologies for mixing a rebase into this. But the rebase involved reworking a lot
of stuff across the SSLConnectJob removal, so you should review those changes
too.

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:2021: if (!SSL_session_reused(ssl_)) {
On 2015/03/17 00:50:33, Ryan Sleevi wrote:
> Why is this check needed? Isn't it a nop if it exists? Doesn't this prevent
> refreshing the MRU of the session?

I guess it's not strictly needed? MRU is updated on lookup, and this is
consistent with the old code. (ssl_update_cache only calls new_session_cb if
!s->hit.)

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/994263002/diff/100001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:302: bool certificate_verified_;
On 2015/03/17 00:50:33, Ryan Sleevi wrote:
> First blush review: This strikes me as a bad thing. "marked_session_as_good_"
> was meant to also consider the Finished message and not mark a session as good
> that has a bad finished message.
> 
> (After reviewing .cc): Hrm. I see that was a mistaken assumption on my part. I
> suppose this rename is fine.

Acknowledged.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:15: }
On 2015/03/17 00:50:33, Ryan Sleevi wrote:
> }  // namespace

N/A with the in-line initialization.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.h (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:17: class NET_EXPORT
SSLClientSessionCacheOpenSSL : public base::NonThreadSafe {
On 2015/03/17 00:50:33, Ryan Sleevi wrote:
> grumblenit: I dislike inheritance from traits classes (such as
> base::NonThreadSafe), prefering instead to encapsulate traits via composition
> (base::ThreadChecker)

Done.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:20: Config();
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Initialization
> recommends in-class member initialization, which is blessed by
> https://chromium-cpp.appspot.com/

Done, although I'm unclear... the syntax I went with doesn't add a static
initializer, does it? I was kinda surprised it even compiled. You're apparently
allowed to even implicitly reference |this| in these things, so I'm guessing
it's fine?

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:31: ~SSLClientSessionCacheOpenSSL();
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> If you have virtual methods, you should have a virtual dtor.

Done.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:33: size_t size() { return
cache_.size(); }
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> 1) const
> 2) Don't inline this; you have no idea what complexity goes on with respect to
> instantiating cache_ or calling .size()

Done.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:38: SSL_SESSION* Lookup(const
std::string& cache_key);
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> const? non-const?

Non-const. It updates the MRU. base::MRUCache is the same. I've added a comment
about it.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:50: virtual base::Time Now();
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> Rather than inheritance for testing, why not use a base::Clock /
base::TickClock
> for this, passed in construction?

Done.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:59: base::Time expiration;
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> base::TimeTicks, I would presume; otherwise, if the user adjusts their clock,
> everything gets ruined here.

I used that initially, but I think it actually doesn't want to be TimeTicks. If
you suspend the machine for an hour and bring it back, will TimeTicks fast
forward for an hour? I don't think it does, but you still want to avoid reusing
an expired session. IOW, the expiration time is about how much longer, in wall
clock time, will I continue using this key.

(The old code used the wall clock time. Actually it relied on OpenSSL to stuff
the time in with time(), so it wasn't even mockable.)

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:61: using CacheEntryMap =
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> newline

Done.

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/100001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:182: for (size_t i = 0; i <
config.expiration_check_count - 1; i++) {
On 2015/03/17 00:50:34, Ryan Sleevi wrote:
> superflous braces

Even on a for loop? Eugh, okay...

[Ryan Sleevi, 2015-03-24 23:47:22]

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:206: // every 256 connections.
Is there any automated pruning of expired sessions now? Or if I leave Chrome
running for a day, don't touch it, then come back, *all* the sessions will be
expired-but-cached?

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1939: "424386
SSLClientSocketOpenSSL::InfoCallback"));
Nuke this. Vadim orphaned the bug :(

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:172: // is cached.
It took me a while to parse this comment, as I thought you were referring to
InfoCallback

// Called after the the initial handshake has completed and after
// the server certificate has been verified.
// The order of handshake completion/cert verification is depends on
// whether or not the connection was false started. After both have
// happened (thus calling this twice), the session is safe to cache
// and will be cached.

Or something. I'm not quite sure, but it was "the events may happen in either
order" and the number of times this is called that confuse me.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:175: void InfoCallback(int type, int
val);
Document. How does this differ from say 164? May just need a comment like
162/163 (this is after every record received, right?)

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2822:
EXPECT_TRUE(sock->GetSSLInfo(&ssl_info));
Should this be an ASSERT on 2820? Otherwise isn't this sketch? GetSSLInfo
doesn't actually check that you connected.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2827: EXPECT_EQ(OK,
callback.GetResult(transport->Connect(callback.callback())));
ASSERT

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2830: EXPECT_EQ(OK,
callback.GetResult(sock->Connect(callback.callback())));
ASSERT

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2834: // Using a different HostPortPair
uses a different session cache key.
This doesn't seem right - you use the same underlying address. NSS uses this as
the session cache key.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2836: EXPECT_EQ(OK,
callback.GetResult(transport->Connect(callback.callback())));
ASSERT

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2839: EXPECT_EQ(OK,
callback.GetResult(sock->Connect(callback.callback())));
ASSERT

[davidben, 2015-03-26 20:22:57]

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:206: // every 256 connections.
On 2015/03/24 23:47:21, Ryan Sleevi wrote:
> Is there any automated pruning of expired sessions now? Or if I leave Chrome
> running for a day, don't touch it, then come back, *all* the sessions will be
> expired-but-cached?

That comment and call is the same as the file from before. Looking at it again,
the comment is confusing and the call is almost completely useless:

- NO_AUTO_CLEAR is a no-op if you have NO_INTERNAL_STORE. It will try to do
something (though I have a CL not yet rolled into DEPS to make it actually not
do anything), but it's moot because OpenSSL has nothing to iterate over.

- Since we only use this on the client, both NO_AUTO_CLEAR and NO_INTERNAL_STORE
are no-ops unless SSL_SESS_CACHE_CLIENT is set.

- If SSL_SESS_CACHE_CLIENT is not set, neither the internal cache nor the
SSL_CTX-level cache hooks are used. But it doesn't matter because we're not
using them.

- The default is SSL_SESS_CACHE_SERVER without SSL_SESS_CACHE_CLIENT.

- Now that I look at the code, there seems to be a bug on the server end where
SSL_SESS_CACHE_SERVER is only consulted on lookup and not install. Ooops.

- There's also NO_INTERNAL_LOOKUP which is only meaningful on the server's end.
Even when using the internal session cache, OpenSSL expects the caller to
perform the lookup because OpenSSL knows nothing about session cache keys.

- Basically this API is confusing, the previous SSLSessionCacheOpenSSL was
somewhat botched, and, as always with OpenSSL, the only way to have any clue
what's going on is to read the source. :-)

The upshot is we could remove that line altogether and everything would still
work.

I've replaced it with SSL_SESS_CACHE_CLIENT|SSL_SESS_CACHE_NO_INTERNAL. I think
that's a good mix of not being overly confusing or reliant on whatever the
default session cache mode happens to be. SSL_SESS_CACHE_OFF (0) would work just
as well, but it seems slightly confusing there; I could imagine the API being
defined to, say, disable requesting session tickets altogether. (Although it
does not do such a thing.) NO_INTERNAL is NO_INTERNAL_STORE +
NO_INTERNAL_LOOKUP.


The pruning happens every 256 lookups into the cache. This is the same as the
current code. It is also roughly the same as the even older code that used the
internal session cache. It may be worth revising this[*], but I've left it alone
for now. My main goal was to fix the explosions if you cache two sessions with
the same id. That bug's been sitting around for a while and is probably a remote
DoS of some sort, though I haven't analyzed it carefully.

I gone ahead and made a slight improvement over the old code which is that it
will also check for expiration before returning anything in Lookup. So at least
it behaves as if it constantly checked expiration, but for size() being off.

[*] I could imagine keeping a list of the sessions in order of expiration time
to avoid having the magic 256 parameter.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:1939: "424386
SSLClientSocketOpenSSL::InfoCallback"));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> Nuke this. Vadim orphaned the bug :(

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.h (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:172: // is cached.
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> It took me a while to parse this comment, as I thought you were referring to
> InfoCallback
> 
> // Called after the the initial handshake has completed and after
> // the server certificate has been verified.
> // The order of handshake completion/cert verification is depends on
> // whether or not the connection was false started. After both have
> // happened (thus calling this twice), the session is safe to cache
> // and will be cached.
> 
> Or something. I'm not quite sure, but it was "the events may happen in either
> order" and the number of times this is called that confuse me.

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.h:175: void InfoCallback(int type, int
val);
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> Document. How does this differ from say 164?

They're basically totally unrelated. :-P

> May just need a comment like
> 162/163

> (this is after every record received, right?)

It's called when... Stuff Happens. It's not really clear and kind of called all
over the place. Sometime later I mean to look at all the users and see if I can
trim it down, since it even sees handshake state machine transitions with the
internal state.

Added a comment.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2822:
EXPECT_TRUE(sock->GetSSLInfo(&ssl_info));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> Should this be an ASSERT on 2820? Otherwise isn't this sketch? GetSSLInfo
> doesn't actually check that you connected.

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2827: EXPECT_EQ(OK,
callback.GetResult(transport->Connect(callback.callback())));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> ASSERT

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2830: EXPECT_EQ(OK,
callback.GetResult(sock->Connect(callback.callback())));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> ASSERT

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2834: // Using a different HostPortPair
uses a different session cache key.
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> This doesn't seem right - you use the same underlying address. NSS uses this
as
> the session cache key.

If NSS only used that as the session cache key, all kinds of things would
explode. :-P It also uses the peer_id string which includes the HostPortPair.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2836: EXPECT_EQ(OK,
callback.GetResult(transport->Connect(callback.callback())));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> ASSERT

Done.

https://codereview.chromium.org/994263002/diff/140001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:2839: EXPECT_EQ(OK,
callback.GetResult(sock->Connect(callback.callback())));
On 2015/03/24 23:47:22, Ryan Sleevi wrote:
> ASSERT

Done.

[Ryan Sleevi, 2015-04-02 06:53:15]

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:205: // externally.
// externally (i.e. by SSLClientSessionCacheOpenSSL).

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3065:
TEST_F(SSLClientSocketFalseStartTest, NoSessionResumptionBeforeFinish) {
Could you add a test for a *bad* finished message? Too complex?

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:89: return now > expiration;
I still can't help but feel like this is going to lead to a bad time - possibly
even a security bug!

Does it make sense to store either creation time / tick counts with it, to
ensure that things are sane? I'd rather pay the memory cost overhead of getting
the clocks right than run the risk of some sort of session replay *handwave*
thing

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.h (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:34: virtual
~SSLClientSessionCacheOpenSSL();
This doesn't need to be virtual, does it? No vtable otherwise.

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:55: struct CacheEntry {
I probably asked this earlier, but since you're storing pointers, do you for
sure need this? If MRUCacheBase doesn't call MRUCachePointerDeletor<> in the
sizing of the object (which I don't think it should), you should be able to get
away with SFINAE and delay the instantiation of the methods until the .cc calls
them, which means a forward decl would work.

Minor, I suppose...

[davidben, 2015-04-02 07:21:45]

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:89: return now > expiration;
On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> I still can't help but feel like this is going to lead to a bad time -
possibly
> even a security bug!
> 
> Does it make sense to store either creation time / tick counts with it, to
> ensure that things are sane? I'd rather pay the memory cost overhead of
getting
> the clocks right than run the risk of some sort of session replay *handwave*
> thing

We could, but I'm not sure I see the difference. The check is now >
creation_time + timeout. Both of those are constant, so we may as well store the
precomputed sum. Is the worry that you might double-insert a session and refresh
the creation time? Storing the creation time would have the same failure mode.
Either we get confused and refresh things or we don't. I think that's best
avoided by only inserting fresh SSL_SESSIONs into the cache, which is what the
SSL_session_reused check in ssl_client_socket_openssl.cc buys.

(I'm confused what session replay means. There's nothing saying you can't offer
to resume a session multiple times. Replay is what the client_random is for. The
only concern I can imagine is if you want to promise that session key material
doesn't last longer than $TIMEOUT since resumption doesn't have forward secrecy.
This cache does manage that by virtue of line 49. The old one, incidentally,
didn't because it only evaluated expiration on FlushExpiredSessions. OpenSSL's
internal cache is similar.)

[Ryan Sleevi, 2015-04-02 07:46:55]

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:89: return now > expiration;
I meant re-using a session beyond the timelimit configured that we've deemed
acceptable. That is, there's nothing (server side) guaranteeing that we aren't
re-using a session for > 24 hours, for example.

I'm thinking of scenarios like
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr...

which was a reaction to http://crbug.com/132124

TL;DR: Store the time we stored the session. If Now < that time, treat it as
expired (since we can no longer reason about the time difference). If Now > that
time, then make sure Now < expiration time. Otherwise, treat it as expired
(since it truly is expired)

[davidben, 2015-04-02 19:05:10]

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_openssl.cc:205: // externally.
On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> // externally (i.e. by SSLClientSessionCacheOpenSSL).

Done.

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3065:
TEST_F(SSLClientSocketFalseStartTest, NoSessionResumptionBeforeFinish) {
On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> Could you add a test for a *bad* finished message? Too complex?

Added one for the Finished message hitting an error. It's all the same codepath.
OpenSSL never distinguishes an operation failing from it failing to complete.

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:89: return now > expiration;
On 2015/04/02 07:46:55, Ryan Sleevi wrote:
> I meant re-using a session beyond the timelimit configured that we've deemed
> acceptable. That is, there's nothing (server side) guaranteeing that we aren't
> re-using a session for > 24 hours, for example.
> 
> I'm thinking of scenarios like
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/cert/multi_thr...
> 
> which was a reaction to http://crbug.com/132124
> 
> TL;DR: Store the time we stored the session. If Now < that time, treat it as
> expired (since we can no longer reason about the time difference). If Now >
that
> time, then make sure Now < expiration time. Otherwise, treat it as expired
> (since it truly is expired)
> 

Ah, okay. Done.

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.h (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:34: virtual
~SSLClientSessionCacheOpenSSL();
On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> This doesn't need to be virtual, does it? No vtable otherwise.

Done. (Remnant of virtual Now method)

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:55: struct CacheEntry {
On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> I probably asked this earlier, but since you're storing pointers, do you for
> sure need this? If MRUCacheBase doesn't call MRUCachePointerDeletor<> in the
> sizing of the object (which I don't think it should), you should be able to
get
> away with SFINAE and delay the instantiation of the methods until the .cc
calls
> them, which means a forward decl would work.
> 
> Minor, I suppose...

I believe you're right. Done. (I had to break header order though; otherwise the
methods defined above can't do anything.)

[Ryan Sleevi, 2015-04-02 20:00:14]

LGTM % nit but I'm not fixated on it

https://codereview.chromium.org/994263002/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3140: // Finished message failed.
I didn't mean reading the server finished failed - I meant reading (fully
consuming) a server finished, but the server finished is _wrong_ (doesn't match)

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:91: base::Time now) {
const-ref

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:46:
EXPECT_EQ(session3.get(), cache.Lookup("key1"));
EXPECT_NEQ(session3.get(), session1.get()) ?

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:105: base::SimpleTestClock*
clock = new base::SimpleTestClock;
Why store the naked pointer? It's unused.

[davidben, 2015-04-03 00:37:12]

Note: diffing between patch set 9 might be more useful.

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.h (right):

https://codereview.chromium.org/994263002/diff/160001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.h:55: struct CacheEntry {
On 2015/04/02 19:05:10, David Benjamin wrote:
> On 2015/04/02 06:53:15, Ryan Sleevi wrote:
> > I probably asked this earlier, but since you're storing pointers, do you for
> > sure need this? If MRUCacheBase doesn't call MRUCachePointerDeletor<> in the
> > sizing of the object (which I don't think it should), you should be able to
> get
> > away with SFINAE and delay the instantiation of the methods until the .cc
> calls
> > them, which means a forward decl would work.
> > 
> > Minor, I suppose...
> 
> I believe you're right. Done. (I had to break header order though; otherwise
the
> methods defined above can't do anything.)

This seems to upset win8_chromium_rel. I've moved it back to the header.

http://build.chromium.org/p/tryserver.chromium.win/builders/win8_chromium_rel...

https://codereview.chromium.org/994263002/diff/200001/net/socket/ssl_client_s...
File net/socket/ssl_client_socket_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/socket/ssl_client_s...
net/socket/ssl_client_socket_unittest.cc:3140: // Finished message failed.
On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> I didn't mean reading the server finished failed - I meant reading (fully
> consuming) a server finished, but the server finished is _wrong_ (doesn't
match)

Switched it to that. That was actually somewhat less of a pain than I expected.
Though all three are still testing the exact same thing as far as Chromium is
concerned. :-P

I've also now uploaded https://boringssl-review.googlesource.com/#/c/4212/ which
tests, BoringSSL-side, the interactions between the callbacks.

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl.cc:91: base::Time now) {
On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> const-ref

Done.

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:46:
EXPECT_EQ(session3.get(), cache.Lookup("key1"));
On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> EXPECT_NEQ(session3.get(), session1.get()) ?

Er. That's just asserting that SSL_SESSION_new returns different pointers each
time, no?

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:105: base::SimpleTestClock*
clock = new base::SimpleTestClock;
On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> Why store the naked pointer? It's unused.

Copy-pasta. Removed.

[Ryan Sleevi, 2015-04-03 00:59:02]

LGTM

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:46:
EXPECT_EQ(session3.get(), cache.Lookup("key1"));
On 2015/04/03 00:37:11, David Benjamin wrote:
> On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> > EXPECT_NEQ(session3.get(), session1.get()) ?
> 
> Er. That's just asserting that SSL_SESSION_new returns different pointers each
> time, no?

Well, and that no one accidentally stores a pointer to session3.get in the
interim of the test.

[davidben, 2015-04-07 18:35:36]

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
File net/ssl/ssl_client_session_cache_openssl_unittest.cc (right):

https://codereview.chromium.org/994263002/diff/200001/net/ssl/ssl_client_sess...
net/ssl/ssl_client_session_cache_openssl_unittest.cc:46:
EXPECT_EQ(session3.get(), cache.Lookup("key1"));
On 2015/04/03 00:59:02, Ryan Sleevi wrote:
> On 2015/04/03 00:37:11, David Benjamin wrote:
> > On 2015/04/02 20:00:14, Ryan Sleevi wrote:
> > > EXPECT_NEQ(session3.get(), session1.get()) ?
> > 
> > Er. That's just asserting that SSL_SESSION_new returns different pointers
each
> > time, no?
> 
> Well, and that no one accidentally stores a pointer to session3.get in the
> interim of the test.

But that can be determined trivially by inspection of this function. It's not a
property of BoringSSL or SSLClientSessionCacheOpenSSL. session3.get() is a local
variable.

It's also implied by lines 50 and 52.

[davidben, 2015-04-08 01:30:48]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-08 01:30:49]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/994263002/#ps240001
(title: "rebase")

[commit-bot: I haz the power, 2015-04-08 01:31:25]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994263002/240001

[commit-bot: I haz the power, 2015-04-08 04:56:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-08 04:56:36]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2015-04-08 18:50:37]

The CQ bit was checked by davidben@chromium.org

[commit-bot: I haz the power, 2015-04-08 18:52:33]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994263002/240001

[commit-bot: I haz the power, 2015-04-08 19:02:17]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2015-04-08 19:02:19]

Try jobs failed on following builders:
  linux_chromium_clobber_rel_ng on tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[davidben, 2015-04-08 20:11:15]

The CQ bit was checked by davidben@chromium.org

[davidben, 2015-04-08 20:11:16]

The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org
Link to the patchset: https://codereview.chromium.org/994263002/#ps260001
(title: "reebase")

[commit-bot: I haz the power, 2015-04-08 20:12:44]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/994263002/260001

[commit-bot: I haz the power, 2015-04-08 22:54:05]

Committed patchset #13 (id:260001)

[commit-bot: I haz the power, 2015-04-08 22:54:57]

Patchset 13 (id:??) landed as
https://crrev.com/dafe4e53058ed802fabc151e67e75ffded76fd18
Cr-Commit-Position: refs/heads/master@{#324292}