Improve TransportSecurityState data storage.

net/base/transport_security_state.h

Index: net/base/transport_security_state.h
===================================================================
--- net/base/transport_security_state.h	(revision 186438)
+++ net/base/transport_security_state.h	(working copy)
@@ -28,45 +28,42 @@
 // This object manages the in-memory store. Register a Delegate with
 // |SetDelegate| to persist the state to disk.
 //
-// HTTP strict transport security (HSTS) is defined in
-// http://tools.ietf.org/html/ietf-websec-strict-transport-sec, and
+// HTTP strict transport security (HSTS) is defined in RFC 6797, and
 // HTTP-based dynamic public key pinning (HPKP) is defined in
 // http://tools.ietf.org/html/ietf-websec-key-pinning.
 class NET_EXPORT TransportSecurityState
     : NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
-  class Delegate {
-   public:
-    // This function may not block and may be called with internal locks held.
-    // Thus it must not reenter the TransportSecurityState object.
-    virtual void StateIsDirty(TransportSecurityState* state) = 0;
 
-   protected:
-    virtual ~Delegate() {}
-  };
-
-  TransportSecurityState();
-  ~TransportSecurityState();
-
-  // A DomainState describes the transport security state (required upgrade
-  // to HTTPS, and/or any public key pins).
+  // A DomainState describes the entire set of applicable transport security
+  // state for a domain at a particular point in time (required upgrade to
+  // HTTPS, public key pins, etc).  DomainStates are not stored directly
+  // but are calculated by searching through the dynamic and preload entries
+  // and merging all relevant and nonexpired data into the DomainState.
   class NET_EXPORT DomainState {
    public:
-    enum UpgradeMode {
-      // These numbers must match those in hsts_view.js, function modeToString.
-      MODE_FORCE_HTTPS = 0,
-      MODE_DEFAULT = 1,
-    };
-
     DomainState();
     ~DomainState();
 
+    // ShouldUpgradeToSSL returns true iff HTTP requests should be internally
+    // redirected to HTTPS (also if the "ws" WebSocket request should be
+    // upgraded to "wss").
+    bool ShouldUpgradeToSSL() const;
+
+    // ShouldSSLErrorsBeFatal returns true iff HTTPS errors should cause
+    // hard-fail behavior (e.g. if HSTS is set for the domain)
+    bool ShouldSSLErrorsBeFatal() const;
+
+    // Returns true if CheckPublicKeyPins() should be called to verify
+    // SSL/TLS connections.
+    bool HasPublicKeyPins() const;
+
     // Takes a set of SubjectPublicKeyInfo |hashes| and returns true if:
-    //   1) |bad_static_spki_hashes| does not intersect |hashes|; AND
-    //   2) Both |static_spki_hashes| and |dynamic_spki_hashes| are empty
-    //      or at least one of them intersects |hashes|.
+    //   1) |public_key_pins_bad_hashes_| does not intersect |hashes|; AND
+    //   2) |public_key_pins_good_hashes_| is either empty or intersects
+    //      |hashes|.
     //
-    // |{dynamic,static}_spki_hashes| contain trustworthy public key hashes,
+    // |public_key_pins_good_hashes_| contain trustworthy public key hashes,
     // any one of which is sufficient to validate the certificate chain in
     // question. The public keys could be of a root CA, intermediate CA, or
     // leaf certificate, depending on the security vs. disaster recovery
@@ -75,88 +72,72 @@
     // recovery because you can't just get a new certificate signed by the
     // CA.)
     //
-    // |bad_static_spki_hashes| contains public keys that we don't want to
-    // trust.
+    // |public_key_pins_bad_hashes_| contains public keys that we don't want
+    // to trust.
     bool CheckPublicKeyPins(const HashValueVector& hashes) const;
 
-    // Returns true if any of the HashValueVectors |static_spki_hashes|,
-    // |bad_static_spki_hashes|, or |dynamic_spki_hashes| contains any
-    // items.
-    bool HasPublicKeyPins() const;
+    // Returns true iff we have any preloaded public key pins for the domain
+    // and iff its set of required pins is the set we expect for Google
+    // properties.
+    bool IsGooglePinnedProperty() const;
 
-    // ShouldUpgradeToSSL returns true iff, given the |mode| of this
-    // DomainState, HTTP requests should be internally redirected to HTTPS
-    // (also if the "ws" WebSocket request should be upgraded to "wss")
-    bool ShouldUpgradeToSSL() const;
+    // Send an UMA report on pin validation failure, if the host is in a
+    // statically-defined list of domains.
+    void ReportUMAOnPinFailure() const;
 
-    // ShouldSSLErrorsBeFatal returns true iff HTTPS errors should cause
-    // hard-fail behavior (e.g. if HSTS is set for the domain)
-    bool ShouldSSLErrorsBeFatal() const;
+    // Provide direct read-only access for net-internals
+    const HashValueVector& GetPublicKeyPinsGoodHashes() const;
+    const HashValueVector& GetPublicKeyPinsBadHashes() const;
 
-    bool Equals(const DomainState& other) const;
+   private:
+    friend TransportSecurityState;
 
-    UpgradeMode upgrade_mode;
+    bool should_upgrade_;
+    bool has_public_key_pins_;
+    bool is_google_pinned_property_;
+    bool report_uma_on_pin_failure_;
 
-    // The absolute time (UTC) when this DomainState was first created.
-    //
-    // Static entries do not have a created time.
-    base::Time created;
+    HashValueVector public_key_pins_good_hashes_;  // if has_public_key_pins_
+    HashValueVector public_key_pins_bad_hashes_;   // if has_public_key_pins_
 
-    // The absolute time (UTC) when the |upgrade_mode|, if set to
-    // UPGRADE_ALWAYS, downgrades to UPGRADE_NEVER.
-    base::Time upgrade_expiry;
+    size_t second_level_domain_name_;  // if report_uma_on_pin_failure_
+  };
 
