Do minimal processing of gnubby data. Add request timeouts and send error

Do minimal processing of gnubby data. Add request timeouts and send error
responses as necessary.

Requires gnubbyd 0.8.37 or later.

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

[Sergey Ulanov, 2014-03-21 02:09:37]

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:262:
GnubbyAuthHandlerPosix::GetActiveSocket(base::DictionaryValue* dictionary) {
Maybe GetSocketForMessage()? Also rename dictionary->message?

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:281: LOG(ERROR) << "Request timed
out";
HOST_LOG please. Also update the message to indicate that it's Gnubby-related.
Though maybe we don't need this message at all.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.cc (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:37: 
Add DCHECK(IsRequestComplete() && !IsRequestTooLarge())?

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:39: data_out->assign(request_data_.data() + 4,
request_data_.size() - 4);
define a const for 4, e.g. kRequestSizeBytes

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:39: data_out->assign(request_data_.data() + 4,
request_data_.size() - 4);
You can replace the last parameter with request_data_.end(), then you wouldn't
need '-4'.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:46: if (request_data_.size() < 4)
define a const for 4, e.g. kRequestSizeBytes

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:70: socket_->Send(kSshError, sizeof(kSshError) /
sizeof(*kSshError));
Use arraysize() from base/macros.h

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:83: return ((request_data_[0] & 255) << 24) +
((request_data_[1] & 255) << 16) +
DCHECK that there are at least 4 bytes in |request_data_|

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:100: void GnubbySocket::ResetTimer() {
timer_->Reset(); }
move the body to a separate line

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.h (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:14: #include "net/socket/stream_listen_socket.h"
nit: Forward-declare base::Timer and net::StreamListenSocket and move these
includes to the .cc file.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:46: void SetTimerForTesting(base::Timer*);
use scoped_ptr<> to pass ownership here. Also the parameter must be named.

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
File remoting/webapp/gnubby_auth_handler.js (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: var requestData = window.atob(/**
@type {string} */ base64Data);
move @type above base64Data declaration

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: var requestData = window.atob(/**
@type {string} */ base64Data);
The host encodes the data in base64 and here the client reencodes it to an array
of numbers. Is it just to reduce size of the message on the wire, or are the
other reasons the host can't send the message as array of numbers?

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:58: console.error('gnubby callback
failed: ', /** @type {*} */ (err));
Not sure why it may fail here? If we fail to reencode message doesn't it mean
that gnubbyd send a bogus message?

[psj, 2014-03-21 21:30:45]

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:262:
GnubbyAuthHandlerPosix::GetActiveSocket(base::DictionaryValue* dictionary) {
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> Maybe GetSocketForMessage()? Also rename dictionary->message?

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:281: LOG(ERROR) << "Request timed
out";
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> HOST_LOG please. Also update the message to indicate that it's Gnubby-related.
> Though maybe we don't need this message at all.

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.cc (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:37: 
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> Add DCHECK(IsRequestComplete() && !IsRequestTooLarge())?

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:39: data_out->assign(request_data_.data() + 4,
request_data_.size() - 4);
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> define a const for 4, e.g. kRequestSizeBytes

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:39: data_out->assign(request_data_.data() + 4,
request_data_.size() - 4);
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> You can replace the last parameter with request_data_.end(), then you wouldn't
> need '-4'.

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:46: if (request_data_.size() < 4)
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> define a const for 4, e.g. kRequestSizeBytes

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:70: socket_->Send(kSshError, sizeof(kSshError) /
sizeof(*kSshError));
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> Use arraysize() from base/macros.h

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:83: return ((request_data_[0] & 255) << 24) +
((request_data_[1] & 255) << 16) +
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> DCHECK that there are at least 4 bytes in |request_data_|

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:100: void GnubbySocket::ResetTimer() {
timer_->Reset(); }
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> move the body to a separate line

Done. clang_format put it on one line...

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.h (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:14: #include "net/socket/stream_listen_socket.h"
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> nit: Forward-declare base::Timer and net::StreamListenSocket and move these
> includes to the .cc file.

They are both used in scoped_ptr, so the full class definition is needed

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:46: void SetTimerForTesting(base::Timer*);
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> use scoped_ptr<> to pass ownership here. Also the parameter must be named. 

Done and done.

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
File remoting/webapp/gnubby_auth_handler.js (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: var requestData = window.atob(/**
@type {string} */ base64Data);
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> The host encodes the data in base64 and here the client reencodes it to an
array
> of numbers. Is it just to reduce size of the message on the wire, or are the
> other reasons the host can't send the message as array of numbers?

Done.

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: var requestData = window.atob(/**
@type {string} */ base64Data);
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> move @type above base64Data declaration

The compiler can't figure out the type when it is there, but that really doesn't
matter anymore...

https://codereview.chromium.org/205493005/diff/10012/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:58: console.error('gnubby callback
failed: ', /** @type {*} */ (err));
On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> Not sure why it may fail here? If we fail to reencode message doesn't it mean
> that gnubbyd send a bogus message?

It will fail if response doesn't contain data, or if data isn't an array. This
would happen if there is an old version of gnubbyd is installed

[Sergey Ulanov, 2014-03-21 22:53:16]

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.h (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:14: #include "net/socket/stream_listen_socket.h"
On 2014/03/21 21:30:45, psj wrote:
> On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> > nit: Forward-declare base::Timer and net::StreamListenSocket and move these
> > includes to the .cc file.
> 
> They are both used in scoped_ptr, so the full class definition is needed

It still a pointer so you shouldn't need include. If it doesn't compile without
this include then it's probably because you don't have explicit destructor (see
my next comment). In that case the compiled tries to create an implicit
destructor when compiling this header.

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:23: 
declare destructor here and define in .cc file

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.cc (right):

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:75: socket_->Send(kSshError,
arraysize(kSshError));
Can this invoke SendResponse?

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:91: ((request_data_[2] & 255) << 8) +
(request_data_[3] & 255) + 4;
+kRequestSizeBytes at the end

[psj, 2014-03-21 23:24:17]

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.h (right):

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:14: #include "net/socket/stream_listen_socket.h"
On 2014/03/21 22:53:16, Sergey Ulanov wrote:
> On 2014/03/21 21:30:45, psj wrote:
> > On 2014/03/21 02:09:37, Sergey Ulanov wrote:
> > > nit: Forward-declare base::Timer and net::StreamListenSocket and move
these
> > > includes to the .cc file.
> > 
> > They are both used in scoped_ptr, so the full class definition is needed
> 
> It still a pointer so you shouldn't need include. If it doesn't compile
without
> this include then it's probably because you don't have explicit destructor
(see
> my next comment). In that case the compiled tries to create an implicit
> destructor when compiling this header.

Ah, OK. It seemed weird that a forward reference wouldn't work but I didn't
realize it was due to the lack of a destructor

https://codereview.chromium.org/205493005/diff/10012/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.h:23: 
On 2014/03/21 22:53:16, Sergey Ulanov wrote:
> declare destructor here and define in .cc file

Done.

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
File remoting/host/gnubby_socket.cc (right):

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:75: socket_->Send(kSshError,
arraysize(kSshError));
On 2014/03/21 22:53:16, Sergey Ulanov wrote:
> Can this invoke SendResponse?

Done.

https://codereview.chromium.org/205493005/diff/30001/remoting/host/gnubby_soc...
remoting/host/gnubby_socket.cc:91: ((request_data_[2] & 255) << 8) +
(request_data_[3] & 255) + 4;
On 2014/03/21 22:53:16, Sergey Ulanov wrote:
> +kRequestSizeBytes at the end

Arggggg, I was sure I had gotten all of the 4's...

[Sergey Ulanov, 2014-03-22 00:45:56]

LGTM after my comments are addressed.

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:79: if (bytes->GetInteger(i, &value))
{
Suggest formatting as follows:
 if (!bytes->GetInteger(i, &value))
   return false;
 out->push_back(static_cast<char>(value));

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:173: if (!bytes) {
don't need this check.

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:300: if (iter !=
active_sockets_.end()) {
I think it's better to move this condition into RequestTimedOut() - that's the
only place where SendErrorAndCloseActiveSocket() is called with
iter==active_sockets_.end()

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
File remoting/webapp/gnubby_auth_handler.js (right):

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: {'type': 'auth-agent@openssh.com',
nit: space before 'type'

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:54: 'data': getArrayAttr(response,
'data')});
nit: space before }

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:58: 'connectionId': connectionId});
nit: move type field to match the formatting in line 52. Also add space before }

[psj, 2014-03-22 01:30:19]

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
File remoting/host/gnubby_auth_handler_posix.cc (right):

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:79: if (bytes->GetInteger(i, &value))
{
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> Suggest formatting as follows:
>  if (!bytes->GetInteger(i, &value))
>    return false;
>  out->push_back(static_cast<char>(value));

Done.

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:173: if (!bytes) {
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> don't need this check.

Done.

https://codereview.chromium.org/205493005/diff/50001/remoting/host/gnubby_aut...
remoting/host/gnubby_auth_handler_posix.cc:300: if (iter !=
active_sockets_.end()) {
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> I think it's better to move this condition into RequestTimedOut() - that's the
> only place where SendErrorAndCloseActiveSocket() is called with
> iter==active_sockets_.end()

Done.

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
File remoting/webapp/gnubby_auth_handler.js (right):

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:34: {'type': 'auth-agent@openssh.com',
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> nit: space before 'type'

Hmm, I mis-remembered the style guide here - the brace always goes on the
previous line for a multiline initialization.

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:54: 'data': getArrayAttr(response,
'data')});
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> nit: space before }

Similar to the opening brace, the multiline initializer closing brace goes on
the following line

https://codereview.chromium.org/205493005/diff/50001/remoting/webapp/gnubby_a...
remoting/webapp/gnubby_auth_handler.js:58: 'connectionId': connectionId});
On 2014/03/22 00:45:57, Sergey Ulanov wrote:
> nit: move type field to match the formatting in line 52. Also add space before
}

Done.

[Sergey Ulanov, 2014-03-22 01:54:52]

lgtm

[psj, 2014-03-22 02:54:52]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-03-22 02:55:01]

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

[commit-bot: I haz the power, 2014-03-22 02:56:33]

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

[commit-bot: I haz the power, 2014-03-22 02:56:33]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_clang_dbg

[psj, 2014-03-22 05:57:34]

The CQ bit was checked by psj@chromium.org

[commit-bot: I haz the power, 2014-03-22 05:57:42]

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

[commit-bot: I haz the power, 2014-03-24 02:33:45]

Change committed as 258844