[Sync] Persist keystore key across restarts

[Sync] Persist keystore key across restarts

Adds the preference for the bootstrap token and the bootstrapping functionality
in the cryptographer so that we persist the keystore key across restarts. With
this, we should only ever do one GetKey per client, and only on the first time
that client signs in/restarts on a version with GetKey support (although it's 
not persisted across signouts/profile nukes)

BUG=129665
TEST=unit_tests


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

[Nicolas Zea, 2012-06-13 22:13:53]

PTAL. More thorough testing of this functionality will be added once we actually
use the keystore key in encryption (which will be in a later patch).

[rlarocque, 2012-06-13 23:35:03]

https://chromiumcodereview.appspot.com/10540149/diff/1/chrome/browser/sync/gl...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

https://chromiumcodereview.appspot.com/10540149/diff/1/chrome/browser/sync/gl...
chrome/browser/sync/glue/sync_backend_host.cc:724: const std::string&
keystore_bootstrap_token,
Is it really necessary to send the token back on every call to
FinishConfigureDataTypesOnFrontendLoop()?

If the value is not going to change, could we pass it back in some other way? 
Maybe a 'HandleGetKeyReponseOnFrontendLoop()'?

https://chromiumcodereview.appspot.com/10540149/diff/1/sync/util/cryptographe...
File sync/util/cryptographer.cc (right):

https://chromiumcodereview.appspot.com/10540149/diff/1/sync/util/cryptographe...
sync/util/cryptographer.cc:67: NigoriMap::value_type* old_default =
default_nigori_;
Why use linked_ptr<>*?

[Nicolas Zea, 2012-06-15 00:42:06]

PTAL

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:724: const std::string&
keystore_bootstrap_token,
On 2012/06/13 23:35:04, rlarocque wrote:
> Is it really necessary to send the token back on every call to
> FinishConfigureDataTypesOnFrontendLoop()?
> 
> If the value is not going to change, could we pass it back in some other way? 
> Maybe a 'HandleGetKeyReponseOnFrontendLoop()'?

Yeah, I guess the bootstrap token doesn't really relate to configuring
datatypes. I've gone ahead and changed this to reuse the PersistEncryptionToken
method and key off the last_get_key_result being successful in
OnSyncCycleCompleted.

http://codereview.chromium.org/10540149/diff/1/sync/util/cryptographer.cc
File sync/util/cryptographer.cc (right):

http://codereview.chromium.org/10540149/diff/1/sync/util/cryptographer.cc#new...
sync/util/cryptographer.cc:67: NigoriMap::value_type* old_default =
default_nigori_;
On 2012/06/13 23:35:04, rlarocque wrote:
> Why use linked_ptr<>*?

Because they're also contained within the nigori keybag (an array of
linked_ptr<Nigori>'s), and I'm making a copy of it.

That said, I didn't like this hacky swap, so I've gone ahead and added an extra
param to AddKeyImpl so I could use it properly, removing the need for this
logic.

[rlarocque, 2012-06-15 19:02:00]

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:724: const std::string&
keystore_bootstrap_token,
On 2012/06/15 00:42:07, nzea wrote:
> On 2012/06/13 23:35:04, rlarocque wrote:
> > Is it really necessary to send the token back on every call to
> > FinishConfigureDataTypesOnFrontendLoop()?
> > 
> > If the value is not going to change, could we pass it back in some other
way? 
> > Maybe a 'HandleGetKeyReponseOnFrontendLoop()'?
> 
> Yeah, I guess the bootstrap token doesn't really relate to configuring
> datatypes. I've gone ahead and changed this to reuse the
PersistEncryptionToken
> method and key off the last_get_key_result being successful in
> OnSyncCycleCompleted.

I'm not sure that's much better.  Wouldn't it make more sense to only update it
when it changes, as is the case with the bootstrap token?

[Nicolas Zea, 2012-06-15 20:15:30]

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
File chrome/browser/sync/glue/sync_backend_host.cc (right):

