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: https://src.chromium.org/viewvc/chrome?view=rev&revision=150375

[wtc, 2012-08-07 00:46:02]

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

https://chromiumcodereview.appspot.com/10825211/diff/1/net/base/x509_cert_typ...
net/base/x509_cert_types.h:100: return sizeof(fingerprint.sha1.data);

What was the problem that caused the revert?

If the default case cannot be reached, whether we return 0
or sizeof(fingerprint.sha1.data) here should not matter.
I am confused...

[palmer, 2012-08-07 04:41:39]

> What was the problem that caused the revert?

A compiler warning about calling memset with a length argument of 0. It only
appeared sometimes — it didn't even consistently fail.

> If the default case cannot be reached, whether we return 0
> or sizeof(fingerprint.sha1.data) here should not matter.
> I am confused...

Yes. IRC and I were also confused.

[wtc, 2012-08-07 15:00:53]

palmer: which memset call did the compiler warn about?
I searched for "memset" and "size()" in your CL, but
couldn't find a memset call that uses a HashValue object
without initializing its 'tag' member.

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

https://chromiumcodereview.appspot.com/10825211/diff/1/chrome/browser/net/tra...
chrome/browser/net/transport_security_persister_unittest.cc:203: DLOG(WARNING)
<< hashes[net::HASH_VALUE_SHA1].size();

Nit: should these two lines be indented?

[Jeffrey Yasskin, 2012-08-07 17:03:02]

On Mon, Aug 6, 2012 at 9:41 PM,  <palmer@chromium.org> wrote:
> Reviewers: wtc,
>
> Message:
>
>> What was the problem that caused the revert?
>
>
> A compiler warning about calling memset with a length argument of 0. It only
> appeared sometimes — it didn't even consistently fail.

Specifically:

In file included from /usr/include/string.h:640,
                 from ./base/basictypes.h:10,
                 from ./base/string16.h:33,
                 from ./base/hash_tables.h:26,
                 from ./base/file_path.h:109,
                 from ./chrome/browser/net/transport_security_persister.h:38,
                 from
chrome/browser/net/transport_security_persister_unittest.cc:5:
In function 'void* memset(void*, int, size_t)',
    inlined from 'virtual void
TransportSecurityPersisterTest_SerializeData3_Test::TestBody()' at
chrome/browser/net/transport_security_persister_unittest.cc:118:
/usr/include/bits/string3.h:83: error: call to
'__warn_memset_zero_len' declared with attribute warning: memset used
with constant zero length parameter; this could be due to transposed
parameters


gcc emits this warning part-way through optimization, and it looks
like in this case it's decided to copy the memset() into size()'s
switch, but *not* kill off dead switch branches, before checking if
the argument is a constant 0.

>> If the default case cannot be reached, whether we return 0
>> or sizeof(fingerprint.sha1.data) here should not matter.
>> I am confused...
>
>
> Yes. IRC and I were also confused.
>
> Description:
>  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
>
> Please review this at https://chromiumcodereview.appspot.com/10825211/
>
> SVN Base: svn://svn.chromium.org/chrome/trunk/src/
>
> Affected files:
>   M     chrome/browser/net/transport_security_persister.cc
>   M     chrome/browser/net/transport_security_persister_unittest.cc
>   M     chrome/browser/ui/webui/net_internals/net_internals_ui.cc
>   M     net/base/cert_test_util.h
>   M     net/base/cert_test_util.cc
>   M     net/base/cert_verify_proc.h
>   M     net/base/cert_verify_proc.cc
>   M     net/base/cert_verify_proc_mac.cc
>   M     net/base/cert_verify_proc_nss.cc
>   M     net/base/cert_verify_proc_openssl.cc
>   M     net/base/cert_verify_proc_unittest.cc
>   M     net/base/cert_verify_proc_win.cc
>   M     net/base/cert_verify_result.h
>   M     net/base/cert_verify_result.cc
>   M     net/base/ev_root_ca_metadata.h
>   M     net/base/ev_root_ca_metadata.cc
>   M     net/base/ev_root_ca_metadata_unittest.cc
>   M     net/base/multi_threaded_cert_verifier.h
>   M     net/base/multi_threaded_cert_verifier_unittest.cc
>   M     net/base/ssl_info.h
>   M     net/base/ssl_info.cc
>   M     net/base/transport_security_state.h
>   M     net/base/transport_security_state.cc
>   M     net/base/transport_security_state_unittest.cc
>   M     net/base/x509_cert_types.h
>   M     net/base/x509_cert_types.cc
>   M     net/base/x509_certificate.h
>   M     net/base/x509_certificate.cc
>   M     net/base/x509_certificate_mac.cc
>   M     net/base/x509_certificate_nss.cc
>   M     net/base/x509_certificate_openssl.cc
>   M     net/base/x509_certificate_unittest.cc
>   M     net/base/x509_certificate_win.cc
>   M     net/socket/ssl_client_socket_nss.h
>   M     net/socket/ssl_client_socket_nss.cc
>   M     net/url_request/url_request_unittest.cc
>
>

[palmer, 2012-08-07 20:39:35]

>
https://chromiumcodereview.appspot.com/10825211/diff/1/chrome/browser/net/tra...
> chrome/browser/net/transport_security_persister_unittest.cc:203: DLOG(WARNING)
> << hashes[net::HASH_VALUE_SHA1].size();
> 
> Nit: should these two lines be indented?

No, they should be removed! (And have been.)

[wtc, 2012-08-08 18:16:25]

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

https://chromiumcodereview.appspot.com/10825211/diff/12002/net/base/x509_cert...
net/base/x509_cert_types.h:100: return sizeof(fingerprint.sha1.data);

The reason we have to return a non-zero value in the default
case is not obvious, so we should add a comment.

Based on the gcc compiler error message provided by jyasskin,
I think this can be considered a GCC optimization bug.  It
seems that we can avoid this problem by preventing GCC from
inlining this size() method.  Perhaps we can define the size()
method in the .cc file instead?

[palmer, 2012-08-08 18:40:29]

> The reason we have to return a non-zero value in the default
> case is not obvious, so we should add a comment.

Good point. I will add a comment about it when I take another shot at this CL.

> Based on the gcc compiler error message provided by jyasskin,
> I think this can be considered a GCC optimization bug.  It
> seems that we can avoid this problem by preventing GCC from
> inlining this size() method.  Perhaps we can define the size()
> method in the .cc file instead?

I agree that it's a GCC optimization bug. But isn't it the case that the
compiler might inline even a method defined in a .cc file, even if it is not
declared with the inline keyword? It seems like moving it into the .cc file is
*likely* to work, but returning a dummy value is *guaranteed* to work. (By
"work" I mean "successfully work around this compiler bug".)

[wtc, 2012-08-09 01:15:10]

Inlining a method in a .cc file probably will only
happen when whole program optimization is turned on.
The reason I suggested defining the size() method in
a .cc file is that it allows size() to return 0 when
data() returns NULL.  Returning a nonzero value such
as 1 is fine by me, but the reason should be documented.

We should work with one of our GCC/clang people such
as thakis to create a reduced test case and submit a
bug report to GCC.