Separate http_security_headers from transport_security_state

net/base/x509_cert_types.h

Index: net/base/x509_cert_types.h
===================================================================
--- net/base/x509_cert_types.h	(revision 163343)
+++ net/base/x509_cert_types.h	(working copy)
@@ -75,6 +75,11 @@
   HashValue() : tag(HASH_VALUE_SHA1) {}
 
   bool Equals(const HashValue& other) const;
+
+  // Parse/write in this format: "sha1/Guzek9lMwR3KeIS8wwS9gBvVtIg="
+  bool ParseBase64String(const std::string& input);
+  std::string WriteBase64String() const;

[Ryan Sleevi, 2012/10/25 01:59:09]

I feel this is not coupled to the "HashValue" concept, which is used in many
more places than pinning, thus as an API, doesn't belong here.

I think it is more closely aligned with some sort of pinning structure, and thus
belongs in some sort of method for TransportSecurityState or equivalent.

[unsafe, 2012/10/25 06:59:54]

Note that the type/base64 format is *not* the HPKP header format (which is
pin-type="base64").  The type/base64 format is used for logging warning messages
in TSS, for net-internals query/user-entry, and for dynamic JSON.

I think that to the extent that HashValue and HashValueVector are generic
concepts, it makes sense to have some way to read/write them as strings - for
debugging/unit-testing if nothing else - so I don't feel this is overly tied to
pinning..  It might be good to separate the HashValue* stuff out of
x509_cert_types, though...

+
   size_t size() const;
   unsigned char* data();
   const unsigned char* data() const;
@@ -102,6 +107,18 @@
   }
 };
 
+class NET_EXPORT HashValuesEqualPredicate {

[Ryan Sleevi, 2012/10/25 01:59:09]

naming nit: HashValueEquals
design nit: Is this so widely useful as to make sense in a common place? I
actually had the same concern regarding HashValueLessThan (which is apparently
not used anywhere and should thus be removed)

[unsafe, 2012/10/25 06:59:54]

Deleted HashValueLessThan.  HashValueEqualsPredicate is used in this
x509_cert_types.cc and http_security_headers.cc in this patch (for
HashesIntersect() and IsBackupPinPresent()) but arguably all that pinning logic
should be in its own file (instead of split between here and
http_security_headers)... 

+  public:
+  explicit HashValuesEqualPredicate(const HashValue& fingerprint) :
+      fingerprint_(fingerprint) {}
+
+  bool operator()(const HashValue& other) const {
+    return fingerprint_.Equals(other);
+  }
+
+  const HashValue& fingerprint_;
+};
+
 typedef std::vector<HashValue> HashValueVector;
 
 // IsSHA1HashInSortedArray returns true iff |hash| is in |array|, a sorted
@@ -110,6 +127,17 @@
                                         const uint8* array,
                                         size_t array_byte_len);
 
+// Returns true if the intersection of |a| and |b| is not empty. If either
+// |a| or |b| is empty, returns false.
+bool NET_EXPORT HashesIntersect(const HashValueVector& a,
+                                const HashValueVector& b);
+
+// Convert between HashValueVector and comma-separated string format:
+// "sha1/Guzek9lMwR3KeIS8wwS9gBvVtIg=,sha256/U8Sg...
+std::string NET_EXPORT HashesToBase64String(const HashValueVector& hashes);
+bool Base64StringToHashes(const std::string& hashes_str,
+                          HashValueVector* hashes);

[Ryan Sleevi, 2012/10/25 01:59:09]

BUG?: You don't NET_EXPORT Base64StringToHashes, therefore it cannot be used
outside of the "net" component.

Broadly speaking though, "Base64String" is an encoding format for PKP pins, and
thus does not seem to belong to generic net types. For that matter, it has
fundamentally nothing to do with certificates (the hash value stuff has
organically grown perhaps unchecked)

If this is only used in one or two places, this would be better off kept to the
.cc file (eg: not exposed in any header / not exported) that it's used.

[unsafe, 2012/10/25 06:59:54]

Fixed.
HashesToBase64String is used in a couple places: net-internals for queries and
TSS for warning messages.  Base64StringToHashes is used for net-internals
user-entry, but it probably should also be used for adding more HPKP unit tests.

+
 // CertPrincipal represents the issuer or subject field of an X.509 certificate.
 struct NET_EXPORT CertPrincipal {
   CertPrincipal();