Add SecureMessage definitions to CryptAuth.

Add SecureMessage definitions to CryptAuth.

BUG=473471
Committed: https://crrev.com/bbb566c2518db4a962f5ca529bb8204ec53f94c1
Cr-Commit-Position: refs/heads/master@{#324006}

[Tim Song, 2015-04-03 01:27:17]

Patchset #1 (id:1) has been deleted

[Tim Song, 2015-04-03 01:27:36]

tengs@chromium.org changed reviewers:
+ isherman@chromium.org

[Ilya Sherman, 2015-04-03 02:13:05]

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/BUILD.gn:60: "//testing/gtest",
Should :test_support be added here?  If not, what is the :test_support target
for?

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:61: const
securemessage::SigScheme& signature_scheme) {
The signature_scheme is ignored here, but used in the Verify() method.  How does
that work?

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:21: void
GenerateKeyPair(const GenerateKeyPairCallback& callback) override;
nit: Add a "// SecureMessageDelegate:" comment above this line, and remove blank
lines between overridden methods that are part of the same interface.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:50: 
I'd prefer not to add this class until it is actually used in test code -- which
it doesn't seem to currently be.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/proto/securemessage.proto (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/proto/securemessage.proto:46: // The length
of some associated data this is not sent in this SecureMessage,
nit: s/this/that?

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:21: struct
CreateOptions final {
nit: What's up with the use of final?  I haven't seen it used before -- i.e. it
doesn't seem to be a common Chromium style.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:35: std::string
verification_key_id;
nit: What does the "id" part of this name refer to?

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:87: // |header|
fields will be non-emptyif the message was verified successfully.
nit: "emptyif" -> "empty if"

[Tim Song, 2015-04-03 02:46:34]

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/BUILD.gn (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/BUILD.gn:60: "//testing/gtest",
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> Should :test_support be added here?  If not, what is the :test_support target
> for?

Done.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:61: const
securemessage::SigScheme& signature_scheme) {
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> The signature_scheme is ignored here, but used in the Verify() method.  How
does
> that work?

Anything signed by the private key can be verified by the public key, and vice
versa. Verify() simply uses the |signature_scheme| to determine the opposite key
to make the signature with.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:21: void
GenerateKeyPair(const GenerateKeyPairCallback& callback) override;
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> nit: Add a "// SecureMessageDelegate:" comment above this line, and remove
blank
> lines between overridden methods that are part of the same interface.

Done.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:50: 
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> I'd prefer not to add this class until it is actually used in test code --
which
> it doesn't seem to currently be.

Looking at our existing tests, though, it is clear that we'll need this
eventually. Breaking it into this CL reduces the clutter for future ones.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/proto/securemessage.proto (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/proto/securemessage.proto:46: // The length
of some associated data this is not sent in this SecureMessage,
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> nit: s/this/that?

Done. This is copied from the server .proto file.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:21: struct
CreateOptions final {
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> nit: What's up with the use of final?  I haven't seen it used before -- i.e.
it
> doesn't seem to be a common Chromium style.

I didn't want implementations to subclass these structs themselves. I'll remove
the final if it's not commonly used.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:35: std::string
verification_key_id;
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> nit: What does the "id" part of this name refer to?

It identifies the key you should use to verify the signature, and is not the
actual key.

In our system, the |verification_key_id| is the public key of the remote device,
which identifies to the remote device that it should use it's corresponding
private key.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:87: // |header|
fields will be non-emptyif the message was verified successfully.
On 2015/04/03 02:13:05, Ilya Sherman wrote:
> nit: "emptyif" -> "empty if"

Done.

[Tim Song, 2015-04-03 17:33:40]

I added a unit test for the FakeSecureMessageDelegate. Please take another look.

[Ilya Sherman, 2015-04-03 22:14:45]

On 2015/04/03 17:33:40, Tim Song wrote:
> I added a unit test for the FakeSecureMessageDelegate. Please take another
look.

I'm confused.  Isn't the FakeSecureMessageDelegate intended to be used in tests?
 If not, where is it going to be used?  My point was that we shouldn't add it in
the first place if it's not being used by tests, not that the class itself
needed testing.

[Ilya Sherman, 2015-04-03 22:43:42]

I guess it's okay to add the fake implementation in this CL, though it still
feels a little odd to me to add it before it's actually used.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:35: std::string
verification_key_id;
On 2015/04/03 02:46:34, Tim Song wrote:
> On 2015/04/03 02:13:05, Ilya Sherman wrote:
> > nit: What does the "id" part of this name refer to?
> 
> It identifies the key you should use to verify the signature, and is not the
> actual key.
> 
> In our system, the |verification_key_id| is the public key of the remote
device,
> which identifies to the remote device that it should use it's corresponding
> private key.

Please update the comment to clarify, then, that this not a key (or at least,
not the verification key), but is rather an identifier for a key.  Same below.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:62: const
securemessage::SigScheme& signature_scheme) {
nit: The |signature_scheme| is not used, so let's omit it?

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:78: }
I think this method would be clearer if it were structured as:

std::string signing_key;
if (symmetric) {
  signing_key = key;
} else {
  signing_key = opposite_key;
}

std::string expected_signature = Sign(payload, associated_data, signing_key);
return signature == expected_signature;

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:88: <<
signature_scheme;
Hmm?

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:105:
std::string private_key(std::string(kPrivateKeyPrefix + public_key));
nit: No need for the inner "std::string"

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:115: //
Ensure that the same symmetric is derived for DeriveKey(private1, public2)
nit: "same symmetric" -> "same symmetric key"

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:116: // and
DeriveKey(private2, public1).
I think it would help to explain, in the comment, that you're basically
transforming the private key back into its public key.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:119:
private_key.size() > prefix.size()
I'm not really following why you're just comparing the string sizes.  Do you
want to test the strings for prefix equality, as you do above?

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:120: ?
private_key.substr(std::string(kPrivateKeyPrefix).size())
nit: s/std::string(kPrivateKeyPrefix)/prefix

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:160: }
Most of this seems appropriate to do in the base class, rather than in the fake
class.  Maybe the base class should just have protected virtual methods Sign()
and Encrypt()?

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:193: }
Same applies here.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:23: 
nit: Please remove this newline.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc:20:
const char kDecryptionKeyId[] = "it's in your pocket somewhere";
:)

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc:22:
void SaveKeyPair(std::string* private_key_out,
nit: Please write (brief) doc strings for all these little helper methods in the
anonymous namespace.

[Tim Song, 2015-04-04 05:06:15]

The FakeSecureMessageDelegate is pretty complex, so I think it's appropriate to
have a unit test for it. I was inspired by a TotT article that recommends
testing your fakes.

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
File components/proximity_auth/cryptauth/secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/20001/components/proximity_au...
components/proximity_auth/cryptauth/secure_message_delegate.h:35: std::string
verification_key_id;
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> On 2015/04/03 02:46:34, Tim Song wrote:
> > On 2015/04/03 02:13:05, Ilya Sherman wrote:
> > > nit: What does the "id" part of this name refer to?
> > 
> > It identifies the key you should use to verify the signature, and is not the
> > actual key.
> > 
> > In our system, the |verification_key_id| is the public key of the remote
> device,
> > which identifies to the remote device that it should use it's corresponding
> > private key.
> 
> Please update the comment to clarify, then, that this not a key (or at least,
> not the verification key), but is rather an identifier for a key.  Same below.

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:62: const
securemessage::SigScheme& signature_scheme) {
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> nit: The |signature_scheme| is not used, so let's omit it?

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:78: }
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> I think this method would be clearer if it were structured as:
> 
> std::string signing_key;
> if (symmetric) {
>   signing_key = key;
> } else {
>   signing_key = opposite_key;
> }
> 
> std::string expected_signature = Sign(payload, associated_data, signing_key);
> return signature == expected_signature;

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:88: <<
signature_scheme;
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> Hmm?

Sorry, forgot to delete this.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:105:
std::string private_key(std::string(kPrivateKeyPrefix + public_key));
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> nit: No need for the inner "std::string"

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:115: //
Ensure that the same symmetric is derived for DeriveKey(private1, public2)
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> nit: "same symmetric" -> "same symmetric key"

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:116: // and
DeriveKey(private2, public1).
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> I think it would help to explain, in the comment, that you're basically
> transforming the private key back into its public key.

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:119:
private_key.size() > prefix.size()
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> I'm not really following why you're just comparing the string sizes.  Do you
> want to test the strings for prefix equality, as you do above?

I don't want to call substr with an out of bounds index.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:120: ?
private_key.substr(std::string(kPrivateKeyPrefix).size())
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> nit: s/std::string(kPrivateKeyPrefix)/prefix

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:160: }
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> Most of this seems appropriate to do in the base class, rather than in the
fake
> class.  Maybe the base class should just have protected virtual methods Sign()
> and Encrypt()?

We need to keep SecureMessageDelegate as an interface, since the easy-unlock
daemon does all of this serialization and more. It's not ideal, but we still
can't bring securemessage to Chrome itself due to the OpenSSL migration.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:193: }
On 2015/04/03 22:43:41, Ilya Sherman wrote:
> Same applies here.

Same reasoning.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.h (right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.h:23: 
On 2015/04/03 22:43:42, Ilya Sherman wrote:
> nit: Please remove this newline.

Done.

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc:20:
const char kDecryptionKeyId[] = "it's in your pocket somewhere";
On 2015/04/03 22:43:42, Ilya Sherman wrote:
> :)

:)

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate_unittest.cc:22:
void SaveKeyPair(std::string* private_key_out,
On 2015/04/03 22:43:42, Ilya Sherman wrote:
> nit: Please write (brief) doc strings for all these little helper methods in
the
> anonymous namespace.

Done.

[Ilya Sherman, 2015-04-06 18:33:52]

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:119:
private_key.size() > prefix.size()
On 2015/04/04 05:06:14, Tim Song wrote:
> On 2015/04/03 22:43:41, Ilya Sherman wrote:
> > I'm not really following why you're just comparing the string sizes.  Do you
> > want to test the strings for prefix equality, as you do above?
> 
> I don't want to call substr with an out of bounds index.

I mean, sure, but trimming off the prefix only makes sense if the prefix is
actually "private_", not if it's just a long enough key.  So, I was expecting a
StartsWith check here.

[Tim Song, 2015-04-06 19:37:23]

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
File components/proximity_auth/cryptauth/fake_secure_message_delegate.cc
(right):

https://codereview.chromium.org/1060483002/diff/60001/components/proximity_au...
components/proximity_auth/cryptauth/fake_secure_message_delegate.cc:119:
private_key.size() > prefix.size()
On 2015/04/06 18:33:51, Ilya Sherman wrote:
> On 2015/04/04 05:06:14, Tim Song wrote:
> > On 2015/04/03 22:43:41, Ilya Sherman wrote:
> > > I'm not really following why you're just comparing the string sizes.  Do
you
> > > want to test the strings for prefix equality, as you do above?
> > 
> > I don't want to call substr with an out of bounds index.
> 
> I mean, sure, but trimming off the prefix only makes sense if the prefix is
> actually "private_", not if it's just a long enough key.  So, I was expecting
a
> StartsWith check here.

All private keys are expected to start with "private_", so anything else is
undefined behaviour. I changed this check to use StartsWithASCII for clarity.

[Ilya Sherman, 2015-04-06 20:32:55]

LGTM, thanks.

[Tim Song, 2015-04-07 02:02:20]

The CQ bit was checked by tengs@chromium.org

[commit-bot: I haz the power, 2015-04-07 02:02:29]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1060483002/120001

[commit-bot: I haz the power, 2015-04-07 02:53:56]

Committed patchset #6 (id:120001)

[commit-bot: I haz the power, 2015-04-07 02:54:47]

Patchset 6 (id:??) landed as
https://crrev.com/bbb566c2518db4a962f5ca529bb8204ec53f94c1
Cr-Commit-Position: refs/heads/master@{#324006}