net: support SHA512 hashes in DNSSEC chains.

net: support SHA512 hashes in DNSSEC chains.

Patch-by: Simon Arlott

BUG=122239
TEST=https://chromium-122239.test.lp0.eu/


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

[Ryan Sleevi, 2012-04-13 19:35:43]

LGTM. A few nits below, as what would a review from me be without them...

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc
File net/base/dnssec_keyset.cc (right):

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:28: const unsigned char kRSAWithSHA512[] = {
nit: It'd be nice to list the OID to save a little spec hunting, since I always
forget where they're defined. These are the RFC 4055 OIDs, right?

{ id-pkcs-1 13 }
1.2.840.113549.1.1.13

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:29: 0x30, 0xd, 0x6, 0x9, 0x2a, 0x86, 0x48, 0x86, 0xf7,
0xd, 0x1, 0x1, 0xd, 5, 0
nit: 0x5 0x0 ?

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:153: sizeof(kRSAWithSHA512));
minor nit: arraysize (here and 145/149)

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:215: spki_der.data = (uint8*) public_key.data();
minor nit: These casts (here, line 233, 248, 269) feel wrong and are
non-compliant w/r/t the open source style guide, although I understand the
motivation for them.

[Ryan Sleevi, 2012-04-17 00:39:14]

Just a follow-up; since you include the patch-by, should you also include the
e-mail address? And I'm assuming you checked the CLA?

[agl, 2012-04-17 15:16:36]

Other Patch-by's don't include the email address. (It's in AUTHORS however.)

I've checked the CLA list.

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc
File net/base/dnssec_keyset.cc (right):

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:28: const unsigned char kRSAWithSHA512[] = {
On 2012/04/13 19:35:43, Ryan Sleevi wrote:
> nit: It'd be nice to list the OID to save a little spec hunting, since I
always
> forget where they're defined. These are the RFC 4055 OIDs, right?
> 
> { id-pkcs-1 13 }
> 1.2.840.113549.1.1.13

Done.

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:29: 0x30, 0xd, 0x6, 0x9, 0x2a, 0x86, 0x48, 0x86, 0xf7,
0xd, 0x1, 0x1, 0xd, 5, 0
On 2012/04/13 19:35:43, Ryan Sleevi wrote:
> nit: 0x5 0x0 ?

Done.

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:153: sizeof(kRSAWithSHA512));
On 2012/04/13 19:35:43, Ryan Sleevi wrote:
> minor nit: arraysize (here and 145/149)

I really do want the number of bytes here.

http://codereview.chromium.org/10082010/diff/1/net/base/dnssec_keyset.cc#newc...
net/base/dnssec_keyset.cc:215: spki_der.data = (uint8*) public_key.data();
On 2012/04/13 19:35:43, Ryan Sleevi wrote:
> minor nit: These casts (here, line 233, 248, 269) feel wrong and are
> non-compliant w/r/t the open source style guide, although I understand the
> motivation for them.

Right. The alternative is such a mess. If you feel strongly, I'll LGTM a change.
But otherwise I'd rather keep the code readable.

[commit-bot: I haz the power, 2012-04-17 17:37:38]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/agl@chromium.org/10082010/12002

[commit-bot: I haz the power, 2012-04-17 19:05:16]

Change committed as 132612