Add support for AF_UNIX #674

bilby91 · 2023-04-12T17:34:17Z

PR Summary

This pull requests adds support for AF_UNIX sockets.

The implementation modifies contrib/win32/win32compat/socketio.c socketio_acceptEx implementation so that it can handle sa_family == AF_UNIX. contrib/win32/win32compat/w32fd.c has also been modified in order to remove the path that uses fileio_afunix_socket.

The unix socket name is conditionally using C:\\tmp\\ssh-XXXXXXXXXX instead of /tmp/ssh-XXXXXXXXXX since it doesn't work correctly when using ssh in the windows host. I'm not sure if /tmp is the best directory to host the sock in Windows but I left it that way since unix users will expect the agent socks to live there. The caveat here is that you need to create the /tmp folder in Windows in order to properly work.

One known gap that the implementation is missing is conditionally using AF_UNIX. I know that a subset of Windows version support it but I'm not sure what is the best way to handle this. If I can get any guidance in terms of how to approach it I can test it and modify it.

PR Context

The motivation around working on this patch was to get support for SSH Agent Forwarding. With this patch applied, ssh-add -l will list the keys from the local system. I've also tested using git to clone repositories and it's successfully able to use the local system private keys through the agent.

I've also tested that WSL can successfully use the SSH Agent sock as long as the SSH_AUTH_SOCK is defined in the bash session.

This pull request could potentially solve the following issues:

PowerShell/Win32-OpenSSH#1461
PowerShell/Win32-OpenSSH#1761
PowerShell/Win32-OpenSSH#1462

bilby91 · 2023-04-13T14:18:44Z

@tgauth Would it be fine if we guard the different code paths with #ifdef HAVE_AFUNIX_H ?

tgauth · 2023-04-13T14:34:42Z

@tgauth Would it be fine if we guard the different code paths with #ifdef HAVE_AFUNIX_H ?

I don't see why not. If you haven't seen it yet, can add it to: https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/config.h.vs

Thanks for working on this!

bilby91 · 2023-04-13T14:49:48Z

@tgauth Thanks for the quick feedback! I added the HAVE_AFUNIX_H guard. Visual Studio doesn't seem to be picking up the #define statement from config.h.vs, is there anything else I need to run in order to make the change effective when compiling ? Want to make sure stuff is working properly with my tests.

tgauth · 2023-04-13T15:07:51Z

@tgauth Thanks for the quick feedback! I added the HAVE_AFUNIX_H guard. Visual Studio doesn't seem to be picking up the #define statement from config.h.vs, is there anything else I need to run in order to make the change effective when compiling ? Want to make sure stuff is working properly with my tests.

There's a config.h file in the root folder that gets created from config.h.vs during compilation of the config.vcxproj - can you confirm that's picking up the #define statement?

bilby91 · 2023-04-13T15:33:15Z

@tgauth Yes, I can confirm that config.h in the root got updated.

/* Use PIPES instead of a socketpair() */
#define USE_PIPES 1

/* define 1 if afunix.h is available */
#define HAVE_AFUNIX_H 1

Still, it doesn't seem to be compiling with HAVE_AFUNIX_H 1.

tgauth · 2023-04-13T16:14:49Z

@tgauth Yes, I can confirm that config.h in the root got updated.
/* Use PIPES instead of a socketpair() */
#define USE_PIPES 1

/* define 1 if afunix.h is available */
#define HAVE_AFUNIX_H 1
Still, it doesn't seem to be compiling with HAVE_AFUNIX_H 1.

oh, my bad - seems like config.h needs to be explicitly included in posix_compat where HAVE_AFUNIX_H is used. Try putting #include "..\..\..\config.h" in both socketio.c and w32fd.c, or try defining it in w32fd.h since that's imported by both?

bilby91 · 2023-04-13T17:30:06Z

@tgauth That did the trick, thanks! I added the include statement in both files because doing so in w32fd.h had issues with signal.c.

I'm testing now a different approach for temp folder where the unix socket will live. I'm changing the implementation so that it uses fileapi.h GetTempPath to replace the /tmp. Do you think this is a better location ?

tgauth · 2023-04-13T17:44:21Z

I'm testing now a different approach for temp folder where the unix socket will live. I'm changing the implementation so that it uses fileapi.h GetTempPath to replace the /tmp. Do you think this is a better location ?

