Move HostKeyPair into protocol::KeyPair.

Move HostKeyPair into protocol::KeyPair.
This makes the RSAPrivateKey dependency encapsulated inside KeyPair, and makes it a refcounted object, rather than copying the private key on every authenticator constructor. It also allows authenticators to use the KeyPair methods (such as GetSignature() or GetPublicKey()).

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

[rmsousa, 2013-02-23 00:48:51]

Hi Wez,

This refactor is a prerequisite for the token authentication CL (the host
authenticator needs both to generate signatures and to get the public key).
I factored this change out to get it reviewed separately.

https://codereview.chromium.org/12316083/diff/29/remoting/protocol/negotiatin...
File remoting/protocol/negotiating_authenticator.h (left):

https://codereview.chromium.org/12316083/diff/29/remoting/protocol/negotiatin...
remoting/protocol/negotiating_authenticator.h:58: bool certificate_sent_;
Unused member variable
(likely stale, certificate is currently only used inside v2_authenticator)

https://codereview.chromium.org/12316083/diff/29/remoting/protocol/negotiatin...
remoting/protocol/negotiating_authenticator.h:62: std::string remote_cert_;
Unused member variable

[Wez, 2013-02-23 03:43:19]

sergeyu: Can you take a look as well, please?

rmsousa: Mostly this looks pretty good. :)

https://codereview.chromium.org/12316083/diff/30/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:22: #include "remoting/protocol/key_pair.h"
Doesn't look like we need this include here?

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:95: protocol::KeyPair* key_pair,
Now that KeyPair has Copy(), can we use a scoped_ptr<> here?

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:1449: void
HostNPScriptObject::DoGenerateKeyPair(const ScopedRefNPObject& callback) {
Not strictly related to this CL, but... DoGenerateKeyPair really shouldn't be
passing references to ScopedRefNPObject to other threads, since NPObject
ref-counting isn't thread-safe.  We should save the completion callback object
to the HostNPScriptObject and then:

  callback = base::Bind(InvokeGenerateKeyPairCallback, WeakPtr(this));
  worker->PostTask(Bind(DoGenerateKeyPair, callback));

DoGenerateKeyPair then becomes a static.

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.h:29: #include
"remoting/protocol/key_pair.h"
Do we actually need this include?

https://codereview.chromium.org/12316083/diff/30/remoting/host/register_suppo...
File remoting/host/register_support_host_request.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:52: protocol::KeyPair* key_pair,
Now that we can copy, use scoped_ptr<> here?

https://codereview.chromium.org/12316083/diff/30/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:694: if
(!key_pair_.LoadFromString(key_base64)) {
nit: Merge this with the line above w/ logging e.g. "Private key couldn't be
read from config file".

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.cc
File remoting/protocol/key_pair.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.c...
remoting/protocol/key_pair.cc:49: pair->LoadFromString(this->GetAsString());
Doesn't RSAPrivateKey have a Copy() method now?

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.c...
remoting/protocol/key_pair.cc:58: key_->ExportPrivateKey(&key_buf);
IIRC, the reason we didn't provide Copy() before was that ExportPrivateKey() may
fail if the implementation of _key_| is TPM-backed - we should at least CHECK
this call (see CopyPrivateKey()).

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h
File remoting/protocol/key_pair.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:6: #define REMOTING_PROTOCOL_KEY_PAIR_H_
remoting/base/ might be a better place for this, since it's not really
protocol-specific.  That said, at least if it's in remoting/protocol/, it's
clearer that it's security-sensitive!

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:25: void Generate();
Would you mind adding some comments to explain what these APIs do, please?

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:32: crypto::RSAPrivateKey* CopyPrivateKey() const;
Do we ever use this?

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:38: std::string GetAsString() const;
What does this get as a string? The private key?

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:38: std::string GetAsString() const;
I'd move this back up to come after LoadFromString, add a comment explaining
that it returns the thing base64 encoded, and rename them FromString() and
ToString().

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:40: std::string GetSignature(const std::string&
message) const;
nit: SignMessage

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:44: };
DISALLOW_COPY...

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
File remoting/protocol/key_pair_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:30: TEST_F(KeyPairTest, SaveLoad) {
nit: LoadSaveLoad ;)

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:42: TEST_F(KeyPairTest, Signatures) {
nit: We should have tests that differing keys generate differing signatures, and
similarly for differing texts...

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:42: TEST_F(KeyPairTest, Signatures) {
Please also add a test for Copy().

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/negotiatin...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/negotiatin...
remoting/protocol/negotiating_authenticator.h:29: // requested, and must outlive
this authenticator.
I think this comment is scrying the future. ;)

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.cc:76: socket.release(), cert,
key_pair_->private_key(), ssl_config);
What is the SSL Server Socket lifetime requirement on the RSA key its passed? Do
we need to worry about the Socket out-living the Authenticator (and hence the
key), for example?

[Wez, 2013-02-23 03:45:06]

BTW, please add a BUG to the CL description.  If you make the scoped_ptr<>
changes then the CL description becomes "... and cleans up ownership." :)

