Improve TransportSecurityState data storage.

chrome/browser/net/transport_security_persister.cc

 // Copyright (c) 2012 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.
 
 #include "chrome/browser/net/transport_security_persister.h"
 
 #include "base/base64.h"
 #include "base/bind.h"
 #include "base/file_util.h"
 #include "base/files/file_path.h"
@@ skipping 141 matching lines @@
   DCHECK_EQ(transport_security_state_, state);
 
   if (!readonly_)
     writer_.ScheduleWrite(this);
 }
 
 bool TransportSecurityPersister::SerializeData(std::string* output) {
   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();
+  std::map<std::string, TransportSecurityState::DynamicEntry>::const_iterator
+      iter = transport_security_state_->GetHSTSEntries().begin();

[Ryan Sleevi, 2013/04/29 22:06:14]

So I understand why you felt the format should change, but I don't think we want
to just drop serializing HPKP.

[unsafe, 2013/04/30 10:42:57]

I can follow-up this CL with HPKP serialization.

But the existing JSON format doesn't work (which "include_subdomains" and
"created" should I write for a domain where both HSTS and HPKP are set?)

Since the design decisions and code around the HPKP json can be separated out
from this (already large) CL, I'd rather do that.



 

+  for (; iter != transport_security_state_->GetHSTSEntries().end(); ++iter) {
+    const std::string& hashed_host = iter->first;
+    const TransportSecurityState::DynamicEntry& entry = iter->second;
 
     DictionaryValue* serialized = new DictionaryValue;
     serialized->SetBoolean(kIncludeSubdomains,
-                           domain_state.include_subdomains);
-    serialized->SetDouble(kCreated, domain_state.created.ToDoubleT());
-    serialized->SetDouble(kExpiry, domain_state.upgrade_expiry.ToDoubleT());
-    serialized->SetDouble(kDynamicSPKIHashesExpiry,
-                          domain_state.dynamic_spki_hashes_expiry.ToDoubleT());
-
-    switch (domain_state.upgrade_mode) {
-      case TransportSecurityState::DomainState::MODE_FORCE_HTTPS:
-        serialized->SetString(kMode, kForceHTTPS);
-        break;
-      case TransportSecurityState::DomainState::MODE_DEFAULT:
-        serialized->SetString(kMode, kDefault);
-        break;
-      default:
-        NOTREACHED() << "DomainState with unknown mode";
-        delete serialized;
-        continue;
-    }
-
-    serialized->Set(kStaticSPKIHashes,
-                    SPKIHashesToListValue(domain_state.static_spki_hashes));
-
-    if (now < domain_state.dynamic_spki_hashes_expiry) {
-      serialized->Set(kDynamicSPKIHashes,
-                      SPKIHashesToListValue(domain_state.dynamic_spki_hashes));
-    }
-
-    toplevel.Set(HashedDomainToExternalString(hostname), serialized);
+                           entry.include_subdomains_);
+    serialized->SetDouble(kCreated, entry.created_.ToDoubleT());
+    serialized->SetDouble(kExpiry, entry.expiry_.ToDoubleT());
+    serialized->SetString(kMode, kForceHTTPS);
+    toplevel.Set(HashedDomainToExternalString(hashed_host), serialized);
   }
 
   base::JSONWriter::WriteWithOptions(&toplevel,
                                      base::JSONWriter::OPTIONS_PRETTY_PRINT,
                                      output);
   return true;
 }
 
-bool TransportSecurityPersister::DeserializeFromCommandLine(
-    const std::string& serialized) {
-  // Purposefully ignore |dirty| because we do not want to persist entries
-  // deserialized in this way.
-  bool dirty;
-  return Deserialize(serialized, true, &dirty, transport_security_state_);
-}
-
 bool TransportSecurityPersister::LoadEntries(const std::string& serialized,
                                              bool* dirty) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   transport_security_state_->ClearDynamicData();
   return Deserialize(serialized, false, dirty, transport_security_state_);
 }
 
 // static
 bool TransportSecurityPersister::Deserialize(const std::string& serialized,
                                              bool forced,
                                              bool* dirty,
                                              TransportSecurityState* state) {
   scoped_ptr<Value> value(base::JSONReader::Read(serialized));
   DictionaryValue* dict_value;
   if (!value.get() || !value->GetAsDictionary(&dict_value))
     return false;
 
   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)) {
       LOG(WARNING) << "Could not parse entry " << *i << "; skipping entry";
       continue;
     }
 
+    bool include_subdomains = false;
+    double created_double = 0, expiry_double = 0;

[Ryan Sleevi, 2013/04/29 22:06:14]

I think it was better without the default initialization, because it would cause
a compiler error if we attempted to use a value that had not been initialized
yet (eg: via lines 217-224)

[unsafe, 2013/04/30 10:42:57]

Done.

+    base::Time created, expiry;
     std::string mode_string;
-    double created;
-    double expiry;
-    double dynamic_spki_hashes_expiry = 0.0;
-    TransportSecurityState::DomainState domain_state;
 
     if (!parsed->GetBoolean(kIncludeSubdomains,
-                            &domain_state.include_subdomains) ||
+                            &include_subdomains) ||
         !parsed->GetString(kMode, &mode_string) ||
