Use per-user nssdb in onc certificate importer

Use per-user nssdb in onc certificate importer

Instead of always using singleton NSSCertDatabase, pass the
database to use as an argument to CertificateImporter::ImportCertificates

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

[tbarzic, 2014-02-03 22:41:15]

Hey, can you see if this makes sense to you? (especially stuff in
network_configuration_updater.cc). I initially planned to use
GetNSSCertDatabaseForProfile, but network_configuration_updater starts before
NSS database is initialized for the primary user (which happens during Profile
creation), even for the user policy. Observing CertLoader looks like a safe
alternative.

Also, what's the desired behaviour for the device policy installed certificates?
This cl imports certificates to the primary users private slot (as only client
certs seem to be allowed here). Is that OK?
Note that currently device_network_configuration_updater imports the certs
before NSS db is initialized (which happens during the log in), so it tries to
import the certs to the NSS internal key slot (which seems like a bug)

[pneubeck (no reviews), 2014-02-04 09:27:32]

On 2014/02/03 22:41:15, tbarzic wrote:
> Hey, can you see if this makes sense to you? (especially stuff in
> network_configuration_updater.cc). I initially planned to use
> GetNSSCertDatabaseForProfile, but network_configuration_updater starts before
> NSS database is initialized for the primary user (which happens during Profile
> creation), even for the user policy. Observing CertLoader looks like a safe
> alternative.
> 
> Also, what's the desired behaviour for the device policy installed
certificates?
> This cl imports certificates to the primary users private slot (as only client
> certs seem to be allowed here). Is that OK?
> Note that currently device_network_configuration_updater imports the certs
> before NSS db is initialized (which happens during the log in), so it tries to
> import the certs to the NSS internal key slot (which seems like a bug)

Currently, certs through device policy are not allowed (server and CA certs) or
are not working (client certs, because of the reason that you explained).
Also, the DeviceNetworkConfigUpdater cannot wait for the user's database,
because the networks from policy have to be available before login. Thus, I
would suggest removing cert importing from the DeviceNetworkConfigUpdater for
now. If you see a way to import client certs before login, then we could
consider that.

The dependency on CertLoader doesn't seem to be ideal. CertLoader is responsible
for making the list of certificates available on the UI thread, but the
Net*Updater doesn't need that list. How about just calling a
UserNetworkConfigUpdater::InitWithNSSDB(...) from
LoginUtilsImpl::StartCertLoaderWithNSSDB (and renaming the latter)?
The CertLoader dep. seems difficult to trace in the unit tests. The Init
function however would be directly called and more obvious.

What do you think?

[tbarzic, 2014-02-04 23:44:35]

On 2014/02/04 09:27:32, pneubeck wrote:
> On 2014/02/03 22:41:15, tbarzic wrote:
> > Hey, can you see if this makes sense to you? (especially stuff in
> > network_configuration_updater.cc). I initially planned to use
> > GetNSSCertDatabaseForProfile, but network_configuration_updater starts
before
> > NSS database is initialized for the primary user (which happens during
Profile
> > creation), even for the user policy. Observing CertLoader looks like a safe
> > alternative.
> > 
> > Also, what's the desired behaviour for the device policy installed
> certificates?
> > This cl imports certificates to the primary users private slot (as only
client
> > certs seem to be allowed here). Is that OK?
> > Note that currently device_network_configuration_updater imports the certs
> > before NSS db is initialized (which happens during the log in), so it tries
to
> > import the certs to the NSS internal key slot (which seems like a bug)
> 
> Currently, certs through device policy are not allowed (server and CA certs)
or
> are not working (client certs, because of the reason that you explained).
> Also, the DeviceNetworkConfigUpdater cannot wait for the user's database,
> because the networks from policy have to be available before login. Thus, I
> would suggest removing cert importing from the DeviceNetworkConfigUpdater for
> now. If you see a way to import client certs before login, then we could
> consider that.

Yeah, I don't think we can import client certs before login (at least not until
we support system PKCS#11 token).
I've added a comment to DeviceNetworkConfigurationUpdater::ImportCertificates
(and made it NOTREACHED)

> 
> The dependency on CertLoader doesn't seem to be ideal. CertLoader is
responsible
> for making the list of certificates available on the UI thread, but the
> Net*Updater doesn't need that list. How about just calling a
> UserNetworkConfigUpdater::InitWithNSSDB(...) from
> LoginUtilsImpl::StartCertLoaderWithNSSDB (and renaming the latter)?
> The CertLoader dep. seems difficult to trace in the unit tests. The Init
> function however would be directly called and more obvious.
> 
> What do you think?

