Move client certificates retrieval logic out of the SSL sockets.

Move client certificates retrieval logic out of the SSL sockets.

CL 11879048 introduces ClientCertStore API providing client certificate
lookup/filtering logic currently being done at the SSL socket level. This patch
removes this logic from the sockets, plugging the new API in the upper layers instead.

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

[ppi, 2013-01-28 14:08:49]

I have verified that client authentication / client certificate selection works
as expected on Linux with patch set 1 (see run-test-server.sh; thanks for the
hint, Ryan!).

Given that we do have unittests for ClientCertStoreImpl itself, I'd like to
think that this should also work for other platforms.

Could you guys have a look?

[ppi, 2013-01-29 12:44:09]

I will also need a review from content/browser OWNER - could you please have a
look, sky@?

[sky, 2013-01-29 16:34:22]

LGTM

[Ryan Sleevi, 2013-01-29 19:59:49]

You need to write "real" tests, using TestServer, to cover this (in both the
before and after scenarios).

https://codereview.chromium.org/12035105/diff/8002/content/browser/loader/res...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12035105/diff/8002/content/browser/loader/res...
content/browser/loader/resource_loader.cc:275: scoped_ptr<net::ClientCertStore>
store(new net::ClientCertStoreImpl());
Rather than creating a new object every time, you should create it when the
ResourceLoader is created.

I expect that in the future, we'll do some of the following changes:
1) Make it asynchronous
2) When #1 happens, we'll want to make it pooled (based on the CAs requested),
so that we're not spinning up a ton of workers all to request the same client
certs (eg: when using an SSL proxy, a very common scenario)
3) To potentially allow the ClientCertStore to cache results, only flushing when
the OS store changes (which we can get notifications for).
4) If/when we dep-inject

Having it be a member variable sets it up for the transition of
ownership/lifetime in the long-term, and in the near-term it just ensures that
no wrong assumptions get baked in on the lifetime of |store|

https://codereview.chromium.org/12035105/diff/8002/net/data/ssl/scripts/clien...
File net/data/ssl/scripts/client_authentication/run-test-server.sh (right):

https://codereview.chromium.org/12035105/diff/8002/net/data/ssl/scripts/clien...
net/data/ssl/scripts/client_authentication/run-test-server.sh:12: -CAfile
out/root_1.pem
For checking in, this should be a 'real' test using TestServer. Simply having
this script has no guarantees that it won't get broken in the future, so I'm not
sure it helps anyone.

https://codereview.chromium.org/12035105/diff/8002/net/socket/socket_test_uti...
File net/socket/socket_test_util.cc (left):

https://codereview.chromium.org/12035105/diff/8002/net/socket/socket_test_uti...
net/socket/socket_test_util.cc:1230: }
BUG: This change is not correct:
1) You should match what the socket's are doing, which is to Reset() the request
info
2) You should be providing the authorities (as part of a pass-thru), so that we
can properly mock SSL client auth without requiring a real socket.

[This was originally added back in http://crrev.com/71071 . It looks like that
particular test added was removed when we removed false start, but I want to
make sure this change isn't breaking the test bits. Of course, we may just
remove the test bits in a follow-up]

[ppi, 2013-01-30 15:34:56]

Thanks for the remarks, Ryan!

I have addressed most of your comments in patch set 4, although I am unsure
about what kind of testing you have in mind.

My understanding is that the actual change being done here is filling the
SSLCertRequestInfo::client_certs field in
ResourceLoader::OnCertificateRequest(). I imagine that we might be able to
unit-test that bit by mocking both the ClientCertStore (so that we don’t touch
the system store) and SSLClientAuthHandler - but is it worth the effort?

The actual code being tested would be just one line or two, and I understand
that we will be moving the ClientCertStore call from here soon. Moreover,
current design of ResourceLoader does not really allow us to plug-in a mock of
SSLClientAuthHandler for testing purposes. 

Also, if I see this correctly, such testing would not require TestServer - so
did you mean doing more comprehensive / cross-layers tests?

Could you please elaborate a bit on how would you like to see this patch set
tested?

https://codereview.chromium.org/12035105/diff/8002/content/browser/loader/res...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12035105/diff/8002/content/browser/loader/res...
content/browser/loader/resource_loader.cc:275: scoped_ptr<net::ClientCertStore>
store(new net::ClientCertStoreImpl());
On 2013/01/29 19:59:49, Ryan Sleevi wrote:
> Rather than creating a new object every time, you should create it when the
> ResourceLoader is created.
> 
> I expect that in the future, we'll do some of the following changes:
> 1) Make it asynchronous
> 2) When #1 happens, we'll want to make it pooled (based on the CAs requested),
> so that we're not spinning up a ton of workers all to request the same client
> certs (eg: when using an SSL proxy, a very common scenario)
> 3) To potentially allow the ClientCertStore to cache results, only flushing
when
> the OS store changes (which we can get notifications for).
> 4) If/when we dep-inject
> 
> Having it be a member variable sets it up for the transition of
> ownership/lifetime in the long-term, and in the near-term it just ensures that
> no wrong assumptions get baked in on the lifetime of |store|

Sounds good, thanks! Made the pointer a member variable initialized in
ResourceLoader constructor in patch set 4.

https://codereview.chromium.org/12035105/diff/8002/net/data/ssl/scripts/clien...
File net/data/ssl/scripts/client_authentication/run-test-server.sh (right):

https://codereview.chromium.org/12035105/diff/8002/net/data/ssl/scripts/clien...
net/data/ssl/scripts/client_authentication/run-test-server.sh:12: -CAfile
out/root_1.pem
On 2013/01/29 19:59:49, Ryan Sleevi wrote:
> For checking in, this should be a 'real' test using TestServer. Simply having
> this script has no guarantees that it won't get broken in the future, so I'm
not
> sure it helps anyone.

This is just a tiny helper that I thought could be helpful to anyone willing to
manually test client authentication in Chrome (certainly such script would be
helpful to me if we had it before).

That being said, I agree that it is at least optional if not redundant - please
let me know if you would like it out of the patch set.

https://codereview.chromium.org/12035105/diff/8002/net/socket/socket_test_uti...
File net/socket/socket_test_util.cc (left):

https://codereview.chromium.org/12035105/diff/8002/net/socket/socket_test_uti...
net/socket/socket_test_util.cc:1230: }
On 2013/01/29 19:59:49, Ryan Sleevi wrote:
> BUG: This change is not correct:
> 1) You should match what the socket's are doing, which is to Reset() the
request
> info
> 2) You should be providing the authorities (as part of a pass-thru), so that
we
> can properly mock SSL client auth without requiring a real socket.
> 
> [This was originally added back in http://crrev.com/71071 . It looks like that
> particular test added was removed when we removed false start, but I want to
> make sure this change isn't breaking the test bits. Of course, we may just
> remove the test bits in a follow-up]

I see, thanks. Reverted in patch set 4. We will probably revisit this code soon,
e.g. when we remove |client_certs| from SSLCertRequestInfo.

[Ryan Sleevi, 2013-01-30 19:54:51]

Well, what are you doing for manual tests? Let's automate that.

I would expect that a minimal "this patch works correctly" fix would be to

1) Spawn a net::TestServer instance configured to request certs from CA 1
2) Attempt a load through the ResourceLoader for that resource (guessing as a
content_unittest?)
3) Have a stub/mock ClientCertStore that checks that it receives a request for
CA 1 (eg: testing the glue between the net:: and content:: layer) and sends some
dummy certs (eg: no need to be real client certs)
4) Have a stub/mock content client that checks that it receives the SSL cert
request for that load, and that it matches the dummy cert from the
ClientCertStore

Another test might include testing that if the (fake) ClientCertStore says "No
certs available", that the ContentClient is not asked for a cert.

This doesn't require actually performing any client auth.

Depending on how easy it is to mock the sockets from content/ (I don't think
it's that easy, hence why I suggest TestServer), then in an 'ideal' world we
could just have the socket 'lie' about the request and not even need to start
the test server. We might be able to use fake URLRequests, which would be an
even better layer to test at (leave net/ to test the socket -> URLRequest
bridge), but it's been a while since I looked at these tests.

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
content/browser/loader/resource_loader.cc:70: #if defined(USE_NSS) &&
!defined(OS_IOS)
This is the wrong use of the define. USE_NSS == "Use NSS for certificate
validation", not "Use NSS for crypto"

Pretty sure this conditional should be:
#if !defined(USE_OPENSSL)

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
content/browser/loader/resource_loader.cc:285: }
Shouldn't the #endif cover up to here? because client_certs.empty() will always
be true for OpenSSL.

