Search all slots when looking for a key in NSS

Search all slots when looking for a key in NSS

This should make it possible to run on a VM, and still find the private key created for the owner in the software slot.

BUG=chromium-os:15817
TEST=Built an image and tried it on a VM and a device. Both showed restricted users list.

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

[Chris Masone, 2011-06-03 18:12:45]

LGTM w/nit

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.c...
crypto/rsa_private_key_nss.cc:138: return NULL;
Might we worth logging a WARNING here, at least.

[Greg Spencer (Chromium), 2011-06-03 18:19:56]

Adding wtc@ to do an OWNER review.

[wtc, 2011-06-03 18:31:56]

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (left):

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.c...
crypto/rsa_private_key_nss.cc:125: return NULL;
I wonder if this is the bug.  Compare this with your
comment:

  // If we don't have a TPM, this could happen.

in the new code.  Perhaps all you need to do is to go on
to the public slot if the private slot doesn't exist.

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.c...
crypto/rsa_private_key_nss.cc:132: // look in the private slot.
Recall that both rsleevi and I suggested that this function
should ideally search in all slots.

If the correctness of this function depends on a particular
search order, then this function is no longer generally
useful and should be made Chrome OS only.

[Greg Spencer (Chromium), 2011-06-06 22:40:58]

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.cc
File crypto/rsa_private_key_nss.cc (right):

http://codereview.chromium.org/7066070/diff/2001/crypto/rsa_private_key_nss.c...
crypto/rsa_private_key_nss.cc:132: // look in the private slot.
On 2011/06/03 18:31:56, wtc wrote:
> Recall that both rsleevi and I suggested that this function
> should ideally search in all slots.
> 
> If the correctness of this function depends on a particular
> search order, then this function is no longer generally
> useful and should be made Chrome OS only.

OK, I've changed the code to search all slots.  It's not guaranteed to search
the software slot first now, of course, but at least it will not try to search
the hardware slot if it's not loaded.

[wtc, 2011-06-07 00:14:32]

LGTM.

http://codereview.chromium.org/7066070/diff/6002/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

http://codereview.chromium.org/7066070/diff/6002/crypto/nss_util_internal.h#n...
crypto/nss_util_internal.h:33: ~AutoSECMODListReadLock();
Can you define the constructor and destructor inline?

Nit: add a blank line after this line to separate the
public and private sections.

[Greg Spencer (Chromium), 2011-06-07 00:28:08]

http://codereview.chromium.org/7066070/diff/6002/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

http://codereview.chromium.org/7066070/diff/6002/crypto/nss_util_internal.h#n...
crypto/nss_util_internal.h:33: ~AutoSECMODListReadLock();
On 2011/06/07 00:14:32, wtc wrote:
> Can you define the constructor and destructor inline?

Well, I could, but then I'd have to add secmod.h (not just secmodt.h) to the
header because they use SECMOD_ calls.   I was also following the way
AutoNSSWriteLock did in nss_util.h for consistency.

> Nit: add a blank line after this line to separate the
> public and private sections.

Done.

[Greg Spencer (Chromium), 2011-06-08 17:33:13]

Ping.

[Chris Masone, 2011-06-08 17:38:14]

LGTM, still

[Greg Spencer (Chromium), 2011-06-08 17:40:21]

LOL  OK.  Sorry, I guess I thought wtc might want to respond to my not
wanting to make the constructor and destructor inline.  I'll commit and we
can fix that if he feels strongly about it.

On Wed, Jun 8, 2011 at 10:38 AM, <cmasone@chromium.org> wrote:

> LGTM, still
>
>
>
http://codereview.chromium.**org/7066070/<http://codereview.chromium.org/7066...
>

[wtc, 2011-06-08 18:03:48]

http://codereview.chromium.org/7066070/diff/12001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

http://codereview.chromium.org/7066070/diff/12001/crypto/nss_util_internal.h#...
crypto/nss_util_internal.h:9: #include <secmodt.h>
Since you already include <secmodt.h> here, you can replace
it with <secmod.h> so that you can define the AutoSECMODListReadLock
class inline.  Either way is fine by me.