Enable public key pinning of local trust anchors

components/cronet/android/api/src/org/chromium/net/CronetEngine.java

Index: components/cronet/android/api/src/org/chromium/net/CronetEngine.java
diff --git a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
index f30991ac3c7ae2223aaaeabfa683902ab575467f..c0ba787507d1d6ff664b9c455446b8d71f4c90cf 100644
--- a/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
+++ b/components/cronet/android/api/src/org/chromium/net/CronetEngine.java
@@ -101,7 +101,8 @@ public abstract class CronetEngine {
         // See setters below for verbose descriptions.
         private final Context mContext;
         private final List<QuicHint> mQuicHints = new LinkedList<QuicHint>();
-        private final List<Pkp> mPkps = new LinkedList<Pkp>();
+        private final List<Pkp> mPkps = new LinkedList<>();
+        private boolean mPublicKeyPinsForLocalTrustAnchorsEnabled = false;

[xunjieli, 2016/06/20 21:54:53]

Suggest relying on the default initialization of boolean and remove "=false"
here to be consistent with the other booleans.

[kapishnikov, 2016/06/29 23:04:32]

Done.

         private String mUserAgent;
         private String mStoragePath;
         private boolean mLegacyModeEnabled;
@@ -541,6 +542,21 @@ public abstract class CronetEngine {
         }
 
         /**
+         * Enables or disables pinning of the local (user-level) trust anchors.

[Ryan Sleevi, 2016/06/21 00:52:26]

This documentation does not reflect what the code does, because what the code
does is... complicated.

The way it's currently written, this suggests it allows users to pin to a custom
CA. However, that's not the goal of this feature at all, because it would also
have to be accompanied by a change to allow trusting a custom CA, and that's not
something we do.

Fundamentally, this change is the "Disrespect the user" change - in that we
ignore what the user has configured their device as, because we don't want to
(Yes, that's put pejoratively, but it's accurate, even if we did agree to do it)

More importantly, however, this *doesn't* allow user-configured trust anchors to
*override* pinning.

I don't think there's a good way to express this with an "enabled" language - or
at least, I haven't been able to find it. This
disablesPublicKeyPinningBypassForLocalTrustAnchors - but that despite the two
negatives (disable, bypass), you can't rewrite it as a positive and have it mean
the same.

IF you had to write it as a positive, because of some style requirement, then
enablePublicKeyPinningBypassForLocalTrustAnchors, which is default true. With
appropriate documentation that doing so is hostile to users and enterprises, and
should not be set (and if Cronet makes a distinction between first party/third
party consumers, more explicitly call out that first party consumers SHOULD NOT
change this)

[kapishnikov, 2016/06/29 23:04:32]

We decided to go with enablePublicKeyPinningBypassForLocalTrustAnchors to be
consistent. I have also modified the JavaDoc to provide more info and discourage
the disabling of the bypass. Please feel free to suggest any modifications to
the JavaDoc.

+         *
+         * @param value {@code true} to enable pinning, {@code false} to disable.
+         * @return the builder to facilitate chaining.
+         */
+        public Builder enablePublicKeyPinsForLocalTrustAnchors(boolean value) {

[xunjieli, 2016/06/20 21:50:48]

Suggest adding a brief documentation here on why we are not enabling public key
pinning for local trust anchors by default.
Maybe add a reference to this page:
https://www.chromium.org/Home/chromium-security/security-faq#TOC-How-does-key....
It isn't obvious why we are providing the option at first glance.

[kapishnikov, 2016/06/29 23:04:32]

Done.

+            mPublicKeyPinsForLocalTrustAnchorsEnabled = value;
+            return this;
+        }
+
+        boolean publicKeyPinsForLocalTrustAnchorsEnabled() {
+            return mPublicKeyPinsForLocalTrustAnchorsEnabled;
+        }
+
+        /**
          * Checks whether a given string represents a valid host name for PKP and converts it
          * to ASCII Compatible Encoding representation according to RFC 1122, RFC 1123 and
          * RFC 3490. This method is more restrictive than required by RFC 7469. Thus, a host