-    // Are subdomains subject to this DomainState?
-    //
-    // TODO(palmer): Decide if we should have separate |pin_subdomains| and
-    // |upgrade_subdomains|. Alternately, and perhaps better, is to separate
-    // DomainState into UpgradeState and PinState (requiring also changing the
-    // serialization format?).
-    bool include_subdomains;
+  // DynamicEntry stores the HSTS data for a single domain.
+  struct DynamicEntry {
+    DynamicEntry();
+    ~DynamicEntry();
+    DynamicEntry(bool include_subdomains, const base::Time& created,
+                 const base::Time& expiry);
 
-    // Optional; hashes of static pinned SubjectPublicKeyInfos. Unless both
-    // are empty, at least one of |static_spki_hashes| and
-    // |dynamic_spki_hashes| MUST intersect with the set of SPKIs in the TLS
-    // server's certificate chain.
-    //
-    // |dynamic_spki_hashes| take precedence over |static_spki_hashes|.
-    // That is, |IsChainOfPublicKeysPermitted| first checks dynamic pins and
-    // then checks static pins.
-    HashValueVector static_spki_hashes;
+    bool include_subdomains_;
+    base::Time created_;
+    base::Time expiry_;
+  };
 
-    // Optional; hashes of dynamically pinned SubjectPublicKeyInfos.
-    HashValueVector dynamic_spki_hashes;
-
-    // The absolute time (UTC) when the |dynamic_spki_hashes| expire.
-    base::Time dynamic_spki_hashes_expiry;
-
-    // Optional; hashes of static known-bad SubjectPublicKeyInfos which
-    // MUST NOT intersect with the set of SPKIs in the TLS server's
-    // certificate chain.
-    HashValueVector bad_static_spki_hashes;
-
-    // The following members are not valid when stored in |enabled_hosts_|:
-
-    // The domain which matched during a search for this DomainState entry.
-    // Updated by |GetDomainState| and |GetStaticDomainState|.
-    std::string domain;
+  // HPKPEntry stores the HPKP data for a single domain.
+  struct HPKPEntry : public DynamicEntry {
+    HPKPEntry();
+    ~HPKPEntry();
+    HPKPEntry(bool include_subdomains, const base::Time& created,
+              const base::Time& expiry,
+              const HashValueVector& good_hashes);
+    HashValueVector good_hashes_;
   };
 
