Refactor TransportSecurityState.

net/base/transport_security_state.h

 // 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.
 
 #ifndef NET_BASE_TRANSPORT_SECURITY_STATE_H_
 #define NET_BASE_TRANSPORT_SECURITY_STATE_H_
 #pragma once
 
 #include <map>
 #include <string>
 #include <vector>
 
 #include "base/basictypes.h"
 #include "base/gtest_prod_util.h"
 #include "base/threading/non_thread_safe.h"
 #include "base/time.h"
 #include "net/base/net_export.h"
 #include "net/base/x509_certificate.h"
 #include "net/base/x509_cert_types.h"
 
 namespace net {
 
 class SSLInfo;
 
-typedef std::vector<SHA1Fingerprint> FingerprintVector;
-
-// TransportSecurityState
+// We need a generic Fingerprint type, with at least two implementations:
+// SHA1 and SHA256. TODO(palmer): Specify and implement that in

[Ryan Sleevi, 2012/02/17 05:16:01]

Comment nit: Drop the "we"
Comment nit: Place TODO's at the start of a line

That said...

These typedefs don't seem to really belong here. At best, it would seem better
within either TransportSecurityState (as public typedefs) OR where
SHA1Fingerprint is defined (x509_cert_types.h), rather than sticking it into the
net {} namespace here.

[palmer, 2012/03/05 23:47:51]

Done.
I don't want to do too much work in one CL — this thing is going to be monstrous
as it is. Eventually, yes, we'll do a real generic implementation in
x509_cert_types; until then, this typedef gives callers some insulation against
the change.

+// net/base/x509_cert_types.h.
 //
-// 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.
+// Until that work is done, this typedef expresses the intent.
+typedef SHA1Fingerprint Fingerprint;
+
+typedef std::vector<Fingerprint> FingerprintVector;
+
+// Tracks which hosts have enabled strict transport security and/or public
+// key pins.
+//
+// This object manages the in-memory store. Register a SerializationDelegate
+// with |SetSerializationDelegate| 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).
+  // If non-empty, |hsts_hosts| is a JSON string to treat as if it
+  // were a built-in DomainState entry. See |Serialize| for details on the
+  // format.

[Ryan Sleevi, 2012/02/17 05:16:01]

So, it feels very weird to have this interface directly interfacing with JSON
and/or using a string for opaque reasons.

I understand this matches the previous interface, but it feels like if you're
trying to move to a more dynamic, in-memory representation, you should be able
to have all the operations exposed that can help you mutate this class.

If you don't want to expose the operations to mutate this class, perhaps some
sort of visitor or interface that can handle JSON reading.

It just feels like a lot of responsibility to chuck into this class.

It's also weird that TransportSecurityState("") is needed, instead of just
permitting TransportSecurityState() (if no built-in state)

[palmer, 2012/03/05 23:47:51]

Yeah, I know. I was trying to "refactor, but not too much". But you're right.

I've added a utility function:

    bool ParseJSONSecurityState(const std::string& json, TransportSecurityState*
state);

Does that work?

Also, I made the constructor take no arguments.

[Ryan Sleevi, 2012/03/07 06:02:46]

Works a hell of a lot better, thanks. Still not thrilled with the JSON portion
being in this file, but I think the new design is much nicer.

   explicit TransportSecurityState(const std::string& hsts_hosts);
+
   ~TransportSecurityState();
 
-  // A DomainState is the information that we persist about a given domain.
+  // A DomainState describes the transport security state (required upgrade
+  // to HTTPS, and/or any public key pins).
   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,
+    enum UpgradeMode {

[Ryan Sleevi, 2012/02/17 05:16:01]

What does this upgrade?

Upgrade to HTTPS?

Mode {
  MODE_DEFAULT,
  MODE_FORCE_HTTPS,
} ?

[palmer, 2012/03/05 23:47:51]

Done.

+      UPGRADE_NEVER = 0,
+      UPGRADE_ALWAYS = 1,
     };
 
     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);
+
     // 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 any of the FingerprintVectors |static_spki_hashes|,
