Connect: Use SSHAgentLogin when second_factor is set to optional or on

[ravicious]

Fixes gravitational/webapps#793.

tsh always uses client.SSHAgentMFALogin for any second_factor option other than off and otp.

teleport/lib/client/api.go

Lines 2686 to 2696 in 8e0421e

	// TODO(awly): mfa: ideally, clients should always go through mfaLocalLogin
	// (with a nop MFA challenge if no 2nd factor is required). That way we can
	// deprecate the direct login endpoint.
	switch secondFactor {
	case constants.SecondFactorOff, constants.SecondFactorOTP:
	response, err = tc.directLogin(ctx, secondFactor, pub)
	if err != nil {
	return nil, trace.Wrap(err)
	}
	case constants.SecondFactorU2F, constants.SecondFactorWebauthn, constants.SecondFactorOn, constants.SecondFactorOptional:
	response, err = tc.mfaLocalLogin(ctx, pub)

If it's set to on or optional and it turns out the user wants to use an OTP, it bails out to stdin to ask them for it.

Connect currently uses the same logic in its switch branches. However, Connect cannot bail out to stdin, but it still wants to use the auth code from lib/client that it shares with tsh.

So, to temporarily work around this problem, this PR makes it so that we check first if the OTP token was submitted. If yes, then we use client.SSHAgentLogin, which lets us provide the token (which we got from the Electron app) and skip asking for it over stdin. If not, we use client.SSHAgentMFALogin which should handle auth methods that don't use OTP.

---

I'm not 100% sure if it doesn't break other auth methods, but I think it should work. I'm positive now that it shouldn't introduce regressions for other auth methods. #12322 (comment)

If you have webauthn enabled and second_factor set to on/optional, tsh uses a different prompt message:

teleport/lib/client/mfa.go

Lines 137 to 141 in 8e0421e

	if hasWebauthn {
	msg = fmt.Sprintf("Tap any %[1]ssecurity key or enter a code from a %[1]sOTP device", promptDevicePrefix, promptDevicePrefix)
	} else {
	msg = fmt.Sprintf("Enter an OTP code from a %sdevice", promptDevicePrefix)
	}

tsh does that because at this point it doesn't know whether you want to use the hardware key or provide an OTP.

Connect however asks you to select your preferred MFA method before the login attempt:

With the hardware key selected, no OTP code is sent to the tsh daemon that Connect is running, so we know that you wanted to use a hardware key and ask you for it through client.SSHAgentMFALogin.

This PR would make it so that if the OTP code is sent and second_factor is set to on/optional, we route you to client.SSHAgentLogin with the code already provided – same path as if second_factor was set to otp.

[ravicious]

I looked at the frontend code of Connect and the only time we send over the otpToken is when the user chose the "Authenticator App" option. So in all the other scenarios, the app should behave exactly like before this change. Meaning we don't introduce regressions to other auth methods.

[greedy52]

Do we have a plan to properly fix it (vs "temporarily")?

[codingllama]

SSHAgentMFALogin is CLI-centric, so it is behaving as intended. Teleterm can "borrow" it for the U2F/WebAuthn parts because it happens to use HID instead of stdin, but it's a dangerous game, as SSHAgentMFALogin wasn't created with Web interfaces in mind (and stdin reading plays a part in passwordless webauthn).

The best "fix", IMO, is to let go of tsh-specific methods and go straight to the Proxy API. That means reading the OTP from the Teleterm interface (which, iiuc, we do now) and relying on the CredentialsContainer API for WebAuthn.

[ravicious]

Thanks for the tips! At the moment we do have a lot of code copied from tsh. But knowing what to do with the auth methods will help us in deciding which parts of the code we should put inside lib and share and which parts should actually stay separate.

[ravicious]

@codingllama I looked very briefly at doing webauthn through Chromium in Electron and it might be tricky at the moment because in general webauthn in browsers requires websites to use HTTPS (see electron/electron#24573).

Also, my first reaction to the suggestion of not using tsh-specific methods was "Ahh, but tsh already does a lot of work with the keys it gets in response from that end point, it'd be a shame if we were not able to use that code 😢". What I didn't fully realize at the time is that actually going through the MFA challenge and then processing the response are two distinct steps and we actually just copy-pasted the response-processing code from tsh.

So down the road it should be possible to directly talk to the proxy API and use native browser APIs for webauthn. It's all down to what Electron will allow us to do, but I didn't check that and don't plan to for now.

I'm looking at this atm because we're talking about adding per-session MFA support to Connect which requires the user interacting with a hardware key. And I remembered that the current solution is a bit hacky.

[codingllama]

webauthn in browsers requires websites to use HTTPS

Yes, it definitely does. I wouldn't expect that to change.

about adding per-session MFA support to Connect (...)

Caveats aside, I think you should be safe to borrow "PromptMFAChallenge" (or even webauthncli.Login) for MFA challenges. Have a look at the latter, we may even be able to pull some clever stuff with a custom prompt implementation for Connect. Short-term I would suggest looking at dismembering client.SSHAgentMFALogin so you have better control over user prompts, maybe even preemptively asking for OTPs for example. Give me a nudge if you think I can help.

[codingllama]

Seems reasonable to me, with the caveats explained in the comments.

[codingllama]

SSHAgentMFALogin is CLI-centric, so it is behaving as intended. Teleterm can "borrow" it for the U2F/WebAuthn parts because it happens to use HID instead of stdin, but it's a dangerous game, as SSHAgentMFALogin wasn't created with Web interfaces in mind (and stdin reading plays a part in passwordless webauthn).

The best "fix", IMO, is to let go of tsh-specific methods and go straight to the Proxy API. That means reading the OTP from the Teleterm interface (which, iiuc, we do now) and relying on the CredentialsContainer API for WebAuthn.