-  class NET_EXPORT Iterator {
+  class Delegate {
    public:
-    explicit Iterator(const TransportSecurityState& state);
-    ~Iterator();
+    // This function may not block and may be called with internal locks held.
+    // Thus it must not reenter the TransportSecurityState object.
+    virtual void StateIsDirty(TransportSecurityState* state) = 0;
 
-    bool HasNext() const { return iterator_ != end_; }
-    void Advance() { ++iterator_; }
-    const std::string& hostname() const { return iterator_->first; }
-    const DomainState& domain_state() const { return iterator_->second; }
-
-   private:
-    std::map<std::string, DomainState>::const_iterator iterator_;
-    std::map<std::string, DomainState>::const_iterator end_;
+   protected:
+    virtual ~Delegate() {}
   };
 
+  TransportSecurityState();
+  ~TransportSecurityState();
+
   // Assign a |Delegate| for persisting the transport security state. If
   // |NULL|, state will not be persisted. The caller retains
   // ownership of |delegate|.
@@ -172,154 +153,203 @@
   // TransportSecurityState.
   void ClearDynamicData();
 
-  // Inserts |state| into |enabled_hosts_| under the key |hashed_host|.
-  // |hashed_host| is already in the internal representation
-  // HashHost(CanonicalizeHost(host)).
-  // Note: This is only used for serializing/deserializing the
-  // TransportSecurityState.
-  void AddOrUpdateEnabledHosts(const std::string& hashed_host,
-                               const DomainState& state);
-
-  // Inserts |state| into |forced_hosts_| under the key |hashed_host|.
-  // |hashed_host| is already in the internal representation
-  // HashHost(CanonicalizeHost(host)).
-  // Note: This is only used for serializing/deserializing the
-  // TransportSecurityState.
-  void AddOrUpdateForcedHosts(const std::string& hashed_host,
-                              const DomainState& state);
-
-  // Deletes all dynamic data (e.g. HSTS or HPKP data) created since a given
-  // time.
+  // Returns true and updates |*result| iff there is DomainState for |host|.
   //
-  // If any entries are deleted, the new state will be persisted through
-  // the Delegate (if any).
-  void DeleteAllDynamicDataSince(const base::Time& time);
-
-  // Deletes any dynamic data stored for |host| (e.g. HSTS or HPKP data).
-  // If |host| doesn't have an exact entry then no action is taken. Does
-  // not delete static (i.e. preloaded) data.  Returns true iff an entry
-  // was deleted.
-  //
-  // If an entry is deleted, the new state will be persisted through
-  // the Delegate (if any).
-  bool DeleteDynamicDataForHost(const std::string& host);
-
-  // Returns true and updates |*result| iff there is a DomainState for
-  // |host|.
-  //
-  // If |sni_enabled| is true, searches the static pins defined for
-  // SNI-using hosts as well as the rest of the pins.
-  //
-  // If |host| matches both an exact entry and is a subdomain of another
-  // entry, the exact match determines the return value.
-  //
-  // Note that this method is not const because it opportunistically removes
-  // entries that have expired.
+  // If |sni_enabled| is true, searches the preload data defined for
+  // SNI-only hosts as well as the other preload data.
   bool GetDomainState(const std::string& host,
                       bool sni_enabled,
-                      DomainState* result);
+                      DomainState* result) const;
 
