Added a PolicyCertVerifier that uses the trust anchors from the ONC policies.

Added a PolicyCertVerifier that uses the trust anchors from the ONC policies.

The MultiThreadedCertVerifier can optionally use a CertTrustAnchorProvider to
get a list of additional certificates to trust, without importing them into the
NSS database. This CL wraps the MultiThreadedCertVerifier with a custom verifier
that includes a trust anchor provider.

The trust anchor provider returns all the certificates from the user ONC policy
that have the Web trust flag.  The PolicyCertVerifier also writes a preference
in the Profile once any such certificate is used.

This feature is currently behind a flag, until a warning UI is implemented.
The warning should be displayed if UsedPolicyCertificates() is true for the
given profile.

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

[Joao da Silva, 2013-03-24 15:12:21]

This CL introduces a PolicyCertVerifier that is only used for Profiles in
ChromeOS, and is used to use the ONC policy certificates and to mark the Profile
if such a certificate is ever used.

@Philipp: please review the whole thing :-)

@Ryan: please have a look at the PolicyCertVerifier, which wraps a
MultiThreadedCertVerifier. This is where the
"is_issued_by_additional_trust_anchor  flag is used. The unittest for that class
had an issue with repeated tests failing, because ScopedTestNSSDB seems to keep
the imported certificate across test runs; maybe you have a better idea to work
around that? For now I've just linked that with browser_tests, to run each test
in a separate process.

(in case you wonder: this is not necessarily for M27)

Thanks!

[Ryan Sleevi, 2013-03-25 21:09:53]

Initial phase of comments.

Big issues with the "web trust" naming - suggestions below.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:1733: net::CertificateList*
web_trust_certificates) = 0;
The name "web_trust_certificates" is going to be very confusing, because there
is an organization called "WebTrust" that is part of the auditing scheme of
public CAs.

policy_trusted_certificates ?
policy_trust_anchors ?
onc_trusted_certificates ?
onc_trust_anchors ?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1156: // Web trust is
only granted to certificates imported by the user.
Same comments about "web trust"

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:30: class
NetworkConfigurationUpdater::CertTrustAnchorProviderImpl
*mostly* a nit, but I'm inclined to suggest you just make this an
implementation-local thing (eg: don't make it a private class of NCU)

namespace {

class CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {

};

}

Then just use a cast the one place you need to - line 166.

Within the class, you'd just use a CertTrustAnchorProvider* pointer.

[Mostly, this is an aesthetic nit against "Impl" and in favour of better naming]

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:42: void
SetTrustAnchors(scoped_ptr<net::CertificateList> trust_anchors) {
Why are you passing a pointer? Just use a
"const net::CertificateList& trust_anchors"

What's the concern here?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:106:
cert_trust_provider_ = new CertTrustAnchorProviderImpl();
Why the lazy-load?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:168:
base::Passed(&web_trust_certs)));
In the larger context, this highlights an interesting part of your design:
You're currently using a cache key based on the additional trust anchors (which
requires constantly re-calculating hashes for fingerprints).

If you instead adopted a design where the entire cache was explicitly flushed
whenever a new CTAP was set on a CertVerifier, you'd avoid the per-verification
overhead involved - thus amortizing the cost.

If you go the wrapping of a CertVerifier, then you can easily know whenever the
CTAP has changed, and thus flush the delegate CertVerifier.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:148:
ASSERT_EQ(1u, cert_list.size());
Use
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/cert_test...

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:161: //
Initially web trust is disabled.
nit: same comments re: "web trust"

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:34:
base::Bind(&TaintProfile, profile));
I have no clue if this is (thread) safe, but it surprises me if it is.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:46: callback.Run(error);
nit: naming could be better
nit: Isn't an error to supply a null callback to a CertVerifier?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:57: new
net::MultiThreadedCertVerifier(net::CertVerifyProc::CreateDefault());
Because you added the bool to the SSLInfo, I think it's fine to have the
CertVerifier/CertVerifyProc handle this (eg: you do *NOT* need to sublcass a new
CertVerifier).

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:51: // is
fixed.
The TODO has been fixed since NSS 3.13.

Since this only works 3.14.2+, it should be fine. If the code is commented out
in ScopedTestNSSDB, then we should move that to a version check.

[Ryan Sleevi, 2013-03-25 23:55:31]

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:57: new
net::MultiThreadedCertVerifier(net::CertVerifyProc::CreateDefault());
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> Because you added the bool to the SSLInfo, I think it's fine to have the
> CertVerifier/CertVerifyProc handle this (eg: you do *NOT* need to sublcass a
new
> CertVerifier).

I'm an idiot who hadn't had his coffee. Ignore this remark.

[pneubeck (no reviews), 2013-03-26 10:01:25]

Reviewed the network/ONC related part.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:36: //
CertTrustAnchorProvider:
in the header the syntax
  Cert..Provider overrides.
is used

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:62:
cert_trust_provider_(NULL) {
Just create it here.
Makes the code below a bit simpler.
Performance diff is insignificant. We will always create it with the first
ApplyNetw...Configurations() call.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:86: if (!posted)
is that an error/problematic case? if so should we log a message?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:142:
ASSERT_TRUE(file_util::ReadFileToString(cert_path, &cert_data));
is there a way to create fake certs?
how about that constructor:

https://cs.corp.google.com/#chrome/src/net/base/x509_certificate.h&l=139&ct=x...

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:171:
updater.OnUserPolicyInitialized();
this call shouldn't be used a second time.
the implementation might change to ignore the second call.

either use the old trigger or split the test into two?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:274:
connector->GetNetworkConfigurationUpdater()->GetCertTrustAnchorProvider();
seeing these many levels of nesting, should we better add a
  GetCertTrustAnchorProvider()
function to the browser_policy_connector, too?
That would break the dependency from here to the NetConfUpd

https://codereview.chromium.org/13035003/diff/4001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1529: const char kEnableWebTrustCerts[]        
  = "enable-web-trust-certs";
btw. terrible indentation :-)

