[vnet] add vnet-(un)install-service commands for Windows #52164
Conversation
nklaassen
added
no-changelog
github-actions
bot
added
size/md
tsh
|
This means Windows users might have up to 3 different tsh installs on their machine, right?
How do we keep 2 and 3 in sync? What happens when the user updates Teleport Connect (and gets a new version of tsh along with it)? |
Every time the user runs the Connect installer, it will call |
lib/vnet/install_service_windows.go
Outdated
|
|
||
| func serviceNameAndInstallDir(username string) (string, string) { | ||
| serviceName := userServiceName(username) | ||
| serviceInstallDir := filepath.Join(`C:\Program Files`, serviceName) |
There was a problem hiding this comment.
Why does each user need a separate service? And why does each one need to be in a separate directory, directly within Program Files?
There was a problem hiding this comment.
I'm giving each user a separate service simply so that each user can have a different version of Connect installed with a matching version of tsh running the service. Although, only one user can run the service at a time, if another user tries they will fail to create the named TUN interface. Context on Program Files #52164 (comment) the service exe can't be in a path that normal users can write to otherwise we risk arbitrary privilege escalation
There was a problem hiding this comment.
each user can have a different version of Connect installed with a matching version of tsh running the service
This is not something that Windows handles terribly well... if multiple users need a program, it probably shouldn't be installed in a per-user manner i.e. under appdata/similar, and should be installed once under the program files folder instead.
If we really do need different versions, then IMO the service should be installed somewhere like Program Files\Teleport\<version>\<service name> instead, rather than polluting program files for every user. This would be pretty nasty on systems that have dozens, hundreds, or thousands of users log into them in their lifetime.
There was a problem hiding this comment.
On Windows at least we currently don't offer an option to do a per-machine install of Connect rather than per-user. If that's a path we want to go down we could figure out the implications of a per-machine VNet service installation too, but so far I haven't been looking to change this just for the sake of VNet.
One complication of doing a per-version Service along with per-user Connect installations is what to do when one user uninstalls Connect. Do we delete the service for that version and break the other user's installation? do we attempt to maintain a list of which user installations are referencing that service at any given time?
The other complicating factor is that I currently only grant permission for the installing user to start the service, other users are not allowed to start it. Now, if the duplicated files are a concern there are a couple options:
- We could have a separate service for each user
TeleportVNet-Alice,TeleportVNet-Bob, etc and have them all reference the same exe pathProgram Files\TeleportVNet\17.3.0\tsh.exe, but each service would have its own permissions - We could have a single service for each version
TeleportVNet-17.3.0andtsh vnet-install-servicecould add the ACL to allow the current user to start the service without changing the existing permissions
These all seem messier than the per-user installation to me. I'm inclined to keep things as I have them unless and until we decide to support per-machine Connect installations on Windows.
There was a problem hiding this comment.
One complication of doing a per-version Service along with per-user Connect installations is what to do when one user uninstalls Connect. Do we delete the service for that version and break the other user's installation? do we attempt to maintain a list of which user installations are referencing that service at any given time?
If we need to answer questions like these then the vnet feature should be restricted to machine-wide installations. Windows is not designed for services to be ran per-user, as shown by both the problems pointed out above, and the need for an elevated user to install/remove/change the service.
To add onto this, this gets even more complicated when users are removed and re-added (maybe representing the original human user, maybe not), which is relatively common in larger enterprise environments. Their SIDs change, but the username remains the same... so how should Teleport handle this?
If the application needs to manage system-wide resources (such as network interfaces), and run as a different user, and run in the background as a service, and be kept in-sync with "client" which is potentially used by multiple users, then the entire application needs to be installed once per machine, rather than per user.
There was a problem hiding this comment.
Back when we were adding Windows support to Connect, it was not something we thought about too much and went with the defaults used by electron-builder, which is to install the app per user.
A customer once ran into a problem where they were running the installer (through some kind of MDM most probably) and wanted to install the app in Program Files, not in %localappdata%. The customer managed to solve this by using the \D switch to install Connect in a specific directory.
Changing this should be a matter of changing the NSIS section of electron-builder's config to set perMachine to true. As long as the installer gracefully handles this migration between earlier versions and 17.3, we should be fine. Based on Grzegorz's findings when experimenting with oneClick: false, it seems that it should work.
Hopefully it doesn't break setups where people use the \D switch to install it in Program Files. I don't know if changing perMachine to true does any extra stuff that would set it apart from running today's Connect installer with \D set to Program Files.
There was a problem hiding this comment.
I have switched to a per-machine installation. It simplifies things here a bit since the installer now already runs with admin rights and no files need to be copied.
There was a problem hiding this comment.
I have switched to a per-machine installation.
For compatibility reasons should the installer provide an option for per-user (without vnet support) or per-machine?
There was a problem hiding this comment.
I can have a look at making it an option but this will be more relevant to the installer PR #52165
There was a problem hiding this comment.
Fair enough. I don't know if this is needed at all - this is more of a product consideration than a technical one.
nklaassen
force-pushed
the
nklaassen/vnet-service-installer
branch
from
3af6c3f to
cf95e40
Compare
| Trustee: windows.TRUSTEE{ | ||
| TrusteeForm: windows.TRUSTEE_IS_NAME, | ||
| TrusteeType: windows.TRUSTEE_IS_WELL_KNOWN_GROUP, | ||
| TrusteeValue: windows.TrusteeValueFromString("Authenticated Users"), |
There was a problem hiding this comment.
vnet tunnels are specific to each user (even if configured for the entire machine), right? If so, is there any meaningful security impact in one user starting/stopping/querying the service, if it is in use by another user?
There was a problem hiding this comment.
I suppose it would be similar to another user being able to disconnect the machine from a VPN, or connecting to a different VPN. So yes, there's some impact to other users being able to start and stop VNet, but I'm not sure there's much we can do about that with a per-machine installation. The VNet security invariants are not really ideal for multi-user machines in the first place, given that any user could make a TCP connection to one of your Teleport apps authenticated as the active VNet user. But again, this is similar to a VPN
There was a problem hiding this comment.
Not a blocker for this PR, but because of this we may want to add the ability to set the group allowed to start/stop the service at some point.
public-teleport-github-review-bot
bot
removed the request for review
from EdwardDowling
nklaassen
force-pushed
the
nklaassen/vnet-service-installer
branch
from
3d1e007 to
b822cfc
Compare
github-merge-queue
bot
removed this pull request from the merge queue due to failed status checks
|
@nklaassen See the table below for backport results.
|
This PR adds the command
tsh vnet-install-serviceto install the VNet Windows service. It does this by using the Windows Service Control Manager to configure a Windows service to starttsh.exewith the appropriate arguments.tsh.exeshould not be saved in a user-writable path or else the user could overwrite the exe and then start the service to get privilege escalation. To try to prevent misconfiguration, the installer asserts thattsh.exemust be installed underProgram Files, usuallyC:\Program Files\but this can be controlled by the environment variablePROGRAMFILES. Iftsh.exeis not found there, or wintun.dll is not installed next totsh.exe, the command fails. This will require a per-machine install of Connect so thattsh.exeis properly installed under PROGRAMFILES and so that the installer runs with admin rights, which are necessary in order to install the service. The following PR #52165 updates the Connect installer.I'm also adding a
tsh vnet-uninstall-servicecommand which deletes the service configuration, although this will not be called by the Connect uninstaller which runs aftertsh.exehas already been deleted. Fortunately all the uninstaller needs to do is delete the service configuration, which can be done in a single command. Thetsh vnet-uninstall-servicecommand may be useful if any users manually install the service for use withtshinstead of Connect.