net: split the SSL session cache between incognito and normal.

net: split the SSL session cache between incognito and normal.

This change causes incognito requests to effectively have a different SSL
session cache from other requests. SSL session information will therefore not
leak into or out of incognito mode.

BUG=30877
TEST=net_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=114098

[willchan no longer on Chromium, 2011-12-07 21:21:31]

I think this is fine, although as discussed offline, it may be the case that
different profiles should have different SSL session caches. I defer to mirandac
here, unless she defers back to me, in which case I think you should indeed use
separate SSL session caches.

I know you mentioned the early google.com entry from the system
URLRequestContext. Note that the profile specific ones should have early
google.com entries too. GoogleURLTracker will move to being profile-specific I
think.

LGTM.

[agl, 2011-12-09 20:54:18]

joth: please note the OpenSSL changes that I made.

wtc: just an FYI really. The only change that you might care about is the
movement of ClearSessionCache into SSLClientSocket.

[joth, 2011-12-09 23:28:37]

openssl bit lgtm with minor comments

http://codereview.chromium.org/8857002/diff/3001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/8857002/diff/3001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.cc:206: // cache with the
rest of the browser.
maybe put this in a named  constant, so the detailed explanation can sit with it
rather than wedged in this already long function call?

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (left):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:272: // cache on the client certificate
used in addition to the host-port pair.
thanks for handling this one too. if I've followed it right, we re-use the
client cert selection so long as the session remains in the cache?

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:203: // unique HostPortPair.
per unique HostPortPair per shard.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:244: << it->second->first;
nit: no need to line break

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:262: << cache_key;
nit: no need for line break

[wtc, 2011-12-10 01:22:37]

Patch Set 2 LGTM.  Thanks!

High-level comments:

1. I didn't review the openssl code and the new unit tests
in url_request_unittest.cc.

2. It is fine by me to share the SSL sessions between all
the non-incognito profiles.

3. I suggest some renaming (adding "ssl" before "session").

http://codereview.chromium.org/8857002/diff/3001/net/http/http_cache.h
File net/http/http_cache.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/http/http_cache.h#newcod...
net/http/http_cache.h:129: const std::string& session_cache_shard,

Please name this parameter ssl_session_cache_shard, with
the ssl_ prefix.

You should be able to do a global renaming in this CL.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:56: // session cache. SSL sockets with the same
session_cache_shard may resume

session cache => SSL session cache

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss_unittest.cc (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss_unittest.cc:16: }

We should move this unit test to ssl_client_socket_unittest.cc
as TEST(SSLClientSocketTest, ClearSessionCache), and then remove
this file.

http://codereview.chromium.org/8857002/diff/3001/net/test/test_server.h
File net/test/test_server.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/test/test_server.h#newco...
net/test/test_server.h:109: // /session-cache.

/session-cache => /ssl-session-cache

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:71: class RecordingSessionCache(object):

I suggest renaming

  SessionCache => SSLSessionCache

where the context is not SSL-specific.  Up to you...

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1407: if not
self._ShouldHandleRequest('/session-cache'):
/session-cache => /ssl-session-cache

also on line 1966 below.

[agl, 2011-12-12 22:18:20]

http://codereview.chromium.org/8857002/diff/3001/chrome/browser/profiles/off_...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/8857002/diff/3001/chrome/browser/profiles/off_...
chrome/browser/profiles/off_the_record_profile_io_data.cc:206: // cache with the
rest of the browser.
On 2011/12/09 23:28:37, joth wrote:
> maybe put this in a named  constant, so the detailed explanation can sit with
it
> rather than wedged in this already long function call?

Done.

http://codereview.chromium.org/8857002/diff/3001/net/http/http_cache.h
File net/http/http_cache.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/http/http_cache.h#newcod...
net/http/http_cache.h:129: const std::string& session_cache_shard,
On 2011/12/10 01:22:37, wtc wrote:
> 
> Please name this parameter ssl_session_cache_shard, with
> the ssl_ prefix.
> 
> You should be able to do a global renaming in this CL.

Done.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket.h
File net/socket/ssl_client_socket.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket.h:56: // session cache. SSL sockets with the same
session_cache_shard may resume
On 2011/12/10 01:22:37, wtc wrote:
> 
> session cache => SSL session cache

Done.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_nss_unittest.cc (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_nss_unittest.cc:16: }
On 2011/12/10 01:22:37, wtc wrote:
> 
> We should move this unit test to ssl_client_socket_unittest.cc
> as TEST(SSLClientSocketTest, ClearSessionCache), and then remove
> this file.

Done.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (left):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:272: // cache on the client certificate
used in addition to the host-port pair.
On 2011/12/09 23:28:37, joth wrote:
> thanks for handling this one too. if I've followed it right, we re-use the
> client cert selection so long as the session remains in the cache?

Right. And when we add a new client-cert, we flush the session cache.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
File net/socket/ssl_client_socket_openssl.cc (right):

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:203: // unique HostPortPair.
On 2011/12/09 23:28:37, joth wrote:
> per unique HostPortPair per shard.

