Enable system NSS key slot.

Enable system NSS key slot.

This only affects users of domains that the device is registered to for policy.
All other users are unaffected (EnableNSSSystemKeySlotForResourceContext is only called for USER_AFFILIATION_MANAGED)

For the affected users, this enables and uses the slot for
- client authentication for TSL (see ClientCertStoreChromeOS)
- client authentication for 802.1x networks
- listing/removing certificates on the settings page (see CertificateManager)

In a follow up, also the enterprise.platformKeys API will be updated.

Depends on:
https://codereview.chromium.org/426983002/
https://codereview.chromium.org/428933002/

BUG=210525
R=mattm@chromium.org, rsleevi@chromium.org, willchan@chromium.org, xiyuan@chromium.org

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

[pneubeck (no reviews), 2014-07-28 19:52:33]

Finally the CL that enables the system slot for parts of ChromeOS.
This is still missing a test for NSSCertDatabaseChromeOS, which I'm currently
writing.

Btw. this was picked up from Matt's CL
https://codereview.chromium.org/330213002/ .

[pneubeck (no reviews), 2014-07-28 19:55:38]

+Will: please take a look at the profile_io_data change.
Note that I'm not happy with the presence of all these NSS details in
profile_io_data. That's why I'm already working on a larger cleanup
(https://codereview.chromium.org/419013003/) that will remove _all_ of that NSS
initialization out of profile_io_data. But I can't finish that cleanup for R38.
For R38, I hope this change is fine.

[Ryan Sleevi, 2014-07-29 00:23:17]

https://codereview.chromium.org/424523002/diff/40001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/424523002/diff/40001/crypto/nss_util_internal...
crypto/nss_util_internal.h:63: // module, then this test slot will be used and
the initialization continues as
English-wise, this reads a little weird (namely "IfThen")

You can probably nuke the "then"? "If no system token ..."

This |skip_tpm_initialization| really doesn't make sense to me, even after
having read all of the code.

https://codereview.chromium.org/424523002/diff/40001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/40001/net/cert/nss_cert_databa...
net/cert/nss_cert_database_chromeos.h:25: #if defined(OS_CHROMEOS)
You're in a ChromeOS file. Isn't this a given?

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.cc:17: void GotSystemSlotGetPrivateSlot(
This is really poorly named. I don't even know what to suggest as an
alternative.

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.h:19: // |use_system_slot| is true, also
accesses the system slot.
This comment seems confusing.

// Constructs a ClientCertStore that will return client certs
// available on the user's private and public slots. If
// |use_system_slot| is true, certs on the system slot will also
// be returned.

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos_unittest.cc:50: template <bool
read_from_user_slot, bool enable_user_slot>
You can use enums here to make these types more descriptive (e.g. not requiring
the comments like on lines 126/127)

[pneubeck (no reviews), 2014-07-29 16:00:16]

https://codereview.chromium.org/424523002/diff/40001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/424523002/diff/40001/crypto/nss_util_internal...
crypto/nss_util_internal.h:63: // module, then this test slot will be used and
the initialization continues as
On 2014/07/29 00:23:16, Ryan Sleevi wrote:
> English-wise, this reads a little weird (namely "IfThen")
> 
> You can probably nuke the "then"? "If no system token ..."
Yes, the 'then' was a typo.
> 
> This |skip_tpm_initialization| really doesn't make sense to me, even after
> having read all of the code.

The idea was to allow for two kinds of tests:

1) In a unit test one would use skip_tpm_init=true. Without the TPMTokenLoader
being used in the test (which would usually call
InitializeTPMTokenAndSystemSlot), all GetSystemNSSKeySlot calls would still
succeed.

2) In a browser test one would use skip_tpm_init=false. As in a real run on
ChromeOS, TPMTokenLoader would initialize the TPM by calling
InitializeTPMTokenAndSystemSlot and only after the initialization in
nss_utils.cc finds no chaps module, it instead uses the test slot. That way, all
callbacks and callback lists would be tested as well.

However, after I now tried to write such a browser test, I found  that there are
several places that do some special handling if the browser is not running in a
ChromeOS environment (which is in particular true in tests) and skip the TPM
initialization all together. Also, it is currently impossible to set the test
system slot before the EnableNSSSystemKeySlot* function in profile_io_data is
called. To allow that, we either have to drop the IO thread restriction on the
ScopedTestSystemNSSKeySlot helper or add some other testing code.

Thus, I would suggest to punt such tests that cover the TPM initialization to
the larger refactoring/cleanup in 39 and
for now keep only the simple flow 1) with skip_tpm_initialization=true.
I removed the flag in the patch set #3 already.

Actually, once I got rid of more of the global accessors (e.g.
GetSystemNSSKeySlot, GetPublicSlotForChromeOSUser, ...) in net/ and crypto/
several test problems should be solved at the same time.

https://codereview.chromium.org/424523002/diff/40001/net/cert/nss_cert_databa...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/40001/net/cert/nss_cert_databa...
net/cert/nss_cert_database_chromeos.h:25: #if defined(OS_CHROMEOS)
On 2014/07/29 00:23:16, Ryan Sleevi wrote:
> You're in a ChromeOS file. Isn't this a given?

Done. Thanks for catching.

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.cc:17: void GotSystemSlotGetPrivateSlot(
On 2014/07/29 00:23:16, Ryan Sleevi wrote:
> This is really poorly named. I don't even know what to suggest as an
> alternative.

Added a comment and hopefully improved the naming.

I should consider for the future CertDatabase design, to provide a nice way to
retrieve multiple slots asynchronously with a single call. These callback
sequences are... ^^

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos.h:19: // |use_system_slot| is true, also
accesses the system slot.
On 2014/07/29 00:23:16, Ryan Sleevi wrote:
> This comment seems confusing.
> 
> // Constructs a ClientCertStore that will return client certs
> // available on the user's private and public slots. If
> // |use_system_slot| is true, certs on the system slot will also
> // be returned.

Done.

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
File net/ssl/client_cert_store_chromeos_unittest.cc (right):

https://codereview.chromium.org/424523002/diff/40001/net/ssl/client_cert_stor...
net/ssl/client_cert_store_chromeos_unittest.cc:50: template <bool
read_from_user_slot, bool enable_user_slot>
On 2014/07/29 00:23:16, Ryan Sleevi wrote:
> You can use enums here to make these types more descriptive (e.g. not
requiring
> the comments like on lines 126/127)

Yeah, you're right. Should be much more readable now.

[willchan no longer on Chromium, 2014-07-29 16:02:37]

chrome/browser/profiles lgtm

[pneubeck (no reviews), 2014-07-29 18:09:13]

A unit tests for NSSCertDatabaseChromeOS will be added with this CL:
https://codereview.chromium.org/429633004/

[Ryan Sleevi, 2014-07-29 23:53:15]

I think it's mostly good, and a few comments regarding layering
concerns/assumptions that should be easy to update.

Now that Matt's back, perhaps give him a day to give this a once-over.

https://codereview.chromium.org/424523002/diff/180001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/424523002/diff/180001/crypto/nss_util_interna...
crypto/nss_util_internal.h:59: // does not have to be called if the test slot is
set.
Kind of a weird API contract, in that I'd think to actually test things are
working, you'd want the two to be different slots.

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:133: #if defined(OS_CHROMEOS)
This feels like a layering violation to me, to have it declared on the base, but
only OS_CHROMEOS, but then overridden in the _chromeos file.

Should this just be always declared (no ifdef)?

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:19: // |public_slot| is the NSS software
slot for the user.
Comment wise, I think the older bits were more correct.

In particular, there should be no assumption that |public_slot| is a software
slot. In the future, it will be a Chaps slot just the same.

It's just the place to store the public details.

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_chromeos.cc:39: crypto::ScopedPK11Slot
system_slot(crypto::GetSystemNSSKeySlot(
This is temporary, right? Doesn't seem like we should be doing singletons.

[pneubeck (no reviews), 2014-07-30 06:27:40]

https://codereview.chromium.org/424523002/diff/180001/crypto/nss_util_internal.h
File crypto/nss_util_internal.h (right):

https://codereview.chromium.org/424523002/diff/180001/crypto/nss_util_interna...
crypto/nss_util_internal.h:59: // does not have to be called if the test slot is
set.
On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> Kind of a weird API contract, in that I'd think to actually test things are
> working, you'd want the two to be different slots.

No idea what 'two' slots you're meaning, only the system slot is affected here.
Maybe the comment is still misleading because of the inconsistent use of 'test
system slot', and 'test slot' which were meant to be the same?

Thus s/test slot/test system slot/

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:133: #if defined(OS_CHROMEOS)
On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> This feels like a layering violation to me, to have it declared on the base,
but
> only OS_CHROMEOS, but then overridden in the _chromeos file.
Yes! Totally agreed. This shouldn't be present on Linux at all. But,
NSSCertDatabaseChromeOS is not exposed to the consumers in the UI.
And the correct solution might be to have something like a GetChromeOSPart()
which returns an object that contains all ChromeOS specifics. Ok to punt?

> 
> Should this just be always declared (no ifdef)?
I can do that if you think it's better, abstract however will not work because
even on ChromeOS the base class is instantiated in tests.

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:19: // |public_slot| is the NSS software
slot for the user.
On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> Comment wise, I think the older bits were more correct.
> 
> In particular, there should be no assumption that |public_slot| is a software
> slot. In the future, it will be a Chaps slot just the same.
> 
> It's just the place to store the public details.

Actually, this comment should be unnecessary as the arguments are forwarded to
NSSCertDatabase?
With 'older bits' you probably mean the c'tor comment on NSSCertDatabase.

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_chromeos.cc:39: crypto::ScopedPK11Slot
system_slot(crypto::GetSystemNSSKeySlot(
On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> This is temporary, right? Doesn't seem like we should be doing singletons.

Yes. At first I wanted to pass the system slot to the c'tor.
However, ProfileIOData::CreateClientCertStore is synchronous ?!)(#$

Yet-another TODO is to make that function asynchrounous and pass the system slot
(or better all slots) through the c'tor of this store (and then also to the
store of Linux)

That will also simplify the tests again and make them independent of the
singletons in nss_util.

[Ryan Sleevi, 2014-07-30 06:45:30]

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database.h:133: #if defined(OS_CHROMEOS)
On 2014/07/30 06:27:39, pneubeck wrote:
> On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> > This feels like a layering violation to me, to have it declared on the base,
> but
> > only OS_CHROMEOS, but then overridden in the _chromeos file.
> Yes! Totally agreed. This shouldn't be present on Linux at all. But,
> NSSCertDatabaseChromeOS is not exposed to the consumers in the UI.
> And the correct solution might be to have something like a GetChromeOSPart()
> which returns an object that contains all ChromeOS specifics. Ok to punt?


That would still be a pretty egregious layering violation. Agreed to punt, since
we need to work out the details.

> 
> > 
> > Should this just be always declared (no ifdef)?
> I can do that if you think it's better, abstract however will not work because
> even on ChromeOS the base class is instantiated in tests.

Then we definitely have layering issues to fix.

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:19: // |public_slot| is the NSS software
slot for the user.
On 2014/07/30 06:27:39, pneubeck wrote:
> On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> > Comment wise, I think the older bits were more correct.
> > 
> > In particular, there should be no assumption that |public_slot| is a
software
> > slot. In the future, it will be a Chaps slot just the same.
> > 
> > It's just the place to store the public details.
> 
> Actually, this comment should be unnecessary as the arguments are forwarded to
> NSSCertDatabase?
> With 'older bits' you probably mean the c'tor comment on NSSCertDatabase.

Yeah.

Either way, the comment is wrong as is. We shouldn't assume software.

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/180001/net/ssl/client_cert_sto...
net/ssl/client_cert_store_chromeos.cc:39: crypto::ScopedPK11Slot
system_slot(crypto::GetSystemNSSKeySlot(
On 2014/07/30 06:27:40, pneubeck wrote:
> On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> > This is temporary, right? Doesn't seem like we should be doing singletons.
> 
> Yes. At first I wanted to pass the system slot to the c'tor.
> However, ProfileIOData::CreateClientCertStore is synchronous ?!)(#$
> 
> Yet-another TODO is to make that function asynchrounous and pass the system
slot
> (or better all slots) through the c'tor of this store (and then also to the
> store of Linux)

Don't do this. Don't make another factory.

The ClientCertStore is already supposed to handle things async. We shouldn't be
lobbing more async factory functions around - they inevitably signal a coupling
that is too tight.

> 
> That will also simplify the tests again and make them independent of the
> singletons in nss_util.

[pneubeck (no reviews), 2014-07-30 13:54:11]

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/424523002/diff/180001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.h:19: // |public_slot| is the NSS software
slot for the user.
On 2014/07/30 06:45:30, Ryan Sleevi wrote:
> On 2014/07/30 06:27:39, pneubeck wrote:
> > On 2014/07/29 23:53:15, Ryan Sleevi wrote:
> > > Comment wise, I think the older bits were more correct.
> > > 
> > > In particular, there should be no assumption that |public_slot| is a
> software
> > > slot. In the future, it will be a Chaps slot just the same.
> > > 
> > > It's just the place to store the public details.
> > 
> > Actually, this comment should be unnecessary as the arguments are forwarded
to
> > NSSCertDatabase?
> > With 'older bits' you probably mean the c'tor comment on NSSCertDatabase.
> 
> Yeah.
> 
> Either way, the comment is wrong as is. We shouldn't assume software.

Removed.

[mattm, 2014-07-30 22:57:34]

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:584:
tpm_slot_.reset(PK11_ReferenceSlot(test_system_slot_.get()));
need to call tpm_ready_callback_list_ callbacks too?

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:35: crypto::ScopedPK11Slot system_slot) {
Seems a little iffy that initializing the system slot is asynchronous but done
after the NSSCertDatabaseChromeOS is created, if anything uses it during that
time they wouldn't get results from the system slot. Is there a reason the
system slot isn't just another parameter to the constructor?

[pneubeck (no reviews), 2014-07-31 06:31:25]

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:583: EnableTPMTokenForNSS();
This one should actually not be necessary and I'll remove it.
Then, all effects of the ScopedTestSystemNSSKeySlot should be reverted on
destrction.

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:584:
tpm_slot_.reset(PK11_ReferenceSlot(test_system_slot_.get()));
On 2014/07/30 22:57:34, mattm wrote:
> need to call tpm_ready_callback_list_ callbacks too?

These callbacks are usually triggered by a InitializeTPMTokenAndSystemSlot call
from TPMTokenLoader.
However, in all tests that we currently have, the TPMTokenLoader skips this
initialization, see tpm_token_loader.cc:139 
  if (!base::SysInfo::IsRunningOnChromeOS())
    tpm_token_state_ = TPM_DISABLED;

Accordingly, currently no test can rely on the callbacks to be called, instead
they all assume that the TPM is disabled and instead software slots are set.

IMO very ugly, but I can't fix this in this CL.

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:35: crypto::ScopedPK11Slot system_slot) {
On 2014/07/30 22:57:34, mattm wrote:
> Seems a little iffy that initializing the system slot is asynchronous but done
> after the NSSCertDatabaseChromeOS is created, if anything uses it during that
> time they wouldn't get results from the system slot. Is there a reason the
> system slot isn't just another parameter to the constructor?

Yes, full ack.
That's why originally I tried to make the initialization of NSSCertDB explicit.
I tried to do that in user_session_manager, where we already have some cert db
code (and initialize CertTokenLoader). However, when I tried to write tests for
it, I realized that this is the totally wrong place to do it. The login flow and
user session manager are usually modified/mocked in browser tests and the
initialization didn't work as expected.

Instead, I started writing a CL to add a CertDBService, a KeyedService, which
will take care of most initialization on top of nss_util (also replacing
nss_context).
See https://codereview.chromium.org/419013003/

However, this change will not be small and not making it into 38.

Therefore, this not ideal, short-term solution of setting the system slot from
profile_io_data.
According to the current usage, it should never occur that someone access the
NSSCertDB before this setter was called (if it ever get's called).

[mattm, 2014-07-31 10:29:30]

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:584:
tpm_slot_.reset(PK11_ReferenceSlot(test_system_slot_.get()));
On 2014/07/31 06:31:25, pneubeck wrote:
> On 2014/07/30 22:57:34, mattm wrote:
> > need to call tpm_ready_callback_list_ callbacks too?
> 
> These callbacks are usually triggered by a InitializeTPMTokenAndSystemSlot
call
> from TPMTokenLoader.
> However, in all tests that we currently have, the TPMTokenLoader skips this
> initialization, see tpm_token_loader.cc:139 
>   if (!base::SysInfo::IsRunningOnChromeOS())
>     tpm_token_state_ = TPM_DISABLED;
> 
> Accordingly, currently no test can rely on the callbacks to be called, instead
> they all assume that the TPM is disabled and instead software slots are set.
> 
> IMO very ugly, but I can't fix this in this CL.

Right, my point was when the test uses software slots for the system slot, the
code being tested that uses GetSystemNSSKeySlot would expect the callbacks to be
called.  Is the issue that tpm_ready_callback_list_ is used for signalling both
"tpm ready" and "system slot ready"?

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
File net/cert/nss_cert_database_chromeos.cc (right):

https://codereview.chromium.org/424523002/diff/200001/net/cert/nss_cert_datab...
net/cert/nss_cert_database_chromeos.cc:35: crypto::ScopedPK11Slot system_slot) {
On 2014/07/31 06:31:25, pneubeck wrote:
> On 2014/07/30 22:57:34, mattm wrote:
> > Seems a little iffy that initializing the system slot is asynchronous but
done
> > after the NSSCertDatabaseChromeOS is created, if anything uses it during
that
> > time they wouldn't get results from the system slot. Is there a reason the
> > system slot isn't just another parameter to the constructor?
> 
> Yes, full ack.
> That's why originally I tried to make the initialization of NSSCertDB
explicit.
> I tried to do that in user_session_manager, where we already have some cert db
> code (and initialize CertTokenLoader). However, when I tried to write tests
for
> it, I realized that this is the totally wrong place to do it. The login flow
and
> user session manager are usually modified/mocked in browser tests and the
> initialization didn't work as expected.
> 
> Instead, I started writing a CL to add a CertDBService, a KeyedService, which
> will take care of most initialization on top of nss_util (also replacing
> nss_context).
> See https://codereview.chromium.org/419013003/
> 
> However, this change will not be small and not making it into 38.
> 
> Therefore, this not ideal, short-term solution of setting the system slot from
> profile_io_data.
> According to the current usage, it should never occur that someone access the
> NSSCertDB before this setter was called (if it ever get's called).

Okay, sounds reasonable. And thinking about the initialization order more it
does seem reasonably safe.

[pneubeck (no reviews), 2014-07-31 10:48:54]

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:584:
tpm_slot_.reset(PK11_ReferenceSlot(test_system_slot_.get()));
On 2014/07/31 10:29:30, mattm wrote:
> On 2014/07/31 06:31:25, pneubeck wrote:
> > On 2014/07/30 22:57:34, mattm wrote:
> > > need to call tpm_ready_callback_list_ callbacks too?
> > 
> > These callbacks are usually triggered by a InitializeTPMTokenAndSystemSlot
> call
> > from TPMTokenLoader.
> > However, in all tests that we currently have, the TPMTokenLoader skips this
> > initialization, see tpm_token_loader.cc:139 
> >   if (!base::SysInfo::IsRunningOnChromeOS())
> >     tpm_token_state_ = TPM_DISABLED;
> > 
> > Accordingly, currently no test can rely on the callbacks to be called,
instead
> > they all assume that the TPM is disabled and instead software slots are set.
> > 
> > IMO very ugly, but I can't fix this in this CL.
> 
> Right, my point was when the test uses software slots for the system slot, the
> code being tested that uses GetSystemNSSKeySlot would expect the callbacks to
be
> called.  Is the issue that tpm_ready_callback_list_ is used for signalling
both
> "tpm ready" and "system slot ready"?

Setting tpm_slot_ toggles IsTPMTokenReady to true (which actually means "TPM
token for system slot is ready") and thus GetSsytemNSSKeySlot immediately
returns without using tpm_ready_callback_list_ .

[mattm, 2014-07-31 20:27:43]

lgtm, but maybe add some TODOs or comments about the current limitations.

[pneubeck (no reviews), 2014-08-01 10:02:28]

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc
File crypto/nss_util.cc (right):

https://codereview.chromium.org/424523002/diff/200001/crypto/nss_util.cc#newc...
crypto/nss_util.cc:584:
tpm_slot_.reset(PK11_ReferenceSlot(test_system_slot_.get()));
Thinking about this again, I see now what you mean.
I changed it to call the callbacks of tpm_ready_callback_list_.

[pneubeck (no reviews), 2014-08-01 10:06:51]

@Jochen: Can you owner lgtm
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc ?
Matt&Ryan are already the best reviewer of this (he originally added this file
also).

...the OWNERs of this should be probably updated.

[pneubeck (no reviews), 2014-08-01 10:07:15]

@Ryan, ping

[Ryan Sleevi, 2014-08-01 17:31:19]

lgtm

[pneubeck (no reviews), 2014-08-01 23:36:47]

Conditionally initializing the use_system_key_slot members in profile_io_data.cc
as discussed with Will per chat.
I'm working on a clean up of the NSS initialization that should remove most of
the NSS details from profile_io_data.cc soon. So this is temporary.

[pneubeck (no reviews), 2014-08-01 23:36:56]

The CQ bit was checked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-08-01 23:36:59]

The CQ bit was unchecked by pneubeck@chromium.org

[pneubeck (no reviews), 2014-08-02 00:08:39]

+David for OWNER review of
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc

[pneubeck (no reviews), 2014-08-02 00:24:35]

The CQ bit was checked by pneubeck@chromium.org

[commit-bot: I haz the power, 2014-08-02 00:27:15]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/pneubeck@chromium.org/424523002/240001

[xiyuan, 2014-08-02 00:28:00]

lgtm

[pneubeck (no reviews), 2014-08-02 07:37:35]

Committed patchset #8 manually as 287175 (presubmit successful).