[Sync] Move keystore key handling to SyncEncryptionHandlerImpl

[Sync] Move keystore key handling to SyncEncryptionHandlerImpl

The cryptographer has no notion of keystore keys, and we now persist the
keystore key by reusing the OnBoostrapTokenUpdated method (which now takes
an enum as an extra param specifying the type of token).

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

[Nicolas Zea, 2012-08-22 21:20:18]

PTAL

[akalin, 2012-08-22 22:09:33]

a few comments

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/js_sync_encr...
File sync/internal_api/js_sync_encryption_handler_observer.cc (left):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/js_sync_encr...
sync/internal_api/js_sync_encryption_handler_observer.cc:57:
details.SetString("bootstrapToken", "<redacted>");
clearer to keep the redacted bootstrapToken key and change bootstrapTokenUpdated
to "type"

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/public/util/...
File sync/internal_api/public/util/sync_string_conversions.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/public/util/...
sync/internal_api/public/util/sync_string_conversions.cc:47: const char*
BootstrapTokenTypeToString(syncer::BootstrapTokenType type) {
no need syncer::

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl.cc:446: DVLOG(1) << "Keystore
bootstrap token updated.";
usually no periods on log statements

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:74: std::string(),
std::string()));
why not ""?

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:428: std::string
valid_key = "keystore_key";
const char kValidKey[] = ...

http://codereview.chromium.org/10878015/diff/1/sync/protocol/client_debug_inf...
File sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/10878015/diff/1/sync/protocol/client_debug_inf...
sync/protocol/client_debug_info.proto:66: KEYSTORE_TOKEN_UPDATED = 9;  // A new
keystore encryption token was
9 -> 11?

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

http://codereview.chromium.org/10878015/diff/1/sync/util/cryptographer.h#newc...
sync/util/cryptographer.h:138: return is_initialized() &&
one line?

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

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:41: AddKeyImpl(nigori.release());
consider making AddKeyImpl take a scoped_ptr<>?

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.h
File sync/util/cryptographer.h (left):

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.h#o...
sync/util/cryptographer.h:188: NigoriMap::value_type* default_nigori_;  // The
Nigori used for encryption.
it seems unsafe to me to have this as a pointer into the map.  Maybe unpack it
to default_nigori_name_ and default_nigori_?  Or at least make it into an
iterator.

[Nicolas Zea, 2012-08-22 22:48:59]

Done, with a question inline.

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/js_sync_encr...
File sync/internal_api/js_sync_encryption_handler_observer.cc (left):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/js_sync_encr...
sync/internal_api/js_sync_encryption_handler_observer.cc:57:
details.SetString("bootstrapToken", "<redacted>");
On 2012/08/22 22:09:33, akalin wrote:
> clearer to keep the redacted bootstrapToken key and change
bootstrapTokenUpdated
> to "type"

Done.

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/public/util/...
File sync/internal_api/public/util/sync_string_conversions.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/public/util/...
sync/internal_api/public/util/sync_string_conversions.cc:47: const char*
BootstrapTokenTypeToString(syncer::BootstrapTokenType type) {
On 2012/08/22 22:09:33, akalin wrote:
> no need syncer::

Done.

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
File sync/internal_api/sync_encryption_handler_impl.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl.cc:446: DVLOG(1) << "Keystore
bootstrap token updated.";
On 2012/08/22 22:09:33, akalin wrote:
> usually no periods on log statements

Done.

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
File sync/internal_api/sync_encryption_handler_impl_unittest.cc (right):

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:74: std::string(),
std::string()));
On 2012/08/22 22:09:33, akalin wrote:
> why not ""?

Done.

http://codereview.chromium.org/10878015/diff/1/sync/internal_api/sync_encrypt...
sync/internal_api/sync_encryption_handler_impl_unittest.cc:428: std::string
valid_key = "keystore_key";
On 2012/08/22 22:09:33, akalin wrote:
> const char kValidKey[] = ...

Done.

http://codereview.chromium.org/10878015/diff/1/sync/protocol/client_debug_inf...
File sync/protocol/client_debug_info.proto (right):

http://codereview.chromium.org/10878015/diff/1/sync/protocol/client_debug_inf...
sync/protocol/client_debug_info.proto:66: KEYSTORE_TOKEN_UPDATED = 9;  // A new
keystore encryption token was
On 2012/08/22 22:09:33, akalin wrote:
> 9 -> 11?

Woops. Done. Strange the compiler didn't catch that.

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

http://codereview.chromium.org/10878015/diff/1/sync/util/cryptographer.h#newc...
sync/util/cryptographer.h:138: return is_initialized() &&
On 2012/08/22 22:09:33, akalin wrote:
> one line?

