add setter for enable_pkp_bypass_etc

add setter for enable_pkp_bypass_etc

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

Review-Url: https://codereview.chromium.org/2937523002
Cr-Commit-Position: refs/heads/master@{#483025}
Committed: https://chromium.googlesource.com/chromium/src/+/41c31a681b70d8bc7cd4426a928fedf25c62b579

[lilyhoughton, 2017-06-12 17:12:16]

Description was changed from

==========
add setter for enable_pkp_bypass_etc

BUG=
==========

to

==========
add setter for enable_pkp_bypass_etc

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

[lilyhoughton, 2017-06-13 19:20:15]

Description was changed from

==========
add setter for enable_pkp_bypass_etc

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

to

==========
add setter for enable_pkp_bypass_etc

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

[lilyhoughton, 2017-06-13 19:20:16]

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

[lilyhoughton, 2017-06-13 19:21:35]

On 2017/06/13 19:20:16, lilyhoughton wrote:
> mailto:lilyhoughton@chromium.org changed reviewers:
> + mailto:kapishnikov@chromium.org, mailto:mef@chromium.org

Should probably have an error or warning for adding a public key pin when bypass
is set, but other than that, PTAL

[mef, 2017-06-13 20:52:59]

Looks pretty good, but needs better description and bug=

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:52: BOOL gEnablePKPBypassForLocalTrustAnchors =
TRUE;
Hrm, we mix YES/NO and TRUE/FALSE in this file. 
This seems wrong. 
Let's use YES/NO for ObjC BOOL and true/false for C++ bool.

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:240: +
(void)setEnablePublicKeyPinningBypassForLocalTrustAnchors:(bool)enable {
BOOL

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_test_base.h (right):

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
components/cronet/ios/test/cronet_test_base.h:19: +
(void)setEnablePublicKeyPinningBypassForLocalTrustAnchors:(bool)enable;
BOOL

[mef, 2017-06-13 21:12:43]

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

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:145: [Cronet
setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];
This seems to test the default case as if it is not called at all.
Should we have different test where this is set to NO?

[lilyhoughton, 2017-06-14 15:58:42]

Took care of some nits

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:52: BOOL gEnablePKPBypassForLocalTrustAnchors =
TRUE;
On 2017/06/13 20:52:58, mef wrote:
> Hrm, we mix YES/NO and TRUE/FALSE in this file. 
> This seems wrong. 
> Let's use YES/NO for ObjC BOOL and true/false for C++ bool.

Done.

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:240: +
(void)setEnablePublicKeyPinningBypassForLocalTrustAnchors:(bool)enable {
On 2017/06/13 20:52:58, mef wrote:
> BOOL

Done.

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
File components/cronet/ios/test/cronet_test_base.h (right):

https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
components/cronet/ios/test/cronet_test_base.h:19: +
(void)setEnablePublicKeyPinningBypassForLocalTrustAnchors:(bool)enable;
On 2017/06/13 20:52:58, mef wrote:
> BOOL

Done.

[lilyhoughton, 2017-06-14 16:00:44]

On 2017/06/13 21:12:43, mef wrote:
>
https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
> File components/cronet/ios/test/cronet_pkp_test.mm (right):
> 
>
https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
> components/cronet/ios/test/cronet_pkp_test.mm:145: [Cronet
> setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];
> This seems to test the default case as if it is not called at all.
> Should we have different test where this is set to NO?

I think this test (and all of the others) will actually have to change a lot
after changed Andrei makes; since what it will do that is confirm that
|addPublicKeyPin| returns an error if |setEnablePublicKeyPinningBypass...| isn't
set explicitly to false first.

[lilyhoughton, 2017-06-26 18:19:14]

On 2017/06/14 16:00:44, lilyhoughton wrote:
> On 2017/06/13 21:12:43, mef wrote:
> >
>
https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
> > File components/cronet/ios/test/cronet_pkp_test.mm (right):
> > 
> >
>
https://codereview.chromium.org/2937523002/diff/20001/components/cronet/ios/t...
> > components/cronet/ios/test/cronet_pkp_test.mm:145: [Cronet
> > setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];
> > This seems to test the default case as if it is not called at all.
> > Should we have different test where this is set to NO?
> 
> I think this test (and all of the others) will actually have to change a lot
> after changed Andrei makes; since what it will do that is confirm that
> |addPublicKeyPin| returns an error if |setEnablePublicKeyPinningBypass...|
isn't
> set explicitly to false first.

I've finished that work; @kapishnikov PTAL

[kapishnikov, 2017-06-28 03:42:32]

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

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:25: CRNErrorInvalidArgument = 1001,
CRNErrorIncoherentConfig = 1002,
I am not sure the word "Incoherent" is appropriate here. I would call it
"Unsupported configuration error".

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:231: *outError =
In addition to localizedDescription, an error also has
localizedRecoverySuggestion. Should we recommend the client to call the setter
to change the bypass setting?

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

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:145: TEST_F(PkpTest, TestBypass) {
Can we add another test that tests behavior with the default bypass value? I am
afraid that if for some reason we change the default value, we may break
existing applications.

[lilyhoughton, 2017-06-28 14:38:51]

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

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.h:25: CRNErrorInvalidArgument = 1001,
CRNErrorIncoherentConfig = 1002,
On 2017/06/28 03:42:32, kapishnikov wrote:
> I am not sure the word "Incoherent" is appropriate here. I would call it
> "Unsupported configuration error".

Done.

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
File components/cronet/ios/Cronet.mm (right):

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/C...
components/cronet/ios/Cronet.mm:231: *outError =
On 2017/06/28 03:42:32, kapishnikov wrote:
> In addition to localizedDescription, an error also has
> localizedRecoverySuggestion. Should we recommend the client to call the setter
> to change the bypass setting?

Done.

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

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:145: TEST_F(PkpTest, TestBypass) {
On 2017/06/28 03:42:32, kapishnikov wrote:
> Can we add another test that tests behavior with the default bypass value? I
am
> afraid that if for some reason we change the default value, we may break
> existing applications.

I don't know how to do this, since that would require running the test before
any of the tests which call the setter; and I don't think test-order is
guaranteed.

[kapishnikov, 2017-06-28 15:32:24]

The change looks good. Please see some suggestions below.

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

https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
components/cronet/ios/test/cronet_pkp_test.mm:145: TEST_F(PkpTest, TestBypass) {
On 2017/06/28 14:38:51, lilyhoughton wrote:
> On 2017/06/28 03:42:32, kapishnikov wrote:
> > Can we add another test that tests behavior with the default bypass value? I
> am
> > afraid that if for some reason we change the default value, we may break
> > existing applications.
> 
> I don't know how to do this, since that would require running the test before
> any of the tests which call the setter; and I don't think test-order is
> guaranteed.

I was thinking of creating the same test but without calling
[Cronet setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];

Let's assume that somebody in the future will change the definition of
BOOL gEnablePKPBypassForLocalTrustAnchors = YES; to 'NO' by mistake.
Since our PKP tests set the value explicitly, they will still continue passing
but the behavior will change and we will not detect it.

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

https://codereview.chromium.org/2937523002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:432: NSMutableDictionary* errorDictionary =
Could we also add |NSLocalizedFailureReasonErrorKey| saying something like:
"Setting public-key-pinning with local anchor bypass turned on is currently not
supported"?

https://codereview.chromium.org/2937523002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:446: +
(NSError*)createCronetErrorWith:(int)errorCode
Could you rename this method from "createCronetErrorWith" to
"createCronetErrorWithCode"?

[lilyhoughton, 2017-06-28 15:52:07]

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

https://codereview.chromium.org/2937523002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:432: NSMutableDictionary* errorDictionary =
On 2017/06/28 15:32:23, kapishnikov wrote:
> Could we also add |NSLocalizedFailureReasonErrorKey| saying something like:
> "Setting public-key-pinning with local anchor bypass turned on is currently
not
> supported"?

Done.

https://codereview.chromium.org/2937523002/diff/100001/components/cronet/ios/...
components/cronet/ios/Cronet.mm:446: +
(NSError*)createCronetErrorWith:(int)errorCode
On 2017/06/28 15:32:24, kapishnikov wrote:
> Could you rename this method from "createCronetErrorWith" to
> "createCronetErrorWithCode"?

Done.

[lilyhoughton, 2017-06-28 15:57:23]

On 2017/06/28 15:32:24, kapishnikov wrote:
> The change looks good. Please see some suggestions below.
> 
>
https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
> File components/cronet/ios/test/cronet_pkp_test.mm (right):
> 
>
https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
> components/cronet/ios/test/cronet_pkp_test.mm:145: TEST_F(PkpTest, TestBypass)
{
> On 2017/06/28 14:38:51, lilyhoughton wrote:
> > On 2017/06/28 03:42:32, kapishnikov wrote:
> > > Can we add another test that tests behavior with the default bypass value?
I
> > am
> > > afraid that if for some reason we change the default value, we may break
> > > existing applications.
> > 
> > I don't know how to do this, since that would require running the test
before
> > any of the tests which call the setter; and I don't think test-order is
> > guaranteed.
> 
> I was thinking of creating the same test but without calling
> [Cronet setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];
> 
> Let's assume that somebody in the future will change the definition of
> BOOL gEnablePKPBypassForLocalTrustAnchors = YES; to 'NO' by mistake.
> Since our PKP tests set the value explicitly, they will still continue passing
> but the behavior will change and we will not detect it.

I agree that we should test for this!  However, because
|setEnablePublicKeyPinningBypass| sets a global variable, if I just write a test
that doesn't call it explicitly, it _may_ test the default value of
|gEnablePKPBypass|, but it also may test the value that the last test that ran
set it to, since they order that tests run in aren't guaranteed.

[kapishnikov, 2017-06-28 16:08:21]

On 2017/06/28 15:57:23, lilyhoughton wrote:
> On 2017/06/28 15:32:24, kapishnikov wrote:
> > The change looks good. Please see some suggestions below.
> > 
> >
>
https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
> > File components/cronet/ios/test/cronet_pkp_test.mm (right):
> > 
> >
>
https://codereview.chromium.org/2937523002/diff/80001/components/cronet/ios/t...
> > components/cronet/ios/test/cronet_pkp_test.mm:145: TEST_F(PkpTest,
TestBypass)
> {
> > On 2017/06/28 14:38:51, lilyhoughton wrote:
> > > On 2017/06/28 03:42:32, kapishnikov wrote:
> > > > Can we add another test that tests behavior with the default bypass
value?
> I
> > > am
> > > > afraid that if for some reason we change the default value, we may break
> > > > existing applications.
> > > 
> > > I don't know how to do this, since that would require running the test
> before
> > > any of the tests which call the setter; and I don't think test-order is
> > > guaranteed.
> > 
> > I was thinking of creating the same test but without calling
> > [Cronet setEnablePublicKeyPinningBypassForLocalTrustAnchors:YES];
> > 
> > Let's assume that somebody in the future will change the definition of
> > BOOL gEnablePKPBypassForLocalTrustAnchors = YES; to 'NO' by mistake.
> > Since our PKP tests set the value explicitly, they will still continue
passing
> > but the behavior will change and we will not detect it.
> 
> I agree that we should test for this!  However, because
> |setEnablePublicKeyPinningBypass| sets a global variable, if I just write a
test
> that doesn't call it explicitly, it _may_ test the default value of
> |gEnablePKPBypass|, but it also may test the value that the last test that ran
> set it to, since they order that tests run in aren't guaranteed.

Acknowledged.

[kapishnikov, 2017-06-28 16:09:16]

LGTM.

[lilyhoughton, 2017-06-28 16:32:02]

The CQ bit was checked by lilyhoughton@chromium.org

[commit-bot: I haz the power, 2017-06-28 16:32:11]

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-28 16:54:12]

CQ is committing da patch.

Bot data: {"patchset_id": 130001, "attempt_start_ts": 1498667522231800,
"parent_rev": "ac5f75a1f6948c9ace870d57c3df502e9ed802f5", "commit_rev":
"41c31a681b70d8bc7cd4426a928fedf25c62b579"}

[commit-bot: I haz the power, 2017-06-28 16:54:28]

Description was changed from

==========
add setter for enable_pkp_bypass_etc

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

to

==========
add setter for enable_pkp_bypass_etc

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

Review-Url: https://codereview.chromium.org/2937523002
Cr-Commit-Position: refs/heads/master@{#483025}
Committed:
https://chromium.googlesource.com/chromium/src/+/41c31a681b70d8bc7cd4426a928f...
==========

[commit-bot: I haz the power, 2017-06-28 16:54:29]

Committed patchset #8 (id:130001) as
https://chromium.googlesource.com/chromium/src/+/41c31a681b70d8bc7cd4426a928f...