[Sync] Implement keystore migration support.

[Sync] Implement keystore migration support.

We'll now trigger migration if the keystore key is available, the cryptographer
is ready, and the nigori node isn't already properly migrated. Note that this
means we won't trigger migration without at least the implicit gaia password
already available to the cryptographer, in order to support backwards
compatibility with older clients. Eventually that will change.

In addition, once a nigori node has been migrated, any client that supports
keystore encryption will follow the new encryption constraints, whether
or not the --sync-keystore-encryption flag is passed. This means that if
the user sets a custom passphrase, encrypt everything will also be enabled
(and vice versa).

Migration-aware conflict resolution is not implemented yet.

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

[Nicolas Zea, 2012-08-31 00:07:50]

PTAL! Still need to implement the conflict resolution support and modifications
to the UI, but those will be in subsequent patches.

[tim (not reviewing), 2012-09-06 01:04:20]

Still wrapping my head around all this :)  We discussed the main issue I noticed
so far offline, just a few other comments for now and I'll continue looking
tomorrow.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:211: // migrated nigori node,
then we must at least have pending keys, and
This confused me a bit.  If we had pending keys, we wouldn't be in
SetEncryptionPassphrase.  Perhaps re-word this comment -- the part about "just
the client attempting to update the gaia passphrase" is useful.  The fact that
we've migrated implying that we don't need to set a new "first time" passphrase
for encryption is also useful.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:218:
SetCustomPassphrase(passphrase, &trans, &node);
Add comment (or cleverly rename) that this is tentative on certain criteria like
not having an existing custom set.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:303: } else {  //
nigori_has_explicit_passphrase == true
update this comment to reflect the if.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:337: // Once we've migrated to
keystore, we're only ever decrypting an explicit
nit - decrypting keys derived from an explicit passphrase*

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:344:
DecryptExplicitPassphrase(passphrase, &trans, &node);
nit - DecryptPendingExplicitPassphraseKeys ?

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:903: // keys after migrating
to keystore support is if a custom passphrase was
Is there an assertion we can make (DCHECK) that we've migrated to KS?

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:967: const std::string&
bootstrap_token,
Why do we pass this in if we're poking around at the cryptographer below anyway?

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:979: // passphrase).
I guess this is still relevant...

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1229: return false;
We don't need to notify if this fails because there's nothing we can do until we
happen to run another GetKey and wind up trying to SetKeystoreKey again?

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1233:
temp_cryptographer.CanDecrypt(keystore_bootstrap)) {
"bootstrap tokens" as defined by Cryptographer are not syncable across different
machines because of use of the Encryptor class.  They aren't "encrypted" in the
same way as Cryptographer::Encrypt works.  We'll need to change this to create a
different type of "bootstrap token" (possibly named something else, like
keystore_hotwire or _aid_token or migration_aid or _initializer_token or
something, to avoid mind melting confusion).

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.h (right):

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.h:211: // Helper method for
determining if migratino of a nigori node should be
migratino!

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:99: enum PassphraseState {
We called this PassphraseState elsewhere but it's confusing, since 'State' can
mean some sort of payload / collection of data associated with a payload... I
think PassphraseType would be clearer.

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:111: [default = IMPLICIT_PASSPHRASE];
it... seems a bit weird to make the default the value with 'Deprecated' written
next to it.

[Nicolas Zea, 2012-09-06 21:45:27]

Comments addressed, PTAL. s/PassphraseState/PassphraseType made this patch a lot
bigger, but it's just renames.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:211: // migrated nigori node,
then we must at least have pending keys, and
On 2012/09/06 01:04:20, timsteele wrote:
> This confused me a bit.  If we had pending keys, we wouldn't be in
> SetEncryptionPassphrase.  Perhaps re-word this comment -- the part about "just
> the client attempting to update the gaia passphrase" is useful.  The fact that
> we've migrated implying that we don't need to set a new "first time"
passphrase
> for encryption is also useful.

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:218:
SetCustomPassphrase(passphrase, &trans, &node);
On 2012/09/06 01:04:20, timsteele wrote:
> Add comment (or cleverly rename) that this is tentative on certain criteria
like
> not having an existing custom set.

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:303: } else {  //
nigori_has_explicit_passphrase == true
On 2012/09/06 01:04:20, timsteele wrote:
> update this comment to reflect the if.

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:337: // Once we've migrated to
keystore, we're only ever decrypting an explicit
On 2012/09/06 01:04:20, timsteele wrote:
> nit - decrypting keys derived from an explicit passphrase*

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:344:
DecryptExplicitPassphrase(passphrase, &trans, &node);
On 2012/09/06 01:04:20, timsteele wrote:
> nit - DecryptPendingExplicitPassphraseKeys ?

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:903: // keys after migrating
to keystore support is if a custom passphrase was
On 2012/09/06 01:04:20, timsteele wrote:
> Is there an assertion we can make (DCHECK) that we've migrated to KS?

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:967: const std::string&
bootstrap_token,
On 2012/09/06 01:04:20, timsteele wrote:
> Why do we pass this in if we're poking around at the cryptographer below
anyway?

Due to the old SetEncryptionPassphrase logic, we need to be able to update the
bootstrap token when we have a pending old implicit passphrase and provide the
current implicit passphrase, which we can't do from here.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:979: // passphrase).
On 2012/09/06 01:04:20, timsteele wrote:
> I guess this is still relevant...

Right, see my response above.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1229: return false;
On 2012/09/06 01:04:20, timsteele wrote:
> We don't need to notify if this fails because there's nothing we can do until
we
> happen to run another GetKey and wind up trying to SetKeystoreKey again?

If this fails, we'll remain in a pending keys state, and since nothing has
changed don't need to notify. This particular condition failing means we're
probably in an explicit passphrase state, so need to prompt the user for a
passphrase anyways.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1233:
temp_cryptographer.CanDecrypt(keystore_bootstrap)) {
On 2012/09/06 01:04:20, timsteele wrote:
> "bootstrap tokens" as defined by Cryptographer are not syncable across
different
> machines because of use of the Encryptor class.  They aren't "encrypted" in
the
> same way as Cryptographer::Encrypt works.  We'll need to change this to create
a
> different type of "bootstrap token" (possibly named something else, like
> keystore_hotwire or _aid_token or migration_aid or _initializer_token or
> something, to avoid mind melting confusion).

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.h (right):

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.h:211: // Helper method for
determining if migratino of a nigori node should be
On 2012/09/06 01:04:20, timsteele wrote:
> migratino!

Done.

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:99: enum PassphraseState {
On 2012/09/06 01:04:20, timsteele wrote:
> We called this PassphraseState elsewhere but it's confusing, since 'State' can
> mean some sort of payload / collection of data associated with a payload... I
> think PassphraseType would be clearer.

Done. And changed the chrome enum as well.

http://codereview.chromium.org/10916036/diff/5003/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:111: [default = IMPLICIT_PASSPHRASE];
On 2012/09/06 01:04:20, timsteele wrote:
> it... seems a bit weird to make the default the value with 'Deprecated'
written
> next to it.

I think it's correct though. The "migrated" state has certain requirements,
which an empty/default nigori node does not meet.

[tim (not reviewing), 2012-09-10 20:41:51]

s/keystore_decryptor/keystore_decryptor_token, or something, to denote that it's
just state and not a component responsible for taking some decryption action.

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10916036/diff/5003/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1229: return false;
On 2012/09/06 21:45:27, nzea wrote:
> On 2012/09/06 01:04:20, timsteele wrote:
> > We don't need to notify if this fails because there's nothing we can do
until
> we
> > happen to run another GetKey and wind up trying to SetKeystoreKey again?
> 
> If this fails, we'll remain in a pending keys state, and since nothing has
> changed don't need to notify. This particular condition failing means we're
> probably in an explicit passphrase state, so need to prompt the user for a
> passphrase anyways.

How is the explicit passphrase UI triggered, then?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:681: nigori_passphrase_type ==
CUSTOM_PASSPHRASE)) {
in theory passphrase_type_ = frozen implicit can squeak through here, if
nigori_passphrase_type_ is something different. should there be a dcheck?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:692: << "due to full
encryption.";
I think a comment here explaining that this is effectively the "tin foil hat
upgrade", where another client tried to enable full encryption with an implicit
passphrase, and we hoisted their account to full encryption with a custom
passphrase (rather than keystore / google provided passphrase). It's a "late"
version of this upgrade, though, as opposed to the one that is done in
AttemptToMigrateNigoriToKeystore

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:698: // It's possible that
while we're waiting for migration another old
"another old client"... what does 'old' mean here? i.e. does it include current
builds (that understand the new fields like frozen_keybag) as well as older ones
that don't?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:700: if
(nigori.frozen_keybag() &&
can we call this keybag_is_frozen()?  Everytime I see frozen_keybag I think it's
actually a keybag.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:702: passphrase_type_ =
CUSTOM_PASSPHRASE;
before setting, can we DCHECK that passphrase_type is IMPLICIT?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:711: // We only update the
default passphrase if this was a new explicit
nit - '...update the default key...' / need_new_default_key would be clearer.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:735: << "bootstrap key.";
Can we surface this somehow? Is it a LOG(ERROR) because the keystore key can
never change, so we should be able to decrypt?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1008: // Will do nothing if
we're already properly migrated or unable to migrate.
"Will do nothing if we're already properly migrated or unable to migrate (in
other words, if ShouldTriggerMigration is false)."

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1075: // If the nigori is
already migrated but has an old passphrase state,
What is an "old passphrase state"?

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1077: // does not have full
encryption, or the nigori has an implicit passphrase
"if the nigori has an explicit passphrase but does not have full encryption"...
that should have been caught by the original attempt to migrate, right?  So this
is purely for a user-initiated change on a deprecated client *after* the
original migration event horizon?  Try to be more clear about that in the
comments... it's hard to tease apart what's part of the normal migration flow
and what is part of the "late pass" migration flows.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1118:
migrated_nigori.set_keystore_migration_time(migration_time_ms_);
Should we be doing this before actually completing the migration?  I was going
to ask why we can't just check this field in IsNigoriMigratedToKeystore...

http://codereview.chromium.org/10916036/diff/4005/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10916036/diff/4005/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:53: optional bool frozen_keybag = 2;
Explain why this is called "frozen keybag" in a comment.

[Nicolas Zea, 2012-09-11 14:21:31]

Comments addressed, PTAL.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:681: nigori_passphrase_type ==
CUSTOM_PASSPHRASE)) {
On 2012/09/10 20:41:52, timsteele wrote:
> in theory passphrase_type_ = frozen implicit can squeak through here, if
> nigori_passphrase_type_ is something different. should there be a dcheck?

I actually think that's okay. This leaves some room for us to add support for
switching to a custom passphrase from frozen nigori in a later version. Updated
the comment to reflect that.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:692: << "due to full
encryption.";
On 2012/09/10 20:41:52, timsteele wrote:
> I think a comment here explaining that this is effectively the "tin foil hat
> upgrade", where another client tried to enable full encryption with an
implicit
> passphrase, and we hoisted their account to full encryption with a custom
> passphrase (rather than keystore / google provided passphrase). It's a "late"
> version of this upgrade, though, as opposed to the one that is done in
> AttemptToMigrateNigoriToKeystore

Done.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:698: // It's possible that
while we're waiting for migration another old
On 2012/09/10 20:41:52, timsteele wrote:
> "another old client"... what does 'old' mean here? i.e. does it include
current
> builds (that understand the new fields like frozen_keybag) as well as older
ones
> that don't? 

Old client means those that do not have keystore encryption enabled. Reworded.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:700: if
(nigori.frozen_keybag() &&
On 2012/09/10 20:41:52, timsteele wrote:
> can we call this keybag_is_frozen()?  Everytime I see frozen_keybag I think
it's
> actually a keybag.

Done.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:702: passphrase_type_ =
CUSTOM_PASSPHRASE;
On 2012/09/10 20:41:52, timsteele wrote:
> before setting, can we DCHECK that passphrase_type is IMPLICIT?

That dcheck might fire due to nigori races. We handle that by writing a migrated
custom passphrase state into the nigori.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:735: << "bootstrap key.";
On 2012/09/10 20:41:52, timsteele wrote:
> Can we surface this somehow? Is it a LOG(ERROR) because the keystore key can
> never change, so we should be able to decrypt?

Right, if we failed to decrypt a keystore decryptor blob that is non-empty, and
the keystore key is non-empty, something went wrong. I'm planning to surface the
migration state and whether we have a keystore key to about:sync in a subsequent
patch. We'll then be able to detect this situation because the cryptographer
will have pending keys and yet the nigori is migrated with keystore passphrase
type and we have a keystore key.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1008: // Will do nothing if
we're already properly migrated or unable to migrate.
On 2012/09/10 20:41:52, timsteele wrote:
> "Will do nothing if we're already properly migrated or unable to migrate (in
> other words, if ShouldTriggerMigration is false)."

Done.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1075: // If the nigori is
already migrated but has an old passphrase state,
On 2012/09/10 20:41:52, timsteele wrote:
> What is an "old passphrase state"?

Rewritten to reflect that this is detecting a migrated nigori that does not
reflect the explicit passphrase state properly.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1077: // does not have full
encryption, or the nigori has an implicit passphrase
On 2012/09/10 20:41:52, timsteele wrote:
> "if the nigori has an explicit passphrase but does not have full
encryption"...
> that should have been caught by the original attempt to migrate, right?  So
this
> is purely for a user-initiated change on a deprecated client *after* the
> original migration event horizon?  Try to be more clear about that in the
> comments... it's hard to tease apart what's part of the normal migration flow
> and what is part of the "late pass" migration flows.

Right. Noted that these cases are also detecting the case where other clients
introduce invalid passphrase/encryption states.

http://codereview.chromium.org/10916036/diff/4005/sync/internal_api/sync_encr...
sync/internal_api/sync_encryption_handler_impl.cc:1118:
migrated_nigori.set_keystore_migration_time(migration_time_ms_);
On 2012/09/10 20:41:52, timsteele wrote:
> Should we be doing this before actually completing the migration?  I was going
> to ask why we can't just check this field in IsNigoriMigratedToKeystore...

Moved this down so that we don't set migration_time_ms unless we're going to
complete the migration process. Also added condition in
IsNigoriMigratedToKeystore.

http://codereview.chromium.org/10916036/diff/4005/sync/protocol/nigori_specif...
File sync/protocol/nigori_specifics.proto (right):

http://codereview.chromium.org/10916036/diff/4005/sync/protocol/nigori_specif...
sync/protocol/nigori_specifics.proto:53: optional bool frozen_keybag = 2;
On 2012/09/10 20:41:52, timsteele wrote:
> Explain why this is called "frozen keybag" in a comment.

Done.

[tim (not reviewing), 2012-09-13 19:00:18]

Okay, I don't think I'm going to find any more bugs by inspection on this one :)


Let's just be careful with enabling this by default on any channel, since once
an account is migrated, if this code hits stable, the keystore path will get
triggered despite the flags not being enabled there.

LGTM!

[commit-bot: I haz the power, 2012-09-13 19:39:02]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zea@chromium.org/10916036/3020

[commit-bot: I haz the power, 2012-09-13 21:52:30]

Change committed as 156646