Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added experimental support for Level-2 tunnel (Tunnel=ethernet option… #750

Open
wants to merge 6 commits into
base: latestw_all
Choose a base branch
from

Conversation

giusguerrini
Copy link

@giusguerrini giusguerrini commented Sep 23, 2024

PR Summary

This patch adds experimental tunnel management on Windows. It relies on TAP-Windows V9 driver, avilable here:

https://build.openvpn.net/downloads/releases/tap-windows-9.23.3-I601-Win10.exe

Only Level-2 tunnel (Tunnel=ethernet) is implemented.
At the moment (2024/09/23) this patch has been tested only on Windows10-19045
and TAP-Windows driver 9.23.3.601. Most of the tests used client mode
(ssh.exe). Server mode (sshd.exe) has only been tested superficially.
See README-TAP-Windows.txt for more information.

PR Context

Windows is the only platform on which this feature is missing.

@giusguerrini
Copy link
Author

@microsoft-github-policy-service agree

@giusguerrini
Copy link
Author

@microsoft-github-policy-service agree

servconf.c Outdated
@@ -568,7 +570,7 @@ typedef enum {
sPerSourcePenalties, sPerSourcePenaltyExemptList,
sClientAliveInterval, sClientAliveCountMax, sAuthorizedKeysFile,
sGssAuthentication, sGssCleanupCreds, sGssStrictAcceptor,
sAcceptEnv, sSetEnv, sPermitTunnel,
sAcceptEnv, sSetEnv, sPermitTunnel, sTunnelOptions,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we want to add a custom directive to sshd_config as this would be a divergence from the upstream code. Can the existing ethernet argument for the sPermitTunnel directive be used?

Copy link
Author

@giusguerrini giusguerrini Sep 25, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This new option is not a must for me. I added it because I was missing a way to make sure to use only a subset of available TAP instances. My idea was to distinguish TAPs by a fixed prefix in their friendly name. On my PC there are many TAP instances I wouldn't touch because they are (supposed to be) reserved to VPNs, teleservice programs, emulators etc... I even considered to use some weird tricks based on TunnelDevice values and a numeric suffix in the name (e.g. numbers > 10000 are name suffixes...).
Are you suggesting something like "PermitTunnel=ethernet[:options...]"? It looks good, bat valid only for server. In fact I also added the option to the client. Do you think that for client we can keep it, or it's better to withdraw it completely? Alternatively, for the client side we could use "Tunnel=ethernt" option. What do you think?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

By the way, is it possible to change (edit, add, delete files) an existing pull request, or is it better me to submit a new one? (Sorry, I am quite new in github)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes - for the client side, I think the Tunnel and TunnelDevice directives can be used.

As far as GitHub, you can make changes directly to this pull request by pushing the changes to this existing branch!

Copy link
Author

@giusguerrini giusguerrini Sep 30, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed it as we agreed. Bye

@tgauth
Copy link
Collaborator

tgauth commented Sep 25, 2024

/AzurePipelines run

Copy link

Azure Pipelines successfully started running 1 pipeline(s).

…mitTunnel"(sshd) and "Tunnel" (ssh): you can append to the type of tunnel a ":" followed by options (e.g. Tunnel=ethernet:my_option)
…mitTunnel"(sshd) and "Tunnel" (ssh): you can append to the type of tunnel a ":" followed by options (e.g. Tunnel=ethernet:my_option)
@giusguerrini
Copy link
Author

Hi @tgauth! I have two questions for you:

  1. I didn't realize that the main openssh-portable project contains a support for Cygwin. I don't know if my patch interoperates well with Cygwin, but I know that, in general, the direct usage of Win32 API in Cygwin sources my be problematic. Also, since Cygwin aims to emulate a Unix environment, I suppose that the "right" way to add TAP support to Cygwin would be to modify Cygwin's base library to synthetize /dev/tun* nodes, bind them to actual Windows devices etc... instead of modifying ssh/sshd. What is your opinion about this issue?

  2. I am considering to add a TUN support ("Tunnel=point-to-point"), but Windows (as far as I know) does not provide a TUN driver, so Level3 tunnel must be emulated over a Level2 tunnel (TAP). A full and efficient emulation would probably require intervention at the heart of SSH's state machine, a thing I'd avoid (it would apply only to Windows, I imagine it would imply lot of "#ifdef WIN32" in sensitive parts of the code... too bad). I'd like to concentrate the code in openbsd-compat/port-net.c, but I'd have to add a way to redirect read (recv) and write (send) there. I could advantage of the fact that "open()" is a variadic function, so I could add a non standard flag to pass some additional parameters to define callbacks for "read" and "write". Not easy, but it requires interventions only in the windows-specific part (contrib/win32/win32compat/...). Another way is by using a socket as pseudo-tun channel, and adding some thread to implement the L3-to-L2 part. What do you think? Do you have any advice for me?

Thank you in advance

@giusguerrini
Copy link
Author

I found a TUN driver for Windows, but it seems a bit immature to me. It's here: https://www.wintun.net/ . It requires an additional DLL, IMHO it's quite unhandy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants