[vnet] windows service stub #50468

nklaassen · 2024-12-20T02:14:12Z

This commit adds a Windows service for VNet. Currently all the service does is create a TUN interface, it doesn't yet actually handle any networking or establish any IPC, the rest will come in later PRs.

Installation of the service will eventually be handled by the Connect installer.

If you want to test this out on a Windows machine/VM you will need to:

install wintun.dll in the same directory as the tsh exe
install tsh as a Windows service named TeleportVNet service, from an admistrative command prompt:

sc.exe create TeleportVNet binPath="<path-to-tsh> vnet-service" start=demand
# add (A;;RPWPLC;;;AU) to the default service DACL so authenticated users can start/stop/query the service
sc.exe sdset TeleportVNet "D:(A;;CCLCSWRPWPDTLOCRRC;;;SY)(A;;CCDCLCSWRPWPDTLOCRSDRCWDWO;;;BA)(A;;CCLCSWLOCRRC;;;IU)(A;;CCLCSWLOCRRC;;;SU)(A;;RPWPLC;;;AU)S:(AU;FA;CCDCLCSWRPWPDTLOCRSDRCWDWO;;;WD)"

you should now be able to run tsh vnet -d
the service will start sc.exe query TeleportVNet
A TUN interface is created netsh interface show interface
The service writes logs to logs.txt in the directory where tsh is installed (this is temporary until I find a better place for logs).
The service stops and the interface is cleaned up when the user process exits.

nklaassen · 2024-12-20T02:14:27Z

cc @ravicious, I'm once again refactoring a bunch of stuff

nklaassen · 2025-01-07T20:01:30Z

@ravicious @atburke @codingllama this is ready for review

ravicious

I looked through the code and it seems to work fine. I'll give it a go tomorrow in a VM and I'll check if the macOS version still works.

lib/vnet/escalate_windows.go

ravicious · 2025-01-08T15:13:24Z

lib/vnet/escalate_windows.go

+// configureServicePermissions sets the security descriptor DACL on the service
+// such that the user who installed the service (identified by userSIDStr) is
+// allowed to start, stop, and query the service.
+func configureServicePermissions(service *mgr.Service, userSIDStr string) error {


Does this assume that only a single user on the device can install and use the service?

removed everything about installing the service and configuration permissions for now, will solve this later in a way that multiple users can install/use the service

ravicious · 2025-01-08T15:24:44Z

lib/vnet/process_manager.go

+	case <-pm.closed:
+		// Errors are expected after the process manager has been closed,
+		// usually due to context cancellation.
+		return nil


What problems does this solve compared to the version on master? I assume it has to do with what errors are returned by Wait, but by just looking at this file alone is hard to tell what changed in the whole process orchestration compared to master.

Do we just want Close() to not make Wait() return a "context cancelled" error? I know we've changed how we approach this specific error a couple of times, so I'd just want to have some clarity on how to approach it in the future. ;)

teleport/lib/vnet/run.go

Lines 177 to 218 in 47bf4ca

func newProcessManager() (*ProcessManager, context.Context) {

ctx, cancel := context.WithCancel(context.Background())

g, ctx := errgroup.WithContext(ctx)

return &ProcessManager{

g: g,

cancel: cancel,

}, ctx

}

// ProcessManager handles background tasks needed to run VNet.

// Its semantics are similar to an error group with a context, but it cancels the context whenever

// any task returns prematurely, that is, a task exits while the context was not canceled.

type ProcessManager struct {

g *errgroup.Group

cancel context.CancelFunc

}

// AddCriticalBackgroundTask adds a function to the error group. [task] is expected to block until

// the context returned by [newProcessManager] gets canceled. The context gets canceled either by

// calling Close on [ProcessManager] or if any task returns.

func (pm *ProcessManager) AddCriticalBackgroundTask(name string, task func() error) {

pm.g.Go(func() error {

err := task()

if err == nil {

// Make sure to always return an error so that the errgroup context is canceled.

err = fmt.Errorf("critical task %q exited prematurely", name)

}

return trace.Wrap(err)

})

}

// Wait blocks and waits for the background tasks to finish, which typically happens when another

// goroutine calls Close on the process manager.

func (pm *ProcessManager) Wait() error {

return trace.Wrap(pm.g.Wait())

}

// Close stops any active background tasks by canceling the underlying context.

func (pm *ProcessManager) Close() {

pm.cancel()

}