Done.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:244: << it->second->first;
On 2011/12/09 23:28:37, joth wrote:
> nit: no need to line break

Done.

http://codereview.chromium.org/8857002/diff/3001/net/socket/ssl_client_socket...
net/socket/ssl_client_socket_openssl.cc:262: << cache_key;
On 2011/12/09 23:28:37, joth wrote:
> nit: no need for line break

Done.

http://codereview.chromium.org/8857002/diff/3001/net/test/test_server.h
File net/test/test_server.h (right):

http://codereview.chromium.org/8857002/diff/3001/net/test/test_server.h#newco...
net/test/test_server.h:109: // /session-cache.
On 2011/12/10 01:22:37, wtc wrote:
> 
> /session-cache => /ssl-session-cache

Done.

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
File net/tools/testserver/testserver.py (right):

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:71: class RecordingSessionCache(object):
On 2011/12/10 01:22:37, wtc wrote:
> 
> I suggest renaming
> 
>   SessionCache => SSLSessionCache
> 
> where the context is not SSL-specific.  Up to you...

Done.

http://codereview.chromium.org/8857002/diff/3001/net/tools/testserver/testser...
net/tools/testserver/testserver.py:1407: if not
self._ShouldHandleRequest('/session-cache'):
On 2011/12/10 01:22:37, wtc wrote:
> /session-cache => /ssl-session-cache
> 
> also on line 1966 below.

Done.

[commit-bot: I haz the power, 2011-12-12 22:18:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/8857002/14001

[commit-bot: I haz the power, 2011-12-12 22:18:50]

Presubmit check for 8857002-14001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for:
webkit/tools/test_shell/test_shell_request_context.cc

Presubmit checks took 4.6s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Ryan Sleevi, 2011-12-12 23:07:06]

post-review drive-by:

This doesn't touch SSLClientSocketWin/SSLClientSocketMac - are they a concern?
Have we reached the point of not wanting to maintain them (or maintain parity)?

Given that you were initially opposed to working on this bug, what are your
sentiments towards further improvements on this?
 - Support on _win.cc (which uses the cred handle as the primary session cache,
which may necessitate exposing this as an object rather than a string)
 - Support on _mac.cc (which uses an opaque string and thus could be easily
adopted)
 - Support for multi-profile (correlates highly w/ support for incognito
re-entrancy)

http://codereview.chromium.org/8857002/diff/14001/chrome/browser/profiles/off...
File chrome/browser/profiles/off_the_record_profile_io_data.cc (right):

http://codereview.chromium.org/8857002/diff/14001/chrome/browser/profiles/off...
chrome/browser/profiles/off_the_record_profile_io_data.cc:140: static const char
kIncognitoSSLCacheShard[] = "incognito";
agl: Once concern with using a fixed string like this is that entering, exiting,
and then re-entering incognito mode will allow previously cached sessions to be
returned - in essence, 'leaking' state between incognito sessions.

Is this a concern? Both for Desktop (which may run for weeks) and for CrOS?

[agl, 2011-12-12 23:21:37]

On Mon, Dec 12, 2011 at 6:07 PM,  <rsleevi@chromium.org> wrote:
> This doesn't touch SSLClientSocketWin/SSLClientSocketMac - are they a
> concern?
> Have we reached the point of not wanting to maintain them (or maintain
> parity)?

We long ago stopped maintaining parity, although I've of course no
objections to patches to improve them.

>  - Support on _win.cc (which uses the cred handle as the primary session
> cache,
> which may necessitate exposing this as an object rather than a string)

No problem. Changing the type of the ssl_session_cache_shard wouldn't
be too bad. (Although it shouldn't really be called _shard in that
case.)

>  - Support on _mac.cc (which uses an opaque string and thus could be easily
> adopted)

No problem.

>  - Support for multi-profile (correlates highly w/ support for incognito
> re-entrancy)

I talked to Miranda about this and decided not to do it.

We basically have a problem of defining what incognito mode and
multi-profiles are.

incognito mode was (is?) a means to avoid storing local, persistent
state on the machine. But then we implemented with a fresh cookie jar
and an in-memory HTTP cache, rather than read-only layers, so then it
sort of became more like a means to avoid *any* leak of state.

I didn't do this before because, since the session cache is never
written to disk, sharing it doesn't contradict the original incognito
mode purpose. But maybe we're now changing it? If we are, then we
shouldn't leak the (read-only) HSTS state either. (And, if Tor are
going to use it, then that answers that question.)

For multi-profiles, Miranda said the HTTP cache was split simply
because they didn't trust websites to get caching right, not for any
other reason. So I didn't split the SSL session cache for each
profile. Not that I really object to it, but the CL was long enough
already.

> agl: Once concern with using a fixed string like this is that entering,
> exiting, and then re-entering incognito mode will allow previously
> cached sessions to be returned - in essence, 'leaking' state between
> incognito sessions.

Yea, I dunno. What does incognito mode mean? Whether this is a problem
really depends on what Tor think about it.


Cheers

AGL