Clear the SSL Client Auth cache when a new SSL Client Certificate is...

Clear the SSL Client Auth cache when a new SSL Client Certificate is
installed (e.g. via the <keygen> tag).

Replaced by CL: 6588014

[Ryan Hamilton, 2011-02-15 00:34:20]

Hi Wan-Teh,

Here's my attempt to clear the SSL auth cache when a new client cert is
installed.  It ended up being a bit more complicated than that since we also
need to clear the ssl session cache, and close any ssl connections.  Can you
review?

Thanks,

Ryan

[wtc, 2011-02-15 20:50:40]

rch: is there a bug report for this work?

I suggest some changes below.  Most of my comments center
around adding the methods to the right classes.  Thanks.

http://codereview.chromium.org/6487012/diff/14001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/6487012/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:120:
request_->context()->http_transaction_factory()->GetSession();
It seems bad to expose so much internals of HttpNetworkSession.

It seems better to add a method to http_transaction_factory()
or HttpNetworkSession to clear the relevant state.

Are you sure you want to flush socket pools and close SPDY
sessions?  It seems enough to just clear the SSL client
auth cache and the SSL session cache.

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache.h (right):

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache.h:47: // Removes all mappings
Nit: Removes all cache entries.

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache_unittest.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache_unittest.cc:140: // Check that the Clear() method
removes all mappings.
Nit: mappings => cache entries
             or  cached client certificates

Make the same change to line 166 below.

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
net/socket/client_socket_factory.cc:115: return
SSLClientSocketNSS::ClearSessionCache();
For OS_WIN and OS_MACOSX, this function should call
SSLClientSocketNSS::ClearSessionCache() by default.

The complication is that we support the --use-system-ssl
command-line option on these two platforms.  If --use-system-ssl
is specified, we won't use NSS for SSL.  It seems that
you are handling this.

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
net/socket/client_socket_factory.h:78: static void
DefaultClearSSLSessionCache();
These three static methods should be part of the
SSLClientSocket class.  Agreed?

Do you need to expose the DefaultClearSSLSessionCache()
method?

http://codereview.chromium.org/6487012/diff/14001/net/socket/ssl_client_socke...
File net/socket/ssl_client_socket_mac_factory.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/socket/ssl_client_socke...
net/socket/ssl_client_socket_mac_factory.cc:28:
ClientSocketFactory::DefaultClearSSLSessionCache);
BUG: this is wrong.  If the SSL client factory creates
SSLClientSocketMac objects, then the clear SSL session cache
function should not be the default one, which uses NSS.

http://codereview.chromium.org/6487012/diff/14001/webkit/tools/test_shell/sim...
File webkit/tools/test_shell/simple_resource_loader_bridge.cc (right):

http://codereview.chromium.org/6487012/diff/14001/webkit/tools/test_shell/sim...
webkit/tools/test_shell/simple_resource_loader_bridge.cc:891:
net::UseNSSSSLProvider();
Perhaps we should have a SetSSLProvider function that takes
an enum input, with the possible values 'NSS' and 'SYSTEM'.
This will avoid the quadruple-S problem :-)

[Ryan Hamilton, 2011-02-16 17:44:40]

Hi Wan-Teh,

Thanks for the feedback!  I'm going to get started on the refactor I mention in
this comment, but I want to send you my idea before I finish it in case you
conclude it's a terrible idea :>

Cheers,

Ryan

http://codereview.chromium.org/6487012/diff/14001/chrome/browser/renderer_hos...
File chrome/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/6487012/diff/14001/chrome/browser/renderer_hos...
chrome/browser/renderer_host/x509_user_cert_resource_handler.cc:120:
request_->context()->http_transaction_factory()->GetSession();
On 2011/02/15 20:50:40, wtc wrote:
> It seems bad to expose so much internals of HttpNetworkSession.
> 
> It seems better to add a method to http_transaction_factory()
> or HttpNetworkSession to clear the relevant state.

Sounds reasonable.  I move all this logic to a new method:
HttpNetworkSession::ResetSSLState.  (Though I wonder if
HandleNewClientCertificate or OnClientCertificateInstalled might be better?)

> Are you sure you want to flush socket pools and close SPDY
> sessions?  It seems enough to just clear the SSL client
> auth cache and the SSL session cache.

Yes.  The motivating example in a server which does optional SSL client auth. 
If it does not see a cert in the request, it 302s to a different server which
can issue a client certificate.  When that certificate is installed, the user
navigates back to the initial server and attempts to use the certificate.  If we
do not close the open connection to the initial server, the server will still
not see the new certificate, and will 302 back to the auth front end.

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache.h (right):

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache.h:47: // Removes all mappings
On 2011/02/15 20:50:40, wtc wrote:
> Nit: Removes all cache entries.

Done.

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache_unittest.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache_unittest.cc:140: // Check that the Clear() method
removes all mappings.
On 2011/02/15 20:50:40, wtc wrote:
> Nit: mappings => cache entries
>              or  cached client certificates
> 
> Make the same change to line 166 below.

