Added HostKeyPair class, signatures for heartbeat messages.

Added HostKeyPair class, signatures for heartbeat messages.
BUG=None
TEST=unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=54991

[Paweł Hajdan Jr., 2010-08-02 18:45:26]

Drive-by with some test comments. Please let me take another look before you
commit.

http://codereview.chromium.org/3087003/diff/4001/5006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/4001/5006#newcode118
remoting/host/heartbeat_sender_unittest.cc:118: EXPECT_TRUE(signature != NULL);
ASSERT to avoid a crash.

http://codereview.chromium.org/3087003/diff/4001/5006#newcode127
remoting/host/heartbeat_sender_unittest.cc:127: // Allow for 20 seconds
difference in case the test is slow,
This looks flaky to me. There will always be some amount of flake, no matter how
big time difference you will tolerate. Is it possible to test it in some other
way, or maybe split the flaky part to some other test?

[Sergey Ulanov, 2010-08-02 19:08:02]

Thanks.

http://codereview.chromium.org/3087003/diff/4001/5006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/4001/5006#newcode118
remoting/host/heartbeat_sender_unittest.cc:118: EXPECT_TRUE(signature != NULL);
On 2010/08/02 18:45:26, Paweł Hajdan Jr. wrote:
> ASSERT to avoid a crash.

Done.

http://codereview.chromium.org/3087003/diff/4001/5006#newcode127
remoting/host/heartbeat_sender_unittest.cc:127: // Allow for 20 seconds
difference in case the test is slow,
On 2010/08/02 18:45:26, Paweł Hajdan Jr. wrote:
> This looks flaky to me. There will always be some amount of flake, no matter
how
> big time difference you will tolerate. Is it possible to test it in some other
> way, or maybe split the flaky part to some other test?
Verifying this in another test would require duplicating most of this code.
Changed this to 10000 seconds (timeout for the whole test suite when running
under valgrind). This should be good enough: if difference is greater than 10000
seconds the test would timeout anyway.

[awong, 2010-08-02 19:53:49]

Yay!  The bones of some sort of security are starting for form!!!  Go Sergey!

I've added some comments.  The biggest thing that I'd like to see is a
description of the packet format and what the signature is trying to guarantee
written into the header file.  Audience should be people who will inherit this
code after us, and it should list what exactly we are hoping to guarantee (maybe
even briefly sketch the threat model)...

http://codereview.chromium.org/3087003/diff/9001/10002
File remoting/base/constants.h (right):

http://codereview.chromium.org/3087003/diff/9001/10002#newcode15
remoting/base/constants.h:15: extern const char kChromotingXmlNamespace[];
Add comment explaining what this Xml namespace is used for?

http://codereview.chromium.org/3087003/diff/9001/10004
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10004#newcode37
remoting/host/heartbeat_sender.cc:37: DCHECK(state_ != STARTED);
Should this be a positive filter?  That is, specify all the valid end-states
with an "or"?  It's more verbose, but a bit more robust to new states being
added.

