[Connect] Refactor FormLogin and add passwordless

[kimlisa]

Description

• Add passwordless capabilities
• Sneaked in some teleport LoginForm refactoring
• Update teleterm's LoginForm to match teleports: basically copy and paste and then tweaking to fit teleterm. I tried initially to see if i can share some of the logic between teleport and teleterm login form and didn't think it was worth it.
• Addressed the auto focusing issue from a previous PR: the LoginForm can be 1 or 2 views depending on the setting. There is primary, and then there could be a secondary (other sign-in options). Whatever is the first item (for each view) it will get auto focused

Testing

I need help testing this on a mac, I have yet to see what it would look like with touch ID. Following the code my theory is that when user clicks passwordless button, it will get disabled and the system takes over. I wanted to make a build for someone to download and test, but I didn't know how to do that with teleterm...

Otherwise, the manual steps are:

1. Build tsh with passwordless capability, requires this PR: Add passwordless logins to teleterm teleport#14759

$ brew install libfido2
$ go build -tags=libfido2 ./tool/tsh  # located teleport/tsh

# may need this environment variable
$ export PKG_CONFIG_PATH=/usr/local/opt/openssl@1.1/lib/pkgconfig

2. Start Connect: run the following command and afterwards the app will popup after the last command:

$ git clone https://github.com/gravitational/webapps.git
$ cd webapps
$ yarn install
$ git checkout lisa/tconnect-passwordless-main
$ yarn build-term
## TELETERM_TSH_PATH is the environment variable that points to local tsh binary
$ TELETERM_TSH_PATH=$PWD/../teleport/tsh yarn start-term

3. Start Teleport with the following config (not all fields are necessary but i put it there if you wanted to play with all the settings):

  authentication:
    type: local    # local | github | saml | oidc, this will set primary to local or sso
    second_factor: optional
    local_auth: true
    require_session_mfa: no
    passwordless: true            # enables/disables passwordless
    connector_name: passwordless  # sets passwordless as the primary login
    webauthn:
      rp_id: proxy.0.0.0.0.nip.io

proxy_service:
  enabled: yes
  public_addr: ['proxy.0.0.0.0.nip.io:3080']

Demo

for the design i tried to mimic the google prompts:

cc @codingllama

[codingllama]

Hey folks, we need a signed/notarized Connect with all the right toggles for touch ID to work - see https://github.com/gravitational/webapps.e/issues/325. I can hack it together locally using our dev account if I take some time, but that won't work for the actual prod release.

Happy to test on macOS for you.

[ravicious]

@codingllama I'll reach out to you to set up that session about the provisioning profile. Once that's done it'll be fairly easy to make a dev build through drone that will have Touch ID support enabled.

[codingllama]

Ok, I just took this and gravitational/teleport#14759 for a spin using a locally-signed tsh. It works just fine, great job @kimlisa!

I tested the following:

• Touch ID passwordless authn (key registered via tsh mfa add, picker and non-picker)
• Touch ID session MFA (because why not?)
• Touch ID prompt cancel
• PIN passwordless authn (no picker)
• Bio passwordless authn (yes picker)

I sent Lisa a bunch of videos and screenshots for the error messages.

[gzdunek]

I will try to test it tomorrow :)

[ravicious]

I'm working on building Connect with Touch ID support here: #1033.

[kimlisa]

gosh i was trying to test it with all my new changes... but i keep running into different errors below. I don't think it's the code changes (crossing fingers) but more that several things changed for me: i accidentally nuked my repo's while upgrading my go and i nuked my existing teleport data

• ERROR: failed to authenticate with proxy proxy.0.0.0.0.nip.io:3023
• SSH cert not available
• UNAVAILABLE: No connection established

was wondering if any of these looked familiar, i'm going to try again tomorrow (i honestly think my setup just got really messed up after nuking things)

[gzdunek]

ERROR: failed to authenticate with proxy proxy.0.0.0.0.nip.io:3023
SSH cert not available
UNAVAILABLE: No connection established

It seems to be related to my changes adding Windows support, please pull the latest changes in webapps and in teleport and rebuild tsh.

