Implement the signature verification API.

Implement the signature verification API.

R=erikkay,avi
BUG=6305
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=13508

[wtc, 2009-04-07 23:57:18]

erikkay: please review all files.  You may want to focus
on the API specified in signature_verifier.h and how it is
used in signature_verifier_unittest.cc.

avi: please review the following files:
  base/base.gyp
  base/crypto/cssm_init.cc
  base/crypto/cssm_init.h
  base/crypto/signature_verifier.h
  base/crypto/signature_verifier_mac.cc

[Erik does not do reviews, 2009-04-08 00:36:43]

Mostly nits and minor issues since I don't know these libraries.

http://codereview.chromium.org/63089/diff/1010/1011
File base/base.gyp (right):

http://codereview.chromium.org/63089/diff/1010/1011#newcode429
Line 429: 'crypto/cssm_init.cc',
Since these are mac-specific files, isn't the convention to use .m instead of
.cc?  That way they could use objective-c if they wanted to (in the future).

http://codereview.chromium.org/63089/diff/1010/1013
File base/crypto/signature_verifier_mac.cc (right):

http://codereview.chromium.org/63089/diff/1010/1013#newcode14
Line 14: namespace {
It feels weird to have this anonymous namespace just inside of the base
namespace.  No biggie, but I'd personally move it outside.

http://codereview.chromium.org/63089/diff/1010/1013#newcode124
Line 124: // crtn is CSSMERR_CSP_VERIFY_FAILED if signature verification fails.
newline above comment

http://codereview.chromium.org/63089/diff/1010/1013#newcode136
Line 136: return false;
Looks like you forgot the implementation here.

http://codereview.chromium.org/63089/diff/1010/1018
File base/crypto/signature_verifier_nss.cc (right):

http://codereview.chromium.org/63089/diff/1010/1018#newcode92
Line 92: SECStatus rv =  VFY_Update(vfy_context_, data_part, data_part_len);
extra space after =

http://codereview.chromium.org/63089/diff/1010/1018#newcode99
Line 99: // If signature verification fails, the error code is
newline above comment

http://codereview.chromium.org/63089/diff/1010/1018#newcode112
Line 112: return false;
again missing implementation.

http://codereview.chromium.org/63089/diff/1010/1012
File base/crypto/signature_verifier_unittest.cc (right):

http://codereview.chromium.org/63089/diff/1010/1012#newcode146
Line 146: // algotithm
typo

http://codereview.chromium.org/63089/diff/1010/1012#newcode147
Line 147: 0x30, 0x0d,  // a SEQUENCE of length 13
the whitespace in this struct (and some of the others) is very odd.  is there a
purpose to it?

http://codereview.chromium.org/63089/diff/1010/1012#newcode206
Line 206: verifier.VerifyUpdate(tbs_cert + 256, 256);
this code feels awkward.  maybe a loop?  it might also be worth adding a comment
explaining the purpose of testing multiple calls to update vs. a single large
call.  I get it, but might not be obvious to casual reader.

http://codereview.chromium.org/63089/diff/1010/1012#newcode230
Line 230: // A crypto library (e.g., NSS) may detect that the signature is
corrupted
newline above comment

http://codereview.chromium.org/63089/diff/1010/1016
File base/crypto/signature_verifier_win.cc (right):

http://codereview.chromium.org/63089/diff/1010/1016#newcode13
Line 13: namespace {
similar namespace nit to before

http://codereview.chromium.org/63089/diff/1010/1016#newcode140
Line 140: return false;
missing impl again (could this impl be platform independent?  just implemented
in terms of the other methods)

http://codereview.chromium.org/63089/diff/1010/1016#newcode155
Line 155: signature_.clear();
you didn't do this clear in the other two impls.  should it be there too?

[Avi (use Gerrit), 2009-04-08 02:02:17]

A few notes beyond what Erik had to say, but he's right on. The missing
implementations are curious.

As for CSSM, you're probably as expert as I am, but the sequence of calls looks
quite plausible.

http://codereview.chromium.org/63089/diff/1010/1011
File base/base.gyp (right):

http://codereview.chromium.org/63089/diff/1010/1011#newcode429
Line 429: 'crypto/cssm_init.cc',
.m = ObjC, .mm = ObjC++, so it'd have to be .mm. But why? It's perfectly valid
C++ as it stands, and Mac files don't have to use ObjC. For now, I'm happy with
.cc.

http://codereview.chromium.org/63089/diff/1010/1012
File base/crypto/signature_verifier_unittest.cc (right):

http://codereview.chromium.org/63089/diff/1010/1012#newcode147
Line 147: 0x30, 0x0d,  // a SEQUENCE of length 13
On 2009/04/08 00:36:43, Erik Kay wrote:
> the whitespace in this struct (and some of the others) is very odd.  is there
a
> purpose to it?

Looks to me like it's mirroring the hierarchical structure of the data. Though
the weird initial indent on the large blocks is odd.

http://codereview.chromium.org/63089/diff/1010/1012#newcode183
Line 183: // public expoent
typo

[wtc, 2009-04-09 20:07:15]

Erik, Avi, thanks a lot for your comments.  I made all of
your suggested changes, and added lots of comments. Please
review the new Patch Set 4.

Here I answer your questions.

1. Mac's CDSA/CSSM is a C API, so we shouldn't need to
write ObjC or ObjC++ code to use the API.

2. I apologize for the weird formatting of the array
initializers in the unit test.  As Avi said, I wanted
to expose the hierarchical ASN.1 structures hidden in
the binary (DER) encoding.  I have reformatted the
array initializers to make them look more sane (but
still hierarchical).

3. The unimplemented Verify method has been removed.
I turned it into a comment to let people know that I
can implement it if there is interest.

[Erik does not do reviews, 2009-04-09 20:23:04]

LGTM.  thanks!