Update GnubbyAuthHandler to use the current session ID

Update GnubbyAuthHandler to use the current session ID

This CL plumbs the SessionID value through to the Security Key HostExtension.
It also updates the logic in the method which handles SK IPC channel connection
which prevents requests being made from sessions which we are not remoted.

BUG=591746
Committed: https://crrev.com/76487d64faec882985665a1cb6b838b7b12b4150
Cr-Commit-Position: refs/heads/master@{#403045}

[joedow, 2016-06-27 19:01:35]

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

[joedow, 2016-06-27 19:01:35]

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

[joedow, 2016-06-27 19:01:36]

This is the last of the CLs to use the Windows session ID to restrict access to
the security key request IPC Channel.

PTAL!

Thanks,
Joe

[Sergey Ulanov, 2016-06-27 23:51:58]

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:41: const SessionIdCallback&
session_id_callback);
Why does the callback need to be passed to Create() given that you have
SetSessionIdCallback()?
Also do we need a callback at all? I.e. why SetSessionId(id) is not enough?

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler_linux_unittest.cc (right):

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler_linux_unittest.cc:77: uint32_t
GetSessionId() const { return UINT32_MAX; }
can be a static function.

[joedow, 2016-06-28 01:27:34]

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:41: const SessionIdCallback&
session_id_callback);
On 2016/06/27 23:51:58, Sergey Ulanov wrote:
> Why does the callback need to be passed to Create() given that you have
> SetSessionIdCallback()?
> Also do we need a callback at all? I.e. why SetSessionId(id) is not enough?

The session ID can change at any time (e.g. when a user signs into a Windows
session when in curtain mode, we will switch from the RD session into the logged
on user session).

I think that is a fair point about the callback being set in Create and in a
method.  I'll see if I can make this a bit cleaner.

[Sergey Ulanov, 2016-06-28 18:08:03]

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:41: const SessionIdCallback&
session_id_callback);
On 2016/06/28 01:27:34, joedow wrote:
> On 2016/06/27 23:51:58, Sergey Ulanov wrote:
> > Why does the callback need to be passed to Create() given that you have
> > SetSessionIdCallback()?
> > Also do we need a callback at all? I.e. why SetSessionId(id) is not enough?
> 
> The session ID can change at any time (e.g. when a user signs into a Windows
> session when in curtain mode, we will switch from the RD session into the
logged
> on user session).

Even if session ID may change it doesn't mean we need a callback here. With
SetSessionId() instead of the callback the handler implementation will need to
be notified whenever session-id changes by calling SetSessionId(). And if I
understand correctly the host knows when session id can change. But I'm not sure
which approach is simpler here.

> 
> I think that is a fair point about the callback being set in Create and in a
> method.  I'll see if I can make this a bit cleaner.

[joedow, 2016-06-28 21:36:22]

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/100001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:41: const SessionIdCallback&
session_id_callback);
On 2016/06/28 18:08:03, Sergey Ulanov wrote:
> On 2016/06/28 01:27:34, joedow wrote:
> > On 2016/06/27 23:51:58, Sergey Ulanov wrote:
> > > Why does the callback need to be passed to Create() given that you have
> > > SetSessionIdCallback()?
> > > Also do we need a callback at all? I.e. why SetSessionId(id) is not
enough?
> > 
> > The session ID can change at any time (e.g. when a user signs into a Windows
> > session when in curtain mode, we will switch from the RD session into the
> logged
> > on user session).
> 
> Even if session ID may change it doesn't mean we need a callback here. With
> SetSessionId() instead of the callback the handler implementation will need to
> be notified whenever session-id changes by calling SetSessionId(). And if I
> understand correctly the host knows when session id can change. But I'm not
sure
> which approach is simpler here.
> 
> > 
> > I think that is a fair point about the callback being set in Create and in a
> > method.  I'll see if I can make this a bit cleaner.
> 

The host definitely knows when the session id is changing, the complexity comes
from *which* object in the host knows.  AFAICT the DesktopSessionConnector is
the object which handles messages from the Daemon to attach/detach different
desktops.  This interface is implemented by the IpcDesktopEnvironmentFactory. 
The ClientSession (which owns the HostExtensionManager and HostExtensions) has
no knowledge of these events.  Plumbing the events to the ClientSession or
updating it when the session changes feels like the wrong approach. 

The session id is a property that may never be queried/needed unless security
key functionality is enabled and an attempt to connect to the IPC channel is
made so I feel like a lazy/JIT request to get the current id if needed is the
most efficient.

The main problem in this CL is the amount of churn needed due to the callback. 
If additional session details are needed in the future, then I think there is a
valid concern that another round of changes like this would be required.

I should be able to update the design without relying on the callback which
should make this cleaner and easier to maintain later on.