http://codereview.chromium.org/3087003/diff/9001/10004#newcode123
remoting/host/heartbeat_sender.cc:123: buzz::XmlElement*
HeartbeatSender::CreateSignature(const std::string& jid) {
Somewhere, probably at the top of the .h file for HearbeatSender, we should give
a deep description of the packet format (ie., what the XML looks like) and what
the signature is covering.

http://codereview.chromium.org/3087003/diff/9001/10004#newcode132
remoting/host/heartbeat_sender.cc:132: std::string message = jid + ' ' +
time_str;
So we only sign the jid + time_str?

http://codereview.chromium.org/3087003/diff/9001/10005
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/3087003/diff/9001/10005#newcode31
remoting/host/heartbeat_sender.h:31: // cannot be parsed), true otherwise.
Drop the ", true otherwise."  It's implied.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode34
remoting/host/heartbeat_sender.h:34: // Starts heart-beating. Must be called
after init
Period at end of sentence.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode40
remoting/host/heartbeat_sender.h:40: // After it has been stopped, new sender
must be created instead.
What happens if you neglec to call Stop() before destructing the object? 
Will/Should we crash?  Document this behavior?

http://codereview.chromium.org/3087003/diff/9001/10005#newcode54
remoting/host/heartbeat_sender.h:54: void DoStop();
Been trying to switch us over to a different style of thread dispatching.  Can
you look at remoting/client/plugin/pepper_view.cc and see if the pattern there
will work?

Specifically, instad of creating a DoFoo function for ever Foo, favor adding a
block of code tot he start of Foo that does a thread-check and redispatches to
itself if not...

http://codereview.chromium.org/3087003/diff/9001/10005#newcode58
remoting/host/heartbeat_sender.h:58: buzz::XmlElement* CreateSignature(const
std::string& jid);
Who owns the returned XmlElement?

http://codereview.chromium.org/3087003/diff/9001/10006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10006#newcode14
remoting/host/heartbeat_sender_unittest.cc:14: #include
"remoting/host/test_key_pair.h"
alphabetical order

http://codereview.chromium.org/3087003/diff/9001/10006#newcode80
remoting/host/heartbeat_sender_unittest.cc:80: // This test creates a
HeartbeatSender object and then makes sure it does
Can you add a little more description on what high-level assertions this test is
trying to make?  It feels like this test is doing multiple things such as
validating the format of the heartbeat packet, and testing that the send call
actually pushes something to jingle.

Can this be broken apart at all?  The case is a bit hard to follow.  If not,
then a comment explaining the high-level asserts can help, but it'd be nice to
factor the test...

http://codereview.chromium.org/3087003/diff/9001/10006#newcode90
remoting/host/heartbeat_sender_unittest.cc:90: // iq_request is freed by
HeartbeatSender.
iq_request -> |iq_request|

http://codereview.chromium.org/3087003/diff/9001/10006#newcode99
remoting/host/heartbeat_sender_unittest.cc:99: buzz::XmlElement* stanza;
Throw this in a scoped_ptr, you have an  ASSERT_TRUE below.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode116
remoting/host/heartbeat_sender_unittest.cc:116: buzz::XmlElement* signature =
Does this not fit on one line?

http://codereview.chromium.org/3087003/diff/9001/10006#newcode128
remoting/host/heartbeat_sender_unittest.cc:128: // for example under valgrind.
Can we inject a clock?  Add a GetTimestamp() method that proxies the
base::Time::Now() call that we can override to return a constant?

http://codereview.chromium.org/3087003/diff/9001/10007
File remoting/host/host_key_pair.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10007#newcode7
remoting/host/host_key_pair.cc:7: #include <vector>
Alphabetical order

http://codereview.chromium.org/3087003/diff/9001/10007#newcode19
remoting/host/host_key_pair.cc:19: HostKeyPair::HostKeyPair() { };
Trailing semicolons not needed. Here and below.

http://codereview.chromium.org/3087003/diff/9001/10007#newcode30
remoting/host/host_key_pair.cc:30: LOG(ERROR) << "Failed to decode private key."
<< key_base64;
Do we want to dump the private key to screen even if it doesn't parse?

http://codereview.chromium.org/3087003/diff/9001/10007#newcode35
remoting/host/host_key_pair.cc:35: base::RSAPrivateKey *new_key =
Just use key_.reset() here?

http://codereview.chromium.org/3087003/diff/9001/10007#newcode47
remoting/host/host_key_pair.cc:47: bool HostKeyPair::Load(HostConfig*
host_config) {
These functions feel funny...almost like they should be in HostConfig instead of
here.

http://codereview.chromium.org/3087003/diff/9001/10008
File remoting/host/host_key_pair.h (right):

http://codereview.chromium.org/3087003/diff/9001/10008#newcode16
remoting/host/host_key_pair.h:16: }
namespace :P

http://codereview.chromium.org/3087003/diff/9001/10008#newcode23
remoting/host/host_key_pair.h:23: class HostKeyPair : public
base::RefCountedThreadSafe<HostKeyPair> {
Does this thing really need to be refcounted?  Do we actually need shared
lifetime semantics between multiple threads?

http://codereview.chromium.org/3087003/diff/9001/10008#newcode26
remoting/host/host_key_pair.h:26: virtual ~HostKeyPair();
The HearbeatSender class didn't have a virtual destructor.  Do we need to
specify a virtual destructor for RefCountedThreadSafe classes?  I thought there
was a precedent in Chrome to not make the destructor virtual if that's the only
parent and we don't expect descendents preferring instead to make the destructor
private and friending the RefCountedThreadSafe class... see
webkit/glue/plugins/plugin_instance.h for example.

http://codereview.chromium.org/3087003/diff/9001/10009
File remoting/host/host_key_pair_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10009#newcode14
remoting/host/host_key_pair_unittest.cc:14: #include
"remoting/host/test_key_pair.h"
ordering

http://codereview.chromium.org/3087003/diff/9001/10012
File remoting/host/test_key_pair.h (right):

http://codereview.chromium.org/3087003/diff/9001/10012#newcode9
remoting/host/test_key_pair.h:9:
"R3YUiJn/oVz0P+KLJSCWL4Uhjl3CytKg0nzuxgaJ9UOHxrl91k1XnLnwJytqkjl8kgWle+JJbiDx3"
ah...I love source files like these. :P :P

http://codereview.chromium.org/3087003/diff/9001/10016
File remoting/jingle_glue/jingle_client.h (right):

http://codereview.chromium.org/3087003/diff/9001/10016#newcode40
remoting/jingle_glue/jingle_client.h:40: START,
We're getting too many states with different tenses. :(

Could this be STARTING instead?  Or should it be STARTED?  Or maybe INITIALIZED?

[Sergey Ulanov, 2010-08-03 02:10:39]

http://codereview.chromium.org/3087003/diff/9001/10002
File remoting/base/constants.h (right):

http://codereview.chromium.org/3087003/diff/9001/10002#newcode15
remoting/base/constants.h:15: extern const char kChromotingXmlNamespace[];
On 2010/08/02 19:53:49, awong wrote:
> Add comment explaining what this Xml namespace is used for?

Done.

http://codereview.chromium.org/3087003/diff/9001/10004
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10004#newcode37
remoting/host/heartbeat_sender.cc:37: DCHECK(state_ != STARTED);
On 2010/08/02 19:53:49, awong wrote:
> Should this be a positive filter?  That is, specify all the valid end-states
> with an "or"?  It's more verbose, but a bit more robust to new states being
> added.

Done.

http://codereview.chromium.org/3087003/diff/9001/10004#newcode123
remoting/host/heartbeat_sender.cc:123: buzz::XmlElement*
HeartbeatSender::CreateSignature(const std::string& jid) {
On 2010/08/02 19:53:49, awong wrote:
> Somewhere, probably at the top of the .h file for HearbeatSender, we should
give
> a deep description of the packet format (ie., what the XML looks like) and
what
> the signature is covering.

Added info to the .h file. BTW this info is already in the design doc.

http://codereview.chromium.org/3087003/diff/9001/10004#newcode132
remoting/host/heartbeat_sender.cc:132: std::string message = jid + ' ' +
time_str;
On 2010/08/02 19:53:49, awong wrote:
> So we only sign the jid + time_str?
Yes. Do we need to sign anything else?

http://codereview.chromium.org/3087003/diff/9001/10005
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/3087003/diff/9001/10005#newcode31
remoting/host/heartbeat_sender.h:31: // cannot be parsed), true otherwise.
On 2010/08/02 19:53:49, awong wrote:
> Drop the ", true otherwise."  It's implied.

Done.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode34
remoting/host/heartbeat_sender.h:34: // Starts heart-beating. Must be called
after init
On 2010/08/02 19:53:49, awong wrote:
> Period at end of sentence.

Done.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode40
remoting/host/heartbeat_sender.h:40: // After it has been stopped, new sender
must be created instead.
On 2010/08/02 19:53:49, awong wrote:
> What happens if you neglec to call Stop() before destructing the object? 
> Will/Should we crash?  Document this behavior?

Done.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode54
remoting/host/heartbeat_sender.h:54: void DoStop();
On 2010/08/02 19:53:49, awong wrote:
> Been trying to switch us over to a different style of thread dispatching.  Can
> you look at remoting/client/plugin/pepper_view.cc and see if the pattern there
> will work?
> 
> Specifically, instad of creating a DoFoo function for ever Foo, favor adding a
> block of code tot he start of Foo that does a thread-check and redispatches to
> itself if not...

Done.

http://codereview.chromium.org/3087003/diff/9001/10005#newcode58
remoting/host/heartbeat_sender.h:58: buzz::XmlElement* CreateSignature(const
std::string& jid);
On 2010/08/02 19:53:49, awong wrote:
> Who owns the returned XmlElement?
Caller. Added comments about it.

http://codereview.chromium.org/3087003/diff/9001/10006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10006#newcode14
remoting/host/heartbeat_sender_unittest.cc:14: #include
"remoting/host/test_key_pair.h"
On 2010/08/02 19:53:49, awong wrote:
> alphabetical order

Done.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode80
remoting/host/heartbeat_sender_unittest.cc:80: // This test creates a
HeartbeatSender object and then makes sure it does
On 2010/08/02 19:53:49, awong wrote:
> Can you add a little more description on what high-level assertions this test
is
> trying to make?  It feels like this test is doing multiple things such as
> validating the format of the heartbeat packet, and testing that the send call
> actually pushes something to jingle.
> 
> Can this be broken apart at all?  The case is a bit hard to follow.  If not,
> then a comment explaining the high-level asserts can help, but it'd be nice to
> factor the test...

Refactored the message generation to a separate method. Split this test into two
tests.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode90
remoting/host/heartbeat_sender_unittest.cc:90: // iq_request is freed by
HeartbeatSender.
On 2010/08/02 19:53:49, awong wrote:
> iq_request -> |iq_request|

Done.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode99
remoting/host/heartbeat_sender_unittest.cc:99: buzz::XmlElement* stanza;
On 2010/08/02 19:53:49, awong wrote:
> Throw this in a scoped_ptr, you have an  ASSERT_TRUE below.
Can I use SaveArg() with scoped_ptr?

http://codereview.chromium.org/3087003/diff/9001/10006#newcode116
remoting/host/heartbeat_sender_unittest.cc:116: buzz::XmlElement* signature =
On 2010/08/02 19:53:49, awong wrote:
> Does this not fit on one line?

Done.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode128
remoting/host/heartbeat_sender_unittest.cc:128: // for example under valgrind.
On 2010/08/02 19:53:49, awong wrote:
> Can we inject a clock?  Add a GetTimestamp() method that proxies the
> base::Time::Now() call that we can override to return a constant?
Added start_time that clocks the time before the stanza was created, and now
just check that start_time <= time <= now.

http://codereview.chromium.org/3087003/diff/9001/10007
File remoting/host/host_key_pair.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10007#newcode7
remoting/host/host_key_pair.cc:7: #include <vector>
On 2010/08/02 19:53:49, awong wrote:
> Alphabetical order

Done.

http://codereview.chromium.org/3087003/diff/9001/10007#newcode19
remoting/host/host_key_pair.cc:19: HostKeyPair::HostKeyPair() { };
On 2010/08/02 19:53:49, awong wrote:
> Trailing semicolons not needed. Here and below.

Done.

http://codereview.chromium.org/3087003/diff/9001/10007#newcode30
remoting/host/host_key_pair.cc:30: LOG(ERROR) << "Failed to decode private key."
<< key_base64;
On 2010/08/02 19:53:49, awong wrote:
> Do we want to dump the private key to screen even if it doesn't parse?

Done.

http://codereview.chromium.org/3087003/diff/9001/10007#newcode35
remoting/host/host_key_pair.cc:35: base::RSAPrivateKey *new_key =
On 2010/08/02 19:53:49, awong wrote:
> Just use key_.reset() here?
This would reset key_ even if the key cannot be parsed. It will probably never
matter, still it is better to avoid this side-effect.

http://codereview.chromium.org/3087003/diff/9001/10007#newcode47
remoting/host/host_key_pair.cc:47: bool HostKeyPair::Load(HostConfig*
host_config) {
On 2010/08/02 19:53:49, awong wrote:
> These functions feel funny...almost like they should be in HostConfig instead
of
> here.
HostConfig is just a store for all config settings, and it is agnostic to the
format of each setting in the config. If we make HostConfig aware of format of
each and every setting in the config, it would eventually become too fat
(Get/Set for each setting in the config). We could add HostKeyPairStore class
that would read/write HostKeyPair from/to HostConfig, but IMHO it is overkill in
this case.

http://codereview.chromium.org/3087003/diff/9001/10008
File remoting/host/host_key_pair.h (right):

http://codereview.chromium.org/3087003/diff/9001/10008#newcode16
remoting/host/host_key_pair.h:16: }
On 2010/08/02 19:53:49, awong wrote:
> namespace :P

Done.

http://codereview.chromium.org/3087003/diff/9001/10008#newcode23
remoting/host/host_key_pair.h:23: class HostKeyPair : public
base::RefCountedThreadSafe<HostKeyPair> {
On 2010/08/02 19:53:49, awong wrote:
> Does this thing really need to be refcounted?  Do we actually need shared
> lifetime semantics between multiple threads?
The only reason it needs to be ref-counted is because we need to create a Task
for DoSave() when writing config. I would love to get rid this refcounter, if
only we could create Tasks for objects that are not refcounted.

http://codereview.chromium.org/3087003/diff/9001/10008#newcode26
remoting/host/host_key_pair.h:26: virtual ~HostKeyPair();
On 2010/08/02 19:53:49, awong wrote:
> The HearbeatSender class didn't have a virtual destructor.  Do we need to
> specify a virtual destructor for RefCountedThreadSafe classes?  I thought
there
> was a precedent in Chrome to not make the destructor virtual if that's the
only
> parent and we don't expect descendents preferring instead to make the
destructor
> private and friending the RefCountedThreadSafe class... see
> webkit/glue/plugins/plugin_instance.h for example.

removed it.

http://codereview.chromium.org/3087003/diff/9001/10009
File remoting/host/host_key_pair_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10009#newcode14
remoting/host/host_key_pair_unittest.cc:14: #include
"remoting/host/test_key_pair.h"
On 2010/08/02 19:53:49, awong wrote:
> ordering

Done.

http://codereview.chromium.org/3087003/diff/9001/10016
File remoting/jingle_glue/jingle_client.h (right):

http://codereview.chromium.org/3087003/diff/9001/10016#newcode40
remoting/jingle_glue/jingle_client.h:40: START,
On 2010/08/02 19:53:49, awong wrote:
> We're getting too many states with different tenses. :(
> 
> Could this be STARTING instead?  Or should it be STARTED?  Or maybe
INITIALIZED?

Done.

[awong, 2010-08-03 19:21:02]

Couple more comments.

http://codereview.chromium.org/3087003/diff/9001/10004
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10004#newcode123
remoting/host/heartbeat_sender.cc:123: buzz::XmlElement*
HeartbeatSender::CreateSignature(const std::string& jid) {
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > Somewhere, probably at the top of the .h file for HearbeatSender, we should
> give
> > a deep description of the packet format (ie., what the XML looks like) and
> what
> > the signature is covering.
> 
> Added info to the .h file. BTW this info is already in the design doc.


ah...forgot to look there. :)  More reason to put it in the code I guess. :P

http://codereview.chromium.org/3087003/diff/9001/10004#newcode132
remoting/host/heartbeat_sender.cc:132: std::string message = jid + ' ' +
time_str;
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > So we only sign the jid + time_str?
> Yes. Do we need to sign anything else?

Don't think so, but I was expecting to see something that signs the a while XML
node, not just the parsed data from it.  This is fine for now, but can we add a
TODO + file a bug to consider signing the XML node itself?  Or do you think I'm
being too paranoid?

http://codereview.chromium.org/3087003/diff/9001/10006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10006#newcode99
remoting/host/heartbeat_sender_unittest.cc:99: buzz::XmlElement* stanza;
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > Throw this in a scoped_ptr, you have an  ASSERT_TRUE below.
> Can I use SaveArg() with scoped_ptr?

I don't know....I would have expcted somethin glike

  SaveArg<2>(stanza.get())

to be equivalent.

http://codereview.chromium.org/3087003/diff/9001/10006#newcode128
remoting/host/heartbeat_sender_unittest.cc:128: // for example under valgrind.
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > Can we inject a clock?  Add a GetTimestamp() method that proxies the
> > base::Time::Now() call that we can override to return a constant?
> Added start_time that clocks the time before the stanza was created, and now
> just check that start_time <= time <= now. 

That works too.

http://codereview.chromium.org/3087003/diff/9001/10007
File remoting/host/host_key_pair.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10007#newcode35
remoting/host/host_key_pair.cc:35: base::RSAPrivateKey *new_key =
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > Just use key_.reset() here?
> This would reset key_ even if the key cannot be parsed. It will probably never
> matter, still it is better to avoid this side-effect.

hmm...I feel like this may actually even be preferable.  If we want to get all
crazy, I'd be tempted to make LoadFromString a static factory method and make
the HostKeyPair constructor private.  But that also might be overkill :P

http://codereview.chromium.org/3087003/diff/9001/10007#newcode47
remoting/host/host_key_pair.cc:47: bool HostKeyPair::Load(HostConfig*
host_config) {
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > These functions feel funny...almost like they should be in HostConfig
instead
> of
> > here.
> HostConfig is just a store for all config settings, and it is agnostic to the
> format of each setting in the config. If we make HostConfig aware of format of
> each and every setting in the config, it would eventually become too fat
> (Get/Set for each setting in the config). We could add HostKeyPairStore class
> that would read/write HostKeyPair from/to HostConfig, but IMHO it is overkill
in
> this case.
> 

A third class is deifnitely overkill.  Why would a host not have a keypair
though?  Isn't this common for all configs?

http://codereview.chromium.org/3087003/diff/9001/10008
File remoting/host/host_key_pair.h (right):

http://codereview.chromium.org/3087003/diff/9001/10008#newcode23
remoting/host/host_key_pair.h:23: class HostKeyPair : public
base::RefCountedThreadSafe<HostKeyPair> {
On 2010/08/03 02:10:39, sergeyu wrote:
> On 2010/08/02 19:53:49, awong wrote:
> > Does this thing really need to be refcounted?  Do we actually need shared
> > lifetime semantics between multiple threads?
> The only reason it needs to be ref-counted is because we need to create a Task
> for DoSave() when writing config. I would love to get rid this refcounter, if
> only we could create Tasks for objects that are not refcounted.

Ah!  we solved that!

See the DISABLE_RUNNABLE_METHOD_REFCOUNT() inside base/task.h.

Use it like

  DISABLE_RUNNABLE_METHOD_REFCOUNT(remoting::HostKeyPair);

declared outside the namespace.

http://codereview.chromium.org/3087003/diff/23001/24005
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/3087003/diff/23001/24005#newcode55
remoting/host/heartbeat_sender.h:55: // cannot be restarted after it has been
stopped, new sender must be created
"stopped, new sender" -> stopped.  A new sender

http://codereview.chromium.org/3087003/diff/23001/24017
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/3087003/diff/23001/24017#newcode57
remoting/jingle_glue/jingle_thread.h:57: friend class HeartbeatSenderTest;
Sometimes you friend the whole test class, and sometimes you use the
FRIEND_ALL_TEST_PREFIXES macro.  What's the difference?  Should we standardize
on one?

[Paweł Hajdan Jr., 2010-08-03 20:44:03]

Code I commented in the drive-by LGTM.

[Sergey Ulanov, 2010-08-04 01:41:12]

http://codereview.chromium.org/3087003/diff/9001/10004
File remoting/host/heartbeat_sender.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10004#newcode132
remoting/host/heartbeat_sender.cc:132: std::string message = jid + ' ' +
time_str;
On 2010/08/03 19:21:02, awong wrote:
> On 2010/08/03 02:10:39, sergeyu wrote:
> > On 2010/08/02 19:53:49, awong wrote:
> > > So we only sign the jid + time_str?
> > Yes. Do we need to sign anything else?
> 
> Don't think so, but I was expecting to see something that signs the a while
XML
> node, not just the parsed data from it.  This is fine for now, but can we add
a
> TODO + file a bug to consider signing the XML node itself?  Or do you think
I'm
> being too paranoid?
Yeah, that was my firth thought too, but the problem is that the JID is not in
the body of the request, it is in the envelope, i.e. the whole message looks as
follows:
 <iq from="jid" ...>
  <heartbeat>
   <signature>.signature.</signature>
  </heartbeat>
 </iq>

This particular code generates the stuff inside of <iq>, and the <iq> is
generated by IqRequset. Besides, we just need to confirm identity of the host
and signing jid is enough for that, we can trust the GTalk network that the
message is arrived the way it was sent, so no need to sign other fields.

http://codereview.chromium.org/3087003/diff/9001/10006
File remoting/host/heartbeat_sender_unittest.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10006#newcode99
remoting/host/heartbeat_sender_unittest.cc:99: buzz::XmlElement* stanza;
On 2010/08/03 19:21:02, awong wrote:
> On 2010/08/03 02:10:39, sergeyu wrote:
> > On 2010/08/02 19:53:49, awong wrote:
> > > Throw this in a scoped_ptr, you have an  ASSERT_TRUE below.
> > Can I use SaveArg() with scoped_ptr?
> 
> I don't know....I would have expcted somethin glike
> 
>   SaveArg<2>(stanza.get())
> 
> to be equivalent.
> 

This wouldn't work: stanza.get() would return NULL, but you need a place to
store argument value.

http://codereview.chromium.org/3087003/diff/9001/10007
File remoting/host/host_key_pair.cc (right):

http://codereview.chromium.org/3087003/diff/9001/10007#newcode47
remoting/host/host_key_pair.cc:47: bool HostKeyPair::Load(HostConfig*
host_config) {
On 2010/08/03 19:21:02, awong wrote:
> On 2010/08/03 02:10:39, sergeyu wrote:
> > On 2010/08/02 19:53:49, awong wrote:
> > > These functions feel funny...almost like they should be in HostConfig
> instead
> > of
> > > here.
> > HostConfig is just a store for all config settings, and it is agnostic to
the
> > format of each setting in the config. If we make HostConfig aware of format
of
> > each and every setting in the config, it would eventually become too fat
> > (Get/Set for each setting in the config). We could add HostKeyPairStore
class
> > that would read/write HostKeyPair from/to HostConfig, but IMHO it is
overkill
> in
> > this case.
> > 
> 
> A third class is deifnitely overkill.  Why would a host not have a keypair
> though?  Isn't this common for all configs?

HostConfig is probably a bad name for the class, should have named it
HostConfigStore or something. Basically it is similar to PrefStore interfaces
for the browser prefs.

http://codereview.chromium.org/3087003/diff/23001/24005
File remoting/host/heartbeat_sender.h (right):

http://codereview.chromium.org/3087003/diff/23001/24005#newcode55
remoting/host/heartbeat_sender.h:55: // cannot be restarted after it has been
stopped, new sender must be created
On 2010/08/03 19:21:02, awong wrote:
> "stopped, new sender" -> stopped.  A new sender

Done.

http://codereview.chromium.org/3087003/diff/23001/24017
File remoting/jingle_glue/jingle_thread.h (right):

http://codereview.chromium.org/3087003/diff/23001/24017#newcode57
remoting/jingle_glue/jingle_thread.h:57: friend class HeartbeatSenderTest;
On 2010/08/03 19:21:02, awong wrote:
> Sometimes you friend the whole test class, and sometimes you use the
> FRIEND_ALL_TEST_PREFIXES macro.  What's the difference?  Should we standardize
> on one?
HeartbeatSenderTest is a base class for the tests. I moved the code that needs
access to private members to HeartbeatSenderTest and so the
FRIEND_ALL_TEST_PREFIXES is not useful here.

[awong, 2010-08-04 01:48:47]

LGTM

Sounds mostly good.  I'm still slightly uncomfortable about the signing.
 Can you open a bug for that to talk it over with our security guys?

-A

On Tue, Aug 3, 2010 at 9:41 PM, <sergeyu@chromium.org> wrote:

>
> http://codereview.chromium.org/3087003/diff/9001/10004
> File remoting/host/heartbeat_sender.cc (right):
>
> http://codereview.chromium.org/3087003/diff/9001/10004#newcode132
> remoting/host/heartbeat_sender.cc:132: std::string message = jid + ' ' +
> time_str;
> On 2010/08/03 19:21:02, awong wrote:
>
>> On 2010/08/03 02:10:39, sergeyu wrote:
>> > On 2010/08/02 19:53:49, awong wrote:
>> > > So we only sign the jid + time_str?
>> > Yes. Do we need to sign anything else?
>>
>
>  Don't think so, but I was expecting to see something that signs the a
>>
> while XML
>
>> node, not just the parsed data from it.  This is fine for now, but can
>>
> we add a
>
>> TODO + file a bug to consider signing the XML node itself?  Or do you
>>
> think I'm
>
>> being too paranoid?
>>
> Yeah, that was my firth thought too, but the problem is that the JID is
> not in the body of the request, it is in the envelope, i.e. the whole
> message looks as follows:
>  <iq from="jid" ...>
>  <heartbeat>
>   <signature>.signature.</signature>
>  </heartbeat>
>  </iq>
>
> This particular code generates the stuff inside of <iq>, and the <iq> is
> generated by IqRequset. Besides, we just need to confirm identity of the
> host and signing jid is enough for that, we can trust the GTalk network
> that the message is arrived the way it was sent, so no need to sign
> other fields.
>
>
> http://codereview.chromium.org/3087003/diff/9001/10006
> File remoting/host/heartbeat_sender_unittest.cc (right):
>
> http://codereview.chromium.org/3087003/diff/9001/10006#newcode99
> remoting/host/heartbeat_sender_unittest.cc:99: buzz::XmlElement* stanza;
> On 2010/08/03 19:21:02, awong wrote:
>
>> On 2010/08/03 02:10:39, sergeyu wrote:
>> > On 2010/08/02 19:53:49, awong wrote:
>> > > Throw this in a scoped_ptr, you have an  ASSERT_TRUE below.
>> > Can I use SaveArg() with scoped_ptr?
>>
>
>  I don't know....I would have expcted somethin glike
>>
>
>    SaveArg<2>(stanza.get())
>>
>
>  to be equivalent.
>>
>
>
> This wouldn't work: stanza.get() would return NULL, but you need a place
> to store argument value.
>
>
> http://codereview.chromium.org/3087003/diff/9001/10007
> File remoting/host/host_key_pair.cc (right):
>
> http://codereview.chromium.org/3087003/diff/9001/10007#newcode47
> remoting/host/host_key_pair.cc:47: bool HostKeyPair::Load(HostConfig*
> host_config) {
> On 2010/08/03 19:21:02, awong wrote:
>
>> On 2010/08/03 02:10:39, sergeyu wrote:
>> > On 2010/08/02 19:53:49, awong wrote:
>> > > These functions feel funny...almost like they should be in
>>
> HostConfig
>
>> instead
>> > of
>> > > here.
>> > HostConfig is just a store for all config settings, and it is
>>
> agnostic to the
>
>> > format of each setting in the config. If we make HostConfig aware of
>>
> format of
>
>> > each and every setting in the config, it would eventually become too
>>
> fat
>
>> > (Get/Set for each setting in the config). We could add
>>
> HostKeyPairStore class
>
>> > that would read/write HostKeyPair from/to HostConfig, but IMHO it is
>>
> overkill
>
>> in
>> > this case.
>> >
>>
>
>  A third class is deifnitely overkill.  Why would a host not have a
>>
> keypair
>
>> though?  Isn't this common for all configs?
>>
>
> HostConfig is probably a bad name for the class, should have named it
> HostConfigStore or something. Basically it is similar to PrefStore
> interfaces for the browser prefs.
>
>
> http://codereview.chromium.org/3087003/diff/23001/24005
> File remoting/host/heartbeat_sender.h (right):
>
> http://codereview.chromium.org/3087003/diff/23001/24005#newcode55
> remoting/host/heartbeat_sender.h:55: // cannot be restarted after it has
> been stopped, new sender must be created
> On 2010/08/03 19:21:02, awong wrote:
>
>> "stopped, new sender" -> stopped.  A new sender
>>
>
> Done.
>
>
> http://codereview.chromium.org/3087003/diff/23001/24017
> File remoting/jingle_glue/jingle_thread.h (right):
>
> http://codereview.chromium.org/3087003/diff/23001/24017#newcode57
> remoting/jingle_glue/jingle_thread.h:57: friend class
> HeartbeatSenderTest;
> On 2010/08/03 19:21:02, awong wrote:
>
>> Sometimes you friend the whole test class, and sometimes you use the
>> FRIEND_ALL_TEST_PREFIXES macro.  What's the difference?  Should we
>>
> standardize
>
>> on one?
>>
> HeartbeatSenderTest is a base class for the tests. I moved the code that
> needs access to private members to HeartbeatSenderTest and so the
> FRIEND_ALL_TEST_PREFIXES is not useful here.
>
>
> http://codereview.chromium.org/3087003/show
>

[Sergey Ulanov, 2010-08-04 23:05:24]

Checked it in with TODO for signatures
On 2010/08/04 01:48:47, awong wrote:
> LGTM
> 
> Sounds mostly good.  I'm still slightly uncomfortable about the signing.
>  Can you open a bug for that to talk it over with our security guys?
>