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.

bradfitz · 2024-05-28T16:08:53Z

ssh/tailssh/incubator.go

- isShell = true
- args = append(args, "-l") // login shell
- }
+ isShell = ss.RawCommand() == ""
 default:
 panic(fmt.Sprintf("unexpected subsystem: %v", ss.Subsystem()))
 }

 if ss.conn.srv.tailscaledPath == "" {
 // TODO(maisem): this doesn't work with sftp


this comment still accurate? It's confusing in any case. Can you delete or clarify what it means?

Good catch. We really should be returning an error here. I've updated the code and clarified the comment.

bradfitz · 2024-05-28T16:09:17Z

ssh/tailssh/incubator.go

+ if !isSFTP {
+ loginShell := ss.conn.localUser.LoginShell()
+ args := shellArgs(isShell, ss.RawCommand())
+ logf("directly running %s %q", loginShell, args)


where's this log to? Is this still needed or was it leftover debug?

Unless debugIncubator is compiled to true or we're running in tests, this logs to Discard.

bradfitz · 2024-05-28T16:10:31Z

ssh/tailssh/incubator.go

 "--local-user=" + lu.Username,
 "--remote-user=" + remoteUser,
 "--remote-ip=" + ci.src.Addr().String(),
 "--has-tty=false", // updated in-place by startWithPTY
 "--tty-name=", // updated in-place by startWithPTY
 }

+ forceV1Behavior := ss.conn.srv.lb.NetMap().SelfNode.HasCap(tailcfg.NodeAttrSSHBehaviorV1)


might want a method on LocalBackend instead so you can easily guard against the small window where NetMap can turn nil on a logout/switch concurrent with an incoming SSH connection and then crash here on the SelfNode deref.

Ah, nice! Done. The HasCap already guards agains a null SelfNode, but NetMap() can be nil, so your suggestion fixes that.

bradfitz · 2024-05-28T16:11:09Z

ssh/tailssh/incubator.go

- return unix.Exec(ia.loginCmdPath, la, os.Environ())
- }
+ if !shouldAttemptLoginShell(logf, ia) {
+ logf("not attempting login shell")


same question

Logs nowhere in production builds.

I might name it vlogf then. That's what we do elsewhere for such things.

bradfitz · 2024-05-28T16:12:18Z

ssh/tailssh/incubator.go

+func tryLoginWithSU(logf logger.Logf, ia incubatorArgs) (bool, error) {
+ // Currently, we only support falling back to su on Linux. This
+ // potentially could work on BSDs as well, but requires testing.
+ if runtime.GOOS != "linux" {


I'd still like an explicit check for Gokrazy just for clarity that it's not going to need/try to find 'su'.

bradfitz · 2024-05-28T16:13:20Z

ssh/tailssh/tailssh_integration_test.go

- if err != nil {
- t.Fatalf("can't open file: %s", err)
+func fallbackToSUAvailable() bool {
+ if runtime.GOOS != "linux" {


gokrazy here too

This is testing code. Are you saying you want the integration test to be able to run on gocrazy?

ah, okay. I guess I don't care about testing on gokrazy.

bradfitz · 2024-05-28T16:14:25Z

ssh/tailssh/incubator.go

+ sessionCloser, err := maybeStartLoginSession(logf, ia)
+ if err == nil && sessionCloser != nil {
+ defer sessionCloser()
+ }


ignoring err != nil case?

That's old code, but yeah, it seems to ignore the error case. I've removed the error return, since it's actually always nil anyway.

oxtoacart · 2024-05-28T18:12:42Z

#11910 (comment)

That's dead code.

bradfitz

Approved but see comments first.

bradfitz · 2024-05-29T15:51:21Z

ipn/ipnlocal/local.go

+// ForceV1SSHBehavior indicates whether the original V1 SSH behavior should be used.
+func (b *LocalBackend) ForceV1SSHBehavior() bool {
+ b.mu.Lock()
+ defer b.mu.Unlock()
+
+ return b.netMap != nil && b.netMap.SelfNode.HasCap(tailcfg.NodeAttrSSHBehaviorV1)
+}


actually, turns out you don't need this. There's a method that already guards against a nil NetMap:

// HasCap reports whether nm is non-nil and nm.AllCaps contains c. func (nm *NetworkMap) HasCap(c tailcfg.NodeCapability) bool { return nm != nil && nm.AllCaps.Contains(c) }

So you can use that from the ssh code instead, keeping that code localized.

Nice, I like that better.

bradfitz · 2024-05-29T15:51:39Z

ssh/tailssh/tailssh.go

@@ -66,6 +66,7 @@ const (
 // It is used for testing.
 type ipnLocalBackend interface {
 GetSSH_HostKeys() ([]gossh.Signer, error)
+ ForceV1SSHBehavior() bool


... then you don't need to add this

bradfitz · 2024-05-29T15:52:22Z

tailcfg/tailcfg.go

@@ -2274,6 +2274,9 @@ const (
 // depending on the destination address and the configured routes. When present, it also makes
 // the DNS forwarder use UserDial instead of SystemDial when dialing resolvers.
 NodeAttrUserDialUseRoutes NodeCapability = "user-dial-routes"
+
+ // NodeAttrSSHBehaviorV1 forces SSH to use the V1 behavior (no su, run SFTP in-process)


mention a date (yyyy-mm-dd) and Tailscale version number in here?

Will do, and will also update CurrentCapabilityVersion.

bradfitz · 2024-05-29T15:53:51Z

ssh/tailssh/tailssh_integration_test.go

@@ -77,6 +79,10 @@ func TestMain(m *testing.M) {
 if err != nil {
 return
 }
+ defer func() {
+ // wait a little bit for logging to catch up.
+ time.Sleep(2 * time.Second)


Sleeps in tests are never the right answer. They're either too short (flaky) or too long (extra pain for people running tests; they add up)

Also this comment doesn't justify it: catch up to what? What are we actually waiting for here?

Let's remove the sleep and add explicit synchronization if needed.

I don't think there's anything explicit to synchronize with, we're waiting for the tail command to read our logs. There's a delay between when the test terminates and tail actually reads the log file contents, hence the delay. I'll make the comment more explicit.

bradfitz · 2024-05-29T15:57:38Z

ssh/tailssh/incubator.go

- a.cmdArgs = flags.Args()
- return a
+
+ for _, g := range strings.Split(groups, ",") {


strings.Split("", ",") == []string{""} and will thus fail on L206. I guess this one at least (or all of them?) are expected to be non-empty?

This is not new code. AFAICT, on Unix systems, users will always have at least a primary group id, which means we should be fine here. We use these groups to masquerade as this user, so if there are no groups, it's not clear that we'd want to proceed anyway.

bradfitz · 2024-05-29T15:58:21Z

ssh/tailssh/incubator.go

- return unix.Exec(ia.loginCmdPath, la, os.Environ())
- }
+ if !shouldAttemptLoginShell(logf, ia) {
+ logf("not attempting login shell")


I might name it vlogf then. That's what we do elsewhere for such things.

bradfitz · 2024-05-29T16:01:56Z

ssh/tailssh/incubator.go

+// Unlike login, su often does not require a TTY, so on Linux hosts that have
+// an su command which accepts the right flags, we'll use su instead of login
+// when no TTY is available.
+func trySU(logf logger.Logf, ia incubatorArgs) (bool, error) {


I'd probably name these results, as this is an incommon signature. Maybe (hasSU bool, _ error)?

The caller does:

if handled, err := trySU(logf, ia); handled { return err

bradfitz · 2024-05-29T16:04:16Z

ssh/tailssh/incubator.go

+func tryLoginWithSU(logf logger.Logf, ia incubatorArgs) (bool, error) {
+ // Currently, we only support falling back to su on Linux. This
+ // potentially could work on BSDs as well, but requires testing.
+ if runtime.GOOS != "linux" {


I'd still prefer an explicit fast path for Gokrazy for clarity. No need to even try to find it.

Actually now that I think about it, it could be bad to even try to find it on gokrazy: when the 'breakglass' debug package is installed, it's possible there'd be a su binary (that might not work, just included as part of busybox) and we wouldn't want to behave differently in that case when debugging was in use.

This allows pam authentication to run for ssh sessions, triggering automation like pam_mkhomedir. Updates #11854 Signed-off-by: Percy Wegmann <[email protected]>

oxtoacart · 2024-05-29T18:15:14Z

Thanks for the reviews @awly and @bradfitz !

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 7 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 force-pushed the ox/11854-3 branch from 0e65e45 to f94df7c Compare May 16, 2024 02:33

oxtoacart force-pushed the ox/11854-3 branch from f94df7c to 38560b4 Compare May 28, 2024 16:02

bradfitz reviewed May 28, 2024

View reviewed changes

bradfitz approved these changes May 29, 2024

View reviewed changes

oxtoacart force-pushed the ox/11854-3 branch 2 times, most recently from 0385cfc to a9cd6a8 Compare May 29, 2024 17:56

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

e2fa96e

This allows pam authentication to run for ssh sessions, triggering automation like pam_mkhomedir. Updates #11854 Signed-off-by: Percy Wegmann <[email protected]>

oxtoacart force-pushed the ox/11854-3 branch from a9cd6a8 to e2fa96e Compare May 29, 2024 17:57

oxtoacart merged commit 08a9551 into main May 29, 2024
48 checks passed

oxtoacart deleted the ox/11854-3 branch May 29, 2024 18:15

		// 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oxtoacart May 28, 2024 • edited

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 May 28, 2024

bradfitz 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

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 May 29, 2024

oxtoacart commented Apr 29, 2024 •

edited

oxtoacart May 28, 2024 •

edited