Implement RSAPrivateKey::Copy()

Implement RSAPrivateKey::Copy()

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

[Sergey Ulanov, 2011-11-29 21:56:08]

wtc: ping

[wtc, 2011-11-30 00:58:10]

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.cc
File crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.c...
crypto/rsa_private_key_mac.cc:15: bool CopyCssmKey(const CSSM_KEY& source,
CSSM_KEY* destination) {

Nit: CopyCssmKey => CopyCSSMKey

rsleevi: is there a CSSM function for copying a CSSM_KEY?

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
File crypto/rsa_private_key_unittest.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
crypto/rsa_private_key_unittest.cc:54: ASSERT_EQ(privkey1, privkey_copy);

It would be nice to create a separate test for the Copy()
method.

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_win.cc
File crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_win.c...
crypto/rsa_private_key_win.cc:139: if (!copy->InitProvider())

Instead of calling copy->InitProvider(), I think it is better
to just call
  copy->provider_ = CryptContextAddRef(provider_, NULL, 0);

[wtc, 2011-11-30 00:59:00]

Patch Set 1 LGTM.

rsleevi: I have a question for you in my previous email on
this code review.

[Ryan Sleevi, 2011-11-30 02:05:09]

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.cc
File crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.c...
crypto/rsa_private_key_mac.cc:15: bool CopyCssmKey(const CSSM_KEY& source,
CSSM_KEY* destination) {
On 2011/11/30 00:58:10, wtc wrote:
> 
> Nit: CopyCssmKey => CopyCSSMKey
> 
> rsleevi: is there a CSSM function for copying a CSSM_KEY?

No.

Further, it's not necessarily safe to copy KeyData here. According to the
CDSA/CSSM Version 2 spec, KeyData is CSP-specific and applications should not
rely on any particular behaviour.

The salient details are conveyed in C.2.1.1/C.2.1.2 (Key Formats & References)
and C.4 (Memory Management), of the "Cryptographic Service Provider Behaviour"
Appendix.

This is why I used SecKeyRef for the NSS client-side SSL patches - it exposed a
sane refcounting API.

You could potentially use Wrap and Unwrap operations, but that may not work if
the key isn't extractable. The "correct" behaviour (read: 500+ lines of code,
IIRC) is to search through the DLDB for the key if/when you need it. Not fun.

Unfortunately, I don't have any good recommendations/suggestions for how to
implement this on the CDSA/CSSM side.

That said, putting on the "But we only use Security.framework" hat, there is the
private API SecKeyCreateWithCSSMKey(), which does effectively cheat and dupe the
CSSM_KEY. 

You have to be careful though - if a caller does CSSM_FreeKey, they could free
the reference (stored in .Data) that is backing your key. Bad things can happen
- which is why that API is private.

[wtc, 2011-11-30 02:37:15]

rsleevi: thank you for answering my question.

Do you think we should instead make RSAPrivateKey a
ref-counted type?

[Ryan Sleevi, 2011-11-30 02:42:37]

On 2011/11/30 02:37:15, wtc wrote:
> rsleevi: thank you for answering my question.
> 
> Do you think we should instead make RSAPrivateKey a
> ref-counted type?

Yes to RefCounted, but no to RefCountedThreadSafe. Certainly, the Win/Mac
implementations aren't safe for simultaneous sharing.

[Wez, 2011-11-30 20:51:50]

On 2011/11/30 02:42:37, Ryan Sleevi wrote:
> On 2011/11/30 02:37:15, wtc wrote:
> > rsleevi: thank you for answering my question.
> > 
> > Do you think we should instead make RSAPrivateKey a
> > ref-counted type?
> 
> Yes to RefCounted, but no to RefCountedThreadSafe. Certainly, the Win/Mac
> implementations aren't safe for simultaneous sharing.

It feels like we're exposing implementation detail unnecessarily if we take this
route.  All the calling code wants is to be told the private key it should use. 
In some cases, that private key may be used several times, potentially across
several threads.

If we think of RSAPrivateKey as a thread-specific instance of the private key,
then we could pass around RSAPrivateKeyProviders, which would be ref-counted and
thread-safe, and would do the same work that we originally did to fetch the key
data, i.e. the 500+ lines of non-fun.

[Sergey Ulanov, 2011-11-30 22:30:03]

I rewrote the mac version to export key to an array and then import from that
array.

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.cc
File crypto/rsa_private_key_mac.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_mac.c...
crypto/rsa_private_key_mac.cc:15: bool CopyCssmKey(const CSSM_KEY& source,
CSSM_KEY* destination) {
On 2011/11/30 00:58:10, wtc wrote:
> 
> Nit: CopyCssmKey => CopyCSSMKey

Removed this function

> 
> rsleevi: is there a CSSM function for copying a CSSM_KEY?

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
File crypto/rsa_private_key_unittest.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
crypto/rsa_private_key_unittest.cc:54: ASSERT_EQ(privkey1, privkey_copy);
On 2011/11/30 00:58:10, wtc wrote:
> 
> It would be nice to create a separate test for the Copy()
> method.

Done.

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_win.cc
File crypto/rsa_private_key_win.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_win.c...
crypto/rsa_private_key_win.cc:139: if (!copy->InitProvider())
On 2011/11/30 00:58:10, wtc wrote:
> 
> Instead of calling copy->InitProvider(), I think it is better
> to just call
>   copy->provider_ = CryptContextAddRef(provider_, NULL, 0);

Done.

[Sergey Ulanov, 2011-11-30 22:35:23]

On 2011/11/30 02:37:15, wtc wrote:
> rsleevi: thank you for answering my question.
> 
> Do you think we should instead make RSAPrivateKey a
> ref-counted type?

I think that because RSAPrivateKey instances are mutable Copy() would be useful
even if this class was ref-counted. Also changing this class to be ref-counted
would require fixing all code that uses it.

[wtc, 2011-11-30 23:42:37]

On 2011/11/30 22:35:23, sergeyu wrote:
>
> I think that because RSAPrivateKey instances are mutable Copy() would be
useful
> even if this class was ref-counted.

Could you explain why RSAPrivateKey instances are mutable?

> Also changing this class to be ref-counted
> would require fixing all code that uses it.

That's true.

[wtc, 2011-11-30 23:44:37]

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
File crypto/rsa_private_key_unittest.cc (right):

http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
crypto/rsa_private_key_unittest.cc:54: ASSERT_EQ(privkey1, privkey_copy);
On 2011/11/30 22:30:03, sergeyu wrote:
>
> Done.

You didn't upload a new patch set.  Not sure if you forgot
to do that or you're still working on this CL.

[Sergey Ulanov, 2011-11-30 23:49:08]

On 2011/11/30 23:44:37, wtc wrote:
>
http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
> File crypto/rsa_private_key_unittest.cc (right):
> 
>
http://codereview.chromium.org/8727014/diff/1025/crypto/rsa_private_key_unitt...
> crypto/rsa_private_key_unittest.cc:54: ASSERT_EQ(privkey1, privkey_copy);
> On 2011/11/30 22:30:03, sergeyu wrote:
> >
> > Done.
> 
> You didn't upload a new patch set.  Not sure if you forgot
> to do that or you're still working on this CL.

Done. Sorry about it.

[Sergey Ulanov, 2011-11-30 23:50:02]

On 2011/11/30 23:42:37, wtc wrote:
> On 2011/11/30 22:35:23, sergeyu wrote:
> >
> > I think that because RSAPrivateKey instances are mutable Copy() would be
> useful
> > even if this class was ref-counted.
> 
> Could you explain why RSAPrivateKey instances are mutable?

key() and public_key() return non-const pointers.

> 
> > Also changing this class to be ref-counted
> > would require fixing all code that uses it.
> 
> That's true.

[wtc, 2011-12-01 22:40:38]

Patch Set 2 LGTM.

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
File crypto/rsa_private_key_unittest.cc (right):

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:138: TEST(RSAPrivateKeyUnitTest, Copy) {

Nit: Copy => CopyTest

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:141: memcpy(&input.front(),
kTestPrivateKeyInfo, sizeof(kTestPrivateKeyInfo));

Nit: does this work?
  std::vector<uint8> input(kTestPrivateKeyInfo,
                           kTestPrivateKeyInfo + sizeof(kTestPrivateKeyInfo));

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:144:
crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));

You can also just generate a random key and export its key
bytes.  This way you don't need to move kTestPrivateKeyInfo
outside the PublicKeyTest.

[Sergey Ulanov, 2011-12-01 23:05:55]

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
File crypto/rsa_private_key_unittest.cc (right):

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:138: TEST(RSAPrivateKeyUnitTest, Copy) {
On 2011/12/01 22:40:39, wtc wrote:
> 
> Nit: Copy => CopyTest

Done.

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:141: memcpy(&input.front(),
kTestPrivateKeyInfo, sizeof(kTestPrivateKeyInfo));
On 2011/12/01 22:40:39, wtc wrote:
> 
> Nit: does this work?
>   std::vector<uint8> input(kTestPrivateKeyInfo,
>                            kTestPrivateKeyInfo + sizeof(kTestPrivateKeyInfo));

Fixed here and in PublicKeyTest.

http://codereview.chromium.org/8727014/diff/18001/crypto/rsa_private_key_unit...
crypto/rsa_private_key_unittest.cc:144:
crypto::RSAPrivateKey::CreateFromPrivateKeyInfo(input));
On 2011/12/01 22:40:39, wtc wrote:
> 
> You can also just generate a random key and export its key
> bytes.  This way you don't need to move kTestPrivateKeyInfo
> outside the PublicKeyTest.

Yes, but generating a key pair takes long time (up to several seconds) and it
would slow down the test.

[commit-bot: I haz the power, 2011-12-01 23:13:50]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8727014/21005

[commit-bot: I haz the power, 2011-12-02 00:15:43]

Try job failure for 8727014-21005 (previous was lost) (retry) on win_rel for
step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-02 00:59:34]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8727014/23006

[commit-bot: I haz the power, 2011-12-02 02:08:59]

Try job failure for 8727014-23006 (previous was lost) (retry) (previous was
lost) on win_rel for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2011-12-02 20:41:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/sergeyu@chromium.org/8727014/23007

[commit-bot: I haz the power, 2011-12-02 22:03:19]

Try job failure for 8727014-23007 (retry) (previous was lost) (previous was
lost) on mac_rel for step "compile" (clobber build).
It's a second try, previously, step "compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=mac_rel&nu...