[Sync] Add support for performing a GetKey on startup.

[Sync] Add support for performing a GetKey on startup.

The functionality is behind the --sync-keystore-encryption flag, and the key
is not currently consumed by anything, but this lays the groundwork for testing
the server and client interaction.

We request a key anytime we perform a GetUpdates while the cryptographer
does not have a keystore key. But, it is considered an error to request a key
and not receive one, putting us into a state of backoff.

BUG=129665
TEST=sync_unit_tests, running against python server

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

[Nicolas Zea, 2012-05-25 21:13:03]

PTAL. Nearly all of this is just plumbing.

[Nicolas Zea, 2012-06-12 01:10:43]

This is once again ready for review (and based/diffed off
http://codereview.chromium.org/10483015/ and
http://codereview.chromium.org/10541079/).

PTAL

[Nicolas Zea, 2012-06-22 19:57:26]

+cc kevin for sync.proto changes FYI

[Nicolas Zea, 2012-07-19 20:44:54]

This is ready for review again, PTAL.

[tim (not reviewing), 2012-07-19 21:42:02]

http://codereview.chromium.org/10455012/diff/31076/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/31076/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:102: KEY_LENGTH = 16
nit: ENCRYPTION_KEY_LENGTH or KEYSTORE_ is a bit less ambiguous and more
consistent with the patch.

http://codereview.chromium.org/10455012/diff/31076/sync/engine/get_key_comman...
File sync/engine/get_key_command.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/engine/get_key_comman...
sync/engine/get_key_command.cc:32:
SyncerProtoUtil::SetProtocolVersion(&client_to_server_message);
Can we move this to PostClientToServerMessage?

http://codereview.chromium.org/10455012/diff/31076/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:673: if (config_params.keystore_key_status ==
If it's a separate http request, should we have a separate purpose for fetching
the key? Seems like we're hacking around the fact that we don't.  Either way it
doesn't seem like you need to pass in all the params here, the key_status should
suffice.

Did we consider piggybacking on reconfiguration GUs for this, though?  Maybe via
a flag in the GetUpdatesRequest?

http://codereview.chromium.org/10455012/diff/31076/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

http://codereview.chromium.org/10455012/diff/31076/sync/protocol/sync.proto#n...
sync/protocol/sync.proto:458: message GetEncryptionKeyMessage {
Comments on the messages + fields.

http://codereview.chromium.org/10455012/diff/31076/sync/test/engine/mock_conn...
File sync/test/engine/mock_connection_manager.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/test/engine/mock_conn...
sync/test/engine/mock_connection_manager.cc:476:
CHECK(csm->has_get_encryption_key());
why CHECK over ASSERT_TRUE?

[Nicolas Zea, 2012-07-24 22:51:24]

PTAL

http://codereview.chromium.org/10455012/diff/31076/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/31076/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:102: KEY_LENGTH = 16
On 2012/07/19 21:42:03, timsteele wrote:
> nit: ENCRYPTION_KEY_LENGTH or KEYSTORE_ is a bit less ambiguous and more
> consistent with the patch.

Done.

http://codereview.chromium.org/10455012/diff/31076/sync/engine/get_key_comman...
File sync/engine/get_key_command.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/engine/get_key_comman...
sync/engine/get_key_command.cc:32:
SyncerProtoUtil::SetProtocolVersion(&client_to_server_message);
On 2012/07/19 21:42:03, timsteele wrote:
> Can we move this to PostClientToServerMessage?

Added TODO to do that in a follow-up patch.

http://codereview.chromium.org/10455012/diff/31076/sync/engine/sync_scheduler.cc
File sync/engine/sync_scheduler.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/engine/sync_scheduler...
sync/engine/sync_scheduler.cc:673: if (config_params.keystore_key_status ==
On 2012/07/19 21:42:03, timsteele wrote:
> If it's a separate http request, should we have a separate purpose for
fetching
> the key? Seems like we're hacking around the fact that we don't.  Either way
it
> doesn't seem like you need to pass in all the params here, the key_status
should
> suffice.
> 
> Did we consider piggybacking on reconfiguration GUs for this, though?  Maybe
via
> a flag in the GetUpdatesRequest?

Based on the discussion with Kevin, moved the GetKey logic into the GetUpdates
step, so it's not just a normal configuration.

http://codereview.chromium.org/10455012/diff/31076/sync/protocol/sync.proto
File sync/protocol/sync.proto (right):

http://codereview.chromium.org/10455012/diff/31076/sync/protocol/sync.proto#n...
sync/protocol/sync.proto:458: message GetEncryptionKeyMessage {
On 2012/07/19 21:42:03, timsteele wrote:
> Comments on the messages + fields.

Done.

http://codereview.chromium.org/10455012/diff/31076/sync/test/engine/mock_conn...
File sync/test/engine/mock_connection_manager.cc (right):

http://codereview.chromium.org/10455012/diff/31076/sync/test/engine/mock_conn...
sync/test/engine/mock_connection_manager.cc:476:
CHECK(csm->has_get_encryption_key());
On 2012/07/19 21:42:03, timsteele wrote:
> why CHECK over ASSERT_TRUE?

Done.

[tim (not reviewing), 2012-07-26 21:53:14]

http://codereview.chromium.org/10455012/diff/40038/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/40038/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:675: def GetKey(self):
does anything use this?

http://codereview.chromium.org/10455012/diff/40038/sync/engine/download_updat...
File sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/10455012/diff/40038/sync/engine/download_updat...
sync/engine/download_updates_command.cc:119: if (need_encryption_key) {
Can you put this into a helper method for sake of readability?

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
File sync/internal_api/public/sessions/model_neutral_state.h (right):

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
sync/internal_api/public/sessions/model_neutral_state.h:53: SyncerError
last_get_key_result;
We don't technically have GetKey in this patch anymore, consider moving this
aside.

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:385: bool keystore_encryption_enabled,
ahem.  enum police :/  I suppose this is temporary...

also, update comment to mention flag.

[Nicolas Zea, 2012-07-27 00:43:15]

PTAL

http://codereview.chromium.org/10455012/diff/40038/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/40038/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:675: def GetKey(self):
On 2012/07/26 21:53:15, timsteele wrote:
> does anything use this?

Yes, in the HandleGetUpdates at the bottom of the file.

http://codereview.chromium.org/10455012/diff/40038/sync/engine/download_updat...
File sync/engine/download_updates_command.cc (right):

http://codereview.chromium.org/10455012/diff/40038/sync/engine/download_updat...
sync/engine/download_updates_command.cc:119: if (need_encryption_key) {
On 2012/07/26 21:53:15, timsteele wrote:
> Can you put this into a helper method for sake of readability?  

Done.

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
File sync/internal_api/public/sessions/model_neutral_state.h (right):

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
sync/internal_api/public/sessions/model_neutral_state.h:53: SyncerError
last_get_key_result;
On 2012/07/26 21:53:15, timsteele wrote:
> We don't technically have GetKey in this patch anymore, consider moving this
> aside.

For the sake of making it clear where the failure is coming from (since you
could successfully download updates while failing to get a keystore key), I
think it's worth tracking on its own.

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
File sync/internal_api/public/sync_manager.h (right):

http://codereview.chromium.org/10455012/diff/40038/sync/internal_api/public/s...
sync/internal_api/public/sync_manager.h:385: bool keystore_encryption_enabled,
On 2012/07/26 21:53:15, timsteele wrote:
> ahem.  enum police :/  I suppose this is temporary...
> 
> also, update comment to mention flag.

Yeah, since this is just there while the feature is in development (and it's not
a public api method), I figured an enum wasn't necessary. Let me know if you
disagree.

Updated the comment though.

[tim (not reviewing), 2012-07-27 17:21:10]

http://goo.gl/SO5OL

LGTM!!!

[Nicolas Zea, 2012-07-27 17:37:32]

+Fred for net/ ownership

[akalin, 2012-07-31 00:56:56]

testserver changes lgtm

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:476: string.ascii_uppercase +
string.digits) for x in range(
xrange

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:476: string.ascii_uppercase +
string.digits) for x in range(
can you indent so that "for x in range(...)" isn't broken up among two lines?

[Nicolas Zea, 2012-07-31 17:27:06]

Done, committing

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
File net/tools/testserver/chromiumsync.py (right):

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:476: string.ascii_uppercase +
string.digits) for x in range(
On 2012/07/31 00:56:56, akalin wrote:
> xrange

Done.

http://codereview.chromium.org/10455012/diff/56001/net/tools/testserver/chrom...
net/tools/testserver/chromiumsync.py:476: string.ascii_uppercase +
string.digits) for x in range(
On 2012/07/31 00:56:56, akalin wrote:
> can you indent so that "for x in range(...)" isn't broken up among two lines?

Done.

[commit-bot: I haz the power, 2012-07-31 17:27:15]

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

[commit-bot: I haz the power, 2012-07-31 17:27:33]

Presubmit check for 10455012-59001 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for files in these directories:
    chrome

Presubmit checks took 6.1s to calculate.

Was the presubmit check useful? Please send feedback & hate mail to
maruel@chromium.org!

[Nicolas Zea, 2012-07-31 17:44:03]

+Thakis for chrome/common OWNERS

[Nico, 2012-07-31 17:50:51]

chrome/ changes lgtm

https://chromiumcodereview.appspot.com/10455012/diff/59001/chrome/browser/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10455012/diff/59001/chrome/browser/syn...
chrome/browser/sync/glue/sync_backend_host.cc:979: success =
sync_manager_->Init(
nit: Consider using the buider pattern (

  SyncInitGuy()
    .dataFolderPath(sync_data_folder_path_)
    .eventHandler(options.event_handler_)
    .etc
).  The current code look a tad lolly.

[Nicolas Zea, 2012-07-31 17:59:21]

Committing.

https://chromiumcodereview.appspot.com/10455012/diff/59001/chrome/browser/syn...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10455012/diff/59001/chrome/browser/syn...
chrome/browser/sync/glue/sync_backend_host.cc:979: success =
sync_manager_->Init(
On 2012/07/31 17:50:51, Nico wrote:
> nit: Consider using the buider pattern (
> 
>   SyncInitGuy()
>     .dataFolderPath(sync_data_folder_path_)
>     .eventHandler(options.event_handler_)
>     .etc
> ).  The current code look a tad lolly.

Yeah, it's not the prettiest. I'll file a bug to refactor SyncManager
initialization, since that'll definitely need to be a patch of its own.

[commit-bot: I haz the power, 2012-07-31 17:59:48]

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

[commit-bot: I haz the power, 2012-07-31 19:44:27]

Change committed as 149248