diff --git a/client/ssh/server/jwt_test.go b/client/ssh/server/jwt_test.go index b2f3ac6a0..0d1ec5c41 100644 --- a/client/ssh/server/jwt_test.go +++ b/client/ssh/server/jwt_test.go @@ -16,6 +16,7 @@ import ( "testing" "time" + "github.com/gliderlabs/ssh" "github.com/golang-jwt/jwt/v5" cryptossh "golang.org/x/crypto/ssh" @@ -751,3 +752,76 @@ func TestJWTMultipleAudiences(t *testing.T) { }) } } + +// authRequestContext is the context passwordHandler sees: firstUser is what gliderlabs/ssh recorded +// from the first auth request of the connection, currentUser is the name in the request being handled. +type authRequestContext struct { + ssh.Context + firstUser string + currentUser string +} + +func (c authRequestContext) User() string { return c.firstUser } + +func (c authRequestContext) RemoteAddr() net.Addr { + return &net.TCPAddr{IP: net.IPv4(100, 64, 0, 2), Port: 12345} +} + +func (c authRequestContext) Value(key any) any { + if key == (preAuthConnKey{}) { + return authRequestConn{user: c.currentUser} + } + return nil +} + +type authRequestConn struct { + cryptossh.ConnMetadata + user string +} + +func (c authRequestConn) User() string { return c.user } + +// TestJWTRejectsUserNameChange tests that a token authorized for one OS user is not accepted +// once the client switches to another user name on the same connection. +func TestJWTRejectsUserNameChange(t *testing.T) { + jwksServer, privateKey, jwksURL := setupJWKSServer(t) + defer jwksServer.Close() + + const ( + issuer = "https://test-issuer.example.com" + audience = "test-audience" + ) + + server := New(&Config{ + JWT: &JWTConfig{ + Issuer: issuer, + Audiences: []string{audience}, + KeysLocation: jwksURL, + }, + }) + + testUserHash, err := sshuserhash.HashUserID("test-user") + require.NoError(t, err) + + server.UpdateSSHAuth(&sshauth.Config{ + UserIDClaim: sshauth.DefaultUserIDClaim, + AuthorizedUsers: []sshuserhash.UserIDHash{testUserHash}, + MachineUsers: map[string][]uint32{ + "alice": {0}, + "bob": {0}, + }, + }) + + token := generateValidJWT(t, privateKey, issuer, audience) + + assert.True(t, server.passwordHandler(authRequestContext{firstUser: "alice", currentUser: "alice"}, token), + "mapped OS user should be accepted") + assert.False(t, server.passwordHandler(authRequestContext{firstUser: "root", currentUser: "root"}, token), + "unmapped OS user should be rejected") + assert.False(t, server.passwordHandler(authRequestContext{firstUser: "alice", currentUser: "root"}, token), + "change to an unmapped OS user after the first request should be rejected") + assert.False(t, server.passwordHandler(authRequestContext{firstUser: "alice", currentUser: "bob"}, token), + "change to another mapped OS user after the first request should be rejected") + assert.False(t, server.passwordHandler(authRequestContext{firstUser: "root", currentUser: "alice"}, token), + "change to a mapped OS user after the first request should be rejected") +} diff --git a/client/ssh/server/server.go b/client/ssh/server/server.go index b850c8e70..e75fa2910 100644 --- a/client/ssh/server/server.go +++ b/client/ssh/server/server.go @@ -135,6 +135,9 @@ type authKey string // Used to track authenticated connections for status display and port forwarding. type connKey string +// preAuthConnKey is the context key under which the connection being authenticated is stored. +type preAuthConnKey struct{} + func newAuthKey(username string, remoteAddr net.Addr) authKey { return authKey(fmt.Sprintf("%s@%s", username, remoteAddr.String())) } @@ -431,7 +434,7 @@ func (s *Server) buildSessionInfo(state *sessionState) SessionInfo { remoteAddr := session.RemoteAddr().String() info := SessionInfo{ - Username: session.User(), + Username: sessionUser(session), RemoteAddress: remoteAddr, Command: cmd, JWTUsername: state.jwtUsername, @@ -686,6 +689,19 @@ func (s *Server) passwordHandler(ctx ssh.Context, password string) bool { remoteAddr := ctx.RemoteAddr() logger := s.getRequestLogger(ctx) + // ctx.User() is the name from the first auth request of the connection, while the session + // runs as the name from the request that succeeds. Like OpenSSH, refuse a change of user + // name between requests so the account that is authorized is the one the session runs as. + conn, ok := ctx.Value(preAuthConnKey{}).(cryptossh.ConnMetadata) + if !ok { + logger.Warn("SSH auth denied: connection being authenticated is unknown") + return false + } + if conn.User() != osUsername { + logger.Warnf("SSH auth denied: user name changed during authentication from %q to %q", osUsername, conn.User()) + return false + } + if err := s.ensureJWTValidator(); err != nil { logger.Errorf("JWT validator initialization failed: %v", err) return false @@ -904,6 +920,7 @@ func (s *Server) createSSHServer(addr net.Addr) (*ssh.Server, error) { if s.jwtEnabled { server.PasswordHandler = s.passwordHandler + server.ServerConfigCallback = trackPreAuthConn } hostKeyPEM := ssh.HostKeyPEM(s.hostKeyPEM) @@ -915,6 +932,16 @@ func (s *Server) createSSHServer(addr net.Addr) (*ssh.Server, error) { return server, nil } +// trackPreAuthConn stores the connection being authenticated in its context, which gives +// passwordHandler the user name of the auth request it is handling. +func trackPreAuthConn(ctx ssh.Context) *cryptossh.ServerConfig { + return &cryptossh.ServerConfig{ + PreAuthConnCallback: func(conn cryptossh.ServerPreAuthConn) { + ctx.SetValue(preAuthConnKey{}, conn) + }, + } +} + func (s *Server) storeRemoteForwardListener(key forwardKey, ln net.Listener) { s.mu.Lock() defer s.mu.Unlock() diff --git a/client/ssh/server/session_handlers.go b/client/ssh/server/session_handlers.go index 45b0fc3be..2e890d67c 100644 --- a/client/ssh/server/session_handlers.go +++ b/client/ssh/server/session_handlers.go @@ -12,10 +12,17 @@ import ( log "github.com/sirupsen/logrus" ) +// sessionUser returns the OS user name the session was authenticated for. The context keeps the +// name from the first auth request of the connection, which is the one passwordHandler checked, +// while sess.User() is the name from the last request the client sent and may differ from it. +func sessionUser(sess ssh.Session) string { + return sess.Context().User() +} + // associateJWTUsername extracts pending JWT username for the session and associates it with the session state. // Returns the JWT username (empty if none) for logging purposes. func (s *Server) associateJWTUsername(sess ssh.Session, sessionKey sessionKey) string { - key := newAuthKey(sess.User(), sess.RemoteAddr()) + key := newAuthKey(sessionUser(sess), sess.RemoteAddr()) s.mu.Lock() defer s.mu.Unlock() @@ -61,7 +68,7 @@ func (s *Server) sessionHandler(session ssh.Session) { defer drainWindowChanges(winCh) } - privilegeResult, err := s.userPrivilegeCheck(session.User()) + privilegeResult, err := s.userPrivilegeCheck(sessionUser(session)) if err != nil { s.handlePrivError(logger, session, err) return @@ -90,7 +97,7 @@ func (s *Server) registerSession(session ssh.Session, sessionType string) sessio shortID := hex.EncodeToString(hash[:4]) remoteAddr := session.RemoteAddr().String() - username := session.User() + username := sessionUser(session) sessionKey := sessionKey(fmt.Sprintf("%s@%s-%s", username, remoteAddr, shortID)) s.mu.Lock() diff --git a/client/ssh/server/sftp.go b/client/ssh/server/sftp.go index 199444abb..15712bdbf 100644 --- a/client/ssh/server/sftp.go +++ b/client/ssh/server/sftp.go @@ -43,7 +43,7 @@ func (s *Server) sftpSubsystemHandler(sess ssh.Session) { } result := s.CheckPrivileges(PrivilegeCheckRequest{ - RequestedUsername: sess.User(), + RequestedUsername: sessionUser(sess), FeatureSupportsUserSwitch: true, FeatureName: FeatureSFTP, })