Use ChannelMojo for remote security key channels.

Use ChannelMojo for remote security key channels.

Mojo does not support disconnecting from one OS-level pipe and
reconnecting over another, so this changes remote security key
connections to all use the same well-known named pipe.

BUG=604282
Committed: https://crrev.com/b90d19c1d61697c297d7494636c1e0fcfbc59eb9
Cr-Commit-Position: refs/heads/master@{#430557}

[Sam McNally, 2016-11-03 06:43:19]

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

[commit-bot: I haz the power, 2016-11-03 06:43:41]

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

[Sam McNally, 2016-11-03 07:52:30]

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

[commit-bot: I haz the power, 2016-11-03 07:52:45]

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

[Sam McNally, 2016-11-03 08:42:28]

Patchset #1 (id:1) has been deleted

[Sam McNally, 2016-11-03 08:42:31]

Patchset #1 (id:10001) has been deleted

[Sam McNally, 2016-11-03 08:47:42]

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

[commit-bot: I haz the power, 2016-11-03 09:29:26]

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

[commit-bot: I haz the power, 2016-11-03 09:29:27]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[Sam McNally, 2016-11-03 10:16:04]

Patchset #2 (id:50001) has been deleted

[Sam McNally, 2016-11-03 10:16:14]

Patchset #1 (id:30001) has been deleted

[Sam McNally, 2016-11-03 10:16:25]

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

[commit-bot: I haz the power, 2016-11-03 10:16:36]

Dry run: 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-11-03 12:17:12]

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

[commit-bot: I haz the power, 2016-11-03 12:17:13]

Dry run: Try jobs failed on following builders:
  linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_TIMED_OUT,
no build URL)