-  // Processes an HSTS header value from the host, adding entries to
-  // dynamic state if necessary.
-  bool AddHSTSHeader(const std::string& host, const std::string& value);
+  // Processes an HSTS header value from the host, adds/deletes entries
+  // in dynamic state if necessary.
+  void AddHSTSHeader(const std::string& host, const std::string& value);
 
-  // Processes an HPKP header value from the host, adding entries to
-  // dynamic state if necessary.  ssl_info is used to check that
+  // Processes an HPKP header value from the host, adds/deletes entries
+  // in dynamic state if necessary.  ssl_info is used to check that
   // the specified pins overlap with the certificate chain.
-  bool AddHPKPHeader(const std::string& host, const std::string& value,
+  void AddHPKPHeader(const std::string& host, const std::string& value,
                      const SSLInfo& ssl_info);
 
   // Adds explicitly-specified data as if it was processed from an
-  // HSTS header (used for net-internals and unit tests).
-  bool AddHSTS(const std::string& host, const base::Time& expiry,
-               bool include_subdomains);
+  // HSTS/HPKP header (used for net-internals and unit tests).
+  // Returns true iff an entry was succesfully added, false if 
+  // expiry < now or created > now.
+  bool AddHSTS(const std::string& host, const base::Time& created,
+               const base::Time& expiry, bool include_subdomains);
+  bool AddHPKP(const std::string& host, const base::Time& created,
+               const base::Time& expiry, bool include_subdomains,
+               const HashValueVector& hashes);
 
-  // Adds explicitly-specified data as if it was processed from an
-  // HPKP header (used for net-internals and unit tests).
-  bool AddHPKP(const std::string& host, const base::Time& expiry,
-               bool include_subdomains, const HashValueVector& hashes);
+  // As AddHSTS()/AddHPKP() but uses the internal "hashed" representation of
+  // a hostname.  This is used internally by AddHSTS()/AddHPKP() and is also
+  // used for deserializing the TransportSecurityState (since the JSON stores
+  // entries in hashed form).
+  bool AddHSTSHashedHost(const std::string& hashed_host,
+                         const base::Time& created, const base::Time& expiry,
+                         bool include_subdomains);
+  bool AddHPKPHashedHost(const std::string& hashed_host,
+                         const base::Time& created, const base::Time& expiry,
+                         bool include_subdomains,
+                         const HashValueVector& hashes);
 
-  // Returns true iff we have any static public key pins for the |host| and
-  // iff its set of required pins is the set we expect for Google
-  // properties.
+  // Deletes HSTS/HPKP data for the specified |host|.
+  // Returns true iff an entry was succesfully deleted.
+  bool DeleteHSTS(const std::string& host);
+  bool DeleteHPKP(const std::string& host);
+
+  // Deletes any dynamic data stored for |host| (e.g. HSTS or HPKP data).
+  // If |host| doesn't have an exact entry then no action is taken. Does
+  // not delete preload data.  Returns true iff an entry
+  // was deleted.
   //
-  // If |sni_enabled| is true, searches the static pins defined for
-  // SNI-using hosts as well as the rest of the pins.
+  // If an entry is deleted, the new state will be persisted through
+  // the Delegate (if any).
+  bool DeleteDynamicDataForHost(const std::string& host);
+
+  // Deletes all dynamic data (e.g. HSTS or HPKP data) created since a given
+  // time.
   //
-  // If |host| matches both an exact entry and is a subdomain of another
-  // entry, the exact match determines the return value.
-  static bool IsGooglePinnedProperty(const std::string& host,
-                                     bool sni_enabled);
+  // If any entries are deleted, the new state will be persisted through
+  // the Delegate (if any).
+  void DeleteAllDynamicDataSince(const base::Time& time);
 
-  // The maximum number of seconds for which we'll cache an HSTS request.
-  static const long int kMaxHSTSAgeSecs;
+  // Direct (read-only) access to HSTS and HPKP entries.  Used for
+  // serializing.
+  const std::map<std::string, DynamicEntry>& GetHSTSEntries() const;
+  const std::map<std::string, HPKPEntry>& GetHPKPEntries() const;
 
-  // Send an UMA report on pin validation failure, if the host is in a
-  // statically-defined list of domains.
-  //
-  // TODO(palmer): This doesn't really belong here, and should be moved into
-  // the exactly one call site. This requires unifying |struct HSTSPreload|
-  // (an implementation detail of this class) with a more generic
-  // representation of first-class DomainStates, and exposing the preloads
-  // to the caller with |GetStaticDomainState|.
-  static void ReportUMAOnPinFailure(const std::string& host);
-
   // IsBuildTimely returns true if the current build is new enough ensure that
   // built in security information (i.e. HSTS preloading and pinning
   // information) is timely.
   static bool IsBuildTimely();
 
+  // The maximum number of seconds for which we'll cache an HSTS request.
+  static const long int kMaxHSTSAgeSecs;
+
  private:
   friend class TransportSecurityStateTest;
 
-  typedef std::map<std::string, DomainState> DomainStateMap;
+  // If a Delegate is present and any of the dynamic entry maps have become
+  // dirty, notify the Delegate.
+  void CheckDirty();
 
-  // If a Delegate is present, notify it that the internal state has
-  // changed.
-  void DirtyNotify();
+  // Returns true iff there is any DomainState data in preload entries
+  bool GetPreloadDomainState(bool sni_enabled, const base::Time& now,
+                         const std::string& host, DomainState* result) const;
 
-  // Enable TransportSecurity for |host|. |state| supercedes any previous
-  // state for the |host|, including static entries.
-  //
-  // The new state for |host| is persisted using the Delegate (if any).
-  void EnableHost(const std::string& host, const DomainState& state);
+  // Returns true iff there is any DomainState data in dynamic entries
+  bool GetDynamicDomainState(const base::Time& now, const std::string& host,
+                             DomainState* result) const;
 
-  // Converts |hostname| from dotted form ("www.google.com") to the form
-  // used in DNS: "\x03www\x06google\x03com", lowercases that, and returns
-  // the result.
-  static std::string CanonicalizeHost(const std::string& hostname);
+  template<typename T>
+  class DynamicEntryMap : public std::map<std::string, T> {

[Ryan Sleevi, 2013/03/23 04:24:13]

It's not "legal" C++ to derive from containers within the std:: namespace (or
most types, for that matter - eg: std::string)

One technique would be to define the member functions as template functions
within the implementation file, rather than as a class. Then you pass the
std::map<std::string, DynamicEntry> or std::map<std::string, HPKPEntry> to the
member functions, with a possible out value of bool.

I haven't fully thought through this - this is just initial remark on the
design. I'll see if I can't come up with something cleaner, but did want to
provide the feedback that this seems like a bit of a smell (in leaking out
internal details).

[unsafe, 2013/03/23 08:19:39]

Hmmm (going to stackoverflow, coming back)... so, they don't have virtual
destructors, but its not really illegal, is it?... just sometimes ill-advised?
Sounds good to me, I'll do that if you want.

+   public:
+    DynamicEntryMap<T>();
 
-  // Returns true and updates |*result| iff there is a static DomainState for
-  // |host|.
-  //
-  // |GetStaticDomainState| is identical to |GetDomainState| except that it
-  // searches only the statically-defined transport security state, ignoring
-  // all dynamically-added DomainStates.
-  //
-  // If |sni_enabled| is true, searches the static pins defined for
-  // SNI-using hosts as well as the rest of the pins.
-  //
-  // If |host| matches both an exact entry and is a subdomain of another
-  // entry, the exact match determines the return value.
-  //
-  // Note that this method is not const because it opportunistically removes
-  // entries that have expired.
-  bool GetStaticDomainState(const std::string& host,
-                            bool sni_enabled,
-                            DomainState* result);
+    // True if an entry is returned
+    bool GetEntry(const base::Time& now, const std::string& hashed_host,
+                  bool is_full_hostname, T* result_entry) const;
 
-  // The set of hosts that have enabled TransportSecurity.
-  DomainStateMap enabled_hosts_;
+    // True if an entry is successfully added
+    bool AddEntry(const std::string& hashed_host,
+                  const T& new_entry);
 
-  // Extra entries, provided by the user at run-time, to treat as if they
-  // were static.
-  DomainStateMap forced_hosts_;
+    // True if any entries are deleted
+    bool DeleteEntry(const std::string& hashed_host);
 
+    void DeleteEntriesSince(const base::Time& time);
+
+    // Set by any of the non-const member functions when map is changed
+    bool dirty;
+  };
+
+  DynamicEntryMap<DynamicEntry> hsts_entries_;
+  DynamicEntryMap<HPKPEntry> hpkp_entries_;
+
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);
 };
 
