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,41 @@
 // 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();
 
+    // 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;
+
+    // Returns true iff HTTPS errors should cause hard-fail behavior
+    // (e.g. if HSTS is set for the domain).
+    bool ShouldSSLErrorsBeFatal() const;
+
+    // Returns true iff 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 +71,74 @@
     // 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;
+    // Provides direct read-only access for chrome://net-internals.

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

comment nit: Shouldn't be mentioning net-internals
comment nit: Shouldn't be describing how it is/will be used
comment nit: Saying "read only" is a bit superfluous here with the const-ness.

// Returns associated known-good pins for the host. Only
// applicable if HasPublicKeyPins() is true.
// Note: This may be empty if a host only contains blacklisted
// pins.

// Returns associated known-bad pins for a host. Only applicable ... (etc)

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

DomainState already has a high-level CheckPublicKeyPins(), it's ugly to also
expose these internal members.  So I'm explaining why they're here:
net-internals needs to display the underlying data.

That's helpful commentary I'd like to know when reading this.

I'll remove it if you want:  is this a general rule, function comments shouldn't
mention who uses them?

[Ryan Sleevi, 2013/04/30 19:55:09]

Yes.

A function comment should be about how to use it - not how it is used. This is
important for keeping code understandable and reducing churn. That is, there is
absolutely no reason a caller should expect to update a comment if they change
how it's used.

Additionally, comments should reflect the layering of the code. That is, if a
high-level component (like net-internals) depends on a low-level component (like
this), the re shouldn't be any comments going "up" the hierarchy - the same
reason why forward declaring 'higher level' classes is not desired.

The comments should follow the class design - that is, 'knowledge' only flows in
one way - from a higher level component to a lower level component.

+    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;
+    // The following values are only relevant if one of the above
+    // booleans is true:
+    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_

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

Why aren't you using the enum here, and instead casting to size_t? Not
guaranteed to be safe (enums scale to the largest necessary integer type, and no
remarks on signedness)

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

I'd have to pull the enum definition out into a header file (it's currently in a
.h which is included in a .c but is not a proper header file)..

But I guess that's the right thing to do, I'll get to it.

+  };
 
-    // 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 {

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

This inheritance model still creates opportunity for type shearing.

Namely, it's possible to take an HPKPEntry (where all of its fields relate to
HPKP) and treat it as a DynamicEntry (where all of its fields relate to HSTS) -
eg: confusing include_subdomains, created, and expiry.

If you are going to go the struct/inheritance model, then it seems like you need
at least three types

DynamicEntry - the shared state between the two
HSTSEntry
HPKPEntry

Of course, the inheritance just seems to be class short-hand, and it may make
the most sense to just have
HSTSEntry
HPKPEntry

And not conflate the two with inheritance (which is part of how we got to this
problem in the first place)

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

I don't see a problem, but I could derive an HSTSEntry from DynamicEntry if you
think that's clearer.
It is just shorthand, could do that too.

[Ryan Sleevi, 2013/04/30 19:55:09]

I suppose the same problem would exist with the inheritance model - HSTSEntry
would be implicitly converted to a DynamicEntry which would then be used to
initialize an HPKPEntry.

The issue here is that DynamicEntry (implicitly) has the following PUBLIC
operators:

DynamicEntry(const DynamicEntry& other);
DynamicEntry& operator=(const DynamicEntry& other);

As such, that means HPKPEntry has the following public operators:
HPKPEntry(const DynamicEntry& other);
DynamicEntry& operator=(const DynamicEntry& other);

That means you can do something like:

DynamicEntry hsts_data;
hsts_data.include_subdomains = false;
HPKPEntry entry(hsts_data);

will work just fine.

The issue with this is, from the caller's perspective, it's easy to get things
wrong

HPKPEntry i_want_the_hpkp_entry = TSS.ButICallAFunctionReturningAnHSTSEntry();

And there will be no compile errors with that.

That's the general messiness of having these two types share an inheritance
tree. I would suggest breaking it entirely (eg: NO shared base).

There's no real overhead to the code (except in the debug symbols, and COMDAT
folding will handle that), and it will ensure you get meaningful compiler errors
when you conflate HPKP with HSTS.

+    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 {

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

The intent of this class was/is to provide a de-coupling between the internal
state details of how TSS stores its members and how they're
serialized/deserialized.

It's a real shame to lose that, and I'm not entirely sure why this is necessary.

+  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,149 +154,99 @@
   // 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.

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

comment nit: Seems like you should just be saying "true iff an entry was added".

comment nit: drop the "(used for net-internals and unit tests)"

+  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).

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

comment nit: Leaking abstractions about serialization here (eg: talking about
JSON)

+  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);
+  // Only used by template functions within the .cc to signal that
+  // the dynamic entry maps were changed
+  void StateIsDirty();
 
-  // IsBuildTimely returns true if the current build is new enough ensure that
+  // 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;
+  // 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;
 
-  // If a Delegate is present, notify it that the internal state has
-  // changed.
-  void DirtyNotify();
+  // Returns true iff there is any DomainState data in dynamic entries
+  bool GetDynamicDomainState(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);
+  std::map<std::string, DynamicEntry> hsts_entries_;
+  std::map<std::string, HPKPEntry> hpkp_entries_;
 
-  // 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);
-
-  // 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);
-
-  // The set of hosts that have enabled TransportSecurity.
-  DomainStateMap enabled_hosts_;
-
-  // Extra entries, provided by the user at run-time, to treat as if they
-  // were static.
-  DomainStateMap forced_hosts_;
-
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);