Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1417363004:
Verify certificate of Privet v3 device (Closed)
|
Created:
5 years, 2 months ago by Vitaly Buka (NO REVIEWS) Modified:
5 years, 1 month ago CC:
chromium-reviews Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionVerify certificate of Privet v3 device
Privet v3 uses https to secure communication with device.
Device has self-signed certificate so we can't use normal
certificate verification. Certificate is validated by
matching fingerprints with value received during pairing
process.
During pairing process device sends certificate fingerprint
using insecure channel. Fingerprint is signed with secret
generated during pairing session.
BUG=524788
Committed: https://crrev.com/3d39db84f919575d1399b62f76d17566632b0049
Cr-Commit-Position: refs/heads/master@{#355994}
Patch Set 1 #Patch Set 2 : Fri Oct 23 12:10:43 PDT 2015 #Patch Set 3 : Fri Oct 23 12:49:42 PDT 2015 #Patch Set 4 : Fri Oct 23 14:58:42 PDT 2015 #
Total comments: 8
Patch Set 5 : Fri Oct 23 15:17:36 PDT 2015 #Patch Set 6 : Fri Oct 23 15:23:44 PDT 2015 #Patch Set 7 : Fri Oct 23 18:12:45 PDT 2015 #Patch Set 8 : Fri Oct 23 18:16:41 PDT 2015 #Patch Set 9 : #Patch Set 10 : #Patch Set 11 : #
Total comments: 26
Patch Set 12 : #Patch Set 13 : #
Total comments: 2
Patch Set 14 : #Patch Set 15 : #
Total comments: 5
Patch Set 16 : #Patch Set 17 : #Patch Set 18 : #Patch Set 19 : #Messages
Total messages: 56 (22 generated)
The CQ bit was checked by vitalybuka@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/1417363004/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/1
Description was changed from ========== Thu Oct 22 21:22:39 PDT 2015 BUG= ========== to ========== Verify certificate of Privet v3 device Privet v3 uses https to secure communication with device. Device has self-signed certificate so we can't use normal certificate verification. Certificate is validated by matching fingerprints with value received during pairing process. During pairing process device sends certificate fingerprint using insecure channel. Fingerprint is signed with secret generated during pairing session. BUG=524788 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitalybuka@chromium.org to run a CQ dry run
vitalybuka@chromium.org changed reviewers: + alekseys@chromium.org, avakulenko@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417363004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/40001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_gn_chromeos_rel on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by vitalybuka@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/1417363004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/60001
https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... File chrome/browser/local_discovery/privetv3_session.cc (right): https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:337: if (fingerprint.size() != arraysize(hash.data)) { this should be sizeof() https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:338: LOG(ERROR) << "Infalid fingerprint size: " << response; Invalid https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:341: memcpy(hash.data, fingerprint.data(), arraysize(hash.data)); and this https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... File chrome/browser/local_discovery/privetv3_session.h (right): https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.h:112: int https_port_ = 0; uint16_t
https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... File chrome/browser/local_discovery/privetv3_session.cc (right): https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:337: if (fingerprint.size() != arraysize(hash.data)) { On 2015/10/23 22:14:44, Alex Vakulenko wrote: > this should be sizeof() Done. https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:338: LOG(ERROR) << "Infalid fingerprint size: " << response; On 2015/10/23 22:14:44, Alex Vakulenko wrote: > Invalid Done. https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.cc:341: memcpy(hash.data, fingerprint.data(), arraysize(hash.data)); On 2015/10/23 22:14:44, Alex Vakulenko wrote: > and this Done. https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... File chrome/browser/local_discovery/privetv3_session.h (right): https://codereview.chromium.org/1417363004/diff/60001/chrome/browser/local_di... chrome/browser/local_discovery/privetv3_session.h:112: int https_port_ = 0; On 2015/10/23 22:14:44, Alex Vakulenko wrote: > uint16_t Done.
The CQ bit was checked by vitalybuka@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/1417363004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/80001
lgtm
The CQ bit was checked by vitalybuka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alekseys@chromium.org Link to the patchset: https://codereview.chromium.org/1417363004/#ps140001 (title: "Fri Oct 23 18:16:41 PDT 2015")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417363004/140001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/140001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalybuka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alekseys@chromium.org Link to the patchset: https://codereview.chromium.org/1417363004/#ps160001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417363004/160001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/160001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...)
The CQ bit was checked by vitalybuka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alekseys@chromium.org Link to the patchset: https://codereview.chromium.org/1417363004/#ps180001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417363004/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/180001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by vitalybuka@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from alekseys@chromium.org Link to the patchset: https://codereview.chromium.org/1417363004/#ps200001 (title: " ")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1417363004/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1417363004/200001
Message was sent while issue was closed.
Committed patchset #11 (id:200001)
Message was sent while issue was closed.
Patchset 11 (id:??) landed as https://crrev.com/3d39db84f919575d1399b62f76d17566632b0049 Cr-Commit-Position: refs/heads/master@{#355994}
Message was sent while issue was closed.
Description was changed from ========== Verify certificate of Privet v3 device Privet v3 uses https to secure communication with device. Device has self-signed certificate so we can't use normal certificate verification. Certificate is validated by matching fingerprints with value received during pairing process. During pairing process device sends certificate fingerprint using insecure channel. Fingerprint is signed with secret generated during pairing session. BUG=524788 Committed: https://crrev.com/3d39db84f919575d1399b62f76d17566632b0049 Cr-Commit-Position: refs/heads/master@{#355994} ========== to ========== Verify certificate of Privet v3 device Privet v3 uses https to secure communication with device. Device has self-signed certificate so we can't use normal certificate verification. Certificate is validated by matching fingerprints with value received during pairing process. During pairing process device sends certificate fingerprint using insecure channel. Fingerprint is signed with secret generated during pairing session. BUG=524788 Committed: https://crrev.com/3d39db84f919575d1399b62f76d17566632b0049 Cr-Commit-Position: refs/heads/master@{#355994} ==========
Message was sent while issue was closed.
vitalybuka@chromium.org changed reviewers: + rsleevi@chromium.org
Message was sent while issue was closed.
Ryan, would you like to take a look at chrome/browser/local_discovery/privet_http_impl.cc
Message was sent while issue was closed.
rsleevi@chromium.org changed reviewers: + mmenke@chromium.org
Message was sent while issue was closed.
Is there a launch bug for this? This strikes me as very security-sensitive and should be done with care. CC'ing mmenke to review the URLRequestContext stuff, which doesn't look right to me, but Matt's far more authoritative on those. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_impl.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:102: verify_result->verified_cert = cert; BUG: You fail to reset the verify_result, causing old results to be potentially re-used. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:109: // Privet v3 devices should supports https but with self-signed sertificates. TYPO: certificates https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:112: // client side Spake2 on insecure channel. Than device sends fingerprint typo: SPAKE2 // Before using HTTPS, Privet v3 pairing generates a shared secret using // SPAKE2 over an insecure channel. Then the device sends the fingerprint // to the client, over the insecure channel, but signed using the // shared secret. // For more information on paring: // https:// (The comments at present have spelling and grammar issues) https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:117: class FingerprintVerifier : public net::CertVerifier { Your class documentation doesn't explain at all what's going on here; it's entirely unrelated to the FingerprintVerifier, which simply verifies that fingerprints match what are expected. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:174: return context_.get(); This is all VERY surprising; creating new URL request contexts should be a very rare thing. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:861: context_getter_->GetNetworkTaskRunner(), certificate_fingerprint); I'm fairly certain this is all sorts of wrong re: lifetimes of URLRequestContexts and friends. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1160: https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, svaldez@ has been working very hard to get rid of instances of SpawnedTestServer in favour of EmbeddedTestServer, which seems totally acceptable for these tests. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1191: // TODO(vitalybuka): Remove #ifdef when test server is fixed. Where's the bug for this? This seems sort of lackadasical to just disable tests on one platform w/o any documentation about bug or follow-up. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_url_fetcher.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:110: DCHECK(tries_ == 0); DCHECK_EQ https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:157: DVLOG(1) << "Try no. " << tries_; Why abbreviate number in a subtle way like this? https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:162: net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SEND_COOKIES); did "git cl format" do this? It doesn't seem consistent w/ how it does.
Message was sent while issue was closed.
Not yet. We are still building this feature. API is used only by single internal application. But we don't open HTTP traffic. On 2015/10/28 22:52:16, Ryan Sleevi wrote: > Is there a launch bug for this? This strikes me as very security-sensitive and > should be done with care. > > CC'ing mmenke to review the URLRequestContext stuff, which doesn't look right to > me, but Matt's far more authoritative on those. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > File chrome/browser/local_discovery/privet_http_impl.cc (right): > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:102: > verify_result->verified_cert = cert; > BUG: You fail to reset the verify_result, causing old results to be potentially > re-used. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:109: // Privet v3 devices > should supports https but with self-signed sertificates. > TYPO: certificates > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:112: // client side Spake2 on > insecure channel. Than device sends fingerprint > typo: SPAKE2 > > // Before using HTTPS, Privet v3 pairing generates a shared secret using > // SPAKE2 over an insecure channel. Then the device sends the fingerprint > // to the client, over the insecure channel, but signed using the > // shared secret. > // For more information on paring: > // https:// > > (The comments at present have spelling and grammar issues) > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:117: class > FingerprintVerifier : public net::CertVerifier { > Your class documentation doesn't explain at all what's going on here; it's > entirely unrelated to the FingerprintVerifier, which simply verifies that > fingerprints match what are expected. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:174: return context_.get(); > This is all VERY surprising; creating new URL request contexts should be a very > rare thing. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:861: > context_getter_->GetNetworkTaskRunner(), certificate_fingerprint); > I'm fairly certain this is all sorts of wrong re: lifetimes of > URLRequestContexts and friends. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > File chrome/browser/local_discovery/privet_http_unittest.cc (right): > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_unittest.cc:1160: > https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, > svaldez@ has been working very hard to get rid of instances of SpawnedTestServer > in favour of EmbeddedTestServer, which seems totally acceptable for these tests. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_unittest.cc:1191: // > TODO(vitalybuka): Remove #ifdef when test server is fixed. > Where's the bug for this? This seems sort of lackadasical to just disable tests > on one platform w/o any documentation about bug or follow-up. > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > File chrome/browser/local_discovery/privet_url_fetcher.cc (right): > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_url_fetcher.cc:110: DCHECK(tries_ == 0); > DCHECK_EQ > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_url_fetcher.cc:157: DVLOG(1) << "Try no. " > << tries_; > Why abbreviate number in a subtle way like this? > > https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_url_fetcher.cc:162: > net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SEND_COOKIES); > did "git cl format" do this? It doesn't seem consistent w/ how it does.
Message was sent while issue was closed.
I'll re-open this issue to simplify review. I'd like to avoid reverting right now as it will switch back communication to HTTP.
https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_impl.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:102: verify_result->verified_cert = cert; On 2015/10/28 22:52:15, Ryan Sleevi wrote: > BUG: You fail to reset the verify_result, causing old results to be potentially > re-used. That one was copied from ssl_hmac_channel_authenticator.cc https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:102: verify_result->verified_cert = cert; On 2015/10/28 22:52:15, Ryan Sleevi wrote: > BUG: You fail to reset the verify_result, causing old results to be potentially > re-used. Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:109: // Privet v3 devices should supports https but with self-signed sertificates. On 2015/10/28 22:52:15, Ryan Sleevi wrote: > TYPO: certificates Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:112: // client side Spake2 on insecure channel. Than device sends fingerprint On 2015/10/28 22:52:15, Ryan Sleevi wrote: > typo: SPAKE2 > > // Before using HTTPS, Privet v3 pairing generates a shared secret using > // SPAKE2 over an insecure channel. Then the device sends the fingerprint > // to the client, over the insecure channel, but signed using the > // shared secret. > // For more information on paring: > // https:// > > (The comments at present have spelling and grammar issues) Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:117: class FingerprintVerifier : public net::CertVerifier { On 2015/10/28 22:52:15, Ryan Sleevi wrote: > Your class documentation doesn't explain at all what's going on here; it's > entirely unrelated to the FingerprintVerifier, which simply verifies that > fingerprints match what are expected. Moved large part of comment to SwitchToHttps to keep only relevant info here. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:174: return context_.get(); On 2015/10/28 22:52:15, Ryan Sleevi wrote: > This is all VERY surprising; creating new URL request contexts should be a very > rare thing. I create one context per context getter and two context getters per device. One before certificate and one after we have certificate. I can have one getter for entire local discovery. But I don't think performance is critical here. With shared getter we will need share Verifier between devices. So we will have some way to pick correct fingerprint. This will make code more complicated without known benefits. Also it's not clear what is reused inside of context, or will be reused in future. Seems having context per device is more secure. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:861: context_getter_->GetNetworkTaskRunner(), certificate_fingerprint); On 2015/10/28 22:52:15, Ryan Sleevi wrote: > I'm fairly certain this is all sorts of wrong re: lifetimes of > URLRequestContexts and friends. Did you mean performance? Two getters per device? https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1160: https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, I didn't know that. Looked up this somewhere else. I'll try to switch to EmbeddedTestServer. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1191: // TODO(vitalybuka): Remove #ifdef when test server is fixed. On 2015/10/28 22:52:15, Ryan Sleevi wrote: > Where's the bug for this? This seems sort of lackadasical to just disable tests > on one platform w/o any documentation about bug or follow-up. Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_url_fetcher.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:110: DCHECK(tries_ == 0); On 2015/10/28 22:52:16, Ryan Sleevi wrote: > DCHECK_EQ Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:157: DVLOG(1) << "Try no. " << tries_; On 2015/10/28 22:52:15, Ryan Sleevi wrote: > Why abbreviate number in a subtle way like this? Done. https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_url_fetcher.cc:162: net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SEND_COOKIES); On 2015/10/28 22:52:15, Ryan Sleevi wrote: > did "git cl format" do this? It doesn't seem consistent w/ how it does. yes, it's git cl format
https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1160: https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, EmbeddedTestServer does not support HTTPS.
https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1160: https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, On 2015/10/29 02:01:58, Vitaly Buka wrote: > EmbeddedTestServer does not support HTTPS. Well....Fancy you should mention that. I guess you haven't updated your checkout in the last 6 hours, 53 minutes?
https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/200001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1160: https_server_.reset(new SpawnedTestServer(net::BaseTestServer::TYPE_HTTP, Done. Actually it was code search to blame :-) On 2015/10/29 02:35:11, mmenke wrote: > On 2015/10/29 02:01:58, Vitaly Buka wrote: > > EmbeddedTestServer does not support HTTPS. > > Well....Fancy you should mention that. I guess you haven't updated your > checkout in the last 6 hours, 53 minutes? https://codereview.chromium.org/1417363004/diff/240001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/240001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1181: EXPECT_EQ(PrivetURLFetcher::UNKNOWN_ERROR, error_); here request fails, as expected, but takes too long with EmbeddedTestServer. about 30sec on linux
https://codereview.chromium.org/1417363004/diff/240001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_unittest.cc (right): https://codereview.chromium.org/1417363004/diff/240001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_unittest.cc:1181: EXPECT_EQ(PrivetURLFetcher::UNKNOWN_ERROR, error_); New bug for that: https://code.google.com/p/chromium/issues/detail?id=548971
I defer to Ryan on security issues (Of which I have little knowledge), but if this is the approach you're planning to take, I think you want a security review sooner rather than later. Not fair to the security team to put them in the position of having to nix an approach after you've been building code on top of it for months. https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_impl.cc (right): https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:179: ~PrivetContextGetter() override = default; BUG: The URLRequestContext must be deleted on the network thread. I guess your tests are just using one thread? The URLRequestContext should have caught this, otherwise. Suggest adding a network thread to your tests, for just such issues. https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:808: context_getter_(new PrivetContextGetter(net_task_runner)), URLRequestContexts are very heavy weight objects. What's the lifetime of this object? How often are they created? It's not clear from the code.
On 2015/10/29 16:12:27, mmenke wrote: > I defer to Ryan on security issues (Of which I have little knowledge), but if > this is the approach you're planning to take, I think you want a security review > sooner rather than later. > > Not fair to the security team to put them in the position of having to nix an > approach after you've been building code on top of it for months. > I agree with that and sorry for this situation. How about if we cleanup this CL. Then for Chrome 49 I'll restructure the code to remove dependency of Privetv3Session on PrivetHttp. PrivetHttp was for local printing only and it's not pretend to be secure. PrivetHttp used by Privetv3Session only to create fetchers. Privetv3Session is for secure Weave stuff. After refactoring code will be much cleaner and we will do real security review.
https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_impl.cc (right): https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:179: ~PrivetContextGetter() override = default; On 2015/10/29 16:12:26, mmenke wrote: > BUG: The URLRequestContext must be deleted on the network thread. I guess your > tests are just using one thread? The URLRequestContext should have caught this, > otherwise. Suggest adding a network thread to your tests, for just such issues. That's not what I see from code: https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... context_ is deleted with PrivetContextGetter. PrivetContextGetter is derived from net::URLRequestContextGetter which always deletes itself on network thread. Test uses real IO thread and main thread, adding DCHECK(net_task_runner_->BelongsToCurrentThread()); does not trigger any issues https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:808: context_getter_(new PrivetContextGetter(net_task_runner)), On 2015/10/29 16:12:26, mmenke wrote: > URLRequestContexts are very heavy weight objects. What's the lifetime of this > object? How often are they created? It's not clear from the code. For PrivetHTTPClient printing functionality this can be created many times when we enumerate devices. But I don't need any custom context, browser context is OK. I undo this part. We need own context only to do HTTPS, and it's only happening during device setup. This happens once per SPAKE2 pinCode typed by user. So quite rare.
https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... File chrome/browser/local_discovery/privet_http_impl.cc (right): https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... chrome/browser/local_discovery/privet_http_impl.cc:179: ~PrivetContextGetter() override = default; On 2015/10/29 18:37:12, Vitaly Buka wrote: > On 2015/10/29 16:12:26, mmenke wrote: > > BUG: The URLRequestContext must be deleted on the network thread. I guess > your > > tests are just using one thread? The URLRequestContext should have caught > this, > > otherwise. Suggest adding a network thread to your tests, for just such > issues. > > > That's not what I see from code: > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > context_ is deleted with PrivetContextGetter. > PrivetContextGetter is derived from net::URLRequestContextGetter which always > deletes itself on network thread. > > Test uses real IO thread and main thread, adding > DCHECK(net_task_runner_->BelongsToCurrentThread()); does not trigger any issues You're right - I completely forgot about that magic (Now-a-days, none of the primary contents are owned by their getters).
On 2015/10/29 18:40:18, mmenke wrote: > https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... > File chrome/browser/local_discovery/privet_http_impl.cc (right): > > https://codereview.chromium.org/1417363004/diff/280001/chrome/browser/local_d... > chrome/browser/local_discovery/privet_http_impl.cc:179: ~PrivetContextGetter() > override = default; > On 2015/10/29 18:37:12, Vitaly Buka wrote: > > On 2015/10/29 16:12:26, mmenke wrote: > > > BUG: The URLRequestContext must be deleted on the network thread. I guess > > your > > > tests are just using one thread? The URLRequestContext should have caught > > this, > > > otherwise. Suggest adding a network thread to your tests, for just such > > issues. > > > > > > That's not what I see from code: > > > https://code.google.com/p/chromium/codesearch#chromium/src/net/url_request/ur... > > > > context_ is deleted with PrivetContextGetter. > > PrivetContextGetter is derived from net::URLRequestContextGetter which always > > deletes itself on network thread. > > > > Test uses real IO thread and main thread, adding > > DCHECK(net_task_runner_->BelongsToCurrentThread()); does not trigger any > issues > > You're right - I completely forgot about that magic (Now-a-days, none of the > primary contents are owned by their getters). Destroyed by their getters, rather (They're destroyed earlier, because of the whole ref counting thing, and they can't outlive the profile...anyways, not really relevant)
Updated that patch to hide HTTPS related code behind command line switch. In separate CL I will add chrome://flags to enable this feature. Please review and I'll replace landed one with this one.
I have uploaded original landed CL and closing this issue. Please continue on https://codereview.chromium.org/1415653004/ |