i don't feel strongly about this, for some reason while writing i wanted it not to return an error after the processmanager was closed, if you have another opinion i can revert

tool/tsh/common/tsh.go

ravicious · 2025-01-08T15:42:27Z

tool/tsh/common/vnet_windows.go

+			newVnetCommand(app),
+			newVnetInstallServiceCommand(app),
+			newVnetUninstallServiceCommand(app),


Connect will have to perform its own equivalent of those commands, right?

no, the vnet service command will run when tshd re-execs itself, it should be handled by tsh fine

ravicious · 2025-01-09T13:59:58Z

lib/vnet/escalate_windows.go

+				return trace.Wrap(err, "querying admin service")
+			} else {
+				if status.State != svc.Running && status.State != svc.StartPending {
+					return trace.Errorf("service stopped running prematurely, status: %v", status)


tsh vnet works fine on Windows. The macOS daemon works too.

At first I copied the wrong wintun.dll. I copied the arm64 version but my VM is actually amd64. This resulted in tsh vnet returning this:

$ ./build/tsh.exe vnet VNet is ready. ERROR: running admin process in the background service stopped running prematurely, status: {1 0 0 0 0 0 0}

Is it possible to convert the status to some kind of a text representation?

yeah switched to %+v which gives a better view

codingllama

Overall this is pretty dense at times (Windows services are nobody's specialty) and holds a large number of related-but-seemingly unnecessary changes (800+ lines in 25 files is no joke). I would advise, in the future, that you split PRs like this into smaller, more focused parts.

Given the number of comments it's probably not worth splitting now, apart from tool/tsh changes which I do think could still be reasonably split.

lib/teleterm/vnet/service.go

lib/vnet/admin_process_darwin.go

lib/vnet/admin_process_windows.go

lib/vnet/process_manager.go

lib/vnet/unsupported_os.go

tool/tsh/common/tsh.go

nklaassen · 2025-01-10T02:22:59Z

Back to draft for now, i'm taking a stab at chunking this down a bit

nklaassen · 2025-01-10T02:27:05Z

Moved the tsh changes in #50935

nklaassen · 2025-01-16T02:34:19Z

@codingllama @ravicious this is ready for review again, i did my best to remove unrelated changes and everything about service installation, it's about half the size now

codingllama

Looks good, thanks for refactoring!

lib/vnet/admin_process_darwin.go

lib/vnet/escalate_daemon_darwin.go

tool/tsh/common/tsh.go

lib/vnet/process_manager.go

codingllama · 2025-01-16T14:16:21Z

lib/vnet/user_process_darwin.go

+// runPlatformUserProcess creates a network stack for VNet and runs it in the
+// background. To do this, it also needs to launch an admin process in the
+// background. It returns a [ProcessManager] which controls the lifecycle of
+// both background tasks.
 //
 // The caller is expected to call Close on the process manager to close the


It's a little strange that the caller has to close the ProcessManager on error, but not on success - typically code goes the other way. Should we make sure the ProcessManager on errors here instead?

yeah this ended up weird when i kept this part here and moved process manager creation into the platform-specific functions. Now i've moved closing the ProcessManager down into the platform functions.

lib/vnet/escalate_windows.go

lib/vnet/admin_process_darwin.go

nklaassen added the no-changelog Indicates that a PR does not require a changelog entry label Dec 20, 2024

github-actions bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Dec 20, 2024

github-actions bot requested review from capnspacehook and smallinsky December 20, 2024 02:14

ravicious self-requested a review December 20, 2024 08:35

nklaassen marked this pull request as draft December 28, 2024 01:18

nklaassen removed request for smallinsky and capnspacehook December 28, 2024 01:19

nklaassen force-pushed the nklaassen/windows-service branch from b97bcdd to 7295ac0 Compare January 7, 2025 01:49

nklaassen marked this pull request as ready for review January 7, 2025 01:53

github-actions bot requested review from atburke and codingllama January 7, 2025 01:53

ravicious reviewed Jan 8, 2025

View reviewed changes

ravicious self-requested a review January 8, 2025 15:43

ravicious reviewed Jan 9, 2025

View reviewed changes

codingllama reviewed Jan 9, 2025

View reviewed changes

nklaassen force-pushed the nklaassen/windows-service branch from 7295ac0 to 83cda08 Compare January 9, 2025 22:36

nklaassen marked this pull request as draft January 10, 2025 02:22

nklaassen force-pushed the nklaassen/windows-service branch from 83cda08 to 36bdd77 Compare January 10, 2025 02:23

nklaassen changed the base branch from master to nklaassen/vnet-tsh January 10, 2025 02:26

This was referenced Jan 10, 2025

RFD 195: Windows VNet Support #50850

Open

[vnet] add windows tsh cli commands #50935

Merged

Base automatically changed from nklaassen/vnet-tsh to master January 14, 2025 01:31

nklaassen force-pushed the nklaassen/windows-service branch 2 times, most recently from 18bf244 to b47a65a Compare January 14, 2025 02:05

nklaassen force-pushed the nklaassen/windows-service branch 4 times, most recently from 9c88cae to 1984e63 Compare January 16, 2025 02:09

[vnet] Windows service

a6f09c8

nklaassen force-pushed the nklaassen/windows-service branch from 1984e63 to a6f09c8 Compare January 16, 2025 02:15

nklaassen marked this pull request as ready for review January 16, 2025 02:33

github-actions bot requested a review from fheinecke January 16, 2025 02:33

github-actions bot added the size/sm label Jan 16, 2025

github-actions bot requested a review from probakowski January 16, 2025 02:33

nklaassen changed the title ~~[vnet] install and run windows service~~ [vnet] windows service stub Jan 16, 2025

codingllama reviewed Jan 16, 2025

View reviewed changes

address alan's comments

f39d444

nklaassen requested a review from codingllama January 16, 2025 20:35

codingllama approved these changes Jan 16, 2025

View reviewed changes

lib/vnet/admin_process_darwin.go Outdated Show resolved Hide resolved

fix typo in comment

10e1c96

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[vnet] windows service stub #50468

[vnet] windows service stub #50468

nklaassen commented Dec 20, 2024 •

edited

Loading

nklaassen commented Dec 20, 2024

nklaassen commented Jan 7, 2025

ravicious left a comment

ravicious Jan 8, 2025

nklaassen Jan 16, 2025

ravicious Jan 8, 2025

nklaassen Jan 16, 2025

ravicious Jan 8, 2025

nklaassen Jan 16, 2025

ravicious Jan 9, 2025

nklaassen Jan 16, 2025

codingllama left a comment

nklaassen commented Jan 10, 2025

nklaassen commented Jan 10, 2025

nklaassen commented Jan 16, 2025

codingllama left a comment

codingllama Jan 16, 2025

nklaassen Jan 16, 2025

	func newProcessManager() (*ProcessManager, context.Context) {
	ctx, cancel := context.WithCancel(context.Background())
	g, ctx := errgroup.WithContext(ctx)

	return &ProcessManager{
	g: g,
	cancel: cancel,
	}, ctx
	}

	// ProcessManager handles background tasks needed to run VNet.
	// Its semantics are similar to an error group with a context, but it cancels the context whenever
	// any task returns prematurely, that is, a task exits while the context was not canceled.
	type ProcessManager struct {
	g *errgroup.Group
	cancel context.CancelFunc
	}

	// AddCriticalBackgroundTask adds a function to the error group. [task] is expected to block until
	// the context returned by [newProcessManager] gets canceled. The context gets canceled either by
	// calling Close on [ProcessManager] or if any task returns.
	func (pm *ProcessManager) AddCriticalBackgroundTask(name string, task func() error) {
	pm.g.Go(func() error {
	err := task()
	if err == nil {
	// Make sure to always return an error so that the errgroup context is canceled.
	err = fmt.Errorf("critical task %q exited prematurely", name)
	}
	return trace.Wrap(err)
	})
	}

	// Wait blocks and waits for the background tasks to finish, which typically happens when another
	// goroutine calls Close on the process manager.
	func (pm *ProcessManager) Wait() error {
	return trace.Wrap(pm.g.Wait())
	}

	// Close stops any active background tasks by canceling the underlying context.
	func (pm *ProcessManager) Close() {
	pm.cancel()
	}

[vnet] windows service stub #50468

Are you sure you want to change the base?

[vnet] windows service stub #50468

Conversation

nklaassen commented Dec 20, 2024 • edited Loading

nklaassen commented Dec 20, 2024

nklaassen commented Jan 7, 2025

ravicious 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

codingllama left a comment

Choose a reason for hiding this comment

nklaassen commented Jan 10, 2025

nklaassen commented Jan 10, 2025

nklaassen commented Jan 16, 2025

codingllama left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nklaassen commented Dec 20, 2024 •

edited

Loading