From e48e7b730a020a044a4a01b57d158773c1a59131 Mon Sep 17 00:00:00 2001 From: Mario Minardi Date: Fri, 24 Jul 2026 15:01:35 -0600 Subject: [PATCH] ssh/tailssh: check if user matching autogroup:nonroot is root Add a check to ensure that the user being matched to an autogroup:nonroot rule is in fact a non-root user on the system. Updates https://github.com/tailscale/corp/issues/43245 Signed-off-by: Mario Minardi --- ssh/tailssh/tailssh.go | 24 ++-- ssh/tailssh/tailssh_test.go | 107 +++++++++++++++++- tstest/integration/testcontrol/testcontrol.go | 9 ++ tstest/natlab/vmtest/ssh_test.go | 34 ++++++ tstest/natlab/vmtest/vmtest.go | 5 +- 5 files changed, 166 insertions(+), 13 deletions(-) diff --git a/ssh/tailssh/tailssh.go b/ssh/tailssh/tailssh.go index 9ce3d1991..4408683eb 100644 --- a/ssh/tailssh/tailssh.go +++ b/ssh/tailssh/tailssh.go @@ -22,7 +22,6 @@ import ( "os" "os/exec" "path/filepath" - "regexp" "runtime" "strconv" "strings" @@ -48,6 +47,7 @@ import ( "tailscale.com/util/clientmetric" "tailscale.com/util/httpm" "tailscale.com/util/mak" + "tailscale.com/version/distro" ) var ( @@ -64,8 +64,6 @@ var ( // hookSSHLoginSuccess is called after successful SSH authentication. // It is set by platform-specific code (e.g., auditd_linux.go). hookSSHLoginSuccess feature.Hook[func(logf logger.Logf, c *conn)] - - uidRegex = regexp.MustCompile("^[0-9]+$") ) const ( @@ -344,10 +342,6 @@ func (c *conn) clientAuth(cm ssh.ConnMetadata) (perms *ssh.Permissions, retErr e return &ssh.Permissions{}, nil } - if uidRegex.MatchString(cm.User()) { - return nil, c.errBanner(fmt.Sprintf("rejecting username %q. Usernames that consist of only digits are not allowed as they are ambiguous with numerical UIDs", cm.User()), nil) - } - if err := c.setInfo(cm); err != nil { return nil, c.errBanner("failed to get connection info", err) } @@ -1262,6 +1256,22 @@ func mapLocalUser(ruleSSHUsers map[string]string, reqSSHUser string) (localUser v = ruleSSHUsers["*"] } if v == "=" { + // Skip lookup for gokrazy as we intentionally fall back to a synthesized + // root user there when user lookup fails. + if distro.Get() == distro.Gokrazy { + return reqSSHUser + } + + // Immediately look up user information for purposes of generating + // hold and delegate URL (if necessary). + lu, err := userLookup(reqSSHUser) + if err != nil { + return "" + } + // Don't match as root for autogroup:nonroot + if lu.Uid == "0" || lu.Username == "root" { + return "" + } return reqSSHUser } return v diff --git a/ssh/tailssh/tailssh_test.go b/ssh/tailssh/tailssh_test.go index ec14405e7..234ac1c12 100644 --- a/ssh/tailssh/tailssh_test.go +++ b/ssh/tailssh/tailssh_test.go @@ -6,6 +6,7 @@ package tailssh import ( + "bufio" "bytes" "context" "crypto/ecdsa" @@ -58,6 +59,10 @@ import ( func TestMatchRule(t *testing.T) { someAction := new(tailcfg.SSHAction) + // nonRootUser is a real non-root user in the local passwd database, + // used by the "ssh-user-equal" case because mapLocalUser now does a + // userLookup (via getent) when the mapped value is "=" and rejects root. + nonRootUser := aNonRootUser(t) tests := []struct { name string rule *tailcfg.SSHRule @@ -219,8 +224,8 @@ func TestMatchRule(t *testing.T) { "*": "=", }, }, - ci: &sshConnInfo{sshUser: "alice"}, - wantUser: "alice", + ci: &sshConnInfo{sshUser: nonRootUser}, + wantUser: nonRootUser, }, } for _, tt := range tests { @@ -396,6 +401,39 @@ var currentUser = func() string { return os.Getenv("USER") }() +// aNonRootUser returns the username of a non-root user that exists in the +// local passwd database (verified via userLookup, which uses getent on +// Linux). It prefers the current user, falling back to reading /etc/passwd +// for a real non-root account when the tests are run as root (e.g. in CI). +// It skips the test if no such user can be found. +func aNonRootUser(t *testing.T) string { + t.Helper() + if u, err := userLookup(currentUser); err == nil && u.Uid != "0" && u.Username != "root" { + return currentUser + } + f, err := os.Open("/etc/passwd") + if err != nil { + t.Skipf("can't find a non-root user: %v", err) + } + defer f.Close() + scan := bufio.NewScanner(f) + for scan.Scan() { + fields := strings.Split(scan.Text(), ":") + if len(fields) < 3 { + continue + } + name, uid := fields[0], fields[2] + if uid == "0" || name == "root" { + continue + } + if _, err := userLookup(name); err == nil { + return name + } + } + t.Skip("can't find a non-root user in /etc/passwd") + return "" +} + func (ts *localState) Dialer() *tsdial.Dialer { return &tsdial.Dialer{} } @@ -468,6 +506,7 @@ func newSSHRule(action *tailcfg.SSHAction) *tailcfg.SSHRule { return &tailcfg.SSHRule{ SSHUsers: map[string]string{ "alice": currentUser, + "*": "=", }, Action: action, Principals: []*tailcfg.SSHPrincipal{ @@ -803,6 +842,11 @@ func TestSSHAuthFlow(t *testing.T) { Reject: true, Message: "Go Away!", }) + autogroupNonrootRule := newSSHRule(&tailcfg.SSHAction{ + Accept: true, + Message: "autogroup:nonroot", + }) + autogroupNonrootRule.SSHUsers = map[string]string{"*": "="} tests := []struct { name string @@ -832,7 +876,7 @@ func TestSSHAuthFlow(t *testing.T) { wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "alice"` + "\n"}, }, { - name: "digit-only-username", + name: "user-mismatch-numeric-username", sshUser: "321", state: &localState{ sshEnabled: true, @@ -840,7 +884,62 @@ func TestSSHAuthFlow(t *testing.T) { matchingRule: bobRule, }, authErr: true, - wantBanners: []string{`tailscale: rejecting username "321". Usernames that consist of only digits are not allowed as they are ambiguous with numerical UIDs` + "\n"}, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "321"` + "\n"}, + }, + { + name: "user-mismatch-root-uid", + sshUser: "0", + state: &localState{ + sshEnabled: true, + varRoot: varRoot, + matchingRule: autogroupNonrootRule, + }, + authErr: true, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "0"` + "\n"}, + }, + { + name: "user-mismatch-root-uid-leading-space", + sshUser: " 0", + state: &localState{ + sshEnabled: true, + varRoot: varRoot, + matchingRule: autogroupNonrootRule, + }, + authErr: true, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user " 0"` + "\n"}, + }, + { + name: "user-mismatch-root-uid-force-password-auth", + sshUser: "0+password", + state: &localState{ + sshEnabled: true, + varRoot: varRoot, + matchingRule: autogroupNonrootRule, + }, + authErr: true, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "0"` + "\n"}, + }, + { + name: "user-mismatch-double-zero-force-password-auth", + sshUser: "00+password", + state: &localState{ + sshEnabled: true, + varRoot: varRoot, + matchingRule: autogroupNonrootRule, + }, + authErr: true, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "00"` + "\n"}, + }, + { + name: "user-mismatch-leading-plus", + sshUser: "+0", + state: &localState{ + sshEnabled: true, + varRoot: varRoot, + matchingRule: autogroupNonrootRule, + }, + authErr: true, + wantBanners: []string{`tailscale: tailnet policy does not permit you to SSH as user "+0"` + "\n"}, }, { name: "accept", diff --git a/tstest/integration/testcontrol/testcontrol.go b/tstest/integration/testcontrol/testcontrol.go index dc0979884..ff28c76f0 100644 --- a/tstest/integration/testcontrol/testcontrol.go +++ b/tstest/integration/testcontrol/testcontrol.go @@ -698,6 +698,15 @@ func (s *Server) SetMasqueradeAddresses(pairs []MasqueradePair) { s.updateLocked("SetMasqueradeAddresses", s.nodeIDsLocked(0)) } +// SetSSHPolicy sets the SSH policy sent in MapResponses and notifies all +// connected nodes so they pick up the change. +func (s *Server) SetSSHPolicy(policy *tailcfg.SSHPolicy) { + s.mu.Lock() + defer s.mu.Unlock() + s.SSHPolicy = policy + s.updateLocked("SetSSHPolicy", s.nodeIDsLocked(0)) +} + // SetNodeCapMap overrides the capability map the specified client receives. func (s *Server) SetNodeCapMap(nodeKey key.NodePublic, capMap tailcfg.NodeCapMap) { s.mu.Lock() diff --git a/tstest/natlab/vmtest/ssh_test.go b/tstest/natlab/vmtest/ssh_test.go index 923b31fe9..8f332922e 100644 --- a/tstest/natlab/vmtest/ssh_test.go +++ b/tstest/natlab/vmtest/ssh_test.go @@ -12,6 +12,8 @@ import ( "time" "github.com/creachadair/mds/shell" + "tailscale.com/tailcfg" + "tailscale.com/tstest" "tailscale.com/tstest/natlab/vmtest" "tailscale.com/tstest/natlab/vnet" ) @@ -37,6 +39,8 @@ func TestTailscaleSSH_Ubuntu(t *testing.T) { if out, code := testSuite.ssh(t, "nosuchuser", "true"); code == 0 { t.Errorf("ubuntu nonexistent user: ssh succeeded, want failure:\n%s", out) } + + testSuite.checkAutogroupNonroot(t, "ubuntu") } // TestTailscaleSSH_FreeBSD exercises the Tailscale SSH server ("tailscale up @@ -60,6 +64,8 @@ func TestTailscaleSSH_FreeBSD(t *testing.T) { if out, code := testSuite.ssh(t, "nosuchuser", "true"); code == 0 { t.Errorf("freebsd nonexistent user: ssh succeeded, want failure:\n%s", out) } + + testSuite.checkAutogroupNonroot(t, "freebsd") } // TestTailscaleSSH_Gokrazy exercises the gokrazy-specific cases in the @@ -157,6 +163,34 @@ func (st *sshTest) check(t *testing.T, desc, user string, cmd, want string) { } } +// checkAutogroupNonroot adjusts the server's SSH policy to be equivalent to "autogroup:nonroot" +// and verifies that attempting to SSH as root fails. +func (st *sshTest) checkAutogroupNonroot(t *testing.T, name string) { + t.Helper() + orignalSSHPolicy := st.env.ControlServer().SSHPolicy + defer func() { + st.env.ControlServer().SetSSHPolicy(orignalSSHPolicy) + }() + + st.env.ControlServer().SetSSHPolicy(&tailcfg.SSHPolicy{ + Rules: []*tailcfg.SSHRule{{ + Principals: []*tailcfg.SSHPrincipal{{Any: true}}, + SSHUsers: map[string]string{"*": "="}, + Action: &tailcfg.SSHAction{Accept: true}, + }}, + }) + + if err := tstest.WaitFor(30*time.Second, func() error { + _, code := st.ssh(t, "root", "true") + if code == 0 { + return fmt.Errorf("root SSH still succeeds") + } + return nil + }); err != nil { + t.Fatalf("%s: root SSH still succeeds after policy update to autogroup:nonroot-only; policy may not have propagated", name) + } +} + func newTestSuite(t *testing.T, serverName string, serverOS vmtest.OSImage) *sshTest { t.Helper() env := vmtest.New(t) diff --git a/tstest/natlab/vmtest/vmtest.go b/tstest/natlab/vmtest/vmtest.go index 95cf15311..110e6f5f4 100644 --- a/tstest/natlab/vmtest/vmtest.go +++ b/tstest/natlab/vmtest/vmtest.go @@ -1779,8 +1779,9 @@ func (e *Env) initVnet() { e.server.ControlServer().SSHPolicy = &tailcfg.SSHPolicy{ Rules: []*tailcfg.SSHRule{{ Principals: []*tailcfg.SSHPrincipal{{Any: true}}, - SSHUsers: map[string]string{"*": "="}, - Action: &tailcfg.SSHAction{Accept: true}, + // Allow permissive login + root login by default + SSHUsers: map[string]string{"*": "=", "root": "root"}, + Action: &tailcfg.SSHAction{Accept: true}, }}, } break