[Sergey Ulanov, 2013-02-26 00:14:13]

I don't like that we copy KeyPair objects everywhere. Problem is that
crypto::RSAPrivateKey stores system-specific key handle internally. That means
that, e.g. on Windows, CryptDuplicateKey() API is called for every
KeyPair::Copy() and we don't know how complex it can be internally. It wouldn't
be a problem if we could refactor KeyPair class to store only binary
representation of the key and then create RSAPrivateKey only when it's needed.
That way KeyPair::Copy() will be much cheaper, still not free.

Another alternative is to make it ref-counted :-\ . Maybe it's the right thing
to do here.

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:17: #include
"remoting/protocol/test_key_pair.h"
Looks like you forgot to add this file in the CL.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h
File remoting/protocol/key_pair.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:6: #define REMOTING_PROTOCOL_KEY_PAIR_H_
On 2013/02/23 03:43:20, Wez wrote:
> remoting/base/ might be a better place for this, since it's not really
> protocol-specific.  That said, at least if it's in remoting/protocol/, it's
> clearer that it's security-sensitive!

+1 on putting it in remoting/base

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:20: class KeyPair {
Maybe call it RsaKeyPair? Or do you think we may want this class to support
other algorithms?

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/v2_authent...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/v2_authent...
remoting/protocol/v2_authenticator.h:56: scoped_ptr<KeyPair> key_pair_;
Maybe call it local_key_pair_?

[Sergey Ulanov, 2013-02-26 00:34:42]

Also if we do copy the keys, it might be better to pass them as reference or a
pointer and let the callee copy it only if necessary.

[Wez, 2013-02-26 01:42:35]

On 2013/02/26 00:34:42, sergeyu wrote:
> Also if we do copy the keys, it might be better to pass them as reference or a
> pointer and let the callee copy it only if necessary.

Agreed; this sounds like the cleanest approach for this CL.

[rmsousa, 2013-02-26 02:38:52]

On 2013/02/26 00:14:13, sergeyu wrote:
> I don't like that we copy KeyPair objects everywhere. Problem is that
> crypto::RSAPrivateKey stores system-specific key handle internally. That means
> that, e.g. on Windows, CryptDuplicateKey() API is called for every
> KeyPair::Copy() and we don't know how complex it can be internally. It
wouldn't
> be a problem if we could refactor KeyPair class to store only binary
> representation of the key and then create RSAPrivateKey only when it's needed.
> That way KeyPair::Copy() will be much cheaper, still not free.
> 
> Another alternative is to make it ref-counted :-\ . Maybe it's the right thing
> to do here.

I didn't like the copying either, but the previous code was copying on every
constructor, so I had kept it that way (because I didn't know if there was some
underlying reason for the copy)

Anyway, since it bothered you as well, I changed it to be refcounted. It seems
to work on Linux, but I haven't set up my Windows environment yet, to see if
something weird happens.

https://codereview.chromium.org/12316083/diff/30/remoting/host/chromoting_host.h
File remoting/host/chromoting_host.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/chromoting_hos...
remoting/host/chromoting_host.h:22: #include "remoting/protocol/key_pair.h"
On 2013/02/23 03:43:20, Wez wrote:
> Doesn't look like we need this include here?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender.h:95: protocol::KeyPair* key_pair,
On 2013/02/23 03:43:20, Wez wrote:
> Now that KeyPair has Copy(), can we use a scoped_ptr<> here?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
File remoting/host/heartbeat_sender_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/heartbeat_send...
remoting/host/heartbeat_sender_unittest.cc:17: #include
"remoting/protocol/test_key_pair.h"
On 2013/02/26 00:14:13, sergeyu wrote:
> Looks like you forgot to add this file in the CL.

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.cc:1449: void
HostNPScriptObject::DoGenerateKeyPair(const ScopedRefNPObject& callback) {
On 2013/02/23 03:43:20, Wez wrote:
> Not strictly related to this CL, but... DoGenerateKeyPair really shouldn't be
> passing references to ScopedRefNPObject to other threads, since NPObject
> ref-counting isn't thread-safe.  We should save the completion callback object
> to the HostNPScriptObject and then:
> 
>   callback = base::Bind(InvokeGenerateKeyPairCallback, WeakPtr(this));
>   worker->PostTask(Bind(DoGenerateKeyPair, callback));
> 
> DoGenerateKeyPair then becomes a static.

There's already a TODO comment for it. I'll do that in a separate CL

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
File remoting/host/plugin/host_script_object.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/plugin/host_sc...
remoting/host/plugin/host_script_object.h:29: #include
"remoting/protocol/key_pair.h"
On 2013/02/23 03:43:20, Wez wrote:
> Do we actually need this include?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/host/register_suppo...
File remoting/host/register_support_host_request.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/register_suppo...
remoting/host/register_support_host_request.h:52: protocol::KeyPair* key_pair,
On 2013/02/23 03:43:20, Wez wrote:
> Now that we can copy, use scoped_ptr<> here?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/host/remoting_me2me...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:694: if
(!key_pair_.LoadFromString(key_base64)) {
On 2013/02/23 03:43:20, Wez wrote:
> nit: Merge this with the line above w/ logging e.g. "Private key couldn't be
> read from config file".

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.cc
File remoting/protocol/key_pair.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.c...
remoting/protocol/key_pair.cc:49: pair->LoadFromString(this->GetAsString());
On 2013/02/23 03:43:20, Wez wrote:
> Doesn't RSAPrivateKey have a Copy() method now?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.c...
remoting/protocol/key_pair.cc:58: key_->ExportPrivateKey(&key_buf);
On 2013/02/23 03:43:20, Wez wrote:
> IIRC, the reason we didn't provide Copy() before was that ExportPrivateKey()
may
> fail if the implementation of _key_| is TPM-backed - we should at least CHECK
> this call (see CopyPrivateKey()).

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h
File remoting/protocol/key_pair.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:6: #define REMOTING_PROTOCOL_KEY_PAIR_H_
On 2013/02/26 00:14:13, sergeyu wrote:
> On 2013/02/23 03:43:20, Wez wrote:
> > remoting/base/ might be a better place for this, since it's not really
> > protocol-specific.  That said, at least if it's in remoting/protocol/, it's
> > clearer that it's security-sensitive!
> 
> +1 on putting it in remoting/base

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:6: #define REMOTING_PROTOCOL_KEY_PAIR_H_
On 2013/02/23 03:43:20, Wez wrote:
> remoting/base/ might be a better place for this, since it's not really
> protocol-specific.  That said, at least if it's in remoting/protocol/, it's
> clearer that it's security-sensitive!

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:20: class KeyPair {
On 2013/02/26 00:14:13, sergeyu wrote:
> Maybe call it RsaKeyPair? Or do you think we may want this class to support
> other algorithms?

Its API isn't tied to RSA specifically, but on of its uses is for
SSLServerSocket, which only takes RSA keys, so I guess it's unlikely we'll ever
use anything else. I can rename it once I move it to base.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:25: void Generate();
On 2013/02/23 03:43:20, Wez wrote:
> Would you mind adding some comments to explain what these APIs do, please?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:32: crypto::RSAPrivateKey* CopyPrivateKey() const;
On 2013/02/23 03:43:20, Wez wrote:
> Do we ever use this?

Nope.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:38: std::string GetAsString() const;
On 2013/02/23 03:43:20, Wez wrote:
> I'd move this back up to come after LoadFromString, add a comment explaining
> that it returns the thing base64 encoded, and rename them FromString() and
> ToString().

FromString() sounds a bit like a constructor, rather than an initializer, so I'd
rather keep the current names for now.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:38: std::string GetAsString() const;
On 2013/02/23 03:43:20, Wez wrote:
> What does this get as a string? The private key?

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair.h...
remoting/protocol/key_pair.h:44: };
On 2013/02/23 03:43:20, Wez wrote:
> DISALLOW_COPY...

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
File remoting/protocol/key_pair_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:30: TEST_F(KeyPairTest, SaveLoad) {
On 2013/02/23 03:43:20, Wez wrote:
> nit: LoadSaveLoad ;)

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:42: TEST_F(KeyPairTest, Signatures) {
On 2013/02/23 03:43:20, Wez wrote:
> nit: We should have tests that differing keys generate differing signatures,
and
> similarly for differing texts...

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/key_pair_u...
remoting/protocol/key_pair_unittest.cc:42: TEST_F(KeyPairTest, Signatures) {
On 2013/02/23 03:43:20, Wez wrote:
> Please also add a test for Copy().

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/negotiatin...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/negotiatin...
remoting/protocol/negotiating_authenticator.h:29: // requested, and must outlive
this authenticator.
On 2013/02/23 03:43:20, Wez wrote:
> I think this comment is scrying the future. ;)

Done.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/ssl_hmac_c...
File remoting/protocol/ssl_hmac_channel_authenticator.cc (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/ssl_hmac_c...
remoting/protocol/ssl_hmac_channel_authenticator.cc:76: socket.release(), cert,
key_pair_->private_key(), ssl_config);
On 2013/02/23 03:43:20, Wez wrote:
> What is the SSL Server Socket lifetime requirement on the RSA key its passed?
Do
> we need to worry about the Socket out-living the Authenticator (and hence the
> key), for example?

SSLServerSocket copies the key.

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/v2_authent...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/30/remoting/protocol/v2_authent...
remoting/protocol/v2_authenticator.h:56: scoped_ptr<KeyPair> key_pair_;
On 2013/02/26 00:14:13, sergeyu wrote:
> Maybe call it local_key_pair_?

Done.

[rmsousa, 2013-03-05 23:47:17]

ping?

[Wez, 2013-03-06 00:43:25]

Looking pretty good.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h
File remoting/base/rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:23: // Generates a new (random) private key from
scratch.
nit: No need for both "from scratch" and "(random)"

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:27: bool LoadFromString(const std::string&
key_base64);
Looking at the call-sites, can this be static scoped_refptr<> FromString(), and
similarly for Generate(), above?

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:28: // Returns a base64 encoded string representing
the private key.
Blank line before this comment.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:29: std::string GetAsString() const;
nit: ToString()

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:35: // Returns a base64-encoded string representing
the public key.
nit: Blank line before this comment.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:37: // Returns a base64-encoded signature for the
message.
nit: Blank line before this comment.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:40: crypto::RSAPrivateKey* private_key() { return
key_.get(); }
nit: const?

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
File remoting/base/rsa_key_pair_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:26: class RsaKeyPairTest : public
testing::Test {
Why do we need a base class for these tests, since it contains nothing?

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:29: TEST_F(RsaKeyPairTest, LoadSaveLoad)
{
nit: LoadSaveLoad -> ImportExportImport, since load/save implies storing values
somewhere.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:31: // generate the same signature with
both keys.
nit: This comment seems to be out of date; we don't "load a key to a string",
for example?

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:42: // Sign a message and check that we
get expected signature.
nit: ... get the expected ...

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:55: key_pair2->Generate();
nit: Generating key pairs has an infinitesimally small risk of a clash, and in
the real-world it's not cheap - hard-code some keys and test Generate() once in
a separate test?

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:61: ASSERT_NE(signature_kp1_msg1_base64,
signature_kp1_msg2_base64);
nit: May as well also compare 2_1 to 1_2 while you're here. :)

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:62: ASSERT_NE(key_pair1->GetPublicKey(),
key_pair2->GetPublicKey());
This check is a requirement for the test to be valid, so it really belongs with
the test init block, above, immediately after the keys are generated.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/test_rsa_key...
File remoting/base/test_rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/test_rsa_key...
remoting/base/test_rsa_key_pair.h:8: const char kTestHostKeyPair[] =
nit: kTestRsaKeyPair

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/authenti...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:56: base::Base64Encode(key_string,
&key_base64);
Can we switch to a Base64 encoded string in the file, and avoid the need for
converting here?

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/ssl_hmac...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/ssl_hmac...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:67:
base::Base64Encode(key_string, &key_base64);
See previous comment re updating the test file.

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/v2_authe...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/v2_authe...
remoting/protocol/v2_authenticator.h:15: #include "remoting/base/rsa_key_pair.h"
nit: Can you forward-declare RsaKeyPair?

[rmsousa, 2013-03-06 04:36:49]

ptal

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair.h
File remoting/base/rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:23: // Generates a new (random) private key from
scratch.
On 2013/03/06 00:43:25, Wez wrote:
> nit: No need for both "from scratch" and "(random)"

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:27: bool LoadFromString(const std::string&
key_base64);
On 2013/03/06 00:43:25, Wez wrote:
> Looking at the call-sites, can this be static scoped_refptr<> FromString(),
and
> similarly for Generate(), above?

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:28: // Returns a base64 encoded string representing
the private key.
On 2013/03/06 00:43:25, Wez wrote:
> Blank line before this comment.

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:29: std::string GetAsString() const;
On 2013/03/06 00:43:25, Wez wrote:
> nit: ToString()

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:35: // Returns a base64-encoded string representing
the public key.
On 2013/03/06 00:43:25, Wez wrote:
> nit: Blank line before this comment.

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:37: // Returns a base64-encoded signature for the
message.
On 2013/03/06 00:43:25, Wez wrote:
> nit: Blank line before this comment.

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair.h:40: crypto::RSAPrivateKey* private_key() { return
key_.get(); }
On 2013/03/06 00:43:25, Wez wrote:
> nit: const?

The SSL APIs don't take const parameters. I'm not sure if they actually touch
the key, or if it's just because it's pure-C underneath, but in any case, I
prefer to leave it as non-const than to cast it away on the call site (casting
const away is fairly evil, in general)

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
File remoting/base/rsa_key_pair_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:26: class RsaKeyPairTest : public
testing::Test {
On 2013/03/06 00:43:25, Wez wrote:
> Why do we need a base class for these tests, since it contains nothing?

Organization - All unit tests for class X should be in a class named <X>Test.
This way you can run the tests with a filter like <x>Test.*.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:29: TEST_F(RsaKeyPairTest, LoadSaveLoad)
{
On 2013/03/06 00:43:25, Wez wrote:
> nit: LoadSaveLoad -> ImportExportImport, since load/save implies storing
values
> somewhere.

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:31: // generate the same signature with
both keys.
On 2013/03/06 00:43:25, Wez wrote:
> nit: This comment seems to be out of date; we don't "load a key to a string",
> for example?

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:42: // Sign a message and check that we
get expected signature.
On 2013/03/06 00:43:25, Wez wrote:
> nit: ... get the expected ...

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:55: key_pair2->Generate();
On 2013/03/06 00:43:25, Wez wrote:
> nit: Generating key pairs has an infinitesimally small risk of a clash, and in
> the real-world it's not cheap - hard-code some keys and test Generate() once
in
> a separate test?

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:61: ASSERT_NE(signature_kp1_msg1_base64,
signature_kp1_msg2_base64);
On 2013/03/06 00:43:25, Wez wrote:
> nit: May as well also compare 2_1 to 1_2 while you're here. :)

There's no guarantee we need to assert about signing different messages with
different keys. They could legitimately be the same.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/rsa_key_pair...
remoting/base/rsa_key_pair_unittest.cc:62: ASSERT_NE(key_pair1->GetPublicKey(),
key_pair2->GetPublicKey());
On 2013/03/06 00:43:25, Wez wrote:
> This check is a requirement for the test to be valid, so it really belongs
with
> the test init block, above, immediately after the keys are generated.

The *private keys* being different is the precondition. It's not a given that
the public keys for different private keys are different.

https://codereview.chromium.org/12316083/diff/5035/remoting/base/test_rsa_key...
File remoting/base/test_rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/base/test_rsa_key...
remoting/base/test_rsa_key_pair.h:8: const char kTestHostKeyPair[] =
On 2013/03/06 00:43:25, Wez wrote:
> nit: kTestRsaKeyPair

Done.

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/authenti...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/authenti...
remoting/protocol/authenticator_test_base.cc:56: base::Base64Encode(key_string,
&key_base64);
On 2013/03/06 00:43:25, Wez wrote:
> Can we switch to a Base64 encoded string in the file, and avoid the need for
> converting here?

These files are not owned by us, they're the test certificates for the entirety
of Chromium (they're in net/data/ssl/certificates).

More importantly, the certificate/key format we use, although straightforward,
isn't really a standard for certificate storage - the standards are either
binary DER or PEM (which is base64-encoded DER with some ---- BEGIN PUBLIC KEY
----- style dividers and line breaks).

Using base64-encoded DER without the PEM dividers is the most convenient format
for us (it allows us to handle it as text in Jingle/Javascript/HTTP without
wasting space with PEM headers), but that does put the onus on us to convert it 
to/from the standard formats when needed.

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/ssl_hmac...
File remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/ssl_hmac...
remoting/protocol/ssl_hmac_channel_authenticator_unittest.cc:67:
base::Base64Encode(key_string, &key_base64);
On 2013/03/06 00:43:25, Wez wrote:
> See previous comment re updating the test file.

See the answer there

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/v2_authe...
File remoting/protocol/v2_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/5035/remoting/protocol/v2_authe...
remoting/protocol/v2_authenticator.h:15: #include "remoting/base/rsa_key_pair.h"
On 2013/03/06 00:43:25, Wez wrote:
> nit: Can you forward-declare RsaKeyPair?

Done.

[Sergey Ulanov, 2013-03-06 20:51:34]

LGTM once my nits are addressed.

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:21: RsaKeyPair::RsaKeyPair() { }
remove space between the braces please

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:23: RsaKeyPair::~RsaKeyPair() { }
remove space between the braces please

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:28:
result->key_.reset(crypto::RSAPrivateKey::Create(2048));
Creation may fail in which case Create() will return NULL. In that case you want
to return null from this method.

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.h
File remoting/base/rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.h:45: RsaKeyPair();
Previously objects of this class were mutable, and not they are not (they must
be immutable because they are ref-counted). Yet you have to mutate key_ when
creating new instances.
I suggest that you pass the key_ value to the constructor, and then constructor
should DCHECK that it's not null.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/authent...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/authent...
remoting/protocol/authenticator_test_base.cc:56: base::Base64Encode(key_string,
&key_base64);
ASSERT_TRUE

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/negotia...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/negotia...
remoting/protocol/negotiating_authenticator.h:62: scoped_refptr<RsaKeyPair>
key_pair_;
local_key_pair_ please

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/ssl_hma...
File remoting/protocol/ssl_hmac_channel_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/ssl_hma...
remoting/protocol/ssl_hmac_channel_authenticator.h:87: scoped_refptr<RsaKeyPair>
key_pair_;
local_key_pair_.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/v2_auth...
File remoting/protocol/v2_authenticator.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/v2_auth...
remoting/protocol/v2_authenticator.cc:48: scoped_refptr<RsaKeyPair> key_pair,
local_key_pair

[rmsousa, 2013-03-07 03:27:44]

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
File remoting/base/rsa_key_pair.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:21: RsaKeyPair::RsaKeyPair() { }
On 2013/03/06 20:51:34, sergeyu wrote:
> remove space between the braces please

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:23: RsaKeyPair::~RsaKeyPair() { }
On 2013/03/06 20:51:34, sergeyu wrote:
> remove space between the braces please

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.cc:28:
result->key_.reset(crypto::RSAPrivateKey::Create(2048));
On 2013/03/06 20:51:34, sergeyu wrote:
> Creation may fail in which case Create() will return NULL. In that case you
want
> to return null from this method.

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pair.h
File remoting/base/rsa_key_pair.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/base/rsa_key_pai...
remoting/base/rsa_key_pair.h:45: RsaKeyPair();
On 2013/03/06 20:51:34, sergeyu wrote:
> Previously objects of this class were mutable, and not they are not (they must
> be immutable because they are ref-counted). Yet you have to mutate key_ when
> creating new instances.
> I suggest that you pass the key_ value to the constructor, and then
constructor
> should DCHECK that it's not null.

Good catch. Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/authent...
File remoting/protocol/authenticator_test_base.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/authent...
remoting/protocol/authenticator_test_base.cc:56: base::Base64Encode(key_string,
&key_base64);
On 2013/03/06 20:51:34, sergeyu wrote:
> ASSERT_TRUE

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/negotia...
File remoting/protocol/negotiating_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/negotia...
remoting/protocol/negotiating_authenticator.h:62: scoped_refptr<RsaKeyPair>
key_pair_;
On 2013/03/06 20:51:34, sergeyu wrote:
> local_key_pair_ please

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/ssl_hma...
File remoting/protocol/ssl_hmac_channel_authenticator.h (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/ssl_hma...
remoting/protocol/ssl_hmac_channel_authenticator.h:87: scoped_refptr<RsaKeyPair>
key_pair_;
On 2013/03/06 20:51:34, sergeyu wrote:
> local_key_pair_.

Done.

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/v2_auth...
File remoting/protocol/v2_authenticator.cc (right):

https://codereview.chromium.org/12316083/diff/23001/remoting/protocol/v2_auth...
remoting/protocol/v2_authenticator.cc:48: scoped_refptr<RsaKeyPair> key_pair,
On 2013/03/06 20:51:34, sergeyu wrote:
> local_key_pair

Done.

[commit-bot: I haz the power, 2013-03-07 03:28:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/rmsousa@chromium.org/12316083/33001

[commit-bot: I haz the power, 2013-03-07 03:28:31]

Failed to apply patch for remoting/protocol/negotiating_authenticator.cc:
While running patch -p1 --forward --force --no-backup-if-mismatch;
  patching file remoting/protocol/negotiating_authenticator.cc
  Hunk #1 FAILED at 8.
  1 out of 4 hunks FAILED -- saving rejects to file
remoting/protocol/negotiating_authenticator.cc.rej

Patch:       remoting/protocol/negotiating_authenticator.cc
Index: remoting/protocol/negotiating_authenticator.cc
diff --git a/remoting/protocol/negotiating_authenticator.cc
b/remoting/protocol/negotiating_authenticator.cc
index
a8ed3fe0338410d256bed493cc664531c8da904d..9be148054b3ec7ddf76ce7d25e7d2582d853f07d
100644
--- a/remoting/protocol/negotiating_authenticator.cc
+++ b/remoting/protocol/negotiating_authenticator.cc
@@ -8,9 +8,10 @@
 #include <sstream>
 
 #include "base/bind.h"
+#include "base/callback.h"
 #include "base/logging.h"
 #include "base/string_split.h"
-#include "crypto/rsa_private_key.h"
+#include "remoting/base/rsa_key_pair.h"
 #include "remoting/protocol/channel_authenticator.h"
 #include "remoting/protocol/v2_authenticator.h"
 #include "third_party/libjingle/source/talk/xmllite/xmlelement.h"
@@ -56,13 +57,13 @@ scoped_ptr<Authenticator>
NegotiatingAuthenticator::CreateForClient(
 // static
 scoped_ptr<Authenticator> NegotiatingAuthenticator::CreateForHost(
     const std::string& local_cert,
-    const crypto::RSAPrivateKey& local_private_key,
+    scoped_refptr<RsaKeyPair> key_pair,
     const std::string& shared_secret_hash,
     AuthenticationMethod::HashFunction hash_function) {
   scoped_ptr<NegotiatingAuthenticator> result(
       new NegotiatingAuthenticator(WAITING_MESSAGE));
   result->local_cert_ = local_cert;
-  result->local_private_key_.reset(local_private_key.Copy());
+  result->local_key_pair_ = key_pair;
   result->shared_secret_hash_ = shared_secret_hash;
 
   result->AddMethod(AuthenticationMethod::Spake2(hash_function));
@@ -70,11 +71,9 @@ scoped_ptr<Authenticator>
NegotiatingAuthenticator::CreateForHost(
   return scoped_ptr<Authenticator>(result.Pass());
 }
 
-
 NegotiatingAuthenticator::NegotiatingAuthenticator(
     Authenticator::State initial_state)
-    : certificate_sent_(false),
-      current_method_(AuthenticationMethod::Invalid()),
+    : current_method_(AuthenticationMethod::Invalid()),
       state_(initial_state),
       rejection_reason_(INVALID_CREDENTIALS) {
 }
@@ -226,20 +225,18 @@ NegotiatingAuthenticator::CreateChannelAuthenticator()
const {
 }
 
 bool NegotiatingAuthenticator::is_host_side() const {
-  return local_private_key_.get() != NULL;
+  return local_key_pair_.get() != NULL;
 }
 
 void NegotiatingAuthenticator::CreateAuthenticator(State initial_state) {
   if (is_host_side()) {
     current_authenticator_ = V2Authenticator::CreateForHost(
-        local_cert_, *local_private_key_.get(),
-        shared_secret_hash_, initial_state);
+        local_cert_, local_key_pair_, shared_secret_hash_, initial_state);
   } else {
     current_authenticator_ = V2Authenticator::CreateForClient(
         AuthenticationMethod::ApplyHashFunction(
             current_method_.hash_function(),
-            authentication_tag_, shared_secret_),
-        initial_state);
+            authentication_tag_, shared_secret_), initial_state);
   }
 }

[commit-bot: I haz the power, 2013-03-07 05:16:13]

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

[commit-bot: I haz the power, 2013-03-07 13:43:15]

Change committed as 186709