crypto: convert OpenPGP code to NSS

crypto: convert OpenPGP code to NSS

BUG=none
TEST=crypto_unittests

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

[agl, 2011-07-01 14:19:41]

On Wed, Jun 29, 2011 at 6:04 PM,  <agl@chromium.org> wrote:
> Reviewers: wtc,

bump.


Cheers

AGL

[wtc, 2011-07-01 18:33:32]

LGTM.  I suggested some changes below.

http://codereview.chromium.org/7273080/diff/1/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/7273080/diff/1/crypto/crypto.gyp#newcode186
crypto/crypto.gyp:186: 'openpgp_symmetric_encryption_test_nss.cc',
This unit test file should be named
openpgp_symmetric_encryption_nss_unittest.cc to match
our naming convention.

Maybe it doesn't need _nss in the name because the tests
don't depend on NSS.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_nss.cc (right):

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:7: #include <vector>
<vector> is a C++ system header, so it should be listed
after the C system headers.  We can treat the NSS headers
as system headers.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:109: const struct SECHashObjectStr*
hash = HASH_GetHashObject(hash_function);
struct SECHashObjectStr => SECHashObject

Please search for "struct" in this file and fix all other
occurrences of this problem.

Rather than calling the methods of the 'hash' object, I
suggest that you use the HASH_Create, HASH_Begin, HASH_Update,
HASH_End, and HASH_Destroy functions.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:150: bool SetKey(const uint8* key,
unsigned key_len, ScopedPK11Context* out_key) {
This function should be named CreateAESContext.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:193: ScopedPK11Context key;
"key" is a strange name for a PK11Context.  I think
aes_context or decryption_context would be a better name.

This comment also applies to the other PK11Context
variable/parameter names in this file.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:362: *out_length = length32;
BUG: this is after the return statement.  I think you
intended to do something like:

  uint32 length32;
  if (reader->U32(&length32)) {
    *out_length = length32;
    return true;
  }
  return false;
 
Nit: we usually don't use "else" after a return statement.
But the code here looks nicer this way.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:406: // fallthrough
Nit: fallthrough => Fall through.

[agl, 2011-07-01 20:04:59]

Thanks!

http://codereview.chromium.org/7273080/diff/1/crypto/crypto.gyp
File crypto/crypto.gyp (right):

http://codereview.chromium.org/7273080/diff/1/crypto/crypto.gyp#newcode186
crypto/crypto.gyp:186: 'openpgp_symmetric_encryption_test_nss.cc',
On 2011/07/01 18:33:32, wtc wrote:
> This unit test file should be named
> openpgp_symmetric_encryption_nss_unittest.cc to match
> our naming convention.
> 
> Maybe it doesn't need _nss in the name because the tests
> don't depend on NSS.

Done.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
File crypto/openpgp_symmetric_encryption_nss.cc (right):

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:7: #include <vector>
On 2011/07/01 18:33:32, wtc wrote:
> <vector> is a C++ system header, so it should be listed
> after the C system headers.  We can treat the NSS headers
> as system headers.

Done.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:109: const struct SECHashObjectStr*
hash = HASH_GetHashObject(hash_function);
On 2011/07/01 18:33:32, wtc wrote:
> struct SECHashObjectStr => SECHashObject
> 
> Please search for "struct" in this file and fix all other
> occurrences of this problem.
> 
> Rather than calling the methods of the 'hash' object, I
> suggest that you use the HASH_Create, HASH_Begin, HASH_Update,
> HASH_End, and HASH_Destroy functions.

Done.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:150: bool SetKey(const uint8* key,
unsigned key_len, ScopedPK11Context* out_key) {
On 2011/07/01 18:33:32, wtc wrote:
> This function should be named CreateAESContext.

Done.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:193: ScopedPK11Context key;
On 2011/07/01 18:33:32, wtc wrote:
> "key" is a strange name for a PK11Context.  I think
> aes_context or decryption_context would be a better name.
> 
> This comment also applies to the other PK11Context
> variable/parameter names in this file.

Done.

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:362: *out_length = length32;
On 2011/07/01 18:33:32, wtc wrote:
> BUG: this is after the return statement.

Thanks!

http://codereview.chromium.org/7273080/diff/1/crypto/openpgp_symmetric_encryp...
crypto/openpgp_symmetric_encryption_nss.cc:406: // fallthrough
On 2011/07/01 18:33:32, wtc wrote:
> Nit: fallthrough => Fall through.

Done.

[commit-bot: I haz the power, 2011-07-01 20:05:08]

Can't process patch for file
crypto/openpgp_symmetric_encryption_nss_unittest.cc.
A +