Skip to content

refactor SFTP backend to use upstream dep, not our fork#23786

Merged
capnspacehook merged 7 commits intomasterfrom
capnspacehook/sftp-backend-refactor
Apr 7, 2023
Merged

refactor SFTP backend to use upstream dep, not our fork#23786
capnspacehook merged 7 commits intomasterfrom
capnspacehook/sftp-backend-refactor

Conversation

@capnspacehook
Copy link
Copy Markdown
Contributor

@capnspacehook capnspacehook commented Mar 29, 2023

This change also greatly reduces the number of SFTP audit logs. Now SFTP events are only sent when files are opened or modified in any way, instead of for every SFTP request.

Fixes #21518.
Closes #22932.

@capnspacehook capnspacehook requested a review from jakule March 29, 2023 16:20
@capnspacehook capnspacehook force-pushed the capnspacehook/sftp-backend-refactor branch from 96e604b to 665074c Compare March 29, 2023 16:22
@github-actions github-actions Bot requested review from greedy52 and strideynet March 29, 2023 16:45
@github-actions github-actions Bot added audit-log Issues related to Teleports Audit Log size/md labels Mar 29, 2023
This change also greatly reduces the number of SFTP audit logs.
Now SFTP events are only sent when files are opened or modified
in any way, instead of for *every* SFTP request.
@capnspacehook capnspacehook force-pushed the capnspacehook/sftp-backend-refactor branch from 665074c to d1cb50e Compare March 29, 2023 20:21
@capnspacehook capnspacehook requested review from r0mant and removed request for jakule April 4, 2023 15:30
Comment thread lib/sshutils/sftp/local.go Outdated
}

err := os.MkdirAll(path, mode)
err := os.MkdirAll(path, 0o755)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Why are we no longer passing the mode but hardcoding it to 755 and 644 everywhere?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Modifications in our sftp fork allowed us to do that before, now we have to call Chmod directly after creating a file

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

@capnspacehook Can you backport your mkdir implementation upstream? I think that it should be accepted, and then the chmod trick won't be needed.

CC @r0mant

Comment thread tool/teleport/common/sftp.go Outdated
if !ok {
// We don't care about this type of SFTP request, move on
// Filecmd handles file modification requests.
func (s *sftpHandler) Filecmd(req *sftp.Request) (retErr error) {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

It's a shame we basically have to reimplement these command handlers ourselves. Are there no "default handlers" somewhere in the sftp package we could wrap to add auditing?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Nope, the closest thing is in-memory handlers for implementing a VFS which we obviously don't want

Comment thread lib/sshutils/sftp/sftp.go Outdated
@capnspacehook capnspacehook requested a review from r0mant April 6, 2023 17:26
@capnspacehook
Copy link
Copy Markdown
Contributor Author

Friendly ping @greedy52

@capnspacehook capnspacehook added sftp Issues related to Teleport's SFTP implementation and removed backport/branch/v10 labels Apr 6, 2023
@public-teleport-github-review-bot public-teleport-github-review-bot Bot removed the request for review from greedy52 April 7, 2023 00:54
@capnspacehook capnspacehook added this pull request to the merge queue Apr 7, 2023
Merged via the queue into master with commit 40c113b Apr 7, 2023
@capnspacehook capnspacehook deleted the capnspacehook/sftp-backend-refactor branch April 7, 2023 02:12
This was referenced Apr 19, 2023
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 sftp Issues related to Teleport's SFTP implementation size/md

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Switch our SFTP to upstream SFTP transfer produces too many audit events

4 participants