Fix issue where users would get locked out when using OTP tokens. #1383
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
russjones
force-pushed
the
rjones/real-fix-user-lock
branch
2 times, most recently
from
458bb73 to
4b4e036
Compare
klizhentas
approved these changes
Oct 11, 2017
klizhentas
approved these changes
Oct 11, 2017
russjones
force-pushed
the
rjones/real-fix-user-lock
branch
from
de5af00 to
3d0ce56
Compare
hatched
pushed a commit
that referenced
this pull request
Dec 20, 2022
* tshd events: Wait for listeners before responding When tshd is sending the relogin event, it needs to be able to know when the Electron app has finished relogging the user. I wanted to implement this by simply waiting for the response from the RPC. I'm so glad we did not use gRPC streams for tshd events as this would be much harder to implement with streams. * Return a function from ModalsService.openDialog which closes dialog With the introduction of important and regular modals, this will help us close the specific dialog if need arises. * Make tshd events listeners aware of request cancellation This will be useful in the upcoming commits. Basically, tshd is going to ask the Electron app to relogin the user, with a 1 minute timeout. The Electron app will show a login modal but if the user doesn't submit it within 1 minute, tshd is going to cancel the request. In that situation, we need to be able to close the modal. * Add support for passing reason in DialogClusterConnect * Add support for important modals This will let us show the relogin modal on expired cert, even if the user was using some other modal at that moment. * Remove title attr from notification text The user can read more by expanding the notification. The title attribute persisted even after expanding the notification, making reading it harder. * Add WindowsManager.forceFocusWindow * Use IAppContext instead of AppContext The next commit is going to add a private method to AppContext. IAppContext is an interface which enables us to pass a mocked version of AppContext in tests. That mock is not going have that private method, so any place accepting AppContext wouldn't be able to accept the mocked AppContext. Instead, classes & functions should accept IAppContext rather than AppContext. * Implement handlers for new tshd events tshd needs to be able to do two things: - Ask the user to log in again. - Forward errors from goroutines running gateways to the Electron app in form of a notification. Otherwise those error would be visible only in the logs. * Don't restart gateways after logging in Restarting the gateways on login was a workaround from times where gateways didn't manage their own certs. In the new flow, a gateway takes care of refreshing the certs itself through the middleware passed to alpnproxy.LocalProxy. syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two functions: - syncRootClusterAndCatchErrors - restartClusterGatewaysAndCatchErrors The second function is no longer necessary, so we can make any place that was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors call just syncRootClusterAndCatchErrors instead.
hatched
pushed a commit
that referenced
this pull request
Jan 30, 2023
…1416) * tshd events: Wait for listeners before responding When tshd is sending the relogin event, it needs to be able to know when the Electron app has finished relogging the user. I wanted to implement this by simply waiting for the response from the RPC. I'm so glad we did not use gRPC streams for tshd events as this would be much harder to implement with streams. * Return a function from ModalsService.openDialog which closes dialog With the introduction of important and regular modals, this will help us close the specific dialog if need arises. * Make tshd events listeners aware of request cancellation This will be useful in the upcoming commits. Basically, tshd is going to ask the Electron app to relogin the user, with a 1 minute timeout. The Electron app will show a login modal but if the user doesn't submit it within 1 minute, tshd is going to cancel the request. In that situation, we need to be able to close the modal. * Add support for passing reason in DialogClusterConnect * Add support for important modals This will let us show the relogin modal on expired cert, even if the user was using some other modal at that moment. * Remove title attr from notification text The user can read more by expanding the notification. The title attribute persisted even after expanding the notification, making reading it harder. * Add WindowsManager.forceFocusWindow * Use IAppContext instead of AppContext The next commit is going to add a private method to AppContext. IAppContext is an interface which enables us to pass a mocked version of AppContext in tests. That mock is not going have that private method, so any place accepting AppContext wouldn't be able to accept the mocked AppContext. Instead, classes & functions should accept IAppContext rather than AppContext. * Implement handlers for new tshd events tshd needs to be able to do two things: - Ask the user to log in again. - Forward errors from goroutines running gateways to the Electron app in form of a notification. Otherwise those error would be visible only in the logs. * Don't restart gateways after logging in Restarting the gateways on login was a workaround from times where gateways didn't manage their own certs. In the new flow, a gateway takes care of refreshing the certs itself through the middleware passed to alpnproxy.LocalProxy. syncRootClusterAndRestartClusterGatewaysAndCatchErrors used to call two functions: - syncRootClusterAndCatchErrors - restartClusterGatewaysAndCatchErrors The second function is no longer necessary, so we can make any place that was calling syncRootClusterAndRestartClusterGatewaysAndCatchErrors call just syncRootClusterAndCatchErrors instead.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Purpose
For local accounts to obtain SSH certificates, connect to an
AuthTunnelwith theAuthMethodbeing password, password+otp, or password+u2f. When we addedWithUserLockin #1306 we didn't take into account that the Go HTTP client would use a connection pool to make requests. Since the OTP token can only be used once and the HTTP client would open multiple connections, this causes the fail count to go up by 2 for every successful login. In addition, successful logins would not decrease the fail count.This means after two successful login with password+otp, you would be locked out of Teleport.
Implementation
The long term fix for this is to separate out obtain a SSH certificate from operations on the Auth Server. The short term fix is as follows.
TunClientto a single request. Added the/ca/user/certs/bundleendpoint and addedTunDisableRefreshto enable this.Related Issues
Fixes #1347