[chromiumos] Start TPM token initialization re-tries on login

[chromiumos] Start TPM token initialization re-tries on login

The current TPM token setup logic attempts initialization once after
the user logs in. Asynchronous attempts for TPM token setup are not
triggered unless the user opens the VPN or WiFi config panel (attempt
retries are triggered via a call to CertLibraryImpl::RequestCertificates()).

This means that if the first attempt fails and the user never opens
up the WiFi config or VPN config panel, the TPM token will stay
uninitialized. This breaks the certificate manager (list of certs is
empty), the SPDY proxy extension, amongst other things.

Essentially, any part of the network subsystem that depends on the
private hardware NSS slot (via crypto::GetPrivateNSSKeySlot) stays
broken if the first attempt fails. (So, this is not just an issue with
the list of certs not being displayed correctly).

This CL changes that so that retry logic for TPM token init is
triggered right after the user logs in.

BUG=chromium-os:20933
TEST=Log in, verify from logs that TPM initialization attempts start
     immediately after. Verify that the missing certificates issue
     no longer happens.

Change-Id: I9c609bdb198a88db8ceb2019cc92c19d1983bc05


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

[gauravsh, 2011-10-09 00:17:27]

For now this is just an RFC CL to figure out if going down this path is ok.

We want the TPM token initialization attempt retry logic to be triggered right
after the user logs in and not tie them to whether or not they open the UI
elements corresponding to the certificate manager, wifi options panel or vpn
options panel since GetPrivateNSSKeySlot() could be called anytime (e.g. SPDY,
or SSL subsystem).

Pending/Considering:
1) Adding a back-off to the re-try duration (post delayed task). May reduce jank
on first login. 

2) Make {wifi|vpn}_config_view.{cc|h} not directly call RequestCertificates()
but wait for a callback that the token init is complete, the cert list has been
updated and can be retrieved now.

[gauravsh, 2011-10-09 00:22:08]

On 2011/10/09 00:17:27, gauravsh wrote:
> For now this is just an RFC CL to figure out if going down this path is ok.
> 
> We want the TPM token initialization attempt retry logic to be triggered right
> after the user logs in and not tie them to whether or not they open the UI
> elements corresponding to the certificate manager, wifi options panel or vpn
> options panel since GetPrivateNSSKeySlot() could be called anytime (e.g. SPDY,
> or SSL subsystem).
> 
> Pending/Considering:
> 1) Adding a back-off to the re-try duration (post delayed task). May reduce
jank
> on first login. 
> 
> 2) Make {wifi|vpn}_config_view.{cc|h} not directly call RequestCertificates()
> but wait for a callback that the token init is complete, the cert list has
been
> updated and can be retrieved now.

Also, if there is a standard way to measure the delta of jankiness (if any),
please let me know. I tested a debug build on an Alex, and also looked at the
LOG() messages and didn't notice anything particular egregious.

[stevenjb, 2011-10-10 17:18:13]

One reason we didn't initialize the TPM on login originally is that it takes
time and is not usually necessary, so we only initialize it when we need to.
This does introduce a slight delay to the UI, but that was considered
acceptable.

I don't think it is unreasonable to always initialize it, but I still think it
would be preferable to initialize it only when we might need it, e.g. when the
certificate manager UI is loaded or a proxy that requires certificates loads.

http://codereview.chromium.org/8212003/diff/2001/crypto/nss_util.cc
File crypto/nss_util.cc (left):

http://codereview.chromium.org/8212003/diff/2001/crypto/nss_util.cc#oldcode239
crypto/nss_util.cc:239: EnsureTPMTokenReady();
Why remove this? If the new logic gets changed, or isn't called (guest mode?
developer testing?) we might still want to initialize the TPM here, and it
shouldn't cost anything if it's already been initialized?

[stevenjb, 2011-10-10 17:53:35]

I just spoke with kmixter@, and it sounds like the Chrome overhead for this
should actually be minimal, so I think this is the right approach.

[gauravsh, 2011-10-10 20:55:49]

PTAL. I made changes to wifi|vpn_config_view to not make a call to
RequestCertificates() but instead setup a callback if the certificates are yet
to be loaded.

