Moderated Session Role Filter Throws `illegal rune literal` When Using Single Quotes

programmerq commented 5 months ago

Expected Behavior

When defining a require_session_join filter in Teleport role configurations, the system should correctly parse string literals within the conditions for the filter, whether they are enclosed in single or double quotes, without causing session join failures.

Alternatively, Teleport could reject conditionals that include single quotes since they are known to cause this problem.

Current Behavior

A moderator role with a require_session_join filter using single quotes in the string literal throws a parsing error: illegal rune literal. For example, contains(user.spec.roles, 'team-auditor') will cause an auditor attempting to join a session as an observer to fail with the mentioned error.

Bug Details

Teleport Version

15.1.3

Recreation Steps

Create a moderated session. Connect as a user that has a role with a require_session_join filter using single quotes in the conditional statement.
Attempt to join the session as an observer with a role that should fulfill the filter condition.
Observe the 'illegal rune literal' error causing the join attempt to fail, and the SSH session fail.

Debug Logs

On the SSH node, when the party attempts to join, the node parses any conditionals specified in the role. The following stack trace appears in the Teleport node logs, and the user is disconnected:

2024-03-15T12:44:13Z INFO [SESSION:N] New party ServerContext(10.0.0.88:56088->77.88.66.77:60662, user=-teleport-internal-join, id=2) party(id=0c2f3a6b-1ad4-797b-a498-91d58453ee91) joined the session with participant mode: observer. session_id:75132169-8843-74e8-5b8c-6d82456a22ce srv/sess.go:1850
2024-03-15T12:44:13Z ERRO             failure handling SSH "shell" request error:[
ERROR REPORT:
Original Error: scanner.ErrorList 1:27: illegal rune literal
Stack Trace:
        github.com/gravitational/teleport/lib/auth/session_access.go:147 github.com/gravitational/teleport/lib/auth.(*SessionAccessEvaluator).matchesPredicate
        github.com/gravitational/teleport/lib/auth/session_access.go:312 github.com/gravitational/teleport/lib/auth.(*SessionAccessEvaluator).FulfilledFor
        github.com/gravitational/teleport/lib/srv/sess.go:1794 github.com/gravitational/teleport/lib/srv.(*session).checkIfStart
        github.com/gravitational/teleport/lib/srv/sess.go:1864 github.com/gravitational/teleport/lib/srv.(*session).addParty
        github.com/gravitational/teleport/lib/srv/sess.go:1921 github.com/gravitational/teleport/lib/srv.(*session).join
        github.com/gravitational/teleport/lib/srv/sess.go:315 github.com/gravitational/teleport/lib/srv.(*SessionRegistry).OpenSession
        github.com/gravitational/teleport/lib/srv/termhandlers.go:128 github.com/gravitational/teleport/lib/srv.(*TermHandlers).HandleShell
        github.com/gravitational/teleport/lib/srv/regular/sshserver.go:1688 github.com/gravitational/teleport/lib/srv/regular.(*Server).dispatch
        github.com/gravitational/teleport/lib/srv/regular/sshserver.go:1611 github.com/gravitational/teleport/lib/srv/regular.(*Server).handleSessionRequests
        github.com/gravitational/teleport/lib/srv/regular/sshserver.go:1378 github.com/gravitational/teleport/lib/srv/regular.(*Server).HandleNewChan.func1
        runtime/asm_amd64.s:1650 runtime.goexit
User Message: 1:27: illegal rune literal] regular/sshserver.go:2121
2024-03-15T12:44:13Z DEBU [TERM:LOCA] Closing TTY srv/term.go:341
2024-03-15T12:44:13Z DEBU [TERM:LOCA] Closed TTY srv/term.go:359
2024-03-15T12:44:13Z DEBU [TERM:LOCA] Closed PTY srv/term.go:379
2024-03-15T12:44:13Z INFO [SESSION:N] Closing party 0c2f3a6b-1ad4-797b-a498-91d58453ee91 srv/sess.go:2019
2024-03-15T12:44:13Z INFO [SESSION:N] Removing party ServerContext(10.0.0.88:56088->77.88.66.77:60662, user=-teleport-internal-join, id=2) party(id=0c2f3a6b-1ad4-797b-a498-91d58453ee91) from session session_id:75132169-8843-74e8-5b8c-6d82456a22ce srv/sess.go:1519
2024-03-15T12:44:13Z DEBU [SESSION:N] No longer tracking participant: 0c2f3a6b-1ad4-797b-a498-91d58453ee91 session_id:75132169-8843-74e8-5b8c-6d82456a22ce srv/sess.go:1527

Example role:

kind: role
metadata:
  name: team-access
spec:
  allow:
    logins:
    - ec2-user
    node_labels:
      '*': '*'
    require_session_join:
    - count: 1
      filter: contains(user.spec.roles, 'team-auditor')
      kinds:
      - ssh
      modes:
      - observer
      name: Auditor oversight
      on_leave: ""
  deny: {}

Notes

It may be beneficial to include input validation when saving role in the first place. In this particular case, it might also work for it to automatically replace single quotes with double quotes. It is very common for single quotes and double quotes to be relatively interchangeable, so this is likely to trip up users in the future.

zmb3 commented 5 months ago

the system should correctly parse string literals within the conditions for the filter, whether they are enclosed in single or double quotes

Our predicate language is based on Go syntax. Single quotes are not valid string literals in Go, and allowing our predicate language to diverge from Go would be a fair amount of work for little gain.

The proper fix here IMO would be to reject the request that creates or updates a role with an invalid filter, not to make this invalid filter valid.

zmb3 commented 3 months ago

As a side note - the same issue applies for label expressions. They aren't evaluated on create/update, so Teleport will accept a role update with an invalid label expression only to fail later on when the role is evaluated.

cc @nklaassen - maybe a good thing to clean up on a support shift in the future.

gravitational / teleport