Refactor ASN1 parsing/serializationg

Refactor ASN1 parsing/serialization

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

[rafaelw, 2009-10-03 00:25:12]

+wtc (cc)

[rafaelw, 2009-10-03 00:28:56]

+cevans (also cc)

[Aaron Boodman, 2009-10-03 00:44:09]

http://codereview.chromium.org/242136/diff/1/4
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/1/4#newcode27
Line 27: // Used internally by RSAPrivateKeyInfo for serializing and
deserializing
You mean RSAPrivateKey.

http://codereview.chromium.org/242136/diff/1/4#newcode29
Line 29: class PrivateKeyInfo {
Naming suggestion: PrivateKeyInfoEncoder?

http://codereview.chromium.org/242136/diff/1/4#newcode42
Line 42: PrivateKeyInfo() {}
These two lines can be removed.

http://codereview.chromium.org/242136/diff/1/4#newcode62
Line 62: void PrependBytesInOrder(uint8* val, int start, int num_bytes,
The phrase "InOrder" is not needed here.

http://codereview.chromium.org/242136/diff/1/4#newcode93
Line 93: // Direct access & modification of the components of the PrivateKeyInfo
are
This comment confuses me because these members are not public.

http://codereview.chromium.org/242136/diff/1/5
File base/crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/242136/diff/1/5#newcode141
Line 141: return (private_key_info.Import(private_key_info_) &&
I don't see where private_key_info_ is defined.

http://codereview.chromium.org/242136/diff/1/7
File base/crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/242136/diff/1/7#newcode40
Line 40: while (num_bytes > 1 && val[num_bytes - 1] == 0x00)
It's a shame to have this logic in two places. What about just making a copy of
the input buffer, reversing it in-place, then passing that to the methods in the
base class?

http://codereview.chromium.org/242136/diff/1/7#newcode66
Line 66: // The last byte can be zero to force positiveness. We can ignore this.
Same here. In this case you can pass the output buffer to the base class, then
reverse the bytes inside the buffer.

[wtc, 2009-10-05 18:59:08]

Rafael,

Since Aaron did a detailed review, I did a cursory review.
I have some suggested changes below.  Note: I'm not sure if
the new name I suggested for the PrivateKeyInfo class is
better than the current name.  But if you can come up with
a name that suggests the decoding/encoding aspects of that
class, that'll be great.

http://codereview.chromium.org/242136/diff/1/3
File base/crypto/rsa_private_key.cc (right):

http://codereview.chromium.org/242136/diff/1/3#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
Please use
  svn cp rsa_private_key_win.cc rsa_private_key.cc
to add this file so that the SVN history of the code is
preserved.

http://codereview.chromium.org/242136/diff/1/3#newcode335
Line 335: }  // namespace
This comment should say
  // namespace base

http://codereview.chromium.org/242136/diff/1/4
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/1/4#newcode29
Line 29: class PrivateKeyInfo {
How about naming this class RSAKeyCodec or RSAPrivateKeyCodec?
Because
1. It supports only RSA keys
2. It handles both private and public keys
3. It does both encoding and decoding

Consider moving this class to its own files.

http://codereview.chromium.org/242136/diff/1/4#newcode53
Line 53: //implementation can override for little-endian support.
Nit: add a space after //.  (cpplint.py should have warned
about this.)

http://codereview.chromium.org/242136/diff/1/4#newcode54
Line 54: virtual void PrependInteger(const std::vector<uint8>& in,
Please format these function declarations using one of the
forms recommended by the style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/242136/diff/1/6
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/242136/diff/1/6#newcode23
Line 23: static bool ReadAttribute(SECKEYPrivateKey* key, CK_ATTRIBUTE_TYPE
type,
Please format this function definition according to the
style guide:
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

http://codereview.chromium.org/242136/diff/1/6#newcode108
Line 108: if (!ReadAttribute(key_, CKA_COEFFICIENT,
&private_key_info.coefficient_) ||
Nit: If you have some spare time, would be nice to list these
components in the normal order.  (This suggested change is
optional.)

[rafaelw, 2009-10-06 21:57:47]

Based on Aaron's comment about simply reversing the order of component integers
for the windows little-endian PrivateKeyInfoCodec, I changed it's implementation
to take a |big_endian| argument, rather than have a private subclass.

http://codereview.chromium.org/242136/diff/1/3
File base/crypto/rsa_private_key.cc (right):

http://codereview.chromium.org/242136/diff/1/3#newcode1
Line 1: // Copyright (c) 2009 The Chromium Authors. All rights reserved.
On 2009/10/05 18:59:08, wtc wrote:
> Please use
>   svn cp rsa_private_key_win.cc rsa_private_key.cc
> to add this file so that the SVN history of the code is
> preserved.

I don't think I have a good way to do that with git. The diffs from the
rsa_private_key_win will be significant.

I can make an svn checkout, do the copy and apply there so as to retain the
history. Let me know if you think it's worth it.

http://codereview.chromium.org/242136/diff/1/3#newcode335
Line 335: }  // namespace
On 2009/10/05 18:59:08, wtc wrote:
> This comment should say
>   // namespace base

Done.

http://codereview.chromium.org/242136/diff/1/4
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/1/4#newcode27
Line 27: // Used internally by RSAPrivateKeyInfo for serializing and
deserializing
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> You mean RSAPrivateKey.

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode29
Line 29: class PrivateKeyInfo {
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> Naming suggestion: PrivateKeyInfoEncoder?

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode29
Line 29: class PrivateKeyInfo {
On 2009/10/05 18:59:08, wtc wrote:
> How about naming this class RSAKeyCodec or RSAPrivateKeyCodec?
> Because
> 1. It supports only RSA keys
> 2. It handles both private and public keys
> 3. It does both encoding and decoding
> 
> Consider moving this class to its own files.

I mixed the two suggestions: PrivateKeyInfoCodec. Reasoning: it's encoding and
decoding. It's mostly dealing with der encodings of PrivateKeyInfo.

Also, wtc, let me know if you feel strongly about seperate files. I had
intentionally designed this to be an internal class only for use by the
RSAPrivateKey class.

http://codereview.chromium.org/242136/diff/1/4#newcode42
Line 42: PrivateKeyInfo() {}
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> These two lines can be removed.

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode53
Line 53: //implementation can override for little-endian support.
On 2009/10/05 18:59:08, wtc wrote:
> Nit: add a space after //.  (cpplint.py should have warned
> about this.)

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode54
Line 54: virtual void PrependInteger(const std::vector<uint8>& in,
On 2009/10/05 18:59:08, wtc wrote:
> Please format these function declarations using one of the
> forms recommended by the style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode62
Line 62: void PrependBytesInOrder(uint8* val, int start, int num_bytes,
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> The phrase "InOrder" is not needed here.

Done.

http://codereview.chromium.org/242136/diff/1/4#newcode93
Line 93: // Direct access & modification of the components of the PrivateKeyInfo
are
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> This comment confuses me because these members are not public.

Done.

http://codereview.chromium.org/242136/diff/1/5
File base/crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/242136/diff/1/5#newcode141
Line 141: return (private_key_info.Import(private_key_info_) &&
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> I don't see where private_key_info_ is defined.

chris hawk changed the implementation of rsa_private_key_mac, which this patch
was waiting on. I've updated this patch to import from the CSSM_KEY, rather than
the old private_key_info_.

http://codereview.chromium.org/242136/diff/1/6
File base/crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/242136/diff/1/6#newcode23
Line 23: static bool ReadAttribute(SECKEYPrivateKey* key, CK_ATTRIBUTE_TYPE
type,
On 2009/10/05 18:59:08, wtc wrote:
> Please format this function definition according to the
> style guide:
>
http://google-styleguide.googlecode.com/svn/trunk/cppguide.xml#Function_Decla...

Done.

http://codereview.chromium.org/242136/diff/1/7
File base/crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/242136/diff/1/7#newcode40
Line 40: while (num_bytes > 1 && val[num_bytes - 1] == 0x00)
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> It's a shame to have this logic in two places. What about just making a copy
of
> the input buffer, reversing it in-place, then passing that to the methods in
the
> base class?

Done.

http://codereview.chromium.org/242136/diff/1/7#newcode66
Line 66: // The last byte can be zero to force positiveness. We can ignore this.
On 2009/10/03 00:44:09, Aaron Boodman wrote:
> Same here. In this case you can pass the output buffer to the base class, then
> reverse the bytes inside the buffer.

Done.

[Aaron Boodman, 2009-10-06 23:36:26]

Nice work.

http://codereview.chromium.org/242136/diff/2004/2007
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/2004/2007#newcode43
Line 43: bool Export(std::vector<uint8>* output);
Put some comments on here explaining which struct is being imported/exported?
(for the public key it is obvious, but not the others).

http://codereview.chromium.org/242136/diff/2004/2007#newcode120
Line 120: std::vector<uint8> modulus_;
I think it is sensible (and probably improves future testability) to just expose
these.

It makes sense to me that a Codec class would decode input and expose its
component parts.

[Aaron Boodman, 2009-10-06 23:42:02]

http://codereview.chromium.org/242136/diff/2004/2007
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/2004/2007#newcode42
Line 42: explicit PrivateKeyInfoCodec(bool big_endian) : big_endian_(big_endian)
{}
On this one, I think it is worth clarifying that the big_endian is wrt the byte
ordering of the members prime1_, prime2_, etc. (actually that is one reason
again why those seem like they should be public).

[rafaelw, 2009-10-07 00:50:32]

http://codereview.chromium.org/242136/diff/2004/2007
File base/crypto/rsa_private_key.h (right):

http://codereview.chromium.org/242136/diff/2004/2007#newcode42
Line 42: explicit PrivateKeyInfoCodec(bool big_endian) : big_endian_(big_endian)
{}
On 2009/10/06 23:42:02, Aaron Boodman wrote:
> On this one, I think it is worth clarifying that the big_endian is wrt the
byte
> ordering of the members prime1_, prime2_, etc. (actually that is one reason
> again why those seem like they should be public).

Done.

http://codereview.chromium.org/242136/diff/2004/2007#newcode43
Line 43: bool Export(std::vector<uint8>* output);
On 2009/10/06 23:36:26, Aaron Boodman wrote:
> Put some comments on here explaining which struct is being imported/exported?
> (for the public key it is obvious, but not the others).

Done.

http://codereview.chromium.org/242136/diff/2004/2007#newcode120
Line 120: std::vector<uint8> modulus_;
On 2009/10/06 23:36:26, Aaron Boodman wrote:
> I think it is sensible (and probably improves future testability) to just
expose
> these.
> 
> It makes sense to me that a Codec class would decode input and expose its
> component parts.

Done.

[Aaron Boodman, 2009-10-07 00:59:35]

Cool. LGTM.

[wtc, 2009-10-07 02:38:27]

Rafael,

I don't have time to review this CL again, so please
feel free to ignore any of my suggested changes.