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

RFD 195: Windows VNet Support #50850

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

RFD 195: Windows VNet Support #50850

wants to merge 5 commits into from

Conversation

nklaassen
Copy link
Contributor

No description provided.

@github-actions github-actions bot added rfd Request for Discussion size/md labels Jan 8, 2025
@nklaassen nklaassen mentioned this pull request Jan 8, 2025
1 task
Comment on lines 160 to 168
When calling the `AuthenticateProcess` RPC, the Windows service will:
1. Create a Windows named pipe and give the installing user SID permission to open the pipe.
1. Pass the name of the pipe (via the RPC) to the user process.
1. Wait for the user process to dial the named pipe.
1. Use the Windows API `GetNamedPipeClientProcessId` to get the pipe client
process handle.
1. Once it has the user process handle, it can confirm the path of the exe
matches the path of the Windows service, and confirm that the exe is signed
by the same issuer as itself.
Copy link
Contributor

Choose a reason for hiding this comment

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

What happens if this fails for some reason? What is the UX for the user?

rfd/0195-vnet-windows.md Outdated Show resolved Hide resolved
@nklaassen nklaassen force-pushed the rfd/0195-vnet-windows branch from aa25e62 to 2b4ce36 Compare January 9, 2025 21:45
@nklaassen nklaassen added the no-changelog Indicates that a PR does not require a changelog entry label Jan 9, 2025
@nklaassen
Copy link
Contributor Author

nklaassen commented Jan 9, 2025

edit: done in the latest commit
TODO: when installing the windows service, the exe for the service is identified by a filesystem path to the exe, and every time the service starts it will execute that exe. if any user can overwrite the exe at that path, they could run their own exe as a service, with admin rights, and they would be allowed to start the service without admin rights in the current design. To prevent this we need to install the tsh exe in a path that cannot be written without admin rights, probably under "Program Files". Currently the electron installer installs everything under C:\Users<user>\AppData, which the user can always overwrite, so this is a problem. Perhaps when we install the service we can copy the tsh exe to "Program Files" and use that path for the service. Need to consider conflicts if the user installs both Connect and standalone tsh, and if they can share this path

1. Wait for the user process to dial the named pipe.
1. Use the Windows API `GetNamedPipeClientProcessId` to get the pipe client
process handle.
1. Once it has the user process handle, it can confirm the path of the exe
Copy link
Member

Choose a reason for hiding this comment

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

TODO: when installing the windows service, the exe for the service is identified by a filesystem path to the exe, and every time the service starts it will execute that exe. if any user can overwrite the exe at that path, they could run their own exe as a service, with admin rights, and they would be allowed to start the service without admin rights in the current design. To prevent this we need to install the tsh exe in a path that cannot be written without admin rights, probably under "Program Files". Currently the electron installer installs everything under C:\Users\AppData, which the user can always overwrite, so this is a problem. Perhaps when we install the service we can copy the tsh exe to "Program Files" and use that path for the service. Need to consider conflicts if the user installs both Connect and standalone tsh, and if they can share this path

Ah, so to put in other words, the Windows service registry cannot guarantee that no one tampered with the service after it's been added, right?

I feel like manipulating the install path might create a bit of a mess. FWIW, there might be customers that install Connect in Program Files through the \D switch provided to the Windows installer. See this year old Slack thread for more details. On top of that, the directory with tsh.exe is automatically added to Path on Windows during installation.

If down the road we decide to add auto updates to Connect, copying tsh to an extra location might pose a problem too.

I found this, but I assume implementing any of this might require too much effort. https://learn.microsoft.com/en-us/windows/win32/services/protecting-anti-malware-services-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was thinking of "installing" the service exe at runtime by copying the tsh binary to a specific path, rather than actually doing this in the installer. This would be a part of the service installation the first time VNet runs, and it could auto-update (with a UAC prompt) if we detect the service and the tsh process binaries no longer match. We could make a copy per-user to allow multiple users to run different versions. so on first VNet start it would look like:

  1. generate service name "VNetService-Username"
  2. if service is already installed, run it and skip remaining steps
  3. copy os.Executable() to C:\Program Files\TeleportVNet\VNetService-Username
  4. install service via sc.exe with path set to the above

if the service is already installed, after starting it we check that the version matches, if it doesn't match, reinstall following the above steps

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think the anti-malware service is a non-starter because of "The service EXE must be page hash signed, and any non-Windows DLLs that get loaded into the service must be also signed with the same certificates" and we need to load wintun.dll which will be signed by a different cert (it is pre-signed and that's the only supported way to distribute it)

Copy link
Member

@ravicious ravicious Jan 13, 2025

Choose a reason for hiding this comment

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

I'm thinking how we could make this work when Connect is installed through some kind of mobile device management software, like the one mentioned in that Slack thread. And now that I understand that the Program Files path would be used only for the service, I'm thinking that doing it through the installer would perhaps be better for the majority of users.

Currently, Connect on Windows can be updated only through the installer. We already run custom post-install and post-uninstall stuff on Windows. We could have a command that prepares a VNet service for the current user and run it from the installer (if possible). If I understand it correctly, this would completely remove the need for any UAC prompts after a successful installation of Connect. It also saves us from implementing some version detection to see if the admin service and Connect use the same version of tsh.

MDM admins could re-run this command with a --user flag after running the installer. tsh doesn't have an installer on Windows, so it could expect the user to manually run the command after every update.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ravicious is the idea to install the service using a post-install script and uninstall it with a post-uninstall script, and leave all the current binaries where they currently are? I could see this being a pretty good option

by "install the service" i mean copying the tsh binary to C:\Program Files\<something-user-specific-path> and installing it as a service with sc.exe

Copy link
Member

Choose a reason for hiding this comment

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

Yes, exactly. I don't know how to run a command from an NSIS installer though, but it should be doable. I assume that by the time the customInstall macro is executed, the files are already copied.

When it comes to customUnInstall, we'd have to check if it's executed before files are removed or after. If it runs after the files are removed, then we wouldn't be able to uninstall the service but merely remove the files from Program Files.

IIRC, those macros are custom to electron-builder, they're not part of NSIS. https://www.electron.build/nsis#custom-nsis-script I see that it has customUnInstall and customRemoveFiles, so hopefully customUnInstall runs before the files are removed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added my new installation plan in the latest commit

Comment on lines 67 to 68
The signed `wintun.dll` file will be distributed with Connect and installed in
the same directory as `tshd`.
Copy link
Member

Choose a reason for hiding this comment

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

As I mentioned in the other comment, the directory that has tsh.exe is added to Path. https://github.com/gravitational/teleport/blob/master/rfd/0099-link-bundled-tsh.md

From what I see, this shouldn't pose a problem to us specifically. It might be a problem for other apps which depend on the same dll being in Path, but I'm not that familiar with best practices there. https://learn.microsoft.com/en-us/windows/win32/dlls/dynamic-link-library-search-order#standard-search-order-for-unpackaged-apps

Comment on lines 86 to 87
We will install the Service with a security descriptor that allows the
installing user's SID (user ID) to launch the service without elevated privileges.
Copy link
Member

Choose a reason for hiding this comment

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

I asked about this under the other PR (#50468 (comment)), but I guess this is a better place to do this. Does it mean that only a single user on the device can install and use the service?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated this in the latest to support multiple per-user installations and per-machine installations.

@nklaassen nklaassen added the vnet label Jan 17, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
no-changelog Indicates that a PR does not require a changelog entry rfd Request for Discussion size/md vnet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants