Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2225483002:
[ios] Removed CertVerifierBlockAdapter. (Closed)
|
Created:
4 years, 4 months ago by Eugene But (OOO till 7-30) Modified:
4 years, 4 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, asvitkine+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
Description[ios] Removed CertVerifierBlockAdapter.
cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert
verification failure, so there is no reason to use CertVerifier for
web view cert verification.
Removing CertVerifier usage should have positive impact on battery
life (because second versification will not be performed) and simplify
the code (especially threading part).
BUG=603634
TEST=interstitials show correct reason of failure
Committed: https://crrev.com/bbaae2f91795c442ef62bdfe42be1d5ea75f7cc2
Cr-Commit-Position: refs/heads/master@{#412096}
Patch Set 1 #Patch Set 2 : Self review #
Total comments: 15
Patch Set 3 : Addressed review comments #Patch Set 4 : static CertVerifyProcIOS::GetCertFailureStatusFromTrust #Patch Set 5 : Fixed compilation #Patch Set 6 : Updated includes #
Total comments: 28
Patch Set 7 : Resolved review comments #Patch Set 8 : Addressed review comments #
Messages
Total messages: 49 (26 generated)
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. BUG=603634 ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 ==========
eugenebut@chromium.org changed reviewers: + rsleevi@chromium.org
https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:107: verify_result.verified_cert = cert_; Unused? https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:119: result.verified_cert = cert_; Unused? https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:132: result.verified_cert = cert_; Unused? https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:145: // Tests that allowCert:forHost:status: strips all intermidiate certs. typo: intermediate https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:152: result.verified_cert = cert_; Unused? https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:199: result.verified_cert = cert_; Unused? https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util.h File net/cert/sec_trust_util.h (right): https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util... net/cert/sec_trust_util.h:6: #define NET_CERT_SEC_TRUST_UTIL_H_ Why are you introducing this file? I'm particularly hostile towards _util files (because they grow and grow), and am curious why this can't/doesn't say where it was before. That is, it's unclear from the description & initial message, as well the preliminary review, the design necessity of splitting this out, or the alternatives considered. That's not to say it may not be warranted, but I'm trying to understand more the decision here, because ideally, we'd avoid it if possible :)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). GetFailureFromTrustProperties was extracted to a separate file, so it's not coupled with CertVerifyProcIOS. BUG=603634 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Thanks! Let me know if you want to remove sec_trust_util.* files. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:107: verify_result.verified_cert = cert_; On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Unused? Done. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:119: result.verified_cert = cert_; On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Unused? Done. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:132: result.verified_cert = cert_; On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Unused? Done. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:145: // Tests that allowCert:forHost:status: strips all intermidiate certs. On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > typo: intermediate Done. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:152: result.verified_cert = cert_; On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Unused? Done. https://codereview.chromium.org/2225483002/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller_unittest.mm:199: result.verified_cert = cert_; On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Unused? Done. https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util.h File net/cert/sec_trust_util.h (right): https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util... net/cert/sec_trust_util.h:6: #define NET_CERT_SEC_TRUST_UTIL_H_ On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > Why are you introducing this file? > > I'm particularly hostile towards _util files (because they grow and grow), and > am curious why this can't/doesn't say where it was before. That is, it's unclear > from the description & initial message, as well the preliminary review, the > design necessity of splitting this out, or the alternatives considered. > > That's not to say it may not be warranted, but I'm trying to understand more the > decision here, because ideally, we'd avoid it if possible :) I don't think that sec_trust_util.h will grow, because its name sets certain limitation to what should be added. Here are a few pros in extracting a separate file: - Clients will not need to include extra stuff (cert_verify_proc_ios.h has CertVerifyProcIOS which is not useful for crw_cert_verification_controller) - GetCertFailureStatusFromTrust does not conceptually belong to CertVerifyProcIOS class API If you strongly believe that GetCertFailureStatusFromTrust should live in cert_verify_proc_ios.h I will move it back there. Having it in a separate file breaks compilation (cert_verify_proc_ios is not compiled on other platforms probably because of ios suffix), so getting rid of sec_trust_util will make things simpler :)
https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util.h File net/cert/sec_trust_util.h (right): https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util... net/cert/sec_trust_util.h:6: #define NET_CERT_SEC_TRUST_UTIL_H_ On 2016/08/08 20:22:23, Eugene But wrote: > On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > I don't think that sec_trust_util.h will grow, because its name sets certain > limitation to what should be added. It deals with SecTrust, that's a pretty big API :) > Here are a few pros in extracting a separate file: > - Clients will not need to include extra stuff (cert_verify_proc_ios.h has > CertVerifyProcIOS which is not useful for crw_cert_verification_controller) Well, they still need to include extra stuff (just sec_trust_util now) > - GetCertFailureStatusFromTrust does not conceptually belong to > CertVerifyProcIOS class API That second one doesn't really feel like a 'pro'. Hidden behind it is the statement that you don't believe it belongs to CertVerifyProcIOS, but what I was trying to get at with the review feedback is that it's not clear to me why you feel that way. > If you strongly believe that GetCertFailureStatusFromTrust should live in > cert_verify_proc_ios.h I will move it back there. Having it in a separate file > breaks compilation (cert_verify_proc_ios is not compiled on other platforms > probably because of ios suffix), so getting rid of sec_trust_util will make > things simpler :) Put differently, there's still a piece of this question missing, and I'm having to try to glean it from the code, rather than your explanation :) I'm trying to understand why you need GetCertFailureStatusFromTrust. In the old world, it was in CertVerifyProcIOS, which did the work. crw_cert_verification_controller depended on CVPIOS to explain additional information about the failure. You're removing CVPIOS, and thus losing the ability to explain why the failure happened. It seems like you're now introducing this abstraction so that crw_c_v_c doesn't have to depend on CVPIOS (header), but it's unclear why you need to do that, or what you considered. The concern here, which I only briefly touched on: - _util files tend to grow - This doesn't have any indication it's iOS specific (SecTrust is also used on OS X, but we *wouldn't* want this method used there) - There's still an implicit tight coupling between crw_cert_verification_controller and CertVerifyProciOS (we *want* them to produce same output), but that coupling is now abstracted away through a util file that may not yield the same results. With respect to the crw_c_v_c changes, what is your goal? To simply map to a CertStatus/net_error code? Why (for the error bypassing?). Should we be using SecTrustSetExceptions/CopyExceptions instead, if we're going all-in for SecTrust? Is it because the same SSL error handler will be used by stuff that goes through crw_c_v_c and through CertVerifyProciOS (do we have such situations? should we?) There's a lot of design rationale hidden behind the design, and I'm just trying to better understand what your constraints were. My gut is that we could simply have a static method on CertVerifyProcIOS, which would at least express the coupling much better (in that crw_c_v_c would have to call that static method), but there's broader aspects to unwinding the code that I'm unclear of, and that this subtle change highlights :)
On 2016/08/08 20:44:18, Ryan Sleevi (slow) wrote: > https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util.h > File net/cert/sec_trust_util.h (right): > > https://codereview.chromium.org/2225483002/diff/20001/net/cert/sec_trust_util... > net/cert/sec_trust_util.h:6: #define NET_CERT_SEC_TRUST_UTIL_H_ > On 2016/08/08 20:22:23, Eugene But wrote: > > On 2016/08/08 17:47:37, Ryan Sleevi (slow) wrote: > > I don't think that sec_trust_util.h will grow, because its name sets certain > > limitation to what should be added. > > It deals with SecTrust, that's a pretty big API :) > > > Here are a few pros in extracting a separate file: > > - Clients will not need to include extra stuff (cert_verify_proc_ios.h has > > CertVerifyProcIOS which is not useful for crw_cert_verification_controller) > > Well, they still need to include extra stuff (just sec_trust_util now) > > > - GetCertFailureStatusFromTrust does not conceptually belong to > > CertVerifyProcIOS class API > > That second one doesn't really feel like a 'pro'. Hidden behind it is the > statement that you don't believe it belongs to CertVerifyProcIOS, but what I was > trying to get at with the review feedback is that it's not clear to me why you > feel that way. > > > If you strongly believe that GetCertFailureStatusFromTrust should live in > > cert_verify_proc_ios.h I will move it back there. Having it in a separate file > > breaks compilation (cert_verify_proc_ios is not compiled on other platforms > > probably because of ios suffix), so getting rid of sec_trust_util will make > > things simpler :) > > Put differently, there's still a piece of this question missing, and I'm having > to try to glean it from the code, rather than your explanation :) > > I'm trying to understand why you need GetCertFailureStatusFromTrust. > > In the old world, it was in CertVerifyProcIOS, which did the work. > crw_cert_verification_controller depended on CVPIOS to explain additional > information about the failure. > > You're removing CVPIOS, and thus losing the ability to explain why the failure > happened. It seems like you're now introducing this abstraction so that > crw_c_v_c doesn't have to depend on CVPIOS (header), but it's unclear why you > need to do that, or what you considered. > > The concern here, which I only briefly touched on: > - _util files tend to grow > - This doesn't have any indication it's iOS specific (SecTrust is also used on > OS X, but we *wouldn't* want this method used there) > - There's still an implicit tight coupling between > crw_cert_verification_controller and CertVerifyProciOS (we *want* them to > produce same output), but that coupling is now abstracted away through a util > file that may not yield the same results. > > > With respect to the crw_c_v_c changes, what is your goal? To simply map to a > CertStatus/net_error code? Why (for the error bypassing?). Should we be using > SecTrustSetExceptions/CopyExceptions instead, if we're going all-in for > SecTrust? Is it because the same SSL error handler will be used by stuff that > goes through crw_c_v_c and through CertVerifyProciOS (do we have such > situations? should we?) > > There's a lot of design rationale hidden behind the design, and I'm just trying > to better understand what your constraints were. My gut is that we could simply > have a static method on CertVerifyProcIOS, which would at least express the > coupling much better (in that crw_c_v_c would have to call that static method), > but there's broader aspects to unwinding the code that I'm unclear of, and that > this subtle change highlights :) Ok, so looks like you generally not sure about the purpose of this CL. So let me try explaining that. Currently iOS does 2 cert verifications: 1.) Using SecTrust API to learn if cert is good or bad 2.) Using CertVerifier to learn the reason of the failure Second verification requires additional code, drains battery and makes threading model more complex. Initially we had this second verification so we could get the reason of the failure from NSS, but if we can get it from SecTrust directly then we should do that. So the purpose of this CL is to simplify the code and improve performance/battery life. If this explanation makes sense then I will make CertVerifyProcIOS::GetCertFailureStatusFromTrust static method.
On 2016/08/08 21:02:31, Eugene But wrote: > Ok, so looks like you generally not sure about the purpose of this CL. So let me > try explaining that. No, all of that was clear. > > Currently iOS does 2 cert verifications: > 1.) Using SecTrust API to learn if cert is good or bad > 2.) Using CertVerifier to learn the reason of the failure > > Second verification requires additional code, drains battery and makes threading > model more complex. Initially we had this second verification so we could get > the reason of the failure from NSS, but if we can get it from SecTrust directly > then we should do that. So the purpose of this CL is to simplify the code and > improve performance/battery life. I'm aware of all of that. But none of this touches on understanding the design decisions you made :) > If this explanation makes sense then I will make > CertVerifyProcIOS::GetCertFailureStatusFromTrust static method. It still leaves, as open questions, all of those things I asked. That is, it highlights that this change - with good intentions - has a lot of subtlety to look out for, and it's unclear if you did or not.
My design decision was simple: stop using CertVerifier for WKWebView cert verification, because it is overkill in the current situation :). >> With respect to the crw_c_v_c changes, what is your goal? I hope I explained this. Improve code/performance/battery life. >> To simply map to a CertStatus/net_error code? Yes. crw_c_v_c used CertVerifier for this mapping but currently it is too much overhead and complexity for no benefits. Using GetCertFailureStatusFromTrust does the job. >> Why (for the error bypassing?) Get the reason of failure for interstitials, ssl lock, and remembering failures accepted by the user. >> Should we be using SecTrustSetExceptions/CopyExceptions instead, if we're going all-in for SecTrust? I don't think SecTrustSetExceptions/CopyExceptions could help. >> Is it because the same SSL error handler will be used by stuff that goes through crw_c_v_c and through CertVerifyProciOS (do we have such situations? should we?) I don't understand this question, but will try explaining cases where we use CertVerifyProciOS and crw_c_v_c. CertVerifyProciOS will still be used by chrome net stack (f.e. for translate). crw_c_v_c will be used for interstitials and SSL lock as it is used now.
I'm afraid we're completely talking past eachother at this point, which is probably as frustrating for you :) I want to reiterate: I understand your high-level goal. That's not the concern and never was :) I'm specifically trying to understand why you created sec_trust_util, compared to the many alternatives that exist, because it's a design that seems to have downsides. That is, I'm *not* questioning why you're stopping using CertVerifier for a second pass, I'm specifically asking compared to the alternatives. I'm afraid my further design questions were similarly misunderstood. I'm happy to setup a meeting if it would help, but I'd also love to try to see if we can work out where the communication breakdown happened, to streamline future CLs :) Trying to restart the line of questioning: As far as I can tell, the only reason you're constructing a CertStatus is for (decidedLoadPolicyForRejectedTrustResult, loadPolicyforRejectedTrustResult). Is that correct? If not, could you explain what I'm missing? Fundamentally, I'm questioning whether crw_cert_verification_controller actually needs to know about CertStatus. From the code (which is hard to read because of all the threading), it _seems_ like it's only being used to determine a web::CertAcceptPolicy, and that doesn't seem to need a CertStatus.
I moved GetCertFailureStatusFromTrust back to CertVerifyProcIOS in the latest patch and made it a static method per your suggestion. I was never attached to sec_trust_util, sorry for not saying that explicitly :). >> As far as I can tell, the only reason you're constructing a CertStatus is for (decidedLoadPolicyForRejectedTrustResult, loadPolicyforRejectedTrustResult). Is that correct? If not, could you explain what I'm missing? There are 2 public methods which need CertStatus: 1.) decideLoadPolicyForAcceptedTrustResult: used to display the failure reason on SSL interstitial 2.) querySSLStatusForTrust: used for SSL lock status At this point I think we are on the same page. Sorry about the treading code. In this CL I'm actually trying to make it much simpler.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). GetFailureFromTrustProperties was extracted to a separate file, so it's not coupled with CertVerifyProcIOS. BUG=603634 ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 TEST=interstitials show correct reason of failure ==========
Ryan?
As the description indicated, a bit slow, and this is a fairly tricky CL that is taking a lot of time to review :)
On 2016/08/11 23:36:22, Ryan Sleevi (slow) wrote: > As the description indicated, a bit slow, and this is a fairly tricky CL that is > taking a lot of time to review :) No worries and sorry for pinging (wanted to make sure that you did not miss the email) :)
https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:47: // not be for a valid cert. Must be called on IO thread. Just trying to make sure I understand why the threading complexity remains: 1) CertPolicyCache is constructed on the UI thread, owned by the UI thread (line 70) 2) It's refcounted so it can be shared with both (bleh) 3) Its only use is in the RequestTrackerImpl (Is that code even still used?) Because the CPC needs to only be touched on the IO thread, we have loadPolicyForRejectedTrust (IO) and decideLoadPolicyForRejectedTrustResult (UI) to marshal around? A big part of my challenge here has been trying to understand the code in its new form, and understand how much we need. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: web::SECURITY_STYLE_AUTHENTICATED) { COMMENT: It's unclear to me why we indirect through SECURITY_STYLE here, as opposed to using the kSecTrustResult directory. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:141: if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) It's a little weird that you check securityStyle here (which is a mapped value from trustResult) but then use trustResult directly (line 144) For me at least, using trustResult directly seemed to make more sense, but am I missing something? https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:145: return net::CERT_STATUS_AUTHORITY_INVALID; Having this short-circuit *seems* like it means we'd show less information if the user affirmatively rejected a cert. Doesn't CertVerifyProciOS already handle this? https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:166: web::WebThread::IO, FROM_HERE, base::BindBlock(^{ Making sure I've grokked the reasoning for this: The whole reason to thread-hop to the IO thread is because you need to check the cert_policy_cache (in loadPolicyForRejectedTrustResult), right? https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:190: if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) { Curious: Is there any reason we don't/didn't use SecTrustEvaluateAsync here? Seems like it could avoid the base::WorkerPool requirement https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:178: CertVerifyProcIOS::~CertVerifyProcIOS() {} This (the dtor) should not be moved. Declaration and definition order should be kept in sync. The old code was buggy too, but more apparent here :) You can also = default; these, rather than {} https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.h (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.h:10: #include <Security/Security.h> nit: Can you not forward declare this? Or include the more specific header (Security/SecTrust.h) https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios_unittest.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios_unittest.cc:67: EXPECT_TRUE(status & CERT_STATUS_INVALID); This surprises me - have you debugged into why we're setting CERT_STATUS_INVALID here? I would not expect this.
The CQ bit was checked by eugenebut@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:47: // not be for a valid cert. Must be called on IO thread. On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > Just trying to make sure I understand why the threading complexity remains: > > 1) CertPolicyCache is constructed on the UI thread, owned by the UI thread (line > 70) > 2) It's refcounted so it can be shared with both (bleh) > 3) Its only use is in the RequestTrackerImpl (Is that code even still used?) > > Because the CPC needs to only be touched on the IO thread, we have > loadPolicyForRejectedTrust (IO) and decideLoadPolicyForRejectedTrustResult (UI) > to marshal around? > > A big part of my challenge here has been trying to understand the code in its > new form, and understand how much we need. Correct CertPolicyCache is constructed and owned by UI thread. Its methods can only be called on IO thread though and that's why we still have threading complexity. RequestTrackerImpl was created for UIWebView and we plan to get rid of it but it will not happen soon. Also CertPolicyCache is used in other places like TabModel (downstream) and CRWSessionCertificatePolicyManager (upstream). https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: web::SECURITY_STYLE_AUTHENTICATED) { On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > COMMENT: It's unclear to me why we indirect through SECURITY_STYLE here, as > opposed to using the kSecTrustResult directory. |web::GetSecurityStyleFromTrustResult(trustResult) == web::SECURITY_STYLE_AUTHENTICATED| is the indication that cert is good. Using |trustResult == kSecTrustResultProceed || trustResult == kSecTrustResultUnspecified| everywhere is worse alternative (f.e. what if Apple add third option which indicate good cert?). https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:141: if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) On 2016/08/12 00:16:22, Ryan Sleevi (slow) wrote: > It's a little weird that you check securityStyle here (which is a mapped value > from trustResult) but then use trustResult directly (line 144) > > For me at least, using trustResult directly seemed to make more sense, but am I > missing something? I didn't want to use |trustResult| here because if would require code like this: |trustResult == kSecTrustResultProceed || trustResult == kSecTrustResultUnspecified| which is less maintainable than using |GetSecurityStyleFromTrustResult|. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:145: return net::CERT_STATUS_AUTHORITY_INVALID; On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > Having this short-circuit *seems* like it means we'd show less information if > the user affirmatively rejected a cert. Doesn't CertVerifyProciOS already handle > this? Yes, this is from CertVerifyProciOS https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_ios.cc?q=case+... Do you think I should extract lines 266-282 from cert_verify_proc_ios.cc to a separate method and use it here? https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:166: web::WebThread::IO, FROM_HERE, base::BindBlock(^{ On 2016/08/12 00:16:22, Ryan Sleevi (slow) wrote: > Making sure I've grokked the reasoning for this: > > The whole reason to thread-hop to the IO thread is because you need to check the > cert_policy_cache (in loadPolicyForRejectedTrustResult), right? Correct. I added comments to the code. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:190: if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) { On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > Curious: Is there any reason we don't/didn't use SecTrustEvaluateAsync here? > Seems like it could avoid the base::WorkerPool requirement SecTrustEvaluateAsync takes GCD queue as param and we want to use Chromium threading primitives instead of GCD. https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.cc:178: CertVerifyProcIOS::~CertVerifyProcIOS() {} On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > This (the dtor) should not be moved. Declaration and definition order should be > kept in sync. The old code was buggy too, but more apparent here :) > > You can also = default; these, rather than {} Moved to the bottom. https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios.h (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios.h:10: #include <Security/Security.h> On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > nit: Can you not forward declare this? Or include the more specific header > (Security/SecTrust.h) Including subframework would be a Style Guide violation: go/objc-style#Use_Umbrella_Frameworks This is from Objective-C style guide, but it's more a planform thing rather than a language thing. There is no maintainable way to predeclare Core Foundation-like types and this is how it would look: |typedef struct CF_BRIDGED_TYPE(id) __SecTrust *SecTrustRef;| https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios_unittest.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios_unittest.cc:67: EXPECT_TRUE(status & CERT_STATUS_INVALID); On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > This surprises me - have you debugged into why we're setting CERT_STATUS_INVALID > here? I would not expect this. I actually debugged it and wanted to ask about this, but then I forgot :( so thanks for reminding me. We do not handle "Hostname mismatch." error and defaulting to CERT_STATUS_INVALID. Should we handle it?
https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: web::SECURITY_STYLE_AUTHENTICATED) { On 2016/08/12 16:16:39, Eugene But wrote: > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > COMMENT: It's unclear to me why we indirect through SECURITY_STYLE here, as > > opposed to using the kSecTrustResult directory. > > |web::GetSecurityStyleFromTrustResult(trustResult) == > web::SECURITY_STYLE_AUTHENTICATED| is the indication that cert is good. Right, I understood that part perfectly :) > Using > |trustResult == kSecTrustResultProceed || trustResult == > kSecTrustResultUnspecified| everywhere is worse alternative (f.e. what if Apple > add third option which indicate good cert?). This is what I was trying to understand - the reasoning behind not using trustResult directly. Apple won't change it because of the API committment, because it's explicitly documented these are the only two good ones. Your hypothetical third status would still require Chrome code change, but it seems you're arguing that it would only require changing one place, rather than every place dealing with it? The reason this stands out to me is that, at least in Chrome, SecurityStyle is a //content abstraction, but trust results are a //net abstraction, and //net sits under //content. It feels "weird" to me, in the sense that it feels like a //net layer (cert verification) is reaching up to the UI layer (SecurityStyle) to figure out how to trust a certificate. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:141: if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) On 2016/08/12 16:16:39, Eugene But wrote: > On 2016/08/12 00:16:22, Ryan Sleevi (slow) wrote: > > It's a little weird that you check securityStyle here (which is a mapped value > > from trustResult) but then use trustResult directly (line 144) > > > > For me at least, using trustResult directly seemed to make more sense, but am > I > > missing something? > I didn't want to use |trustResult| here because if would require code like this: > |trustResult == kSecTrustResultProceed || trustResult == > kSecTrustResultUnspecified| > > which is less maintainable than using |GetSecurityStyleFromTrustResult|. > I guess I disagree (see the other comment :P) Ultimately the layering feels weird here, and that's tripping me up when I try to map this code onto how we've structured the Chromium code, but it's not something I'd push back on this CL for, just as something to consider (again, with the previous comment) https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:145: return net::CERT_STATUS_AUTHORITY_INVALID; On 2016/08/12 16:16:39, Eugene But wrote: > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > Having this short-circuit *seems* like it means we'd show less information if > > the user affirmatively rejected a cert. Doesn't CertVerifyProciOS already > handle > > this? > Yes, this is from CertVerifyProciOS > https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_ios.cc?q=case+... > > Do you think I should extract lines 266-282 from cert_verify_proc_ios.cc to a > separate method and use it here? In general, I would strongly discourage extracting when you just have two consumers. In general, I'm a subscriber of the Rule of Three ( https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) ) - repetition can lead to significantly more maintainable code, as counter-intuitive as that is, because it reduces your dependency graph and allows you to reason about the impact of a change. And it's not the same as what's in CVPIOS. CVPIOS ors the result - so it guarantees you'll *at least* set AUTHORITY_INVALID, but may set additional stuff. To match what CVPIOS does, you should do net::CertStatus certStatus; if (trustResult == kSecTrustResultDeny) certStatus |= CERT_STATUS_AUTHORITY_INVALID; certStatus |= net::CertVerifyProcIOS::GetCertFailureStatusFromTrust(trust); if (!net::IsCertStatusError(certStatus)) { certStatus |= net::CERT_STATUS_INVALID; } return certStatus; https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:190: if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) { On 2016/08/12 16:16:39, Eugene But wrote: > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > Curious: Is there any reason we don't/didn't use SecTrustEvaluateAsync here? > > Seems like it could avoid the base::WorkerPool requirement > SecTrustEvaluateAsync takes GCD queue as param and we want to use Chromium > threading primitives instead of GCD. That feels like a very strange response to me, considering lines 193-195. I suppose there's some nuance here between that code (and all the blocks being taken here throughout this file) and GCD, but as a non-main iOS developer, it seems very inconsistent. https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios_unittest.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios_unittest.cc:67: EXPECT_TRUE(status & CERT_STATUS_INVALID); On 2016/08/12 16:16:40, Eugene But wrote: > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > This surprises me - have you debugged into why we're setting > CERT_STATUS_INVALID > > here? I would not expect this. > I actually debugged it and wanted to ask about this, but then I forgot :( so > thanks for reminding me. > We do not handle "Hostname mismatch." error and defaulting to > CERT_STATUS_INVALID. Should we handle it? If iOS tells us the hostname is mismatched, we certainly should add code to the CVPIOS to handle that, yeah.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Oh, and I forgot the LGTM, sorry, meant to give that sooner (that is, contingent on my thread understanding being correct, which it was) Most of these were design feedback/areas I was tripped up while reviewing, but not blockers for this CL.
Thanks! I really appreciate your very detailed review. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: web::SECURITY_STYLE_AUTHENTICATED) { On 2016/08/12 16:28:33, Ryan Sleevi (slow) wrote: > On 2016/08/12 16:16:39, Eugene But wrote: > > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > > COMMENT: It's unclear to me why we indirect through SECURITY_STYLE here, as > > > opposed to using the kSecTrustResult directory. > > > > |web::GetSecurityStyleFromTrustResult(trustResult) == > > web::SECURITY_STYLE_AUTHENTICATED| is the indication that cert is good. > > Right, I understood that part perfectly :) > > > Using > > |trustResult == kSecTrustResultProceed || trustResult == > > kSecTrustResultUnspecified| everywhere is worse alternative (f.e. what if > Apple > > add third option which indicate good cert?). > > This is what I was trying to understand - the reasoning behind not using > trustResult directly. Apple won't change it because of the API committment, > because it's explicitly documented these are the only two good ones. Your > hypothetical third status would still require Chrome code change, but it seems > you're arguing that it would only require changing one place, rather than every > place dealing with it? Yes, that's my reasoning. > The reason this stands out to me is that, at least in Chrome, SecurityStyle is a > //content abstraction, but trust results are a //net abstraction, and //net sits > under //content. It feels "weird" to me, in the sense that it feels like a //net > layer (cert verification) is reaching up to the UI layer (SecurityStyle) to > figure out how to trust a certificate. Thanks, now I understand your concern. I'm split on this and it's hard for me to choose between maintainability and layering consistency. But I see you have a stronger preference which makes my choice easier. Done. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:141: if (securityStyle == web::SECURITY_STYLE_AUTHENTICATED) On 2016/08/12 16:28:33, Ryan Sleevi (slow) wrote: > On 2016/08/12 16:16:39, Eugene But wrote: > > On 2016/08/12 00:16:22, Ryan Sleevi (slow) wrote: > > > It's a little weird that you check securityStyle here (which is a mapped > value > > > from trustResult) but then use trustResult directly (line 144) > > > > > > For me at least, using trustResult directly seemed to make more sense, but > am > > I > > > missing something? > > I didn't want to use |trustResult| here because if would require code like > this: > > |trustResult == kSecTrustResultProceed || trustResult == > > kSecTrustResultUnspecified| > > > > which is less maintainable than using |GetSecurityStyleFromTrustResult|. > > > > I guess I disagree (see the other comment :P) Ultimately the layering feels > weird here, and that's tripping me up when I try to map this code onto how we've > structured the Chromium code, but it's not something I'd push back on this CL > for, just as something to consider (again, with the previous comment) Done. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:145: return net::CERT_STATUS_AUTHORITY_INVALID; On 2016/08/12 16:28:33, Ryan Sleevi (slow) wrote: > On 2016/08/12 16:16:39, Eugene But wrote: > > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > > Having this short-circuit *seems* like it means we'd show less information > if > > > the user affirmatively rejected a cert. Doesn't CertVerifyProciOS already > > handle > > > this? > > Yes, this is from CertVerifyProciOS > > > https://cs.chromium.org/chromium/src/net/cert/cert_verify_proc_ios.cc?q=case+... > > > > Do you think I should extract lines 266-282 from cert_verify_proc_ios.cc to a > > separate method and use it here? > > In general, I would strongly discourage extracting when you just have two > consumers. In general, I'm a subscriber of the Rule of Three ( > https://en.wikipedia.org/wiki/Rule_of_three_(computer_programming) ) - > repetition can lead to significantly more maintainable code, as > counter-intuitive as that is, because it reduces your dependency graph and > allows you to reason about the impact of a change. > > And it's not the same as what's in CVPIOS. CVPIOS ors the result - so it > guarantees you'll *at least* set AUTHORITY_INVALID, but may set additional > stuff. > > To match what CVPIOS does, you should do > > net::CertStatus certStatus; > if (trustResult == kSecTrustResultDeny) > certStatus |= CERT_STATUS_AUTHORITY_INVALID; > certStatus |= net::CertVerifyProcIOS::GetCertFailureStatusFromTrust(trust); > if (!net::IsCertStatusError(certStatus)) { > certStatus |= net::CERT_STATUS_INVALID; > } > return certStatus; CVPIOS has a switch-case so I matched that. https://codereview.chromium.org/2225483002/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:190: if (SecTrustEvaluate(trust.get(), &trustResult) != errSecSuccess) { On 2016/08/12 16:28:33, Ryan Sleevi (slow) wrote: > On 2016/08/12 16:16:39, Eugene But wrote: > > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > > Curious: Is there any reason we don't/didn't use SecTrustEvaluateAsync here? > > > Seems like it could avoid the base::WorkerPool requirement > > SecTrustEvaluateAsync takes GCD queue as param and we want to use Chromium > > threading primitives instead of GCD. > > That feels like a very strange response to me, considering lines 193-195. I > suppose there's some nuance here between that code (and all the blocks being > taken here throughout this file) and GCD, but as a non-main iOS developer, it > seems very inconsistent. |dispatch_async(dispatch_get_main_queue()| is equivalent to |web::WebThread::PostTask(web::WebThread::UI, FROM_HERE, base::BindBlock(^{|, both run block on UI thread. GCD queues are different from threads (different queues may share the same thread). But consistency is a fair point, so I won't use |dispatch_async(dispatch_get_main_queue()| in this file. https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... File net/cert/cert_verify_proc_ios_unittest.cc (right): https://codereview.chromium.org/2225483002/diff/100001/net/cert/cert_verify_p... net/cert/cert_verify_proc_ios_unittest.cc:67: EXPECT_TRUE(status & CERT_STATUS_INVALID); On 2016/08/12 16:28:33, Ryan Sleevi (slow) wrote: > On 2016/08/12 16:16:40, Eugene But wrote: > > On 2016/08/12 00:16:23, Ryan Sleevi (slow) wrote: > > > This surprises me - have you debugged into why we're setting > > CERT_STATUS_INVALID > > > here? I would not expect this. > > I actually debugged it and wanted to ask about this, but then I forgot :( so > > thanks for reminding me. > > We do not handle "Hostname mismatch." error and defaulting to > > CERT_STATUS_INVALID. Should we handle it? > > If iOS tells us the hostname is mismatched, we certainly should add code to the > CVPIOS to handle that, yeah. Used COMMON_NAME_INVALID for this error.
eugenebut@chromium.org changed reviewers: + isherman@chromium.org
+isherman@ for metrics
histograms.xml lgtm, thanks
Thanks you, Ilya.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/2225483002/#ps140001 (title: "Addressed review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 TEST=interstitials show correct reason of failure ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 TEST=interstitials show correct reason of failure ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001)
Message was sent while issue was closed.
Description was changed from ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 TEST=interstitials show correct reason of failure ========== to ========== [ios] Removed CertVerifierBlockAdapter. cert_verify_proc_ios.cc uses SecTrustRef to get the reason of cert verification failure, so there is no reason to use CertVerifier for web view cert verification. Removing CertVerifier usage should have positive impact on battery life (because second versification will not be performed) and simplify the code (especially threading part). BUG=603634 TEST=interstitials show correct reason of failure Committed: https://crrev.com/bbaae2f91795c442ef62bdfe42be1d5ea75f7cc2 Cr-Commit-Position: refs/heads/master@{#412096} ==========
Message was sent while issue was closed.
Patchset 8 (id:??) landed as https://crrev.com/bbaae2f91795c442ef62bdfe42be1d5ea75f7cc2 Cr-Commit-Position: refs/heads/master@{#412096} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
