[Sync] Move keystore key handling to SyncEncryptionHandlerImpl

sync/util/cryptographer.h

Index: sync/util/cryptographer.h
diff --git a/sync/util/cryptographer.h b/sync/util/cryptographer.h
index 77a164f75a7e508237c4320b04e336ca1dbcf9a9..ed9ea6ef528c8e42fe73564e872100876a9dfeec 100644
--- a/sync/util/cryptographer.h
+++ b/sync/util/cryptographer.h
@@ -63,10 +63,6 @@ class Cryptographer {
   // never call Bootstrap at all.
   void Bootstrap(const std::string& restored_bootstrap_token);
 
-  // Bootstrap the keystore key.
-  void BootstrapKeystoreKey(
-      const std::string& restored_keystore_bootstrap_token);
-
   // Returns whether we can decrypt |encrypted| using the keys we currently know
   // about.
   bool CanDecrypt(const sync_pb::EncryptedData& encrypted) const;
@@ -138,8 +134,10 @@ class Cryptographer {
   bool is_initialized() const { return !nigoris_.empty() && default_nigori_; }
 
   // Returns whether this Cryptographer is ready to encrypt and decrypt data.
-  bool is_ready() const { return is_initialized() &&
-                          has_pending_keys() == false; }
+  bool is_ready() const {
+    return is_initialized() &&
+           has_pending_keys() == false;
+  }
 
   // Returns whether there is a pending set of keys that needs to be decrypted.
   bool has_pending_keys() const { return NULL != pending_keys_.get(); }
@@ -149,18 +147,6 @@ class Cryptographer {
   // can't be created (i.e. if this Cryptograhper doesn't have valid keys).
   bool GetBootstrapToken(std::string* token) const;
 
-  // Obtain the bootstrap token based on the keystore encryption key.
-  bool GetKeystoreKeyBootstrapToken(std::string* token) const;
-
-  // Set the keystore-derived nigori from the provided key.
-  // Returns true if we succesfully create the keystore derived nigori from the
-  // provided key, false otherwise.
-  bool SetKeystoreKey(const std::string& keystore_key);
-
-  // Returns true if we currently have a keystore-derived nigori, false
-  // otherwise.
-  bool HasKeystoreKey() const;
-
   Encryptor* encryptor() const { return encryptor_; }
 
  private:
@@ -173,9 +159,8 @@ class Cryptographer {
   // Does not update the default nigori.
   void InstallKeyBag(const sync_pb::NigoriKeyBag& bag);
 
-  // Helper method to add a nigori as either the new default nigori or the new
-  // keystore nigori.
-  bool AddKeyImpl(Nigori* nigori, bool is_keystore_key);
+  // Helper method to add a nigori as the default key.
+  bool AddKeyImpl(Nigori* nigori);
 
   // Functions to serialize + encrypt a Nigori object in an opaque format for
   // persistence by sync infrastructure.
@@ -186,7 +171,6 @@ class Cryptographer {
 
   NigoriMap nigoris_;  // The Nigoris we know about, mapped by key name.
   NigoriMap::value_type* default_nigori_;  // The Nigori used for encryption.

[akalin, 2012/08/22 22:09:34]

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]

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?

-  NigoriMap::value_type* keystore_nigori_; // Nigori generated from keystore.
 
   scoped_ptr<sync_pb::EncryptedData> pending_keys_;