Forward ssh-agent data between ssh-agent and RDP #4122

ben-cohen · 2017-09-07T19:42:44Z

Add the sshagent plugin to forward the ssh-agent protocol over an RDP
dynamic virtual channel, just as the normal ssh-agent forwards it over
an SSH channel. Add the "/ssh-agent" command line option to enable it.
Usage:

Run FreeRDP with the ssh-agent plugin enabled:

xfreerdp /ssh-agent ...

In the remote desktop session run xrdp-ssh-agent and evaluate the output
in the shell as for ssh-agent to set the required environment variables
(specifically $SSH_AUTH_SOCK):

eval "$(xrdp-ssh-agent -s)"

This is the same as for the normal ssh-agent. You would typically do
this in your Xsession or /etc/xrdp/startwm.sh.

Limitations:

Error checking and handling could be improved.
This is only tested on Linux and will only work on systems where
clients talk to the ssh-agent via Unix domain sockets. It won't
currently work on Windows but it could be ported.

freerdp-bot · 2017-09-07T19:42:47Z

Can one of the admins verify this patch?

ben-cohen · 2017-09-07T20:08:20Z

Please see neutrinolabs/xrdp#868 for more details.

hardening

There's definitely all the copyright statements to adjust. Plus some of my remarks.
Nice job, thanks for the contribution.

hardening · 2017-09-07T20:43:37Z

channels/sshagent/CMakeLists.txt

+# FreeRDP cmake build script
+#
+# Copyright 2012 Marc-Andre Moreau <marcandre.moreau@gmail.com>
+#


I guess it's not the right copyright...

I basically copied the file from echo so I can't claim much copyright! I've added myself, anyway :)

hardening · 2017-09-07T20:45:52Z

channels/sshagent/client/sshagent_main.c