http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
chrome/browser/sync/glue/sync_backend_host.cc:724: const std::string&
keystore_bootstrap_token,
On 2012/06/15 19:02:00, rlarocque wrote:
> On 2012/06/15 00:42:07, nzea wrote:
> > On 2012/06/13 23:35:04, rlarocque wrote:
> > > Is it really necessary to send the token back on every call to
> > > FinishConfigureDataTypesOnFrontendLoop()?
> > > 
> > > If the value is not going to change, could we pass it back in some other
> way? 
> > > Maybe a 'HandleGetKeyReponseOnFrontendLoop()'?
> > 
> > Yeah, I guess the bootstrap token doesn't really relate to configuring
> > datatypes. I've gone ahead and changed this to reuse the
> PersistEncryptionToken
> > method and key off the last_get_key_result being successful in
> > OnSyncCycleCompleted.
> 
> I'm not sure that's much better.  Wouldn't it make more sense to only update
it
> when it changes, as is the case with the bootstrap token?

This is effectively the same logic used in the bootstrap token, i.e. any time we
successfully set a passphrase we update the bootstrap token (even if it didn't
change). We don't have any logic comparing the already persisted token and the
new one; the pref service does that for you (it checks to see if the new pref is
different or not). I don't see why we should duplicate that logic.

Note also that we'll only ever perform a GetKey if we didn't have a valid key in
the first place. In other words, if the GetKey succeeded, it means we're writing
a new key anyways (barring those cases where the getkey succeeds but the
download updates fails, but that goes back to the pref service only persisting
new strings).

[rlarocque, 2012-06-15 21:14:09]

On 2012/06/15 20:15:30, nzea wrote:
>
http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
> File chrome/browser/sync/glue/sync_backend_host.cc (right):
> 
>
http://codereview.chromium.org/10540149/diff/1/chrome/browser/sync/glue/sync_...
> chrome/browser/sync/glue/sync_backend_host.cc:724: const std::string&
> keystore_bootstrap_token,
> On 2012/06/15 19:02:00, rlarocque wrote:
> > On 2012/06/15 00:42:07, nzea wrote:
> > > On 2012/06/13 23:35:04, rlarocque wrote:
> > > > Is it really necessary to send the token back on every call to
> > > > FinishConfigureDataTypesOnFrontendLoop()?
> > > > 
> > > > If the value is not going to change, could we pass it back in some other
> > way? 
> > > > Maybe a 'HandleGetKeyReponseOnFrontendLoop()'?
> > > 
> > > Yeah, I guess the bootstrap token doesn't really relate to configuring
> > > datatypes. I've gone ahead and changed this to reuse the
> > PersistEncryptionToken
> > > method and key off the last_get_key_result being successful in
> > > OnSyncCycleCompleted.
> > 
> > I'm not sure that's much better.  Wouldn't it make more sense to only update
> it
> > when it changes, as is the case with the bootstrap token?
> 
> This is effectively the same logic used in the bootstrap token, i.e. any time
we
> successfully set a passphrase we update the bootstrap token (even if it didn't
> change). We don't have any logic comparing the already persisted token and the
> new one; the pref service does that for you (it checks to see if the new pref
is
> different or not). I don't see why we should duplicate that logic.
> 
> Note also that we'll only ever perform a GetKey if we didn't have a valid key
in
> the first place. In other words, if the GetKey succeeded, it means we're
writing
> a new key anyways (barring those cases where the getkey succeeds but the
> download updates fails, but that goes back to the pref service only persisting
> new strings).

Resolved offline.  I didn't realize that the GetKey commands would have to pass
through the Syncer.  If that's the case, then I suppose that piggy-backing this
result on the snapshot is as good as we can do for now.  

LGTM.

[commit-bot: I haz the power, 2012-07-31 23:20:35]

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

[commit-bot: I haz the power, 2012-07-31 23:20:41]

Presubmit check for 10540149-37001 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 1.7s to calculate.

[Nicolas Zea, 2012-07-31 23:32:06]

+Nico for chrome/common

[Nico, 2012-07-31 23:42:39]

chrome/common lgtm

Will these flags go away eventually? If not, is there a way to have only one of
the two?

[Nicolas Zea, 2012-07-31 23:50:29]

On 2012/07/31 23:42:39, Nico wrote:
> chrome/common lgtm
> 
> Will these flags go away eventually? If not, is there a way to have only one
of
> the two?

Eventually the plan is for the first of the two to go away (the one I'm adding
will replace it entirely). But that is not for a while, as any migration process
in sync has a very long tail.

[commit-bot: I haz the power, 2012-07-31 23:50:48]

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

[commit-bot: I haz the power, 2012-08-01 01:13:40]

Change committed as 149344