Support SHA-256 in public key pins for HTTPS.

Support SHA-256 in public key pins for HTTPS.

The HTTP-based Public Key Pinning Internet Draft
(tools.ietf.org/html/draft-ietf-websec-key-pinning) requires this.

Per wtc, give the *Fingeprint* types more meaningful *HashValue* names.

Cleaning up lint along the way.

BUG=117914
TEST=net_unittests, unit_tests TransportSecurityPersisterTest

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

[palmer, 2012-06-14 01:54:45]

To fully implement the HTTP-based public key pinning spec, we need to support
other hash functions (currently, at least SHA-1 and SHA-256). TACK, if we ever
adopt it, requires agility too. This CL is the first step in that process.

I went with a tagged-union design because we need the Fingerprint type to be
generic at runtime (people might pin a set of public keys using more than one
hash function; some with SHA-1, some with SHA-256). If there is a better way to
achieve this limited type variation, lemme know and I'll do that.

Thanks!

[mmenke, 2012-06-14 15:47:29]

I'm not a good reviewer for this (agl and wtc are, however).  Just one quick
comment.

https://chromiumcodereview.appspot.com/10545166/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://chromiumcodereview.appspot.com/10545166/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1087: continue;
I'd suggest a NOTREACHED() and outputting a label of "???/" or similar in this
case, so it's more likely this will be noticed when/if new fingerprint types are
added..

[palmer, 2012-06-14 18:56:43]

Thanks Matt!

https://chromiumcodereview.appspot.com/10545166/diff/1/chrome/browser/ui/webu...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://chromiumcodereview.appspot.com/10545166/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1087: continue;
On 2012/06/14 15:47:29, Matt Menke wrote:
> I'd suggest a NOTREACHED() and outputting a label of "???/" or similar in this
> case, so it's more likely this will be noticed when/if new fingerprint types
are
> added..

Done.

[Ryan Sleevi, 2012-06-15 21:43:59]

drive by nits :)

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:83: DCHECK(false) << "Unknown FingerprintTag " <<
tag;
NOTREACHED() rather than DCHECK(false). You can still stream error messages
though.

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:98: return false;
return 0;

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:102: unsigned char* data() const {
This should either be:

const unsigned char* data() const { <-- logical const correct
or
unsigned char* data() { <-- non-const

If you need to provide two versions, that's more preferable than the const-cast.

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:127: const Fingerprint& rhs) const {
You should compare their types & sizes first, before doing the memory
comparison.

[palmer, 2012-06-15 22:40:48]

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:83: DCHECK(false) << "Unknown FingerprintTag " <<
tag;
On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> NOTREACHED() rather than DCHECK(false). You can still stream error messages
> though.

Done.

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:98: return false;
On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> return 0;

Done.

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:102: unsigned char* data() const {
On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> This should either be:
> 
> const unsigned char* data() const { <-- logical const correct
> or
> unsigned char* data() { <-- non-const
> 
> If you need to provide two versions, that's more preferable than the
const-cast.

Done.

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:127: const Fingerprint& rhs) const {
On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> You should compare their types & sizes first, before doing the memory
> comparison.

What should I do if they don't match? This is for use in sorting
FingerprintVectors, for example. Basically I'm treating this like strcmp. Doing
e.g. NOTREACHED for lhs.tag != rhs.tag doesn't seem appropriate.

[Ryan Sleevi, 2012-06-15 22:51:45]

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:127: const Fingerprint& rhs) const {
On 2012/06/15 22:40:49, Chris P. wrote:
> On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> > You should compare their types & sizes first, before doing the memory
> > comparison.
> 
> What should I do if they don't match? This is for use in sorting
> FingerprintVectors, for example. Basically I'm treating this like strcmp.
Doing
> e.g. NOTREACHED for lhs.tag != rhs.tag doesn't seem appropriate.

Compare their tags/sizes.

SHA1 sorts lexically SHA256.
Within SHA1, hashes are sorted based memcmp
Within SHA256, hashes are sorted based on memcmp

simply:
if (lhs.tag != rhs.tag)
  return lhs.tag < rhs.tag;
if (lhs.size != rhs.size)  // not strictly needed?
  return lhs.size < rhs.size;  // not strictly needed?
return memcmp(lhs.data(), rhs.data(), lhs_size)

[Ryan Sleevi, 2012-06-15 22:54:49]

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:127: const Fingerprint& rhs) const {
On 2012/06/15 22:51:45, Ryan Sleevi wrote:
> On 2012/06/15 22:40:49, Chris P. wrote:
> > On 2012/06/15 21:44:00, Ryan Sleevi wrote:
> > > You should compare their types & sizes first, before doing the memory
> > > comparison.
> > 
> > What should I do if they don't match? This is for use in sorting
> > FingerprintVectors, for example. Basically I'm treating this like strcmp.
> Doing
> > e.g. NOTREACHED for lhs.tag != rhs.tag doesn't seem appropriate.
> 
> Compare their tags/sizes.
> 
> SHA1 sorts lexically SHA256.
> Within SHA1, hashes are sorted based memcmp
> Within SHA256, hashes are sorted based on memcmp
> 
> simply:
> if (lhs.tag != rhs.tag)
>   return lhs.tag < rhs.tag;
> if (lhs.size != rhs.size)  // not strictly needed?
>   return lhs.size < rhs.size;  // not strictly needed?
> return memcmp(lhs.data(), rhs.data(), lhs_size)

sorry, the last one would be

return memcmp(lhs.data(), rhs.data(), lhs_size) < 0;

[agl, 2012-06-18 15:12:37]

LGTM with rsleevi's comments addressed.

[mmenke, 2012-06-18 15:17:57]

net_internals LGTM as well.  Forgot you'd need an OWNERs signoff for that.

[wtc, 2012-06-18 17:00:29]

Review comments on patch set 3:

I'm sorry I didn't have time to review this CL.  I just have
a comment about the name of the type "Fingerprint".

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10545166/diff/7002/net/base/x509_cert_...
net/base/x509_cert_types.h:71: struct NET_EXPORT Fingerprint {

It would be nice to come up with a better name than Fingerprint.

The type has been renamed once, from
X509Certificate::SHA1Fingerprint to SHA1Fingerprint.
If we shorten it again to just "Fingerprint", it is no longer
descriptive enough to someone unfamiliar with the code.

I think we should use something like HashResult, HashValue,
or HashFingerprint.

[palmer, 2012-07-30 21:08:06]

> It would be nice to come up with a better name than Fingerprint.
> 
> The type has been renamed once, from
> X509Certificate::SHA1Fingerprint to SHA1Fingerprint.
> If we shorten it again to just "Fingerprint", it is no longer
> descriptive enough to someone unfamiliar with the code.
> 
> I think we should use something like HashResult, HashValue,
> or HashFingerprint.

I picked HashValue.

[commit-bot: I haz the power, 2012-07-31 18:40:58]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/palmer@chromium.org/10545166/22040

[commit-bot: I haz the power, 2012-07-31 20:16:02]

Change committed as 149261