Refactor TransportSecurityState.

chrome/browser/transport_security_persister.cc

Index: chrome/browser/transport_security_persister.cc
===================================================================
--- chrome/browser/transport_security_persister.cc	(revision 128526)
+++ chrome/browser/transport_security_persister.cc	(working copy)
@@ -4,16 +4,25 @@
 
 #include "chrome/browser/transport_security_persister.h"
 
+#include "base/base64.h"
 #include "base/bind.h"
 #include "base/file_path.h"
 #include "base/file_util.h"
+#include "base/json/json_reader.h"
+#include "base/json/json_writer.h"
 #include "base/message_loop.h"
 #include "base/path_service.h"
+#include "base/values.h"
 #include "chrome/common/chrome_paths.h"
 #include "content/public/browser/browser_thread.h"
+#include "crypto/sha2.h"
 #include "net/base/transport_security_state.h"
+#include "net/base/x509_certificate.h"
 
 using content::BrowserThread;
+using net::Fingerprint;
+using net::FingerprintVector;
+using net::TransportSecurityState;
 
 class TransportSecurityPersister::Loader {
  public:
@@ -52,7 +61,7 @@
 };
 
 TransportSecurityPersister::TransportSecurityPersister(
-    net::TransportSecurityState* state,
+    TransportSecurityState* state,
     const FilePath& profile_path,
     bool readonly)
     : transport_security_state_(state),
@@ -80,11 +89,168 @@
   transport_security_state_->SetDelegate(NULL);
 }
 
