Make relogin attempts use the strongest auth method #11781
Merged
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
github-actions
bot
added
the
tsh
|
Tested on master using tsh both with and without In regards to the stdin hijack, the problem is a bit more nuanced than the description. There are roughly two ways in which we read from stdin: I think there are two viable technical options out of this: 1. taking full control of the terminal (raw mode) or 2. running tc.Login in a separate process. 1. is tough on portability, likely difficult to implement and may or may not solve anything. 2. makes tsh way more complex than I would like. The TL;DR is that a UX change that avoids the issue is the best call. @zmb3 took this up with some folks and we seem to be happy with this workaround, so here it is. |
zmb3
approved these changes
Apr 6, 2022
|
Confirmed that changes work for v9 (codingllama/v9-hungry-hungry-tsh). Prepping a v8 backport now. |
codingllama
force-pushed
the
codingllama/hungry-tsh-ssh
branch
from
24a6db1
to
71acd0b
Compare
|
Friendly ping @greedy52 @smallinsky ? |
greedy52
approved these changes
Apr 7, 2022
codingllama
force-pushed
the
codingllama/hungry-tsh-ssh
branch
2 times, most recently
from
f8bba4e
to
95d9117
Compare
|
Thanks, @greedy52. @smallinsky, let me know if you'd like to take a look or if you are happy for me to merge with the current approvals. |
codingllama
force-pushed
the
codingllama/hungry-tsh-ssh
branch
from
95d9117
to
c862cee
Compare
|
I'll go ahead and merge this one so I can get the backports going. Happy to address any comments in a future PR. |
codingllama
added a commit
that referenced
this pull request
Apr 8, 2022
Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).
The problematic scenario is as follows:
1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
`tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"
In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.
Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.
Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.
Fixes #11709.
* Add UseStrongestAuth flag to PromptMFAChallenge
* Add TeleportClient.UseStrongestAuth and set it true for relogin
* Proper testing
* Address review comments
codingllama
added a commit
that referenced
this pull request
Apr 8, 2022
Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).
The problematic scenario is as follows:
1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
`tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"
In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.
Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.
Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.
Issue #11709.
codingllama
added a commit
that referenced
this pull request
Apr 11, 2022
Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).
The problematic scenario is as follows:
1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
`tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"
In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.
Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.
Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.
Issue #11709.
codingllama
added a commit
that referenced
this pull request
Apr 11, 2022
…1848) Fixes a potential stdin hijacking bug by making relogin attempts default to a single MFA method (the strongest available). The problematic scenario is as follows: 1. User has both OTP and security keys registered 2. "Relogin" is triggered via a tsh command (say, `tsh logout; tsh ssh --proxy=example.com llama@myserver`) 3. User is prompted to pick either OTP or security key ("Tap any security key or enter a code from a OTP device") 4. An stdin read is fired in the background to read the OTP code (via prompt.Stdin) 5. User picks the security method, thus the stdin read is "abandoned" In most cases this is fine, as the program ends right after. The issue is when a relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in this case the stdin hijack causes input to be swallowed. Forcing a single MFA option avoids the potential stdin hijack, fixing the problem for all relogin invocations. `tsh login` behavior remains the same. Note that we have to default to cluster's most secure method _without_ checking the user devices. The user is not logged in yet, thus the backend cannot reveal any information about that user. Issue #11709. * Make relogin attempts use the strongest auth method (#11781) * Fix conflicts for v8
codingllama
added a commit
that referenced
this pull request
Apr 11, 2022
Fixes a potential stdin hijacking bug by making relogin attempts default to a
single MFA method (the strongest available).
The problematic scenario is as follows:
1. User has both OTP and security keys registered
2. "Relogin" is triggered via a tsh command (say,
`tsh logout; tsh ssh --proxy=example.com llama@myserver`)
3. User is prompted to pick either OTP or security key ("Tap any security key or
enter a code from a OTP device")
4. An stdin read is fired in the background to read the OTP code (via
prompt.Stdin)
5. User picks the security method, thus the stdin read is "abandoned"
In most cases this is fine, as the program ends right after. The issue is when a
relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in
this case the stdin hijack causes input to be swallowed.
Forcing a single MFA option avoids the potential stdin hijack, fixing the
problem for all relogin invocations. `tsh login` behavior remains the same.
Note that we have to default to cluster's most secure method _without_ checking
the user devices. The user is not logged in yet, thus the backend cannot reveal
any information about that user.
Fixes #11709.
* Add UseStrongestAuth flag to PromptMFAChallenge
* Add TeleportClient.UseStrongestAuth and set it true for relogin
* Proper testing
* Address review comments
codingllama
added a commit
that referenced
this pull request
Apr 11, 2022
…1847) Fixes a potential stdin hijacking bug by making relogin attempts default to a single MFA method (the strongest available). The problematic scenario is as follows: 1. User has both OTP and security keys registered 2. "Relogin" is triggered via a tsh command (say, `tsh logout; tsh ssh --proxy=example.com llama@myserver`) 3. User is prompted to pick either OTP or security key ("Tap any security key or enter a code from a OTP device") 4. An stdin read is fired in the background to read the OTP code (via prompt.Stdin) 5. User picks the security method, thus the stdin read is "abandoned" In most cases this is fine, as the program ends right after. The issue is when a relogin is triggered by a long living tsh invocation (again, `tsh ssh ...`): in this case the stdin hijack causes input to be swallowed. Forcing a single MFA option avoids the potential stdin hijack, fixing the problem for all relogin invocations. `tsh login` behavior remains the same. Note that we have to default to cluster's most secure method _without_ checking the user devices. The user is not logged in yet, thus the backend cannot reveal any information about that user. Issue #11709. * Make relogin attempts use the strongest auth method (#11781) * Fix conflicts for v9
Closed
This was referenced May 16, 2022
Closed
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.
Fixes a potential stdin hijacking bug by making relogin attempts default to a single MFA method (the strongest available).
The problematic scenario is as follows:
tsh logout; tsh ssh --proxy=example.com llama@myserver)In most cases this is fine, as the program ends right after. The issue is when a relogin is triggered by a long living tsh invocation (again,
tsh ssh ...): in this case the stdin hijack causes input to be swallowed.Forcing a single MFA option avoids the potential stdin hijack, fixing the problem for all relogin invocations.
tsh loginbehavior remains the same.Note that we have to default to cluster's most secure method without checking the user devices. The user is not logged in yet, thus the backend cannot reveal any information about that user.
Fixes #11709.