Done.

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

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:41: AddKeyImpl(nigori.release());
On 2012/08/22 22:09:34, akalin wrote:
> consider making AddKeyImpl take a scoped_ptr<>?

Done.

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.h
File sync/util/cryptographer.h (left):

http://codereview.chromium.org/10878015/diff/4002/sync/util/cryptographer.h#o...
sync/util/cryptographer.h:188: NigoriMap::value_type* default_nigori_;  // The
Nigori used for encryption.
On 2012/08/22 22:09:34, akalin wrote:
> it seems unsafe to me to have this as a pointer into the map.  Maybe unpack it
> to default_nigori_name_ and default_nigori_?  Or at least make it into an
> iterator.

Hmm. I don't think an iterator would have any benefit, since it can still be
erased without you knowing, and there's no way to safely verify if the iterator
is still valid. I'd also rather not introduce unnecessary lookups, since it's
basically a bug for the default to get deleted without clearing this, and it's
better to crash than to encrypt with the wrong key.

So I guess the solution is to ensure a bug forces a crash, without risking
deref'ing an invalid pointer. How about having Nigori provide a weak ptr, and
keeping a weakptr here? WDYT?

[Nicolas Zea, 2012-08-24 23:29:22]

Comment addressed, PTAL.

[akalin, 2012-08-27 22:06:55]

few more cryptographer comments

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

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:60: DCHECK(nigoris_.find(default_nigori_name_) !=
nigoris_.end());
do we want to return false if either of these checks fail?

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:174: default_nigori_ =
nigoris_.find(key_name)->second;
it seems like this'll crash (in release) if the DCHECK above fails.  Perhaps
something like:

NigoriMap::const_iterator it = nigoris_.find(key_name);
// or CHECK(it != nigoris_.end());
if (it == nigoris_.end()) {
  NOTREACHED();
  return;
}
default_nigori_name_ = key_name;
default_nigori_ = it->second;

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:208: const std::string& new_default_key_name =
pending_keys_->key_name();
can you use SetDefaultKey here?

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:140: return is_initialized() && has_pending_keys() ==
false;
has_pending_keys() == false -> !has_pending_keys()

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:176: NigoriMap::value_type::second_type
default_nigori_;
i think it's clearer to just specify linked_ptr<const Nigori> as the type here.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:177: // The key name associated with
|default_nigori_|.
Maybe I'm just OCD, but I prefer default_nigori_name_ to come first, since it's
the key.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:178: std::string default_nigori_name_;
Add a comment saying that the pair (default_nigori_name_, default_nigori_) must
exist in nigoris_.  But that the worst case if that invariant is broken is that
we encrypt with an old nigori.

Also, we cache |default_nigori_| for speed, right?  May want to mention that in
the comment.

[Nicolas Zea, 2012-08-28 01:18:50]

PTAL

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

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:60: DCHECK(nigoris_.find(default_nigori_name_) !=
nigoris_.end());
On 2012/08/27 22:06:55, akalin wrote:
> do we want to return false if either of these checks fail?

Given that getting this wrong runs the risk of corrupting data (not setting the
proper key name on the blob), I suppose it makes sense to just do the lookup
always. I worry about letting sync continue on though, as opposed to forcing a
crash so we know something is very wrong. Leaving it as a LOG(ERROR) + return
false for now.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:174: default_nigori_ =
nigoris_.find(key_name)->second;
On 2012/08/27 22:06:55, akalin wrote:
> it seems like this'll crash (in release) if the DCHECK above fails.  Perhaps
> something like:
> 
> NigoriMap::const_iterator it = nigoris_.find(key_name);
> // or CHECK(it != nigoris_.end());
> if (it == nigoris_.end()) {
>   NOTREACHED();
>   return;
> }
> default_nigori_name_ = key_name;
> default_nigori_ = it->second;

I think a crash would be appropriate here. If this is called improperly,
something is broken and we shouldn't be attempting to go further.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.cc#ne...
sync/util/cryptographer.cc:208: const std::string& new_default_key_name =
pending_keys_->key_name();
On 2012/08/27 22:06:55, akalin wrote:
> can you use SetDefaultKey here?

Done.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:140: return is_initialized() && has_pending_keys() ==
false;
On 2012/08/27 22:06:55, akalin wrote:
> has_pending_keys() == false -> !has_pending_keys()

Done.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:176: NigoriMap::value_type::second_type
default_nigori_;
On 2012/08/27 22:06:55, akalin wrote:
> i think it's clearer to just specify linked_ptr<const Nigori> as the type
here. 

