Add GetSystemNSSKeySlot, merge GetPrivateNSSKeySlot/GetPublicNSSKeySlot to GetPersistentNSSKeySlot.

Add GetSystemNSSKeySlot, merge GetPrivateNSSKeySlot/GetPublicNSSKeySlot to GetPersistentNSSKeySlot.

GetSystemNSSKeySlot returns the ChromeOS system-wide TPM slot.

ChromeOS has separate slots for each user and linux doesn't have a public/private split, so GetPrivateNSSKeySlot no longer makes sense.

BUG=210525
TBR=stevenjb@chromium.org

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=282862

[mattm, 2014-07-10 01:07:06]

Depends on https://codereview.chromium.org/317613004.

(Splitting https://codereview.chromium.org/330213002 up a bit.)

[Ryan Sleevi, 2014-07-10 01:55:06]

Rather than Default, which I'm certain will cause a resurgence of the issues in
the past (eg: using chaps to do hashing), perhaps GetPersistentPrivateKeySlot()
or something similar, to indicate it's where you stick long-term persistables?

[mattm, 2014-07-10 02:33:36]

On 2014/07/10 01:55:06, Ryan Sleevi wrote:
> Rather than Default, which I'm certain will cause a resurgence of the issues
in
> the past (eg: using chaps to do hashing), perhaps
GetPersistentPrivateKeySlot()
> or something similar, to indicate it's where you stick long-term persistables?

done

[Ryan Sleevi, 2014-07-10 02:36:50]

LGTM, but pneubeck should check closely too.

[pneubeck (no reviews), 2014-07-10 09:44:11]

lgtm with minor comments

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:646: // TODO(mattm): If the system slot is disabled,
InitializeTPMToken would
I looked for the cause of this.
IIUC, then TPMTokenLoader is now responsible to initialize the system token.
I'll write a CL to update the class comment there.

Since TPMTokenLoader always uses the kDefaultSlot = 0, the behavior that you
describe appears. Could you mention that here?

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.h#newcod...
crypto/nss_util.h:121: CRYPTO_EXPORT void InitializeTPMToken(
optional nit: this function and the internal ones with the name
"InitializeTPMToken" should maybe be renamed to InitializeTPMTokenAndSystemSlot.

https://codereview.chromium.org/383593002/diff/60001/net/socket/ssl_server_so...
File net/socket/ssl_server_socket_nss.cc (right):

https://codereview.chromium.org/383593002/diff/60001/net/socket/ssl_server_so...
net/socket/ssl_server_socket_nss.cc:449: PK11SlotInfo* slot =
PK11_GetInternalSlot();
This changed from PK11_GetInternalKeySlot to PK11_GetInternalSlot, which seems
fine according to the descriptions that I could find.

If I understand GetInternalSlot correctly, then this slot is a temporary,
not-persistent one provided by NSS that doesn't require authentication?

[mattm, 2014-07-10 22:20:45]

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.cc#newco...
crypto/nss_util.cc:646: // TODO(mattm): If the system slot is disabled,
InitializeTPMToken would
On 2014/07/10 09:44:11, pneubeck wrote:
> I looked for the cause of this.
> IIUC, then TPMTokenLoader is now responsible to initialize the system token.
> I'll write a CL to update the class comment there.
> 
> Since TPMTokenLoader always uses the kDefaultSlot = 0, the behavior that you
> describe appears. Could you mention that here?

Done.

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.h
File crypto/nss_util.h (right):

https://codereview.chromium.org/383593002/diff/60001/crypto/nss_util.h#newcod...
crypto/nss_util.h:121: CRYPTO_EXPORT void InitializeTPMToken(
On 2014/07/10 09:44:11, pneubeck wrote:
> optional nit: this function and the internal ones with the name
> "InitializeTPMToken" should maybe be renamed to
InitializeTPMTokenAndSystemSlot.

Done.

https://codereview.chromium.org/383593002/diff/60001/net/socket/ssl_server_so...
File net/socket/ssl_server_socket_nss.cc (right):

https://codereview.chromium.org/383593002/diff/60001/net/socket/ssl_server_so...
net/socket/ssl_server_socket_nss.cc:449: PK11SlotInfo* slot =
PK11_GetInternalSlot();
On 2014/07/10 09:44:11, pneubeck wrote:
> This changed from PK11_GetInternalKeySlot to PK11_GetInternalSlot, which seems
> fine according to the descriptions that I could find.
> 
> If I understand GetInternalSlot correctly, then this slot is a temporary,
> not-persistent one provided by NSS that doesn't require authentication?

Right. This function only imports a temporary key, so doesn't need a persistent
slot.

[mattm, 2014-07-13 03:34:59]

The CQ bit was checked by mattm@chromium.org

[commit-bot: I haz the power, 2014-07-13 03:36:07]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mattm@chromium.org/383593002/80001

[commit-bot: I haz the power, 2014-07-13 04:24:41]

FYI, CQ is re-trying this CL (attempt #1).
The failing builders are:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[commit-bot: I haz the power, 2014-07-13 04:27:06]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-07-13 04:27:06]

Try jobs failed on following builders:
  chromium_presubmit on tryserver.chromium
(http://build.chromium.org/p/tryserver.chromium/builders/chromium_presubmit/bu...)

[pneubeck (no reviews), 2014-07-13 07:15:25]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-07-13 07:16:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/mattm@chromium.org/383593002/80001

[commit-bot: I haz the power, 2014-07-13 07:19:06]

Change committed as 282862