+    // |bad_static_spki_hashes|, or |dynamic_spki_hashes| contains any
+    // items.
+    bool HasPins() 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);
-
-    // ShouldCertificateErrorsBeFatal returns true iff, given the |mode| of this
-    // DomainState, certificate errors on this domain should be fatal (i.e. no
-    // user bypass).
-    bool ShouldCertificateErrorsBeFatal() const;
+    // Used to see if a dynamic DomainState should override a static one.
+    bool IsMoreStrict(const DomainState& other) const;

[Ryan Sleevi, 2012/02/17 05:16:01]

IsStricterThan?

That said, both strictness and strength seem a bit like abstract concepts, so it
might help to explain what they mean. 

Without having looked at the implementation yet, I would assume some hierarchy:
  1) HSTS
  2) HSTS w/ include subdomains
  3) STS with 5 pins
  3) STS with 3 pins
  4) STS with 1 pin

etc

[palmer, 2012/03/05 23:47:51]

The implementation is not even that well-defined, unfortunately. Fortunately, it
is used in only one place with a "TODO(palmer): Reconsider this?" comment. So,
let's reconsider this and just get rid of it. Maybe someday we'll want it again.

The one place it is used is in EnableHost, to determine if a dynamic entry
should override a static entry. So by getting rid of
IsMoreStrict/IsStricterThan, we are changing policy (allowing weak dynamic
entries to override static ones). However, we are hearing from site operators
that they affirmatively want the ability to un-do pinning (and/or HSTS), so
perhaps that is best.

 
     // 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?
-
-    // Optional; hashes of preloaded "pinned" SubjectPublicKeyInfos. Unless
-    // both are empty, at least one of |preloaded_spki_hashes| and
+    UpgradeMode upgrade_mode;
+
+    // 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

[Ryan Sleevi, 2012/02/17 05:16:01]

nit: Mentioning the means at which it can be set seems like an implementation
detail. Additionally, referencing chrome://net-internals seems like a 'layering'
violation by mentioning the Chrome-implementation details (I don't think
content/ layer for embedders exposes net-internals, for example)

// Optional; hashes of dynamically pinned SPKIs.

[palmer, 2012/03/05 23:47:51]

Done.

-    // could be set e.g. by an HTTP header or by a superfluous certificate.)
+    // could be set e.g. by an HTTP Public-Key-Pins header, or by a
+    // superfluous certificate, or by the user in
+    // chrome://net-internals/#hsts.)
     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;
-
-    // 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
+    FingerprintVector 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;
   };
 
-  class Delegate {
+  class SerializationDelegate {

[Ryan Sleevi, 2012/02/17 05:16:01]

Why the rename? Delegate is certainly the more common name for these things.
When there are > 1 events, we add new virtual methods (that clients can then
no-op), rather than add more delegate types.

Plus I'm not fond of the added |Serialization| naming that limits the scope of
what this can do.

Is there any way that this can be done without locks? I mean, this class is
base::NonThreadSafe, so why should locks even matter?

[palmer, 2012/03/05 23:47:51]

Wow, I bet that is confusing.

But if that's the way we do it, OK. Renamed back to just Delegate.
I'm not sure; It Was Like That When I Got Here. svn blame says phajdan.jr wrote
this:

http://src.chromium.org/viewvc/chrome?view=rev&revision=103012

The safety guarantee/locking seems to be the calls to CalledOnValidThread in
transport_security_state.cc.

[Ryan Sleevi, 2012/03/07 06:02:46]

Not really, since any inheritance from a nested class requires fully qualifying
the name

eg:
class MySerializationDelegate : public TransportSecurityState::Delegate {
 
  // TransportSecurityState::Delegate implementation
  virtual void StateIsDirty(TransportSecurityState* state);
};

    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() {}
+    virtual ~SerializationDelegate() {}
   };
 
-  void SetDelegate(Delegate* delegate);
-
-  // Enable TransportSecurity for |host|.
+  void SetSerializationDelegate(SerializationDelegate* delegate);
+
+  // Enable TransportSecurity for |host|. |state| supercedes any previous
+  // state for the |host|, including static entries.
+  //
+  // The new state for |host| is persisted using the SerializationDelegate
+  // (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 SerializationDelegate
+  // (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.
-  //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
-  //
-  // Note that |*result| is always overwritten on every call.
-  // TODO(palmer): Only update |*result| on success.
+  // Deletes all records created since a given time.
+  void DeleteSince(const base::Time& time);
+
+  // Returns true and updates |*result| if there is a DomainState for
+  // |host|. (If there is no DomainState for |host|, |*result| is not

[Ryan Sleevi, 2012/02/17 05:16:01]

You can remove the ().

Also note
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml?showone=Functi...
on the superflous comments.

It feels like the implicit contract is that it does what it says it does on
success (returns true, updates |*result), then the converse should also be true
(returns false, doesn't update |*result|).

I understand the (historic) reason why you spelled out the false behaviour, but
I think it's ok to leave it implied/understood, as that's certainly the dominant
pattern in our code.

You could always change the if to iff if you have reservations about
non-obviousness.

[palmer, 2012/03/05 23:47:51]

Done.

+  // updated.)
+  //
+  // If |sni_available| is true, searches the static pins defined for

[Ryan Sleevi, 2012/02/17 05:16:01]

The whole |sni_available| doesn't feel like an aptly-named variable, in light of
the recent refactoring, and that it hearkens back to SSLv3 vs TLSv1 versioning.

|sni_enabled| perhaps?

[palmer, 2012/03/05 23:47:51]

Done.

+  // 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.
   bool GetDomainState(DomainState* result,
                       const std::string& host,
-                      bool sni_available);
-
-  // 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.
-  //
-  // Note that |*result| is always overwritten on every call, regardless of
-  // whether or not pins are enabled.
-  //
-  // If |sni_available| is true, searches the preloads defined for SNI-using
-  // hosts as well as the usual preload list.
-  //
-  // TODO(palmer): Only update |*result| if pins exist.
-  bool HasPinsForHost(DomainState* result,
-                      const std::string& host,
-                      bool sni_available);
-
-  // 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
+                      bool sni_available) const;
+
+  // Returns true and updates |*result| if there is a static DomainState for
+  // |host|. (If there is no static DomainState for |host|, |*result| is not
+  // updated.)

[Ryan Sleevi, 2012/02/17 05:16:01]

Same comment here as on 188

[palmer, 2012/03/05 23:47:51]

Done.

+  //
+  // |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 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.
+  bool GetStaticDomainState(DomainState* result,
+                            const std::string& host,
+                            bool sni_available) const;
+
+  // Serializes the transport security state (the set of DomainStates
+  // for all hosts) into |*output|. Returns true if all DomainStates were
+  // serialized correctly.
+  //
+  // The serialization format is JSON; the JSON represents a dictionary of
+  // host:DomainState pairs (host is a string). The DomainState is
+  // represented as a dictionary containing the following keys and value
+  // types (not all keys will always be present):
+  //
+  //     "include_subdomains": true|false
+  //     "created": double
+  //     "expiry": double
+  //     "dynamic_spki_hashes_expiry": double
+  //     "mode": "always"|"never"
+  //             legacy value synonyms "strict"|"pinning-only"
+  //             legacy value "spdy-only" is unused and ignored
+  //     "static_spki_hashes": list of strings
+  //         legacy key synonym "preloaded_spki_hashes"
+  //     "bad_static_spki_hashes": list of strings
+  //         legacy key synonym "bad_preloaded_spki_hashes"
+  //     "dynamic_spki_hashes": list of strings
+  bool Serialize(std::string* output) const;

[Ryan Sleevi, 2012/02/17 05:16:01]

see comments re: serialization being part of this class interface

[palmer, 2012/03/05 23:47:51]

Is it better to implement a const_iterator for TransportSecurityState
(TSS:c_i::operator* returns a DomainState*)?

In effect, TSS is a map of string => DomainState. It's just that the lookup is a
bit weird due to CanonicalizeHost and include_subdomains. Otherwise it would be
a straight-up map. Hence, expose an iterator, and let callers handle
serialization in whatever way is appropriate for them, getting all JSON/et c.
policy out of this class.

[Ryan Sleevi, 2012/03/07 06:02:46]

+1. I recently did that for expiring_cache, and that's the policy taken by
base::DictionaryValue and other types. It doesn't even need to derive from an
STL iterator - we tend to be more verbose and use explicit CapitalizedMethods
for custom iterators.

eg:
SomeType::Iterator it(my_some_type);
for (; it.GetNext(); it.IsValid()) {
  std::string hostname = ...;
  ...
}

[palmer, 2012/03/13 20:13:39]

Done.

+
+  // Clears any existing non-static entries, and then re-populates the
+  // transport security state from the JSON string |state|. Returns true if
+  // all entries were parsed and deserialized correctly.
+  //
+  // Sets |*dirty| to true if the new state differs from the persisted
+  // state; false otherwise.
+  bool LoadEntries(const std::string& state, bool* dirty);
+
+  // 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.
-  //
-  // 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 |sni_available| 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.
   static bool IsGooglePinnedProperty(const std::string& host,

[Ryan Sleevi, 2012/02/17 05:16:01]

This never quite felt like the right layer to stick this. Any ideas on moving
this out, say into CFCR?

[palmer, 2012/03/05 23:47:51]

The implementation relies on private implementation details in
transport_security_state.cc (such as comparison with kGoogleAcceptableCerts). So
to move it to CFCR, we'd have to expose that currently-private stuff. And I'm
not sure that's the right idea. But maybe it is.

[Ryan Sleevi, 2012/03/07 06:02:46]

Fundamentally, how does this differ from GetStaticDomainState() ? Simply
checking if the static pins == kGoogleAcceptableCerts ?

It would still seem like you could replace this with a lookup of the static
domain state for a host and then compare the pin results, all at a higher layer.
WDYT?

[palmer, 2012/03/13 20:13:39]

Correct.
That would require making kGoogleAcceptableCerts public; currently, it is a
private implementation detail in TSS. I'd like to keep it that way, because the
existence of this function is already ugly enough as it is. Making its guts
public increases the size and weirdness of our interface, which is the opposite
of what this refactoring is about. So I think we should leave this wart alone.

                                      bool sni_available);
 
   // 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.
+  // mail.google.com), and only if |host| has a static entry.
   static void ReportUMAOnPinFailure(const std::string& host);

[Ryan Sleevi, 2012/02/17 05:16:01]

This seems like it belongs somewhere else. Unfortunately, I don't know where
might be appropriate. Either private to the implementation (automatically
reporting UMAs) or up in the layer that calls this (and is UMA aware)

[palmer, 2012/03/05 23:47:51]

Unlike IsGooglePinnedProperty, it is fairly straightforward to move this up into
the caller (which is net/socket/ssl_client_socket_nss.cc).

 
   // 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,

[Ryan Sleevi, 2012/02/17 05:16:01]

It seems like this would be better suited in X509Certificate, not here.

[palmer, 2012/03/05 23:47:51]

Done.

-                               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|.
+                               Fingerprint* spki_hash);
+
+  // 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);
+
+  // The maximum number of seconds for which we'll cache an HSTS request.
+  static const long int kMaxHSTSAgeSecs;
+
+  // 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);

