Merge cryptohome::RetrievedKeyData with cryptohome::KeyDefinition

chromeos/cryptohome/cryptohome_parameters.h

Index: chromeos/cryptohome/cryptohome_parameters.h
diff --git a/chromeos/cryptohome/cryptohome_parameters.h b/chromeos/cryptohome/cryptohome_parameters.h
index fe43f2c21a8d05969c87c3d60db0348d512d6e55..18ca32a7a62cdad01b9e489df46302f8fddf519f 100644
--- a/chromeos/cryptohome/cryptohome_parameters.h
+++ b/chromeos/cryptohome/cryptohome_parameters.h
@@ -10,7 +10,6 @@
 
 #include "base/basictypes.h"
 #include "base/memory/scoped_ptr.h"
-#include "base/memory/scoped_vector.h"
 #include "chromeos/chromeos_export.h"
 
 namespace cryptohome {
@@ -37,65 +36,85 @@ struct CHROMEOS_EXPORT Identification {
 // It contains authorization data along with extra parameters like perimissions

[Darren Krahn, 2014/09/02 18:21:53]

old code nit: perimissions -> permissions

[bartfab (slow), 2014/09/04 10:14:18]

Done.

 // associated with this key.
 struct CHROMEOS_EXPORT KeyDefinition {
-  KeyDefinition(const std::string& key,
-                const std::string& label,
-                int /*AuthKeyPrivileges*/ privileges);
-  ~KeyDefinition();
-
-  bool operator==(const KeyDefinition& other) const;
-
-  std::string label;
+  enum Type {
+    TYPE_PASSWORD = 0
+  };
 
-  int revision;
-  std::string key;
+  struct AuthorizationData {

[Darren Krahn, 2014/09/02 18:21:53]

IMO, this is under-documented in the protobuf and here's a chance to make it
easier to understand. A description of AuthorizationData and its intended use
would be great.

[bartfab (slow), 2014/09/04 10:14:18]

Unfortunately, I do not understand it either. I see that everyone always sets
this to |TYPE_HMACSHA256|. I have no idea what this is for and I do not know
when the other value would be used. Is there a doc that explains this somewhere?
I would be happy to distill it into a comment.

[Darren Krahn, 2014/09/04 23:13:23]

I shared the closest doc I could find. My understanding is that this allows a
key to be managed server-side -- or by a supervisor or some entity other than
the user. It is configured with AddKey and verified by cryptohome on UpdateKey.

Currently the only mechanism supported by cryptohome (AFAIK) is HMAC-SHA256 for
signing.

[bartfab (slow), 2014/09/15 12:04:44]

I cannot find any doc shared with me on that day. If you are OK with it, I will
land the CL as-is and check with Will on the exact purpose of AuthorizationData.
I do not want to add a description that ends up being incorrect.

+    enum Type {
+      TYPE_HMACSHA256 = 0,
+      TYPE_AES256CBC_HMACSHA256
+    };
 
-  std::string encryption_key;
-  std::string signature_key;
-  // Privileges associated with key. Combination of |AuthKeyPrivileges| values.
-  int privileges;
-};
+    struct Secret {
+      Secret(bool encrypt,

[Darren Krahn, 2014/09/02 18:21:53]

A constructor for the way we typically use this would be nice. E.g.

Secret(const std::string& hmac_key);

Also, because this is a struct, would a default constructor be appropriate?

[bartfab (slow), 2014/09/04 10:14:18]

Our typical use actuall always needs at least |encrypt|, |sign| and
|symmetric_key| and in some cases, the other two arguments as well. We never
construct a Secret with just a |symmetric_key|.
I added default constructors for all structs in this file.

+             bool sign,
+             const std::string& symmetric_key,
+             const std::string& public_key,
+             bool wrapped);
 
-// Authorization attempt data for user.
-struct CHROMEOS_EXPORT Authorization {
-  Authorization(const std::string& key, const std::string& label);
-  explicit Authorization(const KeyDefinition& key);
+      bool operator==(const Secret& other) const;
 
-  bool operator==(const Authorization& other) const;
+      bool encrypt;
+      bool sign;
+      std::string symmetric_key;
+      std::string public_key;
+      bool wrapped;
+    };
 
-  std::string key;
-  std::string label;
-};
+    AuthorizationData();

[Darren Krahn, 2014/09/02 18:21:53]

Same here, a typical usage constructor would be nice (in addition to this one).
E.g.

AuthorizationData(const std::string& hmac_key);

[bartfab (slow), 2014/09/04 10:14:18]

Done.

+    ~AuthorizationData();
 
-// Information about keys returned by GetKeyDataEx().
-struct CHROMEOS_EXPORT RetrievedKeyData {
-  enum Type {
-    TYPE_PASSWORD = 0
-  };
+    bool operator==(const AuthorizationData& other) const;
 
-  enum AuthorizationType {
-    AUTHORIZATION_TYPE_HMACSHA256 = 0,
-    AUTHORIZATION_TYPE_AES256CBC_HMACSHA256
+    Type type;
+    std::vector<Secret> secrets;
   };
 
   struct ProviderData {

[Darren Krahn, 2014/09/02 18:21:53]

Same here -- a bit of documentation would be great.

[bartfab (slow), 2014/09/04 10:14:18]

Done.

     explicit ProviderData(const std::string& name);
+    explicit ProviderData(const ProviderData& other);
+    void operator=(const ProviderData& other);
     ~ProviderData();
 
+    bool operator==(const ProviderData& other) const;
+
     std::string name;
     scoped_ptr<int64> number;
     scoped_ptr<std::string> bytes;
   };
 
-  RetrievedKeyData(Type type, const std::string& label, int64 revision);
-  ~RetrievedKeyData();
+  KeyDefinition(const std::string& secret,
+                const std::string& label,
+                int privileges);

[Darren Krahn, 2014/09/02 18:21:53]

[optional] Default constructor here too? It just seems intuitive to me that a
struct can be declared like:

MyStruct my_struct;

[bartfab (slow), 2014/09/04 10:14:18]

Done.

+  ~KeyDefinition();
+
+  bool operator==(const KeyDefinition& other) const;
+
+  void AddSymmetricKey(bool encrypt,

[Darren Krahn, 2014/09/02 18:21:53]

It's not clear what this method does -- would it be better to call it
AddAuthorizationData or AddAuthorizationKey or AddAuthorizationHMACKey?

It's also not intuitive what the parameters actually mean. AFAIK, only 'sign'
with 'hmac' is supported so we could drop the two bools unless we intend to use
them in the short-medium term.

[bartfab (slow), 2014/09/04 10:14:18]

I removed the method as AuthorizationData() has a handy constructor now. We do
use both |sign| and |encrypt| by the way. The two places that create
authorization data (SupervisedUserCreationControllerNew and
ManagerPasswordService) create two keys each, an ecryption key and a signing
key.

[Darren Krahn, 2014/09/04 23:13:23]

Ok, I guess they are just future-proofing, currently the only encryption
mechanism is AES256CBC_HMAC256 and it's not supported by cryptohome.

+                       bool sign,
+                       const std::string& symmetric_key);
 
   Type type;
   std::string label;
   // Privileges associated with key. Combination of |AuthKeyPrivileges| values.
   int privileges;
-  int64 revision;
-  std::vector<AuthorizationType> authorization_types;
-  ScopedVector<ProviderData> provider_data;
+  int revision;
+  std::string secret;
+
+  std::vector<AuthorizationData> authorization_data;
+  std::vector<ProviderData> provider_data;
+};
+
+// Authorization attempt data for user.
+struct CHROMEOS_EXPORT Authorization {
+  Authorization(const std::string& key, const std::string& label);
+  explicit Authorization(const KeyDefinition& key);
+
+  bool operator==(const Authorization& other) const;
+
+  std::string key;
+  std::string label;
 };
 
 // Parameters for Mount call.