http://codereview.chromium.org/8212003/diff/2001/crypto/nss_util.cc
File crypto/nss_util.cc (left):

http://codereview.chromium.org/8212003/diff/2001/crypto/nss_util.cc#oldcode239
crypto/nss_util.cc:239: EnsureTPMTokenReady();
On 2011/10/10 17:18:13, Steven Bennetts wrote:
> Why remove this? If the new logic gets changed, or isn't called (guest mode?
> developer testing?) we might still want to initialize the TPM here, and it
> shouldn't cost anything if it's already been initialized?

Is it used anywhere else? I couldn't find it.

The only place EnableTPMTokenForNSS I saw was in user_manager.cc in
NotifyOnLogin(). A call to RequestCertificates() already makes an initial
attempt to call EnsureTPMTokenReady() before posting a delayed task. So this is
redundant. We want to route all calls to this initialization code through
CertLibrary::RequestCertificates() since that handles the re-try part
(EnsureTPMTokenReady() asynchronously checks if the TPM token has been
initialized, it doesn't actually trigger the initialization).

[stevenjb, 2011-10-11 16:44:09]

This looks great, just one question:
If we do not provide kLoadOpencryptoki, will CertLibrary::CertificatesLoaded()
return true? If so, then this should be fine. If not, we may want to change that
so that we don't display "Loading..." UI when we are not actually loading certs.

[gauravsh, 2011-10-11 17:04:59]

On Tue, Oct 11, 2011 at 9:44 AM,  <stevenjb@chromium.org> wrote:
> This looks great, just one question:
> If we do not provide kLoadOpencryptoki, will
> CertLibrary::CertificatesLoaded()
> return true? If so, then this should be fine. If not, we may want to change
> that
> so that we don't display "Loading..." UI when we are not actually loading
> certs.

I think we probably want to get rid of that flag and always use the
opencryptoki path. The bugs referenced in TODO are both fixed and it
can be got rid of.

If there is no TPM, line 121 of RequestCertificates() ensures that we
don't immediately try to load the certificates, which eventually will
set certificates_loaded_ to true.





>
>
> http://codereview.chromium.org/8212003/
>



-- 
-g

[Greg Spencer (Chromium), 2011-10-11 17:13:38]

LGTM from my perspective.

[stevenjb, 2011-10-11 18:01:02]

OK, cool, just wanted to double check that.
LGTM

[commit-bot: I haz the power, 2011-10-12 00:08:33]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/8212003/6001

[commit-bot: I haz the power, 2011-10-12 00:08:36]

Presubmit check for 8212003-6001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: crypto/nss_util.cc

Presubmit checks took 1.3s to calculate.

[gauravsh, 2011-10-12 00:14:28]

Wan-Teh: I need OWNER approval. The (minor) change is to a Chrome OS specific
piece of code in nss_utility.cc. It removes a redundant call that is not needed
any more (comments have a better discussion).

[wtc, 2011-10-12 17:20:34]

Patch Set 3 LGTM.

High-level comment:

I believe we don't need to add the cert_library_ member
to UserManager.  It can be a local variable in
UserManager::NotifyOnLogin().

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:699:
ALLOW_THIS_IN_INITIALIZER_LIST(method_factory_(this)) {

The UserManager constructor should initialize
cert_library_ to NULL.

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:771:
cert_library_->RequestCertificates();

If this is the only place where cert_library_ is used,
it doesn't need to be a class member.  It can be just
a local variable in this block.

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.h (right):

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.h:34: class CertLibrary;

Nit: may want to list the forward declarations in alphabetical
order.

[gauravsh, 2011-10-12 17:37:01]

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/user_manager.cc (right):

http://codereview.chromium.org/8212003/diff/6001/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/user_manager.cc:771:
cert_library_->RequestCertificates();
On 2011/10/12 17:20:34, wtc wrote:
> 
> If this is the only place where cert_library_ is used,
> it doesn't need to be a class member.  It can be just
> a local variable in this block.

Done.

[wtc, 2011-10-12 17:39:31]

Patch Set 4 LGTM.  Thanks!

[commit-bot: I haz the power, 2011-10-12 17:39:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/gauravsh@chromium.org/8212003/13002

[commit-bot: I haz the power, 2011-10-12 20:01:57]

Change committed as 105136