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

Fix missing roles in access lists causing users to be locked out of their account #49456

Merged
merged 1 commit into from
Dec 16, 2024

Conversation

rudream
Copy link
Contributor

@rudream rudream commented Nov 26, 2024

Purpose

This PR resolves #43775

This PR fixes an issue where users would be logged out and locked out of their accounts if their access list references a role which no longer exists. This can occur when the role in the access list has a TTL and expires.

When generating the UserLoginState, any access lists which reference one or more non-existent roles will be skipped entirely, and an audit event is emitted as a notice.

image
image
image

changelog: Fix missing roles in access lists causing users to be locked out of their account

@fspmarshall
Copy link
Contributor

While use of deny rules in access lists is discouraged, we do not have a mechanism of preventing it, meaning that it isn't sane to skip roles as it might constitute a privilege escalation.

Because of the risk of malformed access lists breaking login, we've already made a single exception to this rule. We allow access list application to fail "atomically". I.e. we assume that failure to apply the entire access lists will not result in privilege escalation, and we make it the user's responsibility to only ever use access lists as a whole as a means of privilege escalation, not privilege limiting the way an individual role might be used.

We can solve missing roles locking out users, but it must be solved s.t. the entire access list that assigned the roles is skipped, rather than just skipping the specific role (i.e. none of the roles, traits, etc from that list get applied).

@rudream rudream force-pushed the yassine/access-list-missing-role branch from 9b32e7f to 8c8055b Compare December 11, 2024 23:24
@rudream
Copy link
Contributor Author

rudream commented Dec 11, 2024

We can solve missing roles locking out users, but it must be solved s.t. the entire access list that assigned the roles is skipped, rather than just skipping the specific role (i.e. none of the roles, traits, etc from that list get applied).

@fspmarshall After confirming with the customer, I've gone with this approach and updated this PR.

@zmb3
Copy link
Collaborator

zmb3 commented Dec 11, 2024

@smallinsky I would appreciate your review on this one too if you have a few minutes this week 🙏

@rudream rudream requested a review from smallinsky December 11, 2024 23:33
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/events/api.go Outdated Show resolved Hide resolved
@rudream rudream requested a review from smallinsky December 12, 2024 21:21
Copy link
Collaborator

@zmb3 zmb3 left a comment

Choose a reason for hiding this comment

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

Just some minor cleanup suggestions.

api/proto/teleport/legacy/types/events/events.proto Outdated Show resolved Hide resolved
api/proto/teleport/legacy/types/events/events.proto Outdated Show resolved Hide resolved
api/proto/teleport/legacy/types/events/events.proto Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Show resolved Hide resolved
lib/auth/userloginstate/generator.go Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
@marcoandredinis marcoandredinis removed their request for review December 13, 2024 08:30
@rudream rudream requested a review from zmb3 December 13, 2024 21:53
@rudream rudream force-pushed the yassine/access-list-missing-role branch from 8845fb9 to b91148a Compare December 13, 2024 22:02
Copy link
Contributor

@smallinsky smallinsky left a comment

Choose a reason for hiding this comment

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

LGTM.

lib/auth/userloginstate/generator.go Outdated Show resolved Hide resolved
@rudream rudream force-pushed the yassine/access-list-missing-role branch from b91148a to eccfbeb Compare December 16, 2024 18:48
@rudream rudream enabled auto-merge December 16, 2024 18:48
@rudream rudream added this pull request to the merge queue Dec 16, 2024
Merged via the queue into master with commit 5047402 Dec 16, 2024
46 checks passed
@rudream rudream deleted the yassine/access-list-missing-role branch December 16, 2024 19:25
@public-teleport-github-review-bot

@rudream See the table below for backport results.

Branch Result
branch/v16 Failed
branch/v17 Failed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User is locked out if role expires before Access List
4 participants