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

emit an audit event with file transfer statistics when an SFTP session is completed #48051

Merged
merged 4 commits into from
Oct 30, 2024

Conversation

capnspacehook
Copy link
Contributor

@capnspacehook capnspacehook commented Oct 29, 2024

When an SFTP session ends a new event will now be emitted listing all the files that were read/written to and the amount of bytes that were transferred per file. More fine grained info on SFTP requests during a session will be continued to be emitted as well.

This also removes some unused SFTP event codes. They were removed here in v13 so it should be completely fine to backport this all the way to v14, since we haven't been using these event codes in awhile.

Closes https://github.com/gravitational/customer-sensitive-requests/issues/336.

changelog: emit an audit event with file transfer information when an SFTP session is completed

@github-actions github-actions bot requested review from avatus and kiosion October 29, 2024 01:15
@github-actions github-actions bot added audit-log Issues related to Teleports Audit Log size/md ui labels Oct 29, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-48051.d3pp5qlev8mo18.amplifyapp.com

@zmb3
Copy link
Collaborator

zmb3 commented Oct 29, 2024

Why not restore the original SFTP write event?

tool/teleport/common/sftp.go Outdated Show resolved Hide resolved
tool/teleport/common/sftp.go Show resolved Hide resolved
web/packages/teleport/src/services/audit/types.ts Outdated Show resolved Hide resolved
@capnspacehook
Copy link
Contributor Author

The original SFTP write and other events were removed because they were too noisy, reading/writing to a file typically happens in multiple small requests instead of one large one even for small files. This consolidates all info on an SFTP session into one event. The potential downside is if someone keeps an SFTP connection open for a really long time the amount of data transferred would be emitted for a long time. Given the nature of the request for this event though I don't think that's a big concern though.

@capnspacehook capnspacehook requested a review from zmb3 October 29, 2024 16:20
@@ -1892,6 +1892,54 @@ message SFTP {
string Error = 12 [(gogoproto.jsontag) = "error,omitempty"];
}

// SFTPSummary is emitted at the end of an SFTP transfer
message SFTPSummary {
Copy link
Collaborator

Choose a reason for hiding this comment

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

How can I tell if this was a upload or download? Looks at bytes read vs written?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep, technically you can write and read to the same file handle so classifying isn't as straightforward as SCP. But for 99% of cases the program that is issuing SFTP requests will likely only be reading or writing to files, so I can label this if needed

@capnspacehook capnspacehook force-pushed the capnspacehook/sftp-summary branch from 7f05bcc to 0f3cb7b Compare October 29, 2024 22:00
lib/srv/regular/sftp.go Outdated Show resolved Hide resolved
tool/teleport/common/sftp.go Outdated Show resolved Hide resolved
@capnspacehook capnspacehook removed this pull request from the merge queue due to a manual request Oct 30, 2024
@capnspacehook capnspacehook force-pushed the capnspacehook/sftp-summary branch from 4f0b98d to 3bfd292 Compare October 30, 2024 18:33
@capnspacehook capnspacehook added this pull request to the merge queue Oct 30, 2024
Merged via the queue into master with commit 1589413 Oct 30, 2024
45 checks passed
@capnspacehook capnspacehook deleted the capnspacehook/sftp-summary branch October 30, 2024 19:10
@public-teleport-github-review-bot

@capnspacehook See the table below for backport results.

Branch Result
branch/v14 Failed
branch/v15 Failed
branch/v16 Failed
branch/v17 Create PR

@webvictim
Copy link
Contributor

Screenshot of the new audit event

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
audit-log Issues related to Teleports Audit Log backport/branch/v14 backport/branch/v15 backport/branch/v16 backport/branch/v17 sftp Issues related to Teleport's SFTP implementation size/md ui
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants