From 2ad312bfa6d10bb2da613a0e44d365d2ca8e8500 Mon Sep 17 00:00:00 2001 From: Zac Bergquist Date: Thu, 10 Oct 2024 10:22:59 -0600 Subject: [PATCH] Fix active session filtering for legacy sessions This code never worked correctly, but mostly went unnoticed because it is only triggered when using legacy roles prior to RoleV5. Prior to moderated sessions, RBAC for viewing active sessions was based on whether or not you could join a session as the OS login that is being used, along with a pseudo-resource of kind "ssh_session". With moderated sessions we introduced more flexible RBAC semantics that allow you to join sessions in different modes (peer, observer, moderator), even if you don't actually have permission to start sessions. In #11223 we decided that we need to support both types of RBAC checks (legacy checks against the "ssh_session" resource, and newer checks against the session_tracker and join_sessions policies). The code that was doing the legacy checks was flawed for two reasons: 1. It used (types.SessionTracker).GetKind() (which will always be "session_tracker") instead of (types.SessionTracker).GetSessionKind(). 2. When checking whether the session was SSH, it was checking for the legacy "ssh_session" value, instead of the "ssh" value that session trackers actually use. --- api/types/constants.go | 5 ++++- api/types/session_tracker.go | 5 +++++ lib/auth/auth_with_roles.go | 10 ++++----- lib/auth/auth_with_roles_test.go | 36 +++++++++++++++++++++++++++++++- 4 files changed, 49 insertions(+), 7 deletions(-) diff --git a/api/types/constants.go b/api/types/constants.go index 23e733a1d99d..66ea9bbb614a 100644 --- a/api/types/constants.go +++ b/api/types/constants.go @@ -106,7 +106,10 @@ const ( // KindSession is a recorded SSH session. KindSession = "session" - // KindSSHSession is an active SSH session. + // KindSSHSession represents an active SSH session in early versions of Teleport + // prior to the introduction of moderated sessions. Note that ssh_session is not + // a "real" resource, and it is never used as the "session kind" value in the + // session_tracker resource. KindSSHSession = "ssh_session" // KindWebSession is a web session resource diff --git a/api/types/session_tracker.go b/api/types/session_tracker.go index 2d205ca31e8f..8574c092db11 100644 --- a/api/types/session_tracker.go +++ b/api/types/session_tracker.go @@ -28,7 +28,12 @@ import ( // SessionKind is a type of session. type SessionKind string +// These represent the possible values for the kind field in session trackers. const ( + // SSHSessionKind is the kind used for session tracking with the + // session_tracker resource used in Teleport 9+. Note that it is + // different from the legacy [types.KindSSHSession] value that was + // used prior to the introduction of moderated sessions. SSHSessionKind SessionKind = "ssh" KubernetesSessionKind SessionKind = "k8s" DatabaseSessionKind SessionKind = "db" diff --git a/lib/auth/auth_with_roles.go b/lib/auth/auth_with_roles.go index 36eb6f997538..2bb1961df383 100644 --- a/lib/auth/auth_with_roles.go +++ b/lib/auth/auth_with_roles.go @@ -417,12 +417,12 @@ func (a *ServerWithRoles) CreateSessionTracker(ctx context.Context, tracker type return tracker, nil } -func (a *ServerWithRoles) filterSessionTracker(ctx context.Context, joinerRoles []types.Role, tracker types.SessionTracker, verb string) bool { +func (a *ServerWithRoles) filterSessionTracker(joinerRoles []types.Role, tracker types.SessionTracker, verb string) bool { // Apply RFD 45 RBAC rules to the session if it's SSH. // This is a bit of a hack. It converts to the old legacy format // which we don't have all data for, luckily the fields we don't have aren't made available // to the RBAC filter anyway. - if tracker.GetKind() == types.KindSSHSession { + if tracker.GetSessionKind() == types.SSHSessionKind { ruleCtx := &services.Context{User: a.context.User} ruleCtx.SSHSession = &session.Session{ Kind: tracker.GetSessionKind(), @@ -573,7 +573,7 @@ func (a *ServerWithRoles) GetSessionTracker(ctx context.Context, sessionID strin return nil, trace.Wrap(err) } - ok := a.filterSessionTracker(ctx, joinerRoles, tracker, types.VerbRead) + ok := a.filterSessionTracker(joinerRoles, tracker, types.VerbRead) if !ok { return nil, trace.NotFound("session %v not found", sessionID) } @@ -600,7 +600,7 @@ func (a *ServerWithRoles) GetActiveSessionTrackers(ctx context.Context) ([]types } for _, sess := range sessions { - ok := a.filterSessionTracker(ctx, joinerRoles, sess, types.VerbList) + ok := a.filterSessionTracker(joinerRoles, sess, types.VerbList) if ok { filteredSessions = append(filteredSessions, sess) } @@ -628,7 +628,7 @@ func (a *ServerWithRoles) GetActiveSessionTrackersWithFilter(ctx context.Context } for _, sess := range sessions { - ok := a.filterSessionTracker(ctx, joinerRoles, sess, types.VerbList) + ok := a.filterSessionTracker(joinerRoles, sess, types.VerbList) if ok { filteredSessions = append(filteredSessions, sess) } diff --git a/lib/auth/auth_with_roles_test.go b/lib/auth/auth_with_roles_test.go index d97f64aa3d75..4d45cf82cd5a 100644 --- a/lib/auth/auth_with_roles_test.go +++ b/lib/auth/auth_with_roles_test.go @@ -5387,7 +5387,41 @@ func TestGetActiveSessionTrackers(t *testing.T) { require.NoError(t, err) return getActiveSessionsTestCase{"no access with match expression", tracker, role, false} - }()} + }(), func() getActiveSessionsTestCase { + tracker, err := types.NewSessionTracker(types.SessionTrackerSpecV1{ + SessionID: "1", + Kind: string(types.SSHSessionKind), + }) + require.NoError(t, err) + + role, err := types.NewRoleWithVersion("dev", types.V3, types.RoleSpecV6{ + Allow: types.RoleConditions{ + AppLabels: types.Labels{"*": []string{"*"}}, + DatabaseLabels: types.Labels{"*": []string{"*"}}, + KubernetesLabels: types.Labels{"*": []string{"*"}}, + KubernetesResources: []types.KubernetesResource{ + {Kind: types.KindKubePod, Name: "*", Namespace: "*", Verbs: []string{"*"}}, + }, + NodeLabels: types.Labels{"*": []string{"*"}}, + NodeLabelsExpression: `contains(user.spec.traits["cluster_ids"], labels["cluster_id"]) || contains(user.spec.traits["sub"], labels["owner"])`, + Logins: []string{"{{external.sub}}"}, + WindowsDesktopLabels: types.Labels{"cluster_id": []string{"{{external.cluster_ids}}"}}, + WindowsDesktopLogins: []string{"{{external.sub}}", "{{external.windows_logins}}"}, + }, + Deny: types.RoleConditions{ + Rules: []types.Rule{ + { + Resources: []string{types.KindDatabaseServer, types.KindAppServer, types.KindSession, types.KindSSHSession, types.KindKubeService, types.KindSessionTracker}, + Verbs: []string{"list", "read"}, + }, + }, + }, + }) + require.NoError(t, err) + + return getActiveSessionsTestCase{"filter bug v3 role", tracker, role, false} + }(), + } for _, testCase := range testCases { t.Run(testCase.name, func(t *testing.T) {