Add gnubby authentication to remoting host

Add gnubby authentication to remoting host

The remoting host changes, including the associated policy definitions,
required to proxy gnubby traffic through a Chrome Remote Desktop
session.

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

[psj, 2014-01-23 21:28:39]

sergeyu: Please review everything under remoting/, or delegate to
lambroslambrou@ or rmsousa@ as necessary

dconnelly: Please review policy-related changes under chrome/

asvitkine: Please review histograms.xml

[Alexei Svitkine (slow), 2014-01-24 21:06:01]

This looks like a sizeable enough CL that it should probably have a
corresponding crbug.

[Sergey Ulanov, 2014-01-24 22:09:34]

On 2014/01/24 21:06:01, Alexei Svitkine wrote:
> This looks like a sizeable enough CL that it should probably have a
> corresponding crbug.

We already have crbug.com/134250. Paul please add BUG=134250 in the description.

[Sergey Ulanov, 2014-01-25 02:03:32]

Thanks for working on this feature. I can't wait to try it out!

https://codereview.chromium.org/138753005/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/138753005/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:268: {
key::kRemoteAccessHostAllowGnubbyAuth,
I don't think you need this. Chromoting host doesn't rely on policy code from
chrome (instead it uses remoting/host/policy_hack). We only share policy
templates.

https://codereview.chromium.org/138753005/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/138753005/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1937: const char kRemoteAccessAllowGnubbyAuth[] =
I don't think you need this.

https://codereview.chromium.org/138753005/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/138753005/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:236:
"RemoteAccessHostAllowGnubbyAuth": {
This is also not necessary.

https://codereview.chromium.org/138753005/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/138753005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:637: If this setting is
disabled or not configured, gnubby authentication requests will not be
proxied.''',
I think we should have it enabled by default.

https://codereview.chromium.org/138753005/diff/1/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/138753005/diff/1/remoting/host/client_session...
remoting/host/client_session.cc:203: } else if (message.type() == "gnubby-auth")
{
we added ExtensionMessage as a hack that allows to add new features  in cases
when we can't modify the client plugin. I hope that we can remove it completely
in the future after we migrate the client plugin to NaCl. It's better to add a
new message or multiple messages in remoting/proto/control.proto. Then you
wouldn't need to use JSON to format the messages.

https://codereview.chromium.org/138753005/diff/1/remoting/host/gnubby_adverti...
File remoting/host/gnubby_advertiser.cc (right):

https://codereview.chromium.org/138753005/diff/1/remoting/host/gnubby_adverti...
remoting/host/gnubby_advertiser.cc:29:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
It looks like you are using network_task_runner just to check that all methods
are called on the right thread. It better to use base::NonThreadSafe for this.

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

https://codereview.chromium.org/138753005/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:970: HOST_LOG << "Policy enables gnubby
auth.";
add {} because there is else case.

https://codereview.chromium.org/138753005/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/138753005/diff/1/remoting/remoting.gyp#newcod...
remoting/remoting.gyp:2616: 'host/gnubby_auth_handler.cc',
you do not need to duplicate these files here. remoting_unittest already depends
on remoting_host target which builds these files. (I have a pending CL that
removes the other non-test files above:
https://codereview.chromium.org/145003019/)

[psj, 2014-01-29 09:07:14]

https://codereview.chromium.org/138753005/diff/1/chrome/browser/policy/config...
File chrome/browser/policy/configuration_policy_handler_list_factory.cc (right):

https://codereview.chromium.org/138753005/diff/1/chrome/browser/policy/config...
chrome/browser/policy/configuration_policy_handler_list_factory.cc:268: {
key::kRemoteAccessHostAllowGnubbyAuth,
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> I don't think you need this. Chromoting host doesn't rely on policy code from
> chrome (instead it uses remoting/host/policy_hack). We only share policy
> templates.

I added it because other remoting policies are named here. I can definitely
remove it, though, if it really isn't needed.

https://codereview.chromium.org/138753005/diff/1/chrome/common/pref_names.cc
File chrome/common/pref_names.cc (right):

https://codereview.chromium.org/138753005/diff/1/chrome/common/pref_names.cc#...
chrome/common/pref_names.cc:1937: const char kRemoteAccessAllowGnubbyAuth[] =
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> I don't think you need this.

Same as before - other remoting policies are here.

https://codereview.chromium.org/138753005/diff/1/chrome/test/data/policy/poli...
File chrome/test/data/policy/policy_test_cases.json (right):

https://codereview.chromium.org/138753005/diff/1/chrome/test/data/policy/poli...
chrome/test/data/policy/policy_test_cases.json:236:
"RemoteAccessHostAllowGnubbyAuth": {
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> This is also not necessary.

Other remoting already here

https://codereview.chromium.org/138753005/diff/1/components/policy/resources/...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/138753005/diff/1/components/policy/resources/...
components/policy/resources/policy_templates.json:637: If this setting is
disabled or not configured, gnubby authentication requests will not be
proxied.''',
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> I think we should have it enabled by default.

Done.

https://codereview.chromium.org/138753005/diff/1/remoting/host/client_session.cc
File remoting/host/client_session.cc (right):

https://codereview.chromium.org/138753005/diff/1/remoting/host/client_session...
remoting/host/client_session.cc:203: } else if (message.type() == "gnubby-auth")
{
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> we added ExtensionMessage as a hack that allows to add new features  in cases
> when we can't modify the client plugin. I hope that we can remove it
completely
> in the future after we migrate the client plugin to NaCl. It's better to add a
> new message or multiple messages in remoting/proto/control.proto. Then you
> wouldn't need to use JSON to format the messages.

Wez said that I should use the extension message.

https://codereview.chromium.org/138753005/diff/1/remoting/host/gnubby_adverti...
File remoting/host/gnubby_advertiser.cc (right):

https://codereview.chromium.org/138753005/diff/1/remoting/host/gnubby_adverti...
remoting/host/gnubby_advertiser.cc:29:
scoped_refptr<base::SingleThreadTaskRunner> network_task_runner,
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> It looks like you are using network_task_runner just to check that all methods
> are called on the right thread. It better to use base::NonThreadSafe for this.

Done.

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

https://codereview.chromium.org/138753005/diff/1/remoting/host/remoting_me2me...
remoting/host/remoting_me2me_host.cc:970: HOST_LOG << "Policy enables gnubby
auth.";
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> add {} because there is else case.

Done.

https://codereview.chromium.org/138753005/diff/1/remoting/remoting.gyp
File remoting/remoting.gyp (right):

https://codereview.chromium.org/138753005/diff/1/remoting/remoting.gyp#newcod...
remoting/remoting.gyp:2616: 'host/gnubby_auth_handler.cc',
On 2014/01/25 02:03:33, Sergey Ulanov wrote:
> you do not need to duplicate these files here. remoting_unittest already
depends
> on remoting_host target which builds these files. (I have a pending CL that
> removes the other non-test files above:
> https://codereview.chromium.org/145003019/)

Hmm, I was getting unresolved link errors before I added those, but it seems to
be working fine now...

[dconnelly, 2014-01-29 09:21:07]

lgtm after you update the supported_on version.

https://codereview.chromium.org/138753005/diff/320001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/138753005/diff/320001/components/policy/resou...
components/policy/resources/policy_templates.json:632: 'supported_on':
['chrome.*:30-'],
Update the supported_on field to whichever Chrome release you're targeting.

[Alexei Svitkine (slow), 2014-01-29 14:54:55]

histograms.xml lgtm

[psj, 2014-02-04 23:43:56]

Changed implementation to use Unix Domain Sockets instead of TCP sockets

https://codereview.chromium.org/138753005/diff/320001/components/policy/resou...
File components/policy/resources/policy_templates.json (right):

https://codereview.chromium.org/138753005/diff/320001/components/policy/resou...
components/policy/resources/policy_templates.json:632: 'supported_on':
['chrome.*:30-'],
On 2014/01/29 09:21:08, dconnelly wrote:
> Update the supported_on field to whichever Chrome release you're targeting.

Done.

[psj, 2014-02-08 00:44:17]

Ping. remoting code is ready for review

[Sergey Ulanov, 2014-02-09 22:29:53]

I'm still not sure if we should use protocol extensions for gnubby. I'll talk
Wez about it.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/client_se...
remoting/host/client_session.h:101: bool enable_gnubby_auth);
I suggest adding a method for GnubbyAuthHandler in DesktopEnvironment. That way
you wouldn't need this flag. Instead DesktopEnvironment would just return NULL
if gnubby auth is not enabled.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:63: for (ActiveSockets::iterator iter =
active_sockets_.begin();
STLDeleteContainerPairFirstPointers() from stl_util.h

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:154: message.set_data("control " +
control_type + " " + data);
It seems strange to use JSON to encode message type for incoming messages, and
this ad-hoc encoding for outgoing ones.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:29: 
nit: don't need this empty line.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:36: // StreamListenSocket::Delegate
interface.
nit: If this interface is not intended to be used externally then it's better to
move this declaration to the private section (inheritance is still public). Not
all code in chromium follows that style, but some does, and it's considered the
right thing to do. In either case it's better to move it after all other public
methods.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:38: scoped_ptr<net::StreamListenSocket>
socket);
add OVERRIDE at then end of overridden method.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:45: virtual void DeliverClientMessage(const
std::string message);
do these methods need to be virtual?

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:45: virtual void DeliverClientMessage(const
std::string message);
use const reference to pass strings. Same for the methods below.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:56: static void
SetGnubbySocketName(base::FilePath gnubby_socket_name);
const base::FilePath&

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:56: static void
SetGnubbySocketName(base::FilePath gnubby_socket_name);
nit: move this after the destructor before all  public, non-static methods.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:60: private:
nit: empty line above this one.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:71: int current_connection_id_;
Maybe last_connection_id_? To make it clear that this is used to generate new
Ids

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:74: typedef
std::vector<std::pair<net::StreamListenSocket*, int> > ActiveSockets;
Can this be std::map? Then you wouldn't need both CompareSocket() and
CompareConnection()

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
File remoting/host/gnubby_utils.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.cc:29: class BlobReader {
Reuse Pickle from base/pickle.h?

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.cc:61: class BlobWriter {
ditto

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
File remoting/host/gnubby_utils.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:30: static void GetResponseFromJson(std::string
json, std::string* data);
const-ref for the first argument please.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:33: // No construction - all methods are static.
Don't really need a class in this case - just declare two functions (similar to
auth_util.h)

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:38: static void WebSafeBase64Encode(const
std::string& data,
Since these are all private they don't need to be in the .h file, they can be
just static functions in .cc file.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:112: const char kAuthSocknameSwitchName[] =
"ssh-auth-sockname";
can it be called gnubby-auth-sockname (because it has nothing to do with SSH).

https://codereview.chromium.org/138753005/diff/380001/remoting/remoting_host....
File remoting/remoting_host.gypi (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/remoting_host....
remoting/remoting_host.gypi:115: 'host/gnubby_auth_handler.cc',
This file is posix-specific, and I think it won't compile on windows. Which
means it needs to be excluded on windows. If you add _posix suffix then it will
be excluded automatically and then we will need some ifdefs in the code where
it's used. But it would be better to create a stub gnubby_auto_handler_win.cc
which declares GnubbyAuthHandler::Create() that just returns NULL - that's
consistent with how other cross-platform code works.

[Sergey Ulanov, 2014-02-10 19:09:26]

On 2014/02/09 22:29:53, Sergey Ulanov wrote:
> I'm still not sure if we should use protocol extensions for gnubby. I'll talk
> Wez about it.

I've just discussed it with Wez. We agreed that using extensions (current
approach) is the right thing to do.

[psj, 2014-02-10 22:57:21]

Ready for another review of remoting. Thanks

https://codereview.chromium.org/138753005/diff/380001/remoting/host/client_se...
File remoting/host/client_session.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/client_se...
remoting/host/client_session.h:101: bool enable_gnubby_auth);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> I suggest adding a method for GnubbyAuthHandler in DesktopEnvironment. That
way
> you wouldn't need this flag. Instead DesktopEnvironment would just return NULL
> if gnubby auth is not enabled.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:63: for (ActiveSockets::iterator iter =
active_sockets_.begin();
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> STLDeleteContainerPairFirstPointers() from stl_util.h

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:154: message.set_data("control " +
control_type + " " + data);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> It seems strange to use JSON to encode message type for incoming messages, and
> this ad-hoc encoding for outgoing ones.

They aren't so different as they are both a space-delimited set of string
arguments. In the "data" message case, it just happens that the last argument is
json while the "control" message is plain text. At any rate, this became
obsolete when I switched to the Unix Domain Socket and it is now gone.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:29: 
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> nit: don't need this empty line.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:36: // StreamListenSocket::Delegate
interface.
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> nit: If this interface is not intended to be used externally then it's better
to
> move this declaration to the private section (inheritance is still public).
Not
> all code in chromium follows that style, but some does, and it's considered
the
> right thing to do. In either case it's better to move it after all other
public
> methods. 

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:38: scoped_ptr<net::StreamListenSocket>
socket);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> add OVERRIDE at then end of overridden method.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:45: virtual void DeliverClientMessage(const
std::string message);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> use const reference to pass strings. Same for the methods below.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:45: virtual void DeliverClientMessage(const
std::string message);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> do these methods need to be virtual?

They are virtual for test mocks

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:56: static void
SetGnubbySocketName(base::FilePath gnubby_socket_name);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> const base::FilePath&

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:56: static void
SetGnubbySocketName(base::FilePath gnubby_socket_name);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> const base::FilePath&

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:60: private:
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> nit: empty line above this one.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:71: int current_connection_id_;
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> Maybe last_connection_id_? To make it clear that this is used to generate new
> Ids

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:74: typedef
std::vector<std::pair<net::StreamListenSocket*, int> > ActiveSockets;
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> Can this be std::map? Then you wouldn't need both CompareSocket() and
> CompareConnection()

No, I need to search both by socket (for incoming request data) and connection
id (for response from webapp)

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
File remoting/host/gnubby_utils.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.cc:29: class BlobReader {
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> Reuse Pickle from base/pickle.h?

Unfortunately not. Pickle pads to int boundaries after every write, while the
gnubbyd extension expects the data to be packed.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.cc:61: class BlobWriter {
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> ditto

ditto

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
File remoting/host/gnubby_utils.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:30: static void GetResponseFromJson(std::string
json, std::string* data);
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> const-ref for the first argument please.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:33: // No construction - all methods are static.
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> Don't really need a class in this case - just declare two functions (similar
to
> auth_util.h)

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_ut...
remoting/host/gnubby_utils.h:38: static void WebSafeBase64Encode(const
std::string& data,
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> Since these are all private they don't need to be in the .h file, they can be
> just static functions in .cc file.

Done.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:112: const char kAuthSocknameSwitchName[] =
"ssh-auth-sockname";
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> can it be called gnubby-auth-sockname (because it has nothing to do with SSH).

I proposed a name with gnubby, but Wez preferred this as it matches the value of
the SSH_AUTH_SOCK environment variable set in the script.

https://codereview.chromium.org/138753005/diff/380001/remoting/remoting_host....
File remoting/remoting_host.gypi (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/remoting_host....
remoting/remoting_host.gypi:115: 'host/gnubby_auth_handler.cc',
On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> This file is posix-specific, and I think it won't compile on windows. Which
> means it needs to be excluded on windows. If you add _posix suffix then it
will
> be excluded automatically and then we will need some ifdefs in the code where
> it's used. But it would be better to create a stub gnubby_auto_handler_win.cc
> which declares GnubbyAuthHandler::Create() that just returns NULL - that's
> consistent with how other cross-platform code works.

Done. I looked at net.gyp to see if unix_domain_socket_posix.cc was
platform-specific, but I didn't realize that file names themselves are
significant within the build system.

[Sergey Ulanov, 2014-02-11 08:20:36]

Mostly style nits left :-)

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:154: message.set_data("control " +
control_type + " " + data);
On 2014/02/10 22:57:22, psj wrote:
> On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> > It seems strange to use JSON to encode message type for incoming messages,
and
> > this ad-hoc encoding for outgoing ones.
> 
> They aren't so different as they are both a space-delimited set of string
> arguments. In the "data" message case, it just happens that the last argument
is
> json while the "control" message is plain text. At any rate, this became
> obsolete when I switched to the Unix Domain Socket and it is now gone.

I was pointing at DeliverClientMessage() - that method parses extension messages
received from the client as JSON. But DeliverHostDataMessage() formats messages
sent to the client as space-delimited lists.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:74: typedef
std::vector<std::pair<net::StreamListenSocket*, int> > ActiveSockets;
On 2014/02/10 22:57:22, psj wrote:
> On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> > Can this be std::map? Then you wouldn't need both CompareSocket() and
> > CompareConnection()
> 
> No, I need to search both by socket (for incoming request data) and connection
> id (for response from webapp)

Right, but you can still use find_if() with map<> to search in the reverse
direction direction. It's just seems more natural to use map<> because it's
essentially a map.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:11: #include "base/files/file_path.h"
nit: FilePath can be forward-declared.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:12: #include "base/logging.h"
This can be remove if you remove DCHECK below - see my comment about
client_stub_

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:16: namespace protocol {
nit: empty line above this one

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:47: protocol::ClientStub* client_stub_;
Please move this to GnubbyAuthHandlerPosix. GnubbyAuthHandler should be a pure
interface with no data members. It will be more consistent with the rest of
chromoting code this way.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:121: bool result =
value->GetAsList(&list);
value may be NULL here if |message| is not valid JSON. 

Also if value is not a list this function will just ignore the message. At least
this problem deserves to be logged. Also it's more common in chromium code to
exit as soon as an error is detected e.g.:
if (!value->GetAsList(&list)) {
 LOG(ERROR) << "Received invalid gnubby message: " << message;
 return;
}

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:186: if
(GetJsonFromGnubbyRequest(data, len, &json)) {
Given that the |socket| is a stream socket can it happen that we receive only
partial message here and need to wait for the second DidRead() before parsing
the message?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:190: HOST_LOG << "Could not decode
blob";
Maybe "Could not decode gnubby request" to make it clear that the message is
gnubby-related?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:219:
net::UnixDomainSocket::NoAuthentication());
Is it acceptable to allow other users on this machine to connect to the socket?
Maybe limit it only to processes with the same UID?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
File remoting/host/gnubby_util.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:45: for (int i = 0; i < num_equals; ++i) {
temp.append(num_equals, '=');

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:105: void WriteByte(char value);
uint8_t instead of char?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:108: void WriteSize(int value);
size_t to indicate that it's unsigned?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:117: void GetData(std::string* data) const;
Maybe just return std::string?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:145: *value = le32toh(
AFAIK this is not a standard function. E.g. it doesn't exist on Mac. You can use
base::NetToHost32() from base/sys_byteorder.h, but I don't think you need it
given that the value is assembled byte by byte anyway. I.e. you can replace this
expression with:
 *value = 
       (data_[index_] & 255) + ((data_[index_ + 1] & 255) << 8) +
       ((data_[index_ + 2] & 255) << 16) + ((data_[index_ + 3] & 255) << 24);

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:153: int blobSize;
blob_size

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:154: if (!ReadSize(&blobSize) || data_len_ -
(index_ + blobSize) < 0) {
data_len_ < index_ + blobSize?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:165: if (!ReadSize(&length) || data_len_ - (index_
+ length) < 0) {
data_len_ < index_ + length?

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:181: int le_value = htole32(value);
Same comment as the one above about le32toh()

https://codereview.chromium.org/138753005/diff/500001/remoting/host/me2me_des...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/me2me_des...
remoting/host/me2me_desktop_environment.cc:70: if (gnubby_auth_enabled_) {
please add thread DCHECK():
 DCHECK(caller_task_runner()->BelongsToCurrentThread());

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:620: if (!gnubby_socket_name.empty()) {
nit: remove {} in single-line if statement (I do realize this file is not super
consistent about it)

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:990: if (desktop_environment_factory_) {
nit: move this after the if statement in line 994.
remove {} here for consistency with the following if.

[psj, 2014-02-12 09:01:00]

I addressed all of your comments. Please take another look at remoting.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.cc (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.cc:154: message.set_data("control " +
control_type + " " + data);
On 2014/02/11 08:20:37, Sergey Ulanov wrote:
> On 2014/02/10 22:57:22, psj wrote:
> > On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> > > It seems strange to use JSON to encode message type for incoming messages,
> and
> > > this ad-hoc encoding for outgoing ones.
> > 
> > They aren't so different as they are both a space-delimited set of string
> > arguments. In the "data" message case, it just happens that the last
argument
> is
> > json while the "control" message is plain text. At any rate, this became
> > obsolete when I switched to the Unix Domain Socket and it is now gone.
> 
> I was pointing at DeliverClientMessage() - that method parses extension
messages
> received from the client as JSON. But DeliverHostDataMessage() formats
messages
> sent to the client as space-delimited lists.

Ah, I see what you mean now. This is due to the extension message implementation
in the webapp, which wraps host-bound data in JSON. Since the ExtensionMessage
docs say that the encoding of data is on a per-message-type basis, I believe it
is wrong to do so. Since nobody else is using extension messages, I'll change
the webapp to stop doing this.

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/380001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:74: typedef
std::vector<std::pair<net::StreamListenSocket*, int> > ActiveSockets;
On 2014/02/11 08:20:37, Sergey Ulanov wrote:
> On 2014/02/10 22:57:22, psj wrote:
> > On 2014/02/09 22:29:54, Sergey Ulanov wrote:
> > > Can this be std::map? Then you wouldn't need both CompareSocket() and
> > > CompareConnection()
> > 
> > No, I need to search both by socket (for incoming request data) and
connection
> > id (for response from webapp)
> 
> Right, but you can still use find_if() with map<> to search in the reverse
> direction direction. It's just seems more natural to use map<> because it's
> essentially a map.

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler.h (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:11: #include "base/files/file_path.h"
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> nit: FilePath can be forward-declared.

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:12: #include "base/logging.h"
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> This can be remove if you remove DCHECK below - see my comment about
> client_stub_

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:16: namespace protocol {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> nit: empty line above this one

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler.h:47: protocol::ClientStub* client_stub_;
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Please move this to GnubbyAuthHandlerPosix. GnubbyAuthHandler should be a pure
> interface with no data members. It will be more consistent with the rest of
> chromoting code this way.

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:121: bool result =
value->GetAsList(&list);
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> value may be NULL here if |message| is not valid JSON. 
> 
> Also if value is not a list this function will just ignore the message. At
least
> this problem deserves to be logged. Also it's more common in chromium code to
> exit as soon as an error is detected e.g.:
> if (!value->GetAsList(&list)) {
>  LOG(ERROR) << "Received invalid gnubby message: " << message;
>  return;
> }

Ack

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:186: if
(GetJsonFromGnubbyRequest(data, len, &json)) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Given that the |socket| is a stream socket can it happen that we receive only
> partial message here and need to wait for the second DidRead() before parsing
> the message?

No idea. The message easily fits in the socket read buffer but I guess it's
possible for only a partial write/read to occur.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:190: HOST_LOG << "Could not decode
blob";
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Maybe "Could not decode gnubby request" to make it clear that the message is
> gnubby-related?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:219:
net::UnixDomainSocket::NoAuthentication());
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Is it acceptable to allow other users on this machine to connect to the
socket?
> Maybe limit it only to processes with the same UID?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
File remoting/host/gnubby_util.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:45: for (int i = 0; i < num_equals; ++i) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> temp.append(num_equals, '=');

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:105: void WriteByte(char value);
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> uint8_t instead of char?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:108: void WriteSize(int value);
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> size_t to indicate that it's unsigned?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:117: void GetData(std::string* data) const;
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Maybe just return std::string?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:145: *value = le32toh(
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> AFAIK this is not a standard function. E.g. it doesn't exist on Mac. You can
use
> base::NetToHost32() from base/sys_byteorder.h, but I don't think you need it
> given that the value is assembled byte by byte anyway. I.e. you can replace
this
> expression with:
>  *value = 
>        (data_[index_] & 255) + ((data_[index_ + 1] & 255) << 8) +
>        ((data_[index_ + 2] & 255) << 16) + ((data_[index_ + 3] & 255) << 24);

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:153: int blobSize;
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> blob_size

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:154: if (!ReadSize(&blobSize) || data_len_ -
(index_ + blobSize) < 0) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> data_len_ < index_ + blobSize?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:165: if (!ReadSize(&length) || data_len_ - (index_
+ length) < 0) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> data_len_ < index_ + length?

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:181: int le_value = htole32(value);
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> Same comment as the one above about le32toh()

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/me2me_des...
File remoting/host/me2me_desktop_environment.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/me2me_des...
remoting/host/me2me_desktop_environment.cc:70: if (gnubby_auth_enabled_) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> please add thread DCHECK():
>  DCHECK(caller_task_runner()->BelongsToCurrentThread());

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
File remoting/host/remoting_me2me_host.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:620: if (!gnubby_socket_name.empty()) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> nit: remove {} in single-line if statement (I do realize this file is not
super
> consistent about it)

Done.

https://codereview.chromium.org/138753005/diff/500001/remoting/host/remoting_...
remoting/host/remoting_me2me_host.cc:990: if (desktop_environment_factory_) {
On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> nit: move this after the if statement in line 994.
> remove {} here for consistency with the following if.

Done.

[Sergey Ulanov, 2014-02-14 07:31:58]

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:186: if
(GetJsonFromGnubbyRequest(data, len, &json)) {
On 2014/02/12 09:01:01, psj wrote:
> On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> > Given that the |socket| is a stream socket can it happen that we receive
only
> > partial message here and need to wait for the second DidRead() before
parsing
> > the message?
> 
> No idea. The message easily fits in the socket read buffer but I guess it's
> possible for only a partial write/read to occur.

Right. So it looks like this code will work properly in most cases, but it's
very fragile. It will break if messages become bigger, or StreamListenSocket is
changed to use different buffer size, or the sender sends a partial message, or
we try to run it on a different OS that handles socket buffers differently (e.g.
it may not work on OS X).

I think it should not be hard to fix it: Each message starts with it's size, so
we can always know if we've received a full message and if not, the data should
be saved for when then next chunk is received. Just need to limit total buffer
size, to make sure we don't buffer data indefinitely when we receive completely
bogus data.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:93: base::SplitString(message, ' ',
&values);
This will also split the third component that is in JSON (and so it may contain
spaces internally), and I don't think we want it here, so you can't use
base::SplitString() here.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:108:
GetGnubbyResponseFromJson(values[2], &reply);
You need to verify that |values| contains at least 3 elements. This code will
crash if it doesn't. Same for values[1] in control messages.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
File remoting/host/gnubby_util.cc (right):

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:7: #include <endian.h>
remove this include - won't compile on OSX.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:232: result = result &&
blob->ReadBlobReader(&sign_request);
nit: I think you can write this expression like this:
   result = blob->Read(..) &&
       sign_request.Read(..) &&
       sign_request.Read(..) &&
       sign_request.Read(..);
IMO it's more readable this way, but feel free to disagree.

[psj, 2014-02-15 00:01:33]

remoting ready for another look

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/500001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:186: if
(GetJsonFromGnubbyRequest(data, len, &json)) {
On 2014/02/14 07:31:59, Sergey Ulanov wrote:
> On 2014/02/12 09:01:01, psj wrote:
> > On 2014/02/11 08:20:38, Sergey Ulanov wrote:
> > > Given that the |socket| is a stream socket can it happen that we receive
> only
> > > partial message here and need to wait for the second DidRead() before
> parsing
> > > the message?
> > 
> > No idea. The message easily fits in the socket read buffer but I guess it's
> > possible for only a partial write/read to occur.
> 
> Right. So it looks like this code will work properly in most cases, but it's
> very fragile. It will break if messages become bigger, or StreamListenSocket
is
> changed to use different buffer size, or the sender sends a partial message,
or
> we try to run it on a different OS that handles socket buffers differently
(e.g.
> it may not work on OS X).
> 
> I think it should not be hard to fix it: Each message starts with it's size,
so
> we can always know if we've received a full message and if not, the data
should
> be saved for when then next chunk is received. Just need to limit total buffer
> size, to make sure we don't buffer data indefinitely when we receive
completely
> bogus data.

Yes, I already implemented this. See active_requests_

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:93: base::SplitString(message, ' ',
&values);
On 2014/02/14 07:31:59, Sergey Ulanov wrote:
> This will also split the third component that is in JSON (and so it may
contain
> spaces internally), and I don't think we want it here, so you can't use
> base::SplitString() here.

Done.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_au...
remoting/host/gnubby_auth_handler_posix.cc:108:
GetGnubbyResponseFromJson(values[2], &reply);
On 2014/02/14 07:31:59, Sergey Ulanov wrote:
> You need to verify that |values| contains at least 3 elements. This code will
> crash if it doesn't. Same for values[1] in control messages.

Ack

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
File remoting/host/gnubby_util.cc (right):

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:7: #include <endian.h>
On 2014/02/14 07:31:59, Sergey Ulanov wrote:
> remove this include - won't compile on OSX.

Done.

https://codereview.chromium.org/138753005/diff/630001/remoting/host/gnubby_ut...
remoting/host/gnubby_util.cc:232: result = result &&
blob->ReadBlobReader(&sign_request);
On 2014/02/14 07:31:59, Sergey Ulanov wrote:
> nit: I think you can write this expression like this:
>    result = blob->Read(..) &&
>        sign_request.Read(..) &&
>        sign_request.Read(..) &&
>        sign_request.Read(..);
> IMO it's more readable this way, but feel free to disagree.

Done.

[psj, 2014-02-15 01:31:25]

Added a buffer size limit to active_requests

[Sergey Ulanov, 2014-02-19 09:34:27]

lgtm

[psj, 2014-02-19 17:12:40]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-19 17:18:03]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/890001

[commit-bot: I haz the power, 2014-02-19 17:35:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 17:36:00]

Try jobs failed on following builders: linux_chromium_rel

[psj, 2014-02-19 17:53:58]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-19 17:58:12]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/890001

[commit-bot: I haz the power, 2014-02-19 18:14:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 18:14:55]

Try jobs failed on following builders: linux_chromium_rel, mac_chromium_rel

[psj, 2014-02-19 19:06:43]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-19 19:36:59]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1220001

[commit-bot: I haz the power, 2014-02-19 20:23:48]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 20:23:49]

Try jobs failed on following builders: linux_chromium_rel

[psj, 2014-02-19 21:21:39]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-19 21:24:25]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1430001

[commit-bot: I haz the power, 2014-02-19 22:47:42]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-02-19 22:47:49]

Retried try job too often on win_rel for step(s) app_list_unittests,
ash_unittests, aura_unittests, base_unittests, browser_tests,
cacheinvalidation_unittests, cc_unittests, check_deps, check_deps2git,
chrome_elf_unittests, chromedriver_unittests, components_unittests,
compositor_unittests, content_browsertests, content_unittests, crypto_unittests,
events_unittests, google_apis_unittests, gpu_unittests,
installer_util_unittests, interactive_ui_tests, ipc_tests, jingle_unittests,
media_unittests, mini_installer_test, nacl_integration, net_unittests,
ppapi_unittests, printing_unittests, remoting_unittests, sql_unittests,
sync_integration_tests, sync_unit_tests, telemetry_perf_unittests,
telemetry_unittests, unit_tests, views_unittests
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...

[psj, 2014-02-19 23:06:34]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-19 23:17:35]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1430001

[psj, 2014-02-20 00:31:04]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-20 01:06:42]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1430001

[commit-bot: I haz the power, 2014-02-20 01:07:44]

Failed to trigger a try job on win_x64_rel
HTTP Error 400: Bad Request

[psj, 2014-02-20 01:30:56]

The CQ bit was unchecked by psj@chromium.org

[psj, 2014-02-20 01:31:40]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-02-20 01:42:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1710001

[commit-bot: I haz the power, 2014-02-20 05:12:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1710001

[commit-bot: I haz the power, 2014-02-20 09:08:39]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1710001

[commit-bot: I haz the power, 2014-02-20 12:29:48]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/psj@chromium.org/138753005/1710001

[commit-bot: I haz the power, 2014-02-20 13:28:22]

Change committed as 252217