Create host users once per conn (#45102) (#45682)

This change updates host user creation to create users at the start
of a new SSH connection, rather than at the start of certain new
channels opening.

integration/hostuser_test.go

@@ -20,6 +20,7 @@ limitations under the License.
 package integration
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"os"
@@ -28,17 +29,22 @@ import (
 	"path/filepath"
 	"slices"
 	"testing"
+	"time"
 
+	"github.com/google/uuid"
 	"github.com/gravitational/trace"
 	log "github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/require"
 
 	"github.com/gravitational/teleport/api/types"
+	"github.com/gravitational/teleport/integration/helpers"
+	"github.com/gravitational/teleport/lib/auth/testauthority"
 	"github.com/gravitational/teleport/lib/backend"
 	"github.com/gravitational/teleport/lib/backend/lite"
 	"github.com/gravitational/teleport/lib/services"
 	"github.com/gravitational/teleport/lib/services/local"
 	"github.com/gravitational/teleport/lib/srv"
+	"github.com/gravitational/teleport/lib/utils"
 	"github.com/gravitational/teleport/lib/utils/host"
 )
 
@@ -439,3 +445,101 @@ func TestRootHostUsers(t *testing.T) {
 		}
 	})
 }
+
+func TestRootLoginAsHostUser(t *testing.T) {
+	requireRoot(t)
+	// Create test instance.
+	privateKey, publicKey, err := testauthority.New().GenerateKeyPair()
+	require.NoError(t, err)
+
+	instance := helpers.NewInstance(t, helpers.InstanceConfig{
+		ClusterName: helpers.Site,
+		HostID:      uuid.New().String(),
+		NodeName:    Host,
+		Priv:        privateKey,
+		Pub:         publicKey,
+		Log:         utils.NewLoggerForTests(),
+	})
+
+	// Create a user that can create a host user.
+	username := "test-user"
+	login := generateLocalUsername(t)
+	groups := []string{"foo", "bar"}
+	role, err := types.NewRole("ssh-host-user", types.RoleSpecV6{
+		Options: types.RoleOptions{
+			CreateHostUserMode: types.CreateHostUserMode_HOST_USER_MODE_KEEP,
+		},
+		Allow: types.RoleConditions{
+			Logins:     []string{login},
+			HostGroups: groups,
+			NodeLabels: types.Labels{types.Wildcard: []string{types.Wildcard}},
+		},
+	})
+	require.NoError(t, err)
+	instance.Secrets.Users[username] = &helpers.User{
+		Username:      username,
+		AllowedLogins: []string{login},
+		Roles:         []types.Role{role},
+	}
+
+	require.NoError(t, instance.Create(t, nil, true, nil))
+	require.NoError(t, instance.Start())
+	t.Cleanup(func() {
+		require.NoError(t, instance.StopAll())
+	})
+
+	tests := []struct {
+		name      string
+		command   []string
+		stdinText string
+	}{
+		{
+			name:    "non-interactive session",
+			command: []string{"whoami"},
+		},
+		{
+			name:      "interactive session",
+			stdinText: "whoami\nexit\n",
+		},
+	}
+
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			stdin := bytes.NewBufferString(tc.stdinText)
+			stdout := utils.NewSyncBuffer()
+			t.Cleanup(func() {
+				require.NoError(t, stdout.Close())
+			})
+
+			client, err := instance.NewClient(helpers.ClientConfig{
+				TeleportUser: username,
+				Login:        login,
+				Cluster:      helpers.Site,
+				Host:         Host,
+				Port:         helpers.Port(t, instance.SSH),
+				Stdin:        stdin,
+				Stdout:       stdout,
+			})
+			require.NoError(t, err)
+
+			// Run an SSH session to completion.
+			t.Cleanup(cleanupUsersAndGroups([]string{login}, groups))
+			ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second)
+			t.Cleanup(cancel)
+			err = client.SSH(ctx, tc.command)
+			require.NoError(t, err)
+			// Check for correct result from whoami command.
+			require.Contains(t, stdout.String(), login)
+
+			// Verify that a host user was created.
+			u, err := user.Lookup(login)
+			require.NoError(t, err)
+			createdGroups, err := u.GroupIds()
+			require.NoError(t, err)
+			for _, group := range createdGroups {
+				_, err := user.LookupGroupId(group)
+				require.NoError(t, err)
+			}
+		})
+	}
+}

integration/port_forwarding_test.go

@@ -75,18 +75,21 @@ func waitForSessionToBeEstablished(ctx context.Context, namespace string, site a
 	}
 }
 
-func testPortForwarding(t *testing.T, suite *integrationTestSuite) {
-	invalidOSLogin := uuid.NewString()[:12]
-	notFound := false
+func generateLocalUsername(t *testing.T) string {
+	login := uuid.NewString()[:12]
 	for i := 0; i < 10; i++ {
-		if _, err := user.Lookup(invalidOSLogin); err == nil {
-			invalidOSLogin = uuid.NewString()[:12]
+		if _, err := user.Lookup(login); err == nil {
+			login = uuid.NewString()[:12]
 			continue
 		}
-		notFound = true
-		break
+		return login
 	}
-	require.True(t, notFound, "unable to locate invalid os user")
+	require.Fail(t, "unable to locate invalid os user")
+	return ""
+}
+
+func testPortForwarding(t *testing.T, suite *integrationTestSuite) {
+	invalidOSLogin := generateLocalUsername(t)
 
 	// Providing our own logins to Teleport so we can verify that a user
 	// that exists within Teleport but does not exist on the local node

lib/srv/ctx.go

@@ -447,9 +447,6 @@ type ServerContext struct {
 	// ServerSubKind if the sub kind of the node this context is for.
 	ServerSubKind string
 
-	// UserCreatedByTeleport is true when the system user was created by Teleport user auto-provision.
-	UserCreatedByTeleport bool
-
 	// approvedFileReq is an approved file transfer request that will only be
 	// set when the session's pending file transfer request is approved.
 	approvedFileReq *FileTransferRequest

lib/srv/regular/sshserver.go

@@ -1260,16 +1260,39 @@ func (s *Server) HandleRequest(ctx context.Context, r *ssh.Request) {
 }
 
 // HandleNewConn is called by sshutils.Server once for each new incoming connection,
-// prior to handling any channels or requests.  Currently this callback's only
-// function is to apply session control restrictions.
+// prior to handling any channels or requests.
 func (s *Server) HandleNewConn(ctx context.Context, ccx *sshutils.ConnectionContext) (context.Context, error) {
 	identityContext, err := s.authHandlers.CreateIdentityContext(ccx.ServerConn)
 	if err != nil {
 		return ctx, trace.Wrap(err)
 	}
 
+	// Apply session control restrictions.
 	ctx, err = s.sessionController.AcquireSessionContext(ctx, identityContext, ccx.ServerConn.LocalAddr().String(), ccx.ServerConn.RemoteAddr().String(), ccx)
-	return ctx, trace.Wrap(err)
+	if err != nil {
+		return ctx, trace.Wrap(err)
+	}
+
+	// Create host user.
+	created, userCloser, err := s.termHandlers.SessionRegistry.TryCreateHostUser(identityContext)
+	if err != nil {
+		return ctx, trace.Wrap(err)
+	}
+	// Indicate that the user was created by Teleport.
+	ccx.UserCreatedByTeleport = created
+	if userCloser != nil {
+		ccx.AddCloser(userCloser)
+	}
+
+	sudoersCloser, err := s.termHandlers.SessionRegistry.TryWriteSudoersFile(identityContext)
+	if err != nil {
+		return ctx, trace.Wrap(err)
+	}
+	if sudoersCloser != nil {
+		ccx.AddCloser(sudoersCloser)
+	}
+
+	return ctx, nil
 }
 
 // HandleNewChan is called when new channel is opened
@@ -1748,22 +1771,10 @@ func (s *Server) dispatch(ctx context.Context, ch ssh.Channel, req *ssh.Request,
 	case tracessh.TracingRequest:
 		return nil
 	case sshutils.ExecRequest:
-		if err := s.termHandlers.SessionRegistry.TryCreateHostUser(serverContext); err != nil {
-			return trace.Wrap(err)
-		}
-		if err := s.termHandlers.SessionRegistry.TryWriteSudoersFile(serverContext); err != nil {
-			return trace.Wrap(err)
-		}
 		return s.termHandlers.HandleExec(ctx, ch, req, serverContext)
 	case sshutils.PTYRequest:
 		return s.termHandlers.HandlePTYReq(ctx, ch, req, serverContext)
 	case sshutils.ShellRequest:
-		if err := s.termHandlers.SessionRegistry.TryCreateHostUser(serverContext); err != nil {
-			return trace.Wrap(err)
-		}
-		if err := s.termHandlers.SessionRegistry.TryWriteSudoersFile(serverContext); err != nil {
-			return trace.Wrap(err)
-		}
 		return s.termHandlers.HandleShell(ctx, ch, req, serverContext)
 	case constants.InitiateFileTransfer:
 		return s.termHandlers.HandleFileTransferRequest(ctx, ch, req, serverContext)
@@ -1790,14 +1801,6 @@ func (s *Server) dispatch(ctx context.Context, ch ssh.Channel, req *ssh.Request,
 		// https://tools.ietf.org/html/draft-ietf-secsh-agent-02
 		// the open ssh proto spec that we implement is here:
 		// http://cvsweb.openbsd.org/cgi-bin/cvsweb/src/usr.bin/ssh/PROTOCOL.agent
-		if err := s.termHandlers.SessionRegistry.TryCreateHostUser(serverContext); err != nil {
-			s.Logger.Warn(err)
-			return nil
-		}
-		if err := s.termHandlers.SessionRegistry.TryWriteSudoersFile(serverContext); err != nil {
-			s.Logger.Warn(err)
-			return nil
-		}
 
 		// to maintain interoperability with OpenSSH, agent forwarding requests
 		// should never fail, all errors should be logged and we should continue

lib/srv/sess.go

@@ -234,65 +234,65 @@ func (sc *sudoersCloser) Close() error {
 	return nil
 }
 
-func (s *SessionRegistry) TryWriteSudoersFile(ctx *ServerContext) error {
+// TryWriteSudoersFile tries to write the needed sudoers entry to the sudoers
+// file, if any. If the returned closer is not nil, it must be called at the
+// end of the session to cleanup the sudoers file.
+func (s *SessionRegistry) TryWriteSudoersFile(identityContext IdentityContext) (io.Closer, error) {
 	if s.sudoers == nil {
-		return nil
+		return nil, nil
 	}
 
-	sudoers, err := ctx.Identity.AccessChecker.HostSudoers(ctx.srv.GetInfo())
+	sudoers, err := identityContext.AccessChecker.HostSudoers(s.Srv.GetInfo())
 	if err != nil {
-		return trace.Wrap(err)
+		return nil, trace.Wrap(err)
 	}
 	if len(sudoers) == 0 {
 		// not an error, sudoers may not be configured.
-		return nil
+		return nil, nil
 	}
-	if err := s.sudoers.WriteSudoers(ctx.Identity.Login, sudoers); err != nil {
-		return trace.Wrap(err)
+	if err := s.sudoers.WriteSudoers(identityContext.Login, sudoers); err != nil {
+		return nil, trace.Wrap(err)
 	}
 
-	s.sessionsByUser.add(ctx.Identity.Login)
-	ctx.AddCloser(&sudoersCloser{
-		username:     ctx.Identity.Login,
+	s.sessionsByUser.add(identityContext.Login)
+
+	return &sudoersCloser{
+		username:     identityContext.Login,
 		userSessions: s.sessionsByUser,
 		cleanup:      s.sudoers.RemoveSudoers,
-	})
-
-	return nil
+	}, nil
 }
 
-func (s *SessionRegistry) TryCreateHostUser(ctx *ServerContext) error {
-	if !ctx.srv.GetCreateHostUser() {
+// TryCreateHostUser attempts to create a local user on the host if needed.
+// If the returned closer is not nil, it must be called at the end of the
+// session to clean up the local user.
+func (s *SessionRegistry) TryCreateHostUser(identityContext IdentityContext) (created bool, closer io.Closer, err error) {
+	if !s.Srv.GetCreateHostUser() || s.users == nil {
 		s.log.Debug("Not creating host user: node has disabled host user creation.")
-		return nil // not an error to not be able to create host users
+		return false, nil, nil // not an error to not be able to create host users
 	}
 
-	ui, err := ctx.Identity.AccessChecker.HostUsers(ctx.srv.GetInfo())
+	ui, err := identityContext.AccessChecker.HostUsers(s.Srv.GetInfo())
 	if err != nil {
 		if trace.IsAccessDenied(err) {
-			return nil
+			log.Warnf("Unable to create host users: %v", err)
+			return false, nil, nil
 		}
 		log.Debug("Error while checking host users creation permission: ", err)
-		return trace.Wrap(err)
+		return false, nil, trace.Wrap(err)
 	}
 
-	existsErr := s.users.UserExists(ctx.Identity.Login)
+	existsErr := s.users.UserExists(identityContext.Login)
 	if trace.IsAccessDenied(err) && existsErr != nil {
-		return trace.WrapWithMessage(err, "Insufficient permission for host user creation")
-	}
-	userCloser, err := s.users.UpsertUser(ctx.Identity.Login, *ui)
-	if userCloser != nil {
-		ctx.AddCloser(userCloser)
+		return false, nil, trace.WrapWithMessage(err, "Insufficient permission for host user creation")
 	}
+	userCloser, err := s.users.UpsertUser(identityContext.Login, *ui)
 	if err != nil && !trace.IsAlreadyExists(err) {
-		log.Debugf("Error creating user %s: %s", ctx.Identity.Login, err)
-		return trace.Wrap(err)
+		log.Debugf("Error creating user %s: %s", identityContext.Login, err)
+		return false, nil, trace.Wrap(err)
 	}
 
-	// Indicate that the user was created by Teleport.
-	ctx.UserCreatedByTeleport = true
-
-	return nil
+	return true, userCloser, nil
 }
 
 // OpenSession either joins an existing active session or starts a new session.

lib/sshutils/ctx.go

@@ -75,6 +75,9 @@ type ConnectionContext struct {
 	// clientLastActive records the last time there was activity from the client.
 	clientLastActive time.Time
 
+	// UserCreatedByTeleport is true when the system user was created by Teleport user auto-provision.
+	UserCreatedByTeleport bool
+
 	clock clockwork.Clock
 }