https://codereview.chromium.org/13035003/diff/4001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:865: // Indicates that the Profile has made
navigations that used a certificate
should be "CA certificate"?

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:53: // requested the Web trust
flag, if not NULL.
NIT: "if not NULL" to the beginning of the sentence

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:61: bool
ParseAndStoreCertificate(
you can make this one private. it's never called from the outside.

[pneubeck (no reviews), 2013-03-26 11:04:50]

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.h:24: virtual int
Verify(net::X509Certificate* cert,
mention that |callback| may be null, in contrast to net::CertVerifier's
callback.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:101:
LoadCertificate("ok_cert.pem", net::SERVER_CERT);
again the same question: do you have an idea if we can replace these loads by
fake certs?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:111: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
int error = ...

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:120: //
Issuing the same request again hits the cache. This tests the synchronous
The cache is in the implementation of MultiThreadedCertVerifier? Then this tests
the MultiThreadedCertVerifier, is that really necessary?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:154:
net::NSSCertDatabase::TrustBits trust =
This verifies that ImportCACerts works? Isn't that already tested elsewhere? Or
do you only to ensure that this particular cert can be imported, but then
|failure_list| shouldn't be empty, should it?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:158: //
Verify() successfully verifies |cert| now.
remove "now" as it seems to refer to the previous test.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:165: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
int error =...

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:201: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
int error = ...

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:493: mutable
scoped_ptr<net::CertVerifier> cert_verifier_;
doesn't look like data anymore. Is this really the right place?

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:189: ASSERT_EQ(1u,
result_list_.size());
remove this here or in AddCertificateFromFile

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:207: ASSERT_EQ(1u,
result_list_.size());
ditto

[Joao da Silva, 2013-03-31 19:22:14]

Thanks for all the comments! Please have another look.

Also adding reviewers for owners checks:

@gspencer: please review chrome/browser/chromeos/cros changes
@mmenke: please review chrome/browser/profiles and net_internals changes
@jochen: please review gypi changes

Thanks!

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library.h:1733: net::CertificateList*
web_trust_certificates) = 0;
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> The name "web_trust_certificates" is going to be very confusing, because there
> is an organization called "WebTrust" that is part of the auditing scheme of
> public CAs.
> 
> policy_trusted_certificates ?
> policy_trust_anchors ?
> onc_trusted_certificates ?
> onc_trust_anchors ?

Good point. LoadOncNetworks() can also load onc from a local file (user import),
so the list doesn't come necessarily from policy; and the certificates can be
used for other purposes, so I've renamed the argument to
"onc_trusted_certificates".

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
File chrome/browser/chromeos/cros/network_library_impl_base.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/cr...
chrome/browser/chromeos/cros/network_library_impl_base.cc:1156: // Web trust is
only granted to certificates imported by the user.
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> Same comments about "web trust"

Renamed to "allow_trust_imports".

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:30: class
NetworkConfigurationUpdater::CertTrustAnchorProviderImpl
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> *mostly* a nit, but I'm inclined to suggest you just make this an
> implementation-local thing (eg: don't make it a private class of NCU)
> 
> namespace {
> 
> class CrosTrustAnchorProvider : public net::CertTrustAnchorProvider {
> 
> };
> 
> }
> 
> Then just use a cast the one place you need to - line 166.
> 
> Within the class, you'd just use a CertTrustAnchorProvider* pointer.
> 
> [Mostly, this is an aesthetic nit against "Impl" and in favour of better
naming]

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:36: //
CertTrustAnchorProvider:
On 2013/03/26 10:01:25, pneubeck wrote:
> in the header the syntax
>   Cert..Provider overrides.
> is used

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:42: void
SetTrustAnchors(scoped_ptr<net::CertificateList> trust_anchors) {
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> Why are you passing a pointer? Just use a
> "const net::CertificateList& trust_anchors"
> 
> What's the concern here?

The list is passed from the UI thread to the IO thread, so passing a pointer
avoids copying the vector.

It's a vector of scoped_refptrs so it's not that expensive, but it still
involves a lock/unlock per entry; I'd keep as is.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:62:
cert_trust_provider_(NULL) {
On 2013/03/26 10:01:25, pneubeck wrote:
> Just create it here.
> Makes the code below a bit simpler.
> Performance diff is insignificant. We will always create it with the first
> ApplyNetw...Configurations() call.

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:86: if (!posted)
On 2013/03/26 10:01:25, pneubeck wrote:
> is that an error/problematic case? if so should we log a message?

This occurs when shutdown happens very early, before the IO thread was started.
It's not a real issue, but the memory bots catch that (there's a test that
triggers an early shutdown). We had a similar issue with shutdown of the policy
providers' backends on the FILE thread.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:106:
cert_trust_provider_ = new CertTrustAnchorProviderImpl();
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> Why the lazy-load?

No good reason, removed.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:168:
base::Passed(&web_trust_certs)));
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> In the larger context, this highlights an interesting part of your design:
> You're currently using a cache key based on the additional trust anchors
(which
> requires constantly re-calculating hashes for fingerprints).
> 
> If you instead adopted a design where the entire cache was explicitly flushed
> whenever a new CTAP was set on a CertVerifier, you'd avoid the
per-verification
> overhead involved - thus amortizing the cost.
> 
> If you go the wrapping of a CertVerifier, then you can easily know whenever
the
> CTAP has changed, and thus flush the delegate CertVerifier.

SGTM; I'll leave that for another CL.

That requires a public ClearCache() call at the MTCV. It's also possible that a
Verify() sends a job to the worker thread and then a ClearCache() comes before
the result is ready; the cache is already cleared when the result comes back,
and then it's inserted into the cache but was computed using the trust anchors.

I'd solve that by storing the creation time in the CertVerifierWorker, passing
it to HandleResult, and then only store it in the cache if that creation time is
after the most recent ClearCache.

Another concern is the API of MTCV, which can now be misused if the trust
anchors provided change but ClearCache() isn't invoked. I suggest also caching
the list of trust anchors: if that list is updated at the
trust_anchors_provider, then ClearCache() must be called to flush the cache and
fetch the updated list. The previous problem can also be solved by caching the
fingerprints and copying them into each job; at HandleResult the cache is
updated only if the fingerprints from the job still match the cached
fingerprints.

WDYT?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:142:
ASSERT_TRUE(file_util::ReadFileToString(cert_path, &cert_data));
On 2013/03/26 10:01:25, pneubeck wrote:
> is there a way to create fake certs?
> how about that constructor:
> 
>
https://cs.corp.google.com/#chrome/src/net/base/x509_certificate.h&l=139&ct=x...

These certificate files are used in several unit tests (mostly in net/), and
it's less work; what's the advantage of generating a new cert each time the test
is executed?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:148:
ASSERT_EQ(1u, cert_list.size());
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> Use
>
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/cert_test...

Thanks for the pointer, done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:161: //
Initially web trust is disabled.
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> nit: same comments re: "web trust"

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:171:
updater.OnUserPolicyInitialized();
On 2013/03/26 10:01:25, pneubeck wrote:
> this call shouldn't be used a second time.
> the implementation might change to ignore the second call.
> 
> either use the old trigger or split the test into two?

Using the old trigger.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:34:
base::Bind(&TaintProfile, profile));
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> I have no clue if this is (thread) safe, but it surprises me if it is.

