For PKCS#12 imports, only mark key as unextractable if the PKCS#12 file includes it

For PKCS#12 imports, only mark key as unextractable if the PKCS#12 file includes it

This addresses a potential corner case where we end up marking an already
existing private key as unextractable while importing a corresponding
certificate into a hardware (unextractable slot).

BUG=chromium-os:15838
TEST=Added a new unit test


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

[gauravsh, 2011-07-29 01:28:59]

mattm for review, wtc for review/owners approval, rsleevi as FYI

[Ryan Sleevi, 2011-07-29 01:46:27]

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:207:
while(SEC_PKCS12DecoderIterateNext(dcx, &decoder_item) == SECSuccess) {
nit: "while(" -> "while ("

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:216: CERTCertificate*
cert = PK11_FindCertFromDERCertItem(
LEAK: |cert| is leaked (line 229 or line 231)

[gauravsh, 2011-07-29 02:08:47]

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:207:
while(SEC_PKCS12DecoderIterateNext(dcx, &decoder_item) == SECSuccess) {
On 2011/07/29 01:46:27, Ryan Sleevi wrote:
> nit: "while(" -> "while ("

Done.

http://codereview.chromium.org/7466006/diff/6001/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:216: CERTCertificate*
cert = PK11_FindCertFromDERCertItem(
On 2011/07/29 01:46:27, Ryan Sleevi wrote:
> LEAK: |cert| is leaked (line 229 or line 231)

Done.

[wtc, 2011-07-29 19:08:37]

LGTM.  I suggest some changes below.

http://codereview.chromium.org/7466006/diff/10005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/10005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:231: }
The if (srv) statement on lines 228-231 should be moved inside
the if (privKey) block, after line 225.

We should add a null pointer check for 'cert', so I suggest
this code sequence:
    CERTCertificate* cert = PK11_FindCertFromDERCertItem(...);
    if (!cert)
      continue;  // You can add a LOG(ERROR) statement
    SECKEYPrivateKey* privKey = PK11_FindPrivateKeyFromCert(...)
    CERT_DestroyCertificate(cert);
    if (privKey) {
      ...
    }

Also, do you really want to break out of the loop on
line 230 if we get an error with a private key?

[gauravsh, 2011-07-29 20:04:55]

http://codereview.chromium.org/7466006/diff/10005/net/third_party/mozilla_sec...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/10005/net/third_party/mozilla_sec...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:231: }
On 2011/07/29 19:08:37, wtc wrote:
> The if (srv) statement on lines 228-231 should be moved inside
> the if (privKey) block, after line 225.
> 
> We should add a null pointer check for 'cert', so I suggest
> this code sequence:
>     CERTCertificate* cert = PK11_FindCertFromDERCertItem(...);
>     if (!cert)
>       continue;  // You can add a LOG(ERROR) statement
>     SECKEYPrivateKey* privKey = PK11_FindPrivateKeyFromCert(...)
>     CERT_DestroyCertificate(cert);
>     if (privKey) {
>       ...
>     }

Done.

> 
> Also, do you really want to break out of the loop on
> line 230 if we get an error with a private key?
> 

I guess we could continue with the next PKCS#12 items although my reasoning for
the original break was that a failure to get the private key at this point means
that something failed during the earlier import.

Changed it to continue instead. We can re-visit this if we ever run into this
error case.

[wtc, 2011-07-29 20:23:57]

LGTM.

http://codereview.chromium.org/7466006/diff/4006/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/7466006/diff/4006/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:223: // Importing a Pkcs#12 file with a
certificate but no corresponding
Nit: "PKCS" should be all capital, on this line and line 235.

Good unit test!

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:217: slot,
const_cast<SECItem*>(decoder_item->der),
I will write an NSS patch so that this const_cast won't be
necessary in the future.

But, are you sure the const_cast is necessary?
Although decoder_item is a const pointer, decoder_item->der
is not.

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:235: continue;
Nit: this continue is not necessary.

Using a break is fine.  I just wanted to make sure you used
a break deliberately.

[gauravsh, 2011-07-29 21:57:50]

I am going to check the commit queue box now...

http://codereview.chromium.org/7466006/diff/4006/net/base/cert_database_nss_u...
File net/base/cert_database_nss_unittest.cc (right):

http://codereview.chromium.org/7466006/diff/4006/net/base/cert_database_nss_u...
net/base/cert_database_nss_unittest.cc:223: // Importing a Pkcs#12 file with a
certificate but no corresponding
On 2011/07/29 20:23:57, wtc wrote:
> Nit: "PKCS" should be all capital, on this line and line 235.
> 
> Good unit test!

Done.

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
File net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp (right):

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:217: slot,
const_cast<SECItem*>(decoder_item->der),
On 2011/07/29 20:23:57, wtc wrote:
> I will write an NSS patch so that this const_cast won't be
> necessary in the future.
> 
> But, are you sure the const_cast is necessary?
> Although decoder_item is a const pointer, decoder_item->der
> is not.

You are right, I think this I was a holdout from a different function I was
using here. Removed the cast.

http://codereview.chromium.org/7466006/diff/4006/net/third_party/mozilla_secu...
net/third_party/mozilla_security_manager/nsPKCS12Blob.cpp:235: continue;
On 2011/07/29 20:23:57, wtc wrote:
> Nit: this continue is not necessary.
> 
> Using a break is fine.  I just wanted to make sure you used
> a break deliberately.

Yes, deliberate. And I switched it back. On a failure at this point, it is
probably safer if we bail out.

[gauravsh, 2011-07-29 21:59:07]

Looks like one of you need to make this change run on the trybots first before I
can safely add it to the commit queue. Could you do that?

[Ryan Sleevi, 2011-07-29 22:07:52]

On 2011/07/29 21:59:07, gauravsh wrote:
> Looks like one of you need to make this change run on the trybots first before
I
> can safely add it to the commit queue. Could you do that?

http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/40164
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/40464
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/45918

[gauravsh, 2011-07-29 22:11:40]

On Fri, Jul 29, 2011 at 3:07 PM,  <rsleevi@chromium.org> wrote:
> On 2011/07/29 21:59:07, gauravsh wrote:
>>
>> Looks like one of you need to make this change run on the trybots first
>> before
>
> I
>>
>> can safely add it to the commit queue. Could you do that?
>
> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/40164
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/40464
> http://build.chromium.org/p/tryserver.chromium/builders/win/builds/45918

Looks like I need to rebase my change first. All the 3 try jobs
failed. Will do and update then.

>
> http://codereview.chromium.org/7466006/
>



-- 
-g

[Ryan Sleevi, 2011-07-29 22:12:31]

No, it looks like 'git try --issue [#]' doesn't handle GIT patches the same way
that it does when running locally.

I started another batch up (this time set to email me, incase I screw it up
again) at -p1 to handle git-vs-svn patch differences.

[gauravsh, 2011-07-29 22:16:43]

On Fri, Jul 29, 2011 at 3:12 PM,  <rsleevi@chromium.org> wrote:
> No, it looks like 'git try --issue [#]' doesn't handle GIT patches the same
> way
> that it does when running locally.
>
> I started another batch up (this time set to email me, incase I screw it up
> again) at -p1 to handle git-vs-svn patch differences.

Cool, thanks. I just did a rebase and there weren't any conflicts.

>
> http://codereview.chromium.org/7466006/
>



-- 
-g

[Ryan Sleevi, 2011-07-29 22:18:11]

http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/40166
http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/40466
http://build.chromium.org/p/tryserver.chromium/builders/win/builds/45920

They should also update the status on this issue so that you can CQ it.

[gauravsh, 2011-07-29 23:57:08]

Looks like the linux trybot failed because it couldn't fine the
client-nokey.p12 file. Is that something wrong with my patch? Or did
it somehow not get the new file I had added?

On Fri, Jul 29, 2011 at 3:18 PM,  <rsleevi@chromium.org> wrote:
> http://build.chromium.org/p/tryserver.chromium/builders/linux/builds/40166
> http://build.chromium.org/p/tryserver.chromium/builders/mac/builds/40466
> http://build.chromium.org/p/tryserver.chromium/builders/win/builds/45920
>
> They should also update the status on this issue so that you can CQ it.
>
> http://codereview.chromium.org/7466006/
>



-- 
-g

[Ryan Sleevi, 2011-07-29 23:58:20]

Bah. It's the binary patch issue. Tryjobs don't consider binary blobs in
patchsets.

You have to effectively land it as two commits - first commit the binary file,
then run the tests that depend on the binary file and, if all is well, commit
the actual patch.

[gauravsh, 2011-08-03 19:20:09]

Ok, wtc submitted my other change with the binary blobs, and I have rebased this
on top of that.

This is now ready for the trybots...

On 2011/07/29 23:58:20, Ryan Sleevi wrote:
> Bah. It's the binary patch issue. Tryjobs don't consider binary blobs in
> patchsets.
> 
> You have to effectively land it as two commits - first commit the binary file,
> then run the tests that depend on the binary file and, if all is well, commit
> the actual patch.

[commit-bot: I haz the power, 2011-08-04 20:55:46]

Change committed as 95486