Updating SecurityKeyAuthHandlerPosix socket lifetime management

Updating SecurityKeyAuthHandlerPosix socket lifetime management

This change fixes two problems:
1.) Security key responses would not be delivered if the read end of
    the socket was closed before the response was received.  This
    should be allowed since the write end was still open.
2.) An SSH error was always written when the socket was closed.  This
    was misleading when no error occrred and the socket was being
    closed for a valid reason.

The old behavior would close the socket as soon as a read returned 0
bytes (or an error).  The auth handler would read a request and forward
it to the remote machine, then immediately try to read another request.
If the code on the other end had closed its side of the connection, the
auth handler would receive EOF and close the socket.  It would also
write an error to the write end of the socket which was also wrong.

The simpler change is to only write an error if the request timed out or
if we encountered a socket read error.  Otherwise we just close our end
of the socket and the listener receives an EOF.

The bigger change is that we no longer queue up another socket read
operation until after we have received the response from the remote
machine.  The SecurityKeySocket is meant to receive and respond to one
request at a time (per its class comments), and this new behavior also
makes lifetime management of the socket less complex.

BUG=671041
Committed: https://crrev.com/cb5a2ea87efd87a693c43a79cd1bde92a91b78cd
Cr-Commit-Position: refs/heads/master@{#439675}

[joedow, 2016-12-19 20:22:21]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-19 20:23:05]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-12-19 20:50:25]

joedow@chromium.org changed reviewers:
+ sergeyu@chromium.org

[joedow, 2016-12-19 20:50:25]

PTAL!

[commit-bot: I haz the power, 2016-12-19 21:04:02]

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

[commit-bot: I haz the power, 2016-12-19 21:04:03]

Dry run: This issue passed the CQ dry run.

[Sergey Ulanov, 2016-12-19 23:19:18]

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_posix.cc (right):

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_posix.cc:205: HOST_LOG <<
"Sending client response to socket: " << connection_id;
Do we really need to log for every Gnubby connection?

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_posix.cc:280: HOST_LOG <<
"Closing socket: " << connection_id;
Log only in case of an error?

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_socket.cc (right):

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:49: request_data_.end());
If we get a request and some extra data at the end it would be returned here
(e.g. if the sender tries to send second request without waiting for the
response to the first one).

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:75: void
SecurityKeySocket::StartReadingRequest(
Maybe rename this to ReadRequest()? That would make it more consistent with
other async calls (e.g. net::Socket::Read()).

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:83: DoRead();
Do we need to check if there a complete request in |read_buffer_| before trying
to read again?

[joedow, 2016-12-20 00:25:45]

The CQ bit was checked by joedow@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2016-12-20 00:26:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[joedow, 2016-12-20 00:28:00]

Updated based on feedback, PTAL!

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_posix.cc (right):

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_posix.cc:205: HOST_LOG <<
"Sending client response to socket: " << connection_id;
On 2016/12/19 23:19:18, Sergey Ulanov wrote:
> Do we really need to log for every Gnubby connection?

There shouldn't be too much debug spew but I'll update it to a DLOG.

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_posix.cc:280: HOST_LOG <<
"Closing socket: " << connection_id;
On 2016/12/19 23:19:18, Sergey Ulanov wrote:
> Log only in case of an error?

Updating to DLOG

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
File remoting/host/security_key/security_key_socket.cc (right):

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:49: request_data_.end());
On 2016/12/19 23:19:18, Sergey Ulanov wrote:
> If we get a request and some extra data at the end it would be returned here
> (e.g. if the sender tries to send second request without waiting for the
> response to the first one).

That's true, but it would have happened with the previous implementation as
well.  It shouldn't be too hard to fix it (use the header bytes to define the
number of bytes to transfer and save the rest) but I'd prefer to do that in a
follow-up CL.  I'll add a TODO for now.

https://codereview.chromium.org/2589933002/diff/40001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:83: DoRead();
On 2016/12/19 23:19:18, Sergey Ulanov wrote:
> Do we need to check if there a complete request in |read_buffer_| before
trying
> to read again?

Right now we drain everything from the buffer (line 131/132) w/o honoring the
length of the request.  I do think this will be necessary if we add some logic
around the request length.

[Sergey Ulanov, 2016-12-20 00:54:23]

lgtm

https://codereview.chromium.org/2589933002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_socket.cc (right):

https://codereview.chromium.org/2589933002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:134: // determine the request
length and only read that amount from buffer.
I think it would be easier to keep the current reading logic and update the code
that handles compelte requests: when extra bytes are read they need to be kept
in the buffer and handled later.

[commit-bot: I haz the power, 2016-12-20 00:57:45]

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

[commit-bot: I haz the power, 2016-12-20 00:57:46]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-12-20 02:24:03]

Thanks!

https://codereview.chromium.org/2589933002/diff/60001/remoting/host/security_...
File remoting/host/security_key/security_key_socket.cc (right):

https://codereview.chromium.org/2589933002/diff/60001/remoting/host/security_...
remoting/host/security_key/security_key_socket.cc:134: // determine the request
length and only read that amount from buffer.
On 2016/12/20 00:54:23, Sergey Ulanov wrote:
> I think it would be easier to keep the current reading logic and update the
code
> that handles compelte requests: when extra bytes are read they need to be kept
> in the buffer and handled later.