[ppi, 2013-02-01 22:22:34]

Thanks for the hints, Ryan!

I have addressed your remarks and added an unittest in patch set 6.

In the unittest the OnCertificateRequested() method of the URLRequest delegate
interface is triggered directly by the test body, rather than by a URLRequest;
so the socket layer is bypassed and there is no need to run TestServer.

I check both if the client cert store is being correctly queried and if the
returned certs are being correctly passed to the content browser client.

I tried testing the second scenario that you have suggested (client certs store
returns no certs), but without immediate success - in the current setup there is
no URLRequestJob, so actually going to ContinueWithCertificate() causes a crash.
I intend to revisit this next week.

In the meantime - could you please have another look?

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
File content/browser/loader/resource_loader.cc (right):

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
content/browser/loader/resource_loader.cc:70: #if defined(USE_NSS) &&
!defined(OS_IOS)
On 2013/01/30 19:54:51, Ryan Sleevi wrote:
> This is the wrong use of the define. USE_NSS == "Use NSS for certificate
> validation", not "Use NSS for crypto"
> 
> Pretty sure this conditional should be:
> #if !defined(USE_OPENSSL)

Thanks! Fixed in patch set 6.

https://codereview.chromium.org/12035105/diff/22003/content/browser/loader/re...
content/browser/loader/resource_loader.cc:285: }
On 2013/01/30 19:54:51, Ryan Sleevi wrote:
> Shouldn't the #endif cover up to here? because client_certs.empty() will
always
> be true for OpenSSL.

Definitely, thanks! Fixed in patch set 6.

[Ryan Sleevi, 2013-02-01 22:48:37]

I'm a little uncertain about why you need the mutative methods (eg:
set_client_certs) rather than the more common 'create an immutable helper',
since it seems like the latter would certainly fix here.

More comments below - not sure what's up with your extra use of scoped_ptr<>s in
the test (perhaps you meant to use PassAs() ?)

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader.h:29: class CONTENT_EXPORT ResourceLoader
: public net::URLRequest::Delegate,
This is incorrect, in that ResourceLoader should *not* be exported as part of
the content module.

Perhaps you meant CONTENT_EXPORT_PRIVATE, which allows it to be used in
content_unittests?

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader.h:62: scoped_ptr<net::ClientCertStore>&
store);
style:

1) It is not accepted by Google C++ Style or Chromium C++ style to pass
non-const reference arguments:

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...

2) Neither is it necessary, because of the calling conventions explained on
http://www.chromium.org/developers/smart-pointer-guidelines , scoped_ptr<> has a
C++11 move emulation, that lets you just have

swap_client_cert_store_for_testing(scoped_ptr<net::ClientCertStore> store)  //
Note how it's passed by-val, not by-ref

You then can use .Pass() to pass an existing scoped_ptr in to that method

3) As per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
, this should minimally be

SwapClientCertStoreForTesting() (note the CamelCaps here)

4) Since your test creates its own ResourceLoader, this may be better as a
private ctor that takes a net::ClientCertStore as the fourth argument. This is
also consistent with the earlier remarks about wanting to shift to a D-I long
term.

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:248:
GetContentClient()->set_browser_for_testing(old_client);
I don't understand why this is necessary.

[ppi, 2013-02-04 19:35:54]

Thanks for the remarks, Ryan!

I have addressed your comments in patch set 7 and in the comments below.

I have dropped the mutable "response" state from ClientCertStoreStub and got rid
of redundant scoped_ptrs - thanks!

Could you please have another look?

PS I needed to export two further classes that ResourceLoader inherits from to
make the code compile on Windows.

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
File content/browser/loader/resource_loader.h (right):

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader.h:29: class CONTENT_EXPORT ResourceLoader
: public net::URLRequest::Delegate,
On 2013/02/01 22:48:37, Ryan Sleevi wrote:
> This is incorrect, in that ResourceLoader should *not* be exported as part of
> the content module.
> 
> Perhaps you meant CONTENT_EXPORT_PRIVATE, which allows it to be used in
> content_unittests?

I would have meant that if we had CONTENT_EXPORT_PRIVATE. This was attempted to
be introduced here: https://chromiumcodereview.appspot.com/10735028/ , but
eventually wasn't.

I do see a readability gain in explicit marking of the parts exported only for
the sake of the unittests, although I am not sure if I should introduce
CONTENT_EXPORT_PRIVATE in this CL. Please let me know if you think I should do
so.

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader.h:62: scoped_ptr<net::ClientCertStore>&
store);
On 2013/02/01 22:48:37, Ryan Sleevi wrote:
> style:
> 
> 1) It is not accepted by Google C++ Style or Chromium C++ style to pass
> non-const reference arguments:
> 
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Refere...
> 
> 2) Neither is it necessary, because of the calling conventions explained on
> http://www.chromium.org/developers/smart-pointer-guidelines , scoped_ptr<> has
a
> C++11 move emulation, that lets you just have
> 
> swap_client_cert_store_for_testing(scoped_ptr<net::ClientCertStore> store)  //
> Note how it's passed by-val, not by-ref
> 
> You then can use .Pass() to pass an existing scoped_ptr in to that method
> 
> 3) As per
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
> , this should minimally be
> 
> SwapClientCertStoreForTesting() (note the CamelCaps here)
> 
> 4) Since your test creates its own ResourceLoader, this may be better as a
> private ctor that takes a net::ClientCertStore as the fourth argument. This is
> also consistent with the earlier remarks about wanting to shift to a D-I long
> term.

Thanks! In patch set 7 I have moved to using a private constructor. Ad. 2) - I
was aiming at "swap", rather than "set" behavior. If I see this correctly, that
wouldn't be possible with pass-by-value (althought I am not arguing that it was
a good idea).

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/12035105/diff/11030/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:248:
GetContentClient()->set_browser_for_testing(old_client);
On 2013/02/01 22:48:37, Ryan Sleevi wrote:
> I don't understand why this is necessary.

This is being done consistently in existing content unittests every time
set_browser_for_testing() is employed. My understanding is that the content
browser client is shared between unittests and we want to leave it set to the
original instance (and if the test crashes midway then we do not execute further
content tests anyway).

Please let me know if this doesn't seem right.

[ppi, 2013-02-04 19:41:56]

As I am adding a new test file to content/content_tests.gypi, I will also need a
lgtm from a content OWNER - could you please have a quick look, Brett?

[Ryan Sleevi, 2013-02-04 23:08:33]

Mostly LGTM, a few nits below. Certainly, you'll want to make sure a content/
owner has reviewed this and paid attention to the API, and is happy with it. I
don't want to step on their modularization boundary there, and #ifdefs in
content/ do feel a little like that. They may have a better solution for the
long-term.

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
nit: date

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:191: #if
!defined(USE_OPENSSL)
Rather than using the #ifdef here, can you not just exclude the whole test when
use_openssl is not in the GYP file? It would at least make this test
cleaner/clearer (subjectively)