+        int rc = connect(agent_fd, (struct sockaddr*)&addr, sizeof(addr));
+        if (rc != 0)
+        {
+                WLog_ERR(TAG, "Can't connect to Unix domain socket \"%s\"!",


agent_fd is leaked here

... when connect() fails. Thanks - fixed.

hardening · 2017-09-07T20:47:22Z

channels/sshagent/client/sshagent_main.c

+	callback->channel_mgr = listener_callback->channel_mgr;
+	callback->channel = pChannel;
+
+	if (!(callback->thread


strange code style

hardening · 2017-09-07T20:49:56Z

channels/sshagent/client/sshagent_main.c

+                }
+                else if (bytes_read < 0)
+                {
+                        if (errno != EAGAIN


I may have missed something, but your socket is opened in a blocking way (and is not changed afterwards) so you will never get a EAGAIN or a EWOULDBLOCK, and the same applies for writes.

I've removed them.

hardening · 2017-09-07T20:52:22Z

channels/sshagent/CMakeLists.txt

+	add_channel_client(${MODULE_PREFIX} ${CHANNEL_NAME})
+endif()
+
+if(WITH_SERVER_CHANNELS)


I don't see the server code...

hardening · 2017-09-07T20:54:45Z

@freerdp-bot can you test that first shot buddy ?

freerdp-bot · 2017-09-07T21:12:55Z

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/2261/

ben-cohen · 2017-09-08T19:11:40Z

@hardening For the server code, I wrote and tested it against xrdp (#867: xrdp-ssh-agent.c).

It is designed to be run as a command-line app that prints out the agent socket path, just as ssh-agent does.

The same file compiles with minimal changes - different include files - in FreeRDP's tree. I am struggling to compile the Linux FreeRDP server so I haven't been able to test it yet. I have pushed it anyway - please let me know of any problems.

akallabeth · 2017-09-11T11:40:10Z

@freerdp-bot test

freerdp-bot · 2017-09-11T12:17:28Z

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/2264/

akallabeth · 2017-11-06T15:56:19Z

@hardening Are your issues with this resolved?

hardening · 2017-11-06T22:33:44Z

include/freerdp/client/sshagent.h

+/**
+ * FreeRDP: A Remote Desktop Protocol Implementation
+ * Clipboard Virtual Channel Extension
+ *


Wrong comment, and copyright below are also wrong

hardening · 2017-11-06T22:34:22Z

channels/sshagent/server/sshagent_main.c

@@ -0,0 +1,401 @@
+/**
+ * xrdp: A Remote Desktop Protocol server.
+ *


wrong header here

hardening · 2017-11-06T22:34:40Z

channels/sshagent/client/sshagent_main.h

@@ -0,0 +1,42 @@
+/**
+ * FreeRDP: A Remote Desktop Protocol Implementation
+ * Echo Virtual Channel Extension


comment should be changed

hardening · 2017-11-06T22:35:54Z

You should really fix copyright statements and descriptions in file headers, apart from that that LGTM.

ben-cohen · 2017-11-09T21:56:39Z

I fixed the comments at the start of the files mentioned by @hardening and pushed that.

It says there is a conflict. Do you want me to force-push the branch rebased to fix that?

hardening · 2017-11-09T22:06:14Z

Yes please rebase, we've done quite a lot of merge recently

akallabeth · 2017-11-10T09:00:48Z

channels/sshagent/client/CMakeLists.txt

+# FreeRDP: A Remote Desktop Protocol Implementation
+# FreeRDP cmake build script
+#
+# Copyright 2012 Marc-Andre Moreau <marcandre.moreau@gmail.com>


Is this correct? I assume you did write this file or am I mistaken?

Yes: I simply copied channels/sshagent/client/CMakeLists.txt from channels/echo/client/CMakeLists.txt and replaced "echo" with "sshagent". So I left the existing copyright line in and added my own afterwards.

akallabeth · 2017-11-10T09:01:59Z

channels/sshagent/client/sshagent_main.h

+ * limitations under the License.
+ */
+
+#ifndef __SSHAGENT_MAIN_H


Please remove the __, that should not be used in application / library header files. Maybe add some prefix (FREERDP or something to avoid collisions)

akallabeth

LGTM except the minor issue pointed out.

Add the sshagent plugin to forward the ssh-agent protocol over an RDP dynamic virtual channel, just as the normal ssh-agent forwards it over an SSH channel. Add the "/ssh-agent" command line option to enable it. Usage: Run FreeRDP with the ssh-agent plugin enabled: xfreerdp /ssh-agent ... In the remote desktop session run xrdp-ssh-agent and evaluate the output in the shell as for ssh-agent to set the required environment variables (specifically $SSH_AUTH_SOCK): eval "$(xrdp-ssh-agent -s)" This is the same as for the normal ssh-agent. You would typically do this in your Xsession or /etc/xrdp/startwm.sh. Limitations: 1. Error checking and handling could be improved. 2. This is only tested on Linux and will only work on systems where clients talk to the ssh-agent via Unix domain sockets. It won't currently work on Windows but it could be ported.

1. In connect_to_sshagent() if connect() fails, the socket agent_fd is leaked. It needs to be closed before returning. 2. Fix copyright messages. 3. Make if statement with call to CreateThread() clearer to read.

Also fix copyright dates...

This is based on xrdpapi/xrdp-ssh-agent.c from xrdp PR FreeRDP#867.

akallabeth · 2017-11-13T08:35:59Z

@freerdp-bot test

freerdp-bot · 2017-11-13T08:55:01Z

Build finished.

freerdp-bot · 2017-11-13T08:55:02Z

Refer to this link for build results (access rights to CI server needed):
https://ci.freerdp.com//job/PullRequestTester/2360/

bmiklautz · 2017-11-13T14:26:47Z

@ben-cohen nice work! Thank you!

ben-cohen mentioned this pull request Sep 7, 2017

Forward SSH agent over RDP using virtual channel neutrinolabs/xrdp#868

Closed

hardening requested changes Sep 7, 2017

View reviewed changes

metalefty mentioned this pull request Sep 27, 2017

Forward ssh-agent data between ssh clients and RDP neutrinolabs/xrdp#867

Merged

hardening reviewed Nov 6, 2017

View reviewed changes

akallabeth reviewed Nov 10, 2017

View reviewed changes

akallabeth requested changes Nov 10, 2017

View reviewed changes

hardening approved these changes Nov 10, 2017

View reviewed changes

ben-cohen added 7 commits November 10, 2017 20:16

Fix socket fd leak and other changes

7e26221

1. In connect_to_sshagent() if connect() fails, the socket agent_fd is leaked. It needs to be closed before returning. 2. Fix copyright messages. 3. Make if statement with call to CreateThread() clearer to read.

Remove EAGAIN/EWOULDBLOCK from blocking read/write

6399308

Add rdpcontext so read thread can report channel error

c27541e

Also fix copyright dates...

Add server side plugin [UNTESTED]

badb701

This is based on xrdpapi/xrdp-ssh-agent.c from xrdp PR FreeRDP#867.

Fix comments at the start of these files and adjust copyrights

6093ec6

Remove underscores from define in sshagent_main.h

8d54945

ben-cohen force-pushed the sshagent branch from 37764f3 to 8d54945 Compare November 10, 2017 20:21

akallabeth approved these changes Nov 13, 2017

View reviewed changes

akallabeth merged commit ff59cf0 into FreeRDP:master Nov 13, 2017

ben-cohen deleted the sshagent branch November 13, 2017 09:27

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward ssh-agent data between ssh-agent and RDP #4122

Forward ssh-agent data between ssh-agent and RDP #4122

ben-cohen commented Sep 7, 2017

freerdp-bot commented Sep 7, 2017

ben-cohen commented Sep 7, 2017

hardening left a comment

hardening Sep 7, 2017

ben-cohen Sep 8, 2017

hardening Sep 7, 2017

ben-cohen Sep 8, 2017

hardening Sep 7, 2017

ben-cohen Sep 8, 2017

hardening Sep 7, 2017

ben-cohen Sep 8, 2017

hardening Sep 7, 2017

hardening commented Sep 7, 2017

freerdp-bot commented Sep 7, 2017

ben-cohen commented Sep 8, 2017

akallabeth commented Sep 11, 2017

freerdp-bot commented Sep 11, 2017

akallabeth commented Nov 6, 2017

hardening Nov 6, 2017

hardening Nov 6, 2017

hardening Nov 6, 2017

hardening commented Nov 6, 2017

ben-cohen commented Nov 9, 2017

hardening commented Nov 9, 2017

akallabeth Nov 10, 2017

ben-cohen Nov 10, 2017

akallabeth Nov 10, 2017 •

edited

ben-cohen Nov 10, 2017

akallabeth left a comment

akallabeth commented Nov 13, 2017

freerdp-bot commented Nov 13, 2017

freerdp-bot commented Nov 13, 2017

bmiklautz commented Nov 13, 2017

Forward ssh-agent data between ssh-agent and RDP #4122

Forward ssh-agent data between ssh-agent and RDP #4122

Conversation

ben-cohen commented Sep 7, 2017

freerdp-bot commented Sep 7, 2017

ben-cohen commented Sep 7, 2017

hardening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardening commented Sep 7, 2017

freerdp-bot commented Sep 7, 2017

ben-cohen commented Sep 8, 2017

akallabeth commented Sep 11, 2017

freerdp-bot commented Sep 11, 2017

akallabeth commented Nov 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hardening commented Nov 6, 2017

ben-cohen commented Nov 9, 2017

hardening commented Nov 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akallabeth Nov 10, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akallabeth left a comment

Choose a reason for hiding this comment

akallabeth commented Nov 13, 2017

freerdp-bot commented Nov 13, 2017

freerdp-bot commented Nov 13, 2017

bmiklautz commented Nov 13, 2017

akallabeth Nov 10, 2017 •

edited