Add `teleport networking` subprocess for port/agent/x11 forwarding #43756

Joerger · 2024-07-02T17:57:23Z

Add a unified networking subprocess to handle port/agent/x11 forwarding requests. This subprocess is designed to run as the local user being connected to, including any modifications done to that user via the node's PAM stack.

Fixes #17029

Note: this also fixes agent/port forwarding on new auto-provisioned users, which previously failed on the first attempt (or always with insecure-drop). This worked with X11 forwarding before with a more intricate solution, which has been replaced with the simpler "run-as-user" method.

Closes #44479
Closes #43623

github-actions · 2024-07-04T00:53:21Z

The PR changelog entry failed validation: Changelog entry not found in the PR body. Please add a "no-changelog" label to the PR, or changelog lines starting with changelog: followed by the changelog entries for the PR.

espadolini

lib/sshutils/networking would benefit from some package-level docstring to explain the IPC protocol, since socketpairs passed around like this can be a bit confusing for the reader.

lib/sshutils/networking/networking.go

lib/srv/reexec.go

espadolini · 2024-07-04T11:33:32Z

lib/srv/reexec.go

+			var err error
+			err2 := conn.Control(func(descriptor uintptr) {
+				// Disable address reuse to prevent socket replacement.
+				err = syscall.SetsockoptInt(int(descriptor), syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 0)


Is SO_REUSEADDR ever enabled by default?

Re-tested this and it seems it's only enabled by default for tcp listeners.

Is this necessary? If so, for what? SO_REUSEADDR allows listeners to be opened on top of sockets in TIME_WAIT state, opening multiple listeners on the same bind address at the same time is done with SO_REUSEPORT.

I'm not sure, I've preserved this from the remote port forwarding implementation. @atburke is this check necessary for some purpose or can it be removed?

I believe SO_REUSEADDR was added here to prevent some malicious process from being able to hijack the address. You'd probably be able to get a more complete answer from @jentfoo.

lib/srv/reexec.go

espadolini · 2024-07-17T12:02:05Z

lib/srv/reexec.go

+			var err error
+			err2 := conn.Control(func(descriptor uintptr) {
+				// Disable address reuse to prevent socket replacement.
+				err = syscall.SetsockoptInt(int(descriptor), syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 0)


Is this necessary? If so, for what? SO_REUSEADDR allows listeners to be opened on top of sockets in TIME_WAIT state, opening multiple listeners on the same bind address at the same time is done with SO_REUSEPORT.

espadolini · 2024-07-17T12:06:57Z

lib/srv/reexec.go

+				// direct-streamlocal@openssh.com extension, we should revisit this multithreading limitation
+				// to prevent performance degradation.


for future readers: possible options are:

doing manual async operations with nonblocking connect and select/epoll

opening some amount of goroutines that lock the OS thread and open a pam context, and fanning out operations to be done in one of these PAM-enabled threads

espadolini · 2024-07-17T12:08:54Z

lib/srv/reexec.go

+				// There are currently no known issues with tcp listen/dial in a multithreaded PAM context.
+				go handleNetworkingRequest(ctx, controlConn, req)


Until someone comes along and really wants to use a random pam module that does network namespacing 😬

lib/sshutils/networking/networking.go

atburke · 2024-07-20T00:11:12Z

lib/srv/reexec.go

+			var err error
+			err2 := conn.Control(func(descriptor uintptr) {
+				// Disable address reuse to prevent socket replacement.
+				err = syscall.SetsockoptInt(int(descriptor), syscall.SOL_SOCKET, syscall.SO_REUSEADDR, 0)


I believe SO_REUSEADDR was added here to prevent some malicious process from being able to hijack the address. You'd probably be able to get a more complete answer from @jentfoo.

espadolini

Still 100% unconvinced about the utility of unsetting SO_REUSEADDR or of validateListenerSocket in general.

lib/sshutils/networking/networking.go

lib/srv/reexec.go

…nce to connect to it.

…) on reading from the closed process.

Joerger · 2024-08-13T17:58:04Z

@rosstimothy All tests are passing, just needs a flaky test skip. I double checked that TestRootNetworkingCommand isn't flaky, it just ends with an error when the flaky test detector times out at 10m.

rosstimothy · 2024-08-13T21:39:10Z

/excludeflake *

public-teleport-github-review-bot · 2024-08-13T22:20:28Z

@Joerger See the table below for backport results.

Branch	Result
branch/v15	Failed
branch/v16	Failed

webvictim · 2024-11-07T13:39:10Z

This PR was not backported because it introduces a lot of changes. It may be backported later, assuming the v17 test plan doesn't surface any issues with the new functionality.

Joerger changed the title ~~add teleport networkingsubprocess for port/agent/x11 forwarding~~ add teleport networking subprocess for port/agent/x11 forwarding Jul 2, 2024

Joerger changed the title ~~add teleport networking subprocess for port/agent/x11 forwarding~~ Add teleport networking subprocess for port/agent/x11 forwarding Jul 2, 2024

Joerger force-pushed the joerger/teleport-networking-subprocess branch from ed31990 to a58d9cb Compare July 4, 2024 00:30

Joerger requested a review from espadolini July 4, 2024 00:52

Joerger marked this pull request as ready for review July 4, 2024 00:52

github-actions bot added the size/sm label Jul 4, 2024

github-actions bot requested review from AntonAM and r0mant July 4, 2024 00:53

Joerger added the no-changelog Indicates that a PR does not require a changelog entry label Jul 4, 2024

Joerger force-pushed the joerger/teleport-networking-subprocess branch 2 times, most recently from 0a5d488 to 7d965f1 Compare July 4, 2024 01:04

espadolini reviewed Jul 4, 2024

View reviewed changes

Joerger force-pushed the joerger/teleport-networking-subprocess branch 3 times, most recently from d374507 to 8bad5d5 Compare July 10, 2024 00:44

Joerger requested a review from espadolini July 10, 2024 01:32

espadolini requested changes Jul 10, 2024

View reviewed changes

lib/srv/reexec.go Outdated Show resolved Hide resolved

Joerger requested a review from espadolini July 10, 2024 23:11

espadolini reviewed Jul 11, 2024

View reviewed changes

lib/srv/reexec.go Outdated Show resolved Hide resolved

Joerger requested a review from espadolini July 17, 2024 00:47

espadolini reviewed Jul 17, 2024

View reviewed changes

Joerger requested a review from atburke July 19, 2024 18:40

Joerger added backport/branch/v16 backport/branch/v15 labels Jul 19, 2024

Joerger requested a review from espadolini July 19, 2024 20:40

atburke reviewed Jul 20, 2024

View reviewed changes

Joerger force-pushed the joerger/teleport-networking-subprocess branch from 701712b to dabdd1c Compare July 20, 2024 01:43

espadolini reviewed Jul 22, 2024

View reviewed changes

Joerger force-pushed the joerger/teleport-networking-subprocess branch from dabdd1c to 57fba88 Compare July 22, 2024 20:14

Joerger added 2 commits July 26, 2024 19:27

Add networking process tests to replace old tests.

53044e6

Don't close remote file descriptor before the child process has a cha…

d697c84

…nce to connect to it.

Joerger force-pushed the joerger/teleport-networking-subprocess branch from b9062c7 to d697c84 Compare July 27, 2024 02:27

Joerger added 3 commits July 26, 2024 20:15

Fix lint; skip broken test.

af10336

Disable broken test and restore older test to cover for it.

34167b4

Remove unused agent forwarding test.

db60076

Joerger enabled auto-merge July 27, 2024 03:36

rosstimothy disabled auto-merge July 29, 2024 13:03

Merge branch 'master' into joerger/teleport-networking-subprocess

8cb9ec4

Joerger force-pushed the joerger/teleport-networking-subprocess branch from c6b5534 to 386ac4f Compare August 12, 2024 22:52

Joerger added 2 commits August 12, 2024 17:02

Fix lint, replace broken test.

87e7ae4

Fix old test that used testify/require in goroutine.

9070a43

Joerger force-pushed the joerger/teleport-networking-subprocess branch from 386ac4f to 9070a43 Compare August 13, 2024 01:23

Joerger added 2 commits August 13, 2024 10:13

Close request context immediately to avoid deadlock (extended timeout…

09433f1

…) on reading from the closed process.

Merge branch 'master' into joerger/teleport-networking-subprocess

f23c2e3

Joerger enabled auto-merge August 13, 2024 17:58

Merge branch 'master' into joerger/teleport-networking-subprocess

fd8dc1e

Joerger added this pull request to the merge queue Aug 13, 2024

Merged via the queue into master with commit d26ca00 Aug 13, 2024
34 checks passed

Joerger deleted the joerger/teleport-networking-subprocess branch August 13, 2024 22:19

Joerger mentioned this pull request Aug 15, 2024

Restore host user creation for exec requests #45514

Closed

Joerger removed backport/branch/v15 backport/branch/v16 labels Aug 16, 2024

rosstimothy mentioned this pull request Nov 20, 2024

PAM Auth with interactive input is broken. #49028

Closed

Joerger mentioned this pull request Jan 14, 2025

Fix defunct TestX11Forward #51035

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `teleport networking` subprocess for port/agent/x11 forwarding #43756

Add `teleport networking` subprocess for port/agent/x11 forwarding #43756

Joerger commented Jul 2, 2024 •

edited

Loading

github-actions bot commented Jul 4, 2024

espadolini left a comment

espadolini Jul 4, 2024

Joerger Jul 9, 2024

espadolini Jul 17, 2024

Joerger Jul 19, 2024

atburke Jul 20, 2024

espadolini Jul 17, 2024

espadolini Jul 17, 2024 •

edited

Loading

espadolini Jul 17, 2024

atburke Jul 20, 2024

espadolini left a comment

Joerger commented Aug 13, 2024

rosstimothy commented Aug 13, 2024

public-teleport-github-review-bot bot commented Aug 13, 2024

webvictim commented Nov 7, 2024

		// direct-streamlocal@openssh.com extension, we should revisit this multithreading limitation
		// to prevent performance degradation.

		// There are currently no known issues with tcp listen/dial in a multithreaded PAM context.
		go handleNetworkingRequest(ctx, controlConn, req)

Add teleport networking subprocess for port/agent/x11 forwarding #43756

Add teleport networking subprocess for port/agent/x11 forwarding #43756

Conversation

Joerger commented Jul 2, 2024 • edited Loading

github-actions bot commented Jul 4, 2024

espadolini 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

espadolini Jul 17, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

espadolini left a comment

Choose a reason for hiding this comment

Joerger commented Aug 13, 2024

rosstimothy commented Aug 13, 2024

public-teleport-github-review-bot bot commented Aug 13, 2024

webvictim commented Nov 7, 2024

Add `teleport networking` subprocess for port/agent/x11 forwarding #43756

Add `teleport networking` subprocess for port/agent/x11 forwarding #43756

Joerger commented Jul 2, 2024 •

edited

Loading

espadolini Jul 17, 2024 •

edited

Loading