That's reasonable.  I'll work on that tomorrow and get a CL out for it.

[joedow, 2016-12-20 02:24:11]

The CQ bit was checked by joedow@chromium.org

[commit-bot: I haz the power, 2016-12-20 02:24:37]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2016-12-20 02:28:55]

CQ is committing da patch.

Bot data: {"patchset_id": 60001, "attempt_start_ts": 1482200651059100,
"parent_rev": "f7a55ce13e98269a44154bd6662075fda2717318", "commit_rev":
"280ed91ae240e2e9a466c8144db22d75aa3db25f"}

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

Description was changed from

==========
Updating SecurityKeyAuthHandlerPosix socket lifetime management

This change fixes two problems:
1.) Security key responses would not be delivered if the read end of
    the socket was closed before the response was received.  This
    should be allowed since the write end was still open.
2.) An SSH error was always written when the socket was closed.  This
    was misleading when no error occrred and the socket was being
    closed for a valid reason.

The old behavior would close the socket as soon as a read returned 0
bytes (or an error).  The auth handler would read a request and forward
it to the remote machine, then immediately try to read another request.
If the code on the other end had closed its side of the connection, the
auth handler would receive EOF and close the socket.  It would also
write an error to the write end of the socket which was also wrong.

The simpler change is to only write an error if the request timed out or
if we encountered a socket read error.  Otherwise we just close our end
of the socket and the listener receives an EOF.

The bigger change is that we no longer queue up another socket read
operation until after we have received the response from the remote
machine.  The SecurityKeySocket is meant to receive and respond to one
request at a time (per its class comments), and this new behavior also
makes lifetime management of the socket less complex.

BUG=671041
==========

to

==========
Updating SecurityKeyAuthHandlerPosix socket lifetime management

This change fixes two problems:
1.) Security key responses would not be delivered if the read end of
    the socket was closed before the response was received.  This
    should be allowed since the write end was still open.
2.) An SSH error was always written when the socket was closed.  This
    was misleading when no error occrred and the socket was being
    closed for a valid reason.

The old behavior would close the socket as soon as a read returned 0
bytes (or an error).  The auth handler would read a request and forward
it to the remote machine, then immediately try to read another request.
If the code on the other end had closed its side of the connection, the
auth handler would receive EOF and close the socket.  It would also
write an error to the write end of the socket which was also wrong.

The simpler change is to only write an error if the request timed out or
if we encountered a socket read error.  Otherwise we just close our end
of the socket and the listener receives an EOF.

The bigger change is that we no longer queue up another socket read
operation until after we have received the response from the remote
machine.  The SecurityKeySocket is meant to receive and respond to one
request at a time (per its class comments), and this new behavior also
makes lifetime management of the socket less complex.

BUG=671041

Review-Url: https://codereview.chromium.org/2589933002
==========

[commit-bot: I haz the power, 2016-12-20 02:29:23]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2016-12-20 02:31:38]

Description was changed from

==========
Updating SecurityKeyAuthHandlerPosix socket lifetime management

This change fixes two problems:
1.) Security key responses would not be delivered if the read end of
    the socket was closed before the response was received.  This
    should be allowed since the write end was still open.
2.) An SSH error was always written when the socket was closed.  This
    was misleading when no error occrred and the socket was being
    closed for a valid reason.

The old behavior would close the socket as soon as a read returned 0
bytes (or an error).  The auth handler would read a request and forward
it to the remote machine, then immediately try to read another request.
If the code on the other end had closed its side of the connection, the
auth handler would receive EOF and close the socket.  It would also
write an error to the write end of the socket which was also wrong.

The simpler change is to only write an error if the request timed out or
if we encountered a socket read error.  Otherwise we just close our end
of the socket and the listener receives an EOF.

The bigger change is that we no longer queue up another socket read
operation until after we have received the response from the remote
machine.  The SecurityKeySocket is meant to receive and respond to one
request at a time (per its class comments), and this new behavior also
makes lifetime management of the socket less complex.

BUG=671041

Review-Url: https://codereview.chromium.org/2589933002
==========

to

==========
Updating SecurityKeyAuthHandlerPosix socket lifetime management

This change fixes two problems:
1.) Security key responses would not be delivered if the read end of
    the socket was closed before the response was received.  This
    should be allowed since the write end was still open.
2.) An SSH error was always written when the socket was closed.  This
    was misleading when no error occrred and the socket was being
    closed for a valid reason.

The old behavior would close the socket as soon as a read returned 0
bytes (or an error).  The auth handler would read a request and forward
it to the remote machine, then immediately try to read another request.
If the code on the other end had closed its side of the connection, the
auth handler would receive EOF and close the socket.  It would also
write an error to the write end of the socket which was also wrong.

The simpler change is to only write an error if the request timed out or
if we encountered a socket read error.  Otherwise we just close our end
of the socket and the listener receives an EOF.

The bigger change is that we no longer queue up another socket read
operation until after we have received the response from the remote
machine.  The SecurityKeySocket is meant to receive and respond to one
request at a time (per its class comments), and this new behavior also
makes lifetime management of the socket less complex.

BUG=671041

Committed: https://crrev.com/cb5a2ea87efd87a693c43a79cd1bde92a91b78cd
Cr-Commit-Position: refs/heads/master@{#439675}
==========

[commit-bot: I haz the power, 2016-12-20 02:31:41]

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