https://codereview.chromium.org/12035105/diff/37005/content/browser/ssl/ssl_e...
File content/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/12035105/diff/37005/content/browser/ssl/ssl_e...
content/browser/ssl/ssl_error_handler.h:50: class CONTENT_EXPORT Delegate {
Why is this necessary? Shouldn't this be CONTENT_EXPORT_PRIVATE, if for testing?

https://codereview.chromium.org/12035105/diff/37005/content/public/browser/re...
File content/public/browser/resource_controller.h (right):

https://codereview.chromium.org/12035105/diff/37005/content/public/browser/re...
content/public/browser/resource_controller.h:17: class CONTENT_EXPORT
ResourceController {
nit: CONTENT_EXPORT_PRIVATE

[ppi, 2013-02-05 17:05:30]

Thanks for the remarks, Ryan!

I have fixed the date in the new unittest file in patch set 8, and I address
your other comments below.

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
File content/browser/loader/resource_loader_unittest.cc (right):

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:1: // Copyright (c) 2012 The
Chromium Authors. All rights reserved.
On 2013/02/04 23:08:33, Ryan Sleevi wrote:
> nit: date

Thanks, fixed in patch set 8.

https://codereview.chromium.org/12035105/diff/37005/content/browser/loader/re...
content/browser/loader/resource_loader_unittest.cc:191: #if
!defined(USE_OPENSSL)
On 2013/02/04 23:08:33, Ryan Sleevi wrote:
> Rather than using the #ifdef here, can you not just exclude the whole test
when
> use_openssl is not in the GYP file? It would at least make this test
> cleaner/clearer (subjectively)

On the other hand, excluding "resource_loader_unittest.cc" in GYP file for
"use_openssl=1" might be misleading for anyone reading the GYP file, as
ResourceLoader is certainly being used regardless of the openssl usage.

Personally I feel that it is easier to understand the reasons for exclusion of
this test with current ifdef at the source level (also, that matches ifdefs in
the ResourceLoader code).

What do you think?

https://codereview.chromium.org/12035105/diff/37005/content/browser/ssl/ssl_e...
File content/browser/ssl/ssl_error_handler.h (right):

https://codereview.chromium.org/12035105/diff/37005/content/browser/ssl/ssl_e...
content/browser/ssl/ssl_error_handler.h:50: class CONTENT_EXPORT Delegate {
On 2013/02/04 23:08:33, Ryan Sleevi wrote:
> Why is this necessary? Shouldn't this be CONTENT_EXPORT_PRIVATE, if for
testing?

This is a part of ancestor hierarchy for ResourceLoader. We need to export all
ancestors or suppress the warnings to get the code compiling (at least on
Windows).

We do not have a CONTENT_EXPORT_PRIVATE macro. The assumption seems to be that
everything that is exported, but is not in content/public is exported for
testing. See also the discussion here:
https://chromiumcodereview.appspot.com/10735028/ .

Introducing CONTENT_EXPORT_PRIVATE seems like a good idea to me, but I am not
sure that it should be done in this cl.

https://codereview.chromium.org/12035105/diff/37005/content/public/browser/re...
File content/public/browser/resource_controller.h (right):

https://codereview.chromium.org/12035105/diff/37005/content/public/browser/re...
content/public/browser/resource_controller.h:17: class CONTENT_EXPORT
ResourceController {
On 2013/02/04 23:08:33, Ryan Sleevi wrote:
> nit: CONTENT_EXPORT_PRIVATE

/see previous comment/

[brettw, 2013-02-06 19:02:45]

content owners rubberstamp based on rsleevi's review

[Ryan Sleevi, 2013-02-06 19:06:58]

On 2013/02/06 19:02:45, brettw wrote:
> content owners rubberstamp based on rsleevi's review

brettw: Note, I left an open question for 'content/' owners in comment #11 , so
you may want to make sure you're happy with it and don't have a
cleaner/preferred solution.

[ppi, 2013-02-06 19:12:31]

On 2013/02/06 19:06:58, Ryan Sleevi wrote:
> On 2013/02/06 19:02:45, brettw wrote:
> > content owners rubberstamp based on rsleevi's review
> 
> brettw: Note, I left an open question for 'content/' owners in comment #11 ,
so
> you may want to make sure you're happy with it and don't have a
> cleaner/preferred solution.

Brett, please also note that to actually commit this, I need a lgtm.

[brettw, 2013-02-06 19:26:55]

John says we already do OPENSSL ifdefs in content so this should be OK. Thanks
for checking. LGTM.

[ppi, 2013-02-06 19:44:20]

On 2013/02/06 19:26:55, brettw wrote:
> John says we already do OPENSSL ifdefs in content so this should be OK. Thanks
> for checking. LGTM.

Thanks, Brett!

[commit-bot: I haz the power, 2013-02-06 19:47:08]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/12035105/50001

[commit-bot: I haz the power, 2013-02-06 19:47:15]

Presubmit check for 12035105-50001 failed and returned exit status 1.


Running presubmit commit checks ...

** Presubmit Warnings **
License must match:
.*? Copyright (\(c\) )?(2013|2012|2011|2010|2009|2008|2007|2006|2006-2008) The
Chromium Authors\. All rights reserved\.\n.*? Use of this source code is
governed by a BSD-style license that can be\n.*? found in the LICENSE file\.(?:
\*/)?\n
Found a bad license header in these files:
  net/data/ssl/scripts/client_authentication/run-test-server.sh

Presubmit checks took 2.1s to calculate.

[commit-bot: I haz the power, 2013-02-06 19:56:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/ppi@chromium.org/12035105/52003

[commit-bot: I haz the power, 2013-02-07 00:15:27]

Change committed as 181104