Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1357773002:
WKWebView: Implemented recoverable SSL interstitials. (Closed)
|
Created:
5 years, 3 months ago by Eugene But (OOO till 7-30) Modified:
5 years, 1 month ago CC:
chromium-reviews, cbentzel+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@lock_coloring Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionWKWebView: Implemented recoverable SSL interstitials.
Load decision is made inside didReceiveAuthenticationChallenge: and
for recoverable errors cached user decision is read from CertPolicyCache.
That method is called for good and bad certs as long as request is not
served from caches. This method allows accepting bad certs.
SSL Interstitial is presented inside didFailProvisionalNavigation: which
is called when top level frame has bad ssl cert and default behavior
was not overridden inside didReceiveAuthenticationChallenge.
CertStatus is evaluated inside didReceiveAuthenticationChallenge and
its status is cached so it can be reused inside
didFailProvisionalNavigation. It is not possible to re-evaluate cert
inside didFailProvisionalNavigation because that callback does not
provide complete cert chain.
BUG=462427
Committed: https://crrev.com/4e0758a248f23e4cc60465431a013a532ea96c89
Cr-Commit-Position: refs/heads/master@{#357018}
Patch Set 1 #Patch Set 2 : Merged with parent CL #
Total comments: 8
Patch Set 3 : Resolved Stuart's review comments #
Total comments: 32
Patch Set 4 : Addressed review comments #Patch Set 5 : Resolved review comments #Patch Set 6 : Purge cert cache where necessary #Patch Set 7 : Merged with origin/master #Patch Set 8 : Merged with origin/master #
Total comments: 12
Patch Set 9 : Addressed review comments #Patch Set 10 : Self review #
Total comments: 2
Patch Set 11 : Addressed Stuart's review comments #Patch Set 12 : Merged with parent CL #Patch Set 13 : Merged with parent branch #
Total comments: 6
Patch Set 14 : Addressed Joel's review comments #
Total comments: 32
Patch Set 15 : Addressed review comments #
Total comments: 36
Patch Set 16 : Resolved Ryan's review comments #
Total comments: 2
Patch Set 17 : Addressed unit tests review comments #Messages
Total messages: 38 (4 generated)
eugenebut@chromium.org changed reviewers: + felt@chromium.org, jww@chromium.org, rsleevi@chromium.org, stuartmorgan@chromium.org
On 2015/09/19 00:34:06, eugenebut wrote: Quick detail regarding "does not provide complete cert chain." Can you put together a document showing the full contents you get back from each callback (e.g. each certificate). You can use the SecCertificateRef + X509Certificate::GetDEREncoded() to get the raw binary, and then just base64 encode it or something. I realize you sent an abbreviated form on email, and I wasn't sure if you updated the design doc, but that'd make it clearer and I could see if I could offer any concrete suggestions if there are things we could do. Thanks!
https://codereview.chromium.org/1357773002/diff/20001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/20001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:17: template <typename ValueType> Why templated? The actual code only stores one value. Do we expect to use this for something else in the future? https://codereview.chromium.org/1357773002/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/1357773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: [_certVerificationController Are we okay with a strong reference to self here? https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1539: completionHandler:handler]; Same question here; what's the ownership/lifetime guarantee here? Handing a self-owning block to an ivar can be bad news. https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/wk_we... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/wk_we... ios/web/web_state/wk_web_view_security_util.mm:14: // This key was determined by inspecting userInfo dict of an SSL error. These keys were
Thanks, Stuart! Ryan, I've shared sample chains with you. PTAL. https://codereview.chromium.org/1357773002/diff/20001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/20001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:17: template <typename ValueType> On 2015/09/23 17:36:09, stuartmorgan wrote: > Why templated? The actual code only stores one value. Do we expect to use this > for something else in the future? Because ValueType (CertStatus, is_recoverable flag pair) is very specific to WebController and I don't want to make this class depend on workaround for incomplete chains. WDYT? https://codereview.chromium.org/1357773002/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/1357773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: [_certVerificationController On 2015/09/23 17:36:09, stuartmorgan wrote: > Are we okay with a strong reference to self here? Yes, displayed interstitial outlives web controller. Also the same approach is used for UIWebView: https://code.google.com/p/chromium/codesearch#chromium/src/ios/web/web_state/... https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/ui/cr... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1539: completionHandler:handler]; On 2015/09/23 17:36:09, stuartmorgan wrote: > Same question here; what's the ownership/lifetime guarantee here? Handing a > self-owning block to an ivar can be bad news. Good catch. CRWCertVerificationController has -close method, which cancels CertVerifier requests, however -close does not cancel SecTrustEvaluate requests, which retain this block. This needs weak-strong dance. https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/wk_we... File ios/web/web_state/wk_web_view_security_util.mm (right): https://codereview.chromium.org/1357773002/diff/20001/ios/web/web_state/wk_we... ios/web/web_state/wk_web_view_security_util.mm:14: // This key was determined by inspecting userInfo dict of an SSL error. On 2015/09/23 17:36:09, stuartmorgan wrote: > These keys were Done.
Going to hold off on this further until the dependent CL is worked through. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:16: // a template param. This second sentence is unclear. What is Key? Why is it being discussed? Why is Value templated? What's the right way to use it? Is the cache size bounded or infinite? https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:25: bool get(const scoped_refptr<net::X509Certificate>& cert, http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names STYLE: This should be Get() https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; Why is the key type multi-value? This allows a single hostname to store multiple entries by varying the cert. Why not simply keyed off hostname, and then before using a cached value, making sure the certificate is still the same? For that matter, why invent a new object? Why are the (many) existing cache objects not suitable? https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:48: // Holds cert-host pair and provides less-than comparator. What does this comment add? https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:54: bool operator<(const KeyType& other) const { Why is this inlined? It can be out-of-lined via a LessThan comparator https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:55: if (host == other.host) DANGEROUS DESIGN PATTERN: When writing comparators, prefer to do early returns. This pattern you've chosen makes it difficult/complex to add any additional members, due to the nesting. if (host != other.host) return host < other.host; return cert_comparator(cert, other.cert); https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache_unittest.cc:35: TEST_F(CertVerificationCacheTest, Basic) { Why is this one big test? You're testing multiple behaviours - they should each be individual tests. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:31: // user has decided to proceed with this invalid cert. // Cert is not valid. However, caller should proceed with the load, because ... (extra ,) https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate same remarks re: DNS form https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate spelling: intermediate https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate It's unclear why "|leafCert| must not contain any intermediate certs" - seems like an unnecessary API constraint?
Just replying to the comments. I agree that it makes sense to hold on review until issues are resolved in depended CL. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:16: // a template param. On 2015/09/24 22:48:38, Ryan Sleevi wrote: > This second sentence is unclear. What is Key? Why is it being discussed? Why is > Value templated? What's the right way to use it? Is the cache size bounded or > infinite? Value is templated because it is very specific to web controller needs. Do you think it should not be templated? Not sure if I understand your concerns regarding |Key|, but I've updated class comments, hopefully they more clear now. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:25: bool get(const scoped_refptr<net::X509Certificate>& cert, On 2015/09/24 22:48:39, Ryan Sleevi wrote: > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names > > STYLE: This should be Get() Done. Capitalized Set as well for consistency. Style guide permits both set() and Set() names for small methods. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/24 22:48:39, Ryan Sleevi wrote: > Why is the key type multi-value? This allows a single hostname to store multiple > entries by varying the cert. > > Why not simply keyed off hostname, and then before using a cached value, making > sure the certificate is still the same? This cache object will be used for transferring net::CertStatus between didReceiveAuthenticationChallenge: and didFailProvisionalNavigation: callbacks. didReceiveAuthenticationChallenge: will calculate CertStatus in order to make load/no-load decision. didFailProvisionalNavigation: will be called when top level frame load has failed and this callback will be used for presenting SSL interstitial. didReceiveAuthenticationChallenge: is called for all requests, including sub-requests, which means that in case of server misconfiguration this cache may hold multiple certs for the same hostname. This is the reason why hostname alone can't be used as a key. > For that matter, why invent a new object? Why are the (many) existing cache > objects not suitable? I would appreciate if you can point me to something that allows mapping from string-cert pair to arbitrary value. Unfortunately I could not find anything suitable. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:48: // Holds cert-host pair and provides less-than comparator. On 2015/09/24 22:48:39, Ryan Sleevi wrote: > What does this comment add? Removed. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:54: bool operator<(const KeyType& other) const { On 2015/09/24 22:48:38, Ryan Sleevi wrote: > Why is this inlined? It can be out-of-lined via a LessThan comparator This operator is inlined, because it is 3 LOC. Is there an advantage of using a LessThan comparator and making it non inlined? https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:55: if (host == other.host) On 2015/09/24 22:48:39, Ryan Sleevi wrote: > DANGEROUS DESIGN PATTERN: > > When writing comparators, prefer to do early returns. This pattern you've chosen > makes it difficult/complex to add any additional members, due to the nesting. > > if (host != other.host) > return host < other.host; > return cert_comparator(cert, other.cert); Done. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache_unittest.cc:35: TEST_F(CertVerificationCacheTest, Basic) { On 2015/09/24 22:48:39, Ryan Sleevi wrote: > Why is this one big test? > > You're testing multiple behaviours - they should each be individual tests. Split into 3 tests. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:31: // user has decided to proceed with this invalid cert. On 2015/09/24 22:48:39, Ryan Sleevi wrote: > // Cert is not valid. However, caller should proceed with the load, because ... > > (extra ,) Done. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate On 2015/09/24 22:48:39, Ryan Sleevi wrote: > spelling: intermediate Done. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate On 2015/09/24 22:48:39, Ryan Sleevi wrote: > same remarks re: DNS form Done. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate On 2015/09/24 22:48:39, Ryan Sleevi wrote: > It's unclear why "|leafCert| must not contain any intermediate certs" - seems > like an unnecessary API constraint? didFailProvisionalNavigation: callback, where decision is stored may not provide complete/valid cert chain. So per you suggestion CertPolicy::Judgment decision will be stored and read for leaf cert and hostname. This very API constraint is necessary. Otherwise reading CertPolicy::Judgment decision will not work: https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... Does it make sense?
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:25: bool get(const scoped_refptr<net::X509Certificate>& cert, On 2015/09/25 21:24:23, eugenebut wrote: > Done. Capitalized Set as well for consistency. Style guide permits both set() > and Set() names for small methods. The problem is that this is actually quite a large method (.find() gets inlined into a binary search across the key values, as necessitated by using map) http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions I would suggest this is one way in which Google3 and Chromium diverge significantly - in most situations, the cost of compile is passed on to all developers working on code. Every time you include this header, the full code has to be expanded and inlined. This is somewhat covered more in depth in https://www.chromium.org/developers/coding-style#TOC-Inline-functions and https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/25 21:24:23, eugenebut wrote: > This cache object will be used for transferring net::CertStatus between > didReceiveAuthenticationChallenge: and didFailProvisionalNavigation: callbacks. > didReceiveAuthenticationChallenge: will calculate CertStatus in order to make > load/no-load decision. didFailProvisionalNavigation: will be called when top > level frame load has failed and this callback will be used for presenting SSL > interstitial. > > didReceiveAuthenticationChallenge: is called for all requests, including > sub-requests, which means that in case of server misconfiguration this cache may > hold multiple certs for the same hostname. This is the reason why hostname alone > can't be used as a key. I guess I'm still rather confused here. Perhaps it would help to speak more of concrete loading cases and where you would expect something 'bad' to happen. And in case my suggestion wasn't clear, here's what I was concretely suggesting: Store one |value| per host (aka a string key). Store the cert you verified as part of |value|. When you get either of these callbacks, look up based on |host|. Is the |cert| the same? If the cert is the same, you should be able to use the verification result [If you think you can't, it may help to explain further why that's so]. If the cert is not the same, then force the client to go back through the revalidation flow and recompute the cert status, evicting the current |value| from the cache. [If you can't go through revalidation, it may help to explain what scenarios you see going on?] As I see it, there's two types of configuration changes that would invalidate value (assuming value contains cert, rather than the key) 1) The server changes certificates during communication (a server reconfiguration). It's fine to "fail closed" in this case (e.g. recompute status, using more resources, etc) 2) The client changes configuration that may lead to a new result. Since we can't detect when this happens, it's fair to say it's an API limitation. As it stands, I'm concerned that a cert that constantly rotated certificates could cause unbounded growth in the client. The same way that a single page load forcing us to load a ton of pages can also cause unbounded growth in memory. In Chrome, we're fairly aggressive in trying to prevent such situations in the browser process. For example, if a tab goes away, we make sure to flush all the cert caches associated with the renderer process. > I would appreciate if you can point me to something that allows mapping from > string-cert pair to arbitrary value. Unfortunately I could not find anything > suitable. https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/mr... (access based cache) https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_... (arbitrary conditional-based cache; named Expiring, but you get to define what "Expiring" means) https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate On 2015/09/25 21:24:23, eugenebut wrote: > On 2015/09/24 22:48:39, Ryan Sleevi wrote: > > It's unclear why "|leafCert| must not contain any intermediate certs" - seems > > like an unnecessary API constraint? > didFailProvisionalNavigation: callback, where decision is stored may not provide > complete/valid cert chain. So per you suggestion CertPolicy::Judgment decision > will be stored and read for leaf cert and hostname. > > This very API constraint is necessary. Otherwise reading CertPolicy::Judgment > decision will not work: > https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... > > Does it make sense? No. It's not clear to me why you impose this constraint on your callers, rather than 'fixing up' incoming certs to normalize the CertPolicy::Judgement (e.g. stripping off intermediate certificates yourself) That is, I find it weird to force callers to be aware of these semantics, a result of the inconsistency between the two APIs you mentioned (didFailProvisionalNavigation vs interstitials), rather than just fixing it yourself and leaving it as an implementation detail as to why. My understanding is that this method (allowCert) is tightly-coupled to the implementation of decideLoadPolicyForTrust (which I assume is what didFail is calling), but you're forcing callers to be aware of that coupling rather than handling it yourself (e.g. having both allowCert and decideLoadPolicy only consider/store the leaf). By handling it yourself, you provide a strong API contract to callers (these two methods are complementary), without making them worry about the details.
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:25: bool get(const scoped_refptr<net::X509Certificate>& cert, On 2015/09/28 22:46:52, Ryan Sleevi wrote: > On 2015/09/25 21:24:23, eugenebut wrote: > > Done. Capitalized Set as well for consistency. Style guide permits both set() > > and Set() names for small methods. > > The problem is that this is actually quite a large method (.find() gets inlined > into a binary search across the key values, as necessitated by using map) > > http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Inline_Functions > > I would suggest this is one way in which Google3 and Chromium diverge > significantly - in most situations, the cost of compile is passed on to all > developers working on code. Every time you include this header, the full code > has to be expanded and inlined. > > This is somewhat covered more in depth in > https://www.chromium.org/developers/coding-style#TOC-Inline-functions and > https://www.chromium.org/developers/coding-style/cpp-dos-and-donts#TOC-Stop-i... Thanks for detailed explanation. Un-inlined. https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/28 22:46:52, Ryan Sleevi wrote: > On 2015/09/25 21:24:23, eugenebut wrote: > > This cache object will be used for transferring net::CertStatus between > > didReceiveAuthenticationChallenge: and didFailProvisionalNavigation: > callbacks. > > didReceiveAuthenticationChallenge: will calculate CertStatus in order to make > > load/no-load decision. didFailProvisionalNavigation: will be called when top > > level frame load has failed and this callback will be used for presenting SSL > > interstitial. > > > > didReceiveAuthenticationChallenge: is called for all requests, including > > sub-requests, which means that in case of server misconfiguration this cache > may > > hold multiple certs for the same hostname. This is the reason why hostname > alone > > can't be used as a key. > > > I guess I'm still rather confused here. Perhaps it would help to speak more of > concrete loading cases and where you would expect something 'bad' to happen. > > And in case my suggestion wasn't clear, here's what I was concretely suggesting: > > Store one |value| per host (aka a string key). Store the cert you verified as > part of |value|. > > When you get either of these callbacks, look up based on |host|. Is the |cert| > the same? > > If the cert is the same, you should be able to use the verification result [If > you think you can't, it may help to explain further why that's so]. > If the cert is not the same, then force the client to go back through the > revalidation flow and recompute the cert status, evicting the current |value| > from the cache. [If you can't go through revalidation, it may help to explain > what scenarios you see going on?] > > As I see it, there's two types of configuration changes that would invalidate > value (assuming value contains cert, rather than the key) > 1) The server changes certificates during communication (a server > reconfiguration). It's fine to "fail closed" in this case (e.g. recompute > status, using more resources, etc) > 2) The client changes configuration that may lead to a new result. Since we > can't detect when this happens, it's fair to say it's an API limitation. > > As it stands, I'm concerned that a cert that constantly rotated certificates > could cause unbounded growth in the client. The same way that a single page load > forcing us to load a ton of pages can also cause unbounded growth in memory. > > In Chrome, we're fairly aggressive in trying to prevent such situations in the > browser process. For example, if a tab goes away, we make sure to flush all the > cert caches associated with the renderer process. > > > I would appreciate if you can point me to something that allows mapping from > > string-cert pair to arbitrary value. Unfortunately I could not find anything > > suitable. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/mr... > (access based cache) > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_... > (arbitrary conditional-based cache; named Expiring, but you get to define what > "Expiring" means) Consider the following use case: 1. didReceiveAuthenticationChallenge: is called for www.example.com with bad cert A. 2. didReceiveAuthenticationChallenge: is called for www.example.com with bad cert B (certs are different, hosts are the same). 3. didFailProvisionalNavigation: is called with cert A (because it was cert for top level frame request). Now in didFailProvisionalNavigation: I need to retrieve CertStatus for "www.example.com" - "cert A" pair. Your suggestion is totally clear to me, but I don't want to reevaluate cert due to the following reasons: 1. didFailProvisionalNavigation: may not provide a correct chain (not sure if it's actually a problem for SecTrust API, but it can be) 2. Reevaluation is async operation, which needs to be canceled if user navigates back before reevaluation is completed (and it will make code more complex) 3. Reevaluation of a bad cert is expensive so it impacts battery life. I understand the concert about memory usage. But _certVerificationErrors stores certs only for a short period of time (between receiving the cert and actual start/failure of the load). Because of that this cache should not grow as it will be constantly purged. I can't imagine the case when cache will have more than 5 different certs for a single hostname (Browser does not open more than 5 connections for a single host within one session, right?). https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... ios/web/net/crw_cert_verification_controller.h:73: // |host| should be in DNS form. |leafCert| must not contain any intermidiate On 2015/09/28 22:46:52, Ryan Sleevi wrote: > On 2015/09/25 21:24:23, eugenebut wrote: > > On 2015/09/24 22:48:39, Ryan Sleevi wrote: > > > It's unclear why "|leafCert| must not contain any intermediate certs" - > seems > > > like an unnecessary API constraint? > > didFailProvisionalNavigation: callback, where decision is stored may not > provide > > complete/valid cert chain. So per you suggestion CertPolicy::Judgment decision > > will be stored and read for leaf cert and hostname. > > > > This very API constraint is necessary. Otherwise reading CertPolicy::Judgment > > decision will not work: > > > https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/crw_cert_ve... > > > > Does it make sense? > > No. It's not clear to me why you impose this constraint on your callers, rather > than 'fixing up' incoming certs to normalize the CertPolicy::Judgement (e.g. > stripping off intermediate certificates yourself) > > That is, I find it weird to force callers to be aware of these semantics, a > result of the inconsistency between the two APIs you mentioned > (didFailProvisionalNavigation vs interstitials), rather than just fixing it > yourself and leaving it as an implementation detail as to why. My understanding > is that this method (allowCert) is tightly-coupled to the implementation of > decideLoadPolicyForTrust (which I assume is what didFail is calling), but you're > forcing callers to be aware of that coupling rather than handling it yourself > (e.g. having both allowCert and decideLoadPolicy only consider/store the leaf). > By handling it yourself, you provide a strong API contract to callers (these two > methods are complementary), without making them worry about the details. I see your point. Changed allowCert: to strip intermediate certs.
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/29 18:29:07, eugenebut wrote: https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_... > > (arbitrary conditional-based cache; named Expiring, but you get to define what > > "Expiring" means) > Consider the following use case: > 1. didReceiveAuthenticationChallenge: is called for http://www.example.com with bad > cert A. > 2. didReceiveAuthenticationChallenge: is called for http://www.example.com with bad > cert B (certs are different, hosts are the same). > 3. didFailProvisionalNavigation: is called with cert A (because it was cert for > top level frame request). OK, this is super-helpful in understanding your model. First, the odds of 1 & 2 happening are incredibly, incredibly low. It should never happen in the real world, naturally, except for one circumstance - the administrator is reconfiguring their server in between the loads of the user. That's why I think it's OK to handle this situation with a slightly pessimal (to battery/performance) case, because users should *never* run in to it in the wild. This is also why the CertVerifier cache expires results (which this doesn't, which is also problematic) - in the situations where it's expected (e.g. a newly renewed certificate is installed on the server, it's restarted, and the user accesses the site at both #1 and #2), we'll end up flushing any results that lead to a bad experience relatively quickly. > Now in didFailProvisionalNavigation: I need to retrieve CertStatus for > "www.example.com" - "cert A" pair. > > Your suggestion is totally clear to me, but I don't want to reevaluate cert due > to the following reasons: > 1. didFailProvisionalNavigation: may not provide a correct chain (not sure if > it's actually a problem for SecTrust API, but it can be) If didReceiveAuthenticationChallenge handled the load successfully, then re-validating with SecTrust will also succeed. If didReceiveAuthenticationChallenge failed, then recreating a SecTrust will also yield the same results. The chain that's provided here is the server's chain, and that's just an input to SecTrust to get the verified chain. The fact that there's inconsistency between the server chain and verified chain is benign, so long as the leaf cert is the same. > 2. Reevaluation is async operation, which needs to be canceled if user navigates > back before reevaluation is completed (and it will make code more complex) I don't think so. We don't abort validations on any other platform (because we can't; literally, the OS APIs are sync and unabortable), so I don't think it's necessary to design a mechanism for aborting on navigation. > 3. Reevaluation of a bad cert is expensive so it impacts battery life. Agreed in the general case, but if the only way this can come up is if the server reconfigures the server certificate to a different *leaf* in between loads, then the odds of this happening are so low that the complexity tradeoffs for the general case are arguably too high compared to the performance tradeoffs for the edge case. Let's be clear here: by having the Key be the host+cert pair, you're trading memory here (especially since this grows unbounded) in the general case, for power in the edge case. I'm pushing back a little, as I think the priorities should be for optimizing the general case - such as an MRU/LRU keyed by hostname, and with evictions if the cert ever changes (or is evicted by the *RU), and re-evaluations whenever an eviction occurs. That helps you put a tight bound on the memory overhead, and lets you then adjust that memory tradeoff for CPU based on real world usage patterns (e.g. does the user evaluate more than 256 certs within a 5 minute period? etc) > I understand the concert about memory usage. But _certVerificationErrors stores > certs only for a short period of time (between receiving the cert and actual > start/failure of the load). I'm confused here; where's the purging going on? I didn't see it at all in this CL. > Because of that this cache should not grow as it > will be constantly purged. I can't imagine the case when cache will have more > than 5 different certs for a single hostname (Browser does not open more than 5 > connections for a single host within one session, right?). It's... complicated. The //net stack itself has a six socket limit, but there are ways to kick off more validations/connections than that. I don't know what WKWebView has set as it's connection limits - 6 is what we experimentally determined, other browsers do more or less (2 is what the HTTP/1.1 spec originally said, I think it was bumped to 5, others have gone on to 8 or 16 - as we also experimented with)
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/29 20:28:38, Ryan Sleevi wrote: > On 2015/09/29 18:29:07, eugenebut wrote: > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_... > > > (arbitrary conditional-based cache; named Expiring, but you get to define > what > > > "Expiring" means) > > Consider the following use case: > > 1. didReceiveAuthenticationChallenge: is called for http://www.example.com > with bad > > cert A. > > 2. didReceiveAuthenticationChallenge: is called for http://www.example.com > with bad > > cert B (certs are different, hosts are the same). > > 3. didFailProvisionalNavigation: is called with cert A (because it was cert > for > > top level frame request). > > OK, this is super-helpful in understanding your model. > > First, the odds of 1 & 2 happening are incredibly, incredibly low. It should > never happen in the real world, naturally, except for one circumstance - the > administrator is reconfiguring their server in between the loads of the user. Or MITM attack. But I guess it's not a frequent case either. > That's why I think it's OK to handle this situation with a slightly pessimal (to > battery/performance) case, because users should *never* run in to it in the > wild. > > This is also why the CertVerifier cache expires results (which this doesn't, > which is also problematic) - in the situations where it's expected (e.g. a newly > renewed certificate is installed on the server, it's restarted, and the user > accesses the site at both #1 and #2), we'll end up flushing any results that > lead to a bad experience relatively quickly. > > > Now in didFailProvisionalNavigation: I need to retrieve CertStatus for > > "www.example.com" - "cert A" pair. > > > > Your suggestion is totally clear to me, but I don't want to reevaluate cert > due > > to the following reasons: > > 1. didFailProvisionalNavigation: may not provide a correct chain (not sure if > > it's actually a problem for SecTrust API, but it can be) > > If didReceiveAuthenticationChallenge handled the load successfully, then > re-validating with SecTrust will also succeed. If > didReceiveAuthenticationChallenge failed, then recreating a SecTrust will also > yield the same results. > > The chain that's provided here is the server's chain, and that's just an input > to SecTrust to get the verified chain. The fact that there's inconsistency > between the server chain and verified chain is benign, so long as the leaf cert > is the same. > > 2. Reevaluation is async operation, which needs to be canceled if user > navigates > > back before reevaluation is completed (and it will make code more complex) > > I don't think so. We don't abort validations on any other platform (because we > can't; literally, the OS APIs are sync and unabortable), so I don't think it's > necessary to design a mechanism for aborting on navigation. Consider the following scenario: 1.) User navigates to a page with a bad SSL cert for the first time 2.) didFailNavigation: is called with SSL error 3.) Browser starts async reevaluation of bad cert 4.) User clicks "go back" to navigate to the previous page (please note that cert reevaluation has not completed yet) 5.) Cert has been reevaluated but SSL interstitial should not be presented I understand that it should be a rare case in practice, but it can happen and we should have a code to handle that. > > 3. Reevaluation of a bad cert is expensive so it impacts battery life. > > Agreed in the general case, but if the only way this can come up is if the > server reconfigures the server certificate to a different *leaf* in between > loads, then the odds of this happening are so low that the complexity tradeoffs > for the general case are arguably too high compared to the performance tradeoffs > for the edge case. > > Let's be clear here: by having the Key be the host+cert pair, you're trading > memory here (especially since this grows unbounded) in the general case, for > power in the edge case. I'm pushing back a little, as I think the priorities > should be for optimizing the general case - such as an MRU/LRU keyed by > hostname, and with evictions if the cert ever changes (or is evicted by the > *RU), and re-evaluations whenever an eviction occurs. That helps you put a tight > bound on the memory overhead, and lets you then adjust that memory tradeoff for > CPU based on real world usage patterns (e.g. does the user evaluate more than > 256 certs within a 5 minute period? etc) Yes, I'm trading memory to battery usage but *only for edge case*. In general case both battery and memory usage is not affected. The other thing I gain with using cert-host pair is code simplicity (no need for cert reevaluation). Please see my comment below, where I listed places where cache is purged. > > I understand the concert about memory usage. But _certVerificationErrors > stores > > certs only for a short period of time (between receiving the cert and actual > > start/failure of the load). > > I'm confused here; where's the purging going on? I didn't see it at all in this > CL. I actually missed one place, where I should have purged this cache. I've updated CL and the new list is: Line 587 (abortLoad) Line 1462 (load failed) Line 1468 (server started responding with data, which means that TLS handshake has been completed) Line 1505 (navigation has failed) > > Because of that this cache should not grow as it > > will be constantly purged. I can't imagine the case when cache will have more > > than 5 different certs for a single hostname (Browser does not open more than > 5 > > connections for a single host within one session, right?). > > It's... complicated. The //net stack itself has a six socket limit, but there > are ways to kick off more validations/connections than that. I don't know what > WKWebView has set as it's connection limits - 6 is what we experimentally > determined, other browsers do more or less (2 is what the HTTP/1.1 spec > originally said, I think it was bumped to 5, others have gone on to 8 or 16 - as > we also experimented with) Thanks this is interesting and helpful. Was I able to convince you that memory usage is not affected by cert-host caching?
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/29 21:16:47, eugenebut wrote: > Or MITM attack. But I guess it's not a frequent case either. Sure. But doing something battery expensive under MITM is perfectly reasonable. > Consider the following scenario: > 1.) User navigates to a page with a bad SSL cert for the first time > 2.) didFailNavigation: is called with SSL error > 3.) Browser starts async reevaluation of bad cert > 4.) User clicks "go back" to navigate to the previous page (please note that > cert reevaluation has not completed yet) > 5.) Cert has been reevaluated but SSL interstitial should not be presented What I don't understand is why #5 matters at all for this discussion. Perhaps we are using 'aborting' differently. I'm guessing you're wiring up a completion handler that says "show interstitial" to the result of CertVerifier. If you are, that's a wrong pattern, and that may explain why things are complicated. You should model the navigation loading as a state machine, and your completion handler for cert verification should just be "verification completed" - and from that, and based on the navigation state you know of the page, you make a decision there. > Yes, I'm trading memory to battery usage but *only for edge case*. > In general case both battery and memory usage is not affected. > The other thing I gain with using cert-host pair is code simplicity (no need for > cert reevaluation). I'm not sure I'd agree with simplicitly. Much like the SecTrust block adapter, this code feels significantly more complicated than needed, which is why I'm pushing back hard to understand the use case and whether or not we can accomplish this with less code, with existing code, and what the edge cases are. > Please see my comment below, where I listed places where cache is purged. Thanks > Was I able to convince you that memory usage is not affected by cert-host > caching? Not really. This still feels way more complex and complicated than needed, but I fully acknowledge I may be missing something. I'll take another look at the CL and try to offer more constructive feedback, but I'm still incredibly uneasy about the Host+Cert combination as the key. It *feels* to me that any solution that requires that is fundamentally flawed at the design, which is why I'm trying to wrap my head around this code more. If it's a design limitation imposed by Apple, then I can understand and appreciate this more, but if it's a design limitation we're imposing on ourselves, I'd like to try to find a solution. I'll take another stab at this CL in the larger context, but please don't let that discourage you from considering any alternatives you can think of that can simplify the code and complexity here.
Ok I guess we are in the agreement that using cert+host key does not use more memory (let me know if that’s not true). So now the main question is simplicity. And approach presented in this CL is the following: 1.) Verify cert inside didReceiveAuthenticationChallenge: 2.) Cache verification result using cert+host as a key 3.) If didCommitNavigation: fails with SSL error then a). Get read status from the cache b). Present interstitial 4.) Clear cache on load abort/finish/failure Your proposal (as I understand it): 1.) Verify cert inside didReceiveAuthenticationChallenge: 2.) Cache verification result using host as a key (store host as a part of value) 3.) If didCommitNavigation: fails with SSL error then a). Get read status from the cache b). On cache hit present interstitial c). Otherwise reevaluate the cert and if navigation did not happen, present interstitial 4.) Clear cache or not? If not then it will affect the memory usage for general case. Did I misunderstand your proposal?
On 2015/09/29 21:48:33, eugenebut wrote: > Ok I guess we are in the agreement that using cert+host key does not use more > memory > (let me know if that’s not true). > > So now the main question is simplicity. And approach presented in this CL is the > following: > > 1.) Verify cert inside didReceiveAuthenticationChallenge: > 2.) Cache verification result using cert+host as a key > 3.) If didCommitNavigation: fails with SSL error then > a). Get read status from the cache > b). Present interstitial > 4.) Clear cache on load abort/finish/failure > > Your proposal (as I understand it): > > 1.) Verify cert inside didReceiveAuthenticationChallenge: > 2.) Cache verification result using host as a key (store host as a part of > value) > 3.) If didCommitNavigation: fails with SSL error then > a). Get read status from the cache > b). On cache hit present interstitial > c). Otherwise reevaluate the cert and if navigation did not happen, present > interstitial > 4.) Clear cache or not? If not then it will affect the memory usage for general > case. > > Did I misunderstand your proposal? One alternative I can offer for code simplification is to drop cache at all. It means that app will run reevaluation every time before presenting SSL interstitial. Since it's not a general case (unless I misunderstood UMA data), we can probably sacrifice battery live here. WDYT?
Joel, could you please review this CL. We want to land this in M47 timeframe.
On 2015/09/29 21:25:28, Ryan Sleevi (busy till 10-12) wrote: > I'm not sure I'd agree with simplicitly. Much like the SecTrust block adapter, > this code feels significantly more complicated than needed Having read the discussion here several times, and looked over the CL several times, I'm not clear on what the complexity objection is. The only added complexity I see in Eugene's approach is a compound key (which is minimal complexity), and the advantage seems to be that we can guarantee a single, synchronous codepath later rather than having another branch in the logic and another asynchronous cycle. That seems like a pretty significant complexity win to me. >> Was I able to convince you that memory usage is not affected by cert-host >> caching? > > Not really. On the memory side, the fact that this is bounded to a single load cycle seems like it puts a pretty reasonable upper bound on memory. I can't tell if you disagree; if so, it would be really helpful for me if you could explain what the pathological case you're concerned about where growth can be essentially unbounded.
https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/cert_verif... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/cert_verif... ios/web/net/cert_verification_cache.h:36: class CertVerificationCache { Per Ryan's earlier comments, can this be replaced with an existing cache class (e.g., that's already unit tested) https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:165: scoped_refptr<net::X509Certificate> leafCert = Fix the indentation of this statement. (Is this a clang format bug?) https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (_certVerificationErrors.Get(leafCert, base::SysNSStringToUTF8(host), Why |if| rather than a DCHECK? I thought the advantage of your cache approach was that you can't have cache misses. https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:916: [self loadCurrentURL]; There is (currently; it needs to move into NM) special handling of transient entries in reloadInternal, so you may want to call reload instead of this. https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1534: } Per my note in the other CL, I'd prefer everything after this be moved to a helper to avoid someone accidentally turning this into a self-capturing block later.
https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/40001/ios/web/net/cert_verifi... ios/web/net/cert_verification_cache.h:41: map_[KeyType(cert, host)] = value; On 2015/09/28 22:46:52, Ryan Sleevi (busy till 10-12) wrote: > On 2015/09/25 21:24:23, eugenebut wrote: > > This cache object will be used for transferring net::CertStatus between > > didReceiveAuthenticationChallenge: and didFailProvisionalNavigation: > callbacks. > > didReceiveAuthenticationChallenge: will calculate CertStatus in order to make > > load/no-load decision. didFailProvisionalNavigation: will be called when top > > level frame load has failed and this callback will be used for presenting SSL > > interstitial. > > > > didReceiveAuthenticationChallenge: is called for all requests, including > > sub-requests, which means that in case of server misconfiguration this cache > may > > hold multiple certs for the same hostname. This is the reason why hostname > alone > > can't be used as a key. > > > I guess I'm still rather confused here. Perhaps it would help to speak more of > concrete loading cases and where you would expect something 'bad' to happen. > > And in case my suggestion wasn't clear, here's what I was concretely suggesting: > > Store one |value| per host (aka a string key). Store the cert you verified as > part of |value|. > > When you get either of these callbacks, look up based on |host|. Is the |cert| > the same? > > If the cert is the same, you should be able to use the verification result [If > you think you can't, it may help to explain further why that's so]. > If the cert is not the same, then force the client to go back through the > revalidation flow and recompute the cert status, evicting the current |value| > from the cache. [If you can't go through revalidation, it may help to explain > what scenarios you see going on?] > > As I see it, there's two types of configuration changes that would invalidate > value (assuming value contains cert, rather than the key) > 1) The server changes certificates during communication (a server > reconfiguration). It's fine to "fail closed" in this case (e.g. recompute > status, using more resources, etc) > 2) The client changes configuration that may lead to a new result. Since we > can't detect when this happens, it's fair to say it's an API limitation. > > As it stands, I'm concerned that a cert that constantly rotated certificates > could cause unbounded growth in the client. The same way that a single page load > forcing us to load a ton of pages can also cause unbounded growth in memory. > > In Chrome, we're fairly aggressive in trying to prevent such situations in the > browser process. For example, if a tab goes away, we make sure to flush all the > cert caches associated with the renderer process. > > > I would appreciate if you can point me to something that allows mapping from > > string-cert pair to arbitrary value. Unfortunately I could not find anything > > suitable. > > https://code.google.com/p/chromium/codesearch#chromium/src/base/containers/mr... > (access based cache) > https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_... > (arbitrary conditional-based cache; named Expiring, but you get to define what > "Expiring" means) Replaced bicycle with MRUCache. Thanks for suggestions. https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/cert_verif... File ios/web/net/cert_verification_cache.h (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/cert_verif... ios/web/net/cert_verification_cache.h:36: class CertVerificationCache { On 2015/10/08 16:52:39, stuartmorgan wrote: > Per Ryan's earlier comments, can this be replaced with an existing cache class > (e.g., that's already unit tested) Done. https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:165: scoped_refptr<net::X509Certificate> leafCert = On 2015/10/08 16:52:39, stuartmorgan wrote: > Fix the indentation of this statement. (Is this a clang format bug?) Fixed. This probably was not clang format bug. https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (_certVerificationErrors.Get(leafCert, base::SysNSStringToUTF8(host), On 2015/10/08 16:52:39, stuartmorgan wrote: > Why |if| rather than a DCHECK? I thought the advantage of your cache approach > was that you can't have cache misses. I don't want to assert that this leaf cert has been provided to didReceiveAuthenticationChallenge. didReceiveAuthenticationChallenge and didFailNavigation: delegate method receive different chains, and didFailNavigation: in theory can receive some garbage instead of a valid cert. I can add an else case with UMA if you want, but DCHECK does not seem to be a safe approach. https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:916: [self loadCurrentURL]; On 2015/10/08 16:52:39, stuartmorgan wrote: > There is (currently; it needs to move into NM) special handling of transient > entries in reloadInternal, so you may want to call reload instead of this. Are you sure? How is this case different form UIWebView (which calls |loadCurrentURL|)? https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1534: } On 2015/10/08 16:52:39, stuartmorgan wrote: > Per my note in the other CL, I'd prefer everything after this be moved to a > helper to avoid someone accidentally turning this into a self-capturing block > later. Done.
LGTM assuming we go with this design; per my earlier comment I'd like to fully understand rsleevi's objections though. (We could also consider adding UMA on how big the cache is after every insertion to validate that it is indeed small.) https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (_certVerificationErrors.Get(leafCert, base::SysNSStringToUTF8(host), On 2015/10/09 16:32:36, eugenebut wrote: > On 2015/10/08 16:52:39, stuartmorgan wrote: > > Why |if| rather than a DCHECK? I thought the advantage of your cache approach > > was that you can't have cache misses. > I don't want to assert that this leaf cert has been provided to > didReceiveAuthenticationChallenge. > > didReceiveAuthenticationChallenge and didFailNavigation: delegate method receive > different chains, and didFailNavigation: in theory can receive some garbage > instead of a valid cert. > > I can add an else case with UMA if you want, but DCHECK does not seem to be a > safe approach. Yes, UMA sounds good. If this is anything but incredibly rare, we'll need to consider adding a revalidation codepath (i.e., do the async approach Ryan described). https://codereview.chromium.org/1357773002/diff/180001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/180001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:172: BOOL sucess) { Consider pulling everything in here into a helper method, instead of chaining most of the code two handlers deep in a single statement. I suspect it would make the high-level flow of the main method easier to grok if everything from here on were a call to a well-named helper.
Thanks! https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/140001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: if (_certVerificationErrors.Get(leafCert, base::SysNSStringToUTF8(host), On 2015/10/09 21:30:00, stuartmorgan wrote: > On 2015/10/09 16:32:36, eugenebut wrote: > > On 2015/10/08 16:52:39, stuartmorgan wrote: > > > Why |if| rather than a DCHECK? I thought the advantage of your cache > approach > > > was that you can't have cache misses. > > I don't want to assert that this leaf cert has been provided to > > didReceiveAuthenticationChallenge. > > > > didReceiveAuthenticationChallenge and didFailNavigation: delegate method > receive > > different chains, and didFailNavigation: in theory can receive some garbage > > instead of a valid cert. > > > > I can add an else case with UMA if you want, but DCHECK does not seem to be a > > safe approach. > > Yes, UMA sounds good. If this is anything but incredibly rare, we'll need to > consider adding a revalidation codepath (i.e., do the async approach Ryan > described). Added TODO https://codereview.chromium.org/1357773002/diff/180001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/180001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:172: BOOL sucess) { On 2015/10/09 21:30:00, stuartmorgan wrote: > Consider pulling everything in here into a helper method, instead of chaining > most of the code two handlers deep in a single statement. I suspect it would > make the high-level flow of the main method easier to grok if everything from > here on were a call to a well-named helper. Done.
rsleevi@chromium.org changed reviewers: + davidben@chromium.org
David, could you please take a look at this CL as well! Thank you.
lgtm, with some clarifications. Also, are there any browser_tests/integration tests for iOS where you can add tests to verify total behavior? The unit tests look very good, but it would be excellent to have integration tests for the whole stack. Thanks for pulling through on all of this! https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:30: CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_NOT_ACCEPTED_BY_USER, Is this comment still valid? Isn't it actually something more like: // Cert is not valid. However caller should reject the load because // user has decided not to proceed with this invalid cert. and if that's not accurate, than you might consider renaming the variable to something like "CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_UNDECIDED_BY_USER" https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:72: // Used to remember decisions about how to handle problematic certs. nit: Change to something like: "Used to remember user exceptions to invalid certs". Right now, it's a bit ambiguous. https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:181: // Cert verification task did not start. How can this happen? Should this be a DCHECK?
Thank you Joel! We working on integration tests in parallel and I will add you as a reviewer (once we have CLs). https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:30: CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_NOT_ACCEPTED_BY_USER, On 2015/10/17 20:47:19, jww wrote: > Is this comment still valid? Isn't it actually something more like: > // Cert is not valid. However caller should reject the load because > // user has decided not to proceed with this invalid cert. > > and if that's not accurate, than you might consider renaming the variable to > something like "CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_UNDECIDED_BY_USER" The original comment was correct (cert is indeed invalid, but error is recoverable, so load decision will be up to the user). I changed enum name to CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_UNDECIDED_BY_USER. https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:72: // Used to remember decisions about how to handle problematic certs. On 2015/10/17 20:47:19, jww wrote: > nit: Change to something like: "Used to remember user exceptions to invalid > certs". Right now, it's a bit ambiguous. Done. https://codereview.chromium.org/1357773002/diff/240001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:181: // Cert verification task did not start. On 2015/10/17 20:47:19, jww wrote: > How can this happen? Should this be a DCHECK? I believe that IO thread may not start during shutdown, so I would rather gracefully handle this case rather than DCHECK.
https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:19: const char kHostName2[] = "www.chromium.org"; www.example.com is specially reserved for testing, but www.chromium.org is not Use www.chromium.test ( c.f. http://www.iana.org/assignments/special-use-domain-names/special-use-domain-n... ) https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:61: // UI thread. Include a forward reference to allowCert here // Note: Certificate errors may be bypassed by calling // |allowCert:| with the host, certificate, and certificate error // to ignore. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:80: // recoverable. This comment needs rewording // Records that |cert| is permitted to be used for |host| in future calls to |decideLoadPolicyForTrust:|. // |host| should be in an ASCII-compatible form. // Subsequent calls to |decideLoadPolicyForTrust:| for the same // |cert|/|host|/|status| tuple will return CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_ACCEPTED_BY_USER. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:83: status:(net::CertStatus)status; Does this mean you can only ignore a single error? (Logically, it would seem so; I want to confirm what happens if a user encounters Error A, and then later Error B, and then Error A again) https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: // this case |dispatched| argument will be NO). This is really surprising; should this be in a separate CL? COuld you explain more here? https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:259: // methods receive different certificate chains for the same server cert. You should explain more of the context in the code, to prevent people from having to spleunk what you mean // Store user decisions with the leaf cert, ignoring any intermediates. // This is because WKWebView returns the verified certificate chain in // [?? delegate X], but the server-supplied chain in [?? delegate Y]. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:329: completionHandler(net::CertVerifyResult(), NO); Are you sure this is safe to recursively call into here? It seems really weird given line 178 that this isn't an async dispatch or a return code. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:908: // unnecessary recalculations. Verification results are cached for leaf cert, s/for leaf cert/for the leaf cert/ https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:909: // because cert chain in |didReceiveAuthenticationChallenge:| is OS s/because cert chain/because the cert chain/ https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:909: // because cert chain in |didReceiveAuthenticationChallenge:| is OS s/is OS/is the OS/ https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:910: // constructed chain, while |chain| is a chain from the server. s/a chain/the chain/ https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:918: // storing cert decision. // The complete cert chain may not be available, so the leaf // cert is used as a key to retrieve _certVerificationErrors, as well as // for storing the cert decision. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:921: // This cache will be purged anyway so there is no need to use |Get|. This is surprising/non-obvious, and doesn't seem to fit why why you're using MRUCache. It sounds like you're describing behaviour external to this file, and if so, that's a layering concern; if it's in this file, where? Document. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1104: // Cert is invalid and user has not agreed to proceed. Cache cert // The cert is invalid and the user has not agreed to proceed. Cache // the cert verification result in |_certVerificationErrors|, so that it // can later be reused inside |didFailProvisionalNavigation:|. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1109: // using intermidiates may result in keys mismatch. // The leaf cert is used as the key, because the chain provided by // |didFailProvisionalNavigation:| will differ (it is the server-supplied // chain), thus if intermediates were considered, the keys would mismatch.
Thanks! PTAL. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:19: const char kHostName2[] = "www.chromium.org"; On 2015/10/19 23:56:21, Ryan Sleevi wrote: > http://www.example.com is specially reserved for testing, but http://www.chromium.org is not > > Use http://www.chromium.test ( c.f. > http://www.iana.org/assignments/special-use-domain-names/special-use-domain-n... > ) Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:61: // UI thread. On 2015/10/19 23:56:21, Ryan Sleevi wrote: > Include a forward reference to allowCert here > > // Note: Certificate errors may be bypassed by calling > // |allowCert:| with the host, certificate, and certificate error > // to ignore. Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:80: // recoverable. On 2015/10/19 23:56:21, Ryan Sleevi wrote: > This comment needs rewording > > // Records that |cert| is permitted to be used for |host| in future calls to > |decideLoadPolicyForTrust:|. > // |host| should be in an ASCII-compatible form. > // Subsequent calls to |decideLoadPolicyForTrust:| for the same > // |cert|/|host|/|status| tuple will return > CERT_ACCEPT_POLICY_RECOVERABLE_ERROR_ACCEPTED_BY_USER. Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:83: status:(net::CertStatus)status; On 2015/10/19 23:56:21, Ryan Sleevi wrote: > Does this mean you can only ignore a single error? (Logically, it would seem so; > I want to confirm what happens if a user encounters Error A, and then later > Error B, and then Error A again) This will do whatever web::CertificatePolicyCache does. Currently it will ignore error if it is a subset of |status| or exactly the same as |status|. Added to comments. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:86: // this case |dispatched| argument will be NO). On 2015/10/19 23:56:22, Ryan Sleevi wrote: > This is really surprising; should this be in a separate CL? COuld you explain > more here? WKWebView throws an exception (which leads to a crash) if any of it's completion handlers is not called. To prevent that completion handlers of CRWCertVerificationController must always be called as well. Added comments to the header file. I think it's fine if this change is a part of current CL, will make a separate one though if you have very strong opinion. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:259: // methods receive different certificate chains for the same server cert. On 2015/10/19 23:56:21, Ryan Sleevi wrote: > You should explain more of the context in the code, to prevent people from > having to spleunk what you mean > > // Store user decisions with the leaf cert, ignoring any intermediates. > // This is because WKWebView returns the verified certificate chain in > // [?? delegate X], but the server-supplied chain in [?? delegate Y]. Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:329: completionHandler(net::CertVerifyResult(), NO); On 2015/10/19 23:56:21, Ryan Sleevi wrote: > Are you sure this is safe to recursively call into here? It seems really weird > given line 178 that this isn't an async dispatch or a return code. Yes, this is safe. Maybe weird, but still safe. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:908: // unnecessary recalculations. Verification results are cached for leaf cert, On 2015/10/19 23:56:22, Ryan Sleevi wrote: > s/for leaf cert/for the leaf cert/ Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:909: // because cert chain in |didReceiveAuthenticationChallenge:| is OS On 2015/10/19 23:56:22, Ryan Sleevi wrote: > s/is OS/is the OS/ Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:909: // because cert chain in |didReceiveAuthenticationChallenge:| is OS On 2015/10/19 23:56:22, Ryan Sleevi wrote: > s/because cert chain/because the cert chain/ Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:910: // constructed chain, while |chain| is a chain from the server. On 2015/10/19 23:56:22, Ryan Sleevi wrote: > s/a chain/the chain/ Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:918: // storing cert decision. On 2015/10/19 23:56:22, Ryan Sleevi wrote: > // The complete cert chain may not be available, so the leaf > // cert is used as a key to retrieve _certVerificationErrors, as well as > // for storing the cert decision. Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:921: // This cache will be purged anyway so there is no need to use |Get|. On 2015/10/19 23:56:22, Ryan Sleevi wrote: > This is surprising/non-obvious, and doesn't seem to fit why why you're using > MRUCache. MRUCache allows cache to be bounded and evicts least recent errors, so It's actually ideal. > > It sounds like you're describing behaviour external to this file, and if so, > that's a layering concern; if it's in this file, where? Document. Added more comments. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1104: // Cert is invalid and user has not agreed to proceed. Cache cert On 2015/10/19 23:56:22, Ryan Sleevi wrote: > // The cert is invalid and the user has not agreed to proceed. Cache > // the cert verification result in |_certVerificationErrors|, so that it > // can later be reused inside |didFailProvisionalNavigation:|. Done. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1109: // using intermidiates may result in keys mismatch. On 2015/10/19 23:56:22, Ryan Sleevi wrote: > // The leaf cert is used as the key, because the chain provided by > // |didFailProvisionalNavigation:| will differ (it is the server-supplied > // chain), thus if intermediates were considered, the keys would mismatch. Done.
https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:921: // This cache will be purged anyway so there is no need to use |Get|. On 2015/10/21 04:01:02, eugenebut wrote: > On 2015/10/19 23:56:22, Ryan Sleevi wrote: > > This is surprising/non-obvious, and doesn't seem to fit why why you're using > > MRUCache. > MRUCache allows cache to be bounded and evicts least recent errors, so It's > actually ideal. I suppose this wasn't clear, as I'm still uncertain why you're using Peek instead of Get. Are you trying to optimize? Why? As clearly documented in Get(), if you Get() an error, it actually brings it to the front of the MRU. That's how the "R" in MRU is calculated. So it's surprising and non-obvious that you're invoking a behaviour that doesn't change ordering at all, as it just makes it seem like you're using a bounded cache size, rather than an MRU. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:14: // Test cert filenames. newline between 13/14 https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:17: // Test hostnames. newline between 16/17 https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:20: } // namespace newline between 19/20 https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:28: return net::ImportCertFromFile(net::GetTestCertsDirectory(), file_name); Is there a reason you used the fixture here rather than just putting this in tests? It doesn't seem terribly useful here. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:34: scoped_refptr<net::X509Certificate> cert = GetCert(kCertFileName1); BUG/SAFETY: Each of these tests has a BUG in that they (unsafely) presume that GetCert() will never fail. For robustness, you should have these each as scoped_refptr<net::X509Certificate> cert = ...; ASSERT_TRUE(cert); // stuff https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:26: // load and show net error page. s/and show net/and show a net/ https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:28: // Cert is not valid. Caller may present SSL warning and ask user if they s/present SSL/present a SSL/ s/ask user/ask the user/ https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:31: // Cert is not valid. However caller should proceed with the load because s/Cert is/The cert is/ s/However caller/However, the caller/ https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:32: // user has decided to proceed with this invalid cert. s/user/the user/ https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:262: if (cert->GetIntermediateCertificates().size() > 0) { nit: You could optimize this with !.empty() For a vector, it's arguably the same cost, but when using STL containers, prefer .empty() if you're checking for == 0/>0, since other containers (e.g. list/deque), this may be an O(1) operation rather than O(N) https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:266: DCHECK_EQ(0U, cert->GetIntermediateCertificates().size()); ditto re: .empty() https://codereview.chromium.org/1357773002/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/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:68: // Cache holds errors only for pending navigation, so the actual number of s/pending navigation/pending navigations/ https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: net::SSLInfo SSLInfo; DANGER: This variable naming dangerously borders on the type name, such that it creates plenty of ambiguity. Seems like these variables could be named "info" and "status" rather than "SSLInfo" and "SSLStatus" https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: // to use |Get|. That doesn't seem like a terribly useful justification to use something as surprising/non-obvious as Peek https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:941: if (proceed) { What happens for !proceed? https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1124: {is_recoverable, certStatus}); For what it's worth, https://chromium-cpp.appspot.com/ bans Uniform Initialization Syntax. I guess I'm somewhat surprised that the Obj-Cpp policy would allow it, although I can understand why this may be. It does seem "wrong" though https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1545: _certVerificationErrors->Clear(); I find this code confusing enough that I can't convince myself with confidence that this is 'right' (that is, that you're Clear()ing at all the right places and times). It does seem dangerous that you have to repeat this pattern in so many places, as it suggests adding new delegates/methods could easily 'forget' to clear this error stack. Is there a better way to structure this? I'm not sure. But I'm disclaiming what I simply cannot review/convince myself of correctness.
PTAL. Would really appreciate quick feedback here. https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... File ios/web/web_state/ui/crw_wk_web_view_web_controller.mm (right): https://codereview.chromium.org/1357773002/diff/260001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:921: // This cache will be purged anyway so there is no need to use |Get|. On 2015/10/28 21:28:50, Ryan Sleevi wrote: > On 2015/10/21 04:01:02, eugenebut wrote: > > On 2015/10/19 23:56:22, Ryan Sleevi wrote: > > > This is surprising/non-obvious, and doesn't seem to fit why why you're using > > > MRUCache. > > MRUCache allows cache to be bounded and evicts least recent errors, so It's > > actually ideal. > > I suppose this wasn't clear, as I'm still uncertain why you're using Peek > instead of Get. > > Are you trying to optimize? Why? > > As clearly documented in Get(), if you Get() an error, it actually brings it to > the front of the MRU. That's how the "R" in MRU is calculated. So it's > surprising and non-obvious that you're invoking a behaviour that doesn't change > ordering at all, as it just makes it seem like you're using a bounded cache > size, rather than an MRU. _certVerificationErrors will be purged right after reading from it (see the list line in webView:didFailProvisionalNavigation:withError:), so Peek was an optimization. However I clearly see how that optimization (more-likely negligible) made the code confusing. Changing to Get. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:14: // Test cert filenames. On 2015/10/28 21:28:50, Ryan Sleevi wrote: > newline between 13/14 Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:17: // Test hostnames. On 2015/10/28 21:28:50, Ryan Sleevi wrote: > newline between 16/17 Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:20: } // namespace On 2015/10/28 21:28:50, Ryan Sleevi wrote: > newline between 19/20 Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:28: return net::ImportCertFromFile(net::GetTestCertsDirectory(), file_name); On 2015/10/28 21:28:50, Ryan Sleevi wrote: > Is there a reason you used the fixture here rather than just putting this in > tests? It doesn't seem terribly useful here. Moved to a static function. Fixture is indeed unnecessary. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:34: scoped_refptr<net::X509Certificate> cert = GetCert(kCertFileName1); On 2015/10/28 21:28:50, Ryan Sleevi wrote: > BUG/SAFETY: Each of these tests has a BUG in that they (unsafely) presume that > GetCert() will never fail. > > For robustness, you should have these each as > > scoped_refptr<net::X509Certificate> cert = ...; > ASSERT_TRUE(cert); > // stuff Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.h (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:26: // load and show net error page. On 2015/10/28 21:28:51, Ryan Sleevi wrote: > s/and show net/and show a net/ Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:28: // Cert is not valid. Caller may present SSL warning and ask user if they On 2015/10/28 21:28:51, Ryan Sleevi wrote: > s/present SSL/present a SSL/ > s/ask user/ask the user/ Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:31: // Cert is not valid. However caller should proceed with the load because On 2015/10/28 21:28:51, Ryan Sleevi wrote: > s/Cert is/The cert is/ > s/However caller/However, the caller/ Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.h:32: // user has decided to proceed with this invalid cert. On 2015/10/28 21:28:51, Ryan Sleevi wrote: > s/user/the user/ Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... File ios/web/net/crw_cert_verification_controller.mm (right): https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:262: if (cert->GetIntermediateCertificates().size() > 0) { On 2015/10/28 21:28:51, Ryan Sleevi wrote: > nit: You could optimize this with !.empty() > > For a vector, it's arguably the same cost, but when using STL containers, prefer > .empty() if you're checking for == 0/>0, since other containers (e.g. > list/deque), this may be an O(1) operation rather than O(N) Yeah. This is second codereview, where you explaining size vs. empty, so sorry about that. I guess I'm writing to much Objective-C code and too little C++ :( https://codereview.chromium.org/1357773002/diff/280001/ios/web/net/crw_cert_v... ios/web/net/crw_cert_verification_controller.mm:266: DCHECK_EQ(0U, cert->GetIntermediateCertificates().size()); On 2015/10/28 21:28:51, Ryan Sleevi wrote: > ditto re: .empty() Done. https://codereview.chromium.org/1357773002/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/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:68: // Cache holds errors only for pending navigation, so the actual number of On 2015/10/28 21:28:51, Ryan Sleevi wrote: > s/pending navigation/pending navigations/ Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:899: net::SSLInfo SSLInfo; On 2015/10/28 21:28:51, Ryan Sleevi wrote: > DANGER: This variable naming dangerously borders on the type name, such that it > creates plenty of ambiguity. > > Seems like these variables could be named "info" and "status" rather than > "SSLInfo" and "SSLStatus" Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:924: // to use |Get|. On 2015/10/28 21:28:51, Ryan Sleevi wrote: > That doesn't seem like a terribly useful justification to use something as > surprising/non-obvious as Peek Changed to Get. https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:941: if (proceed) { On 2015/10/28 21:28:51, Ryan Sleevi wrote: > What happens for !proceed? Embedder (chrome) will remove transient entry and interstitial will go away. We'll to change this at some point so web/ layer does all the work, but this is how things done now for UIWebView as well. https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1124: {is_recoverable, certStatus}); On 2015/10/28 21:28:51, Ryan Sleevi wrote: > For what it's worth, https://chromium-cpp.appspot.com/ bans Uniform > Initialization Syntax. > > I guess I'm somewhat surprised that the Obj-Cpp policy would allow it, although > I can understand why this may be. It does seem "wrong" though Done. https://codereview.chromium.org/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1545: _certVerificationErrors->Clear(); On 2015/10/28 21:28:51, Ryan Sleevi wrote: > I find this code confusing enough that I can't convince myself with confidence > that this is 'right' (that is, that you're Clear()ing at all the right places > and times). It does seem dangerous that you have to repeat this pattern in so > many places, as it suggests adding new delegates/methods could easily 'forget' > to clear this error stack. > > Is there a better way to structure this? I'm not sure. But I'm disclaiming what > I simply cannot review/convince myself of correctness. Unfortunately I don't see a better way for clearing this cache. Yes it will be easy to forget about cache clearing if some new WKWebView callbacks are added. However it should not lead to serious problems, because cache is currently cleared in all success and failures callbacks, so it will be purged anyway. Stuart seems to be fine with clearing approach, in case if it gives you some confidence.
https://codereview.chromium.org/1357773002/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/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1545: _certVerificationErrors->Clear(); On 2015/10/29 00:39:14, eugenebut wrote: > Stuart seems to be fine with clearing approach, in case if it gives you some > confidence. Right, I agree it's ideal that we have it in a bunch of places, but this is basically to address your concern about keeping the cache lifetime as short as it can possibly be. The downside of missing a new delegate would be a slightly longer lifetime of a cache entry, which is not terrible. In Q1 we'll be revisiting the structure of how we track page lifetimes so it may be easier to get a bottleneck set up.
https://codereview.chromium.org/1357773002/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/1357773002/diff/280001/ios/web/web_state/ui/c... ios/web/web_state/ui/crw_wk_web_view_web_controller.mm:1545: _certVerificationErrors->Clear(); On 2015/10/29 16:41:06, stuartmorgan wrote: > Right, I agree it's ideal *not ideal
LGTM % nits https://codereview.chromium.org/1357773002/diff/300001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/300001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:45: ASSERT_TRUE(GetCert(kCertFileName2)); I was more trying to suggest you should follow line 35 and assign to a local variable - and assert that local variable is fine. This pattern is dangerous because there is zero relationship with the subsequent lines - that is, if you changed line 47 to kCertFilename3, it would have no effect on these. Assigning to a local variable, asserting that variable is valid, and then pushing it to the object gives you the desired guarantee.
The CQ bit was checked by eugenebut@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from stuartmorgan@chromium.org, jww@chromium.org, rsleevi@chromium.org Link to the patchset: https://codereview.chromium.org/1357773002/#ps320001 (title: "Addressed unit tests review comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1357773002/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1357773002/320001
Thank you very much for reviews! https://codereview.chromium.org/1357773002/diff/300001/ios/web/net/cert_host_... File ios/web/net/cert_host_pair_unittest.cc (right): https://codereview.chromium.org/1357773002/diff/300001/ios/web/net/cert_host_... ios/web/net/cert_host_pair_unittest.cc:45: ASSERT_TRUE(GetCert(kCertFileName2)); On 2015/10/29 22:37:27, Ryan Sleevi wrote: > I was more trying to suggest you should follow line 35 and assign to a local > variable - and assert that local variable is fine. > > This pattern is dangerous because there is zero relationship with the subsequent > lines - that is, if you changed line 47 to kCertFilename3, it would have no > effect on these. > > Assigning to a local variable, asserting that variable is valid, and then > pushing it to the object gives you the desired guarantee. Done.
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/4e0758a248f23e4cc60465431a013a532ea96c89 Cr-Commit-Position: refs/heads/master@{#357018} |
