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

Per-session MFA eats 1-2 keypresses (depending on devices) #13021

Closed
gabrielcossette opened this issue May 30, 2022 · 8 comments
Closed

Per-session MFA eats 1-2 keypresses (depending on devices) #13021

gabrielcossette opened this issue May 30, 2022 · 8 comments
Assignees
Labels

Comments

@gabrielcossette
Copy link

Expected behavior

Once the remote prompt appears, all input should get sent to the remote server.

Current behavior

With "Per-session MFA" enabled, depending on what device(s) you have added, tsh ssh logins may eat 1-2 keypresses.

Scenarios:

  1. OTP and WebAuthn added
    1. OTP used = 1 keypress
    2. WebAuthn used = 2 keypresses
  2. OTP only added = 1 keypress
  3. WebAuthn only added = 0 keypress

It looks a bit similar to #11709.

Bug details

Teleport version

9.3.0 (also tested with 9.2.4)

Recreation steps

  1. Enable "Per-session MFA"
  2. Add an OTP device (and optionally a 2nd WebAuthn one)
  3. Try to login in a device tsh ssh

Debug logs

Let me know if you would like them.

I'll be happy to help further, thanks!

@codingllama
Copy link
Contributor

Hey Gabriel, thanks for the report.

It's definitely #11709 in a new clothing. The solution we used there may be a bit difficult to implement here, as defaulting to the strongest auth (WebAuthn) could make starting ssh sessions from remote hosts not possible (aka #12675, but without mitigations).

I'll go back to the drawing board, find a way to reconcile all those issues and see what I can put together.

@codingllama codingllama self-assigned this Jun 2, 2022
codingllama added a commit that referenced this issue Jun 3, 2022
Avoid "input swallowing" bugs by disabling stdin hijacking by default.

Only `tsh login` is allowed to hijack stdin, as it is expected to exit right
after authentication. Any MFA authentication attempts resulting from
non-`tsh login` invocations default to the user's strongest auth method.

Defaulting to the strongest auth method can cause problems in constrained
environments for users that have both Webauthn and OTP registered. For example,
someone using `tsh` under WSL (Windows Subsystem for Linux) or a remote machine
could be locked into Webauthn MFA, which they can't use because their
environment lacks USB access or they don't have physical access to it. In order
to solve this problem I've slightly modified the meaning of the `--mfa-mode`
flag so `otp` can be specified.

The `TELEPORT_MFA_MODE` environment variable may be set to avoid constant flag
passing.

Fixes #12675 and #13021.

* Expand --mfa-mode and disable stdin hijack by default
* Use TELEPORT_ instead of TSH_ for FIDO2 env var
* Use t.Setenv in tests
@codingllama
Copy link
Contributor

I've put together a fix that should land in master shortly (I'll port it to v9 after it lands).

It's similar to the fix #11709, but this time I've gone ahead and made anything that isn't tsh login default to the strongest MFA method. If that doesn't work for you (eg, remote machine), then you can use the --mfa-mode flag to specify a different MFA method (eg, tsh --mfa-mode=otp ssh llama@themachine). Setting the TELEPORT_MFA_MODE environment variable also works.

codingllama added a commit that referenced this issue Jun 6, 2022
Avoid "input swallowing" bugs by disabling stdin hijacking by default.

Only `tsh login` is allowed to hijack stdin, as it is expected to exit right
after authentication. Any MFA authentication attempts resulting from
non-`tsh login` invocations default to the user's strongest auth method.

Defaulting to the strongest auth method can cause problems in constrained
environments for users that have both Webauthn and OTP registered. For example,
someone using `tsh` under WSL (Windows Subsystem for Linux) or a remote machine
could be locked into Webauthn MFA, which they can't use because their
environment lacks USB access or they don't have physical access to it. In order
to solve this problem I've slightly modified the meaning of the `--mfa-mode`
flag so `otp` can be specified.

The `TELEPORT_MFA_MODE` environment variable may be set to avoid constant flag
passing.

Fixes #12675 and #13021.

* Expand --mfa-mode and disable stdin hijack by default
* Use TELEPORT_ instead of TSH_ for FIDO2 env var
* Use t.Setenv in tests
codingllama added a commit that referenced this issue Jun 8, 2022
Avoid "input swallowing" bugs by disabling stdin hijacking by default.

Only `tsh login` is allowed to hijack stdin, as it is expected to exit right
after authentication. Any MFA authentication attempts resulting from
non-`tsh login` invocations default to the user's strongest auth method.

Defaulting to the strongest auth method can cause problems in constrained
environments for users that have both Webauthn and OTP registered. For example,
someone using `tsh` under WSL (Windows Subsystem for Linux) or a remote machine
could be locked into Webauthn MFA, which they can't use because their
environment lacks USB access or they don't have physical access to it. In order
to solve this problem I've slightly modified the meaning of the `--mfa-mode`
flag so `otp` can be specified.

The `TELEPORT_MFA_MODE` environment variable may be set to avoid constant flag
passing.

Fixes #12675 and #13021.

* Expand --mfa-mode and disable stdin hijack by default
* Use TELEPORT_ instead of TSH_ for FIDO2 env var
* Use t.Setenv in tests
@codingllama
Copy link
Contributor

Backport just landed on v9, docs soon to follow. Closing the issue.

@gabrielcossette
Copy link
Author

Thanks a lot @codingllama for looking into this!

I just tested the fix. No more keypresses eaten for WebAuthn. But unfortunately, still 1 keypress is eaten when we use OTP (both when "OTP and WebAuthn" and "Only OTP" has been added).

Tested on 9.3.6.

Thanks again in advance!

@codingllama
Copy link
Contributor

Repro confirmed, doesn't seem to affect v10/master. I've backported a couple more changes I did a while back to v9, those make the all-consuming-stdin-read-loop a bit more gentle, which fixes it for me. PR on the way.

@codingllama
Copy link
Contributor

Thanks for the patience, @gabrielcossette !

@gabrielcossette
Copy link
Author

Awesome!
Can't complain, thanks for your awesome support @codingllama!

codingllama added a commit that referenced this issue Jun 24, 2022
`prompt.ContextReader`, along with various parts of `lib/utils/prompt`, where
re-written in master so we can alleviate input swallowing issues.

For various reasons I didn't backport all changes to v9, but the less-eager
input-swallowing loop is now what stands between us and issue #13021.

This isn't a backport of a specific PR, but instead a port of the entire
`lib/utils/prompt` package, as the PRs that touch the package unfortunately do
more than we want to backport. The interfaces are still compatible and I did
test various `tsh login` and `tsh ssh` scenarios.

I've thrown in #13382 for good measure, as well.

Closes #13021.

* Backport lib/utils/prompt improvements
* Fix lib/client tests
* Restore terminal state on exit (#13382)
@gabrielcossette
Copy link
Author

Release 9.3.9 fixes the remaining issue, thanks for your awesome work @codingllama!

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

No branches or pull requests

2 participants