[Windows][Host] SecurityKey requests from outside remoted session are not handled correctly |
||||||
Issue descriptionI noticed a few days ago that security key requests were not being handled correctly if they were issued from a Windows session which was not the remoted session. This was working as of M55 so I investigated and found that this was regressed in the following CL: https://codereview.chromium.org/2478443002/ The regression occurred because the remote_security_key binary used to receive an IPC channel error if it was in the wrong session (which would trigger it to pass that data on) however now the IPC channel was connecting and then returning an error. This is because the old design would send a new, client specific IPC channel to the remote_security_key process to use, but now that Mojo is being used, the channel always connects first. Need to work on a fix for the new architecture as we want this to work reliably.
,
Dec 20 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58 commit afdd0e9701d38723cb5ebae11e4e26a7c19e3d58 Author: joedow <joedow@chromium.org> Date: Tue Dec 20 03:01:34 2016 Handle Security Key requests from outside the remoted session correctly The recent Mojo conversion changed the communication pattern between the IPC Server and Client classes used for remote security key message forwarding. This change has caused a regression in the scenario where the security key request originates from outside the remoted session. The desired behavior is to pass a message to the local SK handler so it can route/handle the request. Due to the change in the following CL, we would instead report that the remote client would handle it, then return a failure when they sent the request: https://codereview.chromium.org/2478443002/ The reason for this change is that the session detection logic was wrapped in the OnChannelConnected() method. Because you cannot close an IPC channel from within that method, we would defer the close operation. With the old architecture, the client would be waiting for a specific IPC message to complete the connection (and signal success) but instead would receive the deferred close/error message and pass on the right message to the local handler. This behavior was changed such that the client would now signal success in its own 'OnChannelConnected()' and then receive the deferred error. In order to make this scenario more robust, I have added specific success and failure IPC messages to be passed to the client from the server and will use that when establishing the connection. I've also separated out the concept of an invalid session vs. and actual connection error so the IPC client class can provide more accurate details to the local handler. BUG= 674236 Review-Url: https://codereview.chromium.org/2575963002 Cr-Commit-Position: refs/heads/master@{#439685} [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/chromoting_messages.h [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/fake_security_key_ipc_client.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/fake_security_key_ipc_client.h [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/fake_security_key_ipc_server.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/fake_security_key_ipc_server.h [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_auth_handler_win.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_ipc_client.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_ipc_client.h [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_ipc_client_unittest.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_ipc_server_impl.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_ipc_server_unittest.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_message_handler.cc [modify] https://crrev.com/afdd0e9701d38723cb5ebae11e4e26a7c19e3d58/remoting/host/security_key/security_key_message_handler.h
,
Dec 20 2016
,
Dec 21 2016
Your change meets the bar and is auto-approved for M56 (branch: 2924)
,
Dec 21 2016
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/b9c016474def2fac457b08c42b63a4d09cf5c9f8 commit b9c016474def2fac457b08c42b63a4d09cf5c9f8 Author: Joe Downing <joedow@google.com> Date: Wed Dec 21 16:20:49 2016 Handle Security Key requests from outside the remoted session correctly The recent Mojo conversion changed the communication pattern between the IPC Server and Client classes used for remote security key message forwarding. This change has caused a regression in the scenario where the security key request originates from outside the remoted session. The desired behavior is to pass a message to the local SK handler so it can route/handle the request. Due to the change in the following CL, we would instead report that the remote client would handle it, then return a failure when they sent the request: https://codereview.chromium.org/2478443002/ The reason for this change is that the session detection logic was wrapped in the OnChannelConnected() method. Because you cannot close an IPC channel from within that method, we would defer the close operation. With the old architecture, the client would be waiting for a specific IPC message to complete the connection (and signal success) but instead would receive the deferred close/error message and pass on the right message to the local handler. This behavior was changed such that the client would now signal success in its own 'OnChannelConnected()' and then receive the deferred error. In order to make this scenario more robust, I have added specific success and failure IPC messages to be passed to the client from the server and will use that when establishing the connection. I've also separated out the concept of an invalid session vs. and actual connection error so the IPC client class can provide more accurate details to the local handler. BUG= 674236 Review-Url: https://codereview.chromium.org/2575963002 Cr-Commit-Position: refs/heads/master@{#439685} (cherry picked from commit afdd0e9701d38723cb5ebae11e4e26a7c19e3d58) Review-Url: https://codereview.chromium.org/2596543003 . Cr-Commit-Position: refs/branch-heads/2924@{#577} Cr-Branched-From: 3a87aecc31cd1ffe751dd72c04e5a96a1fc8108a-refs/heads/master@{#433059} [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/chromoting_messages.h [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/fake_security_key_ipc_client.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/fake_security_key_ipc_client.h [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/fake_security_key_ipc_server.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/fake_security_key_ipc_server.h [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_auth_handler_win.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_ipc_client.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_ipc_client.h [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_ipc_client_unittest.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_ipc_server_impl.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_ipc_server_unittest.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_message_handler.cc [modify] https://crrev.com/b9c016474def2fac457b08c42b63a4d09cf5c9f8/remoting/host/security_key/security_key_message_handler.h
,
Dec 21 2016
Change has landed in M56 and M57. I'm not sure how frequently the M56 builds are being generated but the next one should have this change. One way to verify this fix is: - log on to the local session on a windows machine with curtain mode enabled - Connect to the windows machine from a second machine, but stay at the logon screen - Run a security key enabled scenario, you should see the local SK flash instead of the remote one (from the second machine). - After signing in at the logon screen, the SK requests should be forwarded to the second machine.
,
Dec 28 2016
Checking on the latest M56 and M57 Windows Hosts, the first part works, the second part does not. When on the Windows logon screen, the sk flashes locally as expected. Once logged in, sk prompts are not given when prompted on the Host. Checked with 57.0.2964.0 and 56.0.2924.41
,
Jan 19 2017
|
||||||
►
Sign in to add a comment |
||||||
Comment 1 by joedow@chromium.org
, Dec 14 2016