Implement SHA-256 fingerprint support

Implement SHA-256 fingerprint support

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=155365

[palmer, 2012-08-11 00:48:02]

This Time For Sure!

Please use your most eagley eagle eye for testing coverage. And anything else
that strikes your fancy.

Thank you!

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

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
File net/base/transport_security_state.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:255: if ((size != 30 && size != 46) ||
value[0] != '"' || value[size - 1] != '"')
Having hard-coded numerical values (such as 30 and 46) is awkward. Moreover I
feel that this size check (30 or 46 characters) is rather redundant. The
decoded.size() check a few lines below already verifies that the hash is of the
corrected length.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:268: if (decoded.size() == 20)
Nit: would it be better to use base::kSHA1Length instead of 20?

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:270: else if (decoded.size() == 32)
Nit: would it be better to use crypto::kSHA256Length instead of 32?

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:38: unsigned char data[20];
Nit: would it be better to use base::kSHA1Length instead of 20?

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:54: unsigned char data[32];
Nit: would it be better to use crypto::kSHA256Length instead of 32?

[Ryan Sleevi, 2012-08-11 01:39:54]

High level:

I'm pretty concerned with the use of std::vector<HashValueVector> as a map type,
which seems to be done heavily throughout. I see very little code checking the
consistency of the input, and just automatically going to accessing
std::vector<HashValueVector>[HASH_VALUE_TAG] . If the
std::vector<HashValueVector> is empty, things suddenly go Michael Bay. It
effectively forces callers to do

std::vector<HashValueVector> my_vector(HASH_VALUE_TAGS_COUNT), which I think is
very counter-intuitive and non-obvious for its usage. I'd readily agree that
using a map type that was always a constant size of two wouldn't be worth it,
but I don't think the current pattern of assuming everything is created
correctly is safe either.

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/net/tra...
File chrome/browser/net/transport_security_persister.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/net/tra...
chrome/browser/net/transport_security_persister.cc:35: std::string label;
nit: declare label outside the loop?

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

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1103: }
Duplicate code with transport_security_persister - should there be a common
method?

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
File net/base/cert_verify_proc.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
net/base/cert_verify_proc.cc:267: const HashValueVector& sha1_hashes =
public_key_hashes[HASH_VALUE_SHA1];
BUG: Accessing HASH_VALUE_SHA1 without checking that .size() >= HASH_VALUE_SHA1

Seems like you really want an std::map<HashValueTag, HashValueVector> here as
the arg, but for presumably perf, you're relying on the fact that a HashValueTag
is a viable index into an std::vector<>. I think that's really non-obvious, and
minimally requires more defensiveness.

My gut is that "adding DCHECKs" wouldn't be in itself sufficient

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.cc
File net/base/ssl_info.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.cc#n...
net/base/ssl_info.cc:48: public_key_hashes.reserve(HASH_VALUE_TAGS_COUNT);
public_key_hashes.clear();
public_key_hashes.resize(HASH_VALUE_TAGS_COUNT);

It'll invoke the ctors for the HashValueVector

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.h
File net/base/ssl_info.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.h#ne...
net/base/ssl_info.h:77: // fingerprints made with the algorithm named by the
FingerprintTag.
out of date comment

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:38: unsigned char data[20];
On 2012/08/11 01:05:40, hshi1 wrote:
> Nit: would it be better to use base::kSHA1Length instead of 20?

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.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:87: }
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?

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:110: switch (tag) {
Rather than duping the code, can you not

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

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:155: return r < 0;
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;

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:135: SECItem* encodedResponse, void* pwArg);
nit: Not sure this change is necessary. The style violation is in part to match
the NSS function signature, which uses the original style.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:2768: // them only in the HASH_VALUE_SHA1
vector.
(file a) BUG or it didn't happen?

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

I think I've addressed all the comments — thank you hshi1 and sleevi!! — except
for changing to map<vector<HashValue> >. I'll do that if people really think I
should. Thoughts? Other mechanisms that are more obviously safe?

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/net/tra...
File chrome/browser/net/transport_security_persister.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/net/tra...
chrome/browser/net/transport_security_persister.cc:35: std::string label;
> nit: declare label outside the loop?

This is resolved with the new HashValue.label method in x509_cert_types.h.

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

https://chromiumcodereview.appspot.com/10826257/diff/1/chrome/browser/ui/webu...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:1103: }
> Duplicate code with transport_security_persister - should there be a common
> method?