I'm not sure what it is that you're unsure about here; I guess it's the profile
pointer?

This is the same pattern that ProfileIOData uses;
ProfileManager::IsValidProfile() exists to support it. In theory the pointer
could point to a new Profile that just happened to be allocated at the same
address as the old Profile; in practice Profiles are never deleted before
shutdown. I'd like to have a better way to pass around a handle to a Profile to
different threads (maybe get a WeakPtr?), but this is what we have for now.

Or maybe you meant something else? :-)

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:46: callback.Run(error);
On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> nit: naming could be better

Renamed to |original_callback|.

> nit: Isn't an error to supply a null callback to a CertVerifier?

That's what's documented, but MultiThreadedCertVerifier supports it (see
CertVerifierRequest::Post). Since this wraps a MTCV I think it should support it
as well, even though the interface doesn't allow it.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.h:24: virtual int
Verify(net::X509Certificate* cert,
On 2013/03/26 11:04:50, pneubeck wrote:
> mention that |callback| may be null, in contrast to net::CertVerifier's
> callback.

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:101:
LoadCertificate("ok_cert.pem", net::SERVER_CERT);
On 2013/03/26 11:04:50, pneubeck wrote:
> again the same question: do you have an idea if we can replace these loads by
> fake certs?

We certainly can generate the certs at runtime, but why the additional work and
cost at runtime?

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:111: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
On 2013/03/26 11:04:50, pneubeck wrote:
> int error = ...

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:120: //
Issuing the same request again hits the cache. This tests the synchronous
On 2013/03/26 11:04:50, pneubeck wrote:
> The cache is in the implementation of MultiThreadedCertVerifier? Then this
tests
> the MultiThreadedCertVerifier, is that really necessary?

This relies on that behavior of the MultiThreadedCertVerifier, but tests a
different path in PolicyCertVerifier::Verify. Verify() won't used the wrapped
callback when the inner cert_verifier returns immediately (instead of returning
IO_PENDING).

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:154:
net::NSSCertDatabase::TrustBits trust =
On 2013/03/26 11:04:50, pneubeck wrote:
> This verifies that ImportCACerts works? Isn't that already tested elsewhere?
Or
> do you only to ensure that this particular cert can be imported, but then
> |failure_list| shouldn't be empty, should it?

This test is meant to verify that manual imports don't taint the Profile, so
that users that import certificates don't see the scary warning.

Even though this seems trivial now, a regression in the future could flag the
Profile by mistake; this also checks that the test setup used in the other 2
tests works as expected (that is, they aren't passing because some other
mechanism is making the verification pass). I usually write both positive and
negative tests, to make sure the test isn't passing due to another knob that I'm
not aware of.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:158: //
Verify() successfully verifies |cert| now.
On 2013/03/26 11:04:50, pneubeck wrote:
> remove "now" as it seems to refer to the previous test.

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:165: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
On 2013/03/26 11:04:50, pneubeck wrote:
> int error =...

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:201: error =
cert_verifier_->Verify(cert, "127.0.0.1", 0, NULL,
On 2013/03/26 11:04:50, pneubeck wrote:
> int error = ...

Done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.cc:274:
connector->GetNetworkConfigurationUpdater()->GetCertTrustAnchorProvider();
On 2013/03/26 10:01:25, pneubeck wrote:
> seeing these many levels of nesting, should we better add a
>   GetCertTrustAnchorProvider()
> function to the browser_policy_connector, too?
> That would break the dependency from here to the NetConfUpd

Good idea, done.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
File chrome/browser/profiles/profile_io_data.h (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/profiles/pr...
chrome/browser/profiles/profile_io_data.h:493: mutable
scoped_ptr<net::CertVerifier> cert_verifier_;
On 2013/03/26 11:04:50, pneubeck wrote:
> doesn't look like data anymore. Is this really the right place?

I'm not sure I understand the issue :-)

This class holds data related to the Profile, but on the IO thread (the Profile
lives on UI). Most of it is related to the net/ layer, such as the CertVerifier;
I think this is the most adequate place to hold this (also to make sure it's
cleaned up at shutdown *after* the Profile's request contexts). Do you have a
better idea?

The IOThread class is a similar class for IO data from the BrowserProcess;
that's where the system request context's cert_verifier is owned too.

https://codereview.chromium.org/13035003/diff/4001/chrome/common/chrome_switc...
File chrome/common/chrome_switches.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/common/chrome_switc...
chrome/common/chrome_switches.cc:1529: const char kEnableWebTrustCerts[]        
  = "enable-web-trust-certs";
On 2013/03/26 10:01:25, pneubeck wrote:
> btw. terrible indentation :-)

Following the file structure. Looks alright with syntax highlight :-)

https://codereview.chromium.org/13035003/diff/4001/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/common/pref_names.c...
chrome/common/pref_names.cc:865: // Indicates that the Profile has made
navigations that used a certificate
On 2013/03/26 10:01:25, pneubeck wrote:
> should be "CA certificate"?

Also works for server certificates. The administrator may override only
"example.com", and install a self-signed certificate for that server only via
ONC.

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer.h (right):

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:53: // requested the Web trust
flag, if not NULL.
On 2013/03/26 10:01:25, pneubeck wrote:
> NIT: "if not NULL" to the beginning of the sentence

Done.

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer.h:61: bool
ParseAndStoreCertificate(
On 2013/03/26 10:01:25, pneubeck wrote:
> you can make this one private. it's never called from the outside.

Done.

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
File chromeos/network/onc/onc_certificate_importer_unittest.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:189: ASSERT_EQ(1u,
result_list_.size());
On 2013/03/26 11:04:50, pneubeck wrote:
> remove this here or in AddCertificateFromFile

This must stay in both places, because the first element is indexed in the next
line. If |result_list| is empty then that line would crash the unit tests; the
assertion makes only this test fail.

A different ONC file is imported in each test too, so it's not guaranteed that
the result would be the same. Additionally, the order of execution of the tests
is not guaranteed either (the swarm bots run in completely random order, for
example).

https://codereview.chromium.org/13035003/diff/4001/chromeos/network/onc/onc_c...
chromeos/network/onc/onc_certificate_importer_unittest.cc:207: ASSERT_EQ(1u,
result_list_.size());
On 2013/03/26 11:04:50, pneubeck wrote:
> ditto

See above

[Greg Spencer (Chromium), 2013-04-01 16:03:41]

LGTM for chrome/browser/chromeos/cros changes.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.h (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:61: // This
getter must be used on UI, and the provider must be used on IO. It
"on UI" --> "on the UI thread"
(same with IO)

[pneubeck (no reviews), 2013-04-02 08:12:07]

LGTM

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:38: void
CallbackWrapper(void* profile,
to replace some of the void* by Profile*, could you do the reinterpret cast
already here? Or does it have to happen on the UI thread?

[jochen (gone - plz use gerrit), 2013-04-02 10:01:05]

*.gypi lgtm

[Ryan Sleevi, 2013-04-02 19:16:06]

lgtm

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/network_configuration_updater.cc:168:
base::Passed(&web_trust_certs)));
On 2013/03/31 19:22:14, Joao da Silva wrote:
> On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> > In the larger context, this highlights an interesting part of your design:
> > You're currently using a cache key based on the additional trust anchors
> (which
> > requires constantly re-calculating hashes for fingerprints).
> > 
> > If you instead adopted a design where the entire cache was explicitly
flushed
> > whenever a new CTAP was set on a CertVerifier, you'd avoid the
> per-verification
> > overhead involved - thus amortizing the cost.
> > 
> > If you go the wrapping of a CertVerifier, then you can easily know whenever
> the
> > CTAP has changed, and thus flush the delegate CertVerifier.
> 
> SGTM; I'll leave that for another CL.
> 
> That requires a public ClearCache() call at the MTCV. It's also possible that
a
> Verify() sends a job to the worker thread and then a ClearCache() comes before
> the result is ready; the cache is already cleared when the result comes back,
> and then it's inserted into the cache but was computed using the trust
anchors.
> 
> I'd solve that by storing the creation time in the CertVerifierWorker, passing
> it to HandleResult, and then only store it in the cache if that creation time
is
> after the most recent ClearCache.
> 
> Another concern is the API of MTCV, which can now be misused if the trust
> anchors provided change but ClearCache() isn't invoked. I suggest also caching
> the list of trust anchors: if that list is updated at the
> trust_anchors_provider, then ClearCache() must be called to flush the cache
and
> fetch the updated list. The previous problem can also be solved by caching the
> fingerprints and copying them into each job; at HandleResult the cache is
> updated only if the fingerprints from the job still match the cached
> fingerprints.
> 
> WDYT?

I'll have to think more about this one. We can address it in follow-up though.

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:34:
base::Bind(&TaintProfile, profile));
On 2013/03/31 19:22:14, Joao da Silva wrote:
> On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> > I have no clue if this is (thread) safe, but it surprises me if it is.
> 
> I'm not sure what it is that you're unsure about here; I guess it's the
profile
> pointer?
> 
> This is the same pattern that ProfileIOData uses;
> ProfileManager::IsValidProfile() exists to support it. In theory the pointer
> could point to a new Profile that just happened to be allocated at the same
> address as the old Profile; in practice Profiles are never deleted before
> shutdown. I'd like to have a better way to pass around a handle to a Profile
to
> different threads (maybe get a WeakPtr?), but this is what we have for now.
> 
> Or maybe you meant something else? :-)

It was concern about profile deletion, yes. If the profile was deleted on the UI
thread while the IO thread was running, and then it re-posts the (now dead)
profile to the UI thread for handling.

I've luckily stayed away from profiles, but it looks like IsValidProfile() does
indeed specifically handle this - as long as the pointer is not reused.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:22: void
TaintProfile(void* profile_ptr) {
nit: DCHECK for UI thread here? Even with the added bloat to debug binaries, I
tend to find the DCHECKs the best documentation.

Just my $.0000000000000000000000000002 (super tiny nit)

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.h:17: class
PolicyCertVerifier : public net::CertVerifier {
Comment describing this class please

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:213: 
Additional test case:

1) Verify fails w/o trust
2) Verify success w additional trust anchors
3) Profile is tainted
4) Change trust anchors
5) Verify fails now due to missing trust anchor
6) Profile is still tainted

