Attempt an on-demand time fetch when encountering a date invalid error

chrome/browser/ssl/ssl_browser_tests.cc

Index: chrome/browser/ssl/ssl_browser_tests.cc
diff --git a/chrome/browser/ssl/ssl_browser_tests.cc b/chrome/browser/ssl/ssl_browser_tests.cc
index d8f3a8f5d574adf6e028103b2bcd36c15c9544ea..047c8bba9888256743a9222d50faedbcfd227320 100644
--- a/chrome/browser/ssl/ssl_browser_tests.cc
+++ b/chrome/browser/ssl/ssl_browser_tests.cc
@@ -20,6 +20,8 @@
 #include "base/test/histogram_tester.h"
 #include "base/test/simple_test_clock.h"
 #include "base/threading/thread_task_runner_handle.h"
+#include "base/time/default_clock.h"
+#include "base/time/default_tick_clock.h"
 #include "base/time/time.h"
 #include "build/build_config.h"
 #include "chrome/app/chrome_command_ids.h"
@@ -50,8 +52,9 @@
 #include "chrome/test/base/in_process_browser_test.h"
 #include "chrome/test/base/ui_test_utils.h"
 #include "components/content_settings/core/browser/host_content_settings_map.h"
+#include "components/network_time/network_time_test_utils.h"
 #include "components/network_time/network_time_tracker.h"
-#include "components/prefs/pref_service.h"
+#include "components/prefs/testing_pref_service.h"
 #include "components/security_interstitials/core/controller_client.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "components/security_state/security_state_model.h"
@@ -2853,6 +2856,281 @@ IN_PROC_BROWSER_TEST_F(SSLUITest,
       after_interstitial_ssl_status.Equals(clock_interstitial_ssl_status));
 }
 
+// A fixture for testing on-demand network time queries on SSL
+// certificate date errors.

[meacer, 2016/11/01 22:20:18]

nit: Can you also document the capabilities of this class?

e.g. "Can simulate hanging or delayed network requests..."

[estark, 2016/11/02 19:58:12]

Done.

+class SSLNetworkTimeBrowserTest : public SSLUITest {
+ public:
+  SSLNetworkTimeBrowserTest()
+      : SSLUITest(),
+        field_trial_test_(
+            network_time::FieldTrialTest::CreateForBrowserTest()) {}
+  ~SSLNetworkTimeBrowserTest() override {}
+
+ protected:
+  enum TimeServerBehavior {
+    TIME_SERVER_GOOD_RESPONSE,
+    TIME_SERVER_HANG,
+  };
+
+  network_time::FieldTrialTest* field_trial_test() {

[meacer, 2016/11/01 22:20:17]

const method

[estark, 2016/11/02 19:58:13]

Done.

+    return field_trial_test_.get();
+  }
+
+  // Returns a response that hangs.
+  static std::unique_ptr<net::test_server::HttpResponse> HangingResponseHandler(
+      const net::test_server::HttpRequest& request) {
+    return base::MakeUnique<net::test_server::HungResponse>();
+  }
+
+  // Returns a valid time response when |run_loop| quits.
+  static std::unique_ptr<net::test_server::HttpResponse>
+  WaitingGoodTimeResponseHandler(base::RunLoop* run_loop,

[meacer, 2016/11/01 22:20:18]

nit: Waiting -> Delayed? (I don't feel strongly about this)

[estark, 2016/11/02 19:58:12]

Done.

+                                 const net::test_server::HttpRequest& request) {
+    run_loop->Run();
+    return network_time::GoodTimeResponseHandler(request);
+  }
+
+  void SetUpNetworkTimeServer(TimeServerBehavior behavior) {
+    field_trial_test()->SetNetworkQueriesWithVariationsService(
+        true, 0.0, network_time::FieldTrialTest::FETCHES_ON_DEMAND_ONLY);
+
+    switch (behavior) {
+      case TIME_SERVER_GOOD_RESPONSE:
+        embedded_test_server()->RegisterRequestHandler(base::Bind(
+            &SSLNetworkTimeBrowserTest::WaitingGoodTimeResponseHandler,
+            &waiting_good_time_run_loop_));
+        break;
+      case TIME_SERVER_HANG:
+        embedded_test_server()->RegisterRequestHandler(
+            base::Bind(&SSLNetworkTimeBrowserTest::HangingResponseHandler));
+        break;
+    }
+    ASSERT_TRUE(embedded_test_server()->Start());
+    g_browser_process->network_time_tracker()->SetTimeServerURLForTesting(
+        embedded_test_server()->GetURL("/"));
+  }
+
+  void TriggerGoodTimeResponse() {
+    waiting_good_time_run_loop_.QuitClosure().Run();
+  }
+
+ private:
+  std::unique_ptr<network_time::FieldTrialTest> field_trial_test_;
+  base::RunLoop waiting_good_time_run_loop_;
+
+  DISALLOW_COPY_AND_ASSIGN(SSLNetworkTimeBrowserTest);
+};
+
+// Tests that if an on-demand network time fetch returns that the clock
+// is okay, a normal SSL interstitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockOk) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_GOOD_RESPONSE);
+  // Use a testing clock set to the time that GoodTimeResponseHandler
+  // returns, to simulate the system clock matching the network time.
+  base::SimpleTestClock testing_clock;
+  SSLErrorHandler::SetClockForTest(&testing_clock);
+  testing_clock.SetNow(
+      base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime));
+
+  // Set a long timeout to ensure that the on-demand time fetch completes.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+
+  // Once |interstitial_timer_observer| has fired, the request has been
+  // sent. Override the nonce that NetworkTimeTracker expects so that
+  // when the response comes back, it will validate. The nonce can only
+  // be overriden for the current in-flight request, so the test must
+  // call OverrideNonceForTesting() after the request has been sent and
+  // before the response has been received.
+  interstitial_timer_observer.WaitForTimerStarted();
+  g_browser_process->network_time_tracker()->OverrideNonceForTesting(123123123);
+  TriggerGoodTimeResponse();
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();
+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+}
+
+// Tests that if an on-demand network time fetch returns that the clock
+// is wrong, a bad clock interstitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, OnDemandFetchClockWrong) {