Yep, I added a HashValue.label method in x509_cert_types.h.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
File net/base/cert_verify_proc.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
net/base/cert_verify_proc.cc:267: const HashValueVector& sha1_hashes =
public_key_hashes[HASH_VALUE_SHA1];
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> BUG: Accessing HASH_VALUE_SHA1 without checking that .size() >=
HASH_VALUE_SHA1
> 
> Seems like you really want an std::map<HashValueTag, HashValueVector> here as
> the arg, but for presumably perf, you're relying on the fact that a
HashValueTag
> is a viable index into an std::vector<>. I think that's really non-obvious,
and
> minimally requires more defensiveness.
> 
> My gut is that "adding DCHECKs" wouldn't be in itself sufficient

Well, the constructor guarantees that public_key_hashes has the right number of
items. Can't we count on that?

As I recall using a vector of vectors instead of a map of vectors was your
idea... :) I'm fine with changing it to a map if people think it's necessary or
better.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.cc
File net/base/ssl_info.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.cc#n...
net/base/ssl_info.cc:48: public_key_hashes.reserve(HASH_VALUE_TAGS_COUNT);
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> public_key_hashes.clear();
> public_key_hashes.resize(HASH_VALUE_TAGS_COUNT);
> 
> It'll invoke the ctors for the HashValueVector

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.h
File net/base/ssl_info.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/ssl_info.h#ne...
net/base/ssl_info.h:77: // fingerprints made with the algorithm named by the
FingerprintTag.
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> out of date comment

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
File net/base/transport_security_state.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:255: if ((size != 30 && size != 46) ||
value[0] != '"' || value[size - 1] != '"')
On 2012/08/11 01:05:40, hshi1 wrote:
> Having hard-coded numerical values (such as 30 and 46) is awkward. Moreover I
> feel that this size check (30 or 46 characters) is rather redundant. The
> decoded.size() check a few lines below already verifies that the hash is of
the
> corrected length.

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:268: if (decoded.size() == 20)
On 2012/08/11 01:05:40, hshi1 wrote:
> Nit: would it be better to use base::kSHA1Length instead of 20?

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/transport_sec...
net/base/transport_security_state.cc:270: else if (decoded.size() == 32)
On 2012/08/11 01:05:40, hshi1 wrote:
> Nit: would it be better to use crypto::kSHA256Length instead of 32?

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:38: unsigned char data[20];
> 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.

I'll leave it as-is then.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:54: unsigned char data[32];
> Nit: would it be better to use crypto::kSHA256Length instead of 32?

Leaving as-is, for the reason given above.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:87: }
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> 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?

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:110: switch (tag) {
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> Rather than duping the code, can you not
> 
> return const_cast<unsigned char*>(const_cast<const HashValue*>(this)->data())
?

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:155: return r < 0;
On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> 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;

Done.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
File net/socket/ssl_client_socket_nss.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:135: SECItem* encodedResponse, void* pwArg);
> nit: Not sure this change is necessary. The style violation is in part to
match
> the NSS function signature, which uses the original style.

Ok, in that case I'll change it back.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/socket/ssl_client_...
net/socket/ssl_client_socket_nss.cc:2768: // them only in the HASH_VALUE_SHA1
vector.
> (file a) BUG or it didn't happen?

agl is OK with leaving it as-is, so I'll remove the TODOs but leave the
explanatory comment.

[Ryan Sleevi, 2012-08-14 20:02:50]

I think I'm still very uncomfortable with the vector<vector> and the assumption
that it's always safe to directly index into it.

It feels like you're tightly coupling all of these functions to the
initialization code in CertVerifyResult, and you're requiring that all code have
similar initialization.

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
File net/base/cert_verify_proc.cc (right):

https://chromiumcodereview.appspot.com/10826257/diff/1/net/base/cert_verify_p...
net/base/cert_verify_proc.cc:267: const HashValueVector& sha1_hashes =
public_key_hashes[HASH_VALUE_SHA1];
On 2012/08/14 19:40:42, Chris P. wrote:
> On 2012/08/11 01:39:55, Ryan Sleevi wrote:
> > BUG: Accessing HASH_VALUE_SHA1 without checking that .size() >=
> HASH_VALUE_SHA1
> > 
> > Seems like you really want an std::map<HashValueTag, HashValueVector> here
as
> > the arg, but for presumably perf, you're relying on the fact that a
> HashValueTag
> > is a viable index into an std::vector<>. I think that's really non-obvious,
> and
> > minimally requires more defensiveness.
> > 
> > My gut is that "adding DCHECKs" wouldn't be in itself sufficient
> 
> Well, the constructor guarantees that public_key_hashes has the right number
of
> items. Can't we count on that?

