Get accessInfo based on user on access request drop#31068
Conversation
That's how it used to be before user login state was introduced. When dropping a resource access request, we want to restore certs back to the state before the access request was assumed, so that the user access is not limited only to select resources. In the past, this was done by calculating accessInfo from a plan user object. This approach had the side effect of refreshing the role list of the user based on the current backend state without the need to provide credentials again. Teleport Connect used this side effect to make the setup of Connect My Computer interaction-free. Theoretically, it'd be beneficial for `tsh request drop` to use login state rather than the current backend state, as it'd make it impossible to "escalate" privileges by refreshing the list of roles without authenticating again. However, this brakes the setup of Connect My Computer as it expects GenerateUserCerts to return a role list based on a current user role list. This commit reverts that change. An alternative would be to change Connect My Computer setup to require a one-time relogin midway through.
Tener
left a comment
Tener
left a comment
There was a problem hiding this comment.
If we are changing an important edge case, we should probably have test coverage for that. I'm not following the thread too closely, but would you be able to add one, possibly demonstrating the use case that Connect is benefiting from?
|
Yeah, sorry, I was trying to get this out ASAP before the test plan starts. The old behavior, which this PR restores, was "get access info from the user state". The current behavior is "get access info from the login state, if the login state is missing then get it from the user state". The tests for the old behavior were passing because they themselves didn't create any login state, so they didn't catch the regression. I suppose I could add a test which goes through a normal login procedure (and so it creates a login state), adds a role to the user and then verifies that dropping an access request gets the role list from the user and not the login state. The only reason the existing integration test didn't catch that was because it doesn't actually perform a regular login. |
|
I refactored the existing integration test to use a real login flow. It fails on current master. I also added some comments explaining when to prefer |
public-teleport-github-review-bot
Bot
removed the request for review
from hugoShaka
|
@ravicious See the table below for backport results.
|
4 participants
That's how it used to be before user login state was introduced (#29364). When dropping a resource access request, we want to restore certs back to the state before the access request was assumed, so that the user access is not limited only to select resources. In the past, this was done by calculating accessInfo from a plain user object.
This approach had the side effect of refreshing the role list of the user based on the current backend state, without the need to provide credentials again. Teleport Connect used this side effect to make the setup of Connect My Computer interaction-free.
Theoretically, it'd be beneficial for
tsh request dropto use login state rather than the current backend state, as it'd make it impossible to "escalate" privileges by refreshing the list of roles without authenticating again. However, this brakes the setup of Connect My Computer as it expects GenerateUserCerts to return a role list based on a current user role list.This commit reverts that change. Mike says this behavior change wasn't intentional and that we should revert it. It's also beneficial for Connect, as an alternative would be to change Connect My Computer setup to require a one-time relogin midway through.
See the discussion under #29364 for even more context.