Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1407263010:
[Cronet] Public key pinning for Java API (Closed)
|
Created:
5 years, 1 month ago by kapishnikov Modified:
5 years ago CC:
chromium-reviews, blundell+watchlist_chromium.org, cbentzel+watch_chromium.org, sdefresne+watchlist_chromium.org, droger+watchlist_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[Cronet] Public key pinning for Java API
Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts.
BUG=522275
Committed: https://crrev.com/df5ccabbdbf77c68a0b0e2ee2b83091054f883f6
Cr-Commit-Position: refs/heads/master@{#363017}
Patch Set 1 #Patch Set 2 : Splitted small javadoc and spelling fixes to a separate CL: https://codereview.chromium.org/1426533… #Patch Set 3 : Small javadoc fix #
Total comments: 20
Patch Set 4 : Addressed review comments #
Total comments: 24
Patch Set 5 : Addressed review comments #Patch Set 6 : Refactored X509UtilTest to avoid code duplication #
Total comments: 12
Patch Set 7 : Added pin expiration time and turned off persistence #Patch Set 8 : Small comment fixes #
Total comments: 26
Patch Set 9 : Added host name verification + review comments #
Total comments: 6
Patch Set 10 : Validation of unicode host names #
Total comments: 15
Patch Set 11 : Addressed review comments #
Total comments: 20
Patch Set 12 : Addressed review comments #Patch Set 13 : Verification of persistence and cleanup of host verification #Patch Set 14 : Shutdown CronetEngine between HPKP tests #
Total comments: 6
Patch Set 15 : Renamed HPKP to PKP #Patch Set 16 : Hostname validation using IDN.USE_STD3_ASCII_RULES and conflict resolution #
Total comments: 11
Patch Set 17 : Added a persistence comment to AddHPKP method #Patch Set 18 : Fixed chromium-style error #Patch Set 19 : Addressed review comments #
Total comments: 3
Patch Set 20 : Small var name change #Patch Set 21 : Hostname conversion to ASCII and IPv4-like hostname validation #
Total comments: 11
Patch Set 22 : Comments & Rebase #
Total comments: 45
Patch Set 23 : Addressed review comments #Patch Set 24 : Small changes and rebase #Messages
Total messages: 72 (16 generated)
kapishnikov@chromium.org changed reviewers: + bnc@chromium.org, mef@chromium.org, pauljensen@chromium.org, xunjieli@chromium.org
C 743.987s Main [==========] 228 tests ran. C 743.987s Main [ PASSED ] 228 tests.
Could you separate out javadoc and typo related changes into a different CL? That way it'll be easier to focus. Thanks for catching those!
The CQ bit was checked by kapishnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407263010/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407263010/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: No L-G-T-M from a valid reviewer yet. Only full committers are accepted. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes and the flag that indicates whether the pinning policy should be applied to the host subdomains. The pins never expire but they are not persisted between the client restarts. Also: 1. Fixed inconsistency of CronetEngineBuilderList class location according to its package name. 2. Modified components/cronet/android/DEPS to add dependency on "crypto" which is needed by mock_cert_verifier 3. Other small fixes. BUG=522275 ========== to ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes and the flag that indicates whether the pinning policy should be applied to the host subdomains. The pins never expire but they are not persisted between the client restarts. Also, modified components/cronet/android/DEPS to add dependency on "crypto" which is needed by mock_cert_verifier BUG=522275 ==========
Few styling comments. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: * @param hostName name of the host that should be pinned. that -> which public keys https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:333: // Convert the pint to BASE64 encoding. the pint? https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:359: return "sha256/" + Base64.encodeToString(sha256, Base64.NO_WRAP); add validation that sha256 is 32 bytes long? https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:375: // Iterate through HPKP configuration for every host nit: I believe (although I can't find proof ATM) that according to chromium style guide comments should be full sentences ending in period. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:385: const std::string& pin_hash = **hash_itr; I think you can just inline this. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: auto hashValue = net::HashValue(net::HASH_VALUE_SHA256); no camelCase for local variables. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/test/mock_cert_verifier.cc:25: net::HashValue* outHashValue); no camel case for local variables. https://google.github.io/styleguide/cppguide.html#Variable_Names https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/test/mock_cert_verifier.cc:58: static bool calculatePublicKeySha256(const net::X509Certificate& cert, move this to the top of the file into anonymous namespace {}. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:53: // Name of the pinned host. nit: Host name. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:105: // The list of host pinning-s -s? End in period.
https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: * @param hostName name of the host that should be pinned. On 2015/11/02 17:56:56, mef wrote: > that -> which public keys Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:333: // Convert the pint to BASE64 encoding. On 2015/11/02 17:56:56, mef wrote: > the pint? Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:359: return "sha256/" + Base64.encodeToString(sha256, Base64.NO_WRAP); On 2015/11/02 17:56:56, mef wrote: > add validation that sha256 is 32 bytes long? Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:375: // Iterate through HPKP configuration for every host On 2015/11/02 17:56:56, mef wrote: > nit: I believe (although I can't find proof ATM) that according to chromium > style guide comments should be full sentences ending in period. Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:385: const std::string& pin_hash = **hash_itr; On 2015/11/02 17:56:56, mef wrote: > I think you can just inline this. Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: auto hashValue = net::HashValue(net::HASH_VALUE_SHA256); On 2015/11/02 17:56:56, mef wrote: > no camelCase for local variables. Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/test/mock_cert_verifier.cc:25: net::HashValue* outHashValue); On 2015/11/02 17:56:56, mef wrote: > no camel case for local variables. > https://google.github.io/styleguide/cppguide.html#Variable_Names Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/andro... components/cronet/android/test/mock_cert_verifier.cc:58: static bool calculatePublicKeySha256(const net::X509Certificate& cert, On 2015/11/02 17:56:56, mef wrote: > move this to the top of the file into anonymous namespace {}. Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:53: // Name of the pinned host. On 2015/11/02 17:56:56, mef wrote: > nit: Host name. Done. https://codereview.chromium.org/1407263010/diff/40001/components/cronet/url_r... components/cronet/url_request_context_config.h:105: // The list of host pinning-s On 2015/11/02 17:56:56, mef wrote: > -s? End in period. Done.
https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: * @param hostName name of the host, which public keys should be pinned. either change "which" to "to which" or change "pinned" to "pinned to" https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:311: * @param pinsSha256 a collection of SHA-256 pins. I'm not well versed in key pinning, but can we elaborate on what the hash is of? is it a hash of the whole certificate? or is it the hash code from inside the certificate? https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: * subdomains of the host's domain name. "the host's domain name"->"{@code hostName}" https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:316: * @throws IllegalArgumentException if the provided collection of pins is invalid. can you elaborate on what "invalid" means. You mean if they do not represent a valid SHA-256 hash code? https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:384: hash_itr != hpkp.pin_hashes.end(); ++hash_itr) { for (auto hash_itr : hpkp.pin_hashes) https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: bool good_hash = hash_value.FromString(**hash_itr); I don't understand the net::HashValue type...I would have expected there to be child classes Sha256HashValue and Sha1HashValue...but they're not children...and I don't understand why I see code memcpy or memset the data()... I've filed a bug to refactor net::HashValue to include some type-safety, crbug.com/551067 https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: bool good_hash = hash_value.FromString(**hash_itr); Let's get rid of the base64 encoding here and memcpy into data() https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:395: context_->transport_security_state()->AddHPKP(hpkp.host, base::Time::Max(), AddHPKP's comment says "(used for net-internals and unit tests)" so I don't think we should be using it, or the comment needs to change. https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java (right): https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. why is this file in net/ if it's only used by components/cronet/? can we move to components/cronet/? https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:21: public class CertTestUtil { The majority of this class looks copied from X509UtilTest.java. Please de-duplicate so we don't have two copies to independently maintain.
https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:384: hash_itr != hpkp.pin_hashes.end(); ++hash_itr) { On 2015/11/03 19:59:30, pauljensen wrote: > for (auto hash_itr : hpkp.pin_hashes) for (const auto& hash_itr : hpkp.pin_hashes)
Paul, thanks for the review. I have addressed most of the comments. There are still two open questions: 1) Getting rid of Base64 encoding in cronet_url_request_context_adapter.cc 2) Location of CertTestUtil.java (see below) https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:310: * @param hostName name of the host, which public keys should be pinned. On 2015/11/03 19:59:30, pauljensen wrote: > either change "which" to "to which" or change "pinned" to "pinned to" Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:311: * @param pinsSha256 a collection of SHA-256 pins. On 2015/11/03 19:59:30, pauljensen wrote: > I'm not well versed in key pinning, but can we elaborate on what the hash is of? > is it a hash of the whole certificate? or is it the hash code from inside the > certificate? Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:311: * @param pinsSha256 a collection of SHA-256 pins. On 2015/11/03 19:59:30, pauljensen wrote: > I'm not well versed in key pinning, but can we elaborate on what the hash is of? > is it a hash of the whole certificate? or is it the hash code from inside the > certificate? Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: * subdomains of the host's domain name. On 2015/11/03 19:59:30, pauljensen wrote: > "the host's domain name"->"{@code hostName}" Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:316: * @throws IllegalArgumentException if the provided collection of pins is invalid. On 2015/11/03 19:59:30, pauljensen wrote: > can you elaborate on what "invalid" means. You mean if they do not represent a > valid SHA-256 hash code? Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:384: hash_itr != hpkp.pin_hashes.end(); ++hash_itr) { On 2015/11/03 19:59:30, pauljensen wrote: > for (auto hash_itr : hpkp.pin_hashes) Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:384: hash_itr != hpkp.pin_hashes.end(); ++hash_itr) { On 2015/11/03 20:18:00, mef wrote: > On 2015/11/03 19:59:30, pauljensen wrote: > > for (auto hash_itr : hpkp.pin_hashes) > for (const auto& hash_itr : hpkp.pin_hashes) Done. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: bool good_hash = hash_value.FromString(**hash_itr); On 2015/11/03 19:59:30, pauljensen wrote: > Let's get rid of the base64 encoding here and memcpy into data() Paul, could you elaborate a little more here? The base64 encoded hash comes from the config (json object). However, HashValue.data() stores the hash value in the binary form; therefore, we need to convert base64 to bin. That is what happens in HashValue.FromString() method. https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:395: context_->transport_security_state()->AddHPKP(hpkp.host, base::Time::Max(), On 2015/11/03 19:59:30, pauljensen wrote: > AddHPKP's comment says "(used for net-internals and unit tests)" so I don't > think we should be using it, or the comment needs to change. I have fixed the comment. The method is almost identical to AddHPKPInternal() which is 'internal' according to the suffix. https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java (right): https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/03 19:59:30, pauljensen wrote: > why is this file in net/ if it's only used by components/cronet/? can we move to > components/cronet/? Initially I created this class in components/cronet but I copied it to net/ for two reasons: 1) The class does not contain cronet specific code and can be useful by 'net' tests. 2) I was hoping to de-duplicate X509UtilTest.pemToDer() method; however, classes in net/android/javatests/src do not see classes in net/test/android/javatests/src (not sure if it is true in reverse direction). Would it be reasonable to move the class to base/? https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:21: public class CertTestUtil { On 2015/11/03 19:59:30, pauljensen wrote: > The majority of this class looks copied from X509UtilTest.java. Please > de-duplicate so we don't have two copies to independently maintain. See the previous comment.
https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... components/cronet/android/cronet_url_request_context_adapter.cc:386: bool good_hash = hash_value.FromString(**hash_itr); On 2015/11/03 23:58:17, kapishnikov wrote: > On 2015/11/03 19:59:30, pauljensen wrote: > > Let's get rid of the base64 encoding here and memcpy into data() > > Paul, could you elaborate a little more here? The base64 encoded hash comes from > the config (json object). However, HashValue.data() stores the hash value in the > binary form; therefore, we need to convert base64 to bin. That is what happens > in HashValue.FromString() method. You convert the binary value to base64 and then back to binary. I'm saying let's not convert it at all and just keep it as binary the whole time. https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java (right): https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:1: // Copyright 2015 The Chromium Authors. All rights reserved. On 2015/11/03 23:58:17, kapishnikov wrote: > On 2015/11/03 19:59:30, pauljensen wrote: > > why is this file in net/ if it's only used by components/cronet/? can we move > to > > components/cronet/? > > Initially I created this class in components/cronet but I copied it to net/ for > two reasons: > 1) The class does not contain cronet specific code and can be useful by 'net' > tests. > 2) I was hoping to de-duplicate X509UtilTest.pemToDer() method; however, classes > in net/android/javatests/src do not see classes in > net/test/android/javatests/src (not sure if it is true in reverse direction). Untrue, net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java sees net/test/android/javatests/src/org/chromium/net/test/util/NetworkChangeNotifierTestUtil.java after my change here: https://codereview.chromium.org/1417973002/diff/100001/net/net.gyp > Would it be reasonable to move the class to base/? No, this is networking specific.
pauljensen@chromium.org changed reviewers: + davidben@chromium.org
David, PTAL @ net/http/transport_security_state.h Andrei is proposing using this API for Cronet key pinning; is it okay to expose?
On 2015/11/04 12:36:43, pauljensen wrote: > https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... > File components/cronet/android/cronet_url_request_context_adapter.cc (right): > > https://codereview.chromium.org/1407263010/diff/60001/components/cronet/andro... > components/cronet/android/cronet_url_request_context_adapter.cc:386: bool > good_hash = hash_value.FromString(**hash_itr); > On 2015/11/03 23:58:17, kapishnikov wrote: > > On 2015/11/03 19:59:30, pauljensen wrote: > > > Let's get rid of the base64 encoding here and memcpy into data() > > > > Paul, could you elaborate a little more here? The base64 encoded hash comes > from > > the config (json object). However, HashValue.data() stores the hash value in > the > > binary form; therefore, we need to convert base64 to bin. That is what > happens > > in HashValue.FromString() method. > > You convert the binary value to base64 and then back to binary. I'm saying > let's not convert it at all and just keep it as binary the whole time. We are considering an option of getting rid of config serialization into JSON and passing the arguments directly using JNI instead. > > https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... > File net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java > (right): > > https://codereview.chromium.org/1407263010/diff/60001/net/test/android/javate... > net/test/android/javatests/src/org/chromium/net/test/util/CertTestUtil.java:1: > // Copyright 2015 The Chromium Authors. All rights reserved. > On 2015/11/03 23:58:17, kapishnikov wrote: > > On 2015/11/03 19:59:30, pauljensen wrote: > > > why is this file in net/ if it's only used by components/cronet/? can we > move > > to > > > components/cronet/? > > > > Initially I created this class in components/cronet but I copied it to net/ > for > > two reasons: > > 1) The class does not contain cronet specific code and can be useful by 'net' > > tests. > > 2) I was hoping to de-duplicate X509UtilTest.pemToDer() method; however, > classes > > in net/android/javatests/src do not see classes in > > net/test/android/javatests/src (not sure if it is true in reverse direction). > Untrue, > net/android/javatests/src/org/chromium/net/NetworkChangeNotifierTest.java sees > net/test/android/javatests/src/org/chromium/net/test/util/NetworkChangeNotifierTestUtil.java > after my change here: > https://codereview.chromium.org/1417973002/diff/100001/net/net.gyp > > > Would it be reasonable to move the class to base/? > No, this is networking specific. Done: removed code duplication.
Awaiting David's input on AddHPKP() usage, which he mentioned yesterday persists pins which we don't want.
On 2015/11/05 12:40:26, pauljensen wrote: > Awaiting David's input on AddHPKP() usage, which he mentioned yesterday persists > pins which we don't want. Yeah, it goes to the same bucket as ones discovered from headers, which get persisted. If you don't create a TransportSecurityStatePersister, it won't actually persist anything, so it'd still be fine, though IMO it's a problem waiting to happen. The best thing to do, IMO, would be to tidy up TransportSecurityState a bit. Maybe have some Source interface or something and have the header-based and preloaded pins be implementations of it. And then you can stick in your own which tracks some list in-memory? That would also avoid the base::Time::Max() weirdness since only header-based pins care about max-age. Splitting that out may even be helpful for getting all the preloaded HSTS domains out of Cronet, which is a size savings of about 34k. (Whether you want to unship those is another question. For an app which talks to arbitrary URLs, like a browser, it's valuable. For an app that only talks to a handful of well-known servers, it's less valuable. There is also the note that, if you're not rendering web content, hopefully your application already points to https URLs and not http? That said, do mind the interaction between HSTS and cookie injection. HSTS isn't purely about correcting bad URLs.) Whether you want to block on that or add sufficiently scary comments + just remember you need a rewrite of stuff before you ever persist anything, I'll leave to you all.
Driving by. Paul and Misha are on this, so I'm removing myself. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/DEPS (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/DEPS:4: "+crypto", you might need to add sleevi@ as a reviewer in order to add this dependency. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:334: // Add HPKP_LIST json array element if it is not present nit: s/json/JSON. And add a period at the end. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:347: // Add new element to HPKP_LIST json array nit: s/json/JSON. And add a period at the end. s/new element/a new element https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:20: * Public-Key-Pinning tests of Cronet Java API. Should we say **dynamic** public key pinning here? to separate it from the case of static public key pinning which we do not do. Also in the CL description. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:154: * TODO(kapishnikov) currently QUIC returns ERR_QUIC_PROTOCOL_ERROR instead of expected nit: Probably only one TODO is needed. There should be a colon after the closing paren. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/url_... File components/cronet/url_request_context_config.h (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/url_... components/cronet/url_request_context_config.h:44: // Public-Key-Pinning configuration structure. nit: could mention this is dynamic public key pinning. (as opposed to static public key pinning)
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
On 2015/11/05 22:36:25, davidben wrote: > On 2015/11/05 12:40:26, pauljensen wrote: > > Awaiting David's input on AddHPKP() usage, which he mentioned yesterday > persists > > pins which we don't want. > > Yeah, it goes to the same bucket as ones discovered from headers, which get > persisted. If you don't create a TransportSecurityStatePersister, it won't > actually persist anything, so it'd still be fine, though IMO it's a problem > waiting to happen. > > The best thing to do, IMO, would be to tidy up TransportSecurityState a bit. > Maybe have some Source interface or something and have the header-based and > preloaded pins be implementations of it. And then you can stick in your own > which tracks some list in-memory? That would also avoid the base::Time::Max() > weirdness since only header-based pins care about max-age. Splitting that out > may even be helpful for getting all the preloaded HSTS domains out of Cronet, > which is a size savings of about 34k. > > (Whether you want to unship those is another question. For an app which talks to > arbitrary URLs, like a browser, it's valuable. For an app that only talks to a > handful of well-known servers, it's less valuable. There is also the note that, > if you're not rendering web content, hopefully your application already points > to https URLs and not http? That said, do mind the interaction between HSTS and > cookie injection. HSTS isn't purely about correcting bad URLs.) > > Whether you want to block on that or add sufficiently scary comments + just > remember you need a rewrite of stuff before you ever persist anything, I'll > leave to you all. David, I have explicitly disabled the persister for Cronet, so we do not cache static or dynamic pins in Cronet. It is just in case if somebody enables it the future (see cronet_url_request_context_adapter.cc). We cannot fail explicitly since neither the transport_security_persister_path field in URLRequestContextBuilder nor the persistence delegate in TransportSecurityState are exposed and can be tested. I have also added the pin expiration time for static HPKP. This brings as on par with Android HPKP configuration design.
On 2015/11/06 15:55:40, kapishnikov wrote: > On 2015/11/05 22:36:25, davidben wrote: > > On 2015/11/05 12:40:26, pauljensen wrote: > > > Awaiting David's input on AddHPKP() usage, which he mentioned yesterday > > persists > > > pins which we don't want. > > > > Yeah, it goes to the same bucket as ones discovered from headers, which get > > persisted. If you don't create a TransportSecurityStatePersister, it won't > > actually persist anything, so it'd still be fine, though IMO it's a problem > > waiting to happen. > > > > The best thing to do, IMO, would be to tidy up TransportSecurityState a bit. > > Maybe have some Source interface or something and have the header-based and > > preloaded pins be implementations of it. And then you can stick in your own > > which tracks some list in-memory? That would also avoid the base::Time::Max() > > weirdness since only header-based pins care about max-age. Splitting that out > > may even be helpful for getting all the preloaded HSTS domains out of Cronet, > > which is a size savings of about 34k. > > > > (Whether you want to unship those is another question. For an app which talks > to > > arbitrary URLs, like a browser, it's valuable. For an app that only talks to a > > handful of well-known servers, it's less valuable. There is also the note > that, > > if you're not rendering web content, hopefully your application already points > > to https URLs and not http? That said, do mind the interaction between HSTS > and > > cookie injection. HSTS isn't purely about correcting bad URLs.) > > > > Whether you want to block on that or add sufficiently scary comments + just > > remember you need a rewrite of stuff before you ever persist anything, I'll > > leave to you all. > > David, I have explicitly disabled the persister for Cronet, so we do not cache > static or dynamic pins in Cronet. It is just in case if somebody enables it the > future (see cronet_url_request_context_adapter.cc). We cannot fail explicitly > since neither the transport_security_persister_path field in > URLRequestContextBuilder nor the persistence delegate in TransportSecurityState > are exposed and can be tested. > > I have also added the pin expiration time for static HPKP. This brings as on par > with Android HPKP configuration design. (Oh right, good point. I forgot about that. We put expirations in static things too as a failsafe in case the app doesn't update. Chrome's preloaded pinlist also self-destructs 1 year after the build timestamp.)
Description was changed from ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes and the flag that indicates whether the pinning policy should be applied to the host subdomains. The pins never expire but they are not persisted between the client restarts. Also, modified components/cronet/android/DEPS to add dependency on "crypto" which is needed by mock_cert_verifier BUG=522275 ========== to ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. Also, 1. modified components/cronet/android/DEPS to add dependency on "crypto" which is needed by mock_cert_verifier 2. Explicitly disable persistence for Cronet. BUG=522275 ==========
https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/DEPS (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/DEPS:4: "+crypto", On 2015/11/06 15:47:54, xunjieli wrote: > you might need to add sleevi@ as a reviewer in order to add this dependency. I will do it. Thanks. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:334: // Add HPKP_LIST json array element if it is not present On 2015/11/06 15:47:54, xunjieli wrote: > nit: s/json/JSON. And add a period at the end. I looked at Java style guide and could not find explicitly about the period at the end of the one line comments; however, some of the examples there are given without the period. https://engdoc.corp.google.com/eng/doc/devguide/java/styleguide.shtml?cl=head... I wounder if we have a document that explicitly says whether there should be period or not. https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:20: * Public-Key-Pinning tests of Cronet Java API. On 2015/11/06 15:47:54, xunjieli wrote: > Should we say **dynamic** public key pinning here? to separate it from the case > of static public key pinning which we do not do. > Also in the CL description. I would like to clarify the terminology here. I was under impression that we call the dynamic pins the ones that are added from HTTP response headers during runtime. The static ones are the ones that are added during configuration time and cannot be changed. I also learned that we have other static pins that are hardcoded. So, is it correctly to call the pins that we supply during configuration dynamic? https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:154: * TODO(kapishnikov) currently QUIC returns ERR_QUIC_PROTOCOL_ERROR instead of expected On 2015/11/06 15:47:54, xunjieli wrote: > nit: Probably only one TODO is needed. There should be a colon after the closing > paren. Done.
xunjieli@chromium.org changed reviewers: + xunjieli@chromium.org
https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:20: * Public-Key-Pinning tests of Cronet Java API. On 2015/11/06 16:18:13, kapishnikov wrote: > On 2015/11/06 15:47:54, xunjieli wrote: > > Should we say **dynamic** public key pinning here? to separate it from the > case > > of static public key pinning which we do not do. > > Also in the CL description. > > I would like to clarify the terminology here. I was under impression that we > call the dynamic pins the ones that are added from HTTP response headers during > runtime. The static ones are the ones that are added during configuration time > and cannot be changed. I also learned that we have other static pins that are > hardcoded. So, is it correctly to call the pins that we supply during > configuration dynamic? I got the impression that we do not want to do static key pinning, so I assumed what we have here is considered dynamic key pinning. Don't really know much about the terminology. On closer look, these new pins do look static to me. I haven't followed the discussion closely. I guess the security folks have already given green light? TransportSecurityState::AddHPKP's comment says that it is used for tests and net-internals, we probably need to change the comment there to reflect that the method is also used in production code.
xunjieli@chromium.org changed reviewers: - xunjieli@chromium.org
On 2015/11/06 16:43:50, xunjieli wrote: > https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... > File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java > (right): > > https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... > components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:20: > * Public-Key-Pinning tests of Cronet Java API. > On 2015/11/06 16:18:13, kapishnikov wrote: > > On 2015/11/06 15:47:54, xunjieli wrote: > > > Should we say **dynamic** public key pinning here? to separate it from the > > case > > > of static public key pinning which we do not do. > > > Also in the CL description. > > > > I would like to clarify the terminology here. I was under impression that we > > call the dynamic pins the ones that are added from HTTP response headers > during > > runtime. The static ones are the ones that are added during configuration time > > and cannot be changed. I also learned that we have other static pins that are > > hardcoded. So, is it correctly to call the pins that we supply during > > configuration dynamic? > > I got the impression that we do not want to do static key pinning, so I assumed > what we have here is considered dynamic key pinning. Don't really know much > about the terminology. On closer look, these new pins do look static to me. I > haven't followed the discussion closely. I guess the security folks have already > given green light? TransportSecurityState::AddHPKP's comment says that it is > used for tests and net-internals, we probably need to change the comment there > to reflect that the method is also used in production code. I have fixed TransportSecurityState::AddHPKP comment https://codereview.chromium.org/1407263010/patch/140001/150010
https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/100001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:20: * Public-Key-Pinning tests of Cronet Java API. On 2015/11/06 16:43:50, xunjieli wrote: > On 2015/11/06 16:18:13, kapishnikov wrote: > > On 2015/11/06 15:47:54, xunjieli wrote: > > > Should we say **dynamic** public key pinning here? to separate it from the > > case > > > of static public key pinning which we do not do. > > > Also in the CL description. > > > > I would like to clarify the terminology here. I was under impression that we > > call the dynamic pins the ones that are added from HTTP response headers > during > > runtime. The static ones are the ones that are added during configuration time > > and cannot be changed. I also learned that we have other static pins that are > > hardcoded. So, is it correctly to call the pins that we supply during > > configuration dynamic? > > I got the impression that we do not want to do static key pinning, so I assumed > what we have here is considered dynamic key pinning. Don't really know much > about the terminology. On closer look, these new pins do look static to me. I > haven't followed the discussion closely. I guess the security folks have already > given green light? TransportSecurityState::AddHPKP's comment says that it is > used for tests and net-internals, we probably need to change the comment there > to reflect that the method is also used in production code. "static" vs "dynamic" is whatever we want to call it. This is different from both use cases in //net today (which is why I think the right answer is to tidy up TransportSecurityState a little because calling AddHPKP is somewhat a hack. It's a hack that works for now, but nonetheless it's not what that API is used for. See also persistence discussion). Today, Chrome has two kinds of pins: 1. "static" or "preloaded" pins. These are hardcoded into Chrome. These *cannot* be enabled in Cronet. 2. "dynamic" pins. These are discovered by HPKP headers and persisted when persistence is hooked up. That's what AddHPKP hooks into. This is adding an API to configure pins in code. These should NOT be persisted. What words you want to call them, I dunno. Probably the words "static" and "dynamic" are not appropriate at this point, given that you're adding a third kind of thing.
In subject maybe: Public key pinning for Java API -> Java API for public key pinning. I think on the bug and in the doc we were calling them 'quasi-static', although I'm sure there is better alternative. Also, I would remove 'also' section from the description. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:288: throw new IllegalArgumentException("Illegal QUIC Hint Host: " + host); In the past there were some misuses of API with url passed instead of host. Maybe we should have the same / similar check in addPublicKeyPins? https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: * hash of DER-encoded ASN.1 representation of Subject Public pinning -> pins https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:314: * Key Info (SPKI) of the host X.509 certificate. You can use Usually Chrome frowns upon use of 'you', 'we', etc in comments. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:328: // Validate the input comments should end in period, but reasonably we don't need this comment. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:356: (double) expirationDate.getTime() / 1000); why double? Can it be long? https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:359: Log.e(TAG, "Unable to add public key pins", e); hrm, why wouldn't we re-throw it? The builder is wrong at this point, right? https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:374: throw new IllegalArgumentException("The provided pin is invalid"); provided -> public key https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:320: #if DCHECK_IS_ON() DCHECK_IS_ON() in debug, but not in the release. Why is it only needed in debug? https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:401: maybe add DCHECK here that context->transport_security_state()->transport_security_persister_ is NULL? (I don't see an accessor, so it could be easier said than done). https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:405: hash_value_vector, GURL()); GURL() -> GURL::EmptyGURL(); https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:26: // Populates out_hash_value with the SHA256 hash of the cert public key. out_hash_value -> |out_hash_value| https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:67: // Calculate the public key hash and add it to the verify_result nit: period at the end. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:67: // Calculate the public key hash and add it to the verify_result given that this is test code I think we expect it to always work. Maybe put a CHECK or DCHECK instead?
Description was changed from ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. Also, 1. modified components/cronet/android/DEPS to add dependency on "crypto" which is needed by mock_cert_verifier 2. Explicitly disable persistence for Cronet. BUG=522275 ========== to ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. BUG=522275 ==========
I see. Thanks for clarifying, David and Misha. This gives a much clearer picture. Andrei: Is it possible to create a public, sanitized version of the design doc and link it in the CL description?
The changes will be uploaded with PS9. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:288: throw new IllegalArgumentException("Illegal QUIC Hint Host: " + host); On 2015/11/06 18:02:16, mef wrote: > In the past there were some misuses of API with url passed instead of host. > Maybe we should have the same / similar check in addPublicKeyPins? I will add the proper check. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:313: * hash of DER-encoded ASN.1 representation of Subject Public On 2015/11/06 18:02:17, mef wrote: > pinning -> pins Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:314: * Key Info (SPKI) of the host X.509 certificate. You can use On 2015/11/06 18:02:16, mef wrote: > Usually Chrome frowns upon use of 'you', 'we', etc in comments. Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:328: // Validate the input On 2015/11/06 18:02:16, mef wrote: > comments should end in period, but reasonably we don't need this comment. Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:356: (double) expirationDate.getTime() / 1000); On 2015/11/06 18:02:16, mef wrote: > why double? Can it be long? Our current C++ JsonValueConverter (https://code.google.com/p/chromium/codesearch#chromium/src/base/json/json_val...) parser does not support java long (int64) value type. Probably we could add one. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:359: Log.e(TAG, "Unable to add public key pins", e); On 2015/11/06 18:02:16, mef wrote: > hrm, why wouldn't we re-throw it? The builder is wrong at this point, right? I agree that re-throwing an exception would be the correct way to handle this situation. However, I have noticed that in most cases we do not do it, e.g. https://code.google.com/p/chromium/codesearch#chromium/src/components/cronet/... The same applies to C++ code that I saw, e.g. when the host name is empty or hash cannot be converted to SHA-256. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:374: throw new IllegalArgumentException("The provided pin is invalid"); On 2015/11/06 18:02:16, mef wrote: > provided -> public key Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:320: #if DCHECK_IS_ON() On 2015/11/06 18:02:17, mef wrote: > DCHECK_IS_ON() in debug, but not in the release. > Why is it only needed in debug? The intention was to execute it in debug mode only to avoid production code changes since this is only a safety check for the situation when we enable the persistence in the future. I assume that the developer who enables the persistence in Cronet would write unit tests that will fail because of this DCHECK. On the other hand, if persistence is enabled but there is no enough test coverage, there is a risk that we may miss a bug during development and discover it in release only. I have removed DCHECK to avoid situation with undiscovered bug; however, both arguments have their merits. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:401: On 2015/11/06 18:02:17, mef wrote: > maybe add DCHECK here that > context->transport_security_state()->transport_security_persister_ is NULL? (I > don't see an accessor, so it could be easier said than done). Yes, the problem is that there is no accessor. We decided not to introduce one. We could also check context_builder.set_transport_security_persister_ before constructing the context. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:405: hash_value_vector, GURL()); On 2015/11/06 18:02:17, mef wrote: > GURL() -> GURL::EmptyGURL(); Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:26: // Populates out_hash_value with the SHA256 hash of the cert public key. On 2015/11/06 18:02:17, mef wrote: > out_hash_value -> |out_hash_value| Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:67: // Calculate the public key hash and add it to the verify_result On 2015/11/06 18:02:17, mef wrote: > given that this is test code I think we expect it to always work. Maybe put a > CHECK or DCHECK instead? Done. https://codereview.chromium.org/1407263010/diff/140001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:67: // Calculate the public key hash and add it to the verify_result On 2015/11/06 18:02:17, mef wrote: > nit: period at the end. Done.
kapishnikov@chromium.org changed reviewers: + estark@chromium.org
Looks pretty good. David & Emily, could you comment from Crypto / Security / PKP side? https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:379: private static String convertSha256ToBase64WithPrefix(byte[] sha256) { I wonder whether private methods should go after public. https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:12: private static final String VALID_IP_EXPR = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" I'm very skeptical about this. https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:43: // Convert the public key bytes to SHA256 hash. // Calculate SHA256 hash of public key bytes.
https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:379: private static String convertSha256ToBase64WithPrefix(byte[] sha256) { On 2015/11/09 16:59:28, mef wrote: > I wonder whether private methods should go after public. I could not find anything regarding the method ordering in Android Code Style Guidelines. We don't follow this pattern 100% in the existing code. However, I found this old document written by Sun: http://www.oracle.com/technetwork/java/codeconventions-141855.html It says: "...methods should be grouped by functionality rather than by scope or accessibility. For example, a private class method can be in between two public instance methods. The goal is to make reading and understanding the code easier." https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:12: private static final String VALID_IP_EXPR = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" On 2015/11/09 16:59:28, mef wrote: > I'm very skeptical about this. I can add more tests to https://codereview.chromium.org/1407263010/patch/160001/170005 The problem is that if we don't validate the host name properly, the C++ side will silently fail adding the pins without notifying the client. Also, according to the HPKP spec, the IPv4 addresses should not be pinned which is not obvious. Should I implement the validation without using the regular expression? https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/160001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:43: // Convert the public key bytes to SHA256 hash. On 2015/11/09 16:59:28, mef wrote: > // Calculate SHA256 hash of public key bytes. Done in the next PS.
https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: boolean includeSubdomains, Date expirationDate) { This doesn't seem to match the API discussed in https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... I thought we settled on addPublicKeyPins/removePublicKeyPins so that the caller would manage the expiration? I think expiration date is kind of misleading because the pins aren't persisted. (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... is the particular discussion I'm thinking of.) https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:387: * A valid host name should not match IPv4 address. ipv6 shouldn't be valid either https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:13: private static final String VALID_IP_EXPR = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" Where are these regexes from? It's difficult for me to tell if they're correct. Did you consider doing something more like what https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon.h&sq... does? (And there's no existing code available to do this?) https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:386: for (auto hpkp_itr = config->hpkp_list.begin(); Can this be `for (const auto& hpkp : hpkp_list)`? https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUtilTest.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUtilTest.java:25: assertInvalidHostName("domain.com:300"); IPv6 test cases would be good, I think.
https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: boolean includeSubdomains, Date expirationDate) { On 2015/11/10 19:04:16, estark wrote: > This doesn't seem to match the API discussed in > https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... > I thought we settled on addPublicKeyPins/removePublicKeyPins so that the caller > would manage the expiration? I think expiration date is kind of misleading > because the pins aren't persisted. > > (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... > is the particular discussion I'm thinking of.) Good point. I forgot about that discussion. We've added expirationDate to match design in this document: https://docs.google.com/a/google.com/document/d/1hC_JDISiBoxqNHDxSgzsWD0id51v... but I suppose it is not necessary. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:387: * A valid host name should not match IPv4 address. On 2015/11/10 19:04:16, estark wrote: > ipv6 shouldn't be valid either I think ipv4 is checked explicitly as it includes dots, whereas ipv6 uses semicolons, which are not allowed in host name anyway. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:29: String ascii = IDN.toASCII(hostName); maybe IDN.toASCII(src, IDN.USE_STD3_ASCII_RULES) to match https://code.google.com/p/chromium/codesearch#chromium/src/url/android/java/s...
Emily, thanks for the review. See the comments below. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: boolean includeSubdomains, Date expirationDate) { On 2015/11/10 19:04:16, estark wrote: > This doesn't seem to match the API discussed in > https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... > I thought we settled on addPublicKeyPins/removePublicKeyPins so that the caller > would manage the expiration? I think expiration date is kind of misleading > because the pins aren't persisted. > > (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... > is the particular discussion I'm thinking of.) It is a good point. Currently addPublicKeyPins() is configuration time API used to configure CronetEngine before its instantiation. Since the pins are not persisted, removePublicKeyPins() has limited usability during the configuration. It could only be used to cancel addPublicKeyPins() operation during the same CronetEngine configuration session. It would make perfect sense to have add/remove pair if we provided dynamic pinning or pin persistence. Having expiration date is actually quite useful. My understanding is that in most common use-case, the apps will contain hard-coded pins. Those pins will be passed to CronetEngine on every app launch. If we don't provide expiration date, the apps may stop working after the server certificate is expired and reissued on the server. In order to prevent this situation, the pin can be set to expire same time (or a little earlier) as the certificate. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:13: private static final String VALID_IP_EXPR = "^(([0-9]|[1-9][0-9]|1[0-9]{2}|2[0-4][0-9]|25[0-5])" On 2015/11/10 19:04:16, estark wrote: > Where are these regexes from? It's difficult for me to tell if they're correct. > Did you consider doing something more like what > https://code.google.com/p/chromium/codesearch#chromium/src/url/url_canon.h&sq... > does? (And there's no existing code available to do this?) I took it somewhere from stackoverflow.com. There are tests that verify these regular expressions. I can add some more. My goal was to improve current host validation that checks for the presence of "/" character. It is important to have better validation since the C++ code responsible for adding the pins silently fails without notifying the client when the host is invalid. I will take a look at the code you referenced but at the first glance it looks more complicated. There is Apache Common library that does it. It is a good library and I wonder if we ever considered using it: https://commons.apache.org/proper/commons-validator/apidocs/org/apache/common... https://commons.apache.org/proper/commons-validator/apidocs/org/apache/common... https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:29: String ascii = IDN.toASCII(hostName); On 2015/11/10 22:19:03, mef wrote: > maybe IDN.toASCII(src, IDN.USE_STD3_ASCII_RULES) to match > https://code.google.com/p/chromium/codesearch#chromium/src/url/android/java/s... I will add it. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:386: for (auto hpkp_itr = config->hpkp_list.begin(); On 2015/11/10 19:04:16, estark wrote: > Can this be `for (const auto& hpkp : hpkp_list)`? Will fix it in the next PS. https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/CronetUtilTest.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/CronetUtilTest.java:25: assertInvalidHostName("domain.com:300"); On 2015/11/10 19:04:16, estark wrote: > IPv6 test cases would be good, I think. Will add it in the next PS.
kapishnikov@chromium.org changed reviewers: + nharper@chromium.org
https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: * {@code security.cert.X509Certificate.getPublicKey().getEncoded()} This should be a @link. You may need to add "java." prefix and/or update src/components/cronet/android/api/package-list https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * @throws IllegalArgumentException if the given host name is invalid or {@code pinsSha256} add "the" before pinsSha256 https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:371: * Converts a given SHA256 array of bytes to BASE64 encoding with the prefix. The format Please explain what "the prefix" is. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; Let's not reinvent the wheel. Please use either some java.* function (it doesn't look like java.net.URL or java.net.InetSocketAddress validates the host name, maybe something else does) or even better would be to call to some native code that is going to be used later to validate it, like GURL. Same goes for isValidIPv4. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:320: // of dynamic HPKP. This is a safety measure in case if somebody will in case if somebody will enable->ensuring nobody enables https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:24: public class HpkpTest extends CronetTestBase { can we add a test that the pins are not persisted? https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... components/cronet/url_request_context_config.cc:34: bool GetDateFromDouble(const base::Value* json_value, base::Time* time) { GetTimeFromDouble https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... File components/cronet/url_request_context_config_list.h (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... components/cronet/url_request_context_config_list.h:37: DEFINE_CONTEXT_CONFIG(HPKP_LIST) These needs comments.
https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:317: * {@code security.cert.X509Certificate.getPublicKey().getEncoded()} On 2015/11/17 19:01:23, pauljensen wrote: > This should be a @link. You may need to add "java." prefix and/or update > src/components/cronet/android/api/package-list Done. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * @throws IllegalArgumentException if the given host name is invalid or {@code pinsSha256} On 2015/11/17 19:01:23, pauljensen wrote: > add "the" before pinsSha256 Done. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:371: * Converts a given SHA256 array of bytes to BASE64 encoding with the prefix. The format On 2015/11/17 19:01:23, pauljensen wrote: > Please explain what "the prefix" is. Done. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; On 2015/11/17 19:01:23, pauljensen wrote: > Let's not reinvent the wheel. Please use either some java.* function (it > doesn't look like java.net.URL or java.net.InetSocketAddress validates the host > name, maybe something else does) or even better would be to call to some native > code that is going to be used later to validate it, like GURL. > Same goes for isValidIPv4. Calling native implementation would eliminate the duplication of validation logic. Ideally we would like to verify the hostname when the client makes a call to addPublicKeyPins() method. However, when addPublicKeyPins() is called, the native library may not be loaded yet. We could postpone the verification until the context is created. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:320: // of dynamic HPKP. This is a safety measure in case if somebody will On 2015/11/17 19:01:23, pauljensen wrote: > in case if somebody will enable->ensuring nobody enables Done. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/HpkpTest.java:24: public class HpkpTest extends CronetTestBase { On 2015/11/17 19:01:23, pauljensen wrote: > can we add a test that the pins are not persisted? I am adding the test. Somehow TransportSecurityState is reused and not cleared when a second instance of CronetEngine is created within the same test. Also filed a related bug: http://crbug.com/557425 https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... components/cronet/url_request_context_config.cc:34: bool GetDateFromDouble(const base::Value* json_value, base::Time* time) { On 2015/11/17 19:01:23, pauljensen wrote: > GetTimeFromDouble Done. https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... File components/cronet/url_request_context_config_list.h (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/url_... components/cronet/url_request_context_config_list.h:37: DEFINE_CONTEXT_CONFIG(HPKP_LIST) On 2015/11/17 19:01:23, pauljensen wrote: > These needs comments. Done.
nhaper@ - With davidben@ overloaded, could you review it from security perspective? estark@ - PTAL davidben@ - Is it ok to add +crypto to Cronet DEPS for Sha256 calculation, or is there a better way? https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: boolean includeSubdomains, Date expirationDate) { On 2015/11/10 22:33:33, kapishnikov wrote: > On 2015/11/10 19:04:16, estark wrote: > > This doesn't seem to match the API discussed in > > > https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... > > I thought we settled on addPublicKeyPins/removePublicKeyPins so that the > caller > > would manage the expiration? I think expiration date is kind of misleading > > because the pins aren't persisted. > > > > > (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... > > is the particular discussion I'm thinking of.) > > It is a good point. Currently addPublicKeyPins() is configuration time API used > to configure CronetEngine before its instantiation. Since the pins are not > persisted, removePublicKeyPins() has limited usability during the configuration. > It could only be used to cancel addPublicKeyPins() operation during the same > CronetEngine configuration session. It would make perfect sense to have > add/remove pair if we provided dynamic pinning or pin persistence. > > Having expiration date is actually quite useful. My understanding is that in > most common use-case, the apps will contain hard-coded pins. Those pins will be > passed to CronetEngine on every app launch. If we don't provide expiration date, > the apps may stop working after the server certificate is expired and reissued > on the server. In order to prevent this situation, the pin can be set to expire > same time (or a little earlier) as the certificate. Emily, WDYT? https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; On 2015/11/18 00:10:31, kapishnikov wrote: > On 2015/11/17 19:01:23, pauljensen wrote: > > Let's not reinvent the wheel. Please use either some java.* function (it > > doesn't look like java.net.URL or java.net.InetSocketAddress validates the > host > > name, maybe something else does) or even better would be to call to some > native > > code that is going to be used later to validate it, like GURL. > > Same goes for isValidIPv4. > > Calling native implementation would eliminate the duplication of validation > logic. Ideally we would like to verify the hostname when the client makes a call > to addPublicKeyPins() method. However, when addPublicKeyPins() is called, the > native library may not be loaded yet. We could postpone the verification until > the context is created. We've chatted with Andrei about it, and another option is to do additional config validation on the native side when UrlRequestContext gets created. This will allow us to use native method without needs to load it explicitly. I think it might be better done separately, after Paul's CL that removes JSON. I agree that having multiple subtly different validators is not great, so maybe we should just remove the java-side validation from this CL? https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:12: #include "crypto/sha2.h" This requires adding +crypto to DEPS. David, could you OWNER-approve that or suggest a better way?
https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:12: #include "crypto/sha2.h" On 2015/11/18 22:41:38, mef wrote: > This requires adding +crypto to DEPS. > David, could you OWNER-approve that or suggest a better way? Adding +crypto to DEPS lgtm. I haven't looked at the rest. https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:28: static bool calculatePublicKeySha256(const net::X509Certificate& cert, Style: UpperCamelCase
https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: boolean includeSubdomains, Date expirationDate) { On 2015/11/18 22:41:38, mef wrote: > On 2015/11/10 22:33:33, kapishnikov wrote: > > On 2015/11/10 19:04:16, estark wrote: > > > This doesn't seem to match the API discussed in > > > > > > https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... > > > I thought we settled on addPublicKeyPins/removePublicKeyPins so that the > > caller > > > would manage the expiration? I think expiration date is kind of misleading > > > because the pins aren't persisted. > > > > > > > > > (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... > > > is the particular discussion I'm thinking of.) > > > > It is a good point. Currently addPublicKeyPins() is configuration time API > used > > to configure CronetEngine before its instantiation. Since the pins are not > > persisted, removePublicKeyPins() has limited usability during the > configuration. > > It could only be used to cancel addPublicKeyPins() operation during the same > > CronetEngine configuration session. It would make perfect sense to have > > add/remove pair if we provided dynamic pinning or pin persistence. > > > > Having expiration date is actually quite useful. My understanding is that in > > most common use-case, the apps will contain hard-coded pins. Those pins will > be > > passed to CronetEngine on every app launch. If we don't provide expiration > date, > > the apps may stop working after the server certificate is expired and reissued > > on the server. In order to prevent this situation, the pin can be set to > expire > > same time (or a little earlier) as the certificate. > > Emily, WDYT? Sorry for dropping off on this. I guess this seems okay to me, to match the Android API. https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:343: JSONArray hpkpList = mConfig.optJSONArray(CronetEngineBuilderList.HPKP_LIST); totally optional nit: it's a little confusing to use hpkp in the code here, since the h is for HTTP and this feature doesn't have anything to do with HTTP
On 2015/11/18 22:41:38, mef wrote: > nhaper@ - With davidben@ overloaded, could you review it from security > perspective? > estark@ - PTAL > davidben@ - Is it ok to add +crypto to Cronet DEPS for Sha256 calculation, or is > there a better way? > > https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... > File components/cronet/android/api/src/org/chromium/net/CronetEngine.java > (right): > > https://codereview.chromium.org/1407263010/diff/180001/components/cronet/andr... > components/cronet/android/api/src/org/chromium/net/CronetEngine.java:329: > boolean includeSubdomains, Date expirationDate) { > On 2015/11/10 22:33:33, kapishnikov wrote: > > On 2015/11/10 19:04:16, estark wrote: > > > This doesn't seem to match the API discussed in > > > > > > https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl.... > > > I thought we settled on addPublicKeyPins/removePublicKeyPins so that the > > caller > > > would manage the expiration? I think expiration date is kind of misleading > > > because the pins aren't persisted. > > > > > > > > > (https://docs.google.com/document/d/1HHN_dkfno9jIhglznyS6D86wlSaZf1zAcjZL_jsSl... > > > is the particular discussion I'm thinking of.) > > > > It is a good point. Currently addPublicKeyPins() is configuration time API > used > > to configure CronetEngine before its instantiation. Since the pins are not > > persisted, removePublicKeyPins() has limited usability during the > configuration. > > It could only be used to cancel addPublicKeyPins() operation during the same > > CronetEngine configuration session. It would make perfect sense to have > > add/remove pair if we provided dynamic pinning or pin persistence. > > > > Having expiration date is actually quite useful. My understanding is that in > > most common use-case, the apps will contain hard-coded pins. Those pins will > be > > passed to CronetEngine on every app launch. If we don't provide expiration > date, > > the apps may stop working after the server certificate is expired and reissued > > on the server. In order to prevent this situation, the pin can be set to > expire > > same time (or a little earlier) as the certificate. > > Emily, WDYT? > > https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... > File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): > > https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... > components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + > "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; > On 2015/11/18 00:10:31, kapishnikov wrote: > > On 2015/11/17 19:01:23, pauljensen wrote: > > > Let's not reinvent the wheel. Please use either some java.* function (it > > > doesn't look like java.net.URL or java.net.InetSocketAddress validates the > > host > > > name, maybe something else does) or even better would be to call to some > > native > > > code that is going to be used later to validate it, like GURL. > > > Same goes for isValidIPv4. > > > > Calling native implementation would eliminate the duplication of validation > > logic. Ideally we would like to verify the hostname when the client makes a > call > > to addPublicKeyPins() method. However, when addPublicKeyPins() is called, the > > native library may not be loaded yet. We could postpone the verification until > > the context is created. > > We've chatted with Andrei about it, and another option is to do additional > config validation on the native side when UrlRequestContext gets created. > This will allow us to use native method without needs to load it explicitly. I > think it might be better done separately, after Paul's CL that removes JSON. > > I agree that having multiple subtly different validators is not great, so maybe > we should just remove the java-side validation from this CL? I talked to Paul regarding the validation. Unfortunately, we couldn't find native implementation that we could use directly. Meanwhile, I refactored the regular expressions and added some explanation. It should be easier to understand them now. https://codereview.chromium.org/1407263010/patch/260001/270004 Also added more tests: https://codereview.chromium.org/1407263010/patch/260001/270006 > > https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... > File components/cronet/android/test/mock_cert_verifier.cc (right): > > https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... > components/cronet/android/test/mock_cert_verifier.cc:12: #include > "crypto/sha2.h" > This requires adding +crypto to DEPS. > David, could you OWNER-approve that or suggest a better way?
https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:343: JSONArray hpkpList = mConfig.optJSONArray(CronetEngineBuilderList.HPKP_LIST); On 2015/11/18 23:10:06, estark wrote: > totally optional nit: it's a little confusing to use hpkp in the code here, > since the h is for HTTP and this feature doesn't have anything to do with HTTP Done. Renamed HPKP to PKP everywhere in the new code unless it references real HPKP. https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/260001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:28: static bool calculatePublicKeySha256(const net::X509Certificate& cert, On 2015/11/18 22:46:32, davidben (slow) wrote: > Style: UpperCamelCase Done.
https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; On 2015/11/18 22:41:38, mef wrote: > On 2015/11/18 00:10:31, kapishnikov wrote: > > On 2015/11/17 19:01:23, pauljensen wrote: > > > Let's not reinvent the wheel. Please use either some java.* function (it > > > doesn't look like java.net.URL or java.net.InetSocketAddress validates the > > host > > > name, maybe something else does) or even better would be to call to some > > native > > > code that is going to be used later to validate it, like GURL. > > > Same goes for isValidIPv4. > > > > Calling native implementation would eliminate the duplication of validation > > logic. Ideally we would like to verify the hostname when the client makes a > call > > to addPublicKeyPins() method. However, when addPublicKeyPins() is called, the > > native library may not be loaded yet. We could postpone the verification until > > the context is created. > > We've chatted with Andrei about it, and another option is to do additional > config validation on the native side when UrlRequestContext gets created. > This will allow us to use native method without needs to load it explicitly. I > think it might be better done separately, after Paul's CL that removes JSON. > > I agree that having multiple subtly different validators is not great, so maybe > we should just remove the java-side validation from this CL? For IPv4 validation, you can use Os.inet_pton(): http://developer.android.com/reference/android/system/Os.html#inet_pton(int, java.lang.String) https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; On 2015/11/18 22:41:38, mef wrote: > On 2015/11/18 00:10:31, kapishnikov wrote: > > On 2015/11/17 19:01:23, pauljensen wrote: > > > Let's not reinvent the wheel. Please use either some java.* function (it > > > doesn't look like java.net.URL or java.net.InetSocketAddress validates the > > host > > > name, maybe something else does) or even better would be to call to some > > native > > > code that is going to be used later to validate it, like GURL. > > > Same goes for isValidIPv4. > > > > Calling native implementation would eliminate the duplication of validation > > logic. Ideally we would like to verify the hostname when the client makes a > call > > to addPublicKeyPins() method. However, when addPublicKeyPins() is called, the > > native library may not be loaded yet. We could postpone the verification until > > the context is created. > > We've chatted with Andrei about it, and another option is to do additional > config validation on the native side when UrlRequestContext gets created. > This will allow us to use native method without needs to load it explicitly. I > think it might be better done separately, after Paul's CL that removes JSON. > > I agree that having multiple subtly different validators is not great, so maybe > we should just remove the java-side validation from this CL? I looked around and I don't think we have any native side hostname validation. I think Chrome just generally passes things along to getaddrinfo(). There is some code in GURL to identify the hostname but I don't think its intent is to validate it.
https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/200001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:16: + "[a-zA-Z0-9])\\.)*([a-zA-Z0-9]|[a-zA-Z0-9][a-zA-Z0-9\\-]*[a-zA-Z0-9])$"; On 2015/11/19 04:07:43, pauljensen wrote: > On 2015/11/18 22:41:38, mef wrote: > > On 2015/11/18 00:10:31, kapishnikov wrote: > > > On 2015/11/17 19:01:23, pauljensen wrote: > > > > Let's not reinvent the wheel. Please use either some java.* function (it > > > > doesn't look like java.net.URL or java.net.InetSocketAddress validates the > > > host > > > > name, maybe something else does) or even better would be to call to some > > > native > > > > code that is going to be used later to validate it, like GURL. > > > > Same goes for isValidIPv4. > > > > > > Calling native implementation would eliminate the duplication of validation > > > logic. Ideally we would like to verify the hostname when the client makes a > > call > > > to addPublicKeyPins() method. However, when addPublicKeyPins() is called, > the > > > native library may not be loaded yet. We could postpone the verification > until > > > the context is created. > > > > We've chatted with Andrei about it, and another option is to do additional > > config validation on the native side when UrlRequestContext gets created. > > This will allow us to use native method without needs to load it explicitly. I > > think it might be better done separately, after Paul's CL that removes JSON. > > > > I agree that having multiple subtly different validators is not great, so > maybe > > we should just remove the java-side validation from this CL? > > For IPv4 validation, you can use Os.inet_pton(): > http://developer.android.com/reference/android/system/Os.html#inet_pton(int, > java.lang.String) It looks that the Os class was added in Android 21. In our sample app manifest we specify minSdkVersion="14".
Unfortunately I've been dragged into VC meetings virtually all day today so I haven't had a chance to review before I head out. I looked over the new host name validator and quite like it. The Android documentation for USE_STD3_ASCII_RULES is scant and after tracing the code through to calling into Android's ICU copy, and reading ICU's documentation I still don't really understand it, but if the tests pass and the snippet of Android documentation that says it enforces RFC 1122 and 1123 seem good, then I'm fine with it. I think this CL is about done. As I mentioned in the meeting today, I think we may want to add the scariness to the AddHPKP() comment so others might avoid the unintended-persistence case. I also don't know if we want this to wait for my JSON-removal CL to land or not, probably not. Anyhow, someone should review this, or I may have a chance while OOO.
https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:397: return CronetUtil.isValidHostName(hostName) && !CronetUtil.isValidIPv4(hostName); This doesn't explicitly check that hostName isn't an IPv6 address (although I imagine isValidHostName would fail when given an IPv6 address, I can't be sure). RFC 7469 section 2.3.3 states "Known Pinned Hosts are identified only by domain names, and never IP addresses." https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:26: * does not verify the length of the host name labels, the total length of documentation nit: RFC 3490 section 4.1 states that ToASCII (as defined to operate on a label) checks that the number of code points is in the range 1 to 63 inclusive. Does the IDN toASCII method not do this check? https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:53: return VALID_IP_PATTERN.matcher(addr).matches(); Chrome interprets plenty of things that don't match that regex as IPv4 addresses. The following all resolve to 127.0.0.1: 2130706433 0177.0.0.1 127.0.00.1 Instead of reimplementing the isValidHostName and isValidIPv4 checks here, can we reuse the code that already exists? https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:67: addPkpSha256(mServerHost, nonMatchingHash, EXCLUDE_SUBDOMAINS, DISTANT_FUTURE); nit: calls to addPkpSha256 alternate between inlining the call to generateSomeSha256() (like in testIncludeSubdomainsFlagEqualTrue) and defining byte[] nonMatchingHash. They should all be the same.
Nick, thanks for the review. Please see the comments below. https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:397: return CronetUtil.isValidHostName(hostName) && !CronetUtil.isValidIPv4(hostName); On 2015/11/19 23:47:54, nharper wrote: > This doesn't explicitly check that hostName isn't an IPv6 address (although I > imagine isValidHostName would fail when given an IPv6 address, I can't be sure). > > RFC 7469 section 2.3.3 states "Known Pinned Hosts are identified only by domain > names, and never IP addresses." Yes, any IPv6 is not a valid host name and will be rejected. I added IPv6 to the method description. https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:26: * does not verify the length of the host name labels, the total length of On 2015/11/19 23:47:54, nharper wrote: > documentation nit: RFC 3490 section 4.1 states that ToASCII (as defined to > operate on a label) checks that the number of code points is in the range 1 to > 63 inclusive. Does the IDN toASCII method not do this check? You are right. I have changed the comments and added tests to verify the length of the host labels and the total length of the hostname. https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:53: return VALID_IP_PATTERN.matcher(addr).matches(); On 2015/11/19 23:47:54, nharper wrote: > Chrome interprets plenty of things that don't match that regex as IPv4 > addresses. The following all resolve to 127.0.0.1: > > 2130706433 > 0177.0.0.1 > 127.0.00.1 > > Instead of reimplementing the isValidHostName and isValidIPv4 checks here, can > we reuse the code that already exists? To validate the host name, isValidHostName() method is using IDN class that was in Android since API 9. Regarding IPv4 validation. RFC 3986 Section 3.2.2. Host says: A host identified by an IPv4 literal address is represented in dotted-decimal notation (a sequence of four decimal numbers in the range 0 to 255, separated by "."), as described in [RFC1123] by reference to [RFC0952]. Note that other forms of dotted notation may be interpreted on some platforms, as described in Section 7.4, but only the dotted-decimal form of four octets is allowed by this grammar. IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet dec-octet = DIGIT ; 0-9 / %x31-39 DIGIT ; 10-99 / "1" 2DIGIT ; 100-199 / "2" %x30-34 DIGIT ; 200-249 / "25" %x30-35 ; 250-255 isValidIPv4() method verifies that the address satisfies the above mentioned grammar. Unfortunately, I couldn't find any existing implementation that does it. I have added comments to the method to emphasize the required format. https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:67: addPkpSha256(mServerHost, nonMatchingHash, EXCLUDE_SUBDOMAINS, DISTANT_FUTURE); On 2015/11/19 23:47:54, nharper wrote: > nit: calls to addPkpSha256 alternate between inlining the call to > generateSomeSha256() (like in testIncludeSubdomainsFlagEqualTrue) and defining > byte[] nonMatchingHash. They should all be the same. Done.
https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:53: return VALID_IP_PATTERN.matcher(addr).matches(); On 2015/11/20 16:38:03, kapishnikov wrote: > On 2015/11/19 23:47:54, nharper wrote: > > Chrome interprets plenty of things that don't match that regex as IPv4 > > addresses. The following all resolve to 127.0.0.1: > > > > 2130706433 > > 0177.0.0.1 > > 127.0.00.1 > > > > Instead of reimplementing the isValidHostName and isValidIPv4 checks here, can > > we reuse the code that already exists? > > To validate the host name, isValidHostName() method is using IDN class that was > in Android since API 9. > > Regarding IPv4 validation. RFC 3986 Section 3.2.2. Host says: > > > A host identified by an IPv4 literal address is represented in > dotted-decimal notation (a sequence of four decimal numbers in the > range 0 to 255, separated by "."), as described in [RFC1123] by > reference to [RFC0952]. Note that other forms of dotted notation may > be interpreted on some platforms, as described in Section 7.4, but > only the dotted-decimal form of four octets is allowed by this > grammar. > > IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet > > dec-octet = DIGIT ; 0-9 > / %x31-39 DIGIT ; 10-99 > / "1" 2DIGIT ; 100-199 > / "2" %x30-34 DIGIT ; 200-249 > / "25" %x30-35 ; 250-255 > > isValidIPv4() method verifies that the address satisfies the above mentioned > grammar. Unfortunately, I couldn't find any existing implementation that does > it. I have added comments to the method to emphasize the required format. (This comment really belongs on isValidHostNameForPinning in CronetEngine.java.) My concern here isn't that isValidIPv4 doesn't do what it's name says (I believe that this method correctly checks whether the provided string matches IPv4address as defined in RFC 3986). Instead, my concern is that the combination of isValidHostname && !isValidIPv4 will let through something that isn't a valid IP address, but can be interpreted as one. The existing native code uses https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=443&... to check if the host of the url for a request is an IP address, which allows for all sorts of awful things not in RFC 3986. Even if we accept an IP address-like hostname, it won't be used for pinning because 1) it's not canonicalized, and 2) (more importantly) a request coming in for an IP will never be checked against the pin set. isValidHostNameForPinning should fail so that addPublicKeyPins will throw an exception if an IP address-like hostname is passed in. Maybe the IP address check should be relaxed to something like ^[0-9.]*$ for hostnames to reject? That regex is very ad-hoc, but I can't think of anything that would match it that could actually be a valid domain (unless ICANN starts making gTLDs that consist of only digits). https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:357: pkp.put(CronetEngineBuilderList.PKP_HOST, hostName); At some point, hostName needs to undergo IDN processing. TransportSecurityState::AddHPKP (called in cronet_url_request_context_adapter.cc) expects that its input has already undergone IDN processing.
https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:53: return VALID_IP_PATTERN.matcher(addr).matches(); On 2015/11/20 19:02:47, nharper wrote: > On 2015/11/20 16:38:03, kapishnikov wrote: > > On 2015/11/19 23:47:54, nharper wrote: > > > Chrome interprets plenty of things that don't match that regex as IPv4 > > > addresses. The following all resolve to 127.0.0.1: > > > > > > 2130706433 > > > 0177.0.0.1 > > > 127.0.00.1 > > > > > > Instead of reimplementing the isValidHostName and isValidIPv4 checks here, > can > > > we reuse the code that already exists? > > > > To validate the host name, isValidHostName() method is using IDN class that > was > > in Android since API 9. > > > > Regarding IPv4 validation. RFC 3986 Section 3.2.2. Host says: > > > > > > A host identified by an IPv4 literal address is represented in > > dotted-decimal notation (a sequence of four decimal numbers in the > > range 0 to 255, separated by "."), as described in [RFC1123] by > > reference to [RFC0952]. Note that other forms of dotted notation may > > be interpreted on some platforms, as described in Section 7.4, but > > only the dotted-decimal form of four octets is allowed by this > > grammar. > > > > IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet > > > > dec-octet = DIGIT ; 0-9 > > / %x31-39 DIGIT ; 10-99 > > / "1" 2DIGIT ; 100-199 > > / "2" %x30-34 DIGIT ; 200-249 > > / "25" %x30-35 ; 250-255 > > > > isValidIPv4() method verifies that the address satisfies the above mentioned > > grammar. Unfortunately, I couldn't find any existing implementation that does > > it. I have added comments to the method to emphasize the required format. > > (This comment really belongs on isValidHostNameForPinning in CronetEngine.java.) > My concern here isn't that isValidIPv4 doesn't do what it's name says (I believe > that this method correctly checks whether the provided string matches > IPv4address as defined in RFC 3986). Instead, my concern is that the combination > of isValidHostname && !isValidIPv4 will let through something that isn't a valid > IP address, but can be interpreted as one. The existing native code uses > https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=443&... > to check if the host of the url for a request is an IP address, which allows for > all sorts of awful things not in RFC 3986. > > Even if we accept an IP address-like hostname, it won't be used for pinning > because 1) it's not canonicalized, and 2) (more importantly) a request coming in > for an IP will never be checked against the pin set. isValidHostNameForPinning > should fail so that addPublicKeyPins will throw an exception if an IP > address-like hostname is passed in. Maybe the IP address check should be relaxed > to something like ^[0-9.]*$ for hostnames to reject? That regex is very ad-hoc, > but I can't think of anything that would match it that could actually be a valid > domain (unless ICANN starts making gTLDs that consist of only digits). It is a good point. I agree that we should make the hostname validation more restrictive since we know that IPv4-like hostnames are not going to work (even if they are perfectly valid according to RFC 3986). I will fix it and add more tests that reject host names like '2130706433', '0177.0.0.1', etc. I guess the suggested ^[0-9.]*$ expression should be good. https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:357: pkp.put(CronetEngineBuilderList.PKP_HOST, hostName); On 2015/11/20 19:02:47, nharper wrote: > At some point, hostName needs to undergo IDN processing. > TransportSecurityState::AddHPKP (called in > cronet_url_request_context_adapter.cc) expects that its input has already > undergone IDN processing. Nice catch. I will add a resolution of an internationalized domain to our Cronet test server and will add a PKP test for that domain.
Hi Nick, I have addressed the comments. Please see the remarks below. Thanks, Andrei https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetUtil.java (right): https://codereview.chromium.org/1407263010/diff/300001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetUtil.java:53: return VALID_IP_PATTERN.matcher(addr).matches(); On 2015/11/20 20:40:09, kapishnikov wrote: > On 2015/11/20 19:02:47, nharper wrote: > > On 2015/11/20 16:38:03, kapishnikov wrote: > > > On 2015/11/19 23:47:54, nharper wrote: > > > > Chrome interprets plenty of things that don't match that regex as IPv4 > > > > addresses. The following all resolve to 127.0.0.1: > > > > > > > > 2130706433 > > > > 0177.0.0.1 > > > > 127.0.00.1 > > > > > > > > Instead of reimplementing the isValidHostName and isValidIPv4 checks here, > > can > > > > we reuse the code that already exists? > > > > > > To validate the host name, isValidHostName() method is using IDN class that > > was > > > in Android since API 9. > > > > > > Regarding IPv4 validation. RFC 3986 Section 3.2.2. Host says: > > > > > > > > > A host identified by an IPv4 literal address is represented in > > > dotted-decimal notation (a sequence of four decimal numbers in the > > > range 0 to 255, separated by "."), as described in [RFC1123] by > > > reference to [RFC0952]. Note that other forms of dotted notation may > > > be interpreted on some platforms, as described in Section 7.4, but > > > only the dotted-decimal form of four octets is allowed by this > > > grammar. > > > > > > IPv4address = dec-octet "." dec-octet "." dec-octet "." dec-octet > > > > > > dec-octet = DIGIT ; 0-9 > > > / %x31-39 DIGIT ; 10-99 > > > / "1" 2DIGIT ; 100-199 > > > / "2" %x30-34 DIGIT ; 200-249 > > > / "25" %x30-35 ; 250-255 > > > > > > isValidIPv4() method verifies that the address satisfies the above mentioned > > > grammar. Unfortunately, I couldn't find any existing implementation that > does > > > it. I have added comments to the method to emphasize the required format. > > > > (This comment really belongs on isValidHostNameForPinning in > CronetEngine.java.) > > My concern here isn't that isValidIPv4 doesn't do what it's name says (I > believe > > that this method correctly checks whether the provided string matches > > IPv4address as defined in RFC 3986). Instead, my concern is that the > combination > > of isValidHostname && !isValidIPv4 will let through something that isn't a > valid > > IP address, but can be interpreted as one. The existing native code uses > > > https://code.google.com/p/chromium/codesearch#chromium/src/url/gurl.cc&l=443&... > > to check if the host of the url for a request is an IP address, which allows > for > > all sorts of awful things not in RFC 3986. > > > > Even if we accept an IP address-like hostname, it won't be used for pinning > > because 1) it's not canonicalized, and 2) (more importantly) a request coming > in > > for an IP will never be checked against the pin set. isValidHostNameForPinning > > should fail so that addPublicKeyPins will throw an exception if an IP > > address-like hostname is passed in. Maybe the IP address check should be > relaxed > > to something like ^[0-9.]*$ for hostnames to reject? That regex is very > ad-hoc, > > but I can't think of anything that would match it that could actually be a > valid > > domain (unless ICANN starts making gTLDs that consist of only digits). > > It is a good point. I agree that we should make the hostname validation more > restrictive since we know that IPv4-like hostnames are not going to work (even > if they are perfectly valid according to RFC 3986). I will fix it and add more > tests that reject host names like '2130706433', '0177.0.0.1', etc. I guess the > suggested ^[0-9.]*$ expression should be good. Done with corresponding tests in PkpTest.java. I have deleted CronetUtil and CronetUtilTest classes since they are not needed anymore. https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/360001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:357: pkp.put(CronetEngineBuilderList.PKP_HOST, hostName); On 2015/11/20 20:40:10, kapishnikov wrote: > On 2015/11/20 19:02:47, nharper wrote: > > At some point, hostName needs to undergo IDN processing. > > TransportSecurityState::AddHPKP (called in > > cronet_url_request_context_adapter.cc) expects that its input has already > > undergone IDN processing. > > Nice catch. I will add a resolution of an internationalized domain to our Cronet > test server and will add a PKP test for that domain. I have added IDN processing of the hostname to validateHostNameForPinningAndConvert() method. There is a bug that prevents writing a corresponding unit test: https://code.google.com/p/chromium/issues/detail?id=559343
https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:409: if (INVALID_PKP_HOST_NAME.matcher(hostName).matches()) { Is this enough to cover testHostNameArgumentValidation() or are those covered by IDN.toASCII? https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:26: // Populates |out_hash_value| with the SHA256 hash of the cert public key. nit: cert -> |cert| https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... File net/android/javatests/src/org/chromium/net/X509UtilTest.java (right): https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... net/android/javatests/src/org/chromium/net/X509UtilTest.java:35: if (bytesRead != bytes.length) { nit: no {} around single line in C++.
https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:409: if (INVALID_PKP_HOST_NAME.matcher(hostName).matches()) { On 2015/11/23 19:45:37, mef wrote: > Is this enough to cover testHostNameArgumentValidation() or are those covered by > IDN.toASCII? INVALID_PKP_HOST_NAME matcher discards host names that contain only digits and the dot characters. This covers different variations of IPv4 addresses, e.g. 2130706433, 0177.0.0.1, 127.0.00.1. Those are not valid IPv4 addresses according to RFC 3986 Section 3.2.2; however, the native part will treat them as such. IDN.toASCII does the rest of the validation, e.g. ensures the length of the labels and the host, valid characters, etc. Thus, both parts should be present in order for testHostNameArgumentValidation() to pass. https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:26: // Populates |out_hash_value| with the SHA256 hash of the cert public key. On 2015/11/23 19:45:37, mef wrote: > nit: cert -> |cert| Will fix. https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... File net/android/javatests/src/org/chromium/net/X509UtilTest.java (right): https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... net/android/javatests/src/org/chromium/net/X509UtilTest.java:35: if (bytesRead != bytes.length) { On 2015/11/23 19:45:37, mef wrote: > nit: no {} around single line in C++. I guess this does not apply to Java code. The style guide says: We require braces around the statements for a conditional. Except, if the entire conditional (the condition and the body) fit on one line, you may (but are not obligated to) put it all on one line. That is, this is legal: if (condition) { body(); }
lgtm mod nit. https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:409: if (INVALID_PKP_HOST_NAME.matcher(hostName).matches()) { On 2015/11/23 21:13:35, kapishnikov wrote: > On 2015/11/23 19:45:37, mef wrote: > > Is this enough to cover testHostNameArgumentValidation() or are those covered > by > > IDN.toASCII? > > INVALID_PKP_HOST_NAME matcher discards host names that contain only digits and > the dot characters. This covers different variations of IPv4 addresses, e.g. > 2130706433, 0177.0.0.1, 127.0.00.1. Those are not valid IPv4 addresses according > to RFC 3986 Section 3.2.2; however, the native part will treat them as such. > > IDN.toASCII does the rest of the validation, e.g. ensures the length of the > labels and the host, valid characters, etc. > > Thus, both parts should be present in order for testHostNameArgumentValidation() > to pass. Cool! https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... File net/android/javatests/src/org/chromium/net/X509UtilTest.java (right): https://codereview.chromium.org/1407263010/diff/400001/net/android/javatests/... net/android/javatests/src/org/chromium/net/X509UtilTest.java:35: if (bytesRead != bytes.length) { On 2015/11/23 21:13:36, kapishnikov wrote: > On 2015/11/23 19:45:37, mef wrote: > > nit: no {} around single line in C++. > > I guess this does not apply to Java code. The style guide says: > > We require braces around the statements for a conditional. Except, if the entire > conditional (the condition and the body) fit on one line, you may (but are not > obligated to) put it all on one line. That is, this is legal: > > if (condition) { > body(); > } Oops you're right. NVM.
tiny nit https://codereview.chromium.org/1407263010/diff/400001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/1407263010/diff/400001/net/http/transport_sec... net/http/transport_security_state.h:296: // Note: This method will persist the HPKP if |Delegate| is set. Make sure |Delegate| is set->a Delegate is present (without "a", |Delegate| sounds like it's referring to a variable named Delegate. My rewrite matches line 349 comment)
https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... File components/cronet/android/test/mock_cert_verifier.cc (right): https://codereview.chromium.org/1407263010/diff/400001/components/cronet/andr... components/cronet/android/test/mock_cert_verifier.cc:26: // Populates |out_hash_value| with the SHA256 hash of the cert public key. On 2015/11/23 21:13:36, kapishnikov wrote: > On 2015/11/23 19:45:37, mef wrote: > > nit: cert -> |cert| > > Will fix. Done. https://codereview.chromium.org/1407263010/diff/400001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/1407263010/diff/400001/net/http/transport_sec... net/http/transport_security_state.h:296: // Note: This method will persist the HPKP if |Delegate| is set. Make sure On 2015/11/30 18:18:31, pauljensen wrote: > |Delegate| is set->a Delegate is present > (without "a", |Delegate| sounds like it's referring to a variable named > Delegate. My rewrite matches line 349 comment) Done.
https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:315: * Adds public key pins for a given host. I think we need to make this a lot more verbose. I had no idea what a "public key pin" was until recently. Maybe something along the lines of: Pins a set of public keys for a given host. By pinning a set of public keys, {@code pinsSha256}, communication with {@code hostName} is required to authenticate with a certificate with a public key from the set of pinned ones. Authentication will fail and secure communication will not be established otherwise, even if the host attempts to authenticate with a certificate allowed by the device's trusted store of certificates. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:315: * Adds public key pins for a given host. Do we allow intermediate certificates signed by the pinned certificate? https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:315: * Adds public key pins for a given host. This would probably be a good place to include a link to the relevant RFC in case embedders want to read more about how it works. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:318: * consists of digits and the dot character only is treated as invalid. nit: of digits and the dot character only->of only digits and the dot character https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:318: * consists of digits and the dot character only is treated as invalid. Actually I think we might want to get rid of this sentence as it's a little unexplained, you could replace it with something saying IP addresses are not allowed. At a minimum we should explain why they're explicitly unallowed. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:320: * hash of DER-encoded ASN.1 representation of Subject Public of->of the https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:321: * Key Info (SPKI) of the host X.509 certificate. Use host->host's https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * Certificate.getPublicKey} and }->()} https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * {@link java.security.Key#getEncoded() Key.getEncoded} ditto https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:325: * to obtain DER-encoded ASN.1 representation of SPKI. SPKI->the SPKI https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:328: * @param expirationDate specifies the expiration date for the pins. What happens when they expire? do we fall back to allowing certs that the device's trusted store allows? https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * @throws NullPointerException if one of the input parameters is null. null->{@code null} https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * @throws NullPointerException if one of the input parameters is null. one->any is->are https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:331: * @throws IllegalArgumentException if the given host name is invalid or the or the->or https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:332: * {@code pinsSha256} collection contains a byte array collection contains->contains https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:342: } null check expirationDate also, and test https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:353: Set<String> hashes = new HashSet<>(pinsSha256.size()); I assume you're using HashSet to eliminate duplicates? Might want to add a comment mentioning this implicit action. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:391: auto hash_value = net::HashValue(net::HASH_VALUE_SHA256); is there any reason to explicitly initialize here? Perhaps just: net::HashValue hash_value; https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:392: bool good_hash = hash_value.FromString(*hash); I think this might allow SHA1 values, can we verify it's SHA256? https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:324: NativeTestServer.registerHostResolverProc(urlRequestContextAdapter, false); Should we move this to CronetTestBase as it looks like it's duplicated in other tests? https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... components/cronet/url_request_context_config.cc:140: URLRequestContextConfig::Pkp::Pkp() {} add newline between braces to match line 121 https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... components/cronet/url_request_context_config.cc:142: URLRequestContextConfig::Pkp::~Pkp() {} ditto https://codereview.chromium.org/1407263010/diff/420001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/1407263010/diff/420001/net/http/transport_sec... net/http/transport_security_state.h:297: // that the delegate is |NULL| if the persistence is not desired. |NULL|->nullptr
https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/api/src/org/chromium/net/CronetEngine.java (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:315: * Adds public key pins for a given host. On 2015/12/01 20:50:12, pauljensen wrote: > Do we allow intermediate certificates signed by the pinned certificate? Yes, we do allow intermediate certificates signed by the pinned certificate. That is according to the spec. This Wikipedia page (https://en.wikipedia.org/wiki/HTTP_Public_Key_Pinning) nicely summarizes RFC 7469 Section 4 - Security Considerations: "A website operator can choose to either pin the root certificate public key of a particular root certificate authority, allowing only that certificate authority (and all intermediate authorities signed by its key) to issue valid certificates for the website's domain, and/or to pin the key(s) of one or more intermediate issuing certificates, or to pin the end-entity public key." What we don't mandate is having the backup public key, i.e. a key that does not present in the server's certificate chain. When a user agent gets an HTTP response from a server that contains pinning request, it should verify that there is the backup key. Since Cronet is a programmatic API that is called before any response from the server is received, we cannot check that the user supplied a backup key to addPublicKeyPins() method. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:315: * Adds public key pins for a given host. On 2015/12/01 20:50:12, pauljensen wrote: > This would probably be a good place to include a link to the relevant RFC in > case embedders want to read more about how it works. Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:318: * consists of digits and the dot character only is treated as invalid. On 2015/12/01 20:50:13, pauljensen wrote: > nit: of digits and the dot character only->of only digits and the dot character Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:318: * consists of digits and the dot character only is treated as invalid. On 2015/12/01 20:50:13, pauljensen wrote: > Actually I think we might want to get rid of this sentence as it's a little > unexplained, you could replace it with something saying IP addresses are not > allowed. At a minimum we should explain why they're explicitly unallowed. The problem here is that the API not only restricts IP addresses but also some valid host names that are interpreted as IPv4 addresses by the native implementation. E.g. host name "2130706433" is perfectly valid according to RFC 3986 Section 3.2.2. however it will be translated to 127.0.0.1 IPv4 address by the native code. If we don't want to overload the user API with internal implementation details, it should be reasonable just to state this limitation without going into details. I assume that in practice the host names will contain a top-level-domain that cannot consist of digits, therefore, this limitation should not have significant impact. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:320: * hash of DER-encoded ASN.1 representation of Subject Public On 2015/12/01 20:50:12, pauljensen wrote: > of->of the Done. I also added comments regarding the backup key: "Although, the method does not mandate the presence of the backup pin that can be used if the control of the primary private key has been lost, it is highly recommended to supply one". https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:321: * Key Info (SPKI) of the host X.509 certificate. Use On 2015/12/01 20:50:13, pauljensen wrote: > host->host's Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:323: * Certificate.getPublicKey} and On 2015/12/01 20:50:13, pauljensen wrote: > }->()} Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:324: * {@link java.security.Key#getEncoded() Key.getEncoded} On 2015/12/01 20:50:12, pauljensen wrote: > ditto Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:325: * to obtain DER-encoded ASN.1 representation of SPKI. On 2015/12/01 20:50:12, pauljensen wrote: > SPKI->the SPKI Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:328: * @param expirationDate specifies the expiration date for the pins. On 2015/12/01 20:50:12, pauljensen wrote: > What happens when they expire? do we fall back to allowing certs that the > device's trusted store allows? Yes, that is correct. When the pins expire, the client stops validating them. The device's trusted store is always used regardless whether the a host is pinned or not. Pinning is the extra layer of security on top of the trust store. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * @throws NullPointerException if one of the input parameters is null. On 2015/12/01 20:50:12, pauljensen wrote: > null->{@code null} Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:330: * @throws NullPointerException if one of the input parameters is null. On 2015/12/01 20:50:13, pauljensen wrote: > one->any > is->are Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:331: * @throws IllegalArgumentException if the given host name is invalid or the On 2015/12/01 20:50:12, pauljensen wrote: > or the->or Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:332: * {@code pinsSha256} collection contains a byte array On 2015/12/01 20:50:12, pauljensen wrote: > collection contains->contains Done. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:342: } On 2015/12/01 20:50:12, pauljensen wrote: > null check expirationDate also, and test Done. Added tests to check the exception when any of the parameters is null. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/api/src/org/chromium/net/CronetEngine.java:353: Set<String> hashes = new HashSet<>(pinsSha256.size()); On 2015/12/01 20:50:13, pauljensen wrote: > I assume you're using HashSet to eliminate duplicates? Might want to add a > comment mentioning this implicit action. Good point. From the client point of view it does not really matter if we remove duplicate pins or now. Pinning a host to the same pin second time doesn't change anything. However, we remove duplicates as the optimization step. I agree that we should indicate to the client that there should be no duplicates; therefore, I have changed the signature of the method to accept a set instead of the collection. I think it is semantically more correct even though in HTTP PKP the pins are provided in a form of a list. Also added the following related statement to the method: "Calling this method multiple times with the same host name overrides the previously set pins for the host." https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/cronet_url_request_context_adapter.cc (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:391: auto hash_value = net::HashValue(net::HASH_VALUE_SHA256); On 2015/12/01 20:50:13, pauljensen wrote: > is there any reason to explicitly initialize here? Perhaps just: > net::HashValue hash_value; Done. I wanted to be explicit here that the hash value is SHA256. The actual hash type is retrieved from the string prefix. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/cronet_url_request_context_adapter.cc:392: bool good_hash = hash_value.FromString(*hash); On 2015/12/01 20:50:13, pauljensen wrote: > I think this might allow SHA1 values, can we verify it's SHA256? Yes, the validation already takes place in CronetEngine.Builder. Technically, the native code allows SHA1 pins but the CronetEngine.Builder will not allow to create pins with SHA1. It is too late to validate it here. Failing it here means failing the initialization of the request context. I have added tests to PkpTest to validate scenario when the client passes SHA1 as the pin value. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... File components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/andr... components/cronet/android/test/javatests/src/org/chromium/net/PkpTest.java:324: NativeTestServer.registerHostResolverProc(urlRequestContextAdapter, false); On 2015/12/01 20:50:13, pauljensen wrote: > Should we move this to CronetTestBase as it looks like it's duplicated in other > tests? It is a good idea. The similar logic is used in SdchTest, QuicTest and PkpTest. We can refactor the code in a separate CL to keep this CL smaller. https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... File components/cronet/url_request_context_config.cc (right): https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... components/cronet/url_request_context_config.cc:140: URLRequestContextConfig::Pkp::Pkp() {} On 2015/12/01 20:50:13, pauljensen wrote: > add newline between braces to match line 121 I am running "git cl format" and it moves the closing bracket back. It is actually according to the Code Style Guide: https://google.github.io/styleguide/cppguide.html#Class_Format https://codereview.chromium.org/1407263010/diff/420001/components/cronet/url_... components/cronet/url_request_context_config.cc:142: URLRequestContextConfig::Pkp::~Pkp() {} On 2015/12/01 20:50:13, pauljensen wrote: > ditto Same here. https://codereview.chromium.org/1407263010/diff/420001/net/http/transport_sec... File net/http/transport_security_state.h (right): https://codereview.chromium.org/1407263010/diff/420001/net/http/transport_sec... net/http/transport_security_state.h:297: // that the delegate is |NULL| if the persistence is not desired. On 2015/12/01 20:50:13, pauljensen wrote: > |NULL|->nullptr Done.
lgtm. Andrei and I talked offline and he uploaded a patch set addressing my residual comments.
The CQ bit was checked by kapishnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from davidben@chromium.org, mef@chromium.org Link to the patchset: https://codereview.chromium.org/1407263010/#ps460001 (title: "Small changes and rebase")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1407263010/460001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1407263010/460001
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. BUG=522275 ========== to ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. BUG=522275 ==========
Message was sent while issue was closed.
Committed patchset #24 (id:460001)
Message was sent while issue was closed.
Description was changed from ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. BUG=522275 ========== to ========== [Cronet] Public key pinning for Java API Implements new public-key-pinning Java API (see https://tools.ietf.org/html/rfc7469). The API is accessible through CronetEngine.Builder.addPublicKeyPins() method. The method accepts the host, the a collection of pin hashes, the flag that indicates whether the pinning policy should be applied to the host subdomains and the pin expiration date. The pins are not persisted between client restarts. BUG=522275 Committed: https://crrev.com/df5ccabbdbf77c68a0b0e2ee2b83091054f883f6 Cr-Commit-Position: refs/heads/master@{#363017} ==========
Message was sent while issue was closed.
Patchset 24 (id:??) landed as https://crrev.com/df5ccabbdbf77c68a0b0e2ee2b83091054f883f6 Cr-Commit-Position: refs/heads/master@{#363017} |