-        !parsed->GetDouble(kExpiry, &expiry)) {
+        !parsed->GetDouble(kExpiry, &expiry_double)) {
       LOG(WARNING) << "Could not parse some elements of entry " << *i
                    << "; skipping entry";
       continue;
     }
 
-    // Don't fail if this key is not present.
-    parsed->GetDouble(kDynamicSPKIHashesExpiry,
-                      &dynamic_spki_hashes_expiry);
-
-    ListValue* pins_list = NULL;
-    // preloaded_spki_hashes is a legacy synonym for static_spki_hashes.
-    if (parsed->GetList(kStaticSPKIHashes, &pins_list))
-      SPKIHashesFromListValue(*pins_list, &domain_state.static_spki_hashes);
-    else if (parsed->GetList(kPreloadedSPKIHashes, &pins_list))
-      SPKIHashesFromListValue(*pins_list, &domain_state.static_spki_hashes);
-
-    if (parsed->GetList(kDynamicSPKIHashes, &pins_list))
-      SPKIHashesFromListValue(*pins_list, &domain_state.dynamic_spki_hashes);
-
-    if (mode_string == kForceHTTPS || mode_string == kStrict) {
-      domain_state.upgrade_mode =
-          TransportSecurityState::DomainState::MODE_FORCE_HTTPS;
-    } else if (mode_string == kDefault || mode_string == kPinningOnly) {
-      domain_state.upgrade_mode =
-          TransportSecurityState::DomainState::MODE_DEFAULT;
-    } else {
-      LOG(WARNING) << "Unknown TransportSecurityState mode string "
-                   << mode_string << " found for entry " << *i
-                   << "; skipping entry";
-      continue;
-    }
-
-    domain_state.upgrade_expiry = base::Time::FromDoubleT(expiry);
-    domain_state.dynamic_spki_hashes_expiry =
-        base::Time::FromDoubleT(dynamic_spki_hashes_expiry);
-    if (parsed->GetDouble(kCreated, &created)) {
-      domain_state.created = base::Time::FromDoubleT(created);
+    if (parsed->GetDouble(kCreated, &created_double)) {
+      created = base::Time::FromDoubleT(created_double);
     } 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;
-      domain_state.created = base::Time::Now();
+      created = base::Time::Now();
     }
 
-    if (domain_state.upgrade_expiry <= current_time &&
-        domain_state.dynamic_spki_hashes_expiry <= current_time) {
-      // Make sure we dirty the state if we drop an entry.
-      dirtied = true;
-      continue;
+    if (mode_string == kForceHTTPS || mode_string == kStrict) {
+      std::string hashed_host = ExternalStringToHashedDomain(*i);
+      base::Time expiry = base::Time::FromDoubleT(expiry_double);
+      if (expiry > current_time)
+        state->AddHSTSHashedHost(hashed_host, created, expiry,
+                                 include_subdomains);
+      else
+        dirtied = true;

[Ryan Sleevi, 2013/04/29 22:06:14]

style: You should have braces around this if/else clause

comment: You dropped some very important comments that helped explain this code,
and it took me a while to understand how this code mapped onto the old. 

In some ways, the old flow was easier to read, because it reduced the nesting of
conditions.

BUG? You're explicitly ignoring HPKP, but you're not setting dirtied. Curious
the rationale for this.

Is my understanding of this correct:

if (mode_string != kForceHTTPS && mode_string != kStrict) {
  // Serialized HPKP information. Skip (and don't mark dirty).
  continue;

  // BUG?: Note the old code logged for unhandled TSS mode
  // strings, but here you no longer do.
}

if (expiry <= current_time) {
  // When dropping an entry due to expiration, it's still necessary
  // to mark the state as dirtied.
  dirtied = true;
  continue;
}

state->AddHSTSHashedHost(...)

[unsafe, 2013/04/30 10:42:57]

Done.
Did I?  I think I only dropped 1 relevant line: " Make sure we dirty the state
if we drop an entry".  Which seemed redundant with code.
I've added more comments to describe the dirtied flag.
Is the "dirtied" flag important for anything besides "created"? 

Dirtying the state and rewriting the JSON due to expired entries, or HPKP
entries, or unparseable entries, doesn't seem super useful to me.  The old code
was dirtying for expired but not unparseable entries, was there some reason for
that?

Anyways, I changed it to be consistent and dirty the state on anything that
causes the loaded state to differ from JSON (expired entries, HPKP,
unparseable).
Changed to mark dirty, in this case.
Could add back if it matters, figured it was just a debugging aid.

     }
-
-    std::string hashed = ExternalStringToHashedDomain(*i);
-    if (hashed.empty()) {
-      dirtied = true;
-      continue;
-    }
-
-    if (forced)
-      state->AddOrUpdateForcedHosts(hashed, domain_state);
-    else
-      state->AddOrUpdateEnabledHosts(hashed, domain_state);
   }
 
   *dirty = dirtied;
   return true;
 }
 
 void TransportSecurityPersister::CompleteLoad(const std::string& state) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::IO));
 
   bool dirty = false;
   if (!LoadEntries(state, &dirty)) {
     LOG(ERROR) << "Failed to deserialize state: " << state;
     return;
   }
   if (dirty)
     StateIsDirty(transport_security_state_);
 }