[Ryan Sleevi, 2012/02/17 05:16:01]

Does this really need to be part of the public interface? I feel like this
should be a hidden implementation detail.

[palmer, 2012/03/05 23:47:51]

It looks like it's public for use in transport_security_state_unittest.cc. Does
FRIEND_TEST allow us to keep this private?

[Ryan Sleevi, 2012/03/07 06:02:46]

Yup. You can use either FRIEND_TEST (declared between lines 274-275), if it's
just one or two tests, or friend the test fixture and use TEST_F to have all
unit tests derive from the test fixture.

[palmer, 2012/03/13 20:13:39]

Turns out CanonicalizeHost needs to be public anyway, so that
chrome/browser/transport_security_persister.h can use it to obfuscate hostnames
while persisting.

obfuscated_hostname = sha256(CanonicalizeHost(hostname))

+
+  // Parses |side_info| as a side pin (TODO(agl): document the format). If

[Ryan Sleevi, 2012/02/17 05:16:01]

TODOs on newlines at the end of the description.

// Parses |side_info| as a side pin. If successful, returns
// ...
// A side pin ...
// TODO(agl): Document the format of a side pin

[palmer, 2012/03/05 23:47:51]

Done.

+  // successful, returns true and appends the hash of the public key that signed
+  // |leaf_spki| to |*out_pub_key_hash|.
+  //
+  // 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.
   static bool ParseSidePin(const base::StringPiece& leaf_spki,

[Ryan Sleevi, 2012/02/17 05:16:01]

Another weird sort of utility function here that feels out of place for
transport security, since it just deals with SPKI and fingerprints.

[palmer, 2012/03/05 23:47:51]

Agreed. Got a better candidate place? X509Certificate?

[Ryan Sleevi, 2012/03/07 06:02:46]

Side pins always come in as an X.509 extension, right? If so, then it probably
makes sense there, especially since we can support it on all the platforms
without having to coerce into an NSS handle.

[palmer, 2012/03/13 20:13:39]

Done.

                            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);
-
-  // If we have a callback configured, call it to let our serialiser know that
-  // our state is dirty.
+  // If we have a SerializationDelegate, 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,
+
+  static bool Deserialize(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. The keys are
+  // SHA256(CanonicalizeHost(domain)). The reason for hashing them is so
+  // that the stored state does not trivially reveal a user's browing
+  // history to an attacker reading the serialized state on disk.

[Ryan Sleevi, 2012/02/17 05:16:01]

Since this class doesn't do any formal serialization to disk, it seems odd to
mention.

Does this mean that Serialize() stores in SHA256(CanonicalizeHost(domain))?
Would it make more sense to document that there instead?

[palmer, 2012/03/05 23:47:51]

Agreed.
Yes.
Done.

   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 to treat as if they were static (typically originating
+  // from the command line).

[Ryan Sleevi, 2012/02/17 05:16:01]

Since you mentioned |enabled_hosts_| keys, what is the format for these keys (my
guess: CanonicalizeHost(domain), but that's non-obvious.

[palmer, 2012/03/05 23:47:51]

It's even less usable than that. It's
base64encode(sha256(canonicalize_host?(domain))). Sob...

http://scarybeastsecurity.blogspot.com/2011/04/fiddling-with-chromiums-new-ce...

I sort of think that, instead, we should get rid of this feature and instead
make chrome://net-internals/#hsts easier to use. Perhaps.

agl, cevans, thoughts?

   std::map<std::string, DomainState> forced_hosts_;
 
-  // Our delegate who gets notified when we are dirtied, or NULL.
-  Delegate* delegate_;
+  SerializationDelegate* serialization_delegate_;

[Ryan Sleevi, 2012/02/17 05:16:01]

What's the ownership of this pointer? (I'm guessing no ownership at all). Which
means that |delegate_| must remain valid for the lifetime of the class -
something that should be documented in Set[Serialization]Delegate().

For some reason, I have this idea that the more Chromium-y pattern is to:
a) Take the delegate in the constructor and mention the non-ownership
b) Provide a base::ObserverList

Having the function to set the delegate (at potentially any time during the
lifetime of the class), and without formally documenting the way to unset it
(I'm guessing |NULL| is valid input to unset, but it's not clear), is a bit odd.

[palmer, 2012/03/05 23:47:51]

I agree. I think there is no ownership at all, and so far everyone has simply
been letting the delegate live as long as the class.

I'm inclined to do:

c) Take the delegate in the constructor and have TSS take ownership

Thoughts?

[Ryan Sleevi, 2012/03/07 06:02:46]

That gets a bit messier - then objects which have-a TSS potentially have a loop
or have to write detachable delegates.

I think specifying non-ownership and must-outlive is fine, unless you can think
of a reason that TSS might live a long time.

[palmer, 2012/03/13 20:13:39]

TSS is per-profile, so they do live a long time.

But in the existing code, the Delegate lives the same life as the TSS. So I
think it is ok to pass the Delegate to the TSS constructor and document that the
caller owns the Delegate.

 
   DISALLOW_COPY_AND_ASSIGN(TransportSecurityState);
 };
 
 }  // namespace net
 
 #endif  // NET_BASE_TRANSPORT_SECURITY_STATE_H_