[kimlisa]

ERROR: failed to authenticate with proxy proxy.0.0.0.0.nip.io:3023
SSH cert not available
UNAVAILABLE: No connection established

It seems to be related to my changes adding Windows support, please pull the latest changes in webapps and in teleport and rebuild tsh.

So after hours of banging my head... it works on both my linux and mac now (did not test the latest with touch ID though). I also found that there is a bug in current master. You have to set cache to false for the time being, the bug came from here which is currently being fixed: gravitational/teleport#14698.

teleport:
  cache:
    enabled: false

also, would it be possible to merge this in for 10.1 without touch ID? which is sometime tomorrow? the backend is ready to go.

[ravicious]

I didn't manage to look at the React components and I'm yet to actually run this version of the app, I'll try to do so on Monday.

Overall I thought the implementation of bidirectional streaming here would be much more complex but I like how self-contained it is.

[ravicious]

I think this function flows quite nicely (given that the reader has an understanding of how the whole flow is supposed to look like).

However, I think we should move all this logic outside of tshd.createClient. I don't think that's necessary to do before this PR is merged but could you maybe prepare another one which addreses this?

---

The way I think about createClient is that it provides a thin wrapper around the autogenerated gRPC code which is a giant pile of unidiomatic JavaScript. tshd.createClient let's us have a client that behaves more like how a regular JS object or a class would behave.

A while ago, Grzegorz had to add bidirectional streaming for PTY sessions and I quite like the solution we've arrived at.

The biggest difference between your bidirectional stream and the one for PTY is that yours is pretty self-contained and doesn't need to hide the horrors of gRPC from other JS objects. But I still think it's worthwhile to look at how we solved that problem there.

Here's the definition of that bidirectional stream:

webapps/packages/teleterm/src/sharedProcess/api/proto/ptyHostService.proto

Line 7 in b7eaae2

rpc ExchangeEvents(stream PtyClientEvent) returns (stream PtyServerEvent) {}

Then we have a ptyHostClient which is very similar to the client we're dealing with in this file. It has a method called exchangeEvents which corresponds to the RPC name defined in the proto file.

webapps/packages/teleterm/src/services/pty/ptyHost/ptyHostClient.ts

Lines 55 to 60 in b7eaae2

	exchangeEvents(ptyId) {
	const metadata = new Metadata();
	metadata.set('ptyId', ptyId);
	const stream = client.exchangeEvents(metadata);
	return new PtyEventsStreamHandler(stream);
	},

However, notice that it's main responsibility is to create the stream and then it passes the stream along to another object which actually implements the whole logic.

webapps/packages/teleterm/src/services/pty/ptyHost/ptyEventsStreamHandler.ts

Lines 11 to 14 in b7eaae2

	export class PtyEventsStreamHandler {
	constructor(
	private readonly stream: ClientDuplexStream<PtyClientEvent, PtyServerEvent>
	) {}

---

I think something similar could work here. The tshd client could have a method which merely returns the stream. Then in appContext.ts we could initialize a new service, PasswordlessLoginService or whatever, which would receive the tshd client in an initializer. Then when someone tries to use passwordless login, useClusterLogin instead of calling a method on ClustersService would call that service. Then this service would call the tshd client to create the stream and from there all that logic would be contained in that service.

How does that sound?

[ravicious]

Ah, it's not quite what I meant. A type like this would still allow some incorrect values to pass through as it says that LoginParamsWithKind is any LoginParams combined with any PrimaryAuthType.

Take a look at this patch:

Patch

diff --git a/packages/teleterm/src/services/tshd/types.ts b/packages/teleterm/src/services/tshd/types.ts
index 124d4ecb..bc151c9a 100644
--- a/packages/teleterm/src/services/tshd/types.ts
+++ b/packages/teleterm/src/services/tshd/types.ts
@@ -96,28 +96,24 @@ export type TshAbortSignal = {
   removeEventListener(cb: (...args: any[]) => void): void;
 };
 
-export type LoginLocalParams = {
+interface LoginParamsBase {
   clusterUri: string;
+}
+
+export interface LoginLocalParams extends LoginParamsBase {
   username: string;
   password: string;
   token?: string;
-};
+}
 
-export type LoginSsoParams = {
-  clusterUri: string;
+export interface LoginSsoParams extends LoginParamsBase {
   providerType: string;
   providerName: string;
-};
+}
 
-export type LoginPasswordlessParams = {
-  clusterUri: string;
+export interface LoginPasswordlessParams extends LoginParamsBase {
   onPromptCallback(res: WebauthnLoginPrompt): void;
-};
-
-export type LoginParams =
-  | LoginLocalParams
-  | LoginSsoParams
-  | LoginPasswordlessParams;
+}
 
 export type CreateGatewayParams = {
   targetUri: string;
diff --git a/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/useClusterLogin.ts b/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/useClusterLogin.ts
index 03a523c3..4ad48a80 100644
--- a/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/useClusterLogin.ts
+++ b/packages/teleterm/src/ui/ClusterConnect/ClusterLogin/useClusterLogin.ts
@@ -45,27 +45,25 @@ export default function useClusterLogin(props: Props) {
   });
 
   const [loginAttempt, login, setAttempt] = useAsync(
-    (params: LoginParamsWithKind) => {
+    (params: types.LoginParams) => {
       refAbortCtrl.current = clustersService.client.createAbortController();
+
       switch (params.kind) {
         case 'local':
           return clustersService.loginLocal(
-            params as types.LoginLocalParams,
+            params,
             refAbortCtrl.current.signal
           );
         case 'passwordless':
           return clustersService.loginPasswordless(
-            params as types.LoginPasswordlessParams,
+            params,
             refAbortCtrl.current.signal
           );
         case 'sso':
-          return clustersService.loginSso(
-            params as types.LoginSsoParams,
-            refAbortCtrl.current.signal
-          );
+          return clustersService.loginSso(params, refAbortCtrl.current.signal);
         default:
           throw new Error(
-            `loginAttempt: login params kind ${params.kind} not implemented`
+            `loginAttempt: login params kind not implemented: ${params}`
           );
       }
     }
@@ -201,7 +199,3 @@ export type WebauthnLogin = {
   loginUsernames?: string[];
   onUserResponse?(val: number | string): void;
 };
-
-type LoginParamsWithKind = types.LoginParams & {
-  kind: PrimaryAuthType;
-};
diff --git a/packages/teleterm/src/ui/services/clusters/clustersService.test.ts b/packages/teleterm/src/ui/services/clusters/clustersService.test.ts
index f99271c0..ac9e7a05 100644
--- a/packages/teleterm/src/ui/services/clusters/clustersService.test.ts
+++ b/packages/teleterm/src/ui/services/clusters/clustersService.test.ts
@@ -182,6 +182,7 @@ test('login into cluster and sync resources', async () => {
   const client = getClientMocks();
   const service = createService(client, new NotificationsServiceMock());
   const loginParams = {
+    kind: 'local' as const,
     clusterUri,
     username: 'admin',
     password: 'admin',
diff --git a/packages/teleterm/src/ui/services/clusters/types.ts b/packages/teleterm/src/ui/services/clusters/types.ts
index ed3f4897..130557b0 100644
--- a/packages/teleterm/src/ui/services/clusters/types.ts
+++ b/packages/teleterm/src/ui/services/clusters/types.ts
@@ -37,13 +37,18 @@ export type AuthType = shared.AuthType;
 
 export type AuthProvider = tsh.AuthProvider;
 
-export type LoginParams = tsh.LoginParams;
+export type LoginLocalParams = { kind: 'local' } & tsh.LoginLocalParams;
 
-export type LoginLocalParams = tsh.LoginLocalParams;
+export type LoginPasswordlessParams = {
+  kind: 'passwordless';
+} & tsh.LoginPasswordlessParams;
 
-export type LoginPasswordlessParams = tsh.LoginPasswordlessParams;
+export type LoginSsoParams = { kind: 'sso' } & tsh.LoginSsoParams;
 
-export type LoginSsoParams = tsh.LoginSsoParams;
+export type LoginParams =
+  | LoginLocalParams
+  | LoginPasswordlessParams
+  | LoginSsoParams;
 
 export type Application = tsh.Application;
 

This way it's impossible to call useClusterLogin.login with {kind: 'local', providerType: 'foo', providerName: 'bar'}.

But I see why it might have seemed like I meant doing it the way you did it. When not calling useClusterLogin.login but rather one of the underlying methods, such as ClustersService.loginLocal, you need to pass an additional field which seems unnecessary (as evidenced by a one line update to a test in that patch). This is just an unfortunate effect of how TypeScript support for sum types looks like.

It works better in situations where we have just a single data structure that's always passed around, like documents in Connect.

[kimlisa]

thank you for the clear explanation!

[ravicious]

I tested this with my Yubikey and everything seems to work just fine and I'm able to navigate through the modal using only my keyboard (well, and the hardware key).

Before we merge this, I'd like to go through the test plan steps related to authn to make sure we didn't break any other login method, unless someone already checked that.

@gzdunek did you perhaps already do that? I remember you were asking me a question related to that part of the test plan but I suspect this might have been related to the Windows version?

[kimlisa]

Before we merge this, I'd like to go through the test plan steps related to authn to make sure we didn't break any other login method

i tested the below, but for sso connectors only with local github (i don't think it's necessary to test the others as it's the same pathway, but if we really want to double check does anyone know of a cluster with saml and oidc set up? mine got nuked i think...)

• Auth methods
  • Verify that the app supports clusters using different auth settings
    (auth_service.authentication in the cluster config):
    • type: local, second_factor: "off"
    • type: local, second_factor: "otp"
    • type: local, second_factor: "webauthn"
    • type: local, second_factor: "optional", log in without MFA
    • type: local, second_factor: "optional", log in with OTP
    • type: local, second_factor: "optional", log in with hardware key
    • type: local, second_factor: "on", log in with OTP
    • type: local, second_factor: "on", log in with hardware key
    • Authentication connectors:
      • For those you might want to use clusters that are deployed on the web, specified in parens.
        Or set up the connectors on a local enterprise cluster following the guide from our wiki.
      • GitHub (asteroid)
        • local login on a GitHub-enabled cluster
      • SAML (platform cluster)
      • OIDC (e-demo)

also just as an fyi, i started running into various issues again and I think it happens when I nuke my /var/lib/teleport or this /private/var/lib/teleport. I had to also nuke this to resolve my issue: sudo rm -rf /Users/lisakim/Library/Application\ Support/Electron/tsh/

[gzdunek]

Before we merge this, I'd like to go through the test plan steps related to authn to make sure we didn't break any other login method, unless someone already checked that.

@gzdunek did you perhaps already do that? I remember you were asking me a question related to that part of the test plan but I suspect this might have been related to the Windows version?

Yes, I tested auth methods only for Windows version (without the changes from this PR).

[gzdunek]

I like it much more with the components extracted to their own files!

does anyone know of a cluster with saml and oidc set up?

SAML: https://platform.teleport.sh/
OIDC: https://enterprise.teleportdemo.com/ (@ravicious would you like to invite Lisa?)

And what about this comment? #1019 (comment)
Are you going to do any changes related to it?

[ravicious]

I'll check SAML and OIDC.

[ravicious]

SAML and OIDC work. 👍 Thanks for testing this with other login methods.

[ravicious]

But i was wondering if we needed to create each util in its own file or can it be under a util.ts file? I created the latter, but if we want individual file, then i can change it f202805

It's not so much that we need to do it one way or the other but I see we already have individual files is ui/utils so I suppose it'd be best to follow that?

Almost all of those functions in ui/utils could be moved to a more specific directory but that's a topic for another discussion.

[gzdunek]

I agree with @ravicious that it's better to keep the function in a separate file.

Please remember to reexport it in index.ts, so it can be imported like this:

import { assertUnreachable } from 'teleterm/ui/utils';