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

[v9] Fix session tracker backwards compatibility. #12728

Merged
merged 1 commit into from
May 17, 2022

Conversation

Joerger
Copy link
Contributor

@Joerger Joerger commented May 17, 2022

Ignore access denied errors when creating/getting a session tracker as db, app, or windows desktop service. This fixes compatibility with auth servers which are between v9.0.0 and v9.2.1, where access was only granted to proxy, kube, and node roles.

This change does not need to be applied to master (v10)

@github-actions github-actions bot added application-access audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs desktop-access labels May 17, 2022
@Joerger Joerger changed the title Fix session tracker backwards compatibility. [v9] Fix session tracker backwards compatibility. May 17, 2022
@Joerger Joerger force-pushed the joerger/fix-old-auth-prevents-session-tracking branch 2 times, most recently from 117c8f0 to 2840f3f Compare May 17, 2022 21:28
// Ignore access denied errors, which we may get if the auth
// server is v9.2.1 or earlier, since only node, proxy, and
// kube roles had permission to create session trackers.
if !trace.IsAccessDenied(err) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What happens if you get access denied and session tracker is not actually created? Will anything break down the road?

Copy link
Contributor Author

@Joerger Joerger May 17, 2022

Choose a reason for hiding this comment

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

In this case we are treating that access denied error as a not found error, which is expected since the server can't create/get session trackers. This essentially just maintains the old grace period logic for app, db, and desktop servers with a pre-v9.2.2 auth server.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sounds good!

Copy link
Collaborator

@r0mant r0mant left a comment

Choose a reason for hiding this comment

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

lgtm with a better log message

lib/srv/db/server.go Outdated Show resolved Hide resolved
…s db, app, or windows desktop service.

Add go.work files to .gitignore.
@Joerger Joerger force-pushed the joerger/fix-old-auth-prevents-session-tracking branch from 2840f3f to 84486f7 Compare May 17, 2022 21:59
@Joerger Joerger enabled auto-merge (squash) May 17, 2022 22:00
Copy link
Contributor

@probakowski probakowski left a comment

Choose a reason for hiding this comment

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

LGTM, one super small nit


# Go workspace files
go.work
go.work.sum
Copy link
Contributor

Choose a reason for hiding this comment

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

add new line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry I had auto-merge on 😅

@Joerger Joerger merged commit 963ec39 into branch/v9 May 17, 2022
@Joerger Joerger deleted the joerger/fix-old-auth-prevents-session-tracking branch May 17, 2022 23:28
@webvictim webvictim mentioned this pull request Jun 8, 2022
@marcoandredinis marcoandredinis removed their request for review March 24, 2023 09:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
application-access audit-log Issues related to Teleports Audit Log database-access Database access related issues and PRs desktop-access
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants