WKWebView: Implemented recoverable SSL interstitials.

ios/web/net/cert_verification_cache.h

Index: ios/web/net/cert_verification_cache.h
diff --git a/ios/web/net/cert_verification_cache.h b/ios/web/net/cert_verification_cache.h
new file mode 100644
index 0000000000000000000000000000000000000000..e992b35df7e758484ac9d8237741f3dfd0be9084
--- /dev/null
+++ b/ios/web/net/cert_verification_cache.h
@@ -0,0 +1,72 @@
+// Copyright 2015 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef IOS_WEB_NET_CERT_VERIFICATION_CACHE_H_
+#define IOS_WEB_NET_CERT_VERIFICATION_CACHE_H_
+
+#include <map>
+#include <string>
+
+#include "net/cert/x509_certificate.h"
+
+namespace web {
+
+// Allows caching cert verification data. Key is cert-host pair, value is
+// a template param.

[Ryan Sleevi, 2015/09/24 22:48:38]

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?

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

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.

+template <typename ValueType>
+class CertVerificationCache {
+ public:
+  CertVerificationCache() {}
+  ~CertVerificationCache() {}
+
+  // Retrieves |value| for the given cert-host pair. |cert| cannot be null.
+  // Returns true on success.
+  bool get(const scoped_refptr<net::X509Certificate>& cert,

[Ryan Sleevi, 2015/09/24 22:48:39]

http://google-styleguide.googlecode.com/svn/trunk/cppguide.html#Function_Names

STYLE: This should be Get()

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

Done. Capitalized Set as well for consistency. Style guide permits both set()
and Set() names for small methods.

[Ryan Sleevi, 2015/09/28 22:46:52]

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...

[Eugene But (OOO till 7-30), 2015/09/29 18:29:07]

Thanks for detailed explanation. Un-inlined.

+           const std::string& host,
+           ValueType* value) const {
+    auto it = map_.find(KeyType(cert, host));
+    if (it == map_.end())
+      return false;
+
+    *value = it->second;
+    return true;
+  }
+
+  // Adds |value| for the given cert-host pair. |cert| cannot be null.
+  void set(const scoped_refptr<net::X509Certificate>& cert,
+           const std::string& host,
+           const ValueType& value) {
+    DCHECK(cert);
+    map_[KeyType(cert, host)] = value;

[Ryan Sleevi, 2015/09/24 22:48:39]

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?

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

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 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.

[Ryan Sleevi, 2015/09/28 22:46:52]

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.
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)

[Eugene But (OOO till 7-30), 2015/09/29 18:29:07]

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?).

[Ryan Sleevi, 2015/09/29 20:28:38]

On 2015/09/29 18:29:07, eugenebut wrote:
https://code.google.com/p/chromium/codesearch#chromium/src/net/base/expiring_...
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.
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.
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.
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'm confused here; where's the purging going on? I didn't see it at all in this
CL.
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)

[Eugene But (OOO till 7-30), 2015/09/29 21:16:47]

Or MITM attack. But I guess it's not a frequent case either.
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.
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 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)
Thanks this is interesting and helpful.

Was I able to convince you that memory usage is not affected by cert-host
caching?

[Ryan Sleevi, 2015/09/29 21:25:28]

Sure. But doing something battery expensive under MITM is perfectly reasonable.
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.
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.
Thanks
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.

[Eugene But (OOO till 7-30), 2015/10/09 16:32:36]

Replaced bicycle with MRUCache. Thanks for suggestions.

+  }
+
+  // Clears the cache.
+  void reset() { map_.clear(); }
+
+ private:
+  // Holds cert-host pair and provides less-than comparator.

[Ryan Sleevi, 2015/09/24 22:48:39]

What does this comment add?

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

Removed.

+  struct KeyType {
+    KeyType(const scoped_refptr<net::X509Certificate>& cert,
+            const std::string& host)
+        : cert(cert), host(host) {}
+
+    bool operator<(const KeyType& other) const {

[Ryan Sleevi, 2015/09/24 22:48:38]

Why is this inlined? It can be out-of-lined via a LessThan comparator

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

This operator is inlined, because it is 3 LOC. Is there an advantage of using a
LessThan comparator and making it non inlined?

+      if (host == other.host)

[Ryan Sleevi, 2015/09/24 22:48:39]

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);

[Eugene But (OOO till 7-30), 2015/09/25 21:24:23]

Done.

+        return cert_comparator(cert, other.cert);
+      return host < other.host;
+    }
+
+    scoped_refptr<net::X509Certificate> cert;
+    std::string host;
+
+   private:
+    net::X509Certificate::LessThan cert_comparator;
+  };
+
+  std::map<KeyType, ValueType> map_;
+};
+
+}  // namespace web
+
+#endif  // IOS_WEB_NET_CERT_VERIFICATION_CACHE_H_