Sounds good. I wasn't too keen on the CertLoader dependency either..
though, it could be useful in case we wanted to avoid ListCerts in
onc::CertificateImporter (which is used when certificates are removed), either
by passing in CertLoader instead of database, or passing additional cert_list to
ImportCertificates.

[tbarzic, 2014-02-04 23:44:55]

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/login_utils.h (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
chrome/browser/chromeos/login/login_utils.h:110: virtual void
InitNSSCertDatabaseUsers(Profile* user_profile) = 0;
I currently lack inspiration for a better name :(

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater.cc:89: void
NetworkConfigurationUpdater::ApplyPolicy() {
We could also bail out here if database_ is not set (and if the
NetworkConfigurationUpdater supports certificate importing), but I'm concerned
that it may take a while until NSS db is initialized (e.g. TPMTokenLoader logic
has a retry logic that allows delays up to 5 min).

[pneubeck (no reviews), 2014-02-05 11:03:26]

Looks mostly good. Please consider the larger inline comment.

Thanks for the improved testing!!

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater.cc:89: void
NetworkConfigurationUpdater::ApplyPolicy() {
On 2014/02/04 23:44:56, tbarzic wrote:
> We could also bail out here if database_ is not set (and if the
> NetworkConfigurationUpdater supports certificate importing), but I'm concerned
> that it may take a while until NSS db is initialized (e.g. TPMTokenLoader
logic
> has a retry logic that allows delays up to 5 min).

In case of Device*Updater, we cannot wait for cert_database_.

Sorry for the late insight, but I think we get the most advantages by doing the
asynchronous call GetNSSCertDatabaseForProfile independent of LoginUtils in the
User*UpdaterFactory.
The Factory should then construct the CertificateImporter(nss_database) and pass
the finished Importer to the User*Updater.
The User*Updater would cache the certificates policy until the importer is set.
The CertificateImporter can be removed from the base class and the
Device*Updater.
That requires only one additional pending_certificate_policy_ member in the
User*Updater, the nss_database dependency doesn't have to be passed around and
network updates wouldn't be delayed or the policy be reapplied.
The unit test will be simpler, because the CertificateImporter doesn't have to
be mocked before it's passed to the Updater.


Does that sound reasonable?

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:394: // Does ONC
import once the NSSCertDatabase which should be used for importing
nit: missing commas?  ..., which ... certificates, ...

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:79:
crypto::GetPublicSlotForChromeOSUser(private_user_.username_hash())));
is there no direct way to create slots for testing?

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:146: public_list_
= ListCertsInPublicSlot();
redundant, already done in AddCertificatesFromFile

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:184: bool
CleanupSlotContents() {
Maybe you know better, whether this Cleanup is still required in the TearDown of
the test fixture.
If not, please remove it.

[tbarzic, 2014-02-06 01:19:42]

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater.cc:89: void
NetworkConfigurationUpdater::ApplyPolicy() {
On 2014/02/05 11:03:27, pneubeck wrote:
> On 2014/02/04 23:44:56, tbarzic wrote:
> > We could also bail out here if database_ is not set (and if the
> > NetworkConfigurationUpdater supports certificate importing), but I'm
concerned
> > that it may take a while until NSS db is initialized (e.g. TPMTokenLoader
> logic
> > has a retry logic that allows delays up to 5 min).
> 
> In case of Device*Updater, we cannot wait for cert_database_.
> 
> Sorry for the late insight, but I think we get the most advantages by doing
the
> asynchronous call GetNSSCertDatabaseForProfile independent of LoginUtils in
the
> User*UpdaterFactory.
> The Factory should then construct the CertificateImporter(nss_database) and
pass
> the finished Importer to the User*Updater.
> The User*Updater would cache the certificates policy until the importer is
set.
> The CertificateImporter can be removed from the base class and the
> Device*Updater.
> That requires only one additional pending_certificate_policy_ member in the
> User*Updater, the nss_database dependency doesn't have to be passed around and
> network updates wouldn't be delayed or the policy be reapplied.
> The unit test will be simpler, because the CertificateImporter doesn't have to
> be mocked before it's passed to the Updater.
> 
> 
> Does that sound reasonable?

yes it does. I had a similar idea, but I didn't go that way because the approach
wouldn't work well with device policy.. but apparently, I don't have to worry
about that anymore. Also, there is a problem that User*Updater factory is called
before
GetNSSCertDatabaseForProfile can be used, but I think I can work around this.

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/net_internals/net_internals_ui.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chrome/browser/ui/webui...
chrome/browser/ui/webui/net_internals/net_internals_ui.cc:394: // Does ONC
import once the NSSCertDatabase which should be used for importing
On 2014/02/05 11:03:27, pneubeck wrote:
> nit: missing commas?  ..., which ... certificates, ...

yeah, the comment does not make much sense in the state it is :/

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
File chromeos/network/onc/onc_certificate_importer_impl_unittest.cc (right):

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:79:
crypto::GetPublicSlotForChromeOSUser(private_user_.username_hash())));
On 2014/02/05 11:03:27, pneubeck wrote:
> is there no direct way to create slots for testing?

none that I'm aware of, I'll look into exposing one in a follow up.

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:146: public_list_
= ListCertsInPublicSlot();
On 2014/02/05 11:03:27, pneubeck wrote:
> redundant, already done in AddCertificatesFromFile

Done.

https://codereview.chromium.org/148183013/diff/390001/chromeos/network/onc/on...
chromeos/network/onc/onc_certificate_importer_impl_unittest.cc:184: bool
CleanupSlotContents() {
On 2014/02/05 11:03:27, pneubeck wrote:
> Maybe you know better, whether this Cleanup is still required in the TearDown
of
> the test fixture.
> If not, please remove it.

I don't think it is (as databases are created in random tmp dir, which is
deleted after test).
removed

[tbarzic, 2014-02-06 01:20:25]

+xiyuan: owner for chrome/browser/ui/webui

[xiyuan, 2014-02-06 01:28:07]

https://codereview.chromium.org/148183013/diff/620001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.cc (right):

https://codereview.chromium.org/148183013/diff/620001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_browsertest.cc:109:
content::RunAllPendingInMessageLoop();
Should this and line 107 be inside #if defined(OS_CHROMEOS) block?

[tbarzic, 2014-02-06 01:36:12]

https://codereview.chromium.org/148183013/diff/620001/chrome/browser/ui/webui...
File chrome/browser/ui/webui/options/certificate_manager_browsertest.cc (right):

https://codereview.chromium.org/148183013/diff/620001/chrome/browser/ui/webui...
chrome/browser/ui/webui/options/certificate_manager_browsertest.cc:109:
content::RunAllPendingInMessageLoop();
On 2014/02/06 01:28:08, xiyuan wrote:
> Should this and line 107 be inside #if defined(OS_CHROMEOS) block?

yep

[xiyuan, 2014-02-06 01:42:15]

webui LGTM

[pneubeck (no reviews), 2014-02-06 09:37:46]

One last comment to fix the initialization.
After that lgtm.

Thanks for taking the time to implement this cleanly!

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:589:
PolicyAppliedImidiatellyIfProvidersInitialized) {
Imidiatelly -> Immediately

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_network_configuration_updater.h
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.h:70: //
policies. If there is |pending_policy_onc_|, it gets imported.
pending_policy_onc_ -> pending_certificates_onc_

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.h:109: // If
|ImportCertificates| is called before |certificate_importer_|, gets set
... is called before |SetCertificateImporter|...

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:54:
base::MessageLoopProxy::current()->PostTask(
I think this is too fragile. If the initialization process changes at any time
this will potentially break.

Please wait for chrome::NOTIFICATION_PROFILE_ADDED instead (e.g. like in
user_policy_signin_service_base.cc line 41).
Also the callback from OnDatabaseForImporter has to be bound to a WeakPtr, I
think.

The following should work:

class User..Factory {
  content::NotificationRegistrar registrar_;
}

..Factory::BuildServiceInstanceFor(context) {
  ...
  if (!skip_certificate_importer_creation_for_test) {
    registrar_.Add(this,
                   chrome::NOTIFICATION_PROFILE_ADDED,
                   content::Source<Profile>(profile));
  }
  return updater.release();
}

..Factory::Observe(
    int type,
    const content::NotificationSource& source,
    const content::NotificationDetails& details) {
  DCHECK_EQ(type, PROFILE_ADDED);
  Profile* profile = content::Source<Profile>(source).ptr();
  DCHECK(profile);
  UserNetworkConfigurationUpdater* updater = GetForProfile(profile);
  DCHECK(updater);
  GetNSSCertDatabaseForProfile(profile,
    base::Bind(&OnDatabaseForImporter, weak_ptr_factory_.GetWeakPtr(),
updater));
}

[tbarzic, 2014-02-06 23:02:36]

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/network_configuration_updater_unittest.cc:589:
PolicyAppliedImidiatellyIfProvidersInitialized) {
On 2014/02/06 09:37:46, pneubeck wrote:
> Imidiatelly -> Immediately

Done.

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File chrome/browser/chromeos/policy/user_network_configuration_updater.h
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.h:70: //
policies. If there is |pending_policy_onc_|, it gets imported.
On 2014/02/06 09:37:46, pneubeck wrote:
> pending_policy_onc_ -> pending_certificates_onc_

Done.

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater.h:109: // If
|ImportCertificates| is called before |certificate_importer_|, gets set
On 2014/02/06 09:37:46, pneubeck wrote:
> ... is called before |SetCertificateImporter|...

Done.

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
File
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc
(right):

https://codereview.chromium.org/148183013/diff/680001/chrome/browser/chromeos...
chrome/browser/chromeos/policy/user_network_configuration_updater_factory.cc:54:
base::MessageLoopProxy::current()->PostTask(
On 2014/02/06 09:37:46, pneubeck wrote:
> I think this is too fragile. If the initialization process changes at any time
> this will potentially break.
> 
> Please wait for chrome::NOTIFICATION_PROFILE_ADDED instead (e.g. like in
> user_policy_signin_service_base.cc line 41).
> Also the callback from OnDatabaseForImporter has to be bound to a WeakPtr, I
> think.
> 
> The following should work:
> 
> class User..Factory {
>   content::NotificationRegistrar registrar_;
> }
> 
> ..Factory::BuildServiceInstanceFor(context) {
>   ...
>   if (!skip_certificate_importer_creation_for_test) {
>     registrar_.Add(this,
>                    chrome::NOTIFICATION_PROFILE_ADDED,
>                    content::Source<Profile>(profile));
>   }
>   return updater.release();
> }
> 
> ..Factory::Observe(
>     int type,
>     const content::NotificationSource& source,
>     const content::NotificationDetails& details) {
>   DCHECK_EQ(type, PROFILE_ADDED);
>   Profile* profile = content::Source<Profile>(source).ptr();
>   DCHECK(profile);
>   UserNetworkConfigurationUpdater* updater = GetForProfile(profile);
>   DCHECK(updater);
>   GetNSSCertDatabaseForProfile(profile,
>     base::Bind(&OnDatabaseForImporter, weak_ptr_factory_.GetWeakPtr(),
> updater));
> }

Good point, though, I think it would better to observe the notification in the
service rather than factory.

[pneubeck (no reviews), 2014-02-07 08:56:34]

The advantage of having the profile_added observation in the factory would have
been that: The service has no test-specific code, no dependency on Profile or
Notifications.

But it doesn't make a huge difference in this particular case, so lgtm .

https://codereview.chromium.org/148183013/diff/1060001/chrome/browser/chromeo...
File chrome/browser/chromeos/policy/user_network_configuration_updater.cc
(right):

https://codereview.chromium.org/148183013/diff/1060001/chrome/browser/chromeo...
chrome/browser/chromeos/policy/user_network_configuration_updater.cc:73:
profile_(profile),
please don't store Profile*.
They tend to be misused and only a while ago we removed them from several
services.

[tbarzic, 2014-02-07 18:51:05]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-02-07 18:52:44]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/148183013/1160001

[commit-bot: I haz the power, 2014-02-07 19:45:03]

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

[commit-bot: I haz the power, 2014-02-07 19:45:04]

Retried try job too often on linux_rel for step(s) base_unittests,
browser_tests, cacheinvalidation_unittests, cc_unittests, check_deps,
check_deps2git, chromedriver_unittests, components_unittests,
content_browsertests, content_unittests, crypto_unittests,
google_apis_unittests, gpu_unittests, interactive_ui_tests, ipc_tests,
jingle_unittests, media_unittests, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests,
sandbox_linux_unittests, sql_unittests, sync_integration_tests, sync_unit_tests,
telemetry_perf_unittests, telemetry_unittests, unit_tests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=linux_rel&...

[tbarzic, 2014-02-07 21:50:43]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-02-07 21:53:37]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/148183013/1360002

[commit-bot: I haz the power, 2014-02-07 22:25:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/tbarzic@chromium.org/148183013/1360002

[commit-bot: I haz the power, 2014-02-08 01:29:07]

Change committed as 249881