ssh/tailssh: fall back to using su when no TTY available on Linux #11910

oxtoacart · 2024-04-29T11:37:56Z

This allows pam authentication to run, triggering automation
like pam_mkhomedir.

oxtoacart · 2024-04-29T11:38:52Z

ssh/tailssh/testcontainers/Dockerfile

@@ -3,6 +3,10 @@ FROM ${BASE}

 RUN groupadd -g 10000 groupone
 RUN groupadd -g 10001 grouptwo
-RUN useradd -g 10000 -G 10001 -u 10002 -m testuser
+RUN useradd -g 10000 -G 10001 -u 10002 testuser


Removing the -m flag keeps this from creating a home directory for the user (we want PAM to do that on login).

oxtoacart · 2024-05-01T12:10:58Z

ssh/tailssh/incubator.go

@@ -126,22 +127,7 @@ func (ss *sshSession) newIncubatorCommand() (cmd *exec.Cmd) {
 		if isShell {
 			incubatorArgs = append(incubatorArgs, "--shell")
 		}
-		// Only the macOS version of the login command supports executing a


There's no need to do this outside of the actual incubator process, so this has moved into tryLoginCmd.

awly

This links to https://github.com/tailscale/corp/issues/11854, which is probably the wrong issue.
It would be great to see a description of the problem this PR solves for context (sorry if I missed it somewhere!)

ssh/tailssh/incubator.go

oxtoacart · 2024-05-03T02:36:07Z

This links to tailscale/corp#11854, which is probably the wrong issue. It would be great to see a description of the problem this PR solves for context (sorry if I missed it somewhere!)

Sorry, wrong repo. See #11854.

ssh/tailssh/incubator.go

awly · 2024-05-03T18:20:34Z

ssh/tailssh/incubator.go

+	// First try to execute su -l <user> -c id to make sure su supports the
+	// necessary arguments.


are there versions of su that don't support these arguments?

I don't know about Linux, but my version of MacOS doesn't support -c.

SU(1) General Commands Manual SU(1) NAME su – substitute user identity SYNOPSIS su [-] [-flm] [login [args]]

ssh/tailssh/incubator.go

awly · 2024-05-03T18:25:12Z

ssh/tailssh/incubator.go

+		loginArgs = append(loginArgs, "-c", ia.cmdName)
+		loginArgs = append(loginArgs, ia.cmdArgs...)


I think we need to pass the whole command+args to the -c flag:

Suggested change

loginArgs = append(loginArgs, "-c", ia.cmdName)

loginArgs = append(loginArgs, ia.cmdArgs...)

loginArgs = append(loginArgs, "-c", strings.Join(append([]string{ia.cmdName}, ia.cmdArgs...), " "))

from local testing:

❱ su -l awly -c echo test Password: ❱ su -l awly -c "echo test" Password: test

Nice catch!

Interestingly enough, I can't get this to fail in my integration test because SSH already passes the entire command and arguments as a single command string.

Hah, on MacOS at least, /usr/bin/login suffers from the same issue.

➜ oss git:(ox/11854-3) ✗ sudo /usr/bin/login -f -pq -h 100.100.100.102 testuser /bin/zsh -c ls -l /tmp/sftptest.dat Desktop Documents Downloads Library Movies Music Pictures Public sftptest.dat ➜ oss git:(ox/11854-3) ✗ sudo /usr/bin/login -f -pq -h 100.100.100.102 testuser /bin/zsh -c "ls -l /tmp/sftptest.dat" -rw-r--r--@ 1 testuser wheel 11 May 4 11:38 /tmp/sftptest.dat

So yeah, long story short, we're okay here because the command and its args are already passed as a single string from the invocation of beincubator. I'll add a unit test that asserts this.

Okay, I've added a test that runs "echo test". I've also refactored the test to make it clearer what commands we're calling where, and whether they have arguments or not.

ssh/tailssh/incubator.go

awly · 2024-05-06T17:42:38Z

ssh/tailssh/incubator.go

+// findSU attempts to find an su command which supports the -l and -c flags.
+// This actually calls the su command, which can cause side effects like
+// triggering pam_mkhomedir.
+func findSU(logf logger.Logf, ia incubatorArgs) (string, bool) {


optional: the only return values here are "", false and su, true.
maybe this can just return a single string value, and the called checks if it's empty?

Good idea, changed.

ssh/tailssh/incubator.go

awly · 2024-05-06T17:46:34Z

ssh/tailssh/incubator.go

+func dropPrivileges(logf logger.Logf, ia incubatorArgs) error {
+	return doDropPrivileges(logf, ia.uid, ia.gid, ia.gids)
+}


nit: this wrapper seems redundant now

I kept it to avoid complicating the test code.

This allows pam authentication to run for ssh sessions, triggering automation like pam_mkhomedir. Note - this does not apply to SFTP, only shells and remotely executed commands. Updates #11854 Signed-off-by: Percy Wegmann <percy@tailscale.com>

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Percy Wegmann <ox.to.a.cart@gmail.com>

Signed-off-by: Percy Wegmann <percy@tailscale.com>

…anyway Signed-off-by: Percy Wegmann <percy@tailscale.com>

true being a shell built in is less likely to be overriden with something wrong or malicious. Signed-off-by: Percy Wegmann <percy@tailscale.com>

Signed-off-by: Percy Wegmann <percy@tailscale.com>

oxtoacart commented Apr 29, 2024

View reviewed changes

oxtoacart force-pushed the ox/11854-2 branch 4 times, most recently from 01fbe0d to ec7da58 Compare April 29, 2024 12:45

oxtoacart force-pushed the ox/11854-3 branch from 42a80da to b79cca5 Compare April 29, 2024 12:48

oxtoacart force-pushed the ox/11854-2 branch from ec7da58 to c8399b7 Compare April 29, 2024 14:17

oxtoacart force-pushed the ox/11854-3 branch from b79cca5 to ab7c8d9 Compare April 29, 2024 14:36

oxtoacart force-pushed the ox/11854-2 branch 2 times, most recently from 2d12bda to 4090f69 Compare April 29, 2024 14:40

oxtoacart force-pushed the ox/11854-3 branch 2 times, most recently from bb7fef5 to d8df4cf Compare April 29, 2024 14:54

oxtoacart force-pushed the ox/11854-2 branch from 4090f69 to f411643 Compare April 29, 2024 15:10

oxtoacart force-pushed the ox/11854-3 branch from d8df4cf to e258537 Compare April 29, 2024 15:39

oxtoacart force-pushed the ox/11854-2 branch from f411643 to 3033829 Compare April 29, 2024 15:46

oxtoacart force-pushed the ox/11854-3 branch from e258537 to b7b20d4 Compare April 29, 2024 15:54

oxtoacart force-pushed the ox/11854-2 branch from 3033829 to a707e7d Compare April 29, 2024 15:56

oxtoacart force-pushed the ox/11854-3 branch 3 times, most recently from 7501a84 to 5d22981 Compare April 29, 2024 16:14

oxtoacart marked this pull request as ready for review May 1, 2024 11:58

oxtoacart commented May 1, 2024

View reviewed changes

oxtoacart force-pushed the ox/11854-3 branch 2 times, most recently from 2655cc4 to a1ad9e5 Compare May 1, 2024 12:18

oxtoacart force-pushed the ox/11854-2 branch from a707e7d to d8e68c4 Compare May 1, 2024 14:46

Base automatically changed from ox/11854-2 to main May 1, 2024 16:19

oxtoacart force-pushed the ox/11854-3 branch from a1ad9e5 to 3bedc18 Compare May 1, 2024 16:20

oxtoacart requested review from maisem and bradfitz May 1, 2024 16:20

oxtoacart changed the title ~~ssh/tailssh: fall back to su when login is unavailable~~ ssh/tailssh: fall back to using su when no TTY available on Linux May 1, 2024

oxtoacart force-pushed the ox/11854-3 branch 5 times, most recently from f9f1be8 to 7c3424b Compare May 2, 2024 19:28

awly reviewed May 2, 2024

View reviewed changes

ssh/tailssh/incubator.go Outdated Show resolved Hide resolved

oxtoacart force-pushed the ox/11854-3 branch from 7c3424b to 9093c54 Compare May 3, 2024 02:34

awly reviewed May 3, 2024

View reviewed changes

oxtoacart force-pushed the ox/11854-3 branch 5 times, most recently from 8748e47 to b2b1cf8 Compare May 4, 2024 19:09

awly approved these changes May 6, 2024

View reviewed changes

oxtoacart requested a review from a team as a code owner May 15, 2024 15:48

oxtoacart and others added 13 commits May 15, 2024 21:30

Update ssh/tailssh/incubator.go

3ff34dd

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Percy Wegmann <ox.to.a.cart@gmail.com>

Update ssh/tailssh/incubator.go

6201eb5

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Percy Wegmann <ox.to.a.cart@gmail.com>

Update ssh/tailssh/incubator.go

6c4fdc5

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Percy Wegmann <ox.to.a.cart@gmail.com>

Update ssh/tailssh/incubator.go

dc0b5e2

Co-authored-by: Andrew Lytvynov <awly@tailscale.com> Signed-off-by: Percy Wegmann <ox.to.a.cart@gmail.com>

More updates (will need to be squashed)

7cfd689

Signed-off-by: Percy Wegmann <percy@tailscale.com>

run su before SFTP to trigger pam_mkhomedir

30ea911

Signed-off-by: Percy Wegmann <percy@tailscale.com>

Refactored to make flow in beIncubator clearer

002d229

Signed-off-by: Percy Wegmann <percy@tailscale.com>

Code review updates

c5cacae

Signed-off-by: Percy Wegmann <percy@tailscale.com>

Added SCP tests, removed Fedora tests which weren't actually working …

33d0f91

…anyway Signed-off-by: Percy Wegmann <percy@tailscale.com>

Switched to using true instead of id to check su command

dc42bee

true being a shell built in is less likely to be overriden with something wrong or malicious. Signed-off-by: Percy Wegmann <percy@tailscale.com>

Run sftp as child of incubator in order to get the full PAM treatment

7e6b66c

Signed-off-by: Percy Wegmann <percy@tailscale.com>

support legacy behavior based on new nodeattr

f94df7c

Signed-off-by: Percy Wegmann <percy@tailscale.com>

oxtoacart force-pushed the ox/11854-3 branch from 0e65e45 to f94df7c Compare May 16, 2024 02:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ssh/tailssh: fall back to using su when no TTY available on Linux #11910

ssh/tailssh: fall back to using su when no TTY available on Linux #11910

oxtoacart commented Apr 29, 2024 •

edited

oxtoacart Apr 29, 2024

oxtoacart May 1, 2024

awly left a comment

oxtoacart commented May 3, 2024

awly May 3, 2024

oxtoacart May 4, 2024

awly May 3, 2024

oxtoacart May 4, 2024

oxtoacart May 4, 2024

oxtoacart May 4, 2024

oxtoacart May 4, 2024

oxtoacart May 4, 2024

awly May 6, 2024

oxtoacart May 6, 2024

awly May 6, 2024

oxtoacart May 6, 2024

		// First try to execute su -l <user> -c id to make sure su supports the
		// necessary arguments.

		loginArgs = append(loginArgs, "-c", ia.cmdName)
		loginArgs = append(loginArgs, ia.cmdArgs...)

	loginArgs = append(loginArgs, "-c", ia.cmdName)
	loginArgs = append(loginArgs, ia.cmdArgs...)
	loginArgs = append(loginArgs, "-c", strings.Join(append([]string{ia.cmdName}, ia.cmdArgs...), " "))

ssh/tailssh: fall back to using su when no TTY available on Linux #11910

Are you sure you want to change the base?

ssh/tailssh: fall back to using su when no TTY available on Linux #11910

Conversation

oxtoacart commented Apr 29, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

awly left a comment

Choose a reason for hiding this comment

oxtoacart commented May 3, 2024

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxtoacart commented Apr 29, 2024 •

edited