What constructor?

public_key_hashes is an input argument here. There's no guarantee that the
caller ensured it's in the proper form - or that it's not an empty vector.

That's what I meant here. Adding DCHECKs is great, but since it seems very
unlikely that caller's will be directly supplying hashes (as opposed to crafting
them from user/network input), it seems risky.

> 
> As I recall using a vector of vectors instead of a map of vectors was your
> idea... :) I'm fine with changing it to a map if people think it's necessary
or
> better.

You could pass a (fixed-size) array of (dynamically-sized) vectors ;-) At least
then the compiler "should" warn, IIRC. More testing is needed to confirm I'm not
completely mistaken here

IsPublicKeyBlacklisted(const std::vector<HashValueVector>
(&public_key_hashes)[HASH_VALUES_TAG_COUNT])

That will pass a reference to a fixed-size array of 2 vectors. Beyond the
ugliness (which can probably be accomodated with a typedef), I don't recall what
the behaviour is for copy-assignment between two arrays - whether it performs a
memory-wise copy or if it invokes each members' copy constructor.

[palmer, 2012-08-16 02:26:04]

> IsPublicKeyBlacklisted(const std::vector<HashValueVector>
> (&public_key_hashes)[HASH_VALUES_TAG_COUNT])
> 
> That will pass a reference to a fixed-size array of 2 vectors. Beyond the
> ugliness (which can probably be accomodated with a typedef), I don't recall
what
> the behaviour is for copy-assignment between two arrays - whether it performs
a
> memory-wise copy or if it invokes each members' copy constructor.

I don't know either. But it segfaults unless you have really populated each slot
in the array with a real vector, which puts us at square 1: Someone could pass
in a correctly-sized array of unconstructed vector-sized blobs of junk.

By contrast, map<int, vector<...> > behaves delightfully when you index it with
operator[], but operator[] is non-const (it inserts a default-constructed object
if none existed before). It wouldn't hurt to do that, but It Just Seems Better
if callees get const references (for optimization opportunities if for no other
reason).

What about simply using a flat HashValueVector? Each HashValue is tagged with
its type, after all, and the HashValue::Equals method checks the tags. Perhaps
this double-tagging is over-thinking it.

[Ryan Sleevi, 2012-08-16 19:14:57]

On 2012/08/16 02:26:04, Chris P. wrote:
> > IsPublicKeyBlacklisted(const std::vector<HashValueVector>
> > (&public_key_hashes)[HASH_VALUES_TAG_COUNT])
> > 
> > That will pass a reference to a fixed-size array of 2 vectors. Beyond the
> > ugliness (which can probably be accomodated with a typedef), I don't recall
> what
> > the behaviour is for copy-assignment between two arrays - whether it
performs
> a
> > memory-wise copy or if it invokes each members' copy constructor.
> 
> I don't know either. But it segfaults unless you have really populated each
slot
> in the array with a real vector, which puts us at square 1: Someone could pass
> in a correctly-sized array of unconstructed vector-sized blobs of junk.
> 
> By contrast, map<int, vector<...> > behaves delightfully when you index it
with
> operator[], but operator[] is non-const (it inserts a default-constructed
object
> if none existed before). It wouldn't hurt to do that, but It Just Seems Better
> if callees get const references (for optimization opportunities if for no
other
> reason).
> 
> What about simply using a flat HashValueVector? Each HashValue is tagged with
> its type, after all, and the HashValue::Equals method checks the tags. Perhaps
> this double-tagging is over-thinking it.

Winning.

[palmer, 2012-08-17 21:58:11]

> > What about simply using a flat HashValueVector? Each HashValue is tagged
with
> > its type, after all, and the HashValue::Equals method checks the tags.
Perhaps
> > this double-tagging is over-thinking it.
> 
> Winning.

That is now done. It's considerably cleaner.

[hshi1, 2012-08-17 22:34:52]

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
net/base/x509_cert_types.h:77: { }
nit: I believe the prevalent code style is "{}" without space in between, and
this can be moved to the end of the previous line: "tag(tag) {}". Alternatively
only the "{" is moved to the previous line, while "}" starts at the next line.

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
net/base/x509_cert_types.h:81: { }
nit: same as comment above.