[joedow, 2016-11-03 22:25:19]

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/BUILD.gn (left):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/BUILD.gn:100: "fake_ipc_security_key_auth_handler.h",
Should these files be deleted in this CL?  I don't think they are referenced by
any other build files.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_win_unittest.cc
(left):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_win_unittest.cc:535:
TEST_F(SecurityKeyAuthHandlerWinTest, IpcConnectionFailsFromInvalidSession) {
We still want this to work (i.e. Ipc connections from the wrong session should
be rejected).  Why was this test removed?

If the functionality is preserved and has just been moved, then the member and
function which tracks |desktop_sessions_id_| should also be removed.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:31: bool
SecurityKeyIpcClient::WaitForSecurityKeyIpcServerChannel() {
Since this method doesn't wait anymore, it should be renamed from
WaitForSecurityKeyIpcServerChannel() to CheckForSecurityKeyIpcServerChannel().

Also, I think removing the wait should be OK but we should do some additional
testing to make sure.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:35: channel_handle_ =
mojo::edk::CreateClientHandle(named_channel_handle_);
nit: Use braces for single line conditions here, and elsewhere in the CL.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client_unittest.cc (right):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client_unittest.cc:17: #include
"remoting/host/security_key/security_key_ipc_constants.h"
Remove this header since |fake_security_key_auth_handler_| was removed...also
delete the files since they aren't used.

[Sam McNally, 2016-11-04 02:27:28]

Patchset #2 (id:90001) has been deleted

[Sam McNally, 2016-11-04 02:28:33]

Patchset #3 (id:130001) has been deleted

[Sam McNally, 2016-11-04 02:28:41]

Patchset #2 (id:110001) has been deleted

[Sam McNally, 2016-11-04 02:51:09]

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/BUILD.gn (left):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/BUILD.gn:100: "fake_ipc_security_key_auth_handler.h",
On 2016/11/03 22:25:18, joedow wrote:
> Should these files be deleted in this CL?  I don't think they are referenced
by
> any other build files.

Done.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_auth_handler_win_unittest.cc
(left):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_auth_handler_win_unittest.cc:535:
TEST_F(SecurityKeyAuthHandlerWinTest, IpcConnectionFailsFromInvalidSession) {
On 2016/11/03 22:25:18, joedow wrote:
> We still want this to work (i.e. Ipc connections from the wrong session should
> be rejected).  Why was this test removed?

SecurityKeyAuthHandlerWin no longer receives initial connections.
SecurityKeyIpcServerImpl has the same check for its connections and that's still
tested.
> 
> If the functionality is preserved and has just been moved, then the member and
> function which tracks |desktop_sessions_id_| should also be removed.

Done.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client.cc (right):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:31: bool
SecurityKeyIpcClient::WaitForSecurityKeyIpcServerChannel() {
On 2016/11/03 22:25:18, joedow wrote:
> Since this method doesn't wait anymore, it should be renamed from
> WaitForSecurityKeyIpcServerChannel() to CheckForSecurityKeyIpcServerChannel().
> 
> Also, I think removing the wait should be OK but we should do some additional
> testing to make sure. 

On Windows, CreateClientHandle calls WaitNamedPipe to wait for the pipe to be
ready. This can wait up to the timeout (which defaults to 5s) if a pipe with the
right name exists, but all instances with that name are busy, or fail
immediately if no pipes with the right name exist.

The former could occur if a client connects but doesn't establish the
IPC::Channel over that connection (until the server times out and recreates the
pipe). It's possible that if a client disconnects before the channel for the
next connection is created, the old channel could be closed on the IO thread,
leaving a window of no pipe with the right name existing.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client.cc:35: channel_handle_ =
mojo::edk::CreateClientHandle(named_channel_handle_);
On 2016/11/03 22:25:18, joedow wrote:
> nit: Use braces for single line conditions here, and elsewhere in the CL.

Done.

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client_unittest.cc (right):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client_unittest.cc:17: #include
"remoting/host/security_key/security_key_ipc_constants.h"
On 2016/11/03 22:25:18, joedow wrote:
> Remove this header since |fake_security_key_auth_handler_| was removed...also
> delete the files since they aren't used.

I'm not sure what you mean. The fake auth handler include is gone.

[Sam McNally, 2016-11-04 02:51:14]

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

[commit-bot: I haz the power, 2016-11-04 02:51:47]

Dry run: 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-11-04 05:34:02]

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

[commit-bot: I haz the power, 2016-11-04 05:34:03]

Dry run: This issue passed the CQ dry run.

[joedow, 2016-11-04 15:19:04]

lgtm

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
File remoting/host/security_key/security_key_ipc_client_unittest.cc (right):

https://codereview.chromium.org/2478443002/diff/70001/remoting/host/security_...
remoting/host/security_key/security_key_ipc_client_unittest.cc:17: #include
"remoting/host/security_key/security_key_ipc_constants.h"
On 2016/11/04 02:51:09, Sam McNally wrote:
> On 2016/11/03 22:25:18, joedow wrote:
> > Remove this header since |fake_security_key_auth_handler_| was
removed...also
> > delete the files since they aren't used.
> 
> I'm not sure what you mean. The fake auth handler include is gone.

I noticed the files hadn't been deleted in that patch set and I must have
misread the header name, sorry about that :P

[Sam McNally, 2016-11-06 22:24:42]

sammc@chromium.org changed reviewers:
+ dcheng@chromium.org, rockot@chromium.org

[Sam McNally, 2016-11-06 22:24:43]

+rockot for DEPS

+dcheng for remoting/host/chromoting_messages.h

[Ken Rockot(use gerrit already), 2016-11-08 00:36:41]

lgtm

[dcheng, 2016-11-08 07:54:04]

mojo lgtm with some optional nits

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
File remoting/host/security_key/fake_security_key_ipc_server.cc (right):

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
remoting/host/security_key/fake_security_key_ipc_server.cc:118: new
FakeSecurityKeyIpcServer(
Optional nit: consider auto fake_ipc_server = base::MakeUnique<...>(...);

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
File remoting/host/security_key/security_key_auth_handler_win.cc (right):

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
remoting/host/security_key/security_key_auth_handler_win.cc:206:
base::TimeDelta::FromSeconds(kSecurityKeyRequestTimeoutSeconds));
I know this wasn't introduced by this CL, but consider changing the constant to:

constexpr base::TimeDelta kSecurityKeyRequestTimeout =
    base::TimeDelta::FromSeconds(60);

[Sam McNally, 2016-11-08 08:34:06]

Patchset #4 (id:190001) has been deleted

[Sam McNally, 2016-11-08 08:37:22]

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
File remoting/host/security_key/fake_security_key_ipc_server.cc (right):

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
remoting/host/security_key/fake_security_key_ipc_server.cc:118: new
FakeSecurityKeyIpcServer(
On 2016/11/08 07:54:04, dcheng wrote:
> Optional nit: consider auto fake_ipc_server = base::MakeUnique<...>(...);

Done.

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
File remoting/host/security_key/security_key_auth_handler_win.cc (right):

https://codereview.chromium.org/2478443002/diff/170001/remoting/host/security...
remoting/host/security_key/security_key_auth_handler_win.cc:206:
base::TimeDelta::FromSeconds(kSecurityKeyRequestTimeoutSeconds));
On 2016/11/08 07:54:04, dcheng wrote:
> I know this wasn't introduced by this CL, but consider changing the constant
to:
> 
> constexpr base::TimeDelta kSecurityKeyRequestTimeout =
>     base::TimeDelta::FromSeconds(60);

Done.

[Sam McNally, 2016-11-08 08:37:37]

The CQ bit was checked by sammc@chromium.org

[Sam McNally, 2016-11-08 08:37:38]

The patchset sent to the CQ was uploaded after l-g-t-m from rockot@chromium.org,
dcheng@chromium.org, joedow@chromium.org
Link to the patchset: https://codereview.chromium.org/2478443002/#ps210001
(title: " ")

[commit-bot: I haz the power, 2016-11-08 08:37:50]

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-11-08 09:44:37]

Committed patchset #4 (id:210001)

[commit-bot: I haz the power, 2016-11-08 09:47:08]

Description was changed from

==========
Use ChannelMojo for remote security key channels.

Mojo does not support disconnecting from one OS-level pipe and
reconnecting over another, so this changes remote security key
connections to all use the same well-known named pipe.

BUG=604282
==========

to

==========
Use ChannelMojo for remote security key channels.

Mojo does not support disconnecting from one OS-level pipe and
reconnecting over another, so this changes remote security key
connections to all use the same well-known named pipe.

BUG=604282

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

[commit-bot: I haz the power, 2016-11-08 09:47:10]

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