Implement SHA-256 fingerprint support

net/base/x509_cert_types.h

Index: net/base/x509_cert_types.h
===================================================================
--- net/base/x509_cert_types.h	(revision 151057)
+++ net/base/x509_cert_types.h	(working copy)
@@ -7,10 +7,12 @@
 
 #include <string.h>
 
+#include <algorithm>
 #include <set>
 #include <string>
 #include <vector>
 
+#include "base/logging.h"
 #include "base/string_piece.h"
 #include "build/build_config.h"
 #include "net/base/net_export.h"
@@ -28,32 +30,137 @@
 class X509Certificate;
 
 // SHA-1 fingerprint (160 bits) of a certificate.
-struct NET_EXPORT SHA1Fingerprint {
-  bool Equals(const SHA1Fingerprint& other) const {
+struct NET_EXPORT SHA1HashValue {
+  bool Equals(const SHA1HashValue& other) const {
     return memcmp(data, other.data, sizeof(data)) == 0;
   }
 
   unsigned char data[20];

[hshi1, 2012/08/11 01:05:40]

Nit: would it be better to use base::kSHA1Length instead of 20?

[Ryan Sleevi, 2012/08/11 01:39:55]

I'm not sure how much it will help readability, and it definitely won't help
functionality (the length of SHA1/SHA256 won't change). However, it'd force more
header dependencies here, and this is already one of the most widely included
headers, so it does seem like it might harm compile times ever so slightly.

[palmer, 2012/08/14 19:40:42]

I'll leave it as-is then.

 };
 
-// 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;
+class NET_EXPORT SHA1HashValueLessThan {
+ public:
+  bool operator() (const SHA1HashValue& lhs,
+                   const SHA1HashValue& rhs) const {
+    return memcmp(lhs.data, rhs.data, sizeof(lhs.data)) < 0;
+  }
+};
 
-typedef std::vector<Fingerprint> FingerprintVector;
+struct NET_EXPORT SHA256HashValue {
+  bool Equals(const SHA256HashValue& other) const {
+    return memcmp(data, other.data, sizeof(data)) == 0;
+  }
 
-class NET_EXPORT SHA1FingerprintLessThan {
+  unsigned char data[32];

[hshi1, 2012/08/11 01:05:40]

Nit: would it be better to use crypto::kSHA256Length instead of 32?

[palmer, 2012/08/14 19:40:42]

Leaving as-is, for the reason given above.

+};
+
+class NET_EXPORT SHA256HashValueLessThan {
  public:
-  bool operator() (const SHA1Fingerprint& lhs,
-                   const SHA1Fingerprint& rhs) const {
+  bool operator() (const SHA256HashValue& lhs,
+                   const SHA256HashValue& rhs) const {
     return memcmp(lhs.data, rhs.data, sizeof(lhs.data)) < 0;
   }
 };
 
+enum HashValueTag {
+  HASH_VALUE_SHA1,
+  HASH_VALUE_SHA256,
+
+  // This must always be last.
+  HASH_VALUE_TAGS_COUNT
+};
+
+struct NET_EXPORT HashValue {
+  bool Equals(const HashValue& other) const {
+    if (tag != other.tag)
+      return false;
+    switch (tag) {
+      case HASH_VALUE_SHA1:
+        return fingerprint.sha1.Equals(other.fingerprint.sha1);
+        break;
+      case HASH_VALUE_SHA256:
+        return fingerprint.sha256.Equals(other.fingerprint.sha256);
+        break;
+      default:
+        NOTREACHED() << "Unknown HashValueTag " << tag;
+        return false;
+    }

[Ryan Sleevi, 2012/08/11 01:39:55]

style nit: At this point, the complexity is such that this should be a class,
rather than a struct, since you already have .size() / .data() accessor. Perhaps
take the HashValueTag in the ctor and then make it immutable?

perf nit: I'm concerned these functions may be sufficiently complex to defeat
inlining. Perhaps these should be moved to an implementation file?

[palmer, 2012/08/14 19:40:42]

Done.

+  }
+
+  size_t size() const {
+    switch (tag) {
+      case HASH_VALUE_SHA1:
+        return sizeof(fingerprint.sha1.data);
+        break;
+      case HASH_VALUE_SHA256:
+        return sizeof(fingerprint.sha256.data);
+        break;
+      default:
+        NOTREACHED() << "Unknown HashValueTag " << tag;
+        // Although this is NOTREACHED, this function is inlined and its
+        // return value is passed to memset as the length argument. This may
+        // result in what appears (in some stages of compilation) to be a
+        // call to to memset with a length argument of 0, which results in a
+        // warning. Therefore, we return a dummy value here.
+        return sizeof(fingerprint.sha1.data);
+    }
+  }
+
+  unsigned char* data() {
+    switch (tag) {

[Ryan Sleevi, 2012/08/11 01:39:55]

Rather than duping the code, can you not

return const_cast<unsigned char*>(const_cast<const HashValue*>(this)->data()) ?

[palmer, 2012/08/14 19:40:42]

Done.

+      case HASH_VALUE_SHA1:
+        return fingerprint.sha1.data;
+        break;
+      case HASH_VALUE_SHA256:
+        return fingerprint.sha256.data;
+        break;
+      default:
+        NOTREACHED() << "Unknown HashValueTag " << tag;
+        return NULL;
+    }
+  }
+
+  const unsigned char* data() const {
+    switch (tag) {
+      case HASH_VALUE_SHA1:
+        return fingerprint.sha1.data;
+        break;
+      case HASH_VALUE_SHA256:
+        return fingerprint.sha256.data;
+        break;
+      default:
+        NOTREACHED() << "Unknown HashValueTag " << tag;
+        return NULL;
+    }
+  }
+
+  HashValueTag tag;
+
+  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();
+    int r = memcmp(lhs.data(), rhs.data(), std::min(lhs_size, rhs_size));
+
+    if (r == 0 && lhs_size != rhs_size)
+      return lhs_size < rhs_size;
+    return r < 0;

[Ryan Sleevi, 2012/08/11 01:39:55]

Seems like this can be re-ordered

size_t lhs_size = lhs.size();
size_t rhs_size = rhs.size();

if (lhs_size != rhs_size)
  return lhs_size < rhs_size;

int r = memcmp(...)
return r < 0;

[palmer, 2012/08/14 19:40:42]

Done.

+  }
+};
+
+typedef std::vector<HashValue> HashValueVector;
+
 // IsSHA1HashInSortedArray returns true iff |hash| is in |array|, a sorted
 // array of SHA1 hashes.
-bool NET_EXPORT IsSHA1HashInSortedArray(const SHA1Fingerprint& hash,
+bool NET_EXPORT IsSHA1HashInSortedArray(const SHA1HashValue& hash,
                                         const uint8* array,
                                         size_t array_byte_len);
 
@@ -130,10 +237,10 @@
 
  private:
   // The set of fingerprints of allowed certificates.
-  std::set<SHA1Fingerprint, SHA1FingerprintLessThan> allowed_;
+  std::set<SHA1HashValue, SHA1HashValueLessThan> allowed_;
 
   // The set of fingerprints of denied certificates.
-  std::set<SHA1Fingerprint, SHA1FingerprintLessThan> denied_;
+  std::set<SHA1HashValue, SHA1HashValueLessThan> denied_;
 };
 
 #if defined(OS_MACOSX) && !defined(OS_IOS)