From d37af866f55e9c8df7a80e2498b9e1cb9d9379c8 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Wed, 24 Apr 2024 14:33:05 -0700 Subject: [PATCH 01/17] [vnet] setup TUN and IPv6 on MacOS --- constants.go | 4 + lib/vnet/setup.go | 121 ++++++++++++++++ lib/vnet/setup_darwin.go | 254 +++++++++++++++++++++++++++++++++ lib/vnet/setup_other.go | 45 ++++++ tool/tsh/common/tsh.go | 7 + tool/tsh/common/vnet_darwin.go | 65 +++++++++ tool/tsh/common/vnet_other.go | 44 ++++++ 7 files changed, 540 insertions(+) create mode 100644 lib/vnet/setup.go create mode 100644 lib/vnet/setup_darwin.go create mode 100644 lib/vnet/setup_other.go create mode 100644 tool/tsh/common/vnet_darwin.go create mode 100644 tool/tsh/common/vnet_other.go diff --git a/constants.go b/constants.go index d495099fd61e0..c23edb130d2b5 100644 --- a/constants.go +++ b/constants.go @@ -849,6 +849,10 @@ const ( // until a domain name stops resolving. Its main use is to ensure no // auth instances are still running the previous major version. WaitSubCommand = "wait" + + // VnetAdminSetupSubCommand is the sub-command tsh vnet uses to perform + // a setup as a privileged user. + VnetAdminSetupSubCommand = "vnet-admin-setup" ) const ( diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go new file mode 100644 index 0000000000000..d06522c25ea63 --- /dev/null +++ b/lib/vnet/setup.go @@ -0,0 +1,121 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package vnet + +import ( + "context" + "log/slog" + "os" + + "github.com/gravitational/trace" + "golang.zx2c4.com/wireguard/tun" +) + +// Run is a blocking call to create and start Teleport VNet. +func Run(ctx context.Context) error { + ipv6Prefix, err := IPv6Prefix() + if err != nil { + return trace.Wrap(err) + } + + tun, err := CreateAndSetupTUNDevice(ctx, ipv6Prefix.String()) + if err != nil { + return trace.Wrap(err) + } + + manager, err := NewManager(ctx, &Config{ + TUNDevice: tun, + IPv6Prefix: ipv6Prefix, + }) + if err != nil { + return trace.Wrap(err) + } + + runErr := ignoreCancel(manager.Run()) + destroyErr := ignoreCancel(manager.Destroy()) + return trace.NewAggregate(runErr, destroyErr) +} + +// AdminSubcommand is the tsh subcommand that should run as root that will +// create and setup a TUN device and pass the file descriptor for that device +// over the unix socket found at socketPath. +func AdminSubcommand(ctx context.Context, socketPath, ipv6Prefix string) error { + tun, tunName, err := createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix) + if err != nil { + return trace.Wrap(err, "doing admin setup") + } + if err := sendTUNNameAndFd(socketPath, tunName, tun.File().Fd()); err != nil { + return trace.Wrap(err) + } + return nil +} + +// CreateAndSetupTUNDevice returns a virtual network device and configures the host OS to use that device for +// VNet connections. +func CreateAndSetupTUNDevice(ctx context.Context, ipv6Prefix string) (tun.Device, error) { + var ( + device tun.Device + name string + err error + ) + if os.Getuid() == 0 { + device, name, err = createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix) + } else { + device, name, err = createAndSetupTUNDeviceWithoutRoot(ctx, ipv6Prefix) + } + if err != nil { + return nil, trace.Wrap(err) + } + slog.With("device", name).InfoContext(ctx, "Created TUN device.") + return device, nil +} + +func createAndSetupTUNDeviceAsRoot(ctx context.Context, ipv6Prefix string) (tun.Device, string, error) { + tun, tunName, err := createTUNDevice(ctx) + if err != nil { + return nil, "", trace.Wrap(err) + } + + tunIPv6 := ipv6Prefix + "1" + cfg := osConfig{ + tunName: tunName, + tunIPv6: tunIPv6, + } + if err := configureOS(ctx, &cfg); err != nil { + return nil, "", trace.Wrap(err, "configuring OS") + } + + return tun, tunName, nil +} + +func createTUNDevice(ctx context.Context) (tun.Device, string, error) { + slog.DebugContext(ctx, "Creating TUN device.") + dev, err := tun.CreateTUN("utun", mtu) + if err != nil { + return nil, "", trace.Wrap(err, "creating TUN device") + } + name, err := dev.Name() + if err != nil { + return nil, "", trace.Wrap(err, "getting TUN device name") + } + return dev, name, nil +} + +type osConfig struct { + tunName string + tunIPv6 string +} diff --git a/lib/vnet/setup_darwin.go b/lib/vnet/setup_darwin.go new file mode 100644 index 0000000000000..7b2e1e9d5ba9e --- /dev/null +++ b/lib/vnet/setup_darwin.go @@ -0,0 +1,254 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +//go:build darwin +// +build darwin + +package vnet + +import ( + "context" + "errors" + "fmt" + "log/slog" + "net" + "os" + "os/exec" + "strings" + "time" + + "github.com/gravitational/trace" + "golang.org/x/sys/unix" + "golang.zx2c4.com/wireguard/tun" + + "github.com/gravitational/teleport" +) + +const ( + tunHandoverTimeout = time.Minute +) + +func createAndSetupTUNDeviceWithoutRoot(ctx context.Context, ipv6Prefix string) (tun.Device, string, error) { + slog.InfoContext(ctx, "Spawning child process as root to create and setup TUN device") + socket, socketPath, err := createUnixSocket() + if err != nil { + return nil, "", trace.Wrap(err) + } + + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + adminCommandErr := make(chan error, 1) + go func() { + adminCommandErr <- trace.Wrap(execAdminSubcommand(ctx, socketPath, ipv6Prefix)) + }() + + recvTunErr := make(chan error, 1) + var tunName string + var tunFd uintptr + go func() { + tunName, tunFd, err = recvTUNNameAndFd(ctx, socket) + recvTunErr <- trace.Wrap(err, "receiving TUN name and file descriptor") + }() + +loop: + for { + select { + case err := <-adminCommandErr: + if err != nil { + return nil, "", trace.Wrap(err) + } + case err := <-recvTunErr: + if err != nil { + return nil, "", trace.Wrap(err) + } + break loop + } + } + + tunDevice, err := tun.CreateTUNFromFile(os.NewFile(tunFd, ""), 0) + if err != nil { + return nil, "", trace.Wrap(err, "creating TUN device from file descriptor") + } + + return tunDevice, tunName, nil +} + +func execAdminSubcommand(ctx context.Context, socketPath, ipv6Prefix string) error { + executableName, err := os.Executable() + if err != nil { + return trace.Wrap(err, "getting executable path") + } + + appleScript := fmt.Sprintf(` +set executableName to "%s" +set socketPath to "%s" +set ipv6Prefix to "%s" +do shell script quoted form of executableName & `+ + `" %s --socket " & quoted form of socketPath & `+ + `" --ipv6-prefix " & quoted form of ipv6Prefix `+ + `with prompt "VNet wants to set up a virtual network device" with administrator privileges`, + executableName, socketPath, ipv6Prefix, teleport.VnetAdminSetupSubCommand) + + // The context we pass here has effect only on the password prompt being shown. Once osascript spawns the + // privileged process, canceling the context (and thus killing osascript) has no effect on the privileged + // process. + cmd := exec.CommandContext(ctx, "osascript", "-e", appleScript) + var stderr strings.Builder + cmd.Stderr = &stderr + + if err := cmd.Run(); err != nil { + var exitError *exec.ExitError + if errors.As(err, &exitError) { + stderr := stderr.String() + + // When the user closes the prompt for administrator privileges, the -128 error is returned. + // https://developer.apple.com/library/archive/documentation/AppleScript/Conceptual/AppleScriptLangGuide/reference/ASLR_error_codes.html#//apple_ref/doc/uid/TP40000983-CH220-SW2 + if strings.Contains(stderr, "-128") { + return trace.Errorf("password prompt closed by user") + } + return trace.Wrap(exitError, "admin subcommand exited, stderr: %s", stderr) + } + return trace.Wrap(err) + } + return nil +} + +func createUnixSocket() (*net.UnixListener, string, error) { + // Abuse CreateTemp to find an unused path. + f, err := os.CreateTemp("", "vnet*.sock") + if err != nil { + return nil, "", trace.Wrap(err) + } + socketPath := f.Name() + if err := f.Close(); err != nil { + return nil, "", trace.Wrap(err) + } + if err := os.Remove(socketPath); err != nil { + return nil, "", trace.Wrap(err) + } + socketAddr := &net.UnixAddr{Name: socketPath, Net: "unix"} + l, err := net.ListenUnix(socketAddr.Net, socketAddr) + if err != nil { + return nil, "", trace.Wrap(err, "creating unix socket") + } + if err := os.Chmod(socketPath, 0600); err != nil { + return nil, "", trace.Wrap(err, "setting permissions on unix socket") + } + return l, socketPath, nil +} + +// sendTUNNameAndFd sends the name of the TUN device and its open file descriptor over a unix socket, meant +// for passing the TUN from the root process which must create it to the user process. +func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { + socketAddr := &net.UnixAddr{Name: socketPath, Net: "unix"} + conn, err := net.DialUnix(socketAddr.Net, nil /*laddr*/, socketAddr) + if err != nil { + return trace.Wrap(err) + } + defer conn.Close() + + err = conn.SetDeadline(time.Now().Add(tunHandoverTimeout)) + if err != nil { + return trace.Wrap(err) + } + + // Write the device name as the main message and pass the file desciptor as out-of-band data. + rights := unix.UnixRights(int(fd)) + if _, _, err := conn.WriteMsgUnix([]byte(tunName), rights, socketAddr); err != nil { + return trace.Wrap(err, "writing to unix conn") + } + return nil +} + +// sendTUNNameAndFd receives the name of a TUN device and its open file descriptor over a unix socket, meant +// for passing the TUN from the root process which must create it to the user process. +func recvTUNNameAndFd(ctx context.Context, socket *net.UnixListener) (string, uintptr, error) { + ctx, cancel := context.WithTimeout(ctx, tunHandoverTimeout) + defer cancel() + deadline, _ := ctx.Deadline() + + err := socket.SetDeadline(deadline) + if err != nil { + return "", 0, trace.Wrap(err) + } + go func() { + <-ctx.Done() + socket.Close() + }() + + conn, err := socket.AcceptUnix() + if err != nil { + return "", 0, trace.Wrap(err) + } + go func() { + // Close the connection early to unblock reads if the context is canceled. + <-ctx.Done() + conn.Close() + }() + + msg := make([]byte, 128) + oob := make([]byte, unix.CmsgSpace(4)) // Fd is 4 bytes + n, oobn, _, _, err := conn.ReadMsgUnix(msg, oob) + if err != nil { + return "", 0, trace.Wrap(err, "reading from unix conn") + } + + // Parse the device name from the main message. + if n == 0 { + return "", 0, trace.Errorf("failed to read msg from unix conn") + } + if oobn != len(oob) { + return "", 0, trace.Errorf("failed to read out-of-band data from unix conn") + } + tunName := string(msg[:n]) + + // Parse the file descriptor from the out-of-band data. + scm, err := unix.ParseSocketControlMessage(oob) + if err != nil { + return "", 0, trace.Wrap(err, "parsing socket control message") + } + if len(scm) != 1 { + return "", 0, trace.BadParameter("expect 1 socket control message, got %d", len(scm)) + } + fds, err := unix.ParseUnixRights(&scm[0]) + if err != nil { + return "", 0, trace.Wrap(err, "parsing file descriptors") + } + if len(fds) != 1 { + return "", 0, trace.BadParameter("expected 1 file descriptor, got %d", len(fds)) + } + fd := uintptr(fds[0]) + + return tunName, fd, nil +} + +func configureOS(ctx context.Context, cfg *osConfig) error { + if cfg.tunIPv6 != "" && cfg.tunName != "" { + slog.With("device", cfg.tunName, "address", cfg.tunIPv6).InfoContext(ctx, "Setting IPv6 address for the TUN device.") + cmd := exec.CommandContext(ctx, "ifconfig", cfg.tunName, "inet6", cfg.tunIPv6, "prefixlen", "64") + if err := cmd.Run(); err != nil { + return trace.Wrap(err, "running %v", cmd.Args) + } + + slog.InfoContext(ctx, "Setting an IPv6 route for the VNet.") + cmd = exec.CommandContext(ctx, "route", "add", "-inet6", cfg.tunIPv6, "-prefixlen", "64", "-interface", cfg.tunName) + if err := cmd.Run(); err != nil { + return trace.Wrap(err, "running %v", cmd.Args) + } + } + return nil +} diff --git a/lib/vnet/setup_other.go b/lib/vnet/setup_other.go new file mode 100644 index 0000000000000..9091ed84a2904 --- /dev/null +++ b/lib/vnet/setup_other.go @@ -0,0 +1,45 @@ +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +//go:build !darwin +// +build !darwin + +package vnet + +import ( + "context" + "runtime" + + "github.com/gravitational/trace" + "golang.zx2c4.com/wireguard/tun" +) + +var ( + // VnetNotImplemented is an error indicating that VNet is not implemented on the host OS. + VnetNotImplemented = &trace.NotImplementedError{Message: "VNet is not implemented on " + runtime.GOOS} +) + +func createAndSetupTUNDeviceWithoutRoot(ctx context.Context, ipv6Prefix string) (tun.Device, string, error) { + return nil, "", trace.Wrap(VnetNotImplemented) +} + +func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { + return trace.Wrap(VnetNotImplemented) +} + +func configureOS(ctx context.Context, cfg *osConfig) error { + return trace.Wrap(VnetNotImplemented) +} diff --git a/tool/tsh/common/tsh.go b/tool/tsh/common/tsh.go index 2f7ece78dc17f..dd2bdee6e4f3b 100644 --- a/tool/tsh/common/tsh.go +++ b/tool/tsh/common/tsh.go @@ -1173,6 +1173,9 @@ func Run(ctx context.Context, args []string, opts ...CliOption) error { workloadIdentityCmd := newSVIDCommands(app) + vnetCmd := newVnetCommand(app) + vnetAdminSetupCmd := newVnetAdminSetupCommand(app) + if runtime.GOOS == constants.WindowsOS { bench.Hidden() } @@ -1535,6 +1538,10 @@ func Run(ctx context.Context, args []string, opts ...CliOption) error { err = onHeadlessApprove(&cf) case workloadIdentityCmd.issue.FullCommand(): err = workloadIdentityCmd.issue.run(&cf) + case vnetCmd.FullCommand(): + err = vnetCmd.run(&cf) + case vnetAdminSetupCmd.FullCommand(): + err = vnetAdminSetupCmd.run(&cf) default: // Handle commands that might not be available. switch { diff --git a/tool/tsh/common/vnet_darwin.go b/tool/tsh/common/vnet_darwin.go new file mode 100644 index 0000000000000..51277ed62ca5b --- /dev/null +++ b/tool/tsh/common/vnet_darwin.go @@ -0,0 +1,65 @@ +//go:build darwin +// +build darwin + +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package common + +import ( + "github.com/alecthomas/kingpin/v2" + "github.com/gravitational/teleport" + "github.com/gravitational/trace" + + "github.com/gravitational/teleport/lib/vnet" +) + +type vnetCommand struct { + *kingpin.CmdClause +} + +func newVnetCommand(app *kingpin.Application) *vnetCommand { + cmd := &vnetCommand{ + // TODO(nklaassen): unhide this when ready to ship. + CmdClause: app.Command("vnet", "Start Teleport VNet, a virtual network for TCP application access.").Hidden(), + } + return cmd +} + +func (c *vnetCommand) run(cf *CLIConf) error { + return trace.Wrap(vnet.Run(cf.Context)) +} + +type vnetAdminSetupCommand struct { + *kingpin.CmdClause + // ipv6Prefix is the IPv6 prefix for the VNet. + ipv6Prefix string + // socketPath is a path to a unix socket used for communication with the parent process. + socketPath string +} + +func newVnetAdminSetupCommand(app *kingpin.Application) *vnetAdminSetupCommand { + cmd := &vnetAdminSetupCommand{ + CmdClause: app.Command(teleport.VnetAdminSetupSubCommand, "Start the VNet admin subprocess.").Hidden(), + } + cmd.Flag("ipv6-prefix", "IPv6 prefix for the VNet").StringVar(&cmd.ipv6Prefix) + cmd.Flag("socket", "unix socket path").StringVar(&cmd.socketPath) + return cmd +} + +func (c *vnetAdminSetupCommand) run(cf *CLIConf) error { + return trace.Wrap(vnet.AdminSubcommand(cf.Context, c.socketPath, c.ipv6Prefix)) +} diff --git a/tool/tsh/common/vnet_other.go b/tool/tsh/common/vnet_other.go new file mode 100644 index 0000000000000..9d047fd6b98db --- /dev/null +++ b/tool/tsh/common/vnet_other.go @@ -0,0 +1,44 @@ +//go:build !darwin +// +build !darwin + +// Teleport +// Copyright (C) 2024 Gravitational, Inc. +// +// This program is free software: you can redistribute it and/or modify +// it under the terms of the GNU Affero General Public License as published by +// the Free Software Foundation, either version 3 of the License, or +// (at your option) any later version. +// +// This program is distributed in the hope that it will be useful, +// but WITHOUT ANY WARRANTY; without even the implied warranty of +// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the +// GNU Affero General Public License for more details. +// +// You should have received a copy of the GNU Affero General Public License +// along with this program. If not, see . + +package common + +import ( + "github.com/alecthomas/kingpin/v2" + "github.com/gravitational/trace" + + "github.com/gravitational/teleport/lib/vnet" +) + +func newVnetCommand(app *kingpin.Application) vnetNotSupported { + return vnetNotSupported{} +} + +func newVnetAdminSetupCommand(app *kingpin.Application) vnetNotSupported { + return vnetNotSupported{} +} + +type vnetNotSupported struct{} + +func (vnetNotSupported) FullCommand() string { + return "" +} +func (vnetNotSupported) run(*CLIConf) error { + return trace.Wrap(vnet.VnetNotImplemented) +} From f4cfdebf166cf1e140c1b1bdad86e1a1131392d5 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 26 Apr 2024 08:52:19 -0700 Subject: [PATCH 02/17] edit error message MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-authored-by: Rafał Cieślak --- lib/vnet/setup.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index d06522c25ea63..95f58d0166746 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -56,7 +56,7 @@ func Run(ctx context.Context) error { func AdminSubcommand(ctx context.Context, socketPath, ipv6Prefix string) error { tun, tunName, err := createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix) if err != nil { - return trace.Wrap(err, "doing admin setup") + return trace.Wrap(err, "performing admin setup") } if err := sendTUNNameAndFd(socketPath, tunName, tun.File().Fd()); err != nil { return trace.Wrap(err) From fa6c9cface6522105b6733f6c09640aa798ee00c Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 26 Apr 2024 09:22:06 -0700 Subject: [PATCH 03/17] add comment on running vnet as root --- lib/vnet/setup.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 95f58d0166746..6be67a3ba78ba 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -73,6 +73,8 @@ func CreateAndSetupTUNDevice(ctx context.Context, ipv6Prefix string) (tun.Device err error ) if os.Getuid() == 0 { + // We can get here if the user runs `tsh vnet` as root, but it is not in the expected path when + // started as a regular user, AdminSubcommand directly calls createAndSetupTUNDeviceAsRoot. device, name, err = createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix) } else { device, name, err = createAndSetupTUNDeviceWithoutRoot(ctx, ipv6Prefix) From 7422a35c6bbc5c92cafae3cced31328774552015 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 29 Apr 2024 07:12:10 -0700 Subject: [PATCH 04/17] better context cancellation handling --- lib/vnet/setup.go | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 6be67a3ba78ba..19f0944c24159 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -22,6 +22,7 @@ import ( "os" "github.com/gravitational/trace" + "golang.org/x/sync/errgroup" "golang.zx2c4.com/wireguard/tun" ) @@ -45,9 +46,13 @@ func Run(ctx context.Context) error { return trace.Wrap(err) } - runErr := ignoreCancel(manager.Run()) - destroyErr := ignoreCancel(manager.Destroy()) - return trace.NewAggregate(runErr, destroyErr) + g, ctx := errgroup.WithContext(ctx) + g.Go(manager.Run) + g.Go(func() error { + <-ctx.Done() + return trace.Wrap(manager.Destroy()) + }) + return trace.Wrap(g.Wait()) } // AdminSubcommand is the tsh subcommand that should run as root that will From b82edbf02721c343e765b9da93303e61c38fbb68 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 29 Apr 2024 09:38:06 -0700 Subject: [PATCH 05/17] pass context to manager.Run --- lib/vnet/setup.go | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 19f0944c24159..bb0fcc710d3ca 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -38,7 +38,7 @@ func Run(ctx context.Context) error { return trace.Wrap(err) } - manager, err := NewManager(ctx, &Config{ + manager, err := NewManager(&Config{ TUNDevice: tun, IPv6Prefix: ipv6Prefix, }) @@ -47,10 +47,12 @@ func Run(ctx context.Context) error { } g, ctx := errgroup.WithContext(ctx) - g.Go(manager.Run) + g.Go(func() error { return trace.Wrap(manager.Run(ctx)) }) g.Go(func() error { <-ctx.Done() - return trace.Wrap(manager.Destroy()) + tunErr := tun.Close() + destroyErr := manager.Destroy() + return trace.NewAggregate(tunErr, destroyErr) }) return trace.Wrap(g.Wait()) } From 01ed3f4d368a30ad7dc950be1a5ff9f202da3166 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Tue, 30 Apr 2024 09:54:05 -0700 Subject: [PATCH 06/17] remove unnecessary slog.With --- lib/vnet/setup.go | 2 +- lib/vnet/setup_darwin.go | 2 +- lib/vnet/vnet.go | 4 ++-- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index bb0fcc710d3ca..01a20af7797d7 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -89,7 +89,7 @@ func CreateAndSetupTUNDevice(ctx context.Context, ipv6Prefix string) (tun.Device if err != nil { return nil, trace.Wrap(err) } - slog.With("device", name).InfoContext(ctx, "Created TUN device.") + slog.InfoContext(ctx, "Created TUN device.", "device", name) return device, nil } diff --git a/lib/vnet/setup_darwin.go b/lib/vnet/setup_darwin.go index 7b2e1e9d5ba9e..2bf02b7999c46 100644 --- a/lib/vnet/setup_darwin.go +++ b/lib/vnet/setup_darwin.go @@ -238,7 +238,7 @@ func recvTUNNameAndFd(ctx context.Context, socket *net.UnixListener) (string, ui func configureOS(ctx context.Context, cfg *osConfig) error { if cfg.tunIPv6 != "" && cfg.tunName != "" { - slog.With("device", cfg.tunName, "address", cfg.tunIPv6).InfoContext(ctx, "Setting IPv6 address for the TUN device.") + slog.InfoContext(ctx, "Setting IPv6 address for the TUN device.", "device", cfg.tunName, "address", cfg.tunIPv6) cmd := exec.CommandContext(ctx, "ifconfig", cfg.tunName, "inet6", cfg.tunIPv6, "prefixlen", "64") if err := cmd.Run(); err != nil { return trace.Wrap(err, "running %v", cmd.Args) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 498b3adfae578..a10935a429b66 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -214,7 +214,7 @@ func installVnetRoutes(stack *stack.Stack) error { // Run starts the VNet. func (m *Manager) Run(ctx context.Context) error { - m.slog.With("ipv6_prefix", m.ipv6Prefix).InfoContext(ctx, "Running Teleport VNet.") + m.slog.InfoContext(ctx, "Running Teleport VNet.", "ipv6_prefix", m.ipv6Prefix) g, ctx := errgroup.WithContext(ctx) g.Go(func() error { return m.statsHandler(ctx) }) g.Go(func() error { @@ -254,7 +254,7 @@ func (m *Manager) handleTCP(req *tcp.ForwarderRequest) { handler, ok := m.getTCPHandler(id.LocalAddress) if !ok { - slog.With("addr", id.LocalAddress).DebugContext(ctx, "No handler for address.") + slog.DebugContext(ctx, "No handler for address.", "addr", id.LocalAddress) return } From d79309aeca2cb271ada864c8105edbc243d761e5 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 2 May 2024 10:43:49 -0700 Subject: [PATCH 07/17] Apply suggestions from code review Co-authored-by: Isaiah Becker-Mayer --- lib/vnet/setup.go | 3 ++- lib/vnet/setup_darwin.go | 4 ++-- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 01a20af7797d7..55a393297eade 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -81,7 +81,8 @@ func CreateAndSetupTUNDevice(ctx context.Context, ipv6Prefix string) (tun.Device ) if os.Getuid() == 0 { // We can get here if the user runs `tsh vnet` as root, but it is not in the expected path when - // started as a regular user, AdminSubcommand directly calls createAndSetupTUNDeviceAsRoot. + // started as a regular user. Typically we expect `tsh vnet` to be run as a non-root user, and for + // AdminSubcommand to directly call createAndSetupTUNDeviceAsRoot. device, name, err = createAndSetupTUNDeviceAsRoot(ctx, ipv6Prefix) } else { device, name, err = createAndSetupTUNDeviceWithoutRoot(ctx, ipv6Prefix) diff --git a/lib/vnet/setup_darwin.go b/lib/vnet/setup_darwin.go index 2bf02b7999c46..c9937e956fd30 100644 --- a/lib/vnet/setup_darwin.go +++ b/lib/vnet/setup_darwin.go @@ -145,7 +145,7 @@ func createUnixSocket() (*net.UnixListener, string, error) { if err != nil { return nil, "", trace.Wrap(err, "creating unix socket") } - if err := os.Chmod(socketPath, 0600); err != nil { + if err := os.Chmod(socketPath, 0o600); err != nil { return nil, "", trace.Wrap(err, "setting permissions on unix socket") } return l, socketPath, nil @@ -174,7 +174,7 @@ func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { return nil } -// sendTUNNameAndFd receives the name of a TUN device and its open file descriptor over a unix socket, meant +// recvTUNNameAndFd receives the name of a TUN device and its open file descriptor over a unix socket, meant // for passing the TUN from the root process which must create it to the user process. func recvTUNNameAndFd(ctx context.Context, socket *net.UnixListener) (string, uintptr, error) { ctx, cancel := context.WithTimeout(ctx, tunHandoverTimeout) From 864ee56a115959ac087a3448455044a565fb7381 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 2 May 2024 10:46:23 -0700 Subject: [PATCH 08/17] rename notimplemented error --- lib/vnet/setup_other.go | 10 +++++----- tool/tsh/common/vnet_other.go | 2 +- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/lib/vnet/setup_other.go b/lib/vnet/setup_other.go index 9091ed84a2904..c18b1f227a98b 100644 --- a/lib/vnet/setup_other.go +++ b/lib/vnet/setup_other.go @@ -28,18 +28,18 @@ import ( ) var ( - // VnetNotImplemented is an error indicating that VNet is not implemented on the host OS. - VnetNotImplemented = &trace.NotImplementedError{Message: "VNet is not implemented on " + runtime.GOOS} + // ErrVnetNotImplemented is an error indicating that VNet is not implemented on the host OS. + ErrVnetNotImplemented = &trace.NotImplementedError{Message: "VNet is not implemented on " + runtime.GOOS} ) func createAndSetupTUNDeviceWithoutRoot(ctx context.Context, ipv6Prefix string) (tun.Device, string, error) { - return nil, "", trace.Wrap(VnetNotImplemented) + return nil, "", trace.Wrap(ErrVnetNotImplemented) } func sendTUNNameAndFd(socketPath, tunName string, fd uintptr) error { - return trace.Wrap(VnetNotImplemented) + return trace.Wrap(ErrVnetNotImplemented) } func configureOS(ctx context.Context, cfg *osConfig) error { - return trace.Wrap(VnetNotImplemented) + return trace.Wrap(ErrVnetNotImplemented) } diff --git a/tool/tsh/common/vnet_other.go b/tool/tsh/common/vnet_other.go index 9d047fd6b98db..4342befeb8d2b 100644 --- a/tool/tsh/common/vnet_other.go +++ b/tool/tsh/common/vnet_other.go @@ -40,5 +40,5 @@ func (vnetNotSupported) FullCommand() string { return "" } func (vnetNotSupported) run(*CLIConf) error { - return trace.Wrap(vnet.VnetNotImplemented) + return trace.Wrap(vnet.ErrVnetNotImplemented) } From dffd4d3eed0e31a4b4da2d39fc66afdf9140040d Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 2 May 2024 11:00:02 -0700 Subject: [PATCH 09/17] return aggregate of all errors --- lib/vnet/setup.go | 27 +++++++++++++++++++++++---- 1 file changed, 23 insertions(+), 4 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 55a393297eade..84cd304e2760f 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -46,15 +46,34 @@ func Run(ctx context.Context) error { return trace.Wrap(err) } + ctx, cancel := context.WithCancel(ctx) + defer cancel() + + allErrors := make(chan error, 3) g, ctx := errgroup.WithContext(ctx) - g.Go(func() error { return trace.Wrap(manager.Run(ctx)) }) g.Go(func() error { + // Make sure to cancel the context if manager.Run terminates for any reason. + defer cancel() + err := trace.Wrap(manager.Run(ctx)) + allErrors <- err + return err + }) + g.Go(func() error { + // Wait until the context is canceled, either from receiving SIGTERM or from manager.Run terminating. <-ctx.Done() - tunErr := tun.Close() - destroyErr := manager.Destroy() + + tunErr := trace.Wrap(tun.Close()) + allErrors <- tunErr + + destroyErr := trace.Wrap(manager.Destroy()) + allErrors <- destroyErr + return trace.NewAggregate(tunErr, destroyErr) }) - return trace.Wrap(g.Wait()) + // Deliberately ignoring the error from g.Wait() to return an aggregate of all errors. + _ = g.Wait() + close(allErrors) + return trace.NewAggregateFromChannel(allErrors, context.Background()) } // AdminSubcommand is the tsh subcommand that should run as root that will From c1de85933cde91b1e58bb28be2c37a4fc312d666 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Thu, 2 May 2024 11:08:20 -0700 Subject: [PATCH 10/17] stop abusing os.CreateTemp --- lib/vnet/setup_darwin.go | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/lib/vnet/setup_darwin.go b/lib/vnet/setup_darwin.go index c9937e956fd30..0bc8e76e55722 100644 --- a/lib/vnet/setup_darwin.go +++ b/lib/vnet/setup_darwin.go @@ -27,9 +27,11 @@ import ( "net" "os" "os/exec" + "path/filepath" "strings" "time" + "github.com/google/uuid" "github.com/gravitational/trace" "golang.org/x/sys/unix" "golang.zx2c4.com/wireguard/tun" @@ -128,18 +130,7 @@ do shell script quoted form of executableName & `+ } func createUnixSocket() (*net.UnixListener, string, error) { - // Abuse CreateTemp to find an unused path. - f, err := os.CreateTemp("", "vnet*.sock") - if err != nil { - return nil, "", trace.Wrap(err) - } - socketPath := f.Name() - if err := f.Close(); err != nil { - return nil, "", trace.Wrap(err) - } - if err := os.Remove(socketPath); err != nil { - return nil, "", trace.Wrap(err) - } + socketPath := filepath.Join(os.TempDir(), "vnet"+uuid.NewString()+".sock") socketAddr := &net.UnixAddr{Name: socketPath, Net: "unix"} l, err := net.ListenUnix(socketAddr.Net, socketAddr) if err != nil { From a0e773d687aab147f8ca60b8225075437180c4dc Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 3 May 2024 13:44:03 -0700 Subject: [PATCH 11/17] reliably terminate on context cancellation --- lib/vnet/setup.go | 30 +--------------------------- lib/vnet/vnet.go | 46 ++++++++++++++++++++++++++++++++----------- lib/vnet/vnet_test.go | 5 ----- 3 files changed, 35 insertions(+), 46 deletions(-) diff --git a/lib/vnet/setup.go b/lib/vnet/setup.go index 84cd304e2760f..4d575a08e1c78 100644 --- a/lib/vnet/setup.go +++ b/lib/vnet/setup.go @@ -22,7 +22,6 @@ import ( "os" "github.com/gravitational/trace" - "golang.org/x/sync/errgroup" "golang.zx2c4.com/wireguard/tun" ) @@ -46,34 +45,7 @@ func Run(ctx context.Context) error { return trace.Wrap(err) } - ctx, cancel := context.WithCancel(ctx) - defer cancel() - - allErrors := make(chan error, 3) - g, ctx := errgroup.WithContext(ctx) - g.Go(func() error { - // Make sure to cancel the context if manager.Run terminates for any reason. - defer cancel() - err := trace.Wrap(manager.Run(ctx)) - allErrors <- err - return err - }) - g.Go(func() error { - // Wait until the context is canceled, either from receiving SIGTERM or from manager.Run terminating. - <-ctx.Done() - - tunErr := trace.Wrap(tun.Close()) - allErrors <- tunErr - - destroyErr := trace.Wrap(manager.Destroy()) - allErrors <- destroyErr - - return trace.NewAggregate(tunErr, destroyErr) - }) - // Deliberately ignoring the error from g.Wait() to return an aggregate of all errors. - _ = g.Wait() - close(allErrors) - return trace.NewAggregateFromChannel(allErrors, context.Background()) + return trace.Wrap(manager.Run(ctx)) } // AdminSubcommand is the tsh subcommand that should run as root that will diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index a10935a429b66..f688b075a1e83 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -144,8 +144,9 @@ type tcpHandler interface { handleTCP(context.Context, tcpConnector) error } -// NewManager creates a new VNet manager with the given configuration and root -// context. Call Run() on the returned manager to start the VNet. +// NewManager creates a new VNet manager with the given configuration and root context. It takes ownership of +// [cfg.TUNDevice] and will handle closing it before Run() returns. Call Run() on the returned manager to +// start the VNet. func NewManager(cfg *Config) (*Manager, error) { if err := cfg.CheckAndSetDefaults(); err != nil { return nil, trace.Wrap(err) @@ -212,24 +213,45 @@ func installVnetRoutes(stack *stack.Stack) error { return nil } -// Run starts the VNet. +// Run starts the VNet. It blocks until [ctx] is cancelled, at which point it closes the link endpoint, waits +// for all goroutines to terminate, and destroys the networking stack. func (m *Manager) Run(ctx context.Context) error { m.slog.InfoContext(ctx, "Running Teleport VNet.", "ipv6_prefix", m.ipv6Prefix) + + ctx, cancel := context.WithCancel(ctx) + + allErrors := make(chan error, 3) g, ctx := errgroup.WithContext(ctx) - g.Go(func() error { return m.statsHandler(ctx) }) g.Go(func() error { - return forwardBetweenTunAndNetstack(ctx, m.tun, m.linkEndpoint) + defer cancel() + err := trace.Wrap(m.statsHandler(ctx)) + allErrors <- err + return err + }) + g.Go(func() error { + defer cancel() + err := forwardBetweenTunAndNetstack(ctx, m.tun, m.linkEndpoint) + allErrors <- err + return err + }) + g.Go(func() error { + // When the context is cancelled for any reason, the caller may have cancelled it or one of the other + // concurrent tasks, destroy everything and quit. + <-ctx.Done() + close(m.destroyed) + m.linkEndpoint.Close() + err := trace.Wrap(m.tun.Close(), "closing TUN device") + allErrors <- err + return err }) - return trace.Wrap(g.Wait()) -} -// Destroy closes the link endpoint, waits for all goroutines to terminate, and destroys the networking stack. -func (m *Manager) Destroy() error { - close(m.destroyed) - m.linkEndpoint.Close() + // Deliberately ignoring the error from g.Wait() to return an aggregate of all errors. + _ = g.Wait() m.wg.Wait() m.stack.Destroy() - return nil + + close(allErrors) + return trace.NewAggregateFromChannel(allErrors, context.Background()) } func (m *Manager) handleTCP(req *tcp.ForwarderRequest) { diff --git a/lib/vnet/vnet_test.go b/lib/vnet/vnet_test.go index 601b003ea89a1..b3c6d560f0ae2 100644 --- a/lib/vnet/vnet_test.go +++ b/lib/vnet/vnet_test.go @@ -117,11 +117,6 @@ func newTestPack(t *testing.T, ctx context.Context) *testPack { } return nil }, - Terminate: func() error { - tunErr := tun2.Close() - destroyErr := manager.Destroy() - return trace.NewAggregate(tunErr, destroyErr) - }, }) return &testPack{ From 48a0becf9bbbbb5aaee138fc81de3c9fd3bae316 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Fri, 3 May 2024 14:20:21 -0700 Subject: [PATCH 12/17] fix lint --- lib/vnet/vnet.go | 4 ++-- tool/tsh/common/vnet_darwin.go | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index f688b075a1e83..5a5fcf6caf322 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -213,7 +213,7 @@ func installVnetRoutes(stack *stack.Stack) error { return nil } -// Run starts the VNet. It blocks until [ctx] is cancelled, at which point it closes the link endpoint, waits +// Run starts the VNet. It blocks until [ctx] is canceled, at which point it closes the link endpoint, waits // for all goroutines to terminate, and destroys the networking stack. func (m *Manager) Run(ctx context.Context) error { m.slog.InfoContext(ctx, "Running Teleport VNet.", "ipv6_prefix", m.ipv6Prefix) @@ -235,7 +235,7 @@ func (m *Manager) Run(ctx context.Context) error { return err }) g.Go(func() error { - // When the context is cancelled for any reason, the caller may have cancelled it or one of the other + // When the context is canceled for any reason, the caller may have canceled it or one of the other // concurrent tasks, destroy everything and quit. <-ctx.Done() close(m.destroyed) diff --git a/tool/tsh/common/vnet_darwin.go b/tool/tsh/common/vnet_darwin.go index 51277ed62ca5b..0c8760584e98c 100644 --- a/tool/tsh/common/vnet_darwin.go +++ b/tool/tsh/common/vnet_darwin.go @@ -21,9 +21,9 @@ package common import ( "github.com/alecthomas/kingpin/v2" - "github.com/gravitational/teleport" "github.com/gravitational/trace" + "github.com/gravitational/teleport" "github.com/gravitational/teleport/lib/vnet" ) From ee5f5c814fd5f4db620082db5b9475261294789d Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 6 May 2024 09:00:54 -0700 Subject: [PATCH 13/17] terminate conn signal notifier goroutine when conn is already closed --- lib/vnet/vnet.go | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 5a5fcf6caf322..2e3cf9304cabc 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -23,6 +23,7 @@ import ( "fmt" "io" "log/slog" + "net" "os" "os/signal" "sync" @@ -297,11 +298,15 @@ func (m *Manager) handleTCP(req *tcp.ForwarderRequest) { endpoint.SocketOptions().SetKeepAlive(true) - conn := gonet.NewTCPConn(&wq, endpoint) + conn, connClosed := newConnWithCloseNotifier(gonet.NewTCPConn(&wq, endpoint)) + m.wg.Add(1) go func() { defer m.wg.Done() select { + case <-connClosed: + // Conn is already being closed, nothing to do. + return case <-notifyCh: slog.DebugContext(ctx, "Got HUP or ERR, closing TCP conn.") case <-m.destroyed: @@ -471,3 +476,22 @@ func u32ToBytes(i uint32) []byte { bytes[3] = byte(i >> 0) return bytes } + +// newConnWithCloseNotifier returns a net.Conn and a channel that will be closed when the conn is closed. +func newConnWithCloseNotifier(conn *gonet.TCPConn) (net.Conn, <-chan struct{}) { + ch := make(chan struct{}) + return &connWithCloseNotifier{ + TCPConn: conn, + closeOnce: sync.OnceFunc(func() { close(ch) }), + }, ch +} + +type connWithCloseNotifier struct { + *gonet.TCPConn + closeOnce func() +} + +func (c *connWithCloseNotifier) Close() error { + c.closeOnce() + return c.TCPConn.Close() +} From ebef36afdf3c9b12b562fc7e74fc1109c8a1ac1a Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 6 May 2024 16:17:10 -0700 Subject: [PATCH 14/17] add comments --- lib/vnet/vnet.go | 65 ++++++++++++++++++++++++++++++++++++------------ 1 file changed, 49 insertions(+), 16 deletions(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 2e3cf9304cabc..31f28ccec45f3 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -47,8 +47,10 @@ import ( ) const ( - nicID = 1 - mtu = 1500 + nicID = 1 + mtu = 1500 + tcpReceiveBufferSize = 0 // 0 means a default will be used. + maxInFlightTCPConnectionAttempts = 1024 ) // Config holds configuration parameters for the VNet. @@ -115,14 +117,35 @@ type TUNDevice interface { // Manager holds configuration and state for the VNet. type Manager struct { - tun TUNDevice - stack *stack.Stack + // stack is the gVisor networking stack. + stack *stack.Stack + + // tun is the OS TUN device. Incoming IP/L3 packets will be copied from here to [linkEndpoint], and + // outgoing packets from [linkEndpoint] will be written here. + tun TUNDevice + + // linkEndpoint is the gVisor-side endpoint that emulates the OS TUN device. All incoming IP/L3 packets + // from the OS TUN device will be injected as inbound packets to this endpoint to be processed by the + // gVisor netstack which ultimately calls the TCP or UDP protocol handler. When the protocol handler + // writes packets to the gVisor stack to an address assigned to this endpoint, they will be written to + // this endpoint, and then copied from this endpoint to the OS TUN device. linkEndpoint *channel.Endpoint - ipv6Prefix tcpip.Address - destroyed chan struct{} - wg sync.WaitGroup - state state - slog *slog.Logger + + // ipv6Prefix holds the 96-bit prefix that will be used for all IPv6 addresses assigned in the VNet. + ipv6Prefix tcpip.Address + + // destroyed is a channel that will be closed when the VNet is in the process of being destroyed. + // All goroutines should terminate quickly after either this is closed or the context passed to + // [Manager.Run] is cancelled. + destroyed chan struct{} + // wg is a [sync.WaitGroup] that keeps track of all running goroutines started by the [Manager]. + wg sync.WaitGroup + + // state holds all mutable state for the Manager, it is currently protect by a single RWMutex, this could + // be optimized as necessary. + state state + + slog *slog.Logger } type state struct { @@ -173,11 +196,7 @@ func NewManager(cfg *Config) (*Manager, error) { slog: slog, } - const ( - tcpReceiveBufferSize = 0 // 0 means a default will be used. - maxInFlightConnectionAttempts = 1024 - ) - tcpForwarder := tcp.NewForwarder(m.stack, tcpReceiveBufferSize, maxInFlightConnectionAttempts, m.handleTCP) + tcpForwarder := tcp.NewForwarder(m.stack, tcpReceiveBufferSize, maxInFlightTCPConnectionAttempts, m.handleTCP) m.stack.SetTransportProtocolHandler(tcp.ProtocolNumber, tcpForwarder.HandlePacket) return m, nil @@ -224,31 +243,43 @@ func (m *Manager) Run(ctx context.Context) error { allErrors := make(chan error, 3) g, ctx := errgroup.WithContext(ctx) g.Go(func() error { + // Make sure to cancel the context in case this exits prematurely with a nil error. defer cancel() err := trace.Wrap(m.statsHandler(ctx)) allErrors <- err return err }) g.Go(func() error { + // Make sure to cancel the context in case this exits prematurely with a nil error. defer cancel() err := forwardBetweenTunAndNetstack(ctx, m.tun, m.linkEndpoint) allErrors <- err return err }) g.Go(func() error { - // When the context is canceled for any reason, the caller may have canceled it or one of the other - // concurrent tasks, destroy everything and quit. + // When the context is canceled for any reason (the caller or one of the other concurrent tasks may + // have canceled it) destroy everything and quit. <-ctx.Done() + + // In-flight connections should start terminating after closing [m.destroyed]. close(m.destroyed) + + // Close the link endpoint and the TUN, this should cause [forwardBetweenTunAndNetstack] to terminate + // if it hasn't already. m.linkEndpoint.Close() err := trace.Wrap(m.tun.Close(), "closing TUN device") + allErrors <- err return err }) // Deliberately ignoring the error from g.Wait() to return an aggregate of all errors. _ = g.Wait() + + // Wait for all connections and goroutines to clean themselves up. m.wg.Wait() + + // Now we can destroy the gVisor networking stack and wait for all its goroutines to terminate. m.stack.Destroy() close(allErrors) @@ -256,6 +287,7 @@ func (m *Manager) Run(ctx context.Context) error { } func (m *Manager) handleTCP(req *tcp.ForwarderRequest) { + // Add 1 to the waitgroup because the networking stack runs this in its own goroutine. m.wg.Add(1) defer m.wg.Done() @@ -263,6 +295,7 @@ func (m *Manager) handleTCP(req *tcp.ForwarderRequest) { defer cancel() // Clients of *tcp.ForwarderRequest must eventually call Complete on it exactly once. + // [req] consumes 1 of [maxInFlightTCPConnectionAttempts] until [req.Complete] is called. var completed bool defer func() { if !completed { From fc0e77f3e5a6f56ef8e8c1b8946af9c1695b2fa7 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 6 May 2024 16:20:39 -0700 Subject: [PATCH 15/17] fix typo in comment --- lib/vnet/vnet.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 31f28ccec45f3..527f057ccb851 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -136,7 +136,7 @@ type Manager struct { // destroyed is a channel that will be closed when the VNet is in the process of being destroyed. // All goroutines should terminate quickly after either this is closed or the context passed to - // [Manager.Run] is cancelled. + // [Manager.Run] is canceled. destroyed chan struct{} // wg is a [sync.WaitGroup] that keeps track of all running goroutines started by the [Manager]. wg sync.WaitGroup From 36f48ec76c812a335a8444c40728ae5114507f32 Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 6 May 2024 17:10:20 -0700 Subject: [PATCH 16/17] skip stats handler on windows --- lib/vnet/vnet.go | 20 +++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 527f057ccb851..420c045cdcf0f 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -26,6 +26,7 @@ import ( "net" "os" "os/signal" + "runtime" "sync" "syscall" @@ -44,6 +45,7 @@ import ( "gvisor.dev/gvisor/pkg/waiter" "github.com/gravitational/teleport" + "github.com/gravitational/teleport/api/constants" ) const ( @@ -242,13 +244,17 @@ func (m *Manager) Run(ctx context.Context) error { allErrors := make(chan error, 3) g, ctx := errgroup.WithContext(ctx) - g.Go(func() error { - // Make sure to cancel the context in case this exits prematurely with a nil error. - defer cancel() - err := trace.Wrap(m.statsHandler(ctx)) - allErrors <- err - return err - }) + if runtime.GOOS != constants.WindowsOS { + // Windows doesn't have any appropriate OS signals we can handle to print stats. This is just for + // debug, and we're not supporting Windows yet, so it's not that important. + g.Go(func() error { + // Make sure to cancel the context in case this exits prematurely with a nil error. + defer cancel() + err := trace.Wrap(m.statsHandler(ctx)) + allErrors <- err + return err + }) + } g.Go(func() error { // Make sure to cancel the context in case this exits prematurely with a nil error. defer cancel() From 1b88e36cba8b4ffaea7ec8a6010d05bbdc83e0bc Mon Sep 17 00:00:00 2001 From: Nic Klaassen Date: Mon, 6 May 2024 18:14:13 -0700 Subject: [PATCH 17/17] remove stats handler --- lib/vnet/vnet.go | 33 +-------------------------------- 1 file changed, 1 insertion(+), 32 deletions(-) diff --git a/lib/vnet/vnet.go b/lib/vnet/vnet.go index 420c045cdcf0f..86830685426e8 100644 --- a/lib/vnet/vnet.go +++ b/lib/vnet/vnet.go @@ -20,15 +20,10 @@ import ( "context" "crypto/rand" "errors" - "fmt" "io" "log/slog" "net" - "os" - "os/signal" - "runtime" "sync" - "syscall" "github.com/gravitational/trace" "golang.org/x/sync/errgroup" @@ -45,7 +40,6 @@ import ( "gvisor.dev/gvisor/pkg/waiter" "github.com/gravitational/teleport" - "github.com/gravitational/teleport/api/constants" ) const ( @@ -242,19 +236,8 @@ func (m *Manager) Run(ctx context.Context) error { ctx, cancel := context.WithCancel(ctx) - allErrors := make(chan error, 3) + allErrors := make(chan error, 2) g, ctx := errgroup.WithContext(ctx) - if runtime.GOOS != constants.WindowsOS { - // Windows doesn't have any appropriate OS signals we can handle to print stats. This is just for - // debug, and we're not supporting Windows yet, so it's not that important. - g.Go(func() error { - // Make sure to cancel the context in case this exits prematurely with a nil error. - defer cancel() - err := trace.Wrap(m.statsHandler(ctx)) - allErrors <- err - return err - }) - } g.Go(func() error { // Make sure to cancel the context in case this exits prematurely with a nil error. defer cancel() @@ -391,20 +374,6 @@ func (m *Manager) assignTCPHandler(handler tcpHandler) (tcpip.Address, error) { return addr, nil } -func (m *Manager) statsHandler(ctx context.Context) error { - ch := make(chan os.Signal, 1) - signal.Notify(ch, syscall.SIGUSR1) - for { - select { - case <-ctx.Done(): - return ctx.Err() - case <-ch: - } - stats := m.stack.Stats() - fmt.Printf("%+v\n", stats) - } -} - func forwardBetweenTunAndNetstack(ctx context.Context, tun TUNDevice, linkEndpoint *channel.Endpoint) error { slog.DebugContext(ctx, "Forwarding IP packets between OS and VNet.") g, ctx := errgroup.WithContext(ctx)