yep, that makes sense to me!

bilby91 · 2023-04-13T20:51:39Z

@tgauth I think I have addressed the different issues now. Take a look at the code that generates the temporal directory.

Last commit was pushed using this feature 😎

bilby91 · 2023-04-13T22:24:31Z

@tgauth Was looking at the failing tests. They generally timeout ? Some tests seem to have failed but they look unrelated (at first glance). I wonder if having #define HAVE_AFUNIX_H 1 by default is causing issues in the ADO environment.

Any insight ?

bilby91 · 2023-04-14T12:27:06Z

I think I know what is going on. Will try to fix it over the weekend.

bilby91 · 2023-04-15T00:00:52Z

@tgauth Tests are passing now :)

I had to change a few other calls to socket() due to the ssh-agent's named pipes implementation. I don't love how it's done but the path for the named pipe is fixed so comparing should be safe to decide (based on my understanding and local testing).

I think that a subsequent change could replace the named pipes usage in agent.c and use a native AF_UNIX socket to simplify the implementation.

I'm moved AGENT_PIPE_ID to config.h because I was planning to use it with strcmp but later realized that it's a wchar_t instead of a char * so I would need to convert it before comparing. I can do the transformation or compare with the hardcoded version. What do you think ?

Anything else I might be missing ?

Thanks so much looking at the changes!

tgauth · 2023-04-17T20:59:37Z

I know that a subset of Windows version support it but I'm not sure what is the best way to handle this. If I can get any guidance in terms of how to approach it I can test it and modify it.

Based on https://devblogs.microsoft.com/commandline/af_unix-comes-to-windows/ support started with Windows 10. We have a similar check already defined that I think can also be utilized here: https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/win32compat/misc_internal.h#L15

Anything else I might be missing ?

Ideally, would like to have some test coverage for agent forwarding! There are some upstream agent tests that the CI currently skips - https://github.com/PowerShell/openssh-portable/blob/latestw_all/regress/agent.sh, https://github.com/PowerShell/openssh-portable/blob/latestw_all/contrib/win32/openssh/bash_tests_iterator.ps1#L189. If the tests applicable to agent forwarding could be modified as necessary for Windows and not skipped, that would be great! A single bash test can be run with the following command:
.\bash_tests_iterator.ps1 -OpenSSHBinPath "C:\openssh-portable\bin\x64\Release\" -BashTestsPath "c:\openssh-portable\regress" -shellpath "c:\cygwin64\bin\sh.exe" -TestFilePath "C:\openssh-portable\regress\agent.sh"

tgauth · 2023-04-17T21:04:29Z

I'm moved AGENT_PIPE_ID to config.h because I was planning to use it with strcmp but later realized that it's a wchar_t instead of a char * so I would need to convert it before comparing. I can do the transformation or compare with the hardcoded version. What do you think ?

For maintainability, I'm in favor of doing the transformation so that AGENT_PIPE_ID is the singular definition of the pipe name, as opposed to hardcoding it in a second place.

bilby91 · 2023-04-17T21:47:39Z

@tgauth Thanks for the feedback!

I'll take a look at the tests and doing the transformation for AGENT_PIPE_ID!

In regards to using IsWindows8OrGreater/IsWin7OrLess, you are thinking of replacing the HAVE_AFUNIX_H with calls to those functions at runtime ? Not sure how Microsoft build process works but HAVE_AFUNIX_H could be defined only for Win7 >.

Just want to make sure I fully understand how we want to protect the paths.

bilby91 · 2023-04-19T13:52:18Z

@tgauth I was able to include a fraction of the agent.sh tests! Had to fix a bunch of stuff in the test-exec.sh, which was probably preventing them from running originally.

I had to continue excluding some of the tests for two reasons:

Some tests try to use different agents but Windows only supports one of them. I think this limitation can go away if we implement the agent with AF_UNIX and follow the same strategy as OpenSSH-portable implementation.
I wasn't able to get certificate-based key authentication to work. I saw some other tests skipped as well so I'm assuming this is currently not working.

Having said that, with these changes, we have a little bit more coverage while keeping all the other tests passing.

I also made the change to compare with AGENT_PIPE_ID. I think the last piece remaining would be to decide what to do with HAVE_AFUNIX_H and IsWindows8OrGreater. I think we could keep it as it is with HAVE_AFUNIX_H and decide at build time if the feature needs to be included or not, or, I can change the places where I used HAVE_AFUNIX_H and use IsWindows8OrGreater instead. Let me know what you think!

bilby91 · 2023-04-21T12:50:07Z

Ping @tgauth

tgauth · 2023-04-24T18:25:14Z

@bilby91, thank you for working on this!

The changes look good from an initial review, but we need some additional time to do a comprehensive review, since this would be a new feature for Win32-OpenSSH.

For example, upstream has made some agent restrictions that have not yet been implemented for Windows that would be required to achieve parity.

bilby91 · 2023-04-24T18:52:11Z

@tgauth Thanks for the feedback!

I totally get that this kind of change needs an important level of review before shipping to Windows.

Is there anything on my end that you think I can help with in the meantime?

tgauth · 2023-05-02T17:53:38Z

Is there anything on my end that you think I can help with in the meantime?

Not that I can think of right now, but will keep you posted - thanks for your patience!

bilby91 · 2023-09-30T01:09:15Z

@tgauth was wondering if you had any updates about this topic!

Thanks!

maertendMSFT · 2023-10-02T16:37:57Z

This is delayed due to focusing on more simple PRs into the 9.4 release as well as an in depth security review of the behavior change. Because this is a new feature, not a bug fix, it requires further scrutiny.

bilby91 · 2023-10-02T18:25:05Z

@maertendMSFT I understand. Thanks for the feedback!

arixmkii · 2023-12-05T21:07:27Z

@bilby91 Do you know if this would also work for mux and ControlPath or will it require more work to get it working?

bilby91 · 2023-12-05T22:03:27Z

@arixmkii To be honest I don't know.

Any chance you have local development environment where you could test it ? If not I might be able to help over the weekend.

arixmkii · 2023-12-06T11:04:00Z

@bilby91 I rebased your changes to 9.4.0.0 and built locally. Unfortunately it didn't work, but the error is suspicious, I don't have logs at hand, but it was about socket naming. Might be an error at command parsing level. I will debug it at the end of this week to figure out more details.

arixmkii · 2024-01-09T20:54:27Z

Answering my question. No Mux will not work as everything mux is now covered by no_ops stubs. I will try to investigate the possible subset of Mux, which could be achieved with AF_UNIX additions from this PR.

JojOatXGME