[joedow, 2016-06-29 16:07:38]

Removed the SessionId callback and now pass the ClientSessionDetails interface*
to the GnubbyAuthHandler.

PTAL!

Thanks,
Joe

[Sergey Ulanov, 2016-06-29 22:36:46]

LGTM when my comments are addressed

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:33: // All invocations of the
callbacks are guaranteed to occur before the
rephrase this, now that there is only one callback and client_session_details.

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler_win.cc (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler_win.cc:307: uint32_t
GnubbyAuthHandlerWin::GetDesktopSessionId() const {
Do you really need this method? It's only one line and called in only one place

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/remote_security_key_ipc_server_impl.cc (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/remote_security_key_ipc_server_impl.cc:149:
base::ThreadTaskRunnerHandle::Get()->PostTask(
Should we also close the channel here? Otherwise there is a chance that
ChromotingRemoteSecurityKeyToNetworkMsg_Request will be executed before this
task.

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/remote_security_key_ipc_server_impl.cc:149:
base::ThreadTaskRunnerHandle::Get()->PostTask(
This duplicates the PostTask() call above. Structure this call to avoid
duplication?

bool close_connection = true;
if (!ProcessIdToSessionId()) {
  PLOG ...
} else if (peer_session_id != peer_session_id_) {
  LOG...
} else {
  close_connection = false;
}
if (close_connection) {
  PostTask
  return;
}

[joedow, 2016-06-30 00:10:08]

Thanks!

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler.h (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler.h:33: // All invocations of the
callbacks are guaranteed to occur before the
On 2016/06/29 22:36:46, Sergey Ulanov wrote:
> rephrase this, now that there is only one callback and client_session_details.

Done.

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/gnubby_auth_handler_win.cc (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/gnubby_auth_handler_win.cc:307: uint32_t
GnubbyAuthHandlerWin::GetDesktopSessionId() const {
On 2016/06/29 22:36:46, Sergey Ulanov wrote:
> Do you really need this method? It's only one line and called in only one
place

Done.

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
File remoting/host/security_key/remote_security_key_ipc_server_impl.cc (right):

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/remote_security_key_ipc_server_impl.cc:149:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/06/29 22:36:46, Sergey Ulanov wrote:
> This duplicates the PostTask() call above. Structure this call to avoid
> duplication?
> 
> bool close_connection = true;
> if (!ProcessIdToSessionId()) {
>   PLOG ...
> } else if (peer_session_id != peer_session_id_) {
>   LOG...
> } else {
>   close_connection = false;
> }
> if (close_connection) {
>   PostTask
>   return;
> }

Done.

https://codereview.chromium.org/2085353004/diff/140001/remoting/host/security...
remoting/host/security_key/remote_security_key_ipc_server_impl.cc:149:
base::ThreadTaskRunnerHandle::Get()->PostTask(
On 2016/06/29 22:36:46, Sergey Ulanov wrote:
> Should we also close the channel here? Otherwise there is a chance that
> ChromotingRemoteSecurityKeyToNetworkMsg_Request will be executed before this
> task.

I think that is a good idea, I'll add a member bool to track whether there is a
pending close operation and ignore IPC messages received if that is the case.

[joedow, 2016-06-30 00:10:34]

The CQ bit was checked by joedow@chromium.org

[joedow, 2016-06-30 00:10:35]

The patchset sent to the CQ was uploaded after l-g-t-m from sergeyu@chromium.org
Link to the patchset: https://codereview.chromium.org/2085353004/#ps180001
(title: "Updating a comment")

[commit-bot: I haz the power, 2016-06-30 00:11:58]

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-06-30 00:58:36]

Committed patchset #10 (id:180001)

[commit-bot: I haz the power, 2016-06-30 00:58:49]

CQ bit was unchecked.

[commit-bot: I haz the power, 2016-06-30 01:00:30]

Description was changed from

==========
Update GnubbyAuthHandler to use the current session ID

This CL plumbs the SessionID value through to the Security Key HostExtension.
It also updates the logic in the method which handles SK IPC channel connection
which prevents requests being made from sessions which we are not remoted.

BUG=591746
==========

to

==========
Update GnubbyAuthHandler to use the current session ID

This CL plumbs the SessionID value through to the Security Key HostExtension.
It also updates the logic in the method which handles SK IPC channel connection
which prevents requests being made from sessions which we are not remoted.

BUG=591746

Committed: https://crrev.com/76487d64faec882985665a1cb6b838b7b12b4150
Cr-Commit-Position: refs/heads/master@{#403045}
==========

[commit-bot: I haz the power, 2016-06-30 01:00:32]

Patchset 10 (id:??) landed as
https://crrev.com/76487d64faec882985665a1cb6b838b7b12b4150
Cr-Commit-Position: refs/heads/master@{#403045}