Add passwordless logins to teleterm by kimlisa · Pull Request #14759 · gravitational/teleport

kimlisa · 2022-07-21T17:48:54Z

Description

Adds missing passwordless auth settings for teleterm
Adds passwordless login capability for teleterm (just hooking into wancli.LoginPrompt interface):
- Required use of bi-directional streaming to mimic the tsh login flow
- Required wancli.PromptTouch definitions to return an error even though wancli.DefaultPrompt implementation will never return an error. It was required to allow returning stream errors for this implementation for teleterm.
- Required pulling out existing pwdlessLogin logic into a reusable func that can be used by both tsh and teleterm. Nothing new was added other than allowing the use of CustomPrompt

TODO: I need to test this with a mac with touchID to see if i need further tweaking

codingllama

Quick pass over the non-teleterm parts. I'll come back shortly for the rest.

codingllama · 2022-07-21T18:27:56Z

+	if err := json.Unmarshal(loginRespJSON.Bytes(), loginResp); err != nil {
+		return nil, trace.Wrap(err)
+	}
+	return loginResp, nil


This block is all copy-pasta, right? Just to confirm.

yup! only difference is that i added setting a CustomPrompt if any

codingllama · 2022-07-21T19:48:56Z

 }
+
+// PwdlessLogin processes passwordless logins for this cluster.
+func (c *Cluster) PwdlessLogin(ctx context.Context, stream api.TerminalService_LoginPasswordlessServer) error {


FYI, I wouldn't advise passing gRPC streams around, but I assume you are following suite from other methods.

It tends to make testing awkward, as it ties various parts of the code to gRPC's behavior.

FYI, I wouldn't advise passing gRPC streams around, but I assume you are following suite from other methods.

It tends to make testing awkward, as it ties various parts of the code to gRPC's behavior.

What would be another way to solve this? The only thing which comes to my mind is accepting an interface with Recv/Send, would something like that work?

I tend to write simpler gRPC layers than what I'm seeing here. My first instinct would be to code the stream in the "topmost" gRPC handler and (possibly) call out streaming-unaware methods to solve its stages. This way you'll have a full gRPC test for the
"topmost" method (as we usually do) and plain Go tests for everything downstream. (Passing proto messages around is fine, protos are meant to carry data after all.)

Now, TBH, I would avoid streaming if I have a fixed number of request-response calls, like we have here - I would chain unary RPCs instead. I think streaming shines when we need an arbitrary number of server-to-client messages, but otherwise the feature isn't (wasn't?) very well supported by a number of language clients.

That's my 2c on this particular scenario, but I don't expect any changes to PR. Happy to chat about gRPC/project design/organization sometime.

codingllama

Thanks for the changes, Lisa.

Added a few minor comments, but essentially it LGTM. Just waiting to see if we'll get more code/test changes or if it's settled.

ravicious

Commented on some minor stuff but overall looks good.

ravicious · 2022-08-01T11:01:00Z

+    // PASSWORDLESS_PROMPT_CREDENTIAL is used when we require a user to select a username
+    // associated with their account.
+    PASSWORDLESS_PROMPT_CREDENTIAL = 3;


Thanks a lot for providing meaningful comments! 👍

credits to @codingllama who taught me to write better comments :)

ravicious · 2022-08-01T11:01:25Z

+    message LoginPasswordlessCredentialResponse {
+        // index is the associated number in the list of credentials that the user selected to log
+        // in as.
+        int64 index = 3;


Does this need to be 3 or could it be 1?

ack, thanks for catching this

ravicious · 2022-08-01T11:07:09Z

 }
