Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1322193003:
WKWebView(iOS9): correctly update SSL status for current navigation item (Closed)
|
Created:
5 years, 3 months ago by Eugene But (OOO till 7-30) Modified:
5 years, 2 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@reland_cert_verification Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWKWebView(iOS9): correctly update SSL status for current navigation item
SSL status affects the color of SSL padlock and OIB content. Previously
the code was written with the assumption that WKWebView can not load a
page with invalid SSL cert (which was always true on iOS 8). With iOS 9
it is possible to load pages with invalid cert. This CL adds support for
correct SSL status calculation.
This is precursory CL for implementing recoverable SSL warnings.
BUG=462427, 528668
Design Doc:
https://docs.google.com/document/d/1o8wzJkxzG1fvhvW9Gqf-RdGjhm0btCIehDtbxDfbLcw/edit#
Committed: https://crrev.com/8ac2913b9c37a0a310ed152dc6be9f60e72d1f2b
Cr-Commit-Position: refs/heads/master@{#354575}
Patch Set 1 #Patch Set 2 : Corrected comment #
Total comments: 12
Patch Set 3 : Addressed Stuart's review comments; Do not reevaluate cert if unnecessary; #Patch Set 4 : Removed unnecessary change #
Total comments: 1
Patch Set 5 : Clear all non-error bits for valid certs. #
Total comments: 8
Patch Set 6 : Fixed typos #
Total comments: 32
Patch Set 7 : Resolved review comments #Patch Set 8 : Resolved review comments #Patch Set 9 : Minor comments update. #
Total comments: 18
Patch Set 10 : Resolved Stuart's review comments #Patch Set 11 : Predeclare -certificateChain. #
Total comments: 21
Patch Set 12 : Resolved Ryan's review comments #Patch Set 13 : Updated comment #Patch Set 14 : Switched API to use SecTrustRef to avoid using real-world cert chain for testing #Patch Set 15 : Do not use CertVerifier for good certs #
Total comments: 30
Patch Set 16 : Addressed David's review comments #Patch Set 17 : Self review #Patch Set 18 : Merged with origin/master #
Total comments: 12
Patch Set 19 : Resolved Stuart's review comments #Patch Set 20 : Self review #
Total comments: 50
Patch Set 21 : Addressed review comments #Patch Set 22 : Moved reportHasCertForSecureConnectionUMAWithValue: out of ifdefs. #
Total comments: 4
Patch Set 23 : Updated comments #
Total comments: 24
Patch Set 24 : Addressed David's review comments. #Patch Set 25 : Self review #
Total comments: 10
Patch Set 26 : Addressed David's review comments #
Total comments: 6
Patch Set 27 : Resolved review comments (updated code comments) #
Total comments: 2
Patch Set 28 : Updated comment #Dependent Patchsets: Messages
Total messages: 86 (18 generated)
eugenebut@chromium.org changed reviewers: + stuartmorgan@chromium.org
Stuart, I will add security folks to this review which you feel good about overall structure.
Overall structure looks good, just small stuff. https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:160: if (errSecSuccess != SecTrustEvaluate(trust.get(), &trustResult)) { Chromium style prefers the 'variable == constant' form to 'constant == variable' https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:161: trustResult = kSecTrustResultInvalid; This looks redundant; can STE modify the out param even when it returns failure? https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:863: bool updated = false; updatedCurrentItem? https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:867: web::SSLStatus previousSSLStatus = item->GetSSL(); This should be inside the if; currently you're copying the status for every single item in history, even if only one matches. https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:872: if (navigationManager.GetCurrentEntryIndex() == i && The current index could be called outside the loop, since it won't change. (Optional, but seems like a good time to create a public GetCurrentItemIndex, since the Entry form was just a temporary facade helper.) https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:935: // iOS8 cannot load unauthentificated HTTPS content. s/ificated/icated/
PTAL https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:160: if (errSecSuccess != SecTrustEvaluate(trust.get(), &trustResult)) { On 2015/09/10 22:26:18, stuartmorgan wrote: > Chromium style prefers the 'variable == constant' form to 'constant == variable' Acknowledged. https://codereview.chromium.org/1322193003/diff/20001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:161: trustResult = kSecTrustResultInvalid; On 2015/09/10 22:26:17, stuartmorgan wrote: > This looks redundant; can STE modify the out param even when it returns failure? Not according to this code: http://opensource.apple.com/source/Security/Security-55471/sec/Security/SecTr... Removed. https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:863: bool updated = false; On 2015/09/10 22:26:18, stuartmorgan wrote: > updatedCurrentItem? Done. https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:867: web::SSLStatus previousSSLStatus = item->GetSSL(); On 2015/09/10 22:26:18, stuartmorgan wrote: > This should be inside the if; currently you're copying the status for every > single item in history, even if only one matches. Done. https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:872: if (navigationManager.GetCurrentEntryIndex() == i && On 2015/09/10 22:26:18, stuartmorgan wrote: > The current index could be called outside the loop, since it won't change. > > (Optional, but seems like a good time to create a public GetCurrentItemIndex, > since the Entry form was just a temporary facade helper.) Done. https://codereview.chromium.org/1322193003/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:935: // iOS8 cannot load unauthentificated HTTPS content. On 2015/09/10 22:26:18, stuartmorgan wrote: > s/ificated/icated/ Done.
eugenebut@chromium.org changed reviewers: + jww@chromium.org, rsleevi@chromium.org
https://codereview.chromium.org/1322193003/diff/60001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/60001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:939: // TODO(eugenebut): Add UMA action for this anomaly (crbug.com/528668). Question to networking folks: do you think this can happen in case of server misconfiguration (f.e. non-secure content is served on port 443)?
felt@chromium.org changed reviewers: + felt@chromium.org
The description says that reverification happens only when a page is known to have an invalid cert (as per SecTrust). But the code *looks* to me like it's actually reverifying every time... although I admit to not knowing Objective C's syntax. Can you explain what's up? Thanks.
On 2015/09/15 04:28:57, felt wrote: > The description says that reverification happens only when a page is known to > have an invalid cert (as per SecTrust). But the code *looks* to me like it's > actually reverifying every time... although I admit to not knowing Objective C's > syntax. Can you explain what's up? Thanks. Another thing: as I tried to explain here https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR... the CertVerifier will actually return "invalid" for many reasons beyond SHA-1. Notably, if a root cert is present on the device it will pass SecTrust verification but fail CertVerifier verification (which is not what we want). So if we're going to run CertVerifier every time (and not just when SecTrust says the cert chain is invalid), we'll want to discard the CertVerifier results unless the error is caused by a Chrome policy (SHA-1 or blacklisting).
Adrienne, reverification happens for good and bad certs, I've updated CL description by removing confusing statements. For certs which considered *valid* by Sec Trust API I clear all error bits from CertStatus: https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... Thank you for catching that.
https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presense. nit: "presense" -> "presence" https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:937: // HTTPS, iOS9 and no certificate (this use-case has not been observed). Given that this seems like a silly scenario, and we've never observed this, should there at least be a debug assert that this doesn't happen?
https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:151: // Knowing net::CertStatus is necessry even for valid certs in order to nit: "necessry" -> "necessary" https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/wk_we... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/wk_we... ios/web/web_state/wk_web_view_security_util.mm:138: case kSecTrustResultUnspecified: ^ I'm surprised that "unspecified" ends up being treated as valid
PTAL https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:151: // Knowing net::CertStatus is necessry even for valid certs in order to On 2015/09/15 22:39:11, felt wrote: > nit: "necessry" -> "necessary" Done. https://codereview.chromium.org/1322193003/diff/80001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presense. On 2015/09/15 22:26:25, jww wrote: > nit: "presense" -> "presence" Done. https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/ui/cr... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:937: // HTTPS, iOS9 and no certificate (this use-case has not been observed). On 2015/09/15 22:26:25, jww wrote: > Given that this seems like a silly scenario, and we've never observed this, > should there at least be a debug assert that this doesn't happen? I suspect that this may happen in case of server misconfiguration (f.e. non-secure content is served on port 443). I think that adding UMA metric will be better than DCHECK, because DCHECK should not be used for cases which are possible. https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/wk_we... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/80001/ios/web/web_state/wk_we... ios/web/web_state/wk_web_view_security_util.mm:138: case kSecTrustResultUnspecified: On 2015/09/15 22:39:11, felt wrote: > ^ I'm surprised that "unspecified" ends up being treated as valid Yeah, I understand confusion, but this is actual result of evaluating a valid SSL cert. And this is from Apple documentation: @constant kSecTrustResultUnspecified Indicates the evaluation succeeded and the certificate is implicitly trusted, but user intent was not explicitly specified. This value may be returned by the SecTrustEvaluate function or stored as part of the user trust settings.
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/patch-status/1322193003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
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/patch-status/1322193003/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/100001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
Could you please take a look. We want to land this till this end of September.
On 2015/09/17 15:07:53, eugenebut wrote: > Could you please take a look. We want to land this till this end of September. Should we still review this given your latest email (that the combined approach doesn't work)?
On 2015/09/18 00:53:57, jww wrote: > On 2015/09/17 15:07:53, eugenebut wrote: > > Could you please take a look. We want to land this till this end of September. > > Should we still review this given your latest email (that the combined approach > doesn't work)? ~80% of this code will be unchanged even if we choose not to use combined approach, but stick to certVerifier. So, yes, I would really appreciate if you could take a look.
Generally lgtm, with a few minor comments. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:115: TEST_F(CRWCertVerificationControllerTest, SSLStatusForInvalidCert) { Shouldn't we have tests for various kinds of invalid certs? And shouldn't we have a test for valid certs? https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:923: if (cert) { Given that this is only possible if the above conditional is true, may I suggest moving this "if (cert)" into the above conditional? Then make the following "else" an "if (!cert)" conditional. I think it would make this a lot easier to follow. https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... File net/cert/cert_status_flags.h (right): https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... net/cert/cert_status_flags.h:28: static const CertStatus CERT_STATUS_NON_ERROR_STATUSES = 0xFFFF << 16; This is more something for rsleevi@ to address, but it seems like we might want to make CERT_STATUS_NON_ERROR_STATUES be defined in terms of CERT_STATUS_ALL_ERRORS.
https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:64: // |certificateChain| (an NSArray of SecSertificateRef objects) and |host|. Document what form host is It in URL form? DNS form? Something else? For example, if I load https://[::1], will, I get "[::1]" or "::1" ? If I load "名がドメイン.com" will I get "名がドメイン.com" or "xn--v8jxj3d1dzdz08w.com" ? https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: // support SHA-1 deprecation. I'm not sure what is meant to be accomplished by this? CertStatus is part of the API contract for all certificate validations. That is, it's not about "why", it's _this is the API_. (Technically, CertVerifyResult, not CertStatus, is what is expected) https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); BUG: You need to check the result code here. DESIGN: Does it make more sense to call the async version? Why or why not? https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presence. If there are error bits, we won't necessarily have set the status bits, FWIW. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:169: cert_status &= net::CERT_STATUS_NON_ERROR_STATUSES; cert_status &= ~CERT_STATUS_ALL_ERRORS; https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:867: int currentItemIndex = navigationManager->GetCurrentEntryIndex(); BUG: I realize this is in pre-existing code, but it seems this whole class does unnecessary type coercion that may lead to subtle integer bugs I filed https://code.google.com/p/chromium/issues/detail?id=533848 , if only because I've spent the past two weeks finding and fixing security bugs related to these sorts of issues (I don't think there's an active security bug here, to be clear - just something that is a red flag in reviews) https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:100: return base::ScopedCFTypeRef<SecTrustRef>(); You're preventing NVRO from helping you here with this approach base::ScopedCFTypeRef<SecTrustRef> result; if (certs.count == 0) return result; base::.... SecTrustRef tempResult = nil; // I presume since this is Obj-C++ we're all about nil over nullptr. if (SecTrust(...)) { result = tempResult; return result; } return result; Note how this replaces all return points to a single named variable, which allows the compiler to do NRVO. It also saves you typing :) https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:136: return SECURITY_STYLE_UNKNOWN; Hrm, this implies a more benign result, except it's a pretty bad situation to get a kSecTrustResultInvalid https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... File net/cert/cert_status_flags.h (right): https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... net/cert/cert_status_flags.h:28: static const CertStatus CERT_STATUS_NON_ERROR_STATUSES = 0xFFFF << 16; On 2015/09/19 02:16:34, jww wrote: > This is more something for rsleevi@ to address, but it seems like we might want > to make CERT_STATUS_NON_ERROR_STATUES be defined in terms of > CERT_STATUS_ALL_ERRORS. It's not clear why this is needed at all.
Thanks for review. PTAL https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:64: // |certificateChain| (an NSArray of SecSertificateRef objects) and |host|. On 2015/09/19 12:45:37, Ryan Sleevi wrote: > Document what form host is > > It in URL form? DNS form? Something else? > > For example, if I load https://[::1], will, I get "[::1]" or "::1" ? > If I load "名がドメイン.com" will I get "名がドメイン.com" or "xn--v8jxj3d1dzdz08w.com" ? If you load "https://[::1]" then host will be "::1". If you load "http://名がドメイン.com" you will get "xn--v8jxj3d1dzdz08w.com". I guess this is DNS form, so I updated comments accordingly. Please let me know if it's not DNS form, but something else. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: // support SHA-1 deprecation. On 2015/09/19 12:45:38, Ryan Sleevi wrote: > I'm not sure what is meant to be accomplished by this? > > CertStatus is part of the API contract for all certificate validations. That is, > it's not about "why", it's _this is the API_. > > (Technically, CertVerifyResult, not CertStatus, is what is expected) What I was trying to say with this comment is that "running CertVerifier is necessary even for valid certs and providing 0 CertStatus is not an option (even if we want to save battery life)". Changed the comment to: "Retrieving net::CertVerifyResult is necessary even for valid certs in order to support SHA-1 deprecation." Please let me know if this is still confusing. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); On 2015/09/19 12:45:37, Ryan Sleevi wrote: > BUG: You need to check the result code here. Initially I wrote code like this: if (errSecSuccess != SecTrustEvaluate(trust.get(), &trustResult)) { trustResult = kSecTrustResultInvalid; } Stuart, asked if |trustResult| can actually be modified in case of STE error, and according to source code it's not: http://opensource.apple.com/source/Security/Security-55471/sec/Security/SecTr... However SecTrustEvaluateAsync has this code: if (errSecSuccess != SecTrustEvaluate(trust, &trustResult)) { trustResult = kSecTrustResultInvalid; } so I agree with you and will stick with my original code and check for error code. > DESIGN: Does it make more sense to call the async version? Why or why not? No, SecTrustEvaluateAsync simply calls SecTrustEvaluate on a given gcd queue. In our case there is no value in bouncing to another thread. Calling SecTrustEvaluate here (on IO thread) seems to be the right approach. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presence. On 2015/09/19 12:45:38, Ryan Sleevi wrote: > If there are error bits, we won't necessarily have set the status bits, FWIW. So if SecTrust thinks that cert is valid, but certVerifierResult.cert_status has error bits we may loose SHA-1 bits. I think if there are at least some situations when both status and security bits are set, it worth having this code. I guess the only thing I can do here is document the behavior. Let me know if you want to do something else here. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:169: cert_status &= net::CERT_STATUS_NON_ERROR_STATUSES; On 2015/09/19 12:45:38, Ryan Sleevi wrote: > cert_status &= ~CERT_STATUS_ALL_ERRORS; Done. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:115: TEST_F(CRWCertVerificationControllerTest, SSLStatusForInvalidCert) { On 2015/09/19 02:16:34, jww wrote: > Shouldn't we have tests for various kinds of invalid certs? And shouldn't we > have a test for valid certs? Updated with the following test cases: - valid chain, no errors - valid chain, SHA-1 signature, all CertVerifier errors are ignored - invalid chain, expired cert - invalid chain, hostname mismatch I believe there is no need to add more test cases for invalid cert, because these tests use MockCertVerifier. https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:867: int currentItemIndex = navigationManager->GetCurrentEntryIndex(); On 2015/09/19 12:45:38, Ryan Sleevi wrote: > BUG: I realize this is in pre-existing code, but it seems this whole class does > unnecessary type coercion that may lead to subtle integer bugs > > I filed https://code.google.com/p/chromium/issues/detail?id=533848 , if only > because I've spent the past two weeks finding and fixing security bugs related > to these sorts of issues (I don't think there's an active security bug here, to > be clear - just something that is a red flag in reviews) Yeah, that is type narrowing problem on ARM64. But in practice it should not be possible to have more than 2 billions Navigation Items. https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:923: if (cert) { On 2015/09/19 02:16:34, jww wrote: > Given that this is only possible if the above conditional is true, may I suggest > moving this "if (cert)" into the above conditional? Then make the following > "else" an "if (!cert)" conditional. I think it would make this a lot easier to > follow. Sure. https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:100: return base::ScopedCFTypeRef<SecTrustRef>(); On 2015/09/19 12:45:38, Ryan Sleevi wrote: > You're preventing NVRO from helping you here with this approach > > base::ScopedCFTypeRef<SecTrustRef> result; > > if (certs.count == 0) > return result; > base::.... > SecTrustRef tempResult = nil; // I presume since this is Obj-C++ we're all > about nil over nullptr. > if (SecTrust(...)) { > result = tempResult; > return result; > } > return result; > > Note how this replaces all return points to a single named variable, which > allows the compiler to do NRVO. It also saves you typing :) Done. However I kept nullptr, because nil should be used only for Objective-C object variables. https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:136: return SECURITY_STYLE_UNKNOWN; On 2015/09/19 12:45:38, Ryan Sleevi wrote: > Hrm, this implies a more benign result, except it's a pretty bad situation to > get a kSecTrustResultInvalid Acknowledged. Let me know if you expect something actionable here. https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... File net/cert/cert_status_flags.h (right): https://codereview.chromium.org/1322193003/diff/100001/net/cert/cert_status_f... net/cert/cert_status_flags.h:28: static const CertStatus CERT_STATUS_NON_ERROR_STATUSES = 0xFFFF << 16; On 2015/09/19 12:45:38, Ryan Sleevi wrote: > On 2015/09/19 02:16:34, jww wrote: > > This is more something for rsleevi@ to address, but it seems like we might > want > > to make CERT_STATUS_NON_ERROR_STATUES be defined in terms of > > CERT_STATUS_ALL_ERRORS. > > It's not clear why this is needed at all. Yeah... removed...
https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: // support SHA-1 deprecation. On 2015/09/21 17:23:39, eugenebut wrote: > "Retrieving net::CertVerifyResult is necessary even for valid certs in order to > support SHA-1 deprecation." I still think this comment is a bit shaky on the layering and such. For example, if we did away with the SHA-1 deprecation, we'd still want this code. I find it's easier to discuss such comments in terms of the concepts they're trying to capture, and then providing specific examples (such as SHA-1 deprecation) as possible. As it stands, your proposed wording I still find quite confusing / not helping, and I'm still trying to understand the actual goal here (having read the design docs) // Retrieve the net::CertStatus to allow Chromium-specific policies to be enacted. This is called for all certificates // (valid and invalid alike), since the net::CertVerifier may reject or downgrade certificates that SecTrust accepts. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); On 2015/09/21 17:23:39, eugenebut wrote: > No, SecTrustEvaluateAsync simply calls SecTrustEvaluate on a given gcd queue. In > our case there is no value in bouncing to another thread. Calling > SecTrustEvaluate here (on IO thread) seems to be the right approach. Woah, you're calling this on the IO thread? That's a real threading bug then. You must not block the IO thread on SecTrustEvaluate, and it *will* block. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); On 2015/09/21 17:23:39, eugenebut wrote: > Stuart, asked if |trustResult| can actually be modified in case of STE error, > and according to source code it's not: > http://opensource.apple.com/source/Security/Security-55471/sec/Security/SecTr... I appreciate the explanation, but the rationale for it is shaky. You should follow the public API contract whenever possible - implementations may (and often do) change, but API contracts shouldn't. As it stands, note that quite a bit of the actual implementation of SecTrust APIs for iOS is not part of the public open-sourced Apple stuff. It's... complicated. But whenever using these APIs, try as hard as possible to stick to the contract except when absolutely necessary. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presence. On 2015/09/21 17:23:39, eugenebut wrote: > I think if there are at least some situations when both status and security bits > are set, it worth having this code. > I guess the only thing I can do here is document the behavior. Let me know if > you want to do something else here. This is another comment that appears confusing, especially with the above justification. // If SecTrust has accepted a certificate, clear any error statuses returned by net::CertVerifier, since the // CertVerifier may not have access to the same trust information (e.g. custom-installed roots). Note, this // also disables net::CertVerifier's ability to reject certificates, such as via CRLSets. // However, copy any supplemental status bits over, since those may be used as part of policy logic, such // as deprecating SHA-1 or requiring Certificate Transparency. https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:867: int currentItemIndex = navigationManager->GetCurrentEntryIndex(); "But in practice" is the cause of many a nasty security bug :) Anyways, bug filed, it should be fixed, we'll leave that for that bug :)
PTAL https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:152: // support SHA-1 deprecation. On 2015/09/21 17:39:04, Ryan Sleevi wrote: > On 2015/09/21 17:23:39, eugenebut wrote: > > "Retrieving net::CertVerifyResult is necessary even for valid certs in order > to > > support SHA-1 deprecation." > > I still think this comment is a bit shaky on the layering and such. For example, > if we did away with the SHA-1 deprecation, we'd still want this code. > > I find it's easier to discuss such comments in terms of the concepts they're > trying to capture, and then providing specific examples (such as SHA-1 > deprecation) as possible. > > As it stands, your proposed wording I still find quite confusing / not helping, > and I'm still trying to understand the actual goal here (having read the design > docs) > > // Retrieve the net::CertStatus to allow Chromium-specific policies to be > enacted. This is called for all certificates > // (valid and invalid alike), since the net::CertVerifier may reject or > downgrade certificates that SecTrust accepts. Done. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); On 2015/09/21 17:39:04, Ryan Sleevi wrote: > On 2015/09/21 17:23:39, eugenebut wrote: > > No, SecTrustEvaluateAsync simply calls SecTrustEvaluate on a given gcd queue. > In > > our case there is no value in bouncing to another thread. Calling > > SecTrustEvaluate here (on IO thread) seems to be the right approach. > > Woah, you're calling this on the IO thread? That's a real threading bug then. > You must not block the IO thread on SecTrustEvaluate, and it *will* block. Bounced to WorkerThread. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:160: SecTrustEvaluate(trust.get(), &trustResult); On 2015/09/21 17:39:04, Ryan Sleevi wrote: > On 2015/09/21 17:23:39, eugenebut wrote: > > Stuart, asked if |trustResult| can actually be modified in case of STE error, > > and according to source code it's not: > > > http://opensource.apple.com/source/Security/Security-55471/sec/Security/SecTr... > > I appreciate the explanation, but the rationale for it is shaky. You should > follow the public API contract whenever possible - implementations may (and > often do) change, but API contracts shouldn't. As it stands, note that quite a > bit of the actual implementation of SecTrust APIs for iOS is not part of the > public open-sourced Apple stuff. It's... complicated. > > But whenever using these APIs, try as hard as possible to stick to the contract > except when absolutely necessary. Done. https://codereview.chromium.org/1322193003/diff/100001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:164: // state, but will keep important information, like SHA-1 presence. On 2015/09/21 17:39:04, Ryan Sleevi wrote: > On 2015/09/21 17:23:39, eugenebut wrote: > > I think if there are at least some situations when both status and security > bits > > are set, it worth having this code. > > I guess the only thing I can do here is document the behavior. Let me know if > > you want to do something else here. > > This is another comment that appears confusing, especially with the above > justification. > > // If SecTrust has accepted a certificate, clear any error statuses returned by > net::CertVerifier, since the > // CertVerifier may not have access to the same trust information (e.g. > custom-installed roots). Note, this > // also disables net::CertVerifier's ability to reject certificates, such as via > CRLSets. > // However, copy any supplemental status bits over, since those may be used as > part of policy logic, such > // as deprecating SHA-1 or requiring Certificate Transparency. Done.
https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:60: // asynchronously on UI thread. the UI thread https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:153: host:(NSString*)host Do we want to DCHECK that it's ASCII to try to catch hosts in the wrong format, or is that something that only matters to lower-level code? https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. Should we maybe add a design doc to chromium.org explaining this system, and link to it here? https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:255: bool result = base::WorkerPool::PostTask(FROM_HERE, base::BindBlock(^{ A brief comment here explaining why this is bouncing off and then back onto the IO thread would be good, since it's not obvious without having read the discussion in the CL comments. (Also, capturing the threading of all of this code at a high level in the design doc would be very helpful) https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:61: for (SecCertificateRef intermiiate : cert->GetIntermediateCertificates()) { intermediate https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:253: // Obtains SSL status from given |certChain| and updates it for navigation items "it" here would be the SSL status of the cert chain, but isn't in the navigation items that are being updated? https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:936: NSArray* chain = [_wkWebView performSelector:@selector(certificateChain)]; Why is this performSelector? (If it's an SDK issue, you should predeclare the method in an SDK ifdef guard so it's easy to find and clean up once all bots have the necessary SDK.)
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/patch-status/1322193003/180001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/180001
Thanks! https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:60: // asynchronously on UI thread. On 2015/09/22 20:30:27, stuartmorgan wrote: > the UI thread Done. https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:153: host:(NSString*)host On 2015/09/22 20:30:27, stuartmorgan wrote: > Do we want to DCHECK that it's ASCII to try to catch hosts in the wrong format, > or is that something that only matters to lower-level code? I don't know. Ryan, what do you think about such DCHECK? https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/22 20:30:27, stuartmorgan wrote: > Should we maybe add a design doc to http://chromium.org explaining this system, and > link to it here? What exactly do you mean by "the system"? Chromium has multiple policies, I guess some of them are temporary, some of them are permanent, so I guess that having up to date doc would be non trivial task. https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:255: bool result = base::WorkerPool::PostTask(FROM_HERE, base::BindBlock(^{ On 2015/09/22 20:30:27, stuartmorgan wrote: > A brief comment here explaining why this is bouncing off and then back onto the > IO thread would be good, since it's not obvious without having read the > discussion in the CL comments. > > (Also, capturing the threading of all of this code at a high level in the design > doc would be very helpful) Added comments and updated design doc. https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:61: for (SecCertificateRef intermiiate : cert->GetIntermediateCertificates()) { On 2015/09/22 20:30:27, stuartmorgan wrote: > intermediate Done. https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:253: // Obtains SSL status from given |certChain| and updates it for navigation items On 2015/09/22 20:30:28, stuartmorgan wrote: > "it" here would be the SSL status of the cert chain, but isn't in the navigation > items that are being updated? Yes, navigation items are updated. Fixed comment. https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:936: NSArray* chain = [_wkWebView performSelector:@selector(certificateChain)]; On 2015/09/22 20:30:28, stuartmorgan wrote: > Why is this performSelector? (If it's an SDK issue, you should predeclare the > method in an SDK ifdef guard so it's easy to find and clean up once all bots > have the necessary SDK.) Instead of predeclaring I added ifdefs here, because it looks cleaner. I used the smallest possible scope for this ifdef.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ryan, could you please take a look. Thanks!
https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/22 22:43:04, eugenebut wrote: > On 2015/09/22 20:30:27, stuartmorgan wrote: > > Should we maybe add a design doc to http://chromium.org explaining this > system, and > > link to it here? > What exactly do you mean by "the system"? I was thinking of the relationship between how we use the two different verification systems. https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:936: NSArray* chain = [_wkWebView performSelector:@selector(certificateChain)]; On 2015/09/22 22:43:04, eugenebut wrote: > Instead of predeclaring I added ifdefs here, because it looks cleaner. It may look cleaner, but it causes the code not to actually run on iOS 9 if built with the iOS 8 SDK, which is not the outcome we want. Predeclaring is our standard pattern for writing code against an SDK we don't have completely rolled out yet (for exactly this reason).
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/patch-status/1322193003/200001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/200001
https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/24 14:01:42, stuartmorgan wrote: > On 2015/09/22 22:43:04, eugenebut wrote: > > On 2015/09/22 20:30:27, stuartmorgan wrote: > > > Should we maybe add a design doc to http://chromium.org explaining this > > system, and > > > link to it here? > > What exactly do you mean by "the system"? > > I was thinking of the relationship between how we use the two different > verification systems. I filed a bug to write a public design doc for this: crbug.com/535652 https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/160001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:936: NSArray* chain = [_wkWebView performSelector:@selector(certificateChain)]; On 2015/09/24 14:01:43, stuartmorgan wrote: > On 2015/09/22 22:43:04, eugenebut wrote: > > Instead of predeclaring I added ifdefs here, because it looks cleaner. > > It may look cleaner, but it causes the code not to actually run on iOS 9 if > built with the iOS 8 SDK, which is not the outcome we want. > > Predeclaring is our standard pattern for writing code against an SDK we don't > have completely rolled out yet (for exactly this reason). Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:57: // |completionHandler| on completion. |host| should be in DNS form "DNS form" is still quite ambiguous :) |host| should be in ASCII compatible form (e.g. for "http://...", it should be "xn--...") https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:163: scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(certChain)); DANGER: What about when this conversion fails? It can. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:167: // SecTrust accepts. I realize you copied both comments from me, but it seems like line 166 is inconsistent with line 177 - this suggests that it's because CertVerifier may reject them, but then line 177 explains why we ignore CertVerifier's revocations. I think this requires more care in thinking about how we want to handle these edge cases. For example, a revoked status from net::CertVerifier - should we trust that (it came from either a CRLSet or active revocation check)? I'm quite torn here in figuring out the correct behaviour, and I don't recall it being captured in the design doc. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. I'm still having trouble reconciling whether or not this is correct. Consider the situation like StartCom, where we have multiple possible paths for a given server-supplied certChain. That is, StartCom has two intermediates that are otherwise identical, save for the signature algorithms employed. net::CertVerifier may see and construct the chain using the SHA-1 path, while SecTrust may use the SHA-2 path. Or we can consider the inverse (CertVerifier preferring SHA-2, SecTrust preferring SHA-1) In either case, it feels like this code will do the 'wrong' thing with respect to status bits. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:24: const char kValidCertFileName[] = "twitter-chain.pem"; Don't use this chain. You should avoid all certificates documented in https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/certi... or https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/certi... and ideally explain what you need in the tests and figure out if we should add new test certificates to guarantee to meet the test constraints. https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (SSLStatus.cert_id == certID) { BUG? Not just the same cert - the same cert & hostname. It's unclear whether or not that is implicitly keyed off the navigation entry. https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:947: int oldCertID = SSLStatus.cert_id; Same remarks re: hostname
https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:165: // enacted. This is called for all certificates (valid and invalid alike), Aren't we "not supposed to end up here", per https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR... ?
Thanks! PTAL https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:57: // |completionHandler| on completion. |host| should be in DNS form On 2015/09/24 22:25:32, Ryan Sleevi wrote: > "DNS form" is still quite ambiguous :) > > |host| should be in ASCII compatible form (e.g. for "http://...", it should be > "xn--...") Updated the comment. Do I need to add a DCHECK which ensures than content is ASCII? https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:163: scoped_refptr<net::X509Certificate> cert(web::CreateCertFromChain(certChain)); On 2015/09/24 22:25:32, Ryan Sleevi wrote: > DANGER: What about when this conversion fails? It can. If cert is null then CertVerifier responds with error and default CertVerifyResult: https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/net/cert_v... Please let me know if it's an issue. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:165: // enacted. This is called for all certificates (valid and invalid alike), On 2015/09/24 22:45:31, Ryan Sleevi wrote: > Aren't we "not supposed to end up here", per > https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR... > ? Seem to be fine. Please see: - "For computing lock status / OIB info:" section in the doc you linked; - Adrienne's comment in the same doc: "I'm suggesting that we may also want to run Cert Verifier even when SecTrust returns "valid". See the bottom left quadrant. That lets us do Chrome-specific policies."; - other comment in this CL: https://codereview.chromium.org/1322193003/#msg9. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:167: // SecTrust accepts. On 2015/09/24 22:25:32, Ryan Sleevi wrote: > I realize you copied both comments from me, but it seems like line 166 is > inconsistent with line 177 - this suggests that it's because CertVerifier may > reject them, but then line 177 explains why we ignore CertVerifier's > revocations. Updated the comments. Please let me know if something is still confusing. > I think this requires more care in thinking about how we want to handle these > edge cases. For example, a revoked status from net::CertVerifier - should we > trust that (it came from either a CRLSet or active revocation check)? Adrienne decided on the following approach: - use SecTrust API for load/no-load decision and lock color - use CertVerifier to get a reason why cert is invalid (if SecTrust considers cert as invalid). - use CertVerifier to get cert status bits for valid and invalid certs (f.e. to support SHA-1 deprecation) This should be captured here: https://docs.google.com/document/d/1OH6anqpI5ehMIUJdj5F3RLHVhOTpq4EALEuS_tOIR... So if cert was revoked, but SecTrust API missed that for some reason then the load will happen anyway. And if the load has happened there is not much value for changing SSL lock color to red (f.e. cookies were already sent to the server). That is how I understand the problem but I'm totally can be wrong here. So I will ask Adrienne to comments in this. https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/24 22:25:32, Ryan Sleevi wrote: > I'm still having trouble reconciling whether or not this is correct. > > Consider the situation like StartCom, where we have multiple possible paths for > a given server-supplied certChain. That is, StartCom has two intermediates that > are otherwise identical, save for the signature algorithms employed. > > net::CertVerifier may see and construct the chain using the SHA-1 path, while > SecTrust may use the SHA-2 path. Or we can consider the inverse (CertVerifier > preferring SHA-2, SecTrust preferring SHA-1) > > In either case, it feels like this code will do the 'wrong' thing with respect > to status bits. This code uses the same chain for constructing SecTrustRef and net::X509Certificate (which will be passed to CertVerifier). Do you think that CertVerifier can ignore provided chain and use a different one? Am I missing something? https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:24: const char kValidCertFileName[] = "twitter-chain.pem"; On 2015/09/24 22:25:32, Ryan Sleevi wrote: > Don't use this chain. Then I don't have a way to test querySSLStatusForCertChain: for cases when SecTrust considers the cert as valid. Should I just remove the test? > You should avoid all certificates documented in > https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/certi... > or > https://code.google.com/p/chromium/codesearch#chromium/src/net/data/ssl/certi... > and ideally explain what you need in the tests and figure out if we should add > new test certificates to guarantee to meet the test constraints. I need a cert chain, which will be considered valid by SecTrustRef. Do you have any suggestions for that use case? https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (SSLStatus.cert_id == certID) { On 2015/09/24 22:25:32, Ryan Sleevi wrote: > BUG? Not just the same cert - the same cert & hostname. It's unclear whether or > not that is implicitly keyed off the navigation entry. Yes, this was a bug. Fixed. https://codereview.chromium.org/1322193003/diff/200001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:947: int oldCertID = SSLStatus.cert_id; On 2015/09/24 22:25:32, Ryan Sleevi wrote: > Same remarks re: hostname This can also be a problem in case of redirect. Fixed.
https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/25 00:28:10, eugenebut wrote: > This code uses the same chain for constructing SecTrustRef and > net::X509Certificate (which will be passed to CertVerifier). Do you think that > CertVerifier can ignore provided chain and use a different one? Am I missing > something? Yes, that was the point I was trying to clarify - there is no guarantee (for either SecTrustRef or CertVerifier) that they will use, respect, or make conclusions on the supplied chain (as a whole). The only API contract guarantee is they'll provide a response for the server cert, but a given cert may participate in multiple chains (since certificates are a directed, cyclic graph) https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:24: const char kValidCertFileName[] = "twitter-chain.pem"; On 2015/09/25 00:28:10, eugenebut wrote: > On 2015/09/24 22:25:32, Ryan Sleevi wrote: > > Don't use this chain. > Then I don't have a way to test querySSLStatusForCertChain: for cases when > SecTrust considers the cert as valid. Should I just remove the test? If you can modify the SecTrustRef prior to evaluation, you can use SecTrustSetAnchorCertificates prior to the evaluation - https://developer.apple.com/library/ios/documentation/Security/Reference/cert...
https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/25 01:05:06, Ryan Sleevi wrote: > On 2015/09/25 00:28:10, eugenebut wrote: > > This code uses the same chain for constructing SecTrustRef and > > net::X509Certificate (which will be passed to CertVerifier). Do you think that > > CertVerifier can ignore provided chain and use a different one? Am I missing > > something? > > Yes, that was the point I was trying to clarify - there is no guarantee (for > either SecTrustRef or CertVerifier) that they will use, respect, or make > conclusions on the supplied chain (as a whole). The only API contract guarantee > is they'll provide a response for the server cert, but a given cert may > participate in multiple chains (since certificates are a directed, cyclic graph) 😭 https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:24: const char kValidCertFileName[] = "twitter-chain.pem"; On 2015/09/25 01:05:06, Ryan Sleevi wrote: > On 2015/09/25 00:28:10, eugenebut wrote: > > On 2015/09/24 22:25:32, Ryan Sleevi wrote: > > > Don't use this chain. > > Then I don't have a way to test querySSLStatusForCertChain: for cases when > > SecTrust considers the cert as valid. Should I just remove the test? > > If you can modify the SecTrustRef prior to evaluation, you can use > SecTrustSetAnchorCertificates prior to the evaluation - > https://developer.apple.com/library/ios/documentation/Security/Reference/cert... I changed API to take SecTrustRef, so I can make it valid (even for invalid certs). Removed usage of real-world cert chain.
Please take a look at updated CL! https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/200001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:180: // SHA-1 or requiring Certificate Transparency. On 2015/09/25 17:03:10, eugenebut wrote: > On 2015/09/25 01:05:06, Ryan Sleevi wrote: > > On 2015/09/25 00:28:10, eugenebut wrote: > > > This code uses the same chain for constructing SecTrustRef and > > > net::X509Certificate (which will be passed to CertVerifier). Do you think > that > > > CertVerifier can ignore provided chain and use a different one? Am I missing > > > something? > > > > Yes, that was the point I was trying to clarify - there is no guarantee (for > > either SecTrustRef or CertVerifier) that they will use, respect, or make > > conclusions on the supplied chain (as a whole). The only API contract > guarantee > > is they'll provide a response for the server cert, but a given cert may > > participate in multiple chains (since certificates are a directed, cyclic > graph) > 😭 Per discussion with felt@ we will not use CertVerifier if cert is good according to SecTrust API.
eugenebut@chromium.org changed reviewers: + davidben@chromium.org
Hi David, Ryan cannot do reviews this week, but we need to land this CL as soon as possible. So we would appreciate if you could review this CL. Thanks!
https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:37: // Completion handler called by decidePolicyForCert:host:completionHandler:. querySSLStatusForTrust? https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:87: // synchronously on current thread of worker task can't start. of -> if the https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:87: // synchronously on current thread of worker task can't start. As with the previous CL, the thread-hopping here gets really confusing. I think it would be clearer if you kept the core logic on one thread. (Note that now verifyCert must be callable on two different threads.) You can make verifyCert's callback internally post back to the main thread without changing anything since you're always posting back to the main thread right now. Likewise, verifyTrust's ought to as well. It is, strictly speaking, one extra thread hop, but only on invalid certificates and the result is the code is much easier to reason about. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:157: // Completion handler of |verifyCert:forHost:completionHandler:| will be Nit: Completion -> The completion https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:157: // Completion handler of |verifyCert:forHost:completionHandler:| will be verifyCert:forHost -> verifyTrust? https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:181: forHost:host This will end up calling verifyCert if posting to the WorkerPool failed, but that doesn't make a whole lot of sense since the certificate itself wasn't invalid. There was just an internal error. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:251: // network requests. IO thread should not be blocked by that operation. Nit: The IO thread https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID Yikes. What exactly do you all use cert IDs for? In Chrome, this is some machinery for dealing with multiple processes and I need to get it out of the SSLStatus struct for other work anyway. That doesn't seem applicable here at all. Context here is https://crbug.com/513315. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:958: if (oldCertID != SSLStatus.cert_id || I don't believe this can ever be false. Looking at how Chrome/iOS implements CertStore, it appears to compare by pointers alone, and web::CreateCertFromChain always makes a fresh on. (See above for why you shouldn't depend on cert IDs don't interesting and below for why you would want them to do anything interesting anyway.) https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:962: usingCertChain:chain]; Why update all navigation entries? This is confusing when this function's name suggests it only updates the current one. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:975: false); UMA_HISTOGRAM macros expand to a fair amount of code. It emits a static thing and everything. It's better to have only one instance of it and compute a boolean somewhere. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:104: SecPolicyCreateSSL(TRUE, static_cast<CFStringRef>(host))); (Shouldn't this be YES, or is that a different boolean?)
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/patch-status/1322193003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/320001
Thanks! PTAL https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:37: // Completion handler called by decidePolicyForCert:host:completionHandler:. On 2015/10/05 22:19:10, David Benjamin wrote: > querySSLStatusForTrust? Done. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:87: // synchronously on current thread of worker task can't start. On 2015/10/05 22:19:11, David Benjamin wrote: > of -> if the Done. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:87: // synchronously on current thread of worker task can't start. On 2015/10/05 22:19:11, David Benjamin wrote: > As with the previous CL, the thread-hopping here gets really confusing. I think > it would be clearer if you kept the core logic on one thread. (Note that now > verifyCert must be callable on two different threads.) > > You can make verifyCert's callback internally post back to the main thread > without changing anything since you're always posting back to the main thread > right now. Likewise, verifyTrust's ought to as well. It is, strictly speaking, > one extra thread hop, but only on invalid certificates and the result is the > code is much easier to reason about. verifyCert: and verifyTrust: will be called for making load/no-load decision for bad SSL certs. I do understand that it's not a general case, but I still want to avoid bouncing between threads for performance reasons. For load-no load decision I can avoid 2 extra hops (marked with asterisk): UI (called on UI) -> Worker (SecTrust) -> *UI* -> IO (CertVerifier) -> *UI* -> IO (UserDecisionPolicy) -> UI (reply callback) https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:157: // Completion handler of |verifyCert:forHost:completionHandler:| will be On 2015/10/05 22:19:10, David Benjamin wrote: > verifyCert:forHost -> verifyTrust? Updated comments. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:157: // Completion handler of |verifyCert:forHost:completionHandler:| will be On 2015/10/05 22:19:11, David Benjamin wrote: > Nit: Completion -> The completion Done. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:181: forHost:host On 2015/10/05 22:19:11, David Benjamin wrote: > This will end up calling verifyCert if posting to the WorkerPool failed, but > that doesn't make a whole lot of sense since the certificate itself wasn't > invalid. There was just an internal error. Good point. Fixed. https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:251: // network requests. IO thread should not be blocked by that operation. On 2015/10/05 22:19:11, David Benjamin wrote: > Nit: The IO thread Done. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID On 2015/10/05 22:19:11, David Benjamin wrote: > Yikes. What exactly do you all use cert IDs for? In Chrome, this is some > machinery for dealing with multiple processes and I need to get it out of the > SSLStatus struct for other work anyway. That doesn't seem applicable here at > all. > > Context here is https://crbug.com/513315. Fetching CertStatus is asynchronous operation, so by the time when it's completed *current* NavigationItem may be changed (or even removed completely). The only reliable way to update requested Navigation Item is to find it by cert/host pair. Since multiple navigation items may have same cert/host pair I'm updating them all. I can stop using cert_id, once SSLStatus has cert field, but for now there is no other way. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:958: if (oldCertID != SSLStatus.cert_id || On 2015/10/05 22:19:11, David Benjamin wrote: > I don't believe this can ever be false. Looking at how Chrome/iOS implements > CertStore, it appears to compare by pointers alone, and web::CreateCertFromChain > always makes a fresh on. > > (See above for why you shouldn't depend on cert IDs don't interesting and below > for why you would want them to do anything interesting anyway.) This can be false and w/o this check cert status will be recomputed when unnecessary. SSLStatus does not have any other way to identify cert, so I have to use cert_id at least for now. web::CertStore compares certs using LessThen comparator, not pointers: https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/net/reques... https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:962: usingCertChain:chain]; On 2015/10/05 22:19:11, David Benjamin wrote: > Why update all navigation entries? This is confusing when this function's name > suggests it only updates the current one. Replied above. I did not find a good way to find current Navigation Item at the point when CertStatus is evaluated. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:975: false); On 2015/10/05 22:19:11, David Benjamin wrote: > UMA_HISTOGRAM macros expand to a fair amount of code. It emits a static thing > and everything. It's better to have only one instance of it and compute a > boolean somewhere. Moved to a separate function. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/wk_w... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/wk_w... ios/web/web_state/wk_web_view_security_util.mm:104: SecPolicyCreateSSL(TRUE, static_cast<CFStringRef>(host))); On 2015/10/05 22:19:11, David Benjamin wrote: > (Shouldn't this be YES, or is that a different boolean?) YES is for Objective-C BOOL type. TRUE is for C Boolean type.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID On 2015/10/06 03:10:09, eugenebut wrote: > On 2015/10/05 22:19:11, David Benjamin wrote: > > Yikes. What exactly do you all use cert IDs for? In Chrome, this is some > > machinery for dealing with multiple processes and I need to get it out of the > > SSLStatus struct for other work anyway. That doesn't seem applicable here at > > all. > > > > Context here is https://crbug.com/513315. > Fetching CertStatus is asynchronous operation, so by the time when it's > completed *current* NavigationItem may be changed (or even removed completely). > The only reliable way to update requested Navigation Item is to find it by > cert/host pair. Since multiple navigation items may have same cert/host pair I'm > updating them all. I can stop using cert_id, once SSLStatus has cert field, but > for now there is no other way. Per offline discussion, let's have the navigation item's UniqueID be the primary lookup, and then you can just validate that the answer still applies to that item (e.g., that a redirect hasn't changed the URL). https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:170: } Shouldn't this case early return? https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:176: dispatch_async(dispatch_get_main_queue(), ^{ I'm confused here; the failure case calls the callback on the UI thread (the DCHECK above), but the success case calls it on a different thread? https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:86: #if !defined(__IPHONE_9_0) You're missing: || __IPHONE_OS_VERSION_MAX_ALLOWED < __IPHONE_9_0 https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: int currentItemIndex = navigationManager->GetCurrentEntryIndex(); Are we guaranteed that the lookup can't be for a pending item? I'm pretty sure this will only get the last committed item. Maybe a DCHECK would be a good idea, since if that's not true we presumably want to also check/update the pending entry. https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:948: // Retrieve top level frame certificate. Since the innermost code in the block is updating something, not just retrieving, this seems misleading. Maybe you can factor out some of the code that doesn't modify anything into a helper method to make the separation of responsibilities cleaner? https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:959: NSString* host = [_wkWebView URL].host; Are you sure you want the raw web view URL, rather than the WC's understanding of the URL? If so, please comment why here.
PTAL https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID On 2015/10/06 22:02:36, stuartmorgan wrote: > On 2015/10/06 03:10:09, eugenebut wrote: > > On 2015/10/05 22:19:11, David Benjamin wrote: > > > Yikes. What exactly do you all use cert IDs for? In Chrome, this is some > > > machinery for dealing with multiple processes and I need to get it out of > the > > > SSLStatus struct for other work anyway. That doesn't seem applicable here at > > > all. > > > > > > Context here is https://crbug.com/513315. > > Fetching CertStatus is asynchronous operation, so by the time when it's > > completed *current* NavigationItem may be changed (or even removed > completely). > > The only reliable way to update requested Navigation Item is to find it by > > cert/host pair. Since multiple navigation items may have same cert/host pair > I'm > > updating them all. I can stop using cert_id, once SSLStatus has cert field, > but > > for now there is no other way. > > Per offline discussion, let's have the navigation item's UniqueID be the primary > lookup, and then you can just validate that the answer still applies to that > item (e.g., that a redirect hasn't changed the URL). Done. https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:962: usingCertChain:chain]; On 2015/10/06 03:10:09, eugenebut wrote: > On 2015/10/05 22:19:11, David Benjamin wrote: > > Why update all navigation entries? This is confusing when this function's name > > suggests it only updates the current one. > Replied above. I did not find a good way to find current Navigation Item at the > point when CertStatus is evaluated. Actually there is a way to update only requested entry. Fixed. https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:170: } On 2015/10/06 22:02:36, stuartmorgan wrote: > Shouldn't this case early return? Done. https://codereview.chromium.org/1322193003/diff/340001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:176: dispatch_async(dispatch_get_main_queue(), ^{ On 2015/10/06 22:02:36, stuartmorgan wrote: > I'm confused here; the failure case calls the callback on the UI thread (the > DCHECK above), but the success case calls it on a different thread? Yes. In failure case WorkerThread did not start the task so currect thread is UI. In success case callback is called on worker thread (to avoid unnecessary bouncing to UI thread). David also thinks that threading is confusing, but I wanted to optimize it for performance, which I tried to explain here: https://codereview.chromium.org/1322193003/diff/280001/ios/web/net/crw_cert_v... https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:86: #if !defined(__IPHONE_9_0) On 2015/10/06 22:02:36, stuartmorgan wrote: > You're missing: > || __IPHONE_OS_VERSION_MAX_ALLOWED < __IPHONE_9_0 Done. https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: int currentItemIndex = navigationManager->GetCurrentEntryIndex(); On 2015/10/06 22:02:36, stuartmorgan wrote: > Are we guaranteed that the lookup can't be for a pending item? I'm pretty sure > this will only get the last committed item. > > Maybe a DCHECK would be a good idea, since if that's not true we presumably want > to also check/update the pending entry. There is no need to do a lookup for a pending item. Lookup is happening only for currentNavigationItem, to check if we need to call webControllerDidUpdateSSLStatusForCurrentNavigationItem: delegate method. That delegate method is not supposed to be called for pending item. Please let me know if I misunderstood your question. https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:948: // Retrieve top level frame certificate. On 2015/10/06 22:02:36, stuartmorgan wrote: > Since the innermost code in the block is updating something, not just > retrieving, this seems misleading. Maybe you can factor out some of the code > that doesn't modify anything into a helper method to make the separation of > responsibilities cleaner? Updated comment. I can't really factor much of the code, unless I create an ugly method like: -getCert:certChain:forNavigationItem: But I'm open for suggestions. https://codereview.chromium.org/1322193003/diff/340001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:959: NSString* host = [_wkWebView URL].host; On 2015/10/06 22:02:36, stuartmorgan wrote: > Are you sure you want the raw web view URL, rather than the WC's understanding > of the URL? If so, please comment why here. I wanted to avoid std::string -> NSString conversion, but it's not really a good reason. Changed to use |_documentURL|.
High-level proposal that probably invalidates most of my individual comments: I had a much longer thing and then I realized it was equivalent to this, so here's the shorter version. Except this turned out long anyway, so... :-) So, it seems there's a lot of complications and costs here around doing lots of certificate verifications and especially around updateCurrentNavigationItemSSLStatusUsingCertChain being asynchronous. Also not clear which of the two certificate verifiers is ground truth. I think we can avoid much of this. At a high-level, there's three operations you need to be able to do: 1. Do we block a particular request or no? (Ideally, this involves a certificate verification combined with checking CertificatePolicyCache.) 2. For a successful navigation, how do we decorate the lock icon? (Ideally, this involves synchronously looking up certificate verification information stashed away from operation 1) 3. For a failed navigation, if it was a certificate error, display a prompt. (Ideally, this involves looking up info from operation one again, prompting, and, if clicked through, updating CertificatePolicyCache.) In normal Chrome, we don't have this problem because we can flow whatever information we want from step 1 to steps 2 and 3. And so 2 can be synchronous. Ground truth: - Continue to let SecTrustEvaluate serve as the source of ground truth on the yes/no portion of certificate verification. You doc suggests that didFailProvisionalNavigation doesn't get even get the certificate attached if we don't let iOS do it's default thing, which suggests we really ought to lean on that. - Use CertVerifier as the source of detailed errors and nothing more. It does *not* perform a yes/no decision for us. If CertVerifier returns YES on something we know SecTrustEvaluate said NO on, this turns into some UNKNOWN error or a generic one (INVALID?) or whatever. This is mostly to throw out the "what if CertVerifier and SecTrustEvaluate disagree" case. SecTrustEvaluate is the source of ground truth so we should be consistent and NOT a priori accept certificates that CertVerifier accepted. Assumption: WKWebView interprets didReceiveAuthenticationChallenge on a certificate error as follows: - NSURLSessionAuthChallengeUseCredential means "accept this certificate, don't bother verifying it yourself". - NSURLSessionAuthChallengeCancelAuthenticationChallenge means "reject this certificate and fail the request, don't bother verifying it yourself". - NSURLSessionAuthChallengePerformDefaultHandling means "verify it yourself and do everything as you would have before; if you think the cert is bad, treat it as bad". Assumption: If WKWebView loads something that means that that either: iOS internally (presumably via SecTrustEvaluate) believes the certificate is good. - OR - We explicitly let the certificate through via NSURLSessionAuthChallengeUseCredential The second assumption is the key one. In step 2, we can assume that any bad certificates came from CertificatePolicyCache, so there's no need to call CertVerifier! Just look it up from the CertificatePolicyCache. Okay, first there's a problem that CertificatePolicyCache is currently an IO thread object, but that's resolvable. The bigger problem is that CertificatePolicyCache is not keyed on (host, certificate), but (host, certificate, cert_status). This is because we showed the user one error and if, for whatever reason, got a different error, we prrrroooobably shouldn't let the previous exception stand? (It's unclear how big of a deal this is due to things like not re-validating on socket reuse, but it's not very important.) So instead: CRWWKWebViewWebController maintains a std::map from (hostname, certificate) to net::CertStatus. This map contains the most recent didReceiveAuthenticationChallenge exceptions (and only the exceptions). We never bother clearing it, but since it only stores exceptions, it's not going to store any more things than CertificatePolicyCache does (at most one entry per click-through). Let's call it _lastCertificateException. didReceiveAuthenticationChallenge: - Optimization: If CertificatePolicyCache has no entries for host or (host, cert) at all, return NSURLSessionAuthChallengePerformDefaultHandling and don't bother verifying twice. - Verify with CertVerifier - If NOT success, look up (host, cert, status) in CertificatePolicyCache. If found: - Set _lastCertificateException[host, cert] = status; - return NSURLSessionAuthChallengeUseCredential - return NSURLSessionAuthChallengePerformDefaultHandling updateSSLStatusStuff, called on didCommitNavigation and friends: - If host & cert didn't change, do nothing. - Look up (host, cert) in _lastCertificateException. - If found, set cert_status to that. - Otherwise, set it to success. Assume there was no certificate error. - This one happens synchronously and entirely on the UI thread. didFailProvisionalNavigation: - Extract certificate from error. - Verify with CertVerifier again. (Note that this won't be in _lastCertificateException because of didReceiveAuthenticationChallenge.) - If CertVerifier says success, this is one of those mismatches. Pretend it said UNKNOWN or INVALID - Display an SSL error - If the user clicks through, add to CertPolicyCache Good things: - We never have to call SecTrustEvaluate ourselves ever. - Yes/no decisions are consistent with iOS's default behavior combined with a CertVerifier-augmented CertificatePolicyCache. - SSLStatus is updated synchronously! - The common path (no click-throughs and no certificate errors) does *zero* unnecessary verifications! Bad things: This does have a few small-ish weirdnesses around lock icon coloring. Because we don't call SecTrustEvaluate ourselves ever and instead use _lastCertificateException as signal, if SecTrustEvaluate first doesn't allow some (leaf, host) pair, THEN the user clicks through, THEN SecTrustEvaluate does allow it, we'll show red when we may as well showed green. I think this isn't a huge deal, especially since it required a click-through to begin with. Although note that one way for it to switch is if the server gave us different chains since CertificatePolicyCache only keys by leaf. (I suppose we could switch it to keying by chain...) (By the way, a question: what does [webView certificateChain] return if the default behavior was used? If we bypassed?) Thoughts? https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID On 2015/10/07 15:27:13, eugenebut wrote: > On 2015/10/06 22:02:36, stuartmorgan wrote: > > On 2015/10/06 03:10:09, eugenebut wrote: > > > On 2015/10/05 22:19:11, David Benjamin wrote: > > > > Yikes. What exactly do you all use cert IDs for? In Chrome, this is some > > > > machinery for dealing with multiple processes and I need to get it out of > > the > > > > SSLStatus struct for other work anyway. That doesn't seem applicable here > at > > > > all. > > > > > > > > Context here is https://crbug.com/513315. > > > Fetching CertStatus is asynchronous operation, so by the time when it's > > > completed *current* NavigationItem may be changed (or even removed > > completely). > > > The only reliable way to update requested Navigation Item is to find it by > > > cert/host pair. Since multiple navigation items may have same cert/host pair > > I'm > > > updating them all. I can stop using cert_id, once SSLStatus has cert field, > > but > > > for now there is no other way. > > > > Per offline discussion, let's have the navigation item's UniqueID be the > primary > > lookup, and then you can just validate that the answer still applies to that > > item (e.g., that a redirect hasn't changed the URL). > > Done. Oh, actually, I'd missed that you don't use content::SSLStatus and instead have your own copy of everything. So me messing with content::SSLStatus won't affect you. (Whether you want to switch away from cert IDs or no I'll leave to you.) https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:958: if (oldCertID != SSLStatus.cert_id || On 2015/10/06 03:10:09, eugenebut wrote: > On 2015/10/05 22:19:11, David Benjamin wrote: > > I don't believe this can ever be false. Looking at how Chrome/iOS implements > > CertStore, it appears to compare by pointers alone, and > web::CreateCertFromChain > > always makes a fresh on. > > > > (See above for why you shouldn't depend on cert IDs don't interesting and > below > > for why you would want them to do anything interesting anyway.) > This can be false and w/o this check cert status will be recomputed when > unnecessary. SSLStatus does not have any other way to identify cert, so I have > to use cert_id at least for now. > > web::CertStore compares certs using LessThen comparator, not pointers: > https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/net/reques... > Ah, you're right. I'd missed that. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:66: // Asynchronously provides web::SecurityStyle and net::CertStatus for the given ("Asynchronously" is currently not true. See comment in .mm file.) https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:80: // and will be called asynchronously on IO thread. Nit: If you're sticking with the callbacks as they are, add "May be called on any thread." https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:88: // case |success| argument will be NO). We could avoid some of the two-thread weirdness by having this function return BOOL for whether it successfully posted or failed synchronously. Then perhaps completionHandler should be named completionHandlerOnIO and completionHandlerOnWorker so it's clear the callbacks are not called in the standard way. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:155: completionHandler:(web::StatusQueryHandler)completionHandler { This callback currently almost always runs asynchronously, but in very rare edge cases will run synchronously. You should the callback in the synchronous case by a dispatch_async. Otherwise higher-level code like updateSSLStatusForCurrentNavigationItem needs to account for updateCurrentNavigationItemSSLStatusUsingCertChain possibly doing something and possibly not doing something immediately. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). This is very possible as their implementations have nothing to do with each other. You won't need UMA to determine if it happens. It's probably even possible that two SecTrustEvaluate calls give different answers. This is a very non-deterministic process and you can't make any assumptions on behavior. Make sure your code is okay with this. E.g. you may get didFailProvisionalLoad and have to show an interstitial on a good certificate. I would probably explicitly filter out the ones NSS liked because the rest of your system, based on the design doc, isn't going to accept those certs. Perhaps turn them into some generic error. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:261: // network requests. The IO thread should not be blocked by that operation. IO -> UI? This code I believe runs on the UI thread, so the mention of IO is strange. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:47: NSArray* chain = GetChain(cert_); [Assuming some ObjC magic is what keeps this from leaking.] https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:884: - (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain Should this also be under the ifdef? https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:884: - (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain Nit: I'd maybe name this something with the word "schedule" in the name, so it's clear this doesn't happen synchronously. Maybe scheduleSSLStatusUpdateForCurrentNavigationItem? https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:963: [self reportHasCertForSecureConnectionUMAWithValue:true]; Should this be in line 968? Seems you don't want to record anything when the certificate hasn't changed. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || Could you add a comment here like: // Only recompute the SSLStatus information if the certificate has since changed. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || If events cause updateSSLStatusForCurrentNavigationItem to be called multiple times in a row, I'm not sure this check quite works since SSLStatus only updates when the asynchronous operation completes. Perhaps set cert_id synchronously and then add some kind of pending state? (Alternatively, see top-level comment.) https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1488: host:challenge.protectionSpace.host I'm assuming this will be switched to pass in the SecTrustRef and not call the CertVerifier. Can you add a TODO here so it's clear what the relationship is? (The current arrangement doesn't make any sense. You can't use CertVerifier in the low-level callback here and SecTrustEvaluate in the high-level one.)
On 2015/10/07 20:16:30, David Benjamin wrote: > The bigger problem is that > CertificatePolicyCache is not keyed on (host, certificate), but (host, > certificate, cert_status). This is because we showed the user one error and if, > for whatever reason, got a different error, we prrrroooobably shouldn't let the > previous exception stand? (It's unclear how big of a deal this is due to things > like not re-validating on socket reuse, but it's not very important.) By the way, if we decide we completely do not care about this, we can simplify this further and make didFailProvisionalNavigation the only thing that calls CertVerifier and is even asynchronous at all. But it's easy enough to allow for this, modulo the small quirks around data flowing from step 1 to step 2, so I figure we may as well.
(Oh, to clarify, this proposal does NOT require CertificatePolicyCache become thread-safe or move to the UI thread. I think some stuff could get a marginally simpler if you did, but it's not a big deal either way.)
Thanks David, I divided cert verification problem into 2 parts (and 2 CLs): 1.) Updating SSL Lock Status (this CL) 2.) Accepting bad SSL certs and showing interstitials (https://codereview.chromium.org/1357773002/) For the second problem I use exactly same concept as you described with |_lastCertificateException| (though Ryan, does not like the fact that cert/hostname is used as a cache key). For the first problem we can't use |_lastCertificateException| because didReceiveAuthenticationChallenge is not called in case if a page is loaded from caches. I will reply to you other comments separately once I address them, but for now I just wanted to give you quick update on your proposal. Thanks!
On 2015/10/07 20:33:31, eugenebut wrote: > For the first problem we can't use |_lastCertificateException| because > didReceiveAuthenticationChallenge is not called in case if a page is loaded from > caches. Ick. Good point. I hadn't thought of the HTTP cache. Chrome proper will never caching anything on a certificate click-through. Yeah, if you want to be able to mark those as red, I don't see a way to avoid making that asynchronous. Hrmf. :-/ Yeah, then you might need to keep track of weird "pending" bits on SSLStatus and other fun and exciting things. This does mean you'll randomly switch between red and requiring a click-through after a browser restart. Though I suppose randomly switching between green and requiring a click-through isn't much different.
https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:155: completionHandler:(web::StatusQueryHandler)completionHandler { On 2015/10/07 20:16:29, David Benjamin wrote: > This callback currently almost always runs asynchronously, but in very rare edge > cases will run synchronously. You should the callback in the synchronous case by > a dispatch_async. Otherwise higher-level code like > updateSSLStatusForCurrentNavigationItem needs to account for > updateCurrentNavigationItemSSLStatusUsingCertChain possibly doing something and > possibly not doing something immediately. Alternately, how about we push that down the stack into verifyTrust:completionHandler:, so that it always calls back asynchronously (via dispatch_async in the rare case). https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:169: DCHECK_CURRENTLY_ON_WEB_THREAD(web::WebThread::UI); Instead of this, how about we dispatch_async(dispatch_get_main_queue() here exactly like all the other cases. Yes, it's pointless, but it makes this code significantly less confusing IMO, and it's an exceptionally rare case so the performance doesn't matter. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:884: - (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain On 2015/10/07 20:16:30, David Benjamin wrote: > Nit: I'd maybe name this something with the word "schedule" in the name, so it's > clear this doesn't happen synchronously. Maybe > scheduleSSLStatusUpdateForCurrentNavigationItem? +1. "begin" could also work. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:890: if (!item) Is this something that should actually be possible? (I.e., is this really something that should be handled vs DCHECKed?) https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:901: } Could you pull everything after this into a helper method? Long blocks of code after the strong/weak dance make me nervous, because it's *really* easy for someone to accidentally slip an ivar reference in without anyone noticing. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:910: for (int i = 0; i < navigationManager->GetEntryCount(); i++) { It should almost always be the last entry, so walk backward rather than forward. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:915: if (itemID == item->GetUniqueID() && SSLStatus.cert_id == certID && Flip the first check around; it's backward relative to all the other ones which makes it harder to read. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: return; You actually want to return after finding an itemID match regardless of whether the rest matched, so you'll need to break apart the conditional.
Thanks! PTAL https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:882: - (void)updateSSLStatusForNavigationItemsWithCertID:(int)certID On 2015/10/07 20:16:29, David Benjamin wrote: > On 2015/10/07 15:27:13, eugenebut wrote: > > On 2015/10/06 22:02:36, stuartmorgan wrote: > > > On 2015/10/06 03:10:09, eugenebut wrote: > > > > On 2015/10/05 22:19:11, David Benjamin wrote: > > > > > Yikes. What exactly do you all use cert IDs for? In Chrome, this is some > > > > > machinery for dealing with multiple processes and I need to get it out > of > > > the > > > > > SSLStatus struct for other work anyway. That doesn't seem applicable > here > > at > > > > > all. > > > > > > > > > > Context here is https://crbug.com/513315. > > > > Fetching CertStatus is asynchronous operation, so by the time when it's > > > > completed *current* NavigationItem may be changed (or even removed > > > completely). > > > > The only reliable way to update requested Navigation Item is to find it by > > > > cert/host pair. Since multiple navigation items may have same cert/host > pair > > > I'm > > > > updating them all. I can stop using cert_id, once SSLStatus has cert > field, > > > but > > > > for now there is no other way. > > > > > > Per offline discussion, let's have the navigation item's UniqueID be the > > primary > > > lookup, and then you can just validate that the answer still applies to that > > > item (e.g., that a redirect hasn't changed the URL). > > > > Done. > > Oh, actually, I'd missed that you don't use content::SSLStatus and instead have > your own copy of everything. So me messing with content::SSLStatus won't affect > you. (Whether you want to switch away from cert IDs or no I'll leave to you.) We want to keep web::SSLStatus as close as possible to content::SSLStatus. I CCed myself to https://crbug.com/513315 so we know when you get rid of cert_id. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:66: // Asynchronously provides web::SecurityStyle and net::CertStatus for the given On 2015/10/07 20:16:29, David Benjamin wrote: > ("Asynchronously" is currently not true. See comment in .mm file.) Fixed behavior, thanks! https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:80: // and will be called asynchronously on IO thread. On 2015/10/07 20:16:29, David Benjamin wrote: > Nit: If you're sticking with the callbacks as they are, add "May be called on > any thread." This very callback can be called only on IO thread. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:88: // case |success| argument will be NO). On 2015/10/07 20:16:29, David Benjamin wrote: > We could avoid some of the two-thread weirdness by having this function return > BOOL for whether it successfully posted or failed synchronously. > > Then perhaps completionHandler should be named completionHandlerOnIO and > completionHandlerOnWorker so it's clear the callbacks are not called in the > standard way. I thought about it. But it would be an anti pattern for Objective-C code. In Objective-C if there is a completion handler, it should be called for both success and failure cases. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:155: completionHandler:(web::StatusQueryHandler)completionHandler { On 2015/10/07 20:16:29, David Benjamin wrote: > This callback currently almost always runs asynchronously, but in very rare edge > cases will run synchronously. You should the callback in the synchronous case by > a dispatch_async. Otherwise higher-level code like > updateSSLStatusForCurrentNavigationItem needs to account for > updateCurrentNavigationItemSSLStatusUsingCertChain possibly doing something and > possibly not doing something immediately. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). On 2015/10/07 20:16:29, David Benjamin wrote: > This is very possible as their implementations have nothing to do with each > other. You won't need UMA to determine if it happens. > > It's probably even possible that two SecTrustEvaluate calls give different > answers. This is a very non-deterministic process and you can't make any > assumptions on behavior. We know that mismatch will happen, but we (including felt@) want to know how often it happens. Do you think that knowing how often mismatch happens is unnecessary? > Make sure your code is okay with this. E.g. you may get didFailProvisionalLoad > and have to show an interstitial on a good certificate. UX will be bad, but app can handle that gracefully. > I would probably explicitly filter out the ones NSS liked because the rest of > your system, based on the design doc, isn't going to accept those certs. Perhaps > turn them into some generic error. What do you mean by filter out? https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:261: // network requests. The IO thread should not be blocked by that operation. On 2015/10/07 20:16:29, David Benjamin wrote: > IO -> UI? This code I believe runs on the UI thread, so the mention of IO is > strange. A few patches ago this method was called on IO thread. Fixed comment. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller_unittest.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller_unittest.mm:47: NSArray* chain = GetChain(cert_); On 2015/10/07 20:16:30, David Benjamin wrote: > [Assuming some ObjC magic is what keeps this from leaking.] It is NSAutoreleasePool, which is drained at the end of each test. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:884: - (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain On 2015/10/07 20:16:30, David Benjamin wrote: > Should this also be under the ifdef? We will drop ENABLE_CHROME_NET_STACK_FOR_WKWEBVIEW soon, so it does not really matter. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:884: - (void)updateCurrentNavigationItemSSLStatusUsingCertChain:(NSArray*)chain On 2015/10/07 20:16:30, David Benjamin wrote: > Nit: I'd maybe name this something with the word "schedule" in the name, so it's > clear this doesn't happen synchronously. Maybe > scheduleSSLStatusUpdateForCurrentNavigationItem? Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:890: if (!item) On 2015/10/07 22:30:14, stuartmorgan wrote: > Is this something that should actually be possible? (I.e., is this really > something that should be handled vs DCHECKed?) Currently not possible. In Chromium DCHECKs are not used right before pointer dereferencing, so I'm just removing this early return. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:901: } On 2015/10/07 22:30:14, stuartmorgan wrote: > Could you pull everything after this into a helper method? Long blocks of code > after the strong/weak dance make me nervous, because it's *really* easy for > someone to accidentally slip an ivar reference in without anyone noticing. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:910: for (int i = 0; i < navigationManager->GetEntryCount(); i++) { On 2015/10/07 22:30:14, stuartmorgan wrote: > It should almost always be the last entry, so walk backward rather than forward. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:915: if (itemID == item->GetUniqueID() && SSLStatus.cert_id == certID && On 2015/10/07 22:30:14, stuartmorgan wrote: > Flip the first check around; it's backward relative to all the other ones which > makes it harder to read. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: return; On 2015/10/07 22:30:14, stuartmorgan wrote: > You actually want to return after finding an itemID match regardless of whether > the rest matched, so you'll need to break apart the conditional. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:963: [self reportHasCertForSecureConnectionUMAWithValue:true]; On 2015/10/07 20:16:30, David Benjamin wrote: > Should this be in line 968? Seems you don't want to record anything when the > certificate hasn't changed. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || On 2015/10/07 20:16:30, David Benjamin wrote: > Could you add a comment here like: > > // Only recompute the SSLStatus information if the certificate has since > changed. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || On 2015/10/07 20:16:30, David Benjamin wrote: > If events cause updateSSLStatusForCurrentNavigationItem to be called multiple > times in a row, I'm not sure this check quite works since SSLStatus only updates > when the asynchronous operation completes. Perhaps set cert_id synchronously and > then add some kind of pending state? > > (Alternatively, see top-level comment.) If |updateSSLStatusForCurrentNavigationItem| is called for the second time, then both certID and URL will match and cert revalidation will not be started. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1488: host:challenge.protectionSpace.host On 2015/10/07 20:16:30, David Benjamin wrote: > I'm assuming this will be switched to pass in the SecTrustRef and not call the > CertVerifier. Can you add a TODO here so it's clear what the relationship is? > > (The current arrangement doesn't make any sense. You can't use CertVerifier in > the low-level callback here and SecTrustEvaluate in the high-level one.) Yes, SecTrustRef will be passed. See other CL: https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/ui/cr...
lgtm https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:88: // TODO(eugenebut): cleanup this (crbug.com/523365). "Clean this up" https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:262: // Updates and |style| and |certStatus| for Navigation Item wich has given s/Updates and/Updates/ /for ... given/of the NavigationItem which the given/
https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:88: // TODO(eugenebut): cleanup this (crbug.com/523365). On 2015/10/09 20:28:23, stuartmorgan wrote: > "Clean this up" Done. https://codereview.chromium.org/1322193003/diff/420001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:262: // Updates and |style| and |certStatus| for Navigation Item wich has given On 2015/10/09 20:28:23, stuartmorgan wrote: > s/Updates and/Updates/ > /for ... given/of the NavigationItem which the given/ Done.
I think the main remaining issue is the async NavigationItem update thing, assuming we can't get away from having to do that async altogether. (The disk cache does rather tie our hands here, unfortunately. Unless we're okay with showing the wrong color for cached entries or persisting certificate click-throughs.) https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:80: // and will be called asynchronously on IO thread. On 2015/10/08 16:53:32, eugenebut wrote: > On 2015/10/07 20:16:29, David Benjamin wrote: > > Nit: If you're sticking with the callbacks as they are, add "May be called on > > any thread." > This very callback can be called only on IO thread. I mean verifyCert itself may be called on any thread. It used to be the case that you could assume these would only get called on the UI thread. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:88: // case |success| argument will be NO). On 2015/10/08 16:53:32, eugenebut wrote: > On 2015/10/07 20:16:29, David Benjamin wrote: > > We could avoid some of the two-thread weirdness by having this function return > > BOOL for whether it successfully posted or failed synchronously. > > > > Then perhaps completionHandler should be named completionHandlerOnIO and > > completionHandlerOnWorker so it's clear the callbacks are not called in the > > standard way. > I thought about it. But it would be an anti pattern for Objective-C code. In > Objective-C if there is a completion handler, it should be called for both > success and failure cases. Mm. I would have thought, in Objective-C, if there is a completion handler it should also always be called on the same thread. :-) https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). On 2015/10/08 16:53:32, eugenebut wrote: > On 2015/10/07 20:16:29, David Benjamin wrote: > > This is very possible as their implementations have nothing to do with each > > other. You won't need UMA to determine if it happens. > > > > It's probably even possible that two SecTrustEvaluate calls give different > > answers. This is a very non-deterministic process and you can't make any > > assumptions on behavior. > We know that mismatch will happen, but we (including felt@) want to know how > often it happens. Do you think that knowing how often mismatch happens is > unnecessary? > > Make sure your code is okay with this. E.g. you may get didFailProvisionalLoad > > and have to show an interstitial on a good certificate. > UX will be bad, but app can handle that gracefully. > > > I would probably explicitly filter out the ones NSS liked because the rest of > > your system, based on the design doc, isn't going to accept those certs. > Perhaps > > turn them into some generic error. > What do you mean by filter out? I meant you might want to turn those into a different SecurityStyle or something right now. But I suppose you haven't lost information, so it could be done later. Added a comment about documenting the weirdness in the public API instead. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || On 2015/10/08 16:53:32, eugenebut wrote: > On 2015/10/07 20:16:30, David Benjamin wrote: > > If events cause updateSSLStatusForCurrentNavigationItem to be called multiple > > times in a row, I'm not sure this check quite works since SSLStatus only > updates > > when the asynchronous operation completes. Perhaps set cert_id synchronously > and > > then add some kind of pending state? > > > > (Alternatively, see top-level comment.) > If |updateSSLStatusForCurrentNavigationItem| is called for the second time, then > both certID and URL will match and cert revalidation will not be started. Only if the asynchronous update process has completed already. If this function is called twice in a row, the update process probably won't have completed yet, and you'll trigger more and more of these. https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:67: // |serverTrust| and |host|. |host| should be in ASCII compatible form. I would add something like: // Note: The web::SecurityStyle determines whether the certificate is // trusted. It is possible for an untrusted certificate to return a // net::CertStatus with no errors if the cause could not be determined. // Callers must handle this case gracefully. https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:90: completionHandler:(void (^)(SecTrustResultType, BOOL success))handler; If you want to keep this double-threaded thing, perhaps success -> dispatched? success could mean lots of things (SecTrustEvaluate didn't fail, this cert is great, etc.) https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:262: // Updates |style| and |certStatus| for Navigation Item wich has given Perhaps: // Updates |security_style| and |cert_status| for the NavigationItem with ID |navigationItemID|, if URL and certificate chain still match |host| and |certChain|. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:893: certChain:(NSArray*)chain Here you use chain but in the prototype, it's certChain. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:908: // checking that cert and URL matche is necessary. matche -> match https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:968: web::SSLStatus& SSLStatus = item->GetSSL(); I don't think the C++ style guide allows non-const references. (I assume there isn't anything in Obj-C that overrides that, since this is purely a C++ thing.) https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { Does this check even do anything? item->GetURL() isn't uploaded in flow with SSLStatus. I don't think you want this check but to compare item->GetURL().host() (or _documentURL.host()? I couldn't tell when the former is updated, so I don't know what the difference between the two is) against *the hostname that security_style was computed against*. Otherwise if my current and previous states happen to share a leaf certificate, then the cert_id will match and security_style won't be changed. Probably SSLStatus or NavigationItem needs to grow a new field, say a std::string cert_host which gets updated whenever cert_id and security_status are updated. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:992: [self scheduleSSLStatusUpdateUsingCertChain:chain host:host]; Because this is asynchronous (and unless we decide we're okay showing green when you load from disk cache on a resource cached on a click-through, we can't avoid that), you may show contradicting things in the URL bar. Likewise, there's troubles with all this running multiple times if the asynchronous operation hasn't completed yet. I would suggest that scheduleSSLStatusUpdateUsingCertChain actually set SSLStatus.cert_id (and SSLStatus.cert_host---see comment above), clear cert_status, and then set security_style to a new SECURITY_STYLE_PENDING. Then the URL bar can decide how to display that one. Then you separately flip SECURITY_STYLE_PENDING. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1005: [self reportHasCertForSecureConnectionUMAWithValue:false]; This line will also run every time updateSSLStatusForCurrentNavigationItem is called, and you probably actually want every time... something changes. Perhaps keyed on a similar check to line 989, comparing against previousSSLStatus?
https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { On 2015/10/12 23:21:50, David Benjamin wrote: > Does this check even do anything? item->GetURL() isn't uploaded in flow with > SSLStatus. I don't think you want this check but to compare > item->GetURL().host() (or _documentURL.host()? I couldn't tell when the former > is updated, so I don't know what the difference between the two is) against *the > hostname that security_style was computed against*. > > Otherwise if my current and previous states happen to share a leaf certificate, > then the cert_id will match and security_style won't be changed. > > Probably SSLStatus or NavigationItem needs to grow a new field, say a > std::string cert_host which gets updated whenever cert_id and security_status > are updated. Right, a related problem here is: is it possible for _documentURL.host() to change without something else signaling updateSSLStatusForCurrentNavigationItem, like the chain switching? This is probably worth checking on. Have two hosts with the same chain. Let both through. Then navigate from one to the other. Do you still signal this function? This code is currently relying on this. (Unfortunately, if you can't rely on this, then it's ugly for other reasons. You depend on a both _documentURL.host() and on [_wkWebView certificateChain]. The pair *should* be updated atomically. But if iOS doesn't update them atomically, you could end up scheduling a bogus update temporarily. Which maybe that's fine? Just wasted effort. I could imagine waiting a very small timeout for things to settle before actually cashing in on the pending security_style and firing the verification.)
PTAL https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:80: // and will be called asynchronously on IO thread. On 2015/10/12 23:21:49, David Benjamin wrote: > On 2015/10/08 16:53:32, eugenebut wrote: > > On 2015/10/07 20:16:29, David Benjamin wrote: > > > Nit: If you're sticking with the callbacks as they are, add "May be called > on > > > any thread." > > This very callback can be called only on IO thread. > > I mean verifyCert itself may be called on any thread. It used to be the case > that you could assume these would only get called on the UI thread. Thanks for clarification. Done. https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:88: // case |success| argument will be NO). On 2015/10/12 23:21:49, David Benjamin wrote: > On 2015/10/08 16:53:32, eugenebut wrote: > > On 2015/10/07 20:16:29, David Benjamin wrote: > > > We could avoid some of the two-thread weirdness by having this function > return > > > BOOL for whether it successfully posted or failed synchronously. > > > > > > Then perhaps completionHandler should be named completionHandlerOnIO and > > > completionHandlerOnWorker so it's clear the callbacks are not called in the > > > standard way. > > I thought about it. But it would be an anti pattern for Objective-C code. In > > Objective-C if there is a completion handler, it should be called for both > > success and failure cases. > > Mm. I would have thought, in Objective-C, if there is a completion handler it > should also always be called on the same thread. :-) Ideally yes, completion handler should be called on the same thread. In order to optimize for performance some ugliness is necessary (either this method returns BOOL or handlers will be called on different threads). Personally I believe that calling callback on different thread is more preferable (AFAIK in some cases UIKit does it as well). https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). On 2015/10/12 23:21:49, David Benjamin wrote: > On 2015/10/08 16:53:32, eugenebut wrote: > > On 2015/10/07 20:16:29, David Benjamin wrote: > > > This is very possible as their implementations have nothing to do with each > > > other. You won't need UMA to determine if it happens. > > > > > > It's probably even possible that two SecTrustEvaluate calls give different > > > answers. This is a very non-deterministic process and you can't make any > > > assumptions on behavior. > > We know that mismatch will happen, but we (including felt@) want to know how > > often it happens. Do you think that knowing how often mismatch happens is > > unnecessary? > > > > > > Make sure your code is okay with this. E.g. you may get > didFailProvisionalLoad > > > and have to show an interstitial on a good certificate. > > UX will be bad, but app can handle that gracefully. > > > > > I would probably explicitly filter out the ones NSS liked because the rest > of > > > your system, based on the design doc, isn't going to accept those certs. > > Perhaps > > > turn them into some generic error. > > What do you mean by filter out? > > I meant you might want to turn those into a different SecurityStyle or something > right now. But I suppose you haven't lost information, so it could be done > later. Added a comment about documenting the weirdness in the public API > instead. I see your point. Introducing a new security style will require additional discussions with felt@, so if you ok with current approach I would prefer to leave the code as is, and revisit this topic later. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || On 2015/10/12 23:21:50, David Benjamin wrote: > On 2015/10/08 16:53:32, eugenebut wrote: > > On 2015/10/07 20:16:30, David Benjamin wrote: > > > If events cause updateSSLStatusForCurrentNavigationItem to be called > multiple > > > times in a row, I'm not sure this check quite works since SSLStatus only > > updates > > > when the asynchronous operation completes. Perhaps set cert_id synchronously > > and > > > then add some kind of pending state? > > > > > > (Alternatively, see top-level comment.) > > If |updateSSLStatusForCurrentNavigationItem| is called for the second time, > then > > both certID and URL will match and cert revalidation will not be started. > > Only if the asynchronous update process has completed already. If this function > is called twice in a row, the update process probably won't have completed yet, > and you'll trigger more and more of these. How come? This method (updateSSLStatusForCurrentNavigationItem) is actually called twice in a raw. However updateCurrentNavigationItemSSLStatusUsingCertChain:host is called only once. 1st Call of updateSSLStatusForCurrentNavigationItem: Changes SSLStatus.cert_id from 0 to 1, which leads to updateCurrentNavigationItemSSLStatusUsingCertChain:host: invocation. 2nd Call of updateSSLStatusForCurrentNavigationItem: Does not change SSLStatus.cert_id (because web::CertStore::GetInstance()->StoreCert returns the same id for same cert); certID and URLs are the same, hence |updateCurrentNavigationItemSSLStatusUsingCertChain:host:| is not called. Am I missing something? https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:67: // |serverTrust| and |host|. |host| should be in ASCII compatible form. On 2015/10/12 23:21:50, David Benjamin wrote: > I would add something like: > > // Note: The web::SecurityStyle determines whether the certificate is > // trusted. It is possible for an untrusted certificate to return a > // net::CertStatus with no errors if the cause could not be determined. > // Callers must handle this case gracefully. Done. https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:90: completionHandler:(void (^)(SecTrustResultType, BOOL success))handler; On 2015/10/12 23:21:50, David Benjamin wrote: > If you want to keep this double-threaded thing, perhaps success -> dispatched? > success could mean lots of things (SecTrustEvaluate didn't fail, this cert is > great, etc.) Done. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:262: // Updates |style| and |certStatus| for Navigation Item wich has given On 2015/10/12 23:21:50, David Benjamin wrote: > Perhaps: > > // Updates |security_style| and |cert_status| for the NavigationItem with ID > |navigationItemID|, if URL and certificate chain still match |host| and > |certChain|. Done. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:893: certChain:(NSArray*)chain On 2015/10/12 23:21:50, David Benjamin wrote: > Here you use chain but in the prototype, it's certChain. Changed predeclaration. Longer argument name makes formatting worse in this particular case. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:908: // checking that cert and URL matche is necessary. On 2015/10/12 23:21:50, David Benjamin wrote: > matche -> match Done. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:968: web::SSLStatus& SSLStatus = item->GetSSL(); On 2015/10/12 23:21:50, David Benjamin wrote: > I don't think the C++ style guide allows non-const references. (I assume there > isn't anything in Obj-C that overrides that, since this is purely a C++ thing.) C++ Style Guide does not allow non-const Reference Arguments: https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... But I did not find anything that forbids using non-const references for local variables. Objective-C itself does not support references, so C++ rules apply here. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { On 2015/10/12 23:21:50, David Benjamin wrote: > Does this check even do anything? item->GetURL() isn't uploaded in flow with > SSLStatus. I don't think you want this check but to compare > item->GetURL().host() (or _documentURL.host()? I couldn't tell when the former > is updated, so I don't know what the difference between the two is) against *the > hostname that security_style was computed against*. > > Otherwise if my current and previous states happen to share a leaf certificate, > then the cert_id will match and security_style won't be changed. > > Probably SSLStatus or NavigationItem needs to grow a new field, say a > std::string cert_host which gets updated whenever cert_id and security_status > are updated. I check host, so we can recalculate SSL status after redirects. Updated comments. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { > Right, a related problem here is: is it possible for _documentURL.host() to > change without something else signaling updateSSLStatusForCurrentNavigationItem, > like the chain switching? This is probably worth checking on. _documentURL.host() can not change w/o calling updateSSLStatusForCurrentNavigationItem. _documentURL.host() can be changes only inside webView:didCommitNavigation:, which also calls updateSSLStatusForCurrentNavigationItem. > Have two hosts with the same chain. Let both through. Then navigate from one to > the other. Do you still signal this function? This code is currently relying on > this. I've tried this: 1.) Load youtube.com 2.) Load google.com 3.) Go back scheduleSSLStatusUpdateUsingCertChain:host was not called after going back, which is intended behavior. > (Unfortunately, if you can't rely on this, then it's ugly for other reasons. You > depend on a both _documentURL.host() and on [_wkWebView certificateChain]. The > pair *should* be updated atomically. But if iOS doesn't update them atomically, > you could end up scheduling a bogus update temporarily. Which maybe that's fine? > Just wasted effort. I could imagine waiting a very small timeout for things to > settle before actually cashing in on the pending security_style and firing the > verification.) updateSSLStatusForCurrentNavigationItem is not called until app started receiving data from the server (webView:didCommitNavigation: callback). Because of that both URL and certificateChain are already synchronized, so there is no reason for waiting. Apologies if I did not address all you concerns by this comment. :) https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:992: [self scheduleSSLStatusUpdateUsingCertChain:chain host:host]; On 2015/10/12 23:21:50, David Benjamin wrote: > Because this is asynchronous (and unless we decide we're okay showing green when > you load from disk cache on a resource cached on a click-through, we can't avoid > that), you may show contradicting things in the URL bar. Likewise, there's > troubles with all this running multiple times if the asynchronous operation > hasn't completed yet. > > I would suggest that scheduleSSLStatusUpdateUsingCertChain actually set > SSLStatus.cert_id (and SSLStatus.cert_host---see comment above), clear > cert_status, and then set security_style to a new SECURITY_STYLE_PENDING. Then > the URL bar can decide how to display that one. > > Then you separately flip SECURITY_STYLE_PENDING. I think that clearing cert_status and using web::SECURITY_STYLE_UNKNOWN will be the right thing to do here. It will require some changes in upstream code to correctly handle SECURITY_STYLE_UNKNOWN (crbug.com/542776). So Done, with another security style. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1005: [self reportHasCertForSecureConnectionUMAWithValue:false]; On 2015/10/12 23:21:50, David Benjamin wrote: > This line will also run every time updateSSLStatusForCurrentNavigationItem is > called, and you probably actually want every time... something changes. > > Perhaps keyed on a similar check to line 989, comparing against > previousSSLStatus? Done.
Hopefully the last iteration. :-) https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). On 2015/10/13 20:40:33, eugenebut wrote: > On 2015/10/12 23:21:49, David Benjamin wrote: > > On 2015/10/08 16:53:32, eugenebut wrote: > > > On 2015/10/07 20:16:29, David Benjamin wrote: > > > > This is very possible as their implementations have nothing to do with > each > > > > other. You won't need UMA to determine if it happens. > > > > > > > > It's probably even possible that two SecTrustEvaluate calls give different > > > > answers. This is a very non-deterministic process and you can't make any > > > > assumptions on behavior. > > > We know that mismatch will happen, but we (including felt@) want to know how > > > often it happens. Do you think that knowing how often mismatch happens is > > > unnecessary? > > > > > > > > > > Make sure your code is okay with this. E.g. you may get > > didFailProvisionalLoad > > > > and have to show an interstitial on a good certificate. > > > UX will be bad, but app can handle that gracefully. > > > > > > > I would probably explicitly filter out the ones NSS liked because the rest > > of > > > > your system, based on the design doc, isn't going to accept those certs. > > > Perhaps > > > > turn them into some generic error. > > > What do you mean by filter out? > > > > I meant you might want to turn those into a different SecurityStyle or > something > > right now. But I suppose you haven't lost information, so it could be done > > later. Added a comment about documenting the weirdness in the public API > > instead. > I see your point. Introducing a new security style will require additional > discussions with felt@, so if you ok with current approach I would prefer to > leave the code as is, and revisit this topic later. We mentioned this in the meeting, but UNKNOWN vs a new SecurityStyle both seem fine to me as far as the NavigationItem not being in a confused state (half one page, half another). Whether you want a new one is I think largely a question for whether the UI needs to differentiate the two. https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:967: if (oldCertID != SSLStatus.cert_id || On 2015/10/13 20:40:33, eugenebut wrote: > On 2015/10/12 23:21:50, David Benjamin wrote: > > On 2015/10/08 16:53:32, eugenebut wrote: > > > On 2015/10/07 20:16:30, David Benjamin wrote: > > > > If events cause updateSSLStatusForCurrentNavigationItem to be called > > multiple > > > > times in a row, I'm not sure this check quite works since SSLStatus only > > > updates > > > > when the asynchronous operation completes. Perhaps set cert_id > synchronously > > > and > > > > then add some kind of pending state? > > > > > > > > (Alternatively, see top-level comment.) > > > If |updateSSLStatusForCurrentNavigationItem| is called for the second time, > > then > > > both certID and URL will match and cert revalidation will not be started. > > > > Only if the asynchronous update process has completed already. If this > function > > is called twice in a row, the update process probably won't have completed > yet, > > and you'll trigger more and more of these. > How come? This method (updateSSLStatusForCurrentNavigationItem) is actually > called twice in a raw. > However updateCurrentNavigationItemSSLStatusUsingCertChain:host is called only > once. > > 1st Call of updateSSLStatusForCurrentNavigationItem: > Changes SSLStatus.cert_id from 0 to 1, which leads to > updateCurrentNavigationItemSSLStatusUsingCertChain:host: invocation. > > 2nd Call of updateSSLStatusForCurrentNavigationItem: > Does not change SSLStatus.cert_id (because > web::CertStore::GetInstance()->StoreCert returns the same id for same cert); > certID and URLs are the same, hence > |updateCurrentNavigationItemSSLStatusUsingCertChain:host:| is not called. > > Am I missing something? > Ah, no, you're right. I'd missed that line 965 actually updated things. See also non-const references being very confusing. :-) So the pending state is about ensuring the SSLStatus.security_status and SSLStatus.cert_id always match. If you hadn't updated cert_id, then they would always match without the pending state, but you'd trigger updates a lot. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:968: web::SSLStatus& SSLStatus = item->GetSSL(); On 2015/10/13 20:40:33, eugenebut wrote: > On 2015/10/12 23:21:50, David Benjamin wrote: > > I don't think the C++ style guide allows non-const references. (I assume there > > isn't anything in Obj-C that overrides that, since this is purely a C++ > thing.) > C++ Style Guide does not allow non-const Reference Arguments: > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > But I did not find anything that forbids using non-const references for local > variables. Hrm, me neither. Interesting. I don't think I've ever seen one in Chromium. I think the reasons would still apply either way; it's very unclear from reading the code that you're actually updating something other than a local. > Objective-C itself does not support references, so C++ rules apply here. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { On 2015/10/13 20:40:33, eugenebut wrote: > > Right, a related problem here is: is it possible for _documentURL.host() to > > change without something else signaling > updateSSLStatusForCurrentNavigationItem, > > like the chain switching? This is probably worth checking on. > _documentURL.host() can not change w/o calling > updateSSLStatusForCurrentNavigationItem. > _documentURL.host() can be changes only inside webView:didCommitNavigation:, > which also calls updateSSLStatusForCurrentNavigationItem. > > > Have two hosts with the same chain. Let both through. Then navigate from one > to > > the other. Do you still signal this function? This code is currently relying > on > > this. > I've tried this: > 1.) Load http://youtube.com > 2.) Load http://google.com > 3.) Go back > > scheduleSSLStatusUpdateUsingCertChain:host was not called after going back, > which is intended behavior. > > > (Unfortunately, if you can't rely on this, then it's ugly for other reasons. > You > > depend on a both _documentURL.host() and on [_wkWebView certificateChain]. The > > pair *should* be updated atomically. But if iOS doesn't update them > atomically, > > you could end up scheduling a bogus update temporarily. Which maybe that's > fine? > > Just wasted effort. I could imagine waiting a very small timeout for things to > > settle before actually cashing in on the pending security_style and firing the > > verification.) > updateSSLStatusForCurrentNavigationItem is not called until app started > receiving data from the server > (webView:didCommitNavigation: callback). Because of that both URL and > certificateChain are already synchronized, so there is no reason for waiting. > > Apologies if I did not address all you concerns by this comment. :) If they're already synchronized, how can this check ever do anything? In webView:didCommitNavigation:, you first update _documentURL and then call updateSSLStatusForCurrentNavigationItem. I can't tell where the current NavigationItem's GetURL() is set, but I'm assuming it's also already updated? That means that the current navigation item and _documentURL should both already match the WKWebView's state, right? Which means this check is *always true*. What you actually want to check is the following: The last time you computed security_status for this NavigationItem, it was based on two inputs, the then host and certificate/cert_id. We assume that, if those inputs haven't changed, then the old security_status is also valid. If, somehow, those values have changed, THEN we need to recompute it. The cert_id check is correct because, at this point, we're still on the old cert_id so we're comparing against that. But you're comparing new URL vs new URL, not new URL vs old URL, no? I'm not sure how NavigationItem's are managed, so I don't know when this might or might not be a problem. Perhaps if I location.replace to a different URL that has the same certificate but a different hostname? Or does that create a new NavigationItem? Back/forward, in //content, will hit the network if the cache doesn't have it. So that would be another case where things may change on you (redirect, for instance). https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: // cert_id, which works as a guard to prevent re-entry into this method. I don't understand what this comment is saying. By the time this code runs, cert_id has already been set in line 982. I think the invariant (SSLStatus *always* refers to the current state of the world) would be much clearer if lines 927 and 928 were after line 987 or so. That way you don't update half of SSLStatus one time and half another time. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1004: [self reportHasCertForSecureConnectionUMAWithValue:false]; If all that changed was hasSecureContent, this will still trigger, no? I think you need to decide what are the points you want to sample and put it there. A priori, I would think didCommitNavigation is the right place. Then you sample exactly then. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1037: _documentURL = newURL; This should probably also have a DCHECK that _documentURL.host() doesn't change, right? That's why you don't have to update SSL state here. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1212: if (self.loadPhase == web::LOAD_REQUESTED) { Existing issue (probably ought to be separate), but I don't believe this line is quite correct. It's here I assume because you get a flurry of events on commit. But while a navigation is in flight but not committed, the previous document is still completely alive. The navigation may not even commit. Which means that, as things stand right now, if hasSecureContent (can happen) or certificateChain change (can't happen) while a new navigation (which may not complete!) is in flight, you'll miss indicator updates. This sort of thing can be abused by, say, constantly trying to navigate to a URL that hangs for 10 seconds and then returns HTTP 204. Then the page will shield itself from Chrome noticing properties have changed.
https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1212: if (self.loadPhase == web::LOAD_REQUESTED) { On 2015/10/15 17:46:12, David Benjamin wrote: > Existing issue (probably ought to be separate), but I don't believe this line is > quite correct. It's here I assume because you get a flurry of events on commit. > But while a navigation is in flight but not committed, the previous document is > still completely alive. The navigation may not even commit. IIRC it's because we get told about changes related to the pending entry. WKWebView is unfortunately very happy to report on everything as soon as it is pending (see for instance the URL determination code in this class).
Thanks! Hopefully all done :) https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1322193003/diff/380001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:187: // mismatch (crbug.com/535699). On 2015/10/15 17:46:12, David Benjamin wrote: > On 2015/10/13 20:40:33, eugenebut wrote: > > On 2015/10/12 23:21:49, David Benjamin wrote: > > > On 2015/10/08 16:53:32, eugenebut wrote: > > > > On 2015/10/07 20:16:29, David Benjamin wrote: > > > > > This is very possible as their implementations have nothing to do with > > each > > > > > other. You won't need UMA to determine if it happens. > > > > > > > > > > It's probably even possible that two SecTrustEvaluate calls give > different > > > > > answers. This is a very non-deterministic process and you can't make any > > > > > assumptions on behavior. > > > > We know that mismatch will happen, but we (including felt@) want to know > how > > > > often it happens. Do you think that knowing how often mismatch happens is > > > > unnecessary? > > > > > > > > > > > > > > Make sure your code is okay with this. E.g. you may get > > > didFailProvisionalLoad > > > > > and have to show an interstitial on a good certificate. > > > > UX will be bad, but app can handle that gracefully. > > > > > > > > > I would probably explicitly filter out the ones NSS liked because the > rest > > > of > > > > > your system, based on the design doc, isn't going to accept those certs. > > > > Perhaps > > > > > turn them into some generic error. > > > > What do you mean by filter out? > > > > > > I meant you might want to turn those into a different SecurityStyle or > > something > > > right now. But I suppose you haven't lost information, so it could be done > > > later. Added a comment about documenting the weirdness in the public API > > > instead. > > I see your point. Introducing a new security style will require additional > > discussions with felt@, so if you ok with current approach I would prefer to > > leave the code as is, and revisit this topic later. > > We mentioned this in the meeting, but UNKNOWN vs a new SecurityStyle both seem > fine to me as far as the NavigationItem not being in a confused state (half one > page, half another). Whether you want a new one is I think largely a question > for whether the UI needs to differentiate the two. Thank you for confirmation. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:968: web::SSLStatus& SSLStatus = item->GetSSL(); On 2015/10/15 17:46:12, David Benjamin wrote: > On 2015/10/13 20:40:33, eugenebut wrote: > > On 2015/10/12 23:21:50, David Benjamin wrote: > > > I don't think the C++ style guide allows non-const references. (I assume > there > > > isn't anything in Obj-C that overrides that, since this is purely a C++ > > thing.) > > C++ Style Guide does not allow non-const Reference Arguments: > > > https://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Reference_Ar... > > > > But I did not find anything that forbids using non-const references for local > > variables. > > Hrm, me neither. Interesting. I don't think I've ever seen one in Chromium. I > think the reasons would still apply either way; it's very unclear from reading > the code that you're actually updating something other than a local. > > > Objective-C itself does not support references, so C++ rules apply here. I realize that non-const ref led to come confusion, so I removed this variable. https://codereview.chromium.org/1322193003/diff/440001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:990: item->GetURL().host() != _documentURL.host()) { On 2015/10/15 17:46:12, David Benjamin wrote: > On 2015/10/13 20:40:33, eugenebut wrote: > > > Right, a related problem here is: is it possible for _documentURL.host() to > > > change without something else signaling > > updateSSLStatusForCurrentNavigationItem, > > > like the chain switching? This is probably worth checking on. > > _documentURL.host() can not change w/o calling > > updateSSLStatusForCurrentNavigationItem. > > _documentURL.host() can be changes only inside webView:didCommitNavigation:, > > which also calls updateSSLStatusForCurrentNavigationItem. > > > > > Have two hosts with the same chain. Let both through. Then navigate from one > > to > > > the other. Do you still signal this function? This code is currently relying > > on > > > this. > > I've tried this: > > 1.) Load http://youtube.com > > 2.) Load http://google.com > > 3.) Go back > > > > scheduleSSLStatusUpdateUsingCertChain:host was not called after going back, > > which is intended behavior. > > > > > (Unfortunately, if you can't rely on this, then it's ugly for other reasons. > > You > > > depend on a both _documentURL.host() and on [_wkWebView certificateChain]. > The > > > pair *should* be updated atomically. But if iOS doesn't update them > > atomically, > > > you could end up scheduling a bogus update temporarily. Which maybe that's > > fine? > > > Just wasted effort. I could imagine waiting a very small timeout for things > to > > > settle before actually cashing in on the pending security_style and firing > the > > > verification.) > > updateSSLStatusForCurrentNavigationItem is not called until app started > > receiving data from the server > > (webView:didCommitNavigation: callback). Because of that both URL and > > certificateChain are already synchronized, so there is no reason for waiting. > > > > Apologies if I did not address all you concerns by this comment. :) > > If they're already synchronized, how can this check ever do anything? > > In webView:didCommitNavigation:, you first update _documentURL and then call > updateSSLStatusForCurrentNavigationItem. I can't tell where the current > NavigationItem's GetURL() is set, but I'm assuming it's also already updated? Yes, the update flow is: 1.) webView:didCommitNavigation: 2.) webPageChanged 3.) didStartLoadingURL:updateHistory: 4.) commitPendingEntry > That means that the current navigation item and _documentURL should both already > match the WKWebView's state, right? Which means this check is *always true*. > > What you actually want to check is the following: > > The last time you computed security_status for this NavigationItem, it was based > on two inputs, the then host and certificate/cert_id. We assume that, if those > inputs haven't changed, then the old security_status is also valid. > If, somehow, those values have changed, THEN we need to recompute it. The > cert_id check is correct because, at this point, we're still on the old cert_id > so we're comparing against that. But you're comparing new URL vs new URL, not > new URL vs old URL, no? Good catch. Fixed by adding |host| member to web::SSLStatus (Stuart is ok with that). > > I'm not sure how NavigationItem's are managed, so I don't know when this might > or might not be a problem. Perhaps if I location.replace to a different URL that > has the same certificate but a different hostname? Or does that create a new > NavigationItem? location.replace API does not allow changing the origin. But redirect can. > Back/forward, in //content, will hit the network if the cache doesn't have it. > So that would be another case where things may change on you (redirect, for > instance). https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: // cert_id, which works as a guard to prevent re-entry into this method. On 2015/10/15 17:46:12, David Benjamin wrote: > I don't understand what this comment is saying. By the time this code runs, > cert_id has already been set in line 982. > > I think the invariant (SSLStatus *always* refers to the current state of the > world) would be much clearer if lines 927 and 928 were after line 987 or so. > That way you don't update half of SSLStatus one time and half another time. Moved the code. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1004: [self reportHasCertForSecureConnectionUMAWithValue:false]; On 2015/10/15 17:46:12, David Benjamin wrote: > If all that changed was hasSecureContent, this will still trigger, no? > > I think you need to decide what are the points you want to sample and put it > there. A priori, I would think didCommitNavigation is the right place. Then you > sample exactly then. That's a good suggestion. Done. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1037: _documentURL = newURL; On 2015/10/15 17:46:12, David Benjamin wrote: > This should probably also have a DCHECK that _documentURL.host() doesn't change, > right? That's why you don't have to update SSL state here. Yes it should. Done. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1212: if (self.loadPhase == web::LOAD_REQUESTED) { On 2015/10/15 17:46:12, David Benjamin wrote: > Existing issue (probably ought to be separate), but I don't believe this line is > quite correct. It's here I assume because you get a flurry of events on commit. > But while a navigation is in flight but not committed, the previous document is > still completely alive. The navigation may not even commit. > > Which means that, as things stand right now, if hasSecureContent (can happen) or > certificateChain change (can't happen) while a new navigation (which may not > complete!) is in flight, you'll miss indicator updates. This sort of thing can > be abused by, say, constantly trying to navigate to a URL that hangs for 10 > seconds and then returns HTTP 204. Then the page will shield itself from Chrome > noticing properties have changed. WKWebView API is weird. certificateChain, URL, or hasOnlySecureContent reflect either state of the current page or pending page. F.e. Consider navigation from http://www.amazon.com to https://www.google.com 1.) Navigation started (www.amazon.com content is still displayed in web view) 2.) TLS handshake with google.com succeeded (www.amazon.com is still displayed) 3.) URL changed to google.com (www.amazon.com is still displayed) 4.) certificateChain is now updated to google chain (www.amazon.com is still displayed) 5.) hasOnlySecureContent is now changed to YES (www.amazon.com is still displayed) 6.) www.amazon.com content is still displayed in web view 7.) webView:didCommitNavigation: is called 8.) Only now it is safe to update URL and SSL status, because current page is not amazon anymore So any changes during the pending load more-likely belong to pending load not to current page :(
lgtm! Thanks for your patience and apologies for my usual latency problems. https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/480001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1212: if (self.loadPhase == web::LOAD_REQUESTED) { On 2015/10/15 22:59:34, eugenebut wrote: > On 2015/10/15 17:46:12, David Benjamin wrote: > > Existing issue (probably ought to be separate), but I don't believe this line > is > > quite correct. It's here I assume because you get a flurry of events on > commit. > > But while a navigation is in flight but not committed, the previous document > is > > still completely alive. The navigation may not even commit. > > > > Which means that, as things stand right now, if hasSecureContent (can happen) > or > > certificateChain change (can't happen) while a new navigation (which may not > > complete!) is in flight, you'll miss indicator updates. This sort of thing can > > be abused by, say, constantly trying to navigate to a URL that hangs for 10 > > seconds and then returns HTTP 204. Then the page will shield itself from > Chrome > > noticing properties have changed. > WKWebView API is weird. certificateChain, URL, or hasOnlySecureContent reflect > either state of the current page or pending page. > > F.e. Consider navigation from http://www.amazon.com to https://www.google.com > > 1.) Navigation started (http://www.amazon.com content is still displayed in web view) > 2.) TLS handshake with http://google.com succeeded (http://www.amazon.com is still displayed) > 3.) URL changed to http://google.com (http://www.amazon.com is still displayed) > 4.) certificateChain is now updated to google chain (http://www.amazon.com is still > displayed) > 5.) hasOnlySecureContent is now changed to YES (http://www.amazon.com is still > displayed) > 6.) http://www.amazon.com content is still displayed in web view > 7.) webView:didCommitNavigation: is called > 8.) Only now it is safe to update URL and SSL status, because current page is > not amazon anymore > > So any changes during the pending load more-likely belong to pending load not to > current page :( Gotcha. Sigh. :-) I guess the situation is what it is. May be worth investigating how to get a better signal eventually, but whatever. This is quite tangential. (I was only looking into it because of some other comment elsewhere.) https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:38: // |cert_status_host| is not used for comparisson intentionaly. comparisson -> comparison https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:51: // (f.e. after redirect). f.e. -> e.g. (I'm assuming f.e. is "for example"?) https://codereview.chromium.org/1322193003/diff/500001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/500001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:983: // they will be asynchronously updated in they -> They
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/patch-status/1322193003/520001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/520001
David, thanks a lot for review! Stuart, do you want to make a final pass before submission? https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:38: // |cert_status_host| is not used for comparisson intentionaly. On 2015/10/16 00:15:20, David Benjamin wrote: > comparisson -> comparison Done. https://codereview.chromium.org/1322193003/diff/500001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:51: // (f.e. after redirect). On 2015/10/16 00:15:20, David Benjamin wrote: > f.e. -> e.g. > > (I'm assuming f.e. is "for example"?) Done. https://codereview.chromium.org/1322193003/diff/500001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1322193003/diff/500001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:983: // they will be asynchronously updated in On 2015/10/16 00:15:20, David Benjamin wrote: > they -> They Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still lgtm https://codereview.chromium.org/1322193003/diff/520001/ios/web/public/ssl_sta... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/1322193003/diff/520001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:38: // |cert_status_host| is not used for comparison intentionaly. intentionally
https://codereview.chromium.org/1322193003/diff/520001/ios/web/public/ssl_sta... File ios/web/public/ssl_status.h (right): https://codereview.chromium.org/1322193003/diff/520001/ios/web/public/ssl_sta... ios/web/public/ssl_status.h:38: // |cert_status_host| is not used for comparison intentionaly. On 2015/10/16 19:44:02, stuartmorgan wrote: > intentionally Done.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from jww@chromium.org, davidben@chromium.org, stuartmorgan@chromium.org Link to the patchset: https://codereview.chromium.org/1322193003/#ps540001 (title: "Updated comment")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1322193003/540001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1322193003/540001
Message was sent while issue was closed.
Committed patchset #28 (id:540001)
Message was sent while issue was closed.
Patchset 28 (id:??) landed as https://crrev.com/8ac2913b9c37a0a310ed152dc6be9f60e72d1f2b Cr-Commit-Position: refs/heads/master@{#354575} |
