Clean up Smart Lock cryptohome keys logic:

Clean up Smart Lock cryptohome keys logic:

1. Queue all cryptohome keys operations.
2. Roll ClearRemoteDevices as a specific case of SetRemoteDevices
3. Introduce new RefreshKeys operation using the existing add and remove keys
   operations to replace the current cryptohome keys with new keys.
4. After reauthenticating for setup, remove the old cryptohome keys.

BUG=432996
Committed: https://crrev.com/275152c34d8edc5346757288cfffe63444650faa
Cr-Commit-Position: refs/heads/master@{#309094}

[Tim Song, 2014-12-16 23:50:56]

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

[xiyuan, 2014-12-17 00:23:14]

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:51:
RunNextOperation();
What happens if there is a running operation? This could cause the two
operations to run concurrently. Maybe we should check |operation_in_progress_|
either here or inside RunNextOperation().

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:61:
RunNextOperation();
same here.

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:93:
std::deque<scoped_ptr<EasyUnlockGetKeysOperation>> read_operation_queue_;
Is it safe to put scoped_ptr in stl containers? Should we use ScopedPtrDeque
instead?

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:45:
user_context_, devices_.size(),
Think we need to follow existing code to use the user_context() from the create
operation. This is needed when the phone key inside user_context_ is changed
during the create operation. Once create operation finishes, cryptohome would no
longer accept the old phone key and we have to switch to use the just created
key.

[Tim Song, 2014-12-17 02:05:14]

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:51:
RunNextOperation();
On 2014/12/17 00:23:14, xiyuan wrote:
> What happens if there is a running operation? This could cause the two
> operations to run concurrently. Maybe we should check |operation_in_progress_|
> either here or inside RunNextOperation().

Done. Sorry, I meant to check operation_in_progress_ in RunNextOperation. I had
to shuffle some code around and replace this |operation_in_progress_| variable.

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:61:
RunNextOperation();
On 2014/12/17 00:23:14, xiyuan wrote:
> same here.

Done.

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:93:
std::deque<scoped_ptr<EasyUnlockGetKeysOperation>> read_operation_queue_;
On 2014/12/17 00:23:14, xiyuan wrote:
> Is it safe to put scoped_ptr in stl containers? Should we use ScopedPtrDeque
> instead?

Hmm... I'm not sure how safe it is. There might be unexpected interactions
depending on the stl library used. Unfortunately, ScopedPtrDeque is in cc which
isn't a dependency of chrome.

I'm doing manual memory management for these deques.

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:45:
user_context_, devices_.size(),
On 2014/12/17 00:23:14, xiyuan wrote:
> Think we need to follow existing code to use the user_context() from the
create
> operation. This is needed when the phone key inside user_context_ is changed
> during the create operation. Once create operation finishes, cryptohome would
no
> longer accept the old phone key and we have to switch to use the just created
> key.

Hmm... I tested this and easy sign-in still works across multiple sessions. The
create keys operation doesn't destroy the old key, so it should still be valid
to use for the delete keys operation, right?

I'm not super familiar with the cryptauth code, so if you think this is
necessary, I'll change it.

[xiyuan, 2014-12-17 03:26:33]

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:45:
user_context_, devices_.size(),
On 2014/12/17 02:05:14, Tim Song wrote:
> On 2014/12/17 00:23:14, xiyuan wrote:
> > Think we need to follow existing code to use the user_context() from the
> create
> > operation. This is needed when the phone key inside user_context_ is changed
> > during the create operation. Once create operation finishes, cryptohome
would
> no
> > longer accept the old phone key and we have to switch to use the just
created
> > key.
> 
> Hmm... I tested this and easy sign-in still works across multiple sessions.
The
> create keys operation doesn't destroy the old key, so it should still be valid
> to use for the delete keys operation, right?
> 
> I'm not super familiar with the cryptauth code, so if you think this is
> necessary, I'll change it.

This works now because we have one and only one key and RemoveKey op is actually
a no-op. Create key operation destroy the old key. After it returns, the old key
is no longer accepted by cryptohome. And RemoveKey would end up with
CRYPTOHOME_ERROR_AUTHORIZATION_KEY_NOT_FOUND instead of
CRYPTOHOME_ERROR_KEY_NOT_FOUND.  You can verify that in kernel log as well.

https://codereview.chromium.org/808563004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/808563004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:37: while
(!write_operation_queue_.empty()) {
How about include base/stl_util.h and do STLDeleteElements() ?

[tbarzic, 2014-12-17 03:34:06]

can you make sure you've synced with ToT (specifically
https://crrev.com/cc7df610b49e8ba6c60c8ccbcfb111f5d2084128)

[Tim Song, 2014-12-17 20:57:57]

Rebased to ToT.

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/1/chrome/browser/chromeos/logi...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:45:
user_context_, devices_.size(),
On 2014/12/17 03:26:33, xiyuan wrote:
> On 2014/12/17 02:05:14, Tim Song wrote:
> > On 2014/12/17 00:23:14, xiyuan wrote:
> > > Think we need to follow existing code to use the user_context() from the
> > create
> > > operation. This is needed when the phone key inside user_context_ is
changed
> > > during the create operation. Once create operation finishes, cryptohome
> would
> > no
> > > longer accept the old phone key and we have to switch to use the just
> created
> > > key.
> > 
> > Hmm... I tested this and easy sign-in still works across multiple sessions.
> The
> > create keys operation doesn't destroy the old key, so it should still be
valid
> > to use for the delete keys operation, right?
> > 
> > I'm not super familiar with the cryptauth code, so if you think this is
> > necessary, I'll change it.
> 
> This works now because we have one and only one key and RemoveKey op is
actually
> a no-op. Create key operation destroy the old key. After it returns, the old
key
> is no longer accepted by cryptohome. And RemoveKey would end up with
> CRYPTOHOME_ERROR_AUTHORIZATION_KEY_NOT_FOUND instead of
> CRYPTOHOME_ERROR_KEY_NOT_FOUND.  You can verify that in kernel log as well. 

Done.

https://codereview.chromium.org/808563004/diff/20001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc
(right):

https://codereview.chromium.org/808563004/diff/20001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.cc:37: while
(!write_operation_queue_.empty()) {
On 2014/12/17 03:26:33, xiyuan wrote:
> How about include base/stl_util.h and do STLDeleteElements() ?

Done. I used the base::STLElementDeleter to scope the deques.

[xiyuan, 2014-12-17 21:02:42]

lgtm

https://codereview.chromium.org/808563004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.h
(right):

https://codereview.chromium.org/808563004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.h:36:
const UserContext& user_context() { return user_context_; }
nit: Can we put back "const"?

[Tim Song, 2014-12-17 21:45:23]

https://codereview.chromium.org/808563004/diff/40001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.h
(right):

https://codereview.chromium.org/808563004/diff/40001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_create_keys_operation.h:36:
const UserContext& user_context() { return user_context_; }
On 2014/12/17 21:02:42, xiyuan wrote:
> nit: Can we put back "const"?

Done.

[tbarzic, 2014-12-18 10:49:05]

only nitty stuff...
LGTM

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:83: //
Called when the TPM key is ready to actually queue the refresh operation.
nit:
what do you think about:
...ready to be used for creating Easy Unlock key challenges.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:102: typedef
std::deque<EasyUnlockRefreshKeysOperation*> WriteOperationQueue;
nit: I think we can use:
using WriteOperationQueue = std::dequeue<EasyUnlockRefreshKeysOperation>;

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:47:
// keys operation.
you could mention reason for doing this (otherwise the comment seems a bit
redundant)

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.h
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.h:23:
// new list of keys. This operation is a simple combination of the remove and
nit: create and remove keys operation (to better match their actual order)?
Also, I'd drop "simple". and maybe use "sequence" instead of "combination".

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:115: // Even if the key
operation (removeKey or refreshKey) suceeded, we still
I don't think you're using removeKeys anywhere

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.h:69: // Called with the
user's credentials (eg: username and password) after the
is writing "e.g." as "eg:" a thing?
It's the first time I see it written in this manner :)

[Tim Song, 2014-12-18 22:32:22]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2014-12-18 22:33:50]

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

[Tim Song, 2014-12-18 22:42:55]

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:83: //
Called when the TPM key is ready to actually queue the refresh operation.
On 2014/12/18 10:49:05, tbarzic wrote:
> nit:
> what do you think about:
> ...ready to be used for creating Easy Unlock key challenges.

Done.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_key_manager.h:102: typedef
std::deque<EasyUnlockRefreshKeysOperation*> WriteOperationQueue;
On 2014/12/18 10:49:05, tbarzic wrote:
> nit: I think we can use:
> using WriteOperationQueue = std::dequeue<EasyUnlockRefreshKeysOperation>;

Done.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.cc:47:
// keys operation.
On 2014/12/18 10:49:05, tbarzic wrote:
> you could mention reason for doing this (otherwise the comment seems a bit
> redundant)

Done.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
File
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.h
(right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/chromeos/...
chrome/browser/chromeos/login/easy_unlock/easy_unlock_refresh_keys_operation.h:23:
// new list of keys. This operation is a simple combination of the remove and
On 2014/12/18 10:49:05, tbarzic wrote:
> nit: create and remove keys operation (to better match their actual order)?
> Also, I'd drop "simple". and maybe use "sequence" instead of "combination".

Done.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.cc (right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.cc:115: // Even if the key
operation (removeKey or refreshKey) suceeded, we still
On 2014/12/18 10:49:05, tbarzic wrote:
> I don't think you're using removeKeys anywhere

Done.

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
File chrome/browser/signin/easy_unlock_service_regular.h (right):

https://codereview.chromium.org/808563004/diff/60001/chrome/browser/signin/ea...
chrome/browser/signin/easy_unlock_service_regular.h:69: // Called with the
user's credentials (eg: username and password) after the
On 2014/12/18 10:49:05, tbarzic wrote:
> is writing "e.g." as "eg:" a thing?
> It's the first time I see it written in this manner :)

Done.

[commit-bot: I haz the power, 2014-12-18 22:52:38]

Committed patchset #5 (id:80001)

[commit-bot: I haz the power, 2014-12-18 22:53:49]

Patchset 5 (id:??) landed as
https://crrev.com/275152c34d8edc5346757288cfffe63444650faa
Cr-Commit-Position: refs/heads/master@{#309094}

[benwells, 2014-12-23 02:10:26]

A revert of this CL (patchset #5 id:80001) has been created in
https://codereview.chromium.org/789793005/ by benwells@chromium.org.

The reason for reverting is: Once this change landed chromeos file manager tests
started failing across the MSAN bots.

This is the most likely culprit, so reverting speculatively. If this does not
help will reland this change.

See
http://build.chromium.org/p/chromium.memory.fyi/builders/Linux%20ChromeOS%20M....