[Cronet-iOS] Public-Key-Pinning Tests

[Cronet-iOS] Public-Key-Pinning Tests

1. Added PKP tests
2. Introduced CronetTestBase class that chares common code used in
   cronet HTTP & PKP tests.
3. Enabled ARC for Cronet tests.

BUG=670689
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

patch from issue 2895123003 at patchset 1 (http://crrev.com/2895123003#ps1)

Review-Url: https://codereview.chromium.org/2928653002
Cr-Commit-Position: refs/heads/master@{#480600}
Committed: https://chromium.googlesource.com/chromium/src/+/eac2ab04ef175b2225a961a55108b20254f36a69

[kapishnikov, 2017-06-06 20:36:32]

kapishnikov@chromium.org changed reviewers:
+ lilyhoughton@chromium.org, mef@chromium.org

[kapishnikov, 2017-06-06 20:36:33]

Here are Public-Key-Pinning tests. PTAL.

[lilyhoughton, 2017-06-08 16:34:43]

On 2017/06/06 20:36:33, kapishnikov wrote:
> Here are Public-Key-Pinning tests. PTAL.

This includes/supercedes the CL I made for PKP functionality at all, right?

[kapishnikov, 2017-06-08 17:04:58]

On 2017/06/08 16:34:43, lilyhoughton wrote:
> On 2017/06/06 20:36:33, kapishnikov wrote:
> > Here are Public-Key-Pinning tests. PTAL.
> 
> This includes/supercedes the CL I made for PKP functionality at all, right?

Yes, this CL includes your CL with some slight modifications.

[lilyhoughton, 2017-06-09 16:51:57]

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:76: // <p>
Can the html be replaced with something more like markdown?  Whatever the
effects on doc formatting, it makes the source code itself slightly less
readable, I think.

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:97: // @param hostName name of the host to which
the public keys should be pinned. A
We also don't have @params for any of the other functions - should we?

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:110: //                          subdomains of
{@code hostName}.
is {@code hostName} different from |hostName|?

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:108: void
add_public_key_pins(std::unique_ptr<URLRequestContextConfig::Pkp> pkp) {
does not seem to be used anywhere currently

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/t...
File components/cronet/ios/test/BUILD.gn (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/t...
components/cronet/ios/test/BUILD.gn:37: configs += [
"//build/config/compiler:enable_arc" ]
where does this come from?

[lilyhoughton, 2017-06-09 17:07:52]

A couple more nits

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:65: void
sendRequestAndAssertSuccess(NSURL* url) {
nit: I don't think these are much clearer than using sendRequestAndAssertResult
directly.

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:82: if (expected_success) {
nit: why not just something like |ASSERT_EQ(IsResponseSuccessful(),
expected_success)|?

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:133:
ASSERT_NO_FATAL_FAILURE(sendRequestAndAssertError(request_url_));
Is it possible to make the test more robust with respect to ensuring that this
fails with a mismatching pin error (as opposed to any other kind of error)?

[kapishnikov, 2017-06-09 21:27:02]

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:76: // <p>
On 2017/06/09 16:51:57, lilyhoughton wrote:
> Can the html be replaced with something more like markdown?  Whatever the
> effects on doc formatting, it makes the source code itself slightly less
> readable, I think.

Done.

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:97: // @param hostName name of the host to which
the public keys should be pinned. A
On 2017/06/09 16:51:57, lilyhoughton wrote:
> We also don't have @params for any of the other functions - should we?

I think we should.

The correct way of documenting a method is actually looks like this:
/**
 <#Description#>

 @param host <#host description#>
 @param pinHashes <#pinHashes description#>
 @param includeSubdomains <#includeSubdomains description#>
 @param expirationDate <#expirationDate description#>
 */

It is also an acceptable documentation style according to the google style
guide.

Using three slashes is also possible but it usually used for a single line
documentation.
/// <#Description#>
///
/// @param host <#host description#>
/// @param pinHashes <#pinHashes description#>
/// @param includeSubdomains <#includeSubdomains description#>
/// @param expirationDate <#expirationDate description#>

Double slashes that we currently use are not considered documentation comments.

If a method has a return value, it should be documented like this:
/**
 <#Description#>

 @param fileName <#fileName description#>
 @param logBytes <#logBytes description#>
 @return <#return value description#>
 */
+ (BOOL)startNetLogToFile:(NSString*)fileName logBytes:(BOOL)logBytes;

In order to check if a method is documented correctly, you can alt+click on it.

Maybe we should revisit the documentation of other methods in a separate CL.

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:110: //                          subdomains of
{@code hostName}.
On 2017/06/09 16:51:57, lilyhoughton wrote:
> is {@code hostName} different from |hostName|?

This is the result of copy-paste. Fixed.

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/c...
File components/cronet/ios/cronet_environment.h (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/c...
components/cronet/ios/cronet_environment.h:108: void
add_public_key_pins(std::unique_ptr<URLRequestContextConfig::Pkp> pkp) {
On 2017/06/09 16:51:57, lilyhoughton wrote:
> does not seem to be used anywhere currently

Removed.

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/t...
File components/cronet/ios/test/BUILD.gn (right):

https://codereview.chromium.org/2928653002/diff/60001/components/cronet/ios/t...
components/cronet/ios/test/BUILD.gn:37: configs += [
"//build/config/compiler:enable_arc" ]
On 2017/06/09 16:51:57, lilyhoughton wrote:
> where does this come from?

This line enables automatic reference counting.

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:65: void
sendRequestAndAssertSuccess(NSURL* url) {
On 2017/06/09 17:07:52, lilyhoughton wrote:
> nit: I don't think these are much clearer than using
sendRequestAndAssertResult
> directly.

Done.

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:82: if (expected_success) {
On 2017/06/09 17:07:51, lilyhoughton wrote:
> nit: why not just something like |ASSERT_EQ(IsResponseSuccessful(),
> expected_success)|?

It doesn't show an error message if I change it to ASSERT_EQ. Note, that
IsResponseSuccessful() doesn't return boolean but testing::AssertionResult to
provide an error message.

https://codereview.chromium.org/2928653002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:133:
ASSERT_NO_FATAL_FAILURE(sendRequestAndAssertError(request_url_));
On 2017/06/09 17:07:51, lilyhoughton wrote:
> Is it possible to make the test more robust with respect to ensuring that this
> fails with a mismatching pin error (as opposed to any other kind of error)?

Unfortunately because of http://crbug.com/548378 we cannot. QUIC returns generic
ERR_QUIC_PROTOCOL_ERROR instead of ERR_SSL_PINNED_KEY_NOT_IN_CERT_CHAIN.

[mef, 2017-06-12 22:25:24]

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.h:76: /// Pins a set of public keys for a given
host. This method only has any effect
We should probably make other comments in this header documentable the same way.
Different CL though.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.h:93: /// @param hostName name of the host to which
the public keys should be pinned.
Need to match actual param names:

hostName <-> host
pinsSha356 <-> pinHashes

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:231: DCHECK_EQ(sizeof(hashValue.data),
hash.length)
I'm not sure whether DCHECK is good enough here.
What if app gets pins from the server and messes up the size?
Release version will not have any guards and may read uninitialized memory
beyond the end of hashValue.data.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:369: +
(void)setMockCertVerifier:(std::unique_ptr<net::CertVerifier>)certVerifier {
Should this also have ForTesting: suffix?
Passing std::unique_ptr as argument is interesting.
I guess its ok because this method is not in header files?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:349: // Iterate trhough PKP
configuration for every host.
nit: through (sp)

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:18: const bool kINCLUDE_SUBDOMAINS
= true;
I think constant names are kCamelCase.
Also, if those are ObjC, then I think they should be BOOL and YES?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:44: domain_ =
Maybe add a comment why we need to extract 'example.com' out of
'test.example.com'? OR just hardcode the domain name next to kServerCert?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:48: // Create a Cronet enabled
NSURLSession
nit: Comments should be full sentences ending with period. Here and below.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:61: [Cronet shutdownForTesting];
It is strange that we have an explicit shutdown here but not start up in SetUp.
I see why, but maybe add a comment?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:79: // Adds a give public-key-pin
and starts a Cronet engine for testing.
nit: give -> given

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:96: u_char hash[length];
I don't think u_char is normally used in Chromium.
Can this be std::string(length, '\077').c_str() or somesuch?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:183: //
nit: spurious //

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_test_base.h (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:5: #ifndef
PRODUCTS_CRONET_TEST_BASE_H
nit: COMPONENTS_CRONET_IOS_TEST_CRONET_TEST_BASE_H_

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:8: #include "Cronet/Cronet.h"
Should this be #import <Cronet/Cronet.h>?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:28: @property(strong, atomic)
dispatch_semaphore_t semaphore;
Maybe hide this in implementation and add a 'waitForDone' method?
Possibly add TODO() for another CL?

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:52: // The class automatically
starts and stoppes the test QUIC server.
nit: stops.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:55: static bool
CalculatePublicKeySha256(const net::X509Certificate& cert,
Do we need this in base or in PKP only?

[kapishnikov, 2017-06-16 20:11:05]

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.h:76: /// Pins a set of public keys for a given
host. This method only has any effect
On 2017/06/12 22:25:22, mef wrote:
> We should probably make other comments in this header documentable the same
way.
> Different CL though.

Acknowledged. Filed http://crbug.com/732888

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.h:93: /// @param hostName name of the host to which
the public keys should be pinned.
On 2017/06/12 22:25:22, mef wrote:
> Need to match actual param names:
> 
> hostName <-> host
> pinsSha356 <-> pinHashes

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:231: DCHECK_EQ(sizeof(hashValue.data),
hash.length)
On 2017/06/12 22:25:23, mef wrote:
> I'm not sure whether DCHECK is good enough here.
> What if app gets pins from the server and messes up the size?
> Release version will not have any guards and may read uninitialized memory
> beyond the end of hashValue.data.

Good catch. Changed it to CHECK_EQ.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:369: +
(void)setMockCertVerifier:(std::unique_ptr<net::CertVerifier>)certVerifier {
On 2017/06/12 22:25:23, mef wrote:
> Should this also have ForTesting: suffix?
> Passing std::unique_ptr as argument is interesting.
> I guess its ok because this method is not in header files?

Added the suffix. I think it should be okay since it is not a part of the public
API and used only for testing.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:349: // Iterate trhough PKP
configuration for every host.
On 2017/06/12 22:25:23, mef wrote:
> nit: through (sp)

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:18: const bool kINCLUDE_SUBDOMAINS
= true;
On 2017/06/12 22:25:23, mef wrote:
> I think constant names are kCamelCase.
> Also, if those are ObjC, then I think they should be BOOL and YES?

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:44: domain_ =
On 2017/06/12 22:25:23, mef wrote:
> Maybe add a comment why we need to extract 'example.com' out of
> 'test.example.com'? OR just hardcode the domain name next to kServerCert?

Added new grpc_support::kTestServerDomain constant.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:48: // Create a Cronet enabled
NSURLSession
On 2017/06/12 22:25:23, mef wrote:
> nit: Comments should be full sentences ending with period. Here and below.

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:61: [Cronet shutdownForTesting];
On 2017/06/12 22:25:23, mef wrote:
> It is strange that we have an explicit shutdown here but not start up in
SetUp.
> I see why, but maybe add a comment?

Added comment. It should be safe to call shutdown even if a test didn't call
StartCronet(). Some future tests may not call StartCronet().

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:79: // Adds a give public-key-pin
and starts a Cronet engine for testing.
On 2017/06/12 22:25:23, mef wrote:
> nit: give -> given

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:96: u_char hash[length];
On 2017/06/12 22:25:23, mef wrote:
> I don't think u_char is normally used in Chromium.
> Can this be std::string(length, '\077').c_str() or somesuch?

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:183: //
On 2017/06/12 22:25:23, mef wrote:
> nit: spurious //

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
File components/cronet/ios/test/cronet_test_base.h (right):

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:5: #ifndef
PRODUCTS_CRONET_TEST_BASE_H
On 2017/06/12 22:25:24, mef wrote:
> nit: COMPONENTS_CRONET_IOS_TEST_CRONET_TEST_BASE_H_

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:8: #include "Cronet/Cronet.h"
On 2017/06/12 22:25:24, mef wrote:
> Should this be #import <Cronet/Cronet.h>?

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:28: @property(strong, atomic)
dispatch_semaphore_t semaphore;
On 2017/06/12 22:25:24, mef wrote:
> Maybe hide this in implementation and add a 'waitForDone' method?
> Possibly add TODO() for another CL?

Done. Added waitForDone() to the delegate.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:52: // The class automatically
starts and stoppes the test QUIC server.
On 2017/06/12 22:25:24, mef wrote:
> nit: stops.

Done.

https://codereview.chromium.org/2928653002/diff/100001/components/cronet/ios/...
components/cronet/ios/test/cronet_test_base.h:55: static bool
CalculatePublicKeySha256(const net::X509Certificate& cert,
On 2017/06/12 22:25:24, mef wrote:
> Do we need this in base or in PKP only?

I would consider this method non-PKP specific but we use it only for PKP
currently.

[mef, 2017-06-19 15:02:33]

lgtm with nits.

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:20: /// Cronet Domain Name.
Should it be 'Cronet Error Domain Name'?

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:86: /// |pinsSha256|, communication with
|hostName| is required to
pinHashes

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:86: /// |pinsSha256|, communication with
|hostName| is required to
host

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:315:
config->ConfigureURLRequestContextBuilder(&context_builder, net_log_.get(),
So, ConfigureURLRequestContextBuilder doesn't use config->pkp_list ?

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/test/DEPS (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/test/DEPS:3: "+crypto",
FYI, I think you'll need crypto OWNER approval for this.

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:23: NSDate* g_distant_future =
[NSDate distantFuture];
Can this be kDistantFuture or use [NSDate distantFuture] directly?

https://codereview.chromium.org/2928653002/diff/120001/components/grpc_suppor...
File components/grpc_support/test/quic_test_server.h (right):

https://codereview.chromium.org/2928653002/diff/120001/components/grpc_suppor...
components/grpc_support/test/quic_test_server.h:19: extern const char
kTestServerDomain[];
nit: maybe move it above ServerHost? Alphabetically and logically. :)

[kapishnikov, 2017-06-19 18:32:48]

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/Cronet.h (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:20: /// Cronet Domain Name.
On 2017/06/19 15:02:33, mef wrote:
> Should it be 'Cronet Error Domain Name'?

The convention is not to add the 'Name' suffix. Changed it to
|CRNCronetErrorDomain|.

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:86: /// |pinsSha256|, communication with
|hostName| is required to
On 2017/06/19 15:02:33, mef wrote:
> host

Done.

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/Cronet.h:86: /// |pinsSha256|, communication with
|hostName| is required to
On 2017/06/19 15:02:33, mef wrote:
> host

Done.

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/cronet_environment.mm (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/cronet_environment.mm:315:
config->ConfigureURLRequestContextBuilder(&context_builder, net_log_.get(),
On 2017/06/19 15:02:33, mef wrote:
> So, ConfigureURLRequestContextBuilder doesn't use config->pkp_list ?

Filed a follow up bug: http://crbug.com/734634

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
File components/cronet/ios/test/cronet_pkp_test.mm (right):

https://codereview.chromium.org/2928653002/diff/120001/components/cronet/ios/...
components/cronet/ios/test/cronet_pkp_test.mm:23: NSDate* g_distant_future =
[NSDate distantFuture];
On 2017/06/19 15:02:33, mef wrote:
> Can this be kDistantFuture or use [NSDate distantFuture] directly?

Done.

https://codereview.chromium.org/2928653002/diff/120001/components/grpc_suppor...
File components/grpc_support/test/quic_test_server.h (right):

https://codereview.chromium.org/2928653002/diff/120001/components/grpc_suppor...
components/grpc_support/test/quic_test_server.h:19: extern const char
kTestServerDomain[];
On 2017/06/19 15:02:33, mef wrote:
> nit: maybe move it above ServerHost? Alphabetically and logically. :)

Done.

[kapishnikov, 2017-06-19 18:49:43]

kapishnikov@chromium.org changed reviewers:
+ davidben@chromium.org

[kapishnikov, 2017-06-19 18:49:44]

Adding David to approve PKP tests dependency on crypto. The tests use
crypto::SHA256HashString to compute SHA-256 hash values.

[kapishnikov, 2017-06-19 19:30:06]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2017-06-19 19:30:07]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org
Link to the patchset: https://codereview.chromium.org/2928653002/#ps140001
(title: "Addressed Misha's comments")

[commit-bot: I haz the power, 2017-06-19 19:30:34]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 19:43:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 19:43:07]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[davidben, 2017-06-19 20:02:39]

DEPS lgtm

[kapishnikov, 2017-06-19 20:15:05]

The CQ bit was checked by kapishnikov@chromium.org

[commit-bot: I haz the power, 2017-06-19 20:15:30]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 20:25:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-19 20:25:28]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[kapishnikov, 2017-06-19 20:46:20]

The CQ bit was checked by kapishnikov@chromium.org

[kapishnikov, 2017-06-19 20:46:21]

The patchset sent to the CQ was uploaded after l-g-t-m from mef@chromium.org,
davidben@chromium.org
Link to the patchset: https://codereview.chromium.org/2928653002/#ps160001
(title: "Fixed DEPS")

[commit-bot: I haz the power, 2017-06-19 20:46:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-06-19 22:20:31]

CQ is committing da patch.

Bot data: {"patchset_id": 160001, "attempt_start_ts": 1497905180783490,
"parent_rev": "5512d48f978312c353d8a7b72fa77b594f4470ee", "commit_rev":
"eac2ab04ef175b2225a961a55108b20254f36a69"}

[commit-bot: I haz the power, 2017-06-19 22:20:44]

Description was changed from

==========
[Cronet-iOS] Public-Key-Pinning Tests

1. Added PKP tests
2. Introduced CronetTestBase class that chares common code used in
   cronet HTTP & PKP tests.
3. Enabled ARC for Cronet tests.

BUG=670689

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

patch from issue 2895123003 at patchset 1 (http://crrev.com/2895123003#ps1)
==========

to

==========
[Cronet-iOS] Public-Key-Pinning Tests

1. Added PKP tests
2. Introduced CronetTestBase class that chares common code used in
   cronet HTTP & PKP tests.
3. Enabled ARC for Cronet tests.

BUG=670689

CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

patch from issue 2895123003 at patchset 1 (http://crrev.com/2895123003#ps1)

Review-Url: https://codereview.chromium.org/2928653002
Cr-Commit-Position: refs/heads/master@{#480600}
Committed:
https://chromium.googlesource.com/chromium/src/+/eac2ab04ef175b2225a961a55108...
==========

[commit-bot: I haz the power, 2017-06-19 22:20:45]

Committed patchset #9 (id:160001) as
https://chromium.googlesource.com/chromium/src/+/eac2ab04ef175b2225a961a55108...