cryptohome: Add dbus method and signal name for encryption migration.

cryptohome: Add dbus method and signal name for encryption migration.

To Chromium, this adds

  HomedirMethods::MigrateToExt4Crypto(const Identification& id,
                                      const Authorization& auth,
                                      const Callback& callback);

that requests Cryptohomed to do the migration (for the |id|
and |auth| combination containing the same data that are passed
to MountEx), and

  CryptohomeClient::SetExt4CryptoMigrationProgressHandler(
      const base::Calback<void(uint64_t, uint64_t)>&);

for listening to a periodic progress status update.

Depends on the constants definition in
https://chromium-review.googlesource.com/c/454576/

BUG=699445
Review-Url: https://codereview.chromium.org/2748743004
Cr-Commit-Position: refs/heads/master@{#458268}
Committed: https://chromium.googlesource.com/chromium/src/+/4076785af1b5ff3207dc841d1283719bc3e3df4a

[kinaba, 2017-03-14 13:57:36]

kinaba@chromium.org changed reviewers:
+ dkrahn@chromium.org

[kinaba, 2017-03-14 13:57:37]

PTAL: dkrahn

This CL is closely tied to the ChromiumOS side change.
https://chromium-review.googlesource.com/c/454361/
https://chromium-review.googlesource.com/c/454576/
So, it might be more smooth to finish the review at the ChromiumOS change.

That said, I just wanted to send all 3 at once (rather than sequentially), to
reduce back-and-forth like reverting one CL due to the fix in the later CL, etc.

[Daniel Erat, 2017-03-14 15:37:36]

derat@chromium.org changed reviewers:
+ derat@chromium.org

[Daniel Erat, 2017-03-14 15:37:37]

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:939: const char* method_name =
cryptohome::kCryptohomeMigrateToDircrypto;
nit: just inline this in the next statement? it doesn't look like it gets used
for anything else

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:949: proxy_->CallMethod(&method_call,
kTpmDBusTimeoutMs,
is this dependent on the TPM, or does it make sense to use a different timeout
here?

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:1229: if (!reader.PopUint64(&total)) {
just merge these:

  if (!reader.PopUint64(&current) || !reader.PopUint64(&total)) {
    LOG(ERROR) << ...
    return;
  }

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:1233: if
(!dircrypto_migration_progress_handler_.is_null())
move this check to the top of the method? it doesn't look like there's any
reason to read the message if you don't have a callback.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
File chromeos/dbus/cryptohome_client.h (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:124: // called periodicaly during a migration
is performed by cryptohomed.
nit: "called periodically while a migration is being performed by cryptohomed."

is it guaranteed to be called once after the migration is done? is there some
other way for chrome to know when it's done?

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:568: // method call, and with reply protobuf.
does the callback get invoked immediately, or only after the migration has
finished? it'd be good to mention that here.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:569: // MigrateToDicrypto attempts to migrate
the home dir using given
nit: MigrateToDirCrypto

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:570: // authorization, to the new "dicrypto"
encryption.
nit: "dircrypto", also remove comma on this line

[Darren Krahn, 2017-03-14 16:03:33]

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/fake_cryptoho...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/fake_cryptoho...
chromeos/dbus/fake_cryptohome_client.cc:585: ReturnProtobufMethodCallback(reply,
callback);
Make a few progress calls to test the handler?

[kinaba, 2017-03-15 08:17:30]

Thanks for the comments!

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
File chromeos/dbus/cryptohome_client.cc (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:939: const char* method_name =
cryptohome::kCryptohomeMigrateToDircrypto;
On 2017/03/14 15:37:36, Daniel Erat wrote:
> nit: just inline this in the next statement? it doesn't look like it gets used
> for anything else

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:949: proxy_->CallMethod(&method_call,
kTpmDBusTimeoutMs,
On 2017/03/14 15:37:36, Daniel Erat wrote:
> is this dependent on the TPM, or does it make sense to use a different timeout
> here?

Good catch. Done

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:1229: if (!reader.PopUint64(&total)) {
On 2017/03/14 15:37:36, Daniel Erat wrote:
> just merge these:
> 
>   if (!reader.PopUint64(&current) || !reader.PopUint64(&total)) {
>     LOG(ERROR) << ...
>     return;
>   }

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.cc:1233: if
(!dircrypto_migration_progress_handler_.is_null())
On 2017/03/14 15:37:36, Daniel Erat wrote:
> move this check to the top of the method? it doesn't look like there's any
> reason to read the message if you don't have a callback.

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
File chromeos/dbus/cryptohome_client.h (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:124: // called periodicaly during a migration
is performed by cryptohomed.
On 2017/03/14 15:37:36, Daniel Erat wrote:
> nit: "called periodically while a migration is being performed by
cryptohomed."
> 
> is it guaranteed to be called once after the migration is done? is there some
> other way for chrome to know when it's done?

My intent is that the callback from MigrateToDircrypto fires
only after the migration is finished (by success or failure.)
Adding a comment below as you suggested.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:568: // method call, and with reply protobuf.
On 2017/03/14 15:37:36, Daniel Erat wrote:
> does the callback get invoked immediately, or only after the migration has
> finished? it'd be good to mention that here.

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:569: // MigrateToDicrypto attempts to migrate
the home dir using given
On 2017/03/14 15:37:37, Daniel Erat wrote:
> nit: MigrateToDirCrypto

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/cryptohome_cl...
chromeos/dbus/cryptohome_client.h:570: // authorization, to the new "dicrypto"
encryption.
On 2017/03/14 15:37:36, Daniel Erat wrote:
> nit: "dircrypto", also remove comma on this line

Done.

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/fake_cryptoho...
File chromeos/dbus/fake_cryptohome_client.cc (right):

https://codereview.chromium.org/2748743004/diff/1/chromeos/dbus/fake_cryptoho...
chromeos/dbus/fake_cryptohome_client.cc:585: ReturnProtobufMethodCallback(reply,
callback);
On 2017/03/14 16:03:33, Darren Krahn wrote:
> Make a few progress calls to test the handler?

Good point.

Actually, @fukino is now writing an UI code that drives these
Cryptohome methods, and completing the FakeCryptohomeClient
implementation so that it suits well with the tested UI part.

Is it ok for you to keep this as-is in this CL, and enhance
in a later code that needs to be tested comes in?

[kinaba, 2017-03-15 08:21:10]

kinaba@chromium.org changed reviewers:
+ fukino@chromium.org

[Daniel Erat, 2017-03-15 14:50:52]

lgtm

[kinaba, 2017-03-16 01:00:12]

@Darren, @Fukino, do you think it good to go?

[kinaba, 2017-03-16 01:00:13]

@Darren, @Fukino, do you think it good to go?

[kinaba, 2017-03-16 01:00:14]

@Darren, @Fukino, do you think it good to go?

[fukino, 2017-03-16 01:06:07]

LGTM

[kinaba, 2017-03-16 08:44:32]

To reflect the comment in the cryptohome-side CL:
https://chromium-review.googlesource.com/c/454361/#message-d185acc1368f82d7b4...

Updated the CL a bit, to include the status enum
in the notification signal.

[kinaba, 2017-03-16 22:59:45]

kinaba@chromium.org changed reviewers:
+ hashimoto@chromium.org

[kinaba, 2017-03-16 22:59:46]

Friendly ping: dkrahn@

[Darren Krahn, 2017-03-17 20:54:08]

On 2017/03/16 22:59:46, kinaba wrote:
> Friendly ping: dkrahn@

lgtm

[kinaba, 2017-03-17 20:59:28]

On 2017/03/17 20:54:08, Darren Krahn wrote:
> On 2017/03/16 22:59:46, kinaba wrote:
> > Friendly ping: dkrahn@
> 
> lgtm

Thanks! I'll land after the constants definitions in the other repo landed.

[kinaba, 2017-03-20 23:54:00]

The CQ bit was checked by kinaba@chromium.org

[kinaba, 2017-03-20 23:54:00]

The patchset sent to the CQ was uploaded after l-g-t-m from derat@chromium.org,
fukino@chromium.org, dkrahn@chromium.org
Link to the patchset: https://codereview.chromium.org/2748743004/#ps60001
(title: "Roll DEPS")

[commit-bot: I haz the power, 2017-03-20 23:54:42]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-21 01:32:12]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1490054040563420,
"parent_rev": "08f54ecc76964badb626b2426b6f01f46cac7717", "commit_rev":
"4076785af1b5ff3207dc841d1283719bc3e3df4a"}

[commit-bot: I haz the power, 2017-03-21 01:34:17]

Description was changed from

==========
cryptohome: Add dbus method and signal name for encryption migration.

To Chromium, this adds

  HomedirMethods::MigrateToExt4Crypto(const Identification& id,
                                      const Authorization& auth,
                                      const Callback& callback);

that requests Cryptohomed to do the migration (for the |id|
and |auth| combination containing the same data that are passed
to MountEx), and

  CryptohomeClient::SetExt4CryptoMigrationProgressHandler(
      const base::Calback<void(uint64_t, uint64_t)>&);

for listening to a periodic progress status update.

Depends on the constants definition in
https://chromium-review.googlesource.com/c/454576/

BUG=699445
==========

to

==========
cryptohome: Add dbus method and signal name for encryption migration.

To Chromium, this adds

  HomedirMethods::MigrateToExt4Crypto(const Identification& id,
                                      const Authorization& auth,
                                      const Callback& callback);

that requests Cryptohomed to do the migration (for the |id|
and |auth| combination containing the same data that are passed
to MountEx), and

  CryptohomeClient::SetExt4CryptoMigrationProgressHandler(
      const base::Calback<void(uint64_t, uint64_t)>&);

for listening to a periodic progress status update.

Depends on the constants definition in
https://chromium-review.googlesource.com/c/454576/

BUG=699445

Review-Url: https://codereview.chromium.org/2748743004
Cr-Commit-Position: refs/heads/master@{#458268}
Committed:
https://chromium.googlesource.com/chromium/src/+/4076785af1b5ff3207dc841d1283...
==========

[commit-bot: I haz the power, 2017-03-21 01:34:19]

Committed patchset #4 (id:60001) as
https://chromium.googlesource.com/chromium/src/+/4076785af1b5ff3207dc841d1283...