[palmer, 2012-08-17 23:50:31]

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
File net/base/x509_cert_types.h (right):

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
net/base/x509_cert_types.h:77: { }
On 2012/08/17 22:34:52, hshi1 wrote:
> nit: I believe the prevalent code style is "{}" without space in between, and
> this can be moved to the end of the previous line: "tag(tag) {}".
Alternatively
> only the "{" is moved to the previous line, while "}" starts at the next line.
> 

Done.

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
net/base/x509_cert_types.h:81: { }
On 2012/08/17 22:34:52, hshi1 wrote:
> nit: same as comment above.

Done.

https://chromiumcodereview.appspot.com/10826257/diff/11008/net/base/x509_cert...
net/base/x509_cert_types.h:81: { }
On 2012/08/17 22:34:52, hshi1 wrote:
> nit: same as comment above.

Done.

[hshi1, 2012-08-23 22:06:39]

LGTM but need an owner to approve.

[Ryan Sleevi, 2012-08-23 22:48:20]

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_n...
File net/base/cert_verify_proc_nss.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_n...
net/base/cert_verify_proc_nss.cc:576: DCHECK_EQ(rv, SECSuccess);
nit: DCHECK_EQ(expected, actual)

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_u...
File net/base/cert_verify_proc_unittest.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_u...
net/base/cert_verify_proc_unittest.cc:401: HashValue fingerprint;
nit:
HashValue fingerprint(HASH_VALUE_SHA1);

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_result...
net/base/cert_verify_result.h:51: // first element of each sub-vector.
nit: update comment (re: sub-vector)

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:254: HashValue fp;
nit: Move this to line 261

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:267: return false;
Seems like you should be passing in the expected type as an input parameter to
ParseAndAppendPin, rather than trying to automagically discover it.

Perhaps move the parsing of algorithm name & type into a single method, and just
do something like

StartsWithASCII(token, "sha1-"...

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:801: HashesIntersect(static_spki_hashes,
hashes)) {
BUG? See previous comments about the loss of half of the conditional

!(dynamic_spki_hashes.empty() && static_spki_hashes.empty())

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
File net/base/transport_security_state_unittest.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:110:
base::SHA1HashBytes(reinterpret_cast<const unsigned char*>(spki.data()),
does a static_cast work here?

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:975: // to equifax_sha{1,256}. Bad
chains chain up to equifa through
equifax

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:1052: }
Can you add a test that reflects how CertVerifier and friends will be providing
hashes?

Namely that the hashes are

ee-sha1
ee-sha256
intermediate-sha1
intermediate-sha256
root-sha1
root-sha256

And make sure they behave as appropriate?

Note, you can probably manage this easier as a parameterized test / or using a
test helper structure, where you give the hashes as an array and the expected
result, but that's up to you.

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.cc
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.cc...
net/base/x509_cert_types.cc:205: }
nit: This format for labels - is it intrinsic to all HashValues, or is it
specific to the parsing code(s)

Should this be moved elsewhere? In particular, the choice of delimiter implies
some sort of structured form, I fear.

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h#...
net/base/x509_cert_types.h:43: bool operator() (const SHA1HashValue& lhs,
nit: "operator() (" -> "operator()("

See also line 95

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h#...
net/base/x509_cert_types.h:103: return memcmp(lhs.data(), rhs.data(),
std::min(lhs_size, rhs_size)) < 0;
nit: remove use of std::min, you've already guaranteed they're equal (line 100).
Just use lhs_size

[hshi1, 2012-08-25 01:26:55]

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:251: HashValueVector* fingerprints) {
nit: Now that FingerprintVector is renamed to HashValueVector, the argument name
"fingerprints" is no longer appropriate, how about "hashes".

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:254: HashValue fp;
nit: Now that SHA1Fingerprint is renamed to HashValue, the local variable name
"fp" makes no sense. How about "hash" or "hv" (for HashValue)?

[palmer, 2012-08-29 00:01:24]

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_n...
File net/base/cert_verify_proc_nss.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_n...
net/base/cert_verify_proc_nss.cc:576: DCHECK_EQ(rv, SECSuccess);
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> nit: DCHECK_EQ(expected, actual)

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_u...
File net/base/cert_verify_proc_unittest.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_proc_u...
net/base/cert_verify_proc_unittest.cc:401: HashValue fingerprint;
> nit:
> HashValue fingerprint(HASH_VALUE_SHA1);

Done, plus s/fingerprint/hash/ per hshi1's comment in another file.

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/cert_verify_result...
net/base/cert_verify_result.h:51: // first element of each sub-vector.
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> nit: update comment (re: sub-vector)

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
File net/base/transport_security_state.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:251: HashValueVector* fingerprints) {
On 2012/08/25 01:26:55, hshi1 wrote:
> nit: Now that FingerprintVector is renamed to HashValueVector, the argument
name
> "fingerprints" is no longer appropriate, how about "hashes".

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:254: HashValue fp;
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> nit: Move this to line 261

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:254: HashValue fp;
On 2012/08/25 01:26:55, hshi1 wrote:
> nit: Now that SHA1Fingerprint is renamed to HashValue, the local variable name
> "fp" makes no sense. How about "hash" or "hv" (for HashValue)?

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:267: return false;
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> Seems like you should be passing in the expected type as an input parameter to
> ParseAndAppendPin, rather than trying to automagically discover it.
> 
> Perhaps move the parsing of algorithm name & type into a single method, and
just
> do something like
> 
> StartsWithASCII(token, "sha1-"...

Agreed and done. See also the related changes to the one call site in this file.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state.cc:801: HashesIntersect(static_spki_hashes,
hashes)) {
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> BUG? See previous comments about the loss of half of the conditional
> 
> !(dynamic_spki_hashes.empty() && static_spki_hashes.empty())

Currently, this function is called iff DomainState.HasPins returns true, in its
one call site in net/socket/ssl_client_socket_nss.cc.

However, everybody likes defensive code and the check is cheap, so I am adding
it here too.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
File net/base/transport_security_state_unittest.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:110:
base::SHA1HashBytes(reinterpret_cast<const unsigned char*>(spki.data()),
> does a static_cast work here?

error: invalid static_cast from type ‘const char*’ to type ‘const unsigned
char*’

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:975: // to equifax_sha{1,256}. Bad
chains chain up to equifa through
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> equifax

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/transport_security...
net/base/transport_security_state_unittest.cc:1052: }
> Can you add a test that reflects how CertVerifier and friends will be
providing
> hashes?
> 
> Namely that the hashes are
> 
> ee-sha1
> ee-sha256
> intermediate-sha1
> intermediate-sha256
> root-sha1
> root-sha256
> 
> And make sure they behave as appropriate?

It's not clear to me that we should guarantee a particular ordering for either
public key hashes or hash algorithms. People should treat these as bags or sets.

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.cc
File net/base/x509_cert_types.cc (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.cc...
net/base/x509_cert_types.cc:205: }
> nit: This format for labels - is it intrinsic to all HashValues, or is it
> specific to the parsing code(s)
> 
> Should this be moved elsewhere? In particular, the choice of delimiter implies
> some sort of structured form, I fear.

Yeah, you are right. Moved to a static method in TransportSecurityState, so that
everyone who needs it can get at it.

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h#...
net/base/x509_cert_types.h:43: bool operator() (const SHA1HashValue& lhs,
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> nit: "operator() (" -> "operator()("
> 
> See also line 95

Done.

http://codereview.chromium.org/10826257/diff/8048/net/base/x509_cert_types.h#...
net/base/x509_cert_types.h:103: return memcmp(lhs.data(), rhs.data(),
std::min(lhs_size, rhs_size)) < 0;
On 2012/08/23 22:48:20, Ryan Sleevi wrote:
> nit: remove use of std::min, you've already guaranteed they're equal (line
100).
> Just use lhs_size

Done.

[Ryan Sleevi, 2012-08-29 01:02:02]

LGTM.

I wasn't clear if you were going to touch the tests in this round, but I don't
think it's critical at this point.

http://codereview.chromium.org/10826257/diff/21001/net/base/cert_verify_result.h
File net/base/cert_verify_result.h (right):

http://codereview.chromium.org/10826257/diff/21001/net/base/cert_verify_resul...
net/base/cert_verify_result.h:48: // If the certificate was successfully
verified then this contains the
double check nit: Based on the XP+ECDSA crash, is the "If the certificate was
successfully verified" still correct?

[palmer, 2012-08-30 20:38:01]

Adding cbentzel for chrome/browser/net and sky for chrome/browser/ui review.
Thank you!

[sky, 2012-08-30 21:12:10]

LGTM

[cbentzel, 2012-08-31 00:38:00]

chrome/browser/net LGTM

http://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transpo...
File chrome/browser/net/transport_security_persister.cc (right):

http://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transpo...
chrome/browser/net/transport_security_persister.cc:37:
base::Base64Encode(hash_str, &b64);
I notice that you aren't handling the return boolean of Base64Encode here -
should you?

It could be that the boolean return value for Base64Encode is misguided. Looking
at modp_b64_encode [which it calls], it should always be returning true.

http://codereview.chromium.org/10826257/diff/21001/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

http://codereview.chromium.org/10826257/diff/21001/net/base/x509_cert_types.h...
net/base/x509_cert_types.h:85: union {
It's been a while since I've seen the poor man's polymorphism.

[palmer, 2012-09-04 23:23:33]

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:37:
base::Base64Encode(hash_str, &b64);
> It could be that the boolean return value for Base64Encode is misguided.
Looking
> at modp_b64_encode [which it calls], it should always be returning true. 

I agree. Would you like me to check Base64Encode's return value anyway, just for
good form?

https://codereview.chromium.org/10826257/diff/21001/net/base/cert_verify_resu...
File net/base/cert_verify_result.h (right):

https://codereview.chromium.org/10826257/diff/21001/net/base/cert_verify_resu...
net/base/cert_verify_result.h:48: // If the certificate was successfully
verified then this contains the
On 2012/08/29 01:02:02, Ryan Sleevi wrote:
> double check nit: Based on the XP+ECDSA crash, is the "If the certificate was
> successfully verified" still correct?

I think it is, because https://chromiumcodereview.appspot.com/10879097/ doesn't
seem to impinge on what AppendPublicKeyHashes does in cert_verify_proc_*.cc.

https://codereview.chromium.org/10826257/diff/21001/net/base/x509_cert_types.h
File net/base/x509_cert_types.h (right):

https://codereview.chromium.org/10826257/diff/21001/net/base/x509_cert_types....
net/base/x509_cert_types.h:85: union {
> It's been a while since I've seen the poor man's polymorphism.

I also like to listen to classic jazz and blues on wax cylinders. :)

[cbentzel, 2012-09-05 13:05:47]

LGTM

I'm on vacation this week, so please don't wait for me for any further approval.

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:37:
base::Base64Encode(hash_str, &b64);
On 2012/09/04 23:23:33, Chris P. wrote:
> > It could be that the boolean return value for Base64Encode is misguided.
> Looking
> > at modp_b64_encode [which it calls], it should always be returning true. 
> 
> I agree. Would you like me to check Base64Encode's return value anyway, just
for
> good form?

For now, yes. Probably should fix the function in the long-term.

[palmer, 2012-09-05 22:23:25]

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
File chrome/browser/net/transport_security_persister.cc (right):

https://codereview.chromium.org/10826257/diff/21001/chrome/browser/net/transp...
chrome/browser/net/transport_security_persister.cc:37:
base::Base64Encode(hash_str, &b64);
On 2012/09/05 13:05:47, cbentzel wrote:
> On 2012/09/04 23:23:33, Chris P. wrote:
> > > It could be that the boolean return value for Base64Encode is misguided.
> > Looking
> > > at modp_b64_encode [which it calls], it should always be returning true. 
> > 
> > I agree. Would you like me to check Base64Encode's return value anyway, just
> for
> > good form?
> 
> For now, yes. Probably should fix the function in the long-term.

Done.

[commit-bot: I haz the power, 2012-09-05 22:25:46]

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

[commit-bot: I haz the power, 2012-09-05 22:26:07]

Failed to request the patch to try. Please note that binary filesare still
unsupported at the moment, this is being worked on.

Thanks for your patience.

HTTP Error 500: 
<html><head>
<meta http-equiv="content-type" content="text/html;charset=utf-8">
<title>500 Server Error</title>
</head>
<body text=#000000 bgcolor=#ffffff>
<h1>Error: Server Error</h1>
<h2>The server encountered an error and could not complete your request.<p>If
the problem persists, please <A
HREF="http://code.google.com/appengine/community.html">report</A> your problem
and mention this error message and the query that caused it.</h2>
<h2></h2>
</body></html>

[commit-bot: I haz the power, 2012-09-06 18:40:22]

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

[commit-bot: I haz the power, 2012-09-06 21:40:15]

Try job failure for 10826257-40001 (retry) on mac_rel for step
"sync_integration_tests".
It's a second try, previously, step "sync_integration_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...

[commit-bot: I haz the power, 2012-09-06 21:51:02]

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

[commit-bot: I haz the power, 2012-09-06 22:36:50]

Try job failure for 10826257-40001 (retry) (retry) on win for step "runhooks"
(clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win&number...

[commit-bot: I haz the power, 2012-09-06 22:51:53]

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

[commit-bot: I haz the power, 2012-09-07 12:52:28]

Change committed as 155365