Separate http_security_headers from transport_security_state

net/base/x509_cert_types.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_X509_CERT_TYPES_H_
 #define NET_BASE_X509_CERT_TYPES_H_
 
 #include <string.h>
 
 #include <set>
@@ skipping 57 matching lines @@
   // This must always be last.
   HASH_VALUE_TAGS_COUNT
 };
 
 class NET_EXPORT HashValue {
  public:
   explicit HashValue(HashValueTag tag) : tag(tag) {}
   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;
 
   HashValueTag tag;
 
  private:
   union {
     SHA1HashValue sha1;
     SHA256HashValue sha256;
   } fingerprint;
 };
 
 class NET_EXPORT HashValueLessThan {
  public:
   bool operator()(const HashValue& lhs,
                   const HashValue& rhs) const {
     size_t lhs_size = lhs.size();
     size_t rhs_size = rhs.size();
 
     if (lhs_size != rhs_size)
       return lhs_size < rhs_size;
 
     return memcmp(lhs.data(), rhs.data(), lhs_size) < 0;
   }
 };
 
+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
 // array of SHA1 hashes.
 bool NET_EXPORT IsSHA1HashInSortedArray(const SHA1HashValue& hash,
                                         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();
   explicit CertPrincipal(const std::string& name);
   ~CertPrincipal();
 
 #if (defined(OS_MACOSX) && !defined(OS_IOS)) || defined(OS_WIN)
   // Parses a BER-format DistinguishedName.
   bool ParseDistinguishedName(const void* ber_name_data, size_t length);
 #endif
@@ skipping 87 matching lines @@
 // Attempts to parse |raw_date|, an ASN.1 date/time string encoded as
 // |format|, and writes the result into |*time|. If an invalid date is
 // specified, or if parsing fails, returns false, and |*time| will not be
 // updated.
 bool ParseCertificateDate(const base::StringPiece& raw_date,
                           CertDateFormat format,
                           base::Time* time);
 }  // namespace net
 
 #endif  // NET_BASE_X509_CERT_TYPES_H_