Chromium Code Reviews
Help | Chromium Project | Gerrit Changes | Sign in
(2)

Issue 502087: Use Separate SSL Session Cache in OTR Mode (Closed)

Created:
10 years, 5 months ago by mwenge
Modified:
7 years, 11 months ago
CC:
chromium-reviews, darin (slow to review), ben+cc_chromium.org, wtc, Matt Perry, abarth-chromium, Pam (message me for reviews), Bernhard Bauer
Visibility:
Public.

Description

Use Separate SSL Session Cache in OTR Mode Currently Chromium maintains a persistent TLS session cache between OTR and non-OTR mode. This means that a user can go to https://gmail.com in ordinary mode, open an incognito window, go to https://gmail.com and Chromium will use the TLS Session ID from ordinary mode to resume that TLS session. This patch changes the behaviour as follows: - TLS Session IDs generated in non-OTR mode are only used in non-OTR mode. - TLS Session IDs generated in OTR mode are only used in OTR mode. The patch implements this behavour for Linux, Mac and Windows. Chromium's OTR profile now has it's own copy of the SSL configuration settings. If the profile is OTR, a new member of SSLConfig called otr_mode is set to true. By default, otr_mode is set to false. On Mac and Linux, if otr_mode is true the phrase "-OTR" is added to the hostname and port information when constructing the peer_id used to calculate and store the SSL connection's session ID. This results in a distinct session cache for SSL connections for each mode. On Windows an extra bit is added to SSL_VERSION_MASKS. This bitmask defines the number of CredHandles stored in a CredHandle lookup table. When the SSL connection belongs to a request made in OTR mode the CredHandle for that connection will be stored and retrieved from the CredHandle members of the array reserved for OTR mode. See also: https://groups.google.com/group/chromium-extensions/msg/e83920020719a6b2?hl=en BUG=30877 TEST=https sites perform identically (in particular, https://test-ssev.verisign.com/ and the three pages linked from there)

Patch Set 1 : '' #

Total comments: 3
Unified diffs Side-by-side diffs Delta from patch set Stats (+33 lines, -5 lines) Patch
M chrome/browser/net/ssl_config_service_manager_pref.cc View 3 chunks +6 lines, -0 lines 0 comments Download
M chrome/browser/profile.cc View 3 chunks +6 lines, -1 line 0 comments Download
M net/base/ssl_config_service.h View 2 chunks +4 lines, -1 line 0 comments Download
M net/socket/ssl_client_socket_mac.cc View 1 chunk +3 lines, -0 lines 1 comment Download
M net/socket/ssl_client_socket_nss.cc View 1 chunk +8 lines, -1 line 1 comment Download
M net/socket/ssl_client_socket_win.cc View 2 chunks +6 lines, -2 lines 1 comment Download

Messages

Total messages: 13 (0 generated)
abarth-chromium
Sorry for missing this review! I think wtc is your best bet.
10 years, 3 months ago (2010-02-03 19:58:02 UTC) #1
Matt Perry
Wan-Teh, are you the right person to do this review?
10 years, 3 months ago (2010-02-03 20:01:04 UTC) #2
wtc
Matt: do you want to completely turn off TLS session resumption in OTR mode? Can ...
10 years, 3 months ago (2010-02-03 20:20:23 UTC) #3
Matt Perry
On 2010/02/03 20:20:23, wtc wrote: > Matt: do you want to completely turn off TLS ...
10 years, 3 months ago (2010-02-10 19:15:23 UTC) #4
mwenge
On Wednesday 10 February 2010 19:15:23 mpcomplete@chromium.org wrote: > On 2010/02/03 20:20:23, wtc wrote: > ...
10 years, 3 months ago (2010-02-12 15:52:24 UTC) #5
wtc
On 2010/02/12 15:52:24, mwenge wrote: > > OSX: Session caching isn't currently supported by chromium ...
10 years, 3 months ago (2010-02-12 23:16:02 UTC) #6
mwenge
On Friday 12 February 2010 23:16:03 wtc@chromium.org wrote: > On 2010/02/12 15:52:24, mwenge wrote: > ...
10 years, 3 months ago (2010-02-13 12:49:21 UTC) #7
mwenge
On 2010/02/12 23:16:02, wtc wrote: > Before you write more code, could you state what ...
9 years, 10 months ago (2010-07-21 09:56:55 UTC) #8
davidben
I'm not sure the approach taken here is the right one. The networking layer and ...
9 years, 10 months ago (2010-07-26 21:31:33 UTC) #9
jochen (gone - plz use gerrit)
Just a random note: the issue described in the description of this issue is not ...
9 years, 10 months ago (2010-07-27 18:40:06 UTC) #10
mwenge
On 2010/07/27 18:40:06, jochen wrote: > Just a random note: the issue described in the ...
9 years, 10 months ago (2010-07-27 19:28:06 UTC) #11
davidben
This comment from Robert seemed to only make it to email and not the page ...
9 years, 10 months ago (2010-07-27 20:35:14 UTC) #12
davidben
9 years, 10 months ago (2010-07-27 20:58:32 UTC) #13
> Hi Ben,

Hi. (Actually, I'm David. :-D But I get called Ben often enough that I mostly
answer to it.)

> I don't think I follow you here.
> 
> My patch introduces a separate SSLConfig for each profile. Are you saying
> that instead of assuming the only 'other' profile will ever be OTR I should
> maintain a unique reference in the SSLConfig, such as a counter, and allow
> that to uniquify the entry in the session id cache instead of just
> appending OTR to the host/port?
> 
> I don't know the code well enough to know where URLRequestContexts come
> into it in this situation.

Right, a separate SSLConfigService for each profile is certainly be part of the
solution. But your patch only uniquifies the peer ID by an OTR bit. The network
stack is not intended to depend on the browser components, so a more generic
solution would be better. So yeah; I'm saying the uniquifying should be
independent of OTR mode, possibly by some kind of counter.

The URLRequestContext is an class that users of the network stack subclass and
pass in with requests. State shared between requests hangs off there (cookies,
SSLConfigService, etc.). As "which SSL session" cache to use is such state, it
should ultimately be determined by that class, probably by way of SSLConfig and
SSLConfigService.

I'm not sure just sticking the uniquifier in the SSLConfig will work. It's
conceivable that we instantiate lots of duplicates, or that we may create
variants of the same URLRequestContext, so there should be a mechanism for them
to share caches. I'm not familiar with that code. Others probably could provide
more authoritative comments as to where would be best place. I'm imagining some
kind of a net::SSLSessionCache handle which wraps an internal counter, with the
handle the context chooses selecting the cache.

Powered by Google App Engine
This is Rietveld 408576698