[Browser MFA] Add CompleteBrowserMFAChallenge gRPC

[danielashare]

This PR adds the CompleteBrowserMFAChallenge gRPC endpoint. The RFD for this addition can be found here.

These changes address this part of the flow:

sequenceDiagram
    proxy->>auth: rpc CompleteBrowserMFAChallenge
    auth->>auth: Encrypt WebAuthn response<br/>with secret_key
    auth-->>proxy: Return http://127.0.0.1:port/callback?response={encrypted_webauthn}
    proxy-->>browser: HTTP 200 with redirect URL

Loading

I would like to draw particular attention to login.go. In order to validate the WebAuthn response from the browser without consuming it, I had to split the validation logic out of login.go's finish function in to its own function. I want to avoid consuming it because in the full Browser MFA flow, the WebAuthn response is returned to tsh where it will exchange the response for certificates etc. I wonder if it is worth validating the MFA response from the browser before sending it back to tsh where it will then send the MFA response back to the server where it will have to be validated again anyway?
Decided in comments that there isn't any point double validating the response, instead response is validated once by tsh.

Manual tests:

• Login with OTP
• Login with WebAuthn
• Login with TouchID
• Login with YubiKey
• Passwordless login with TouchID
• Passwordless login with YubiKey
• Passwordless login with WebAuthn

[codingllama]

Reviewed a good chunk of it, but not all.

As stated in the proto PR, I think the RPC could be in the specialized MFAService instead.

This could also, potentially, be split into a few distinct smaller PRs: lib/webauthn changes, browser MFA encrypt/decrypt, and the RPC itself. Once we add tests in various layers it'll likely grow.

[danielashare]

Apologies @codingllama, your comments have lost their context since I split out the webauthn changes

[codingllama]

Apologies codingllama, your comments have lost their context since I split out the webauthn changes

No worries, give me a ping when this is good for another pass.

[danielashare]

No worries, give me a ping when this is good for another pass.

Cheers, @codingllama, I think this is ready for another pass.
I've removed the webauthn validation changes from this branch and moved them to their own branch (as you've seen).

I shouldn't have pointed the browser-mfa-webauthn-validation branch at master because this branch, which relies on those changes, also relies on the proto changes. So, I've pointed this branch at the proto branch and once that goes in, I can point this one back at browser-mfa-webauthn-validation.

[Joerger]

Partial review here.

I wonder if it is worth validating the MFA response from the browser before sending it back to tsh where it will then send the MFA response back to the server where it will have to be validated again anyway?

Agreed, I don't think it is necessary to validate it ahead of time. Perhaps it would be better to remove the validation and name it CompleteBrowserMFAChallenge instead. It doesn't hurt to validate it upfront, but I don't remember a technical/security requirement for it.

[zmb3]

Would something like CLILoginResponse be a better name for this?

[zmb3]

Internal packages (ie those that are under an internal/ directory) are great, but I don't love naming the package itself internal.

How about lib/auth/internal/browsermfa? Something that still describes what the package contains.

[zmb3]

Is there any particular reason to make this a type alias? Why not use authclient.SSHLoginResponse directly?

[danielashare]

There's a conversation somewhere in this PR (that I can't seem to find now), where Alan found it confusing that Browser MFA returns an SSHLoginResponse. So the type alias is to make the code easier to understand that we're returning this data to the redirector

[zmb3]

Suggested change

	assertError func(t *testing.T, err error)
	assertError require.ErrorAssertionFunc

Consider using the existing library type instead of defining your own.

[backport-bot-workflows]

@danielashare See the table below for backport results.

Branch	Result
branch/v18	Failed