Done.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:177: // The key name associated with
|default_nigori_|.
On 2012/08/27 22:06:55, akalin wrote:
> Maybe I'm just OCD, but I prefer default_nigori_name_ to come first, since
it's
> the key.

Done.

http://codereview.chromium.org/10878015/diff/62/sync/util/cryptographer.h#new...
sync/util/cryptographer.h:178: std::string default_nigori_name_;
On 2012/08/27 22:06:55, akalin wrote:
> Add a comment saying that the pair (default_nigori_name_, default_nigori_)
must
> exist in nigoris_.  But that the worst case if that invariant is broken is
that
> we encrypt with an old nigori.
> 
> Also, we cache |default_nigori_| for speed, right?  May want to mention that
in
> the comment.

Actually, I realize the worst case is we encrypt with a nigori and set an
improper name (which will never be decryptable). So I'm making the condition
check in Encrypt non-debug-only.

[akalin, 2012-08-29 00:02:49]

sorry, one more thing re. default_nigori_

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

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:60: if (nigoris_.find(default_nigori_name_) ==
nigoris_.end() ||
please decomp the iterator returned from find in an intermediate variable.  you
can add an extra scope if you want also.

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:61: default_nigori_.get() !=
can you compare linked_ptrs directly? (not sure)

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:63: LOG(ERROR) << "Corrupt default key.";
wait a minute -- since this isn't a DCHECK anymore, doesn't this negate the
speed benefits of caching default_nigori_?  You're already doing the lookup
every time, so you may as well not cache the pointer.

You may lose some speed in CanDecryptUsingDefaultKey, but I don't know if that's
really noticeable.

[Nicolas Zea, 2012-08-29 19:32:20]

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

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:60: if (nigoris_.find(default_nigori_name_) ==
nigoris_.end() ||
On 2012/08/29 00:02:50, akalin wrote:
> please decomp the iterator returned from find in an intermediate variable. 
you
> can add an extra scope if you want also.

Done.

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:61: default_nigori_.get() !=
On 2012/08/29 00:02:50, akalin wrote:
> can you compare linked_ptrs directly? (not sure)

I'm comparing the pointer to the original object, so either way this works. But,
this code has now been removed anyways.

http://codereview.chromium.org/10878015/diff/7034/sync/util/cryptographer.cc#...
sync/util/cryptographer.cc:63: LOG(ERROR) << "Corrupt default key.";
On 2012/08/29 00:02:50, akalin wrote:
> wait a minute -- since this isn't a DCHECK anymore, doesn't this negate the
> speed benefits of caching default_nigori_?  You're already doing the lookup
> every time, so you may as well not cache the pointer.
> 
> You may lose some speed in CanDecryptUsingDefaultKey, but I don't know if
that's
> really noticeable.

Yeah, I suppose at this point I may as well go all the way. Done.

[akalin, 2012-08-29 19:40:15]

LGTM

http://codereview.chromium.org/10878015/diff/26001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10878015/diff/26001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:175: // The key name associated with the default
nigori. Must correspond to a
Perhaps "If non-empty, must correspond..."

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h
File sync/util/nigori.h (right):

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h#newcode11
sync/util/nigori.h:11: #include "base/memory/weak_ptr.h"
not needed, right?

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h#newcode28
sync/util/nigori.h:28: class Nigori : public base::SupportsWeakPtr<Nigori> {
here, too?

[Nicolas Zea, 2012-08-29 19:46:49]

Committing.

http://codereview.chromium.org/10878015/diff/26001/sync/util/cryptographer.h
File sync/util/cryptographer.h (right):

http://codereview.chromium.org/10878015/diff/26001/sync/util/cryptographer.h#...
sync/util/cryptographer.h:175: // The key name associated with the default
nigori. Must correspond to a
On 2012/08/29 19:40:15, akalin wrote:
> Perhaps "If non-empty, must correspond..."

Done.

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h
File sync/util/nigori.h (right):

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h#newcode11
sync/util/nigori.h:11: #include "base/memory/weak_ptr.h"
On 2012/08/29 19:40:15, akalin wrote:
> not needed, right?

Done.

http://codereview.chromium.org/10878015/diff/26001/sync/util/nigori.h#newcode28
sync/util/nigori.h:28: class Nigori : public base::SupportsWeakPtr<Nigori> {
On 2012/08/29 19:40:15, akalin wrote:
> here, too?

Done.

[commit-bot: I haz the power, 2012-08-29 19:48:28]

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

[commit-bot: I haz the power, 2012-08-29 22:55:08]

Try job failure for 10878015-17004 (retry) on win_rel for step "browser_tests".
It's a second try, previously, steps "browser_tests, unit_tests" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[commit-bot: I haz the power, 2012-08-29 23:03:01]

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