Refactor TransportSecurityState.

net/base/transport_security_state.h

Index: net/base/transport_security_state.h
===================================================================
--- net/base/transport_security_state.h	(revision 126245)
+++ net/base/transport_security_state.h	(working copy)
@@ -22,107 +22,146 @@
 
 class SSLInfo;
 
-typedef std::vector<SHA1Fingerprint> FingerprintVector;
+// In the future there will be a generic Fingerprint type, with at least two
+// implementations: SHA1 and SHA256. See http://crbug.com/117914. Until that
+// work is done (in a separate patch) this typedef bridges the gap.
+typedef SHA1Fingerprint Fingerprint;
 
-// TransportSecurityState
+typedef std::vector<Fingerprint> FingerprintVector;
+
+// Tracks which hosts have enabled strict transport security and/or public
+// key pins.
 //
-// Tracks which hosts have enabled *-Transport-Security. This object manages
-// the in-memory store. A separate object must register itself with this object
-// in order to persist the state to disk.
+// 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/draft-ietf-websec-strict-transport-sec-04, and
+// HTTP-based dynamic public key pinning (HPKP) is defined in
+// http://tools.ietf.org/html/draft-ietf-websec-key-pinning-01.
 class NET_EXPORT TransportSecurityState
     : NON_EXPORTED_BASE(public base::NonThreadSafe) {
  public:
-  // If non-empty, |hsts_hosts| is a JSON-formatted string to treat as if it
-  // were a built-in entry (same format as persisted metadata in the
-  // TransportSecurityState file).
-  explicit TransportSecurityState(const std::string& hsts_hosts);
+  // The caller owns |delegate|.
+  TransportSecurityState(TransportSecurityState::Delegate* delegate)

[agl, 2012/03/20 22:12:59]

explicit

[palmer, 2012/03/22 16:39:00]

Done.

+      : delegate_(delegate);

[Ryan Sleevi, 2012/03/15 03:51:15]

This belongs in the .cc file.

[palmer, 2012/03/19 23:37:52]

Done.

+
   ~TransportSecurityState();
 
-  // A DomainState is the information that we persist about a given domain.
-  struct NET_EXPORT DomainState {
-    enum Mode {
-      // Strict mode implies:
-      //   * We generate internal redirects from HTTP -> HTTPS.
-      //   * Certificate issues are fatal.
-      MODE_STRICT = 0,
-      // This used to be opportunistic HTTPS, but we removed support.
-      MODE_OPPORTUNISTIC_REMOVED = 1,
-      // SPDY_ONLY (aka X-Bodge-Transport-Security) is a hopefully temporary
-      // measure. It implies:
-      //   * We'll request HTTP URLs over HTTPS iff we have SPDY support.
-      //   * Certificate issues are fatal.
-      MODE_SPDY_ONLY = 2,
-      // Pinning means there are no HTTP -> HTTPS redirects, however certificate
-      // issues are still fatal and there may be public key pins.
-      MODE_PINNING_ONLY = 3,
+  // A DomainState describes the transport security state (required upgrade
+  // to HTTPS, and/or any public key pins).
+  class NET_EXPORT DomainState {

[Ryan Sleevi, 2012/03/15 03:51:15]

Are you sure you need to NET_EXPORT this? My understanding was that the child
types would automatically be included by way of their parents export.

[palmer, 2012/03/19 23:37:52]

Done.

+    enum UpgradeMode {

[Ryan Sleevi, 2012/03/15 03:51:15]

nit: Missing public

 public:
  enum UpgradeMode {

[palmer, 2012/03/19 23:37:52]

Done.

+      MODE_DEFAULT,
+      MODE_FORCE_HTTPS,
     };
 
     DomainState();
     ~DomainState();
 
+    // Parses |value| as a Public-Key-Pins header. If successful, returns true
+    // and updates the |dynamic_spki_hashes| and |dynamic_spki_hashes_expiry|
+    // fields; otherwise, returns false without updating any fields.
+    bool ParsePinsHeader(const std::string& value, const SSLInfo& ssl_info);
+
+    // Parses |value| as a Strict-Transport-Security header. If successful,
+    // returns true and updates the |upgrade_mode|, |upgrade_expiry| and
+    // |include_subdomains| fields; otherwise, returns false without updating
+    // any fields.
+    bool ParseSTSHeader(const std::string& value);

[Ryan Sleevi, 2012/03/15 03:51:15]

ParsePinsHeader and ParseSTSHeader both seem like they fit elsewhere
(net/http?). 

I'm just thinking that most of our header-related code chills over there -
net/http/http_response_headers.h, net/http/http_auth.h - heck, even
net/http/http_mac_signature.h lives there.

[palmer, 2012/03/19 23:37:52]

Well, the implementations of these two functions legitimately need to noodle
around in the private fields of TSS::DS. To me, that's a sure sign of "should be
a method" instead of "should be a utility function". Allowing net/http/foo to
update TSS::DS would require exposing various getters and setters, which would
increase the size of this interface. I don't currently think that's worth it,
since one of my goals here is to keep this interface small-ish. Perhaps that
will prove impossible...

+
     // Takes a set of SubjectPublicKeyInfo |hashes| and returns true if:
-    //   1) |bad_preloaded_spki_hashes| does not intersect |hashes|; AND
-    //   2) Both |preloaded_spki_hashes| and |dynamic_spki_hashes| are empty
+    //   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|.
     //
-    // |{dynamic,preloaded}_spki_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 tradeoff selected. (Pinning only to leaf certifiates increases
+    // |{dynamic,static}_spki_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
+    // tradeoff selected. (Pinning only to leaf certifiates increases
     // security because you no longer trust any CAs, but it hampers disaster
     // recovery because you can't just get a new certificate signed by the
     // CA.)
     //
-    // |bad_preloaded_spki_hashes| contains public keys that we don't want to
+    // |bad_static_spki_hashes| contains public keys that we don't want to
     // trust.
-    bool IsChainOfPublicKeysPermitted(const FingerprintVector& hashes);
+    bool IsChainOfPublicKeysPermitted(const FingerprintVector& hashes) const;
 
-    // Returns true if |this| describes a more strict policy than |other|.
-    // Used to see if a dynamic DomainState should override a preloaded one.
-    bool IsMoreStrict(const DomainState& other);
+    // Returns true if any of the FingerprintVectors |static_spki_hashes|,
+    // |bad_static_spki_hashes|, or |dynamic_spki_hashes| contains any
+    // items.
+    bool HasPins() const;
 
     // ShouldRedirectHTTPToHTTPS returns true iff, given the |mode| of this
     // DomainState, HTTP requests should be internally redirected to HTTPS.
     bool ShouldRedirectHTTPToHTTPS() const;
 
-    Mode mode;
-    base::Time created;  // when this host entry was first created
-    base::Time expiry;  // the absolute time (UTC) when this record expires
-    bool include_subdomains;  // subdomains included?
+    UpgradeMode upgrade_mode;

[Ryan Sleevi, 2012/03/15 03:51:15]

DomainState is meant to be immutable after creation, right? Should these all be
private: instead?

I'm guessing 'no' if you want to move the deserialization into another class -
but something to consider in terms of general API exposure.

[palmer, 2012/03/19 23:37:52]

Making them private would require at least getters and possibly setters;
furthermore, DomainState is not necessarily immutable after creation. Calling
GetDomainState, updating it, and then calling EnableHost is perfectly legit.

I suppose you could therefore argue that I should now go full-on with the
accessors...

 
-    // Optional; hashes of preloaded "pinned" SubjectPublicKeyInfos. Unless
-    // both are empty, at least one of |preloaded_spki_hashes| and
+    // The absolute time (UTC) when this DomainState was first created.
+    //
+    // Static entries do not have a created time.
+    base::Time created;
+
+    // The absolute time (UTC) when the |upgrade_mode|, if set to
+    // UPGRADE_ALWAYS, downgrades to UPGRADE_NEVER.
+    base::Time upgrade_expiry;
+
+    // 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;
+
+    // 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 |preloaded_spki_hashes|.
-    // That is, when performing pin validation, first check dynamic and then
-    // check preloaded.
-    FingerprintVector preloaded_spki_hashes;
+    // |dynamic_spki_hashes| take precedence over |static_spki_hashes|.
+    // That is, |IsChainOfPublicKeysPermitted| first checks dynamic pins and
+    // then checks static pins.
+    FingerprintVector static_spki_hashes;
 
-    // Optional; hashes of dynamically pinned SubjectPublicKeyInfos. (They
-    // could be set e.g. by an HTTP header or by a superfluous certificate.)
+    // Optional; hashes of dynamically pinned SubjectPublicKeyInfos.
     FingerprintVector dynamic_spki_hashes;
 
     // The absolute time (UTC) when the |dynamic_spki_hashes| expire.
     base::Time dynamic_spki_hashes_expiry;
 
-    // The max-age directive of the Public-Key-Pins header as parsed. Do not
-    // persist this; it is only for testing. TODO(palmer): Therefore, get rid
-    // of it and find a better way to test.
-    int max_age;
-
-    // Optional; hashes of preloaded known-bad SubjectPublicKeyInfos which
+    // Optional; hashes of static known-bad SubjectPublicKeyInfos which
     // MUST NOT intersect with the set of SPKIs in the TLS server's
     // certificate chain.
-    FingerprintVector bad_preloaded_spki_hashes;
+    FingerprintVector bad_static_spki_hashes;
 
-    // The following members are not valid when stored in |enabled_hosts_|.
-    bool preloaded;  // is this a preloaded entry?
-    std::string domain;  // the domain which matched
+    // 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;
   };
 
+  class Iterator {
+   public:
+    Iterator(const TransportSecurityState& state)
+        : iterator_(state.enabled_hosts_.begin()),
+          end_(state.enabled_hosts_.end());
+    ~Iterator();
+
+    bool HasNext() const { return iterator_ != end_; }
+    void Advance() { ++iterator_; }
+    const std::pair<std::string, DomainState>& GetDomainState() {
+      return *iterator_;
+    }
+
+   private:
+    std::map<std::string, DomainState>::const_iterator iterator_;

[Ryan Sleevi, 2012/03/15 03:51:15]

Can you cheat with a typedef here (more aptly, put the typedef at line 250, and
use that typedef here)

Since TSS isn't a class, you shouldn't have to worry about 2-pass evaluation,
and can leave the typedef private (which this class can access by virtue of
being a member class)

[palmer, 2012/03/19 23:37:52]

I'm not sure what you mean. TSS is a class; what do I gain with a typedef?

+    std::map<std::string, DomainState>::const_iterator end_;
+  };
+
   class Delegate {
    public:
     // This function may not block and may be called with internal locks held.
@@ -135,142 +174,86 @@
 
   void SetDelegate(Delegate* delegate);
 
-  // Enable TransportSecurity for |host|.
+  // 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);
 
-  // Delete any entry for |host|. If |host| doesn't have an exact entry then no
-  // action is taken. Returns true iff an entry was deleted.
+  // Delete any entry for |host|. If |host| doesn't have an exact entry then
+  // no action is taken. Does not delete static entries. Returns true iff an
+  // entry was deleted.
+  //
+  // The new state for |host| is persisted using the Delegate (if any).
   bool DeleteHost(const std::string& host);
 
-  // Returns true if |host| has TransportSecurity enabled. Before operating
-  // on this result, consult |result->mode|, as the expected behavior of
-  // TransportSecurity can significantly differ based on mode.
+  // Deletes all records created since a given time.
+  void DeleteSince(const base::Time& time);
+
+  // Returns true and updates |*result| iff there is a DomainState for
+  // |host|.
   //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
+  // If |sni_enabled| is true, searches the static pins defined for
+  // SNI-using hosts as well as the rest of the pins.
   //
-  // Note that |*result| is always overwritten on every call.
-  // TODO(palmer): Only update |*result| on success.
-  bool GetDomainState(DomainState* result,
-                      const std::string& host,
-                      bool sni_available);
+  // If |host| matches both an exact entry and is a subdomain of another
+  // entry, the exact match determines the return value.
+  bool GetDomainState(const std::string& host,
+                      bool sni_enabled,
+                      DomainState* result) const;
 
-  // Returns true if there are any certificates pinned for |host|.
-  // If so, updates the |preloaded_spki_hashes|, |dynamic_spki_hashes|, and
-  // |bad_preloaded_spki_hashes| fields of |*result| with the pins.
+  // Returns true and updates |*result| iff there is a static DomainState for
+  // |host|.
   //
-  // Note that |*result| is always overwritten on every call, regardless of
-  // whether or not pins are enabled.
+  // |GetStaticDomainState| is identical to |GetDomainState| except that it
+  // searches only the statically-defined transport security state, ignoring
+  // all dynamically-added DomainStates.
   //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
+  // If |sni_enabled| is true, searches the static pins defined for
+  // SNI-using hosts as well as the rest of the pins.
   //
-  // TODO(palmer): Only update |*result| if pins exist.
-  bool HasPinsForHost(DomainState* result,
-                      const std::string& host,
-                      bool sni_available);
+  // If |host| matches both an exact entry and is a subdomain of another
+  // entry, the exact match determines the return value.
+  bool GetStaticDomainState(const std::string& host,
+                            bool sni_enabled,
+                            DomainState* result) const;
 
-  // Returns true and updates |*result| if there is any |DomainState|
-  // metadata for |host| in the local TransportSecurityState database;
-  // returns false otherwise. TODO(palmer): Unlike the other
-  // TransportSecurityState lookup functions in this class (e.g
-  // |HasPinsForHost|, |GetDomainState|), |*result| is updated iff metadata
-  // is found. The other functions are buggy and will be fixed to behave
-  // like this one.
-  //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
-  bool HasMetadata(DomainState* result,
-                   const std::string& host,
-                   bool sni_available);
-
-  // Returns true if we have a preloaded certificate pin for the |host| and if
-  // its set of required certificates is the set we expect for Google
+  // 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.
   //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
+  // If |sni_enabled| is true, searches the static pins defined for
+  // SNI-using hosts as well as the rest of the pins.
   //
-  // Note that like HasMetadata, if |host| matches both an exact entry and is a
-  // subdomain of another entry, the exact match determines the return value.
+  // 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_available);
+                                     bool sni_enabled);
 
-  // Reports UMA statistics upon public key pin failure. Reports only down to
-  // the second-level domain of |host| (e.g. google.com if |host| is
-  // mail.google.com), and only if |host| is a preloaded STS host.
-  static void ReportUMAOnPinFailure(const std::string& host);
+  // Decodes a pin string |value| (e.g. "sha1/hvfkN/qlp/zhXR3cuerq6jd2Z7g=").
+  // If parsing succeeded, updates |*out| and returns true; otherwise returns
+  // false without updating |*out|.
+  static bool ParsePin(const std::string& value, Fingerprint* out);

[Ryan Sleevi, 2012/03/15 03:51:15]

See above comments about HTTP header parsing above.

 
-  // Parses |cert|'s Subject Public Key Info structure, hashes it, and writes
-  // the hash into |spki_hash|. Returns true on parse success, false on
-  // failure.
-  static bool GetPublicKeyHash(const X509Certificate& cert,
-                               SHA1Fingerprint* spki_hash);
-
-  // Decodes a pin string |value| (e.g. "sha1/hvfkN/qlp/zhXR3cuerq6jd2Z7g=")
-  // and populates |out|.
-  static bool ParsePin(const std::string& value, SHA1Fingerprint* out);
-
-  // Deletes all records created since a given time.
-  void DeleteSince(const base::Time& time);
-
-  // Parses |value| as a Public-Key-Pins header. If successful, returns |true|
-  // and updates the |dynamic_spki_hashes| and |dynamic_spki_hashes_expiry|
-  // fields of |*state|; otherwise, returns |false| without updating |*state|.
-  static bool ParsePinsHeader(const std::string& value,
-                              const SSLInfo& ssl_info,
-                              DomainState* state);
-
-  // Returns |true| if |value| parses as a valid *-Transport-Security
-  // header value.  The values of max-age and and includeSubDomains are
-  // returned in |max_age| and |include_subdomains|, respectively.  The out
-  // parameters are not modified if the function returns |false|.
-  static bool ParseHeader(const std::string& value,
-                          int* max_age,
-                          bool* include_subdomains);
-
-  // ParseSidePin attempts to parse a side pin from |side_info| which signs the
-  // SubjectPublicKeyInfo in |leaf_spki|. A side pin is a way for a site to
-  // sign their public key with a key that is offline but still controlled by
-  // them. If successful, the hash of the public key used to sign |leaf_spki|
-  // is put into |out_pub_key_hash|.
-  static bool ParseSidePin(const base::StringPiece& leaf_spki,
-                           const base::StringPiece& side_info,
-                           FingerprintVector* out_pub_key_hash);
-
-  bool Serialise(std::string* output);
-  // Existing non-preloaded entries are cleared and repopulated from the
-  // passed JSON string.
-  bool LoadEntries(const std::string& state, bool* dirty);
-
   // The maximum number of seconds for which we'll cache an HSTS request.
   static const long int kMaxHSTSAgeSecs;
 
- private:
-  FRIEND_TEST_ALL_PREFIXES(TransportSecurityStateTest, IsPreloaded);
+  // 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);
 
-  // If we have a callback configured, call it to let our serialiser know that
-  // our state is dirty.
+ private:
+  // If we have a Delegate, call its |StateIsDirty| function.
   void DirtyNotify();
-  bool IsPreloadedSTS(const std::string& canonicalized_host,
-                      bool sni_available,
-                      DomainState* out);
 
-  static std::string CanonicalizeHost(const std::string& host);
-  static bool Deserialise(const std::string& state,
-                          bool* dirty,
-                          std::map<std::string, DomainState>* out);
-
-  // The set of hosts that have enabled TransportSecurity. The keys here
-  // are SHA256(DNSForm(domain)) where DNSForm converts from dotted form
-  // ('www.google.com') to the form used in DNS: "\x03www\x06google\x03com"
+  // The set of hosts that have enabled TransportSecurity.
   std::map<std::string, DomainState> enabled_hosts_;
 
-  // These hosts are extra rules to treat as built-in, passed in the
-  // constructor (typically originating from the command line).
+  // Extra entries, provided by the user at run-time, to treat as if they
+  // were static.
   std::map<std::string, DomainState> forced_hosts_;
 
-  // Our delegate who gets notified when we are dirtied, or NULL.
   Delegate* delegate_;
 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);