[mmenke, 2013-04-02 19:24:19]

Rubberstamp LGTM on profiles/ and net_internals.

On 2013/04/02 19:16:06, Ryan Sleevi wrote:
> lgtm
> 
>
https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
> File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):
> 
>
https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
> chrome/browser/chromeos/policy/network_configuration_updater.cc:168:
> base::Passed(&web_trust_certs)));
> On 2013/03/31 19:22:14, Joao da Silva wrote:
> > On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> > > In the larger context, this highlights an interesting part of your design:
> > > You're currently using a cache key based on the additional trust anchors
> > (which
> > > requires constantly re-calculating hashes for fingerprints).
> > > 
> > > If you instead adopted a design where the entire cache was explicitly
> flushed
> > > whenever a new CTAP was set on a CertVerifier, you'd avoid the
> > per-verification
> > > overhead involved - thus amortizing the cost.
> > > 
> > > If you go the wrapping of a CertVerifier, then you can easily know
whenever
> > the
> > > CTAP has changed, and thus flush the delegate CertVerifier.
> > 
> > SGTM; I'll leave that for another CL.
> > 
> > That requires a public ClearCache() call at the MTCV. It's also possible
that
> a
> > Verify() sends a job to the worker thread and then a ClearCache() comes
before
> > the result is ready; the cache is already cleared when the result comes
back,
> > and then it's inserted into the cache but was computed using the trust
> anchors.
> > 
> > I'd solve that by storing the creation time in the CertVerifierWorker,
passing
> > it to HandleResult, and then only store it in the cache if that creation
time
> is
> > after the most recent ClearCache.
> > 
> > Another concern is the API of MTCV, which can now be misused if the trust
> > anchors provided change but ClearCache() isn't invoked. I suggest also
caching
> > the list of trust anchors: if that list is updated at the
> > trust_anchors_provider, then ClearCache() must be called to flush the cache
> and
> > fetch the updated list. The previous problem can also be solved by caching
the
> > fingerprints and copying them into each job; at HandleResult the cache is
> > updated only if the fingerprints from the job still match the cached
> > fingerprints.
> > 
> > WDYT?
> 
> I'll have to think more about this one. We can address it in follow-up though.
> 
>
https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
> File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):
> 
>
https://codereview.chromium.org/13035003/diff/4001/chrome/browser/chromeos/po...
> chrome/browser/chromeos/policy/policy_cert_verifier.cc:34:
> base::Bind(&TaintProfile, profile));
> On 2013/03/31 19:22:14, Joao da Silva wrote:
> > On 2013/03/25 21:09:53, Ryan Sleevi wrote:
> > > I have no clue if this is (thread) safe, but it surprises me if it is.
> > 
> > I'm not sure what it is that you're unsure about here; I guess it's the
> profile
> > pointer?
> > 
> > This is the same pattern that ProfileIOData uses;
> > ProfileManager::IsValidProfile() exists to support it. In theory the pointer
> > could point to a new Profile that just happened to be allocated at the same
> > address as the old Profile; in practice Profiles are never deleted before
> > shutdown. I'd like to have a better way to pass around a handle to a Profile
> to
> > different threads (maybe get a WeakPtr?), but this is what we have for now.
> > 
> > Or maybe you meant something else? :-)
> 
> It was concern about profile deletion, yes. If the profile was deleted on the
UI
> thread while the IO thread was running, and then it re-posts the (now dead)
> profile to the UI thread for handling.
> 
> I've luckily stayed away from profiles, but it looks like IsValidProfile()
does
> indeed specifically handle this - as long as the pointer is not reused.
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/policy/policy_cert_verifier.cc:22: void
> TaintProfile(void* profile_ptr) {
> nit: DCHECK for UI thread here? Even with the added bloat to debug binaries, I
> tend to find the DCHECKs the best documentation.
> 
> Just my $.0000000000000000000000000002 (super tiny nit)
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/policy/policy_cert_verifier.h:17: class
> PolicyCertVerifier : public net::CertVerifier {
> Comment describing this class please
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc
(right):
> 
>
https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
> chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:213: 
> Additional test case:
> 
> 1) Verify fails w/o trust
> 2) Verify success w additional trust anchors
> 3) Profile is tainted
> 4) Change trust anchors
> 5) Verify fails now due to missing trust anchor
> 6) Profile is still tainted