+
+// PwdlessLogin processes passwordless logins for this cluster.
+func (c *Cluster) PwdlessLogin(ctx context.Context, stream api.TerminalService_LoginPasswordlessServer) error {


Excluding docs and so on, it seems that we generally tend to use the full passwordless form more so I'd be for using it here too.

$ rg passwordless -g '!docs' -g '!*.pb.go' -g '!*.js' -g '!*_pb.d.ts' -g '!*.md' | wc -l 445 $ rg pwdless -g '!docs' -g '!*.pb.go' -g '!*.js' -g '!*_pb.d.ts' -g '!*.md' | wc -l 72

+1 for full spelling in public APIs, well spotted.

Besides being able to use a CustomPrompt, this just mainly rearranges how things are being defined and passed around.

zmb3

bot.

github-actions · 2022-08-05T17:12:12Z

@kimlisa See the table below for backport results.

Branch	Result
branch/v10	Failed

backport #14759

kimlisa added the backport/branch/v10 label Jul 21, 2022

kimlisa requested review from codingllama and ravicious July 21, 2022 17:48

kimlisa force-pushed the lisa/tconnect-pwdless-main branch from 90d44f7 to c55947e Compare July 21, 2022 18:05

codingllama reviewed Jul 21, 2022

View reviewed changes

kimlisa mentioned this pull request Jul 21, 2022

[Connect] Refactor FormLogin and add passwordless gravitational/webapps#1019

Merged

kimlisa force-pushed the lisa/tconnect-pwdless-main branch from 4c35bbf to 1fd78eb Compare July 27, 2022 06:50

codingllama reviewed Jul 27, 2022

View reviewed changes

Comment thread lib/teleterm/api/proto/v1/auth_settings.proto

Comment thread lib/teleterm/api/proto/v1/service.proto Outdated

Comment thread lib/teleterm/api/proto/v1/service.proto Outdated

Comment thread lib/client/weblogin.go Outdated

Comment thread lib/auth/webauthncli/api.go

kimlisa force-pushed the lisa/tconnect-pwdless-main branch 2 times, most recently from 1696ec4 to 4c50bd9 Compare July 28, 2022 14:02

codingllama approved these changes Jul 28, 2022

View reviewed changes

ravicious approved these changes Aug 1, 2022

View reviewed changes

kimlisa added 8 commits August 1, 2022 09:57

define err return value for PromptTouch

c8f9a78

Define a SSHAgentPwdlessLogin func to be shared with tsh and teleterm

769787a

Besides being able to use a CustomPrompt, this just mainly rearranges how things are being defined and passed around.

Define passwordless streaming and auth settings to proto files

76bab33

Add passwordless auth settings and password login to teleterm

c0109fb

Address CR

e730e66

Update proto files from addressing CRs

f18c2d8

Address Crs 2 and update proto files

c0ddebb

Address CR: rename and fix numbered tag

e5be839

kimlisa force-pushed the lisa/tconnect-pwdless-main branch from d3219c0 to e5be839 Compare August 1, 2022 16:57

kimlisa added 5 commits August 1, 2022 11:31

Merge branch 'master' into lisa/tconnect-pwdless-main

de07c66

Merge branch 'master' into lisa/tconnect-pwdless-main

1766fd8

Merge branch 'master' into lisa/tconnect-pwdless-main

182d305

Merge branch 'master' into lisa/tconnect-pwdless-main

9c26694

Add test and update test plan

b93bbc6

codingllama reviewed Aug 3, 2022

View reviewed changes

Comment thread lib/client/weblogin_test.go

kimlisa added 2 commits August 4, 2022 10:39

Address CR: add test that custom prompts can be callable

4d2a060

Address CR: ensure custom prompt func has been called

953ccfb

codingllama approved these changes Aug 5, 2022

View reviewed changes

Merge branch 'master' into lisa/tconnect-pwdless-main

edc6e70

kimlisa enabled auto-merge (squash) August 5, 2022 14:01

Merge branch 'master' into lisa/tconnect-pwdless-main

82e1c36

zmb3 approved these changes Aug 5, 2022

View reviewed changes

Merge branch 'master' into lisa/tconnect-pwdless-main

a909b0c

kimlisa merged commit 13cc52f into master Aug 5, 2022

kimlisa mentioned this pull request Aug 5, 2022

[v10] Add passwordless login capabilities to teleterm #15265

Merged

reedloden pushed a commit that referenced this pull request Aug 10, 2022

[v10] Add passwordless login capabilities to teleterm (#15265)

0c51cf0

backport #14759

zmb3 deleted the lisa/tconnect-pwdless-main branch September 9, 2022 18:57

This was referenced Oct 4, 2022

[connect] access requests for Teleport Connect #16694

Merged

Add file transfer support to Connect #16880

Merged

Conversation

kimlisa commented Jul 21, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codingllama left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

zmb3 left a comment

Choose a reason for hiding this comment

Uh oh!

github-actions Bot commented Aug 5, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kimlisa commented Jul 21, 2022 •

edited

Loading