+static ListValue* SPKIHashesToListValue(const FingerprintVector& hashes) {

[Ryan Sleevi, 2012/03/28 00:50:32]

nit: The dominant pattern in Chromium code is to use unnamed namespaces for such
helper classes, in part because of the general need due to
enums/classes/constants/etc (
http://dev.chromium.org/developers/coding-style#TOC-Unnamed-Namespaces )

This isn't a hard rule in the Chromium guide or the Google Open Source style
guide that Chromium incorporates by reference, but it does encourage the use of
unnamed namespaces (
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Namesp...
)

[palmer, 2012/04/10 23:25:51]

Done.

+  ListValue* pins = new ListValue;
+
+  for (FingerprintVector::const_iterator i = hashes.begin();
+       i != hashes.end(); ++i) {
+    std::string hash_str(reinterpret_cast<const char*>(i->data),
+                         sizeof(i->data));
+    std::string b64;
+    base::Base64Encode(hash_str, &b64);
+    pins->Append(new StringValue("sha1/" + b64));
+  }
+
+  return pins;
+}
+
+static void SPKIHashesFromListValue(FingerprintVector* hashes,
+                                    const ListValue& pins) {

[Ryan Sleevi, 2012/03/28 00:50:32]

nit: Parameter order: in -> out

http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...

[palmer, 2012/04/10 23:25:51]

Done.

+  size_t num_pins = pins.GetSize();
+  for (size_t i = 0; i < num_pins; ++i) {
+    std::string type_and_base64;
+    Fingerprint fingerprint;
+    if (pins.GetString(i, &type_and_base64) &&
+        TransportSecurityState::ParsePin(type_and_base64, &fingerprint)) {
+      hashes->push_back(fingerprint);
+    }
+  }
+}
+
+// This function converts the binary hashes, which we store in
+// |enabled_hosts_|, to a base64 string which we can include in a JSON file.
+static std::string HashedDomainToExternalString(const std::string& hashed) {
+  std::string out;
+  CHECK(base::Base64Encode(hashed, &out));

[Ryan Sleevi, 2012/03/28 00:50:32]

Do you really want to crash the browser process here?

Could you not just return an empty string and call it a day (like the next
function)

[palmer, 2012/04/10 23:25:51]

Done.

+  return out;
+}
+
+// This inverts |HashedDomainToExternalString|, above. It turns an external
+// string (from a JSON file) into an internal (binary) string.
+static std::string ExternalStringToHashedDomain(const std::string& external) {
+  std::string out;
+  if (!base::Base64Decode(external, &out) ||
+      out.size() != crypto::kSHA256Length) {
+    return std::string();
+  }
+
+  return out;
+}
+
+// static
+bool TransportSecurityPersister::Deserialize(const std::string& serialized,
+                                             bool* dirty,
+                                             TransportSecurityState* state) {
+  scoped_ptr<Value> value(
+      base::JSONReader::Read(serialized,
+                             false /* do not allow trailing commas */));
+  if (!value.get() || !value->IsType(Value::TYPE_DICTIONARY))

[Ryan Sleevi, 2012/03/28 00:50:32]

nit: You can replace 147 - 150 with:

DictionaryValue* dict_value;
if (!value.get() || !value->GetAsDictionary(&dict_value))
  return false;

[palmer, 2012/04/10 23:25:51]

Done.

+    return false;
+
+  DictionaryValue* dict_value = reinterpret_cast<DictionaryValue*>(value.get());
+  const base::Time current_time(base::Time::Now());
+  bool dirtied = false;
+
+  for (DictionaryValue::key_iterator i = dict_value->begin_keys();
+       i != dict_value->end_keys(); ++i) {
+    DictionaryValue* parsed;
+    if (!dict_value->GetDictionaryWithoutPathExpansion(*i, &parsed))
+      continue;
+
+    bool include_subdomains;
+    std::string mode_string;
+    double created;
+    double expiry;
+    double dynamic_spki_hashes_expiry = 0.0;
+
+    if (!parsed->GetBoolean("include_subdomains", &include_subdomains) ||
+        !parsed->GetString("mode", &mode_string) ||
+        !parsed->GetDouble("expiry", &expiry)) {
+      continue;
+    }
+
+    // Don't fail if this key is not present.
+    (void) parsed->GetDouble("dynamic_spki_hashes_expiry",

[Ryan Sleevi, 2012/03/28 00:50:32]

This method isn't WARN_UNUSED_RESULT, so is this (void) needed?

[palmer, 2012/04/10 23:25:51]

No, it's not needed. Removed.

+                             &dynamic_spki_hashes_expiry);
+
+    ListValue* pins_list = NULL;
+    FingerprintVector static_spki_hashes;
+    // preloaded_spki_hashes is a legacy synonym for static_spki_hashes.
+    if (parsed->GetList("static_spki_hashes", &pins_list))
+      SPKIHashesFromListValue(&static_spki_hashes, *pins_list);
+    else if (parsed->GetList("preloaded_spki_hashes", &pins_list))
+      SPKIHashesFromListValue(&static_spki_hashes, *pins_list);
+
+    FingerprintVector dynamic_spki_hashes;
+    if (parsed->GetList("dynamic_spki_hashes", &pins_list))

[Ryan Sleevi, 2012/03/28 00:50:32]

nit: Since you re-use these strings in both serialization and deserialization,
consider making them constants (in an unnamed namespace) to ensure any changes
are kept in sync/reflected.

[palmer, 2012/04/10 23:25:51]

Done.

+      SPKIHashesFromListValue(&dynamic_spki_hashes, *pins_list);
+
+    TransportSecurityState::DomainState::UpgradeMode mode;
+    if (mode_string == "force-https" || mode_string == "strict") {
+      mode = TransportSecurityState::DomainState::MODE_FORCE_HTTPS;
+    } else if (mode_string == "default" || mode_string == "pinning-only") {
+      mode = TransportSecurityState::DomainState::MODE_DEFAULT;
+    } else {
+      LOG(WARNING) << "Unknown TransportSecurityState mode string found: "
+                   << mode_string;

[Ryan Sleevi, 2012/03/28 00:50:32]

What happened to spdy-only?

Looks like you're dropping it to the floor. If it's deprecated, can you add a
note about that?

If it's not, and you set mode to something, should you also set dirtied to be
safe?

[palmer, 2012/04/10 23:25:51]

It had been deprecated for a long time, and a search of the code showed it was
not used. So yeah I'm dropping it.

+      continue;
+    }
+
+    base::Time expiry_time = base::Time::FromDoubleT(expiry);
+    base::Time dynamic_spki_hashes_expiry_time =
+        base::Time::FromDoubleT(dynamic_spki_hashes_expiry);
+    base::Time created_time;
+    if (parsed->GetDouble("created", &created)) {
+      created_time = base::Time::FromDoubleT(created);
+    } else {
+      // We're migrating an old entry with no creation date. Make sure we
+      // write the new date back in a reasonable time frame.
+      dirtied = true;
+      created_time = base::Time::Now();
+    }
+
+    if (expiry_time <= current_time &&
+        dynamic_spki_hashes_expiry_time <= current_time) {
+      // Make sure we dirty the state if we drop an entry.
+      dirtied = true;
+      continue;
+    }
+
+    std::string hashed = ExternalStringToHashedDomain(*i);
+    if (hashed.empty()) {
+      dirtied = true;
+      continue;
+    }
+
+    TransportSecurityState::DomainState domain_state;
+    domain_state.upgrade_mode = mode;
+    domain_state.created = created_time;
+    domain_state.upgrade_expiry = expiry_time;
+    domain_state.include_subdomains = include_subdomains;
+    domain_state.static_spki_hashes = static_spki_hashes;
+    domain_state.dynamic_spki_hashes = dynamic_spki_hashes;
+    domain_state.dynamic_spki_hashes_expiry = dynamic_spki_hashes_expiry_time;
+    state->EnableHost(hashed, domain_state);
+  }
+
+  *dirty = dirtied;
+  return true;
+}
+
+bool TransportSecurityPersister::LoadEntries(const std::string& serialized,
+                                             bool* dirty,
+                                             TransportSecurityState* state) {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  state->Clear();
+  return Deserialize(serialized, dirty, state);
+}
+
 void TransportSecurityPersister::CompleteLoad(const std::string& state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   bool dirty = false;
-  if (!transport_security_state_->LoadEntries(state, &dirty)) {
+  if (!LoadEntries(state, &dirty, transport_security_state_)) {
     LOG(ERROR) << "Failed to deserialize state: " << state;
     return;
   }
@@ -93,7 +259,7 @@
 }
 
 void TransportSecurityPersister::StateIsDirty(
-    net::TransportSecurityState* state) {
+    TransportSecurityState* state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
   DCHECK_EQ(transport_security_state_, state);
 
@@ -101,7 +267,56 @@
     writer_.ScheduleWrite(this);
 }
 
+bool TransportSecurityPersister::Serialize(std::string* output) const {
+  DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
+
+  DictionaryValue toplevel;
+  base::Time now = base::Time::Now();
+  TransportSecurityState::Iterator state(*transport_security_state_);
+  for (; state.HasNext(); state.Advance()) {
+    const std::string& hostname = state.hostname();
+    const TransportSecurityState::DomainState& domain_state =
+        state.domain_state();
+
+    DictionaryValue* serialized = new DictionaryValue;
+    serialized->SetBoolean("include_subdomains",
+                           domain_state.include_subdomains);
+    serialized->SetDouble("created", domain_state.created.ToDoubleT());
+    serialized->SetDouble("expiry", domain_state.upgrade_expiry.ToDoubleT());
+    serialized->SetDouble("dynamic_spki_hashes_expiry",
+                          domain_state.dynamic_spki_hashes_expiry.ToDoubleT());
+
+    switch (domain_state.upgrade_mode) {
+      case TransportSecurityState::DomainState::MODE_FORCE_HTTPS:
+        serialized->SetString("mode", "force-https");
+        break;
+      case TransportSecurityState::DomainState::MODE_DEFAULT:
+        serialized->SetString("mode", "default");
+        break;
+      default:
+        NOTREACHED() << "DomainState with unknown mode";
+        delete serialized;
+        continue;
+    }
+
+    serialized->Set("static_spki_hashes",
+                    SPKIHashesToListValue(domain_state.static_spki_hashes));
+
+    if (now < domain_state.dynamic_spki_hashes_expiry) {
+      serialized->Set("dynamic_spki_hashes",
+                      SPKIHashesToListValue(domain_state.dynamic_spki_hashes));
+    }
+
+    toplevel.Set(HashedDomainToExternalString(hostname), serialized);
+  }
+
+  base::JSONWriter::WriteWithOptions(&toplevel,
+                                     base::JSONWriter::OPTIONS_PRETTY_PRINT,
+                                     output);
+  return true;
+}
+
 bool TransportSecurityPersister::SerializeData(std::string* data) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
-  return transport_security_state_->Serialise(data);
+  return Serialize(data);
 }