[Joao da Silva, 2013-04-03 15:24:39]

Thanks for all the comments!

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/network_configuration_updater.h (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/network_configuration_updater.h:61: // This
getter must be used on UI, and the provider must be used on IO. It
On 2013/04/01 16:03:42, Greg Spencer (Chromium) wrote:
> "on UI" --> "on the UI thread"
> (same with IO)

Done.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier.cc (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:22: void
TaintProfile(void* profile_ptr) {
On 2013/04/02 19:16:07, Ryan Sleevi wrote:
> nit: DCHECK for UI thread here? Even with the added bloat to debug binaries, I
> tend to find the DCHECKs the best documentation.
> 
> Just my $.0000000000000000000000000002 (super tiny nit)

Done.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.cc:38: void
CallbackWrapper(void* profile,
On 2013/04/02 08:12:08, pneubeck wrote:
> to replace some of the void* by Profile*, could you do the reinterpret cast
> already here? Or does it have to happen on the UI thread?

It could be done here, but this is still the IO thread and it's not safe to
access the object outside the UI thread. I find it clearer to pass around a
void* on the IO thread, to make sure it's not accessed.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier.h (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier.h:17: class
PolicyCertVerifier : public net::CertVerifier {
On 2013/04/02 19:16:07, Ryan Sleevi wrote:
> Comment describing this class please

Done.

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
File chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc (right):

https://codereview.chromium.org/13035003/diff/26001/chrome/browser/chromeos/p...
chrome/browser/chromeos/policy/policy_cert_verifier_browsertest.cc:213: 
On 2013/04/02 19:16:07, Ryan Sleevi wrote:
> Additional test case:
> 
> 1) Verify fails w/o trust
> 2) Verify success w additional trust anchors
> 3) Profile is tainted
> 4) Change trust anchors
> 5) Verify fails now due to missing trust anchor
> 6) Profile is still tainted

Thanks for the suggestion. Done.

[commit-bot: I haz the power, 2013-04-03 15:24:56]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/joaodasilva@chromium.org/13035003/62001

[commit-bot: I haz the power, 2013-04-03 17:59:25]

Retried try job too often on linux_chromeos for step(s) browser_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_chro...

[Joao da Silva, 2013-04-03 18:19:03]

Committed patchset #5 manually as r192102 (presubmit successful).