Skip to content

[RFD Update] Clarify RBAC rule application for session joining#11223

Merged
xacrimon merged 8 commits intomasterfrom
joel/clarify-moderated-sessions-rbac
Mar 24, 2022
Merged

[RFD Update] Clarify RBAC rule application for session joining#11223
xacrimon merged 8 commits intomasterfrom
joel/clarify-moderated-sessions-rbac

Conversation

@xacrimon
Copy link
Copy Markdown
Contributor

@xacrimon xacrimon commented Mar 17, 2022

During some testing, Steven discovered some inconsistencies in the documented for moderated sessions. Notably, joining a session currently requires node access in addition to the join_policy checks in the Moderated Sessions RBAC model introduced in RFD 43.

I created #11144 to remedy this issue but I then learned about RFD 45 which conflicts with this. The issue lies in the system used to propagate session listing and metadata information to tsh which it uses for routing sessions. Moderated Sessions introduced a new system using the SessionTracker resource upon which filtering is done based on the RBAC join policies from Moderated Sessions. RFD 45 however which was made in parallel with RFD 43 introduces other RBAC checks for limiting access to join sessions but applies these to the legacy session metadata system that's handled by SessionService.

Currently, we require that any user in addition to the moderated sessions checks has access to a node in order to join. This hampers usability and flexibility in the model as a user always needs permission to initiate a session in order to join another one. The original RFD was ambigious if this was the case and this ended up in me implementing it one way which was perhaps not optimal and others assuming other functionality.

This creates a situation where I want to migrate SSH session routing to the new metadata system to get rid of technical debt and so we can get rid of the requirement for full node access in order to join a session. This conflicts with the checks introduced by RFD 45 however.

This PR revises and clarifies RFD 43 and RFD 45 with my proposal to handle this overlap and to remove the node access checks for joining.

This is posted as an RFD change so that we can have a discussion to make sure everyone is in board with getting rid of the current system of requiring full node access in order to join a session and solely rely on the restrictions imposed by Moderated Sessions RBAC policies and the optional deny rules introduced in RFD 45.

My suggested solution for this is to:

  • Get rid of the requirement to be able start a session on the node/pod for joining sessions by using a limited join-only SSH principal and by bypassing authz for session joining in Kubernetes, handling all session join authorization in the downstream moderated session authorizer that lives in each session state.
  • Migrate the SSH join client to rely upon SessionTracker instead of the legacy SessionService session metadata and access filtering.
  • Additionally apply any allow/deny rules as specified in RFD 45 to SessionTracker resource filtering as well so that the functionality introduced in that RFD is preserved and won't cause a breaking UX or access change. The rules weren't designed to be applied to filtering SessionTracker instances but this can be done with some retrofitting of the expression evaluator as the data contained in them is similar.

This strategy only results in one "breaking" change from the current system as of release 9.0.0 (and only for those that have v5 roles to enable moderated sessions in the first place) and that is to get rid of the node access checks for joining sessions. This is already the tone in the current Moderated Sessions documentation and is merely a miss in implementation caused by the interleaved creation of RFD 43 and 45 and a somewhat ambigous language in the RFD. As I understood, this is also how RFD 43 was interpreted but it still wasn't obvious.

I discussed the removal of node access checks with Xin some time ago but wanted to get another lookover since we ran into this.

PR #11144 will implement whatever the outcome of this RFD is and hopefully migrate to the new metadata system to move this forward.

@xacrimon xacrimon added rbac Issues related to Role Based Access Control rfd Request for Discussion moderated-sessions labels Mar 17, 2022
@github-actions github-actions Bot requested review from Tener and rosstimothy March 17, 2022 12:38
@xacrimon xacrimon requested review from klizhentas and removed request for Tener and rosstimothy March 17, 2022 12:41
@zmb3
Copy link
Copy Markdown
Collaborator

zmb3 commented Mar 17, 2022

Sorry @xacrimon I was looking at the wrong PR and accidentally clicked "Update Branch"

Copy link
Copy Markdown
Contributor

@xinding33 xinding33 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm all for this this change but I'd like another reviewer from the engineering side to weigh in on the security ramifications of removing the node access check requirement.

@klizhentas
Copy link
Copy Markdown
Contributor

@xacrimon to confirm the behavior after this change:

To join a session, users would have to only have permission to join the session using new RBAC controls. It would be possible to have users that won't be able to initiate sessions, but only join existing sessions. It will be also possible to have users who can only initiate sessions, but not join any sessions. By default, users in V5 RBAC roles won't have permissions to join any sessions in any capacity unless specified explicitly in the role. By default, users will be able to see their own active sessions, but not join them.

If I understand this behavior change correctly, I agree

@xacrimon
Copy link
Copy Markdown
Contributor Author

@klizhentas Yep, this is correct.

Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm no expert in the current session RBAC controls, but the change seems reasonable to me.

Comment thread rfd/0043-kubeaccess-multiparty.md Outdated
Comment thread rfd/0045-ssh_session-where-condition.md Outdated
@xacrimon xacrimon enabled auto-merge (squash) March 23, 2022 16:21
@xacrimon xacrimon merged commit b996135 into master Mar 24, 2022
@xacrimon xacrimon deleted the joel/clarify-moderated-sessions-rbac branch March 24, 2022 07:10
zmb3 added a commit that referenced this pull request Oct 10, 2024
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.
zmb3 added a commit that referenced this pull request Oct 10, 2024
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.
github-merge-queue Bot pushed a commit that referenced this pull request Oct 14, 2024
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.
github-actions Bot pushed a commit that referenced this pull request Oct 14, 2024
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.
github-actions Bot pushed a commit that referenced this pull request Oct 14, 2024
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.
github-actions Bot pushed a commit that referenced this pull request Oct 14, 2024
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.
github-merge-queue Bot pushed a commit that referenced this pull request Oct 15, 2024
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.
github-merge-queue Bot pushed a commit that referenced this pull request Oct 15, 2024
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.
github-merge-queue Bot pushed a commit that referenced this pull request Oct 15, 2024
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.
mvbrock pushed a commit that referenced this pull request Oct 16, 2024
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

moderated-sessions rbac Issues related to Role Based Access Control rfd Request for Discussion

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants