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

[v9] Make relogin attempts use the strongest auth method (#11781) #11847

Merged
merged 2 commits into from
Apr 11, 2022

Conversation

codingllama
Copy link
Contributor

@codingllama codingllama commented 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.

@github-actions github-actions bot added the tsh tsh - Teleport's command line tool for logging into nodes running Teleport. label Apr 8, 2022
@codingllama codingllama changed the title Make relogin attempts use the strongest auth method (#11781) [v9] Make relogin attempts use the strongest auth method (#11781) Apr 8, 2022
@github-actions github-actions bot requested review from greedy52, smallinsky and zmb3 April 8, 2022 21:08
@codingllama codingllama removed the request for review from smallinsky April 8, 2022 21:08
@codingllama
Copy link
Contributor Author

The commit doesn't backport cleanly, so there is a second commit with a couple fixes.

@codingllama codingllama force-pushed the codingllama/v9-hungry-hungry-tsh branch from f4862e5 to 5946a92 Compare April 11, 2022 14:39
@codingllama codingllama enabled auto-merge (squash) April 11, 2022 14:40
@codingllama codingllama force-pushed the codingllama/v9-hungry-hungry-tsh branch from 5946a92 to 877791f Compare April 11, 2022 15:39
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 codingllama force-pushed the codingllama/v9-hungry-hungry-tsh branch from 877791f to 1227bdd Compare April 11, 2022 16:42
@codingllama codingllama merged commit d0efa1d into branch/v9 Apr 11, 2022
@codingllama codingllama deleted the codingllama/v9-hungry-hungry-tsh branch April 11, 2022 17:05
@webvictim webvictim mentioned this pull request Apr 19, 2022
@webvictim webvictim mentioned this pull request Jun 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport tsh tsh - Teleport's command line tool for logging into nodes running Teleport.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants