*wip* NSS: handle chromeos system slot.

*wip* NSS: handle chromeos system slot.

BUG=210525

[pneubeck (no reviews), 2014-06-13 12:40:43]

Sorry if I'm ranting so much about the low amount of documentation around
certificate handling; but I just find myself reading related code regularly and
still having a hard time to really understand it.
But maybe that's just me...

(I know, it's WIP and I'm not an owner as well :-) )

https://codereview.chromium.org/330213002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:33: public_slot.Pass(),
crypto::ScopedPK11Slot(), crypto::ScopedPK11Slot());
Needs an explanation about the choice of arguments.

https://codereview.chromium.org/330213002/diff/1/chromeos/cert_loader.cc
File chromeos/cert_loader.cc (right):

https://codereview.chromium.org/330213002/diff/1/chromeos/cert_loader.cc#newc...
chromeos/cert_loader.cc:139: // If the key is on the TPM, include the TPM slot
id.
Does that mean, that in tests and in fake non-ChromeOS environments (e.g. Linux
chromeos=1 builds) will not be able to distinguish a system and user slot?
Not a good idea, I'd guess.

https://codereview.chromium.org/330213002/diff/1/chromeos/network/client_cert...
File chromeos/network/client_cert_util.cc (right):

https://codereview.chromium.org/330213002/diff/1/chromeos/network/client_cert...
chromeos/network/client_cert_util.cc:203: // included in the GetPkcs11IdForCert
output?
This has to be discussed with the Shill team (pstew@, quiche@)
If Shill doesn't expect a slot id in the PKCS11Id, and if we don't want to
change their API, then we should just continue sending these properties
separately.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_cert_database_c...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_cert_database_c...
net/cert/nss_cert_database_chromeos.h:21: crypto::ScopedPK11Slot system_slot);
Please add a comment explaining the meaning of each slot.
The naming is also inconsistent with regard to public/private vs.  user/system.

How about always explicitly calling the slots
{public,private}_{system,user}_slot ?

Why is there only one system_slot (which is assigned always a private slot later
in this CL) and not both public_system_slot and private_system_slot? (at least
document the reason)

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:47:
PK11_ReferenceSlot(other.system_slot_.get()) :
btw. I think this is one of the best examples, why scoped_ptr seems to be the
wrong wrapper for slots (ref counted in NSS vs. single explicit owner declared
with scoped_ptr).

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:87: slot == system_slot_.get())
nit: missing {}

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:100: // it. (This assumes that
private_slot_ and system_slot_ are on the same
could you ensure this with a DCHECK?

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:104: module_for_slot !=
PK11_GetModule(private_slot_.get()))
nit: missing {}

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:105: return true;
Although you didn't modify it, could you still add a rational why we would
accept (CA) certs of other modules? In which cases would this case occur and why
is it what we want?

Note that the existing class comment is extremely unspecific in what the purpose
of this class/function is: "...if a given slot or certificate should be used for
a given user."

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
File net/cert/nss_profile_filter_chromeos_unittest.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos_unittest.cc:108: // Can't use ScopedTestNSS
because it changes the result of
ScopedTestNSS -> ScopedTestNSSDB

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
net/ssl/client_cert_store_chromeos.cc:65:
profile_filter_.Init(crypto::GetPublicSlotForChromeOSUser(username_hash_),
Why does this need the public slot?
Aren't client certs only stored in the private slot? Or does this depend on
whether their keys are hardware-backed or not?
(again a clarifying comment somewhere about the purpose of 'private' and
'public' slots would be useful)

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
net/ssl/client_cert_store_chromeos.cc:67:
crypto::ScopedPK11Slot(crypto::GetPrivateNSSKeySlot()));
If GetPrivateNSSKeySlot is referring to the system wide token, then it should be
commented (or maybe even named) that way.
For readers without a lot of background about the naming scheme and the crypto/
or net/ internals I think it's hard to follow.

[mattm, 2014-06-24 03:16:46]

I've removed the chromeos/shill related changes, since I think that will require
some more in-depth look that belongs in a separate CL, maybe written by someone
else more familiar with that.

I addressed your other comments, though this is still WIP-ish. (At the very
least, needs to deal with the case of system slot being disabled, and may need
further refinement of when system slot certs are used, per the email
discussions).

https://codereview.chromium.org/330213002/diff/1/chrome/browser/chromeos/net/...
File chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/chrome/browser/chromeos/net/...
chrome/browser/chromeos/net/cert_verify_proc_chromeos.cc:33: public_slot.Pass(),
crypto::ScopedPK11Slot(), crypto::ScopedPK11Slot());
On 2014/06/13 12:40:42, pneubeck wrote:
> Needs an explanation about the choice of arguments.

Done.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_cert_database_c...
File net/cert/nss_cert_database_chromeos.h (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_cert_database_c...
net/cert/nss_cert_database_chromeos.h:21: crypto::ScopedPK11Slot system_slot);
On 2014/06/13 12:40:42, pneubeck wrote:
> Please add a comment explaining the meaning of each slot.

done.

> The naming is also inconsistent with regard to public/private vs. 
user/system.

That's more an artifact of the nss_util::GetPrivateNSSKeySlot function being
overloaded on chromeos to now mean the system slot.

> 
> How about always explicitly calling the slots
> {public,private}_{system,user}_slot ?
> 
> Why is there only one system_slot (which is assigned always a private slot
later
> in this CL) and not both public_system_slot and private_system_slot? (at least
> document the reason)

public_slot is the NSS software db, private and system slot are TPM backed.
There isn't a system-wide software db.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
File net/cert/nss_profile_filter_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:47:
PK11_ReferenceSlot(other.system_slot_.get()) :
On 2014/06/13 12:40:42, pneubeck wrote:
> btw. I think this is one of the best examples, why scoped_ptr seems to be the
> wrong wrapper for slots (ref counted in NSS vs. single explicit owner declared
> with scoped_ptr).

yep. I guess just no one has felt like writing a general "external refcounted
object" wrapper class.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:87: slot == system_slot_.get())
On 2014/06/13 12:40:42, pneubeck wrote:
> nit: missing {}

Done.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:100: // it. (This assumes that
private_slot_ and system_slot_ are on the same
On 2014/06/13 12:40:42, pneubeck wrote:
> could you ensure this with a DCHECK?

Done.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:104: module_for_slot !=
PK11_GetModule(private_slot_.get()))
On 2014/06/13 12:40:42, pneubeck wrote:
> nit: missing {}

Done.

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos.cc:105: return true;
On 2014/06/13 12:40:42, pneubeck wrote:
> Although you didn't modify it, could you still add a rational why we would
> accept (CA) certs of other modules? In which cases would this case occur and
why
> is it what we want?

This allows for some hypothetical future where we might support plugging in
smartcards/etc. I don't think it actually should happen currently. (I suppose in
dev mode someone could do this today)

> Note that the existing class comment is extremely unspecific in what the
purpose
> of this class/function is: "...if a given slot or certificate should be used
for
> a given user."

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
File net/cert/nss_profile_filter_chromeos_unittest.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/cert/nss_profile_filter_...
net/cert/nss_profile_filter_chromeos_unittest.cc:108: // Can't use ScopedTestNSS
because it changes the result of
On 2014/06/13 12:40:42, pneubeck wrote:
> ScopedTestNSS -> ScopedTestNSSDB

Done.

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
File net/ssl/client_cert_store_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
net/ssl/client_cert_store_chromeos.cc:65:
profile_filter_.Init(crypto::GetPublicSlotForChromeOSUser(username_hash_),
On 2014/06/13 12:40:43, pneubeck wrote:
> Why does this need the public slot?
> Aren't client certs only stored in the private slot? Or does this depend on
> whether their keys are hardware-backed or not?
> (again a clarifying comment somewhere about the purpose of 'private' and
> 'public' slots would be useful)

Client certs can be stored in the software slot also (certificate manager allows
you to import or import hardware backed).

https://codereview.chromium.org/330213002/diff/1/net/ssl/client_cert_store_ch...
net/ssl/client_cert_store_chromeos.cc:67:
crypto::ScopedPK11Slot(crypto::GetPrivateNSSKeySlot()));
On 2014/06/13 12:40:43, pneubeck wrote:
> If GetPrivateNSSKeySlot is referring to the system wide token, then it should
be
> commented (or maybe even named) that way.
> For readers without a lot of background about the naming scheme and the
crypto/
> or net/ internals I think it's hard to follow.

Done.

[pneubeck (no reviews), 2014-06-27 17:07:39]

https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
File chrome/browser/net/nss_context_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
chrome/browser/net/nss_context_chromeos.cc:93: net::NSSCertDatabase*
GetNSSCertDatabaseForResourceContext(
could we change this from lazy initialization to explicit initialization
(triggered by the login) and
pass a parameter about the user's affiliation (i.e. managed by the domain that
the device is enrolled to , or not)
and only in the managed case load/pass the system token?

I'm currently trying to setup a CL for that.

[mattm, 2014-06-27 22:14:43]

https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
File chrome/browser/net/nss_context_chromeos.cc (right):

https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
chrome/browser/net/nss_context_chromeos.cc:93: net::NSSCertDatabase*
GetNSSCertDatabaseForResourceContext(
On 2014/06/27 17:07:39, pneubeck wrote:
> could we change this from lazy initialization to explicit initialization
> (triggered by the login) and
> pass a parameter about the user's affiliation (i.e. managed by the domain that
> the device is enrolled to , or not)
> and only in the managed case load/pass the system token?
> 
> I'm currently trying to setup a CL for that.

Hm, I thought we wanted the system token available for all users?

[pneubeck (no reviews), 2014-06-30 08:56:16]

On 2014/06/27 22:14:43, mattm wrote:
>
https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
> File chrome/browser/net/nss_context_chromeos.cc (right):
> 
>
https://codereview.chromium.org/330213002/diff/30012/chrome/browser/net/nss_c...
> chrome/browser/net/nss_context_chromeos.cc:93: net::NSSCertDatabase*
> GetNSSCertDatabaseForResourceContext(
> On 2014/06/27 17:07:39, pneubeck wrote:
> > could we change this from lazy initialization to explicit initialization
> > (triggered by the login) and
> > pass a parameter about the user's affiliation (i.e. managed by the domain
that
> > the device is enrolled to , or not)
> > and only in the managed case load/pass the system token?
> > 
> > I'm currently trying to setup a CL for that.
> 
> Hm, I thought we wanted the system token available for all users?

As further discussed with Darren the main use cases are covered by "By default,
only managed users have access to the certificates." (see the design doc).
A policy could change this default behavior. But as long as we don't have an
immediate need for that, we should go with the most secure and simple solution.
Not providing the slot to non-managed users should be the safest and doesn't
need modifications of the different users (like the cert autoselection).