Skip to content

Fix session tracker backwards compatibility.#12795

Merged
Joerger merged 10 commits into
masterfrom
joerger/v10/fix-old-auth-prevents-session-tracking
May 25, 2022
Merged

Fix session tracker backwards compatibility.#12795
Joerger merged 10 commits into
masterfrom
joerger/v10/fix-old-auth-prevents-session-tracking

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented May 20, 2022

Forward port of #12728

This isn't strictly necessary, since v10 auth servers aren't expected to be fully compatible with v9 services (nodes, etc.), but it's an easy forward port and was requested.

@Joerger Joerger requested a review from r0mant May 20, 2022 19:21
@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 20, 2022
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

I'm not super familiar with event upload logic, so it's a bit hard to reason about the fact that all AccessDenied errors are safe to swallow.

Is it worth trying to put a test together for this situation?

What do we get by forward porting if it's not necessary? Could this have undesirable side-effects?

Comment thread lib/events/complete.go Outdated
Comment thread lib/srv/app/session.go Outdated
Comment thread lib/srv/db/server.go Outdated
@Joerger Joerger force-pushed the joerger/v10/fix-old-auth-prevents-session-tracking branch from 354818f to ff0f9b9 Compare May 23, 2022 18:24
@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented May 23, 2022

I'm not super familiar with event upload logic, so it's a bit hard to reason about the fact that all AccessDenied errors are safe to swallow.

We expect to get NotFound from v8 and earlier v9 versions, and have the grace period backwards compatibility logic to handle this case -

// DELETE IN 11.0.0
// To support v9/v8 versions which do not use SessionTrackerService,
// sessions are only considered abandoned after the provided grace period.
if u.cfg.GracePeriod != 0 {

With this change we are essentially handling AccessDenied errors in the same way, and defaulting to the grace period logic. So even if we got AccessDenied for another reason (although we never should), then we still have the grace period to fall back on.

Is it worth trying to put a test together for this situation?

Sure, I've added it to the grace period test.

What do we get by forward porting if it's not necessary? Could this have undesirable side-effects?

Although we don't formally support v9 auth with v10 db/app/desktop services, users still do it frequently enough that it would be better from a support standpoint handle fatal errors like this one, which returns an error for any session started with these services.

@Joerger Joerger requested a review from codingllama May 23, 2022 18:27
Copy link
Copy Markdown
Contributor

@codingllama codingllama left a comment

Choose a reason for hiding this comment

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

Thanks for the explanation, Brian. Very helpful.

Comment thread lib/events/complete_test.go Outdated
Co-authored-by: Alan Parra <alan.parra@goteleport.com>
@Joerger Joerger force-pushed the joerger/v10/fix-old-auth-prevents-session-tracking branch from 32e31d3 to 3f52897 Compare May 23, 2022 21:12
@Joerger Joerger enabled auto-merge (squash) May 23, 2022 21:12
@Joerger Joerger merged commit 5ac508a into master May 25, 2022
Joerger added a commit that referenced this pull request Oct 5, 2022
Joerger added a commit that referenced this pull request Oct 6, 2022
* Complete deprecation for OIDC RedirectURL - #12054.

* Complete deprecation for CreateSessionTracker - #12304.

* Complete deprecation for SSO Auth Request http endpoints - #13073.

* Complete deprecation for #12795.

* Complete deprecation for http GenerateToken - #9024.
Joerger added a commit that referenced this pull request Oct 6, 2022
* Complete deprecation for OIDC RedirectURL - #12054.

* Complete deprecation for CreateSessionTracker - #12304.

* Complete deprecation for SSO Auth Request http endpoints - #13073.

* Complete deprecation for #12795.

* Complete deprecation for http GenerateToken - #9024.
Joerger added a commit that referenced this pull request Oct 6, 2022
* Complete deprecation for OIDC RedirectURL - #12054.

* Complete deprecation for CreateSessionTracker - #12304.

* Complete deprecation for SSO Auth Request http endpoints - #13073.

* Complete deprecation for #12795.

* Complete deprecation for http GenerateToken - #9024.
@Joerger Joerger deleted the joerger/v10/fix-old-auth-prevents-session-tracking branch March 20, 2023 19:07
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