network_time_tracker: add temporary time protocol.

components/network_time/network_time_tracker.cc

Index: components/network_time/network_time_tracker.cc
diff --git a/components/network_time/network_time_tracker.cc b/components/network_time/network_time_tracker.cc
index 73f66ddfbd710706e5b75cb8379013414e49f570..e60ec39c3c1dd1b094d81e5aaf9160110dc6d285 100644
--- a/components/network_time/network_time_tracker.cc
+++ b/components/network_time/network_time_tracker.cc
@@ -8,18 +8,27 @@
 #include <utility>
 
 #include "base/i18n/time_formatting.h"
+#include "base/json/json_reader.h"
 #include "base/logging.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/time/tick_clock.h"
 #include "build/build_config.h"
+#include "components/client_update_protocol/ecdsa.h"
 #include "components/network_time/network_time_pref_names.h"
 #include "components/prefs/pref_registry_simple.h"
 #include "components/prefs/pref_service.h"
+#include "net/base/load_flags.h"
+#include "net/http/http_response_headers.h"
+#include "net/url_request/url_fetcher.h"
+#include "url/gurl.h"
 
 namespace network_time {
 
 namespace {
 
+// Minimum number of minutes between time queries.
+const uint32_t kMinimumQueryDelayMinutes = 60;
+
 // Number of time measurements performed in a given network time calculation.
 const uint32_t kNumTimeMeasurements = 7;
 
@@ -41,6 +50,33 @@ const char kPrefUncertainty[] = "uncertainty";
 // Name of a pref that stores the network time via |ToJsTime|.
 const char kPrefNetworkTime[] = "network";
 
+const char kTimeServiceURL[] = "http://clients2.google.com/time/1/current";
+
+// This is an ECDSA prime256v1 named-curve key.
+const int kKeyVersion = 1;
+const uint8_t kKeyPubBytes[] = {
+    0x30, 0x59, 0x30, 0x13, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02,
+    0x01, 0x06, 0x08, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x03, 0x01, 0x07, 0x03,
+    0x42, 0x00, 0x04, 0xeb, 0xd8, 0xad, 0x0b, 0x8f, 0x75, 0xe8, 0x84, 0x36,
+    0x23, 0x48, 0x14, 0x24, 0xd3, 0x93, 0x42, 0x25, 0x43, 0xc1, 0xde, 0x36,
+    0x29, 0xc6, 0x95, 0xca, 0xeb, 0x28, 0x85, 0xff, 0x09, 0xdc, 0x08, 0xec,
+    0x45, 0x74, 0x6e, 0x4b, 0xc3, 0xa5, 0xfd, 0x8a, 0x2f, 0x02, 0xa0, 0x4b,
+    0xc3, 0xc6, 0xa4, 0x7b, 0xa4, 0x41, 0xfc, 0xa7, 0x02, 0x54, 0xab, 0xe3,
+    0xe4, 0xb1, 0x00, 0xf5, 0xd5, 0x09, 0x11};
+
+static std::string GetServerProof(const net::URLFetcher* source) {

[Ryan Sleevi, 2016/04/28 01:08:34]

This is in an unnamed namespace; the static is unnecessary

[mab, 2016/04/28 02:29:04]

Done.

+  const net::HttpResponseHeaders* response_headers =
+      source->GetResponseHeaders();
+  if (response_headers == nullptr) {

[Ryan Sleevi, 2016/04/28 01:08:34]

if (!response_headers) ?

[mab, 2016/04/28 02:29:04]

Sorry, didn't know that was preferred style.  Done.

+    return std::string();
+  }
+  std::string proof;
+  return response_headers->EnumerateHeader(nullptr, "x-cup-server-proof",
+                                           &proof)
+             ? proof
+             : std::string();
+}
+
 }  // namespace
 
 // static
@@ -52,8 +88,10 @@ void NetworkTimeTracker::RegisterPrefs(PrefRegistrySimple* registry) {
 NetworkTimeTracker::NetworkTimeTracker(
     std::unique_ptr<base::Clock> clock,
     std::unique_ptr<base::TickClock> tick_clock,
-    PrefService* pref_service)
-    : clock_(std::move(clock)),
+    PrefService* pref_service,
+    net::URLRequestContextGetter* getter)
+    : getter_(getter),
+      clock_(std::move(clock)),
       tick_clock_(std::move(tick_clock)),
       pref_service_(pref_service) {
   const base::DictionaryValue* time_mapping =
@@ -83,12 +121,104 @@ NetworkTimeTracker::NetworkTimeTracker(
     pref_service_->ClearPref(prefs::kNetworkTimeMapping);
     network_time_at_last_measurement_ = base::Time();  // Reset.
   }
+  if (getter != nullptr) {

[Ryan Sleevi, 2016/04/28 01:08:34]

Chromium base/ owners chided me enough, but I believe the preference here is to
just use the boolean form, as that also 'transparently' works for other pointer
types in Chromium (unique, scoped_refptr, weakptr), without incurring the added
.get() call or requiring overloading the operators

That is,
if (getter) {

[mab, 2016/04/28 02:29:03]

Done.

+    query_signer_ = client_update_protocol::Ecdsa::Create(
+        kKeyVersion,
+        {reinterpret_cast<const char*>(kKeyPubBytes), sizeof(kKeyPubBytes)});

[Ryan Sleevi, 2016/04/28 01:08:34]

Officially, Uniform Initialization Syntax is not allowed (
https://chromium-cpp.appspot.com/ ), but I think that's just because the CL to
update it hasn't landed yet (based on
https://groups.google.com/a/chromium.org/forum/?utm_medium=email&utm_source=f...
)

That said, based on the guidance from the aforementioned internal document, I
don't believe this usage is consistent with that style, although perhaps I've
misunderstood.

[mab, 2016/04/28 02:29:03]

Rewritten to follow the super-fresh advice at
https://sites.google.com/a/chromium.org/dev/developers/coding-style/cpp-dos-a...

+    base::TimeDelta period =
+        base::TimeDelta::FromMinutes(kMinimumQueryDelayMinutes);
+    query_timer_.Start(FROM_HERE, period, this,
+                       &NetworkTimeTracker::QueryTimeService);
+  }
 }
 
 NetworkTimeTracker::~NetworkTimeTracker() {
   DCHECK(thread_checker_.CalledOnValidThread());
 }
 
+void NetworkTimeTracker::QueryTimeService() {
+  DCHECK(thread_checker_.CalledOnValidThread());
+
+  // If GetNetworkTime() returns true, the NetworkTimeTracker thinks
+  // it is in sync, so there is no need to query.
+  base::Time network_time;
+  if (GetNetworkTime(&network_time, nullptr)) {
+    return;
+  }
+
+  std::string query_string;
+  query_signer_->SignRequest({"", 0}, &query_string);

[Ryan Sleevi, 2016/04/28 01:08:34]

Same with UIS here - I believe you should just use base::StringPiece() or simply
nullptr and let implicit conversion work.

[mab, 2016/04/28 02:29:03]

Done.

+  GURL url(kTimeServiceURL);
+  GURL::Replacements replacements;
+  replacements.SetQueryStr(query_string);

[Ryan Sleevi, 2016/04/28 01:08:33]

Is the assumption that kTimeServiceURL can never contain existing query string
parameters?

[mab, 2016/04/28 02:29:04]

I guess so?  That's the case now, and I wasn't anticipating having to change it.

+  url = url.ReplaceComponents(replacements);
+
+  // This cancels any outstanding fetch.
+  time_fetcher_ = net::URLFetcher::Create(url, net::URLFetcher::GET, this);
+  if (time_fetcher_ == nullptr) {

[Ryan Sleevi, 2016/04/28 01:08:34]

if (time_fetcher_)

[mab, 2016/04/28 02:29:04]

if (!time_fetcher_) rather, but done.

+    DVLOG(1) << "tried to make fetch happen; failed";
+    return;
+  }
+  time_fetcher_->SetRequestContext(getter_);
+  // Not expecting any cookies, but just in case.
+  time_fetcher_->SetLoadFlags(
+      net::LOAD_DISABLE_CACHE | net::LOAD_DO_NOT_SEND_COOKIES |
+      net::LOAD_DO_NOT_SAVE_COOKIES | net::LOAD_BYPASS_CACHE |

[Ryan Sleevi, 2016/04/28 01:08:34]

Perhaps structure the cache and cookie directives adjacent to eachother?

[mab, 2016/04/28 02:29:03]

Done.

+      net::LOAD_DO_NOT_SEND_AUTH_DATA);
+  time_fetcher_->Start();
+  fetch_started_ = tick_clock_->NowTicks();
+}
+
+void NetworkTimeTracker::OnURLFetchComplete(const net::URLFetcher* source) {
+  DCHECK(thread_checker_.CalledOnValidThread());
+  DCHECK(source);
+  if (source->GetStatus().status() != net::URLRequestStatus::SUCCESS &&
+      source->GetResponseCode() != 200) {
+    DVLOG(1) << "fetch failed, status=" << source->GetStatus().status()
+             << ",code=" << source->GetResponseCode();
+    return;
+  }
+
+  std::string response_body;
+  if (!source->GetResponseAsString(&response_body)) {

[Ryan Sleevi, 2016/04/28 01:08:34]

Note: Usually this is discouraged, because it may be an unbounded message size.
I defer to Matt here

[mab, 2016/04/28 02:29:04]

What's the best alternative?  I could check the length before decoding and bail
out if it's unreasonably large.

[mmenke, 2016/04/28 15:17:08]

You can't, actually - by the point OnURLFetchComplete is called, we've already
decoded the body, and stored it all in memory.  Currently, you can either save
to a file (Which URLFetcher supports directly) or provide your own
URLFetcherResponseWriter implementation, which caps the length and saves to a
string.

[mab, 2016/04/29 19:42:07]

OK, I did the latter (= provided a size-limiting implementation of
URLFetcherResponseWriter).

+    DVLOG(1) << "failed to get response";
+    return;
+  }
+  DCHECK(query_signer_.get());

[Ryan Sleevi, 2016/04/28 01:08:34]

I believe you drop the .get(), it should still have the same behaviour by
invoking the bool conversion

DCHECK(query_signer_)

[mab, 2016/04/28 02:29:04]

Done.

+  if (!query_signer_->ValidateResponse(response_body, GetServerProof(source))) {
+    DVLOG(1) << "invalid signature";
+    return;
+  }
+  response_body = response_body.substr(5);  // Skips leading )]}'\n
+  base::JSONReader reader;
+  std::unique_ptr<base::Value> value = reader.Read(response_body);

[Ryan Sleevi, 2016/04/28 01:08:34]

Parsing JSON in the browser process is generally discouraged, although
exceptions may exist if the source of the data is 'trusted'. 

rsesek: You good with this?

[Robert Sesek, 2016/04/28 16:51:23]

How trusted is this data? Fetched from pinned, https, Google connection?

We generally prefer to always parse out-of-process, but we don't do so
exclusively.

[Robert Sesek, 2016/04/28 16:51:23]

You can just use the static ::Read if you're not holding onto the reader.

[mmenke, 2016/04/28 16:54:29]

We look to be fetching from an http connection, so not trusted.

[waffles, 2016/04/28 16:58:57]

We fetch over HTTP protected by CUP with a pinned ECDSA key, (the same way
browser updates do). A MITM would have to compromise the ECDSA signature of the
[request, response] pair in order to inject malicious content that passes
query_signer->ValidateResponse on :188. This code should continue to work in the
presence of a compromised HTTPS stack (e.g. clock is bad).

[Robert Sesek, 2016/04/28 22:37:32]

OK, then lgtm.

[mab, 2016/04/29 19:42:07]

Done.

+  if (value == nullptr) {
+    DVLOG(1) << "bad JSON";
+    return;
+  }
+  const base::DictionaryValue* dict;
+  if (!value->GetAsDictionary(&dict)) {
+    DVLOG(1) << "not a dictionary";
+    return;
+  }
+  double current_time_millis;
+  if (!dict->GetDouble("current_time_millis", &current_time_millis)) {
+    DVLOG(1) << "no current_time_millis";
+    return;
+  }
+  // There's also a "server_nonce" key, which we can ignore.

[Ryan Sleevi, 2016/04/28 01:08:34]

Who is the "we" here?

https://groups.google.com/a/chromium.org/d/msg/chromium-dev/NH-S6KCkr2M/yv0z3...

// There's also a "server_nonce" key, but that can be ignored.

[natural next question is why]

[mab, 2016/04/28 02:29:04]

Tried to address both.  If you ask me "why" again, though, I will have to appeal
to authority.  :-/

+  base::Time current_time = base::Time::FromJsTime(current_time_millis);
+  // The extra 10 seconds comes from a property of the time server that we
+  // happen to know, which is its maximum allowable clock skew.  It's unlikely
+  // that it would ever be that badly wrong, but all the same it's included here
+  // to document the very rough nature of the time service provided by this
+  // class.

[Ryan Sleevi, 2016/04/28 01:08:34]

// The 10 seconds happens to be a known property of the time server,
// which is that is its maximum allowable clock skew.
...


Should the magic value '10' be moved up to be associated with the Time Server
URL, to indicate the coupling of these two?

[mab, 2016/04/28 02:29:03]

Done.

+  base::TimeDelta resolution =
+      base::TimeDelta::FromMilliseconds(1) + base::TimeDelta::FromSeconds(10);
+  base::TimeDelta latency = tick_clock_->NowTicks() - fetch_started_;
+  UpdateNetworkTime(current_time, resolution, latency, tick_clock_->NowTicks());
+}
+
 void NetworkTimeTracker::UpdateNetworkTime(base::Time network_time,
                                            base::TimeDelta resolution,
                                            base::TimeDelta latency,
@@ -132,6 +262,10 @@ void NetworkTimeTracker::UpdateNetworkTime(base::Time network_time,
   time_mapping.SetDouble(kPrefNetworkTime,
       network_time_at_last_measurement_.ToJsTime());
   pref_service_->Set(prefs::kNetworkTimeMapping, time_mapping);
+
+  if (!query_timer_.user_task().is_null()) {
+    query_timer_.Reset();

[Ryan Sleevi, 2016/04/28 01:08:34]

Why the conditional? Why not explicitly call it all the time?

(Context, checking .is_null() on a callback is _usually_ an anti-pattern, since
it's largely an implementation detail of the message passing system; but I may
have missed something special to your use here, hence the question)

[mab, 2016/04/28 02:29:03]

Lamely, it's for unit tests.  (You can't Reset the timer if it was never
Started, and you can't even start it when there isn't a task runner.)

Changed to test getter_ instead, to make it clearer what's going on (?).

[mmenke, 2016/04/28 15:17:08]

I'd suggest using a real getter in unit tests. 
net/url_request/url_request_test_util.h has a TestURLRequestContextGetter class
that you can use.  I'd also suggest using the embedded test server to return
both valid responses, and errors.  Seems like not testing OnURLFetchComplete is
a mistake.  Beyond the scope of this CL, though.  Happy to talk more about
specifics, and/or help out reviewing tests that wire things up.

[mab, 2016/04/29 19:42:07]

Done.

Solid unit tests are very much in scope for this CL.  I've rewritten this to use
the EmbeddedTestServer; let me know how it looks.  

(I'm especially unsure of myself when it comes to thread-safety; still getting a
feel for how that works in Chromium.)

+  }
 }
 
 bool NetworkTimeTracker::GetNetworkTime(base::Time* network_time,