Easy Sign-in: Use TPM RSA key to sign nonce in sign-in protocol

This adds utility for creating user-specific RSA key pair in system TPM slot
that is used for easy sign-in protocol, and for signing data provided by
Easy Unlock app using the created private key.

Per user public keys are kept in the local state and added to challenge data when challenges
are created (while resetting cryptohome sign-in secrets).
During challenge creation, existence of the Easy Sign-in TPM key is
checked for user, and the key pair is created if necessary.
Additionally, key pair presence is ensured when EasyUnlockService is
started after user log in. This is done to handle the case where Easy Unlock
has previously been set up. At this time, it is verified that
the private key actually
exists in the TPM slot.

Mapping from user id to public TPM key is kept in local state
so it can be accessed on sign in screen (as it will be needed before a user logs in;
the public key is used to identify the private key in the system slot)

BUG=409027
TEST=Confirmed easy sign-in works

Committed: https://crrev.com/cc7df610b49e8ba6c60c8ccbcfb111f5d2084128
Cr-Commit-Position: refs/heads/master@{#308431}

[tbarzic, 2014-12-02 07:30:27]

tbarzic@chromium.org changed reviewers:
+ pneubeck@chromium.org, xiyuan@chromium.org

[tbarzic, 2014-12-02 07:30:28]

Can you please take a look?
xiyuan: main review
pneubeck: easy_unlock_tpm_key_manager.cc (for usage of chromeos::platform_keys
and the system slot)

(note that this still needs a cl for starting TPMTokenLoader before a user
login)

[xiyuan, 2014-12-02 23:15:59]

Sorry, the comments were on PS11.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:286:
SetKeyInLocalState(user_id, public_key);
Since the private key is protected in TPM, can we only store the public key in
local state? Storing two copies does not seem to have extra benefit. What do you
think?

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:49:
~EasyUnlockTpmKeyManager();
nit: override

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:65: //
|callback|: If the method cannot return immediately, callled when the key
nit: callled -> called

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:67: bool
IsTpmKeyPresent(const std::string& user_id,
nit: IsTpmKeyPresent sounds like just a key presence check and not obvious that
it would create the key. Maybe rename it to something like PrepareTpmKey or
GetTpmKey.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:172:
EXPECT_FALSE(true) << "Not reached";
nit: ADD_FAILURE() << "Not reached";

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/extensio...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc:617:
return false;
nit: SetError('No EasyUnlockTpmKeyManager.') before return false?

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:257:
EasyUnlockTpmKeyManager::ResetLocalStateForUser(user_id);
nit: fix indent

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:692: // If this is called beforei
the session is started, the chances are Chrome
nit: beforei -> before

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:31: #include
"chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_factory.h"
nit: Seems not used.

[tbarzic, 2014-12-03 19:10:29]

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:286:
SetKeyInLocalState(user_id, public_key);
On 2014/12/02 23:15:58, xiyuan wrote:
> Since the private key is protected in TPM, can we only store the public key in
> local state? Storing two copies does not seem to have extra benefit. What do
you
> think?

yeah, sounds reasonable.. Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:49:
~EasyUnlockTpmKeyManager();
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: override

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:65: //
|callback|: If the method cannot return immediately, callled when the key
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: callled -> called

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:67: bool
IsTpmKeyPresent(const std::string& user_id,
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: IsTpmKeyPresent sounds like just a key presence check and not obvious
that
> it would create the key. Maybe rename it to something like PrepareTpmKey or
> GetTpmKey.

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:172:
EXPECT_FALSE(true) << "Not reached";
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: ADD_FAILURE() << "Not reached";

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/extensio...
File
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc
(right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/extensio...
chrome/browser/extensions/api/easy_unlock_private/easy_unlock_private_api.cc:617:
return false;
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: SetError('No EasyUnlockTpmKeyManager.') before return false?

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service.cc (right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:257:
EasyUnlockTpmKeyManager::ResetLocalStateForUser(user_id);
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: fix indent

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service.cc:692: // If this is called beforei
the session is started, the chances are Chrome
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: beforei -> before

Done.

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/729803002/diff/200001/chrome/browser/signin/e...
chrome/browser/signin/easy_unlock_service_regular.cc:31: #include
"chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_factory.h"
On 2014/12/02 23:15:58, xiyuan wrote:
> nit: Seems not used.

Done.

[xiyuan, 2014-12-03 19:25:29]

LGTM

Thank you for taking care of this. :) Please wait for pneubeck@ for the crypto
related changes.

[pneubeck (no reviews), 2014-12-05 12:16:05]

I looked at the crypto part of the manager, which lgtm.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:216:
bool EasyUnlockTpmKeyManager::SetGetSystemSlotTimeoutMs(size_t timeout_ms) {
SetTimeout sounds like you would set the timeout duration but not actually
starting the timeout.
Maybe call it Start.*Timeout ?

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:216:
bool EasyUnlockTpmKeyManager::SetGetSystemSlotTimeoutMs(size_t timeout_ms) {
it seems to be invalid/uneffective if this is called before PerpareTpmKey, is it
worth checking that?

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:286:
get_tpm_slot_weak_ptr_factory_.InvalidateWeakPtrs();
probably document what effect this should have. Currently it aborts silently
both any other pending CreateKeyInSystemSlot and any Timeout.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:290:
base::Bind(&CreateTpmKeyPairOnWorkerThread,
what if this hangs? why does the timeout not skip waiting for this task as well?

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:325:
get_tpm_slot_weak_ptr_factory_.InvalidateWeakPtrs();
same comment as in line 286

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:22:
class EasyUnlockTpmKeyManager : public KeyedService {
I find the state transitions of this implementation rather convoluted and wonder
whether a more explicit enumeration of the states would have been more easy to
reason about.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:56: //
If called, posts a delayed task that cancels |PrepareTpmKey| in case
... and cancels all previously started timeouts.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:58: //
system slot times out |PrepareTpmKey| callback will be called with an empty
nit: comma after 'out'

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:60: bool
SetGetSystemSlotTimeoutMs(size_t timeout_ms);
currently must be called after PrepareTpmKey to have the intended effect.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:235:
// It returns whether the key has been imported. In order for the methos to
typo: methos -> methods

[tbarzic, 2014-12-11 23:43:28]

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:216:
bool EasyUnlockTpmKeyManager::SetGetSystemSlotTimeoutMs(size_t timeout_ms) {
On 2014/12/05 12:16:04, pneubeck wrote:
> SetTimeout sounds like you would set the timeout duration but not actually
> starting the timeout.
> Maybe call it Start.*Timeout ?

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:216:
bool EasyUnlockTpmKeyManager::SetGetSystemSlotTimeoutMs(size_t timeout_ms) {
On 2014/12/05 12:16:04, pneubeck wrote:
> it seems to be invalid/uneffective if this is called before PerpareTpmKey, is
it
> worth checking that?

Hm, it should still be effective if it's called just before PrepareTpmKey,
though the timeout would be shorter (in respect to PrepareTpmKey start).
I'm not really sure this is worth checking, though.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:286:
get_tpm_slot_weak_ptr_factory_.InvalidateWeakPtrs();
On 2014/12/05 12:16:04, pneubeck wrote:
> probably document what effect this should have. Currently it aborts silently
> both any other pending CreateKeyInSystemSlot and any Timeout.

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:290:
base::Bind(&CreateTpmKeyPairOnWorkerThread,
On 2014/12/05 12:16:04, pneubeck wrote:
> what if this hangs? why does the timeout not skip waiting for this task as
well?

My main concern is with GetSystemSlot hanging indefinitely during login (if
there are user flags set, we'll wait for PrepareTpmKey to finish before
proceeding; good news is that under these circumstances in the vast majority of
cases PrepareTpmKey should return synchronously, without actually loading the
system slot).

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:325:
get_tpm_slot_weak_ptr_factory_.InvalidateWeakPtrs();
On 2014/12/05 12:16:04, pneubeck wrote:
> same comment as in line 286

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:22:
class EasyUnlockTpmKeyManager : public KeyedService {
On 2014/12/05 12:16:05, pneubeck wrote:
> I find the state transitions of this implementation rather convoluted and
wonder
> whether a more explicit enumeration of the states would have been more easy to
> reason about.

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:56: //
If called, posts a delayed task that cancels |PrepareTpmKey| in case
On 2014/12/05 12:16:05, pneubeck wrote:
> ... and cancels all previously started timeouts.

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:58: //
system slot times out |PrepareTpmKey| callback will be called with an empty
On 2014/12/05 12:16:05, pneubeck wrote:
> nit: comma after 'out'

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.h:60: bool
SetGetSystemSlotTimeoutMs(size_t timeout_ms);
On 2014/12/05 12:16:05, pneubeck wrote:
> currently must be called after PrepareTpmKey to have the intended effect.

Done.

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc
(right):

https://codereview.chromium.org/729803002/diff/320001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager_unittest.cc:235:
// It returns whether the key has been imported. In order for the methos to
On 2014/12/05 12:16:05, pneubeck wrote:
> typo: methos -> methods

Done.

[pneubeck (no reviews), 2014-12-14 09:39:37]

still lgtm

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:188:
CHECK_EQ(user_id_, user_id);
can't you just remove the |user_id| argument from this function?

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:190:
std::string key = GetPublicTpmKey(user_id);
nit: can be moved after the next if-block

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:195:
create_tpm_key_state_ == CREATE_TPM_KEY_NOT_STARTED)
nit: missing braces

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:222:
create_tpm_key_state_ == CREATE_TPM_KEY_GOT_SYSTEM_SLOT)
nit: missing braces

[tbarzic, 2014-12-15 20:05:47]

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc
(right):

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:188:
CHECK_EQ(user_id_, user_id);
On 2014/12/14 09:39:37, pneubeck wrote:
> can't you just remove the |user_id| argument from this function?

Done.

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:190:
std::string key = GetPublicTpmKey(user_id);
On 2014/12/14 09:39:37, pneubeck wrote:
> nit: can be moved after the next if-block

Done.

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:195:
create_tpm_key_state_ == CREATE_TPM_KEY_NOT_STARTED)
On 2014/12/14 09:39:37, pneubeck wrote:
> nit: missing braces

Done.

https://codereview.chromium.org/729803002/diff/360001/chrome/browser/chromeos...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_tpm_key_manager.cc:222:
create_tpm_key_state_ == CREATE_TPM_KEY_GOT_SYSTEM_SLOT)
On 2014/12/14 09:39:37, pneubeck wrote:
> nit: missing braces

Done.

[tbarzic, 2014-12-15 20:05:53]

The CQ bit was checked by tbarzic@chromium.org

[commit-bot: I haz the power, 2014-12-15 20:07:05]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/729803002/400001

[commit-bot: I haz the power, 2014-12-15 21:54:44]

Committed patchset #21 (id:400001)

[commit-bot: I haz the power, 2014-12-15 21:56:21]

Patchset 21 (id:??) landed as
https://crrev.com/cc7df610b49e8ba6c60c8ccbcfb111f5d2084128
Cr-Commit-Position: refs/heads/master@{#308431}