+// DynamicEntryMap template functions
+
+#define DynamicEntryMapConstIter \
+  typename std::map<std::string, T>::const_iterator
+#define DynamicEntryMapIter \
+  typename std::map<std::string, T>::iterator

[Ryan Sleevi, 2013/03/23 04:24:13]

Per
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Prepro...
, don't #define macros in a .h file

[unsafe, 2013/03/23 08:19:39]

Can't typedef this, due to templates.. perhaps a "Be very cautious" exemption?
:)

+
+template <typename T>
+TransportSecurityState::DynamicEntryMap<T>::DynamicEntryMap():dirty(false) {
+}
+
+template <typename T>
+bool TransportSecurityState::DynamicEntryMap<T>::GetEntry(
+  const base::Time& now, const std::string& hashed_host,
+  bool is_full_hostname, T* result_entry) const {
+  // Find the entry, and return if relevant and nonexpired
+  DynamicEntryMapConstIter find_iter = this->find(hashed_host);
+  if (find_iter != this->end()) {
+    const T& find_entry = find_iter->second;
+    if ((is_full_hostname || find_entry.include_subdomains_) &&
+        find_entry.expiry_ > now) {
+      *result_entry = find_entry;
+      return true;
+    }
+  }
+  return false;
+}
+
+template<typename T>
+bool TransportSecurityState::DynamicEntryMap<T>::AddEntry(
+  const std::string& hashed_host, const T& new_entry) {
+  // Don't add entries with invalid/expired time values
+  const base::Time now = base::Time::Now();
+  if (new_entry.created_ > now)
+    return false;
+  if (new_entry.expiry_ < now)
+    return false;
+
+  DynamicEntryMapIter find_iter = this->find(hashed_host);
+  if (find_iter != this->end()) {
+    // Leave 'created' unchanged
+    T& find_entry = find_iter->second;
+    find_entry.expiry_ = new_entry.expiry_;
+    find_entry.include_subdomains_ = new_entry.include_subdomains_;
+    dirty = true;
+  } else {
+    this->operator[](hashed_host) = new_entry;
+    dirty = true;
+  }
+  return true;
+}
+
+template<typename T>
+bool TransportSecurityState::DynamicEntryMap<T>::DeleteEntry(
+  const std::string& hashed_host) {
+  DynamicEntryMapIter find_iter = this->find(hashed_host);
+  if (find_iter != this->end()) {
+    this->erase(find_iter);
+    dirty = true;
+    return true;
+  }
+  return false;
+}
+
+template<typename T>
+void TransportSecurityState::DynamicEntryMap<T>::DeleteEntriesSince(
+  const base::Time& time) {
+  DynamicEntryMapIter iter = this->begin();
+  while (iter != this->end()) {
+    if (iter->second.created_ >= time) {
+      this->erase(iter++);
+      dirty = true;
+    } else {
+      iter++;
+    }
+  }
+}
+
 }  // namespace net
 
 #endif  // NET_BASE_TRANSPORT_SECURITY_STATE_H_