[meacer, 2016/11/01 22:20:18]

My understanding is that this test sets the clock so that build time heuristic
won't work. That seems a bit fragile if someone decides to change BTH. Can we
ensure that BTH doesn't fire here, maybe with an EXPECT_EQ?

[estark, 2016/11/02 19:58:13]

The purpose of setting the test clock is actually so that the clock is wrong
relative to network time. However, you're right that it's fragile; if the
network time fetch in the test didn't work, then the BTH could fire. I'm now
using SetBuildTimeForTesting() to set the build time equal to the testing clock
time, so that the BTH will never fire. (Unless of course someone makes it so
that the BTH fires when build time == system time. That seems unlikely though.)

+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_GOOD_RESPONSE);
+  // Use a testing clock set to a time that is different from what
+  // GoodTimeResponseHandler returns, simulating a system clock that is
+  // 30 days ahead of the network time.
+  base::SimpleTestClock testing_clock;
+  SSLErrorHandler::SetClockForTest(&testing_clock);
+  testing_clock.SetNow(
+      base::Time::FromJsTime(network_time::kGoodTimeResponseHandlerTime));

[meacer, 2016/11/01 22:20:17]

Is there a particular reason to use JS time and make
kGoodTimeResponseHandlerTime double? Can it be a uint64 instead, and can you use
base::Time::FromMilliseconds(...) or something like that here?

[estark, 2016/11/02 19:58:12]

My thinking here is that it should be relatively easy to update the time that
GoodTimeResponseHandler returns. To do so with the code as is, you run a curl
command to the real server (see
https://cs.chromium.org/chromium/src/components/network_time/network_time_tes...)
and then copy the 'current_time_millis' value into
|kGoodTimeResponseHandlerTime|. If I were to make |kGoodTimeResponseHandlerTime|
use some other representation, you'd have to convert the 'current_time_millis'
value into that representation... which might be easy to do but might also
depend on internal implementation details of base::Time.

Does that make any sense?

[meacer, 2016/11/02 22:43:22]

Ah, okay. Would it help to rename kGoodTimeResponseHandlerTime to
kGoodTimeResponseHandlerJsTime?

+  testing_clock.Advance(base::TimeDelta::FromDays(30));
+
+  // Set a long timeout to ensure that the on-demand time fetch completes.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+
+  // Once |interstitial_timer_observer| has fired, the request has been
+  // sent. Override the nonce that NetworkTimeTracker expects so that
+  // when the response comes back, it will validate. The nonce can only
+  // be overriden for the current in-flight request, so the test must
+  // call OverrideNonceForTesting() after the request has been sent and
+  // before the response has been received.
+  interstitial_timer_observer.WaitForTimerStarted();
+  g_browser_process->network_time_tracker()->OverrideNonceForTesting(123123123);
+  TriggerGoodTimeResponse();
+
+  EXPECT_TRUE(contents->IsLoading());
+  observer.Wait();

[meacer, 2016/11/01 22:20:18]

You might also want to put a WaitForInterstitialAttach(contents) here.

[estark, 2016/11/02 19:58:12]

Done.

+
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(BadClockBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+}
+
+// Tests that if the timeout expires before the network time fetch
+// returns, then a normal SSL intersitial is shown.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
+                       TimeoutExpiresBeforeFetchCompletes) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_HANG);
+  // Set the timer to fire immediately.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta());
+
+  ui_test_utils::NavigateToURL(browser(), https_server_expired_.GetURL("/"));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  EXPECT_TRUE(contents->ShowingInterstitialPage());
+  InterstitialPage* interstitial_page = contents->GetInterstitialPage();
+  ASSERT_TRUE(interstitial_page);
+  ASSERT_EQ(SSLBlockingPage::kTypeForTesting,
+            interstitial_page->GetDelegateForTesting()->GetTypeForTesting());
+}
+
+// Tests that if the user stops the page load before either the network
+// time fetch completes or the timeout expires, then there is no interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, StopBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_HANG);
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(contents);
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);

[meacer, 2016/11/01 22:20:17]

Is there a reason not to use ui_test_utils::NavigateToURL here too?

[estark, 2016/11/02 19:58:12]

This is based on
CommonNameMismatchBrowserTest.InterstitialsStopNavigationWhileLoading. I assume
the idea here is to Stop() while the page is still loading. So we can't use
NavigateToURL() because NavigateToURL() won't return until the page loads (I
thought?), which I assume won't be until the timer fires or the network fetch
completes, i.e. never.

[meacer, 2016/11/02 22:43:22]

Hmm, the documentation of this method says that this method blocks until the
navigation finishes, but then the code doesn't wait when the last param is
BROWSER_TEST_NONE. Thanks ui_test_utils, very helpful!1

I filed https://crbug.com/661804 to rename the param.

+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::WindowedNotificationObserver observer(
+      content::NOTIFICATION_LOAD_STOP,
+      content::NotificationService::AllSources());
+  contents->Stop();
+  observer.Wait();
+
+  SSLErrorHandler* ssl_error_handler =
+      SSLErrorHandler::FromWebContents(contents);
+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(ssl_error_handler);

[meacer, 2016/11/01 22:20:17]

Nit: single line?
EXPECT_FALSE(SSLErrorHandler::FromWebContents(contents))

[estark, 2016/11/02 19:58:12]

Done.

+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+}
+
+// Tests that if the user reloads the page before either the network
+// time fetch completes or the timeout expires, then there is no interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest, ReloadBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_HANG);
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);

[meacer, 2016/11/01 22:20:18]

Same as above: ui_test_utils::NavigateToURL?

[estark, 2016/11/02 19:58:13]

See above

+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::TestNavigationObserver observer(contents, 1);
+  chrome::Reload(browser(), WindowOpenDisposition::CURRENT_TAB);
+  observer.Wait();
+
+  SSLErrorHandler* ssl_error_handler =
+      SSLErrorHandler::FromWebContents(contents);

[meacer, 2016/11/01 22:20:17]

Same as above: single line?

[estark, 2016/11/02 19:58:13]

Done.

+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(ssl_error_handler);
+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+}
+
+// Tests that if the user navigates away before either the network time
+// fetch completes or the timeout expires, then there is no

[meacer, 2016/11/01 22:20:18]

I think we can improve test and the ones above. We don't seem to check what
happens when a network fetch actually happens.

Can we change the hangs to waits, and then trigger the network time response
after navigating away? Not crashing should be fine.

[estark, 2016/11/02 19:58:13]

Done. This turns out to be a nice simplification because I can use the same
response handler for all the tests, instead of using one that delays and one
that hangs indefinitely.

+// interstitial.
+IN_PROC_BROWSER_TEST_F(SSLNetworkTimeBrowserTest,
+                       NavigateAwayBeforeTimeoutExpires) {
+  ASSERT_TRUE(https_server_expired_.Start());
+  ASSERT_TRUE(https_server_.Start());
+  SetUpNetworkTimeServer(TIME_SERVER_HANG);
+  // Set the timer to a long delay.
+  SSLErrorHandler::SetInterstitialDelayForTest(base::TimeDelta::FromHours(1));
+
+  WebContents* contents = browser()->tab_strip_model()->GetActiveWebContents();
+  SSLInterstitialTimerObserver interstitial_timer_observer(contents);
+
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), https_server_expired_.GetURL("/"),
+      WindowOpenDisposition::CURRENT_TAB, ui_test_utils::BROWSER_TEST_NONE);
+  interstitial_timer_observer.WaitForTimerStarted();
+
+  EXPECT_TRUE(contents->IsLoading());
+  content::TestNavigationObserver observer(contents, 1);
+  browser()->OpenURL(content::OpenURLParams(
+      https_server_.GetURL("/"), content::Referrer(),
+      WindowOpenDisposition::CURRENT_TAB, ui::PAGE_TRANSITION_TYPED, false));
+  observer.Wait();
+
+  SSLErrorHandler* ssl_error_handler =
+      SSLErrorHandler::FromWebContents(contents);
+  // Make sure that the |SSLErrorHandler| is deleted.
+  EXPECT_FALSE(ssl_error_handler);
+  EXPECT_FALSE(contents->ShowingInterstitialPage());
+  EXPECT_FALSE(contents->IsLoading());
+}
+

[meacer, 2016/11/01 22:20:17]

Maybe add another test where the tab is closed? You'll probably need two tabs
since closing the only tab would close the browser.

[estark, 2016/11/02 19:58:13]

Done.

 class CommonNameMismatchBrowserTest : public CertVerifierBrowserTest {
  public:
   CommonNameMismatchBrowserTest() : CertVerifierBrowserTest() {}