Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add session_type and format to session.recording.access audit event event #47309

Merged

Conversation

gabrielcorado
Copy link
Contributor

@gabrielcorado gabrielcorado commented Oct 8, 2024

This is part of adding usage metrics for session recording access. This PR adds the fields format and session_type to the audit event. Possible values for format are json, yaml, text, and pty.

  • The session_type value comes from the access checker, which lists the last audit event from the recording to ensure the user can access the recording type. We had to update the actionForKindSession function to return this type based on the last recording event type.
  • format is set by the clients (if applicable) using the gRPC context value. The context was used instead of adding an additional value to the function call because it is only used to enhance the audit event and is not used in the streaming process. This simplifies the usage and avoids increasing the complexity of the streaming functions for metadata values.

Changelog: Include the format (indicates which format the session was accessed in) and session_type (represents the type of the recording, for example, ssh) fields for the session.recording.access audit event.

Example of audit event
{
  "cluster_name": "root.teleport.dev",
  "code": "T2012I",
  "ei": 0,
  "event": "session.recording.access",
  "format": "pty",
  "session_type": "ssh",
  "sid": "a395e5cd-f43a-4891-84d0-cc12efb85662",
  "time": "2024-10-08T05:06:07.713Z",
  "uid": "c4cefd6d-8ba4-48f1-aadf-93b1465cd49a",
  "user": "alice",
  "user_kind": 1
}

@github-actions github-actions bot added size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport. labels Oct 8, 2024
@strideynet
Copy link
Contributor

Is this related to a ticket ? I'm curious on the background around this task, I'm somewhat concerned about how we present information in the audit log that can be potentially spoofed by an attacker.

@greedy52
Copy link
Contributor

greedy52 commented Oct 11, 2024

Is this related to a ticket ? I'm curious on the background around this task, I'm somewhat concerned about how we present information in the audit log that can be potentially spoofed by an attacker.

https://github.com/gravitational/teleport/blob/master/rfd/0171-database-session-playback.md

Postgres PTY playback can be done through web or tsh play. For the web playback, player is run on Proxy. But you are right that for tsh, the format can be spoofed.

The event is mainly used to collect the usage/adoption data of this feature. To me, collecting the format is similar to collecting user agents, where it can be spoofed but it is not critical to the event itself and patterns can be collected even if there is an attacker.

If we really want to prevent spoofing though, some redesign of the backend api may be needed.

@gabrielcorado
Copy link
Contributor Author

gabrielcorado commented Nov 4, 2024

This is the same idea as the access_through from the Teleport Connect use event, where we want to understand which format users are consuming their sessions recordings. And as for there, we decided to keep this field as string so we don't require event changes if we add some other form of consuming the session recordings.

However, the value was added to the audit event as it was already available as always-present data when users access their recordings (which then are converted into prehog events). This is similar to user-agent reporting (as @greedy52 said), which is also present in the audit events (for example, on login events). We could take advantage of knowing the possible values and validate them (avoiding custom values), but it may not make it better (in terms of security).

@gabrielcorado
Copy link
Contributor Author

@strideynet @fheinecke Friendly ping.

api/metadata/metadata.go Outdated Show resolved Hide resolved
lib/auth/auth_with_roles.go Show resolved Hide resolved
lib/auth/auth_with_roles.go Show resolved Hide resolved
@gabrielcorado gabrielcorado added this pull request to the merge queue Nov 12, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 12, 2024
@gabrielcorado gabrielcorado added this pull request to the merge queue Nov 12, 2024
Merged via the queue into master with commit 1c1ec67 Nov 12, 2024
42 checks passed
@gabrielcorado gabrielcorado deleted the gabrielcorado/add-session-recording-access-new-fields branch November 12, 2024 21:21
@public-teleport-github-review-bot

@gabrielcorado See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Failed

gabrielcorado added a commit that referenced this pull request Nov 13, 2024
…vent event (#47309)

* feat: add `session_type` and `format` to session recording access event

* chore(metadata): change conditional
gabrielcorado added a commit that referenced this pull request Nov 13, 2024
…vent event (#47309)

* feat: add `session_type` and `format` to session recording access event

* chore(metadata): change conditional
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport/branch/v16 backport/branch/v17 size/md tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants