Add btmp support for user accounting (#32055)

This change adds support for the btmp file (failed logins)
for user accounting. It also fixes a bug where the remote
address of a connection was not being correctly logged.

integration/utmp_integration_test.go

@@ -19,13 +19,15 @@ package integration
 import (
 	"context"
 	"os"
+	"os/user"
 	"path"
 	"testing"
 	"time"
 
 	"github.com/google/uuid"
 	"github.com/gravitational/trace"
 	"github.com/jonboulle/clockwork"
+	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"golang.org/x/crypto/ssh"
 
@@ -48,8 +50,8 @@ import (
 	"github.com/gravitational/teleport/lib/utils"
 )
 
-// teleportTestUser is additional user used for tests
-const teleportTestUser = "teleport-test"
+// teleportFakeUser is a user that doesn't exist, used for tests.
+const teleportFakeUser = "teleport-fake"
 
 // wildcardAllow is used in tests to allow access to all labels.
 var wildcardAllow = types.Labels{
@@ -64,6 +66,8 @@ type SrvCtx struct {
 	nodeClient *auth.Client
 	nodeID     string
 	utmpPath   string
+	wtmpPath   string
+	btmpPath   string
 }
 
 // TestRootUTMPEntryExists verifies that user accounting is done on supported systems.
@@ -72,51 +76,87 @@ func TestRootUTMPEntryExists(t *testing.T) {
 		t.Skip("This test will be skipped because tests are not being run as root.")
 	}
 
+	user, err := user.Current()
+	require.NoError(t, err)
+	teleportTestUser := user.Name
+
 	ctx := context.Background()
 	s := newSrvCtx(ctx, t)
-	up, err := newUpack(ctx, s, teleportTestUser, []string{teleportTestUser}, wildcardAllow)
+	up, err := newUpack(ctx, s, teleportTestUser, []string{teleportTestUser, teleportFakeUser}, wildcardAllow)
 	require.NoError(t, err)
 
-	sshConfig := &ssh.ClientConfig{
-		User:            teleportTestUser,
-		Auth:            []ssh.AuthMethod{ssh.PublicKeys(up.certSigner)},
-		HostKeyCallback: ssh.FixedHostKey(s.signer.PublicKey()),
-	}
+	t.Run("successful login is logged in utmp and wtmp", func(t *testing.T) {
+		sshConfig := &ssh.ClientConfig{
+			User:            teleportTestUser,
+			Auth:            []ssh.AuthMethod{ssh.PublicKeys(up.certSigner)},
+			HostKeyCallback: ssh.FixedHostKey(s.signer.PublicKey()),
+		}
 
-	client, err := ssh.Dial("tcp", s.srv.Addr(), sshConfig)
-	require.NoError(t, err)
-	defer func() {
-		err := client.Close()
+		client, err := ssh.Dial("tcp", s.srv.Addr(), sshConfig)
 		require.NoError(t, err)
-	}()
+		defer func() {
+			err := client.Close()
+			require.NoError(t, err)
+		}()
 
-	se, err := client.NewSession()
-	require.NoError(t, err)
-	defer se.Close()
+		se, err := client.NewSession()
+		require.NoError(t, err)
+		defer se.Close()
 
-	modes := ssh.TerminalModes{
-		ssh.ECHO:          0,     // disable echoing
-		ssh.TTY_OP_ISPEED: 14400, // input speed = 14.4kbaud
-		ssh.TTY_OP_OSPEED: 14400, // output speed = 14.4kbaud
-	}
+		modes := ssh.TerminalModes{
+			ssh.ECHO:          0,     // disable echoing
+			ssh.TTY_OP_ISPEED: 14400, // input speed = 14.4kbaud
+			ssh.TTY_OP_OSPEED: 14400, // output speed = 14.4kbaud
+		}
 
-	require.NoError(t, se.RequestPty("xterm", 80, 80, modes), nil)
-	err = se.Shell()
-	require.NoError(t, err)
+		require.NoError(t, se.RequestPty("xterm", 80, 80, modes), nil)
+		err = se.Shell()
+		require.NoError(t, err)
 
-	start := time.Now()
-	for time.Since(start) < 5*time.Minute {
-		time.Sleep(time.Second)
-		entryExists := uacc.UserWithPtyInDatabase(s.utmpPath, teleportTestUser)
-		if entryExists == nil {
-			return
+		require.EventuallyWithTf(t, func(collect *assert.CollectT) {
+			require.NoError(collect, uacc.UserWithPtyInDatabase(s.utmpPath, teleportTestUser))
+			require.NoError(collect, uacc.UserWithPtyInDatabase(s.wtmpPath, teleportTestUser))
+			// Ensure than an entry was not written to btmp.
+			require.True(collect, trace.IsNotFound(uacc.UserWithPtyInDatabase(s.btmpPath, teleportTestUser)), "unexpected error: %v", err)
+		}, 5*time.Minute, time.Second, "did not detect utmp entry within 5 minutes")
+	})
+
+	t.Run("unsuccessful login is logged in btmp", func(t *testing.T) {
+		sshConfig := &ssh.ClientConfig{
+			User:            teleportFakeUser,
+			Auth:            []ssh.AuthMethod{ssh.PublicKeys(up.certSigner)},
+			HostKeyCallback: ssh.FixedHostKey(s.signer.PublicKey()),
 		}
-		if !trace.IsNotFound(entryExists) {
+
+		client, err := ssh.Dial("tcp", s.srv.Addr(), sshConfig)
+		require.NoError(t, err)
+		defer func() {
+			err := client.Close()
 			require.NoError(t, err)
+		}()
+
+		se, err := client.NewSession()
+		require.NoError(t, err)
+		defer se.Close()
+
+		modes := ssh.TerminalModes{
+			ssh.ECHO:          0,     // disable echoing
+			ssh.TTY_OP_ISPEED: 14400, // input speed = 14.4kbaud
+			ssh.TTY_OP_OSPEED: 14400, // output speed = 14.4kbaud
 		}
-	}
 
-	t.Errorf("did not detect utmp entry within 5 minutes")
+		require.NoError(t, se.RequestPty("xterm", 80, 80, modes), nil)
+		err = se.Shell()
+		require.NoError(t, err)
+
+		require.EventuallyWithT(t, func(collect *assert.CollectT) {
+			require.NoError(collect, uacc.UserWithPtyInDatabase(s.btmpPath, teleportFakeUser))
+			// Ensure that entries were not written to utmp and wtmp
+			require.True(collect, trace.IsNotFound(uacc.UserWithPtyInDatabase(s.utmpPath, teleportFakeUser)), "unexpected error: %v", err)
+			require.True(collect, trace.IsNotFound(uacc.UserWithPtyInDatabase(s.wtmpPath, teleportFakeUser)), "unexpected error: %v", err)
+		}, 5*time.Minute, time.Second, "did not detect btmp entry within 5 minutes")
+	})
+
 }
 
 // TestUsernameLimit tests that the maximum length of usernames is a hard error.
@@ -139,15 +179,12 @@ func TestRootUsernameLimit(t *testing.T) {
 	host := [4]int32{0, 0, 0, 0}
 	tty := os.NewFile(uintptr(0), "/proc/self/fd/0")
 	err = uacc.Open(utmpPath, wtmpPath, username, "localhost", host, tty)
-	require.Error(t, err)
+	require.True(t, trace.IsBadParameter(err))
 
 	// A 32 character long username.
 	username = "aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"
 	err = uacc.Open(utmpPath, wtmpPath, username, "localhost", host, tty)
-	require.NoError(t, err)
-
-	err = uacc.Close(utmpPath, wtmpPath, tty)
-	require.NoError(t, err)
+	require.False(t, trace.IsBadParameter(err))
 }
 
 // upack holds all ssh signing artifacts needed for signing and checking user keys
@@ -244,11 +281,13 @@ func newSrvCtx(ctx context.Context, t *testing.T) *SrvCtx {
 	uaccDir := t.TempDir()
 	utmpPath := path.Join(uaccDir, "utmp")
 	wtmpPath := path.Join(uaccDir, "wtmp")
-	err = TouchFile(utmpPath)
-	require.NoError(t, err)
-	err = TouchFile(wtmpPath)
-	require.NoError(t, err)
+	btmpPath := path.Join(uaccDir, "btmp")
+	require.NoError(t, TouchFile(utmpPath))
+	require.NoError(t, TouchFile(wtmpPath))
+	require.NoError(t, TouchFile(btmpPath))
 	s.utmpPath = utmpPath
+	s.wtmpPath = wtmpPath
+	s.btmpPath = btmpPath
 
 	lockWatcher, err := services.NewLockWatcher(ctx, services.LockWatcherConfig{
 		ResourceWatcherConfig: services.ResourceWatcherConfig{
@@ -297,7 +336,7 @@ func newSrvCtx(ctx context.Context, t *testing.T) *SrvCtx {
 		regular.SetBPF(&bpf.NOP{}),
 		regular.SetRestrictedSessionManager(&restricted.NOP{}),
 		regular.SetClock(s.clock),
-		regular.SetUtmpPath(utmpPath, utmpPath),
+		regular.SetUserAccountingPaths(utmpPath, wtmpPath, btmpPath),
 		regular.SetLockWatcher(lockWatcher),
 		regular.SetSessionController(nodeSessionController),
 	)

lib/srv/ctx.go

@@ -166,8 +166,8 @@ type Server interface {
 	// Context returns server shutdown context
 	Context() context.Context
 
-	// GetUtmpPath returns the path of the user accounting database and log. Returns empty for system defaults.
-	GetUtmpPath() (utmp, wtmp string)
+	// GetUserAccountingPaths returns the path of the user accounting database and log. Returns empty for system defaults.
+	GetUserAccountingPaths() (utmp, wtmp, btmp string)
 
 	// GetLockWatcher gets the server's lock watcher.
 	GetLockWatcher() *services.LockWatcher
@@ -1279,13 +1279,14 @@ func newUaccMetadata(c *ServerContext) (*UaccMetadata, error) {
 	if err != nil {
 		return nil, trace.Wrap(err)
 	}
-	utmpPath, wtmpPath := c.srv.GetUtmpPath()
+	utmpPath, wtmpPath, btmpPath := c.srv.GetUserAccountingPaths()
 
 	return &UaccMetadata{
 		Hostname:   hostname,
 		RemoteAddr: preparedAddr,
 		UtmpPath:   utmpPath,
 		WtmpPath:   wtmpPath,
+		BtmpPath:   btmpPath,
 	}, nil
 }
 

lib/srv/forward/sshserver.go

@@ -518,11 +518,11 @@ func (s *Server) GetClock() clockwork.Clock {
 	return s.clock
 }
 
-// GetUtmpPath returns the optional override of the utmp and wtmp path.
-// These values are never set for the forwarding server because utmp and wtmp
+// GetUserAccountingPaths returns the optional override of the utmp, wtmp, and btmp path.
+// These values are never set for the forwarding server because utmp, wtmp, and btmp
 // are updated by the target server and not the forwarding server.
-func (s *Server) GetUtmpPath() (string, string) {
-	return "", ""
+func (s *Server) GetUserAccountingPaths() (string, string, string) {
+	return "", "", ""
 }
 
 // GetLockWatcher gets the server's lock watcher.

lib/srv/mock.go

@@ -263,9 +263,9 @@ func (m *mockServer) Context() context.Context {
 	return context.Background()
 }
 
-// GetUtmpPath returns the path of the user accounting database and log. Returns empty for system defaults.
-func (m *mockServer) GetUtmpPath() (utmp, wtmp string) {
-	return "test", "test"
+// GetUserAccountingPaths returns the path of the user accounting database and log. Returns empty for system defaults.
+func (m *mockServer) GetUserAccountingPaths() (utmp, wtmp, btmp string) {
+	return "test", "test", "test"
 }
 
 // GetLockWatcher gets the server's lock watcher.

lib/srv/reexec.go

@@ -193,6 +193,9 @@ type UaccMetadata struct {
 
 	// WtmpPath is the path of the system wtmp log.
 	WtmpPath string `json:"wtmp_path,omitempty"`
+
+	// BtmpPath is the path of the system btmp log.
+	BtmpPath string `json:"btmp_path,omitempty"`
 }
 
 // RunCommand reads in the command to run from the parent process (over a
@@ -285,13 +288,6 @@ func RunCommand() (errw io.Writer, code int, err error) {
 			return errorWriter, teleport.RemoteCommandFailure, trace.BadParameter("pty and tty not found")
 		}
 		errorWriter = tty
-		err = uacc.Open(c.UaccMetadata.UtmpPath, c.UaccMetadata.WtmpPath, c.Login, c.UaccMetadata.Hostname, c.UaccMetadata.RemoteAddr, tty)
-		// uacc support is best-effort, only enable it if Open is successful.
-		// Currently, there is no way to log this error out-of-band with the
-		// command output, so for now we essentially ignore it.
-		if err == nil {
-			uaccEnabled = true
-		}
 	}
 
 	// If PAM is enabled, open a PAM context. This has to be done before anything
@@ -347,9 +343,24 @@ func RunCommand() (errw io.Writer, code int, err error) {
 
 	localUser, err := user.Lookup(c.Login)
 	if err != nil {
+		if uaccErr := uacc.LogFailedLogin(c.UaccMetadata.BtmpPath, c.Login, c.UaccMetadata.Hostname, c.UaccMetadata.RemoteAddr); uaccErr != nil {
+			log.WithError(uaccErr).Debug("uacc unsupported.")
+		}
 		return errorWriter, teleport.RemoteCommandFailure, trace.Wrap(err)
 	}
 
+	if c.Terminal {
+		err = uacc.Open(c.UaccMetadata.UtmpPath, c.UaccMetadata.WtmpPath, c.Login, c.UaccMetadata.Hostname, c.UaccMetadata.RemoteAddr, tty)
+		// uacc support is best-effort, only enable it if Open is successful.
+		// Currently, there is no way to log this error out-of-band with the
+		// command output, so for now we essentially ignore it.
+		if err == nil {
+			uaccEnabled = true
+		} else {
+			log.WithError(err).Debug("uacc unsupported.")
+		}
+	}
+
 	// Build the actual command that will launch the shell.
 	cmd, err := buildCommand(&c, localUser, tty, pty, pamEnvironment)
 	if err != nil {

lib/srv/regular/sshserver.go

@@ -184,6 +184,9 @@ type Server struct {
 	// wtmpPath is the path to the user accounting s.Logger.
 	wtmpPath string
 
+	// btmpPath is the path to the user accounting failed login log.
+	btmpPath string
+
 	// allowTCPForwarding indicates whether the ssh server is allowed to offer
 	// TCP port forwarding.
 	allowTCPForwarding bool
@@ -265,9 +268,9 @@ func (s *Server) GetAccessPoint() srv.AccessPoint {
 	return s.authService
 }
 
-// GetUtmpPath returns the optional override of the utmp and wtmp path.
-func (s *Server) GetUtmpPath() (string, string) {
-	return s.utmpPath, s.wtmpPath
+// GetUserAccountingPaths returns the optional override of the utmp, wtmp, and btmp paths.
+func (s *Server) GetUserAccountingPaths() (string, string, string) {
+	return s.utmpPath, s.wtmpPath, s.btmpPath
 }
 
 // GetPAM returns the PAM configuration for this server.
@@ -404,11 +407,12 @@ func (s *Server) HandleConnection(conn net.Conn) {
 	s.srv.HandleConnection(conn)
 }
 
-// SetUtmpPath is a functional server option to override the user accounting database and log path.
-func SetUtmpPath(utmpPath, wtmpPath string) ServerOption {
+// SetUserAccountingPaths is a functional server option to override the user accounting database and log path.
+func SetUserAccountingPaths(utmpPath, wtmpPath, btmpPath string) ServerOption {
 	return func(s *Server) error {
 		s.utmpPath = utmpPath
 		s.wtmpPath = wtmpPath
+		s.btmpPath = btmpPath
 		return nil
 	}
 }