Done.

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
net/socket/client_socket_factory.cc:115: return
SSLClientSocketNSS::ClearSessionCache();
On 2011/02/15 20:50:40, wtc wrote:
> For OS_WIN and OS_MACOSX, this function should call
> SSLClientSocketNSS::ClearSessionCache() by default.

You're right, of course.  However, I notice a very odd asymmetry between mac and
windows.  On both systems we use NSS by default (well, we use NSS by default
everywhere now).  However, the method DefaultSSLClientSocketFactory says:

#if defined(OS_WIN)
  return new SSLClientSocketWin(transport_socket, host_and_port, ssl_config,
                                cert_verifier);
...
#elif defined(OS_MACOSX)
  return new SSLClientSocketNSS(transport_socket, host_and_port, ssl_config,
                                shi.release(), cert_verifier, dns_cert_checker);
...

Then as a consequence, browser_main is similarly contorted:

browser_main_win.cc:

    if (!parsed_command_line().HasSwitch(switches::kUseSystemSSL)) {
      net::ClientSocketFactory::SetSSLClientSocketFactory(
          net::SSLClientSocketNSSFactory);

browser_main_mac.cc:

    if (parsed_command_line().HasSwitch(switches::kUseSystemSSL)) {
      net::ClientSocketFactory::SetSSLClientSocketFactory(
          net::SSLClientSocketMacFactory);

Notice that the predicates are reverse, as are the actions.

> The complication is that we support the --use-system-ssl
> command-line option on these two platforms.  If --use-system-ssl
> is specified, we won't use NSS for SSL.  It seems that
> you are handling this.

I propose that we make the following change.

* Modify DefaultSSLClientSocketFactory to return new SSLClientSocketNSS for win
(which is to say all cases except for openssl.  Out of curiosity, under what
circumstances to we use openssl?  Only if explicitly compiled to use it?)
* Modify browser_main_mac.cc and browser_main_win.cc to check for
--use-system-ssl and, if present, invoke: ClientSocketFactory::UseSystemSSL()

Does this work for you?

[Ryan Hamilton, 2011-02-16 22:46:44]

On 2011/02/16 17:44:40, Ryan Hamilton wrote:
> Hi Wan-Teh,
> 
> Thanks for the feedback!  I'm going to get started on the refactor I mention
in
> this comment, but I want to send you my idea before I finish it in case you
> conclude it's a terrible idea :>

Ok, I've whipped up a patch that uses the approach I outlined, and it has passed
the try bots.  (I think it also should allow us to close bug 43461).  Feel free
to tell me this is a terrible idea. :>

[Ryan Hamilton, 2011-02-22 19:29:44]

I believe I have addressed all the comments.

[wtc, 2011-02-23 00:38:38]

rch: thank you for making all the suggested changes.  I
reviewed the latest patch set 4.  I have some suggested
changes below.

I can summarize my suggestions as follows.

1. Optional and simple: check in the three
ssl_client_auth_cache* files first.

2. Use a global boolean flag g_use_system_ssl, instead of
the function pointer g_ssl_factory, to implement
DefaultClientSocketFactory::CreateSSLClientSocket.
Note: this part of your CL can also be checked in
independently.

3. Use an Observer interface for CertDatabase::AddUserCert,
and have SSLClientAuthCache, DefaultClientSocketFactory,
SpdySessionPool, etc. register to be observers.

Details comments follow.

http://codereview.chromium.org/6487012/diff/30051/chrome/browser/browser_main...
File chrome/browser/browser_main_win.cc (right):

http://codereview.chromium.org/6487012/diff/30051/chrome/browser/browser_main...
chrome/browser/browser_main_win.cc:273: // The default client socket factory
uses NSS for SSL by default on Win.
Nit: Win => Windows.

http://codereview.chromium.org/6487012/diff/30051/content/browser/renderer_ho...
File content/browser/renderer_host/x509_user_cert_resource_handler.cc (right):

http://codereview.chromium.org/6487012/diff/30051/content/browser/renderer_ho...
content/browser/renderer_host/x509_user_cert_resource_handler.cc:118:
session->ResetSSLState();
I think it's better to call session->ResetSSLState() (or
notify the CertDatabase observers as I suggest below) from
CertDatabase::AddUserCert().

The new SSLAddCertHandler call on line 111 above merely
starts the process to add the new cert, but it may fail.

http://codereview.chromium.org/6487012/diff/30051/net/base/ssl_client_auth_ca...
File net/base/ssl_client_auth_cache_unittest.cc (right):

http://codereview.chromium.org/6487012/diff/30051/net/base/ssl_client_auth_ca...
net/base/ssl_client_auth_cache_unittest.cc:155: scoped_refptr<X509Certificate>
cached_cert(cert1);
Nit: cached_cert should not be initialized to cert1.
See lines 31 and 96 above.

http://codereview.chromium.org/6487012/diff/30051/net/http/http_network_sessi...
File net/http/http_network_session.cc (right):

http://codereview.chromium.org/6487012/diff/30051/net/http/http_network_sessi...
net/http/http_network_session.cc:74: spdy_session_pool()->CloseAllSessions();
Should we imitate NetworkChangeNotifier and add a
CertDatabase::Observer interface, and make these four classes
observers of CertDatabase changes?  This is more in keeping
with the C++ coding style in the Chromium source tree.  See
how the SpdySessionPool class implements the
NetworkChangeNotifier::Observer and
SSLConfigService::Observer interfaces.

Note: SpdySessionPool calls CloseCurrentSessions() in
those two observer methods.  So perhaps it's enough to
call CloseCurrentSessions() here, too.

http://codereview.chromium.org/6487012/diff/30051/net/net.gyp
File net/net.gyp (right):

http://codereview.chromium.org/6487012/diff/30051/net/net.gyp#newcode822
net/net.gyp:822: 'socket/ssl_client_socket_win_factory.h',
Nit: list the _win_factory.* files after the
_nss_factory.* files.

http://codereview.chromium.org/6487012/diff/30051/net/socket/client_socket_fa...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6487012/diff/30051/net/socket/client_socket_fa...
net/socket/client_socket_factory.cc:103: void
ClientSocketFactory::UseSystemSSL() {
This function can just set a global boolean flag:
  bool g_use_system_ssl;

and DefaultClientSocketFactory::CreateSSLClientSocket
can do either new SSLClientSocketNSS or
new SSLClientSocket{Mac,Win} depending on the value of
g_use_system_ssl.

IMPORTANT: the reason I had to use g_ssl_factory for
Windows was that I had doubly-defined symbol linker errors
when linking some ChromeFrame test programs because
those test programs were also linked with NSPR (a dependency
of NSS).  It's possible that this build problem has been
fixed, so we can just use the simpler g_use_system_ssl
method.

Please watch out for ChromeFrame test program build failures
when you check in this CL.

http://codereview.chromium.org/6487012/diff/30051/net/socket/client_socket_fa...
net/socket/client_socket_factory.cc:107: g_ssl_factory =
net::SSLClientSocketMacFactory;
Nit: remove the net:: prefix because this file is in the
'net' namespace.

http://codereview.chromium.org/6487012/diff/30051/net/socket/client_socket_fa...
File net/socket/client_socket_factory.h (right):

http://codereview.chromium.org/6487012/diff/30051/net/socket/client_socket_fa...
net/socket/client_socket_factory.h:69: static void UseSystemSSL();
With this change, the SSLClientSocketFactory type is no longer
used by this header, so SSLClientSocketFactory can probably
be defined elsewhere.

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

http://codereview.chromium.org/6487012/diff/30051/net/socket/ssl_client_socke...
net/socket/ssl_client_socket.h:108: static void UseSystemClearSSLSessionCache();
These two static methods should be combined into one.

The ClearSSLSessionCache() method should test the
g_use_system_ssl boolean flag.  Alternatively,
ClearSSLSessionCache() should be a non-static member of the
ClientSocketFactory class.  I believe this is your original
design.  I'm sorry to have made you change that.

[wtc, 2011-02-23 01:17:34]

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
File net/socket/client_socket_factory.cc (right):

http://codereview.chromium.org/6487012/diff/14001/net/socket/client_socket_fa...
net/socket/client_socket_factory.cc:115: return
SSLClientSocketNSS::ClearSessionCache();
On 2011/02/16 17:44:40, Ryan Hamilton wrote:
> Out of curiosity, under what
> circumstances to we use openssl?  Only if explicitly compiled to use it?

Yes.  The USE_OPENSSL build option is intended for the
Android browser.

[soichih_gmail.com, 2014-07-01 16:56:41]

Is there anyway to expose this SSL reset functionality via some button the 
Chrome UI?  I have several certificates installed on my browser, and in 
order to switch between different certificate (on a same domain) I have to 
close all instances of Chrome (including one under system tray) every time. 
If we could have a button to click to get SSL session reset, that will be 
quite handy.

Thanks!
Soichi Hayashi

On Wednesday, February 16, 2011 5:46:44 PM UTC-5, r...@chromium.org wrote:
>
> On 2011/02/16 17:44:40, Ryan Hamilton wrote:
> > Hi Wan-Teh,
>
> > Thanks for the feedback!  I'm going to get started on the refactor I  
> > mention
> in
> > this comment, but I want to send you my idea before I finish it in case  
> > you
> > conclude it's a terrible idea :>
>
> Ok, I've whipped up a patch that uses the approach I outlined, and it has  
> passed
> the try bots.  (I think it also should allow us to close bug 43461).  Feel 
>  
> free
> to tell me this is a terrible idea. :>
>
> http://codereview.chromium.org/6487012/
>
>

To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.