@bilby91, I just noticed your PR. I am just a bit confused as it looks like it might contradict parts of what I have researched over the weekend. (See also PowerShell/Win32-OpenSSH#1024 (comment).)

I also looked over your PR and made a few comments. However, I was missing some context and could therefore not really understand all of it. (Also note that I am not affiliated with Microsoft.)

What I don't understand at your PR is what has agent forwarding to do with whether SSH on Windows uses unix domain sockets instead of named pipes? This seems like two unrelated topics for me. I assume the changes in session.c apply to sshd? Is it possible that you could also have used named pipes here and then skipped all the other changes regarding unix domain sockets?

I've also tested that WSL can successfully use the SSH Agent sock as long as the SSH_AUTH_SOCK is defined in the bash session.

Are you talking about WSL1? Because I thought that WSL2 does not support interoperability with unix domain sockets (yet).

JojOatXGME · 2024-02-25T16:35:29Z

contrib/win32/win32compat/w32fd.c

+	if(wcstombs(pipeid, AGENT_PIPE_ID, len + 1) != (size_t) -1 && strcmp(addr->sun_path, pipeid) == 0)
+		return w32_fileio_socket(SOCK_STREAM, 0);
+	else
+		return w32_unix_socket(SOCK_STREAM, 0);


If I understand the code correctly, you could technically just call w32_socket(AF_INET, SOCK_STREAM, 0) and w32_socket(AF_UNIX, SOCK_STREAM, 0), instead of introducing two new methods? Although I understand that AF_INET would be misleading.

Besides, instead of checking whether addr->sun_path equals AGENT_PIPE_ID, I guess you could also just check the prefix. If the path starts with \\.\pipe\, I think you can assume that it is a named pipe. This way, the solution stays compatible with other named pipes as well. For example, if a user uses Pageant (PuTTY authentication agent), they might also use named pipes, but the pipe may be named //./pipe/pageant.<username>.<random_sequence>. (This means you should probably also accept both, slash (/) and backslash (\) in the prefix.)

Originally I was thinking of behaving this way just for this implementation but I think the prefix makes sense in order to support other implementations.

JojOatXGME · 2024-02-25T16:57:44Z

authfd.c

-	if ((sock = socket(AF_UNIX, SOCK_STREAM, 0)) == -1)
+	#ifdef HAVE_AFUNIX_H
+	sock = w32_afunix_socket(&sunaddr);
+	#else
+	sock = socket(AF_UNIX, SOCK_STREAM, 0);
+	#endif
+
+	if (sock == -1)


Why do you need this #ifdef condition? Shouldn't you be able to always call w32_afunix_socket as this method already handles the #ifdef condition internally? Same question applies to misc.c and mux.c.

I think you are right. Will look into simplifying! Thanks

JojOatXGME · 2024-02-25T17:13:02Z

contrib/win32/win32compat/socketio.c

+	#ifdef HAVE_AFUNIX_H
+	context->accept_socket = socket(addr.ss_family, SOCK_STREAM, IPPROTO_IP);
+	#else
 	context->accept_socket = socket(addr.ss_family, SOCK_STREAM, IPPROTO_TCP);
+	#endif


Is this #ifdef condition necessary? If IPPROTO_TCP does not work for some reason, would it be possible to always use IPPROTO_IP (or just 0)? man socket(2) suggests that specifying 0 (which is the value of IPPROTO_IP) will use the default, which again is TCP.

I would need to try this out. I don't recall if IPPROTO_TCP was not working.

bilby91 · 2024-02-25T20:04:13Z

@bilby91, I just noticed your PR. I am just a bit confused as it looks like it might contradict parts of what I have researched over the weekend. (See also PowerShell/Win32-OpenSSH#1024 (comment).)

I also looked over your PR and made a few comments. However, I was missing some context and could therefore not really understand all of it. (Also note that I am not affiliated with Microsoft.)

What I don't understand at your PR is what has agent forwarding to do with whether SSH on Windows uses unix domain sockets instead of named pipes? This seems like two unrelated topics for me. I assume the changes in session.c apply to sshd? Is it possible that you could also have used named pipes here and then skipped all the other changes regarding unix domain sockets?

I've also tested that WSL can successfully use the SSH Agent sock as long as the SSH_AUTH_SOCK is defined in the bash session.

Are you talking about WSL1? Because I thought that WSL2 does not support interoperability with unix domain sockets (yet).

@JojOatXGME Thanks for taking a look at the code!

It has been almost an year since I worked on this change but I'm pretty positive I tried using named pipes instead of the AF_UNIX sockets and it was not working. Could have been lack of understanding on my side so it's highly likely than an implementation based on them is possible.

Regarding WSL1 vs WSL2, I think I was using WSL 2 in the machine I used for testing. Unfortunately I don't have that machine any more but I can look into double checking that in a different one potentially.

Are you able to test yourself by any chance ?

JojOatXGME · 2024-02-26T00:23:25Z

Are you able to test yourself by any chance ?

I have to see. I haven't worked on the project before. I actually did not work on any C-project for quite some time. I would have to set up some dev environment to build the project. Anyway, I will try to play with it within the next two weeks.

PS: Assuming we could use named pipes in session.c for the agent forwarding, we might be able to split this PR into three smaller changes: 1. agent forwarding, 2. client side support for native unix domain socket, 3. option to use native unix domain sockets for ssh-agent and sshd (agent forwarding). Maybe splitting the topic would simplify the security review? The biggest security risk or uncertainty comes from 3. in my opinion. If I find time to play with it, this is what I will properly try to check.

thedarkcolour · 2024-09-24T04:09:52Z

mux.c

-	if ((sock = socket(PF_UNIX, SOCK_STREAM, 0)) == -1)
+	#ifdef HAVE_AFUNIX_H
+	sock = w32_afunix_socket(&addr);
+	#elif


Is this elif intentional? It doesn't even compile.

@thedarkcolour That's wrong, it should be an #else. I'll fix it.

Add support for AF_UNIX

3a77587

bilby91 force-pushed the add-af-unix-support branch from 57dabcc to 3a77587 Compare April 12, 2023 22:03

Guard AF_UNIX paths with HAVE_AFUNIX_H

f7a8825

Use Windows GetTempPath instead of /tmp

6dd4311

bilby91 added 4 commits April 15, 2023 03:23

Conditionally use af_unix and fileio

b31d181

Comment regress/cfgmatch.sh temporary

6cc801b

Add back regress/cfgmatch.sh

705ed7e

Fix regress/cgmatch.sh comments

8ca026c

bilby91 force-pushed the add-af-unix-support branch from dafedf8 to 8ca026c Compare April 14, 2023 22:59

maertendMSFT requested a review from tgauth April 17, 2023 16:10

maertendMSFT assigned tgauth Apr 17, 2023

bilby91 added 2 commits April 19, 2023 05:15

WIP: Add agent.sh tests

36e5db5

Compare with AGENT_PIPE_ID

fd096f5

bilby91 force-pushed the add-af-unix-support branch from 795b52e to fd096f5 Compare April 19, 2023 12:34

Okeanos mentioned this pull request Aug 30, 2023

Support MSYS2 ssh-agent sockets on Windows keepassxreboot/keepassxc#4681

Open

JojOatXGME reviewed Feb 25, 2024

View reviewed changes

florisvdg mentioned this pull request Mar 4, 2024

Agent Forwarding Windows client to Windows host not working? PowerShell/Win32-OpenSSH#1865

Open

shanman190 mentioned this pull request Apr 19, 2024

Enable SSH agent support on Windows loft-sh/devpod#1008

Merged

thedarkcolour reviewed Sep 24, 2024

View reviewed changes

Add support for AF_UNIX #674

Are you sure you want to change the base?

Add support for AF_UNIX #674

Conversation

bilby91 commented Apr 12, 2023 • edited Loading

PR Summary

PR Context

bilby91 commented Apr 13, 2023

tgauth commented Apr 13, 2023

bilby91 commented Apr 13, 2023 • edited Loading

tgauth commented Apr 13, 2023

bilby91 commented Apr 13, 2023

tgauth commented Apr 13, 2023

bilby91 commented Apr 13, 2023

tgauth commented Apr 13, 2023

bilby91 commented Apr 13, 2023

bilby91 commented Apr 13, 2023

bilby91 commented Apr 14, 2023

bilby91 commented Apr 15, 2023

tgauth commented Apr 17, 2023 • edited Loading

tgauth commented Apr 17, 2023

bilby91 commented Apr 17, 2023

bilby91 commented Apr 19, 2023

bilby91 commented Apr 21, 2023

tgauth commented Apr 24, 2023

bilby91 commented Apr 24, 2023

tgauth commented May 2, 2023

bilby91 commented Sep 30, 2023

maertendMSFT commented Oct 2, 2023

bilby91 commented Oct 2, 2023

arixmkii commented Dec 5, 2023

bilby91 commented Dec 5, 2023

arixmkii commented Dec 6, 2023

arixmkii commented Jan 9, 2024

JojOatXGME left a comment • edited Loading

Choose a reason for hiding this comment

JojOatXGME Feb 25, 2024 • edited Loading

Choose a reason for hiding this comment

bilby91 Feb 25, 2024

Choose a reason for hiding this comment

JojOatXGME Feb 25, 2024

Choose a reason for hiding this comment

bilby91 Feb 25, 2024

Choose a reason for hiding this comment

JojOatXGME Feb 25, 2024

Choose a reason for hiding this comment

bilby91 Feb 25, 2024

Choose a reason for hiding this comment

bilby91 commented Feb 25, 2024

JojOatXGME commented Feb 26, 2024 • edited Loading

thedarkcolour Sep 24, 2024

Choose a reason for hiding this comment

bilby91 Sep 24, 2024

Choose a reason for hiding this comment

bilby91 commented Apr 12, 2023 •

edited

Loading

bilby91 commented Apr 13, 2023 •

edited

Loading

tgauth commented Apr 17, 2023 •

edited

Loading

JojOatXGME left a comment •

edited

Loading

JojOatXGME Feb 25, 2024 •

edited

Loading

JojOatXGME commented Feb 26, 2024 •

edited

Loading