Teleport Connect headless watcher by Joerger · Pull Request #28844 · gravitational/teleport

Joerger · 2023-07-08T03:24:09Z

This PR adds a headless authentication watcher to Teleport Connect to catch any pending headless authentications for current login sessions, as well as an endpoint for the Electron App to approve/deny a headless authentication.

In this PR, we simply send the notification to the Electron App but don't handle it.

In a follow up PR, the Electron App will be updated to display two modals, similar to that of the web UI to:

approve or deny the headless login
prompt for MFA (when approving)

Note: the MFA prompt will be similar to that of the login flow. The Electron App will display an MFA prompt modal, and then call rpc UpdateHeadlessAuthenticationState(state=approve). Once the rpc succeeds, it will display a success modal or close the prompt.

Updates #27137

ravicious

I need to take another look at it tomorrow, I didn't have time to look at cluster_headless.go. But the part in the daemon looks solid.

gzdunek · 2023-07-12T11:06:42Z

+// startHeadlessWatcher starts a background process to watch for pending headless
+// authentications for this cluster.
+func (s *Service) startHeadlessWatcher(cluster *clusters.Cluster) error {
+	if _, ok := s.headlessWatcherClosers[cluster.URI.String()]; ok {


I think it won't work correctly in the following scenario:

Log in to a cluster.

Wait for a cert to expire.

Try to log in again.

Step 3 will fail with the error saying that the watcher already exists, won't it? I guess we should just close the old watcher before starting the new one.

Good point. I think catching AlreadyExists in StartHeadlessWatcher and simply logging the fact should do the job. Its plural equivalent, StartHeadlessWatchers, should definitely error if a watcher already exists.

I'll update it so that when the watcher goroutine loop ends, it is deleted from the map. This way, if the certs expire, the watcher should clean itself up.

So as I understand, the current implementation assumes that the watcher goroutine loop ends after the cert expires, right? How does this happen? It's not clear for me just from looking at the code. 🤔

You're right...I forget that a client can still function with expired certs, just can't redial. Updated to have StartHeadlessWatcher stop and replace the previous watcher if there is one.

And what is going to happen when the headless auth request is created and my certs are expired? Can a watcher on an expired client receive it? (I don't have much knowledge about watchers)

The watcher will still work and send the headless authn to the Electron App. If the user hasn't logged in however, the Electron app will fail to approve/deny the headless authn, trigger relogin, and retry. At this point, the headless authn can be handled and the old headless watcher will be replaced. So in a roundabout way, the Headless watcher system should handle relogin surprisingly well.

Just to clarify, after the cert expires, the watcher still works and receives updates from the cluster, is that correct?

Right, until the client is closed or receives an error over the connection (Auth down, backend down, etc.).

ravicious · 2023-07-12T13:24:28Z

+// startHeadlessWatcher starts a background process to watch for pending headless
+// authentications for this cluster.
+func (s *Service) startHeadlessWatcher(cluster *clusters.Cluster) error {
+	if _, ok := s.headlessWatcherClosers[cluster.URI.String()]; ok {


Good point. I think catching AlreadyExists in StartHeadlessWatcher and simply logging the fact should do the job. Its plural equivalent, StartHeadlessWatchers, should definitely error if a watcher already exists.

ravicious · 2023-07-13T14:42:02Z

+// startHeadlessWatcher starts a background process to watch for pending headless
+// authentications for this cluster.
+func (s *Service) startHeadlessWatcher(cluster *clusters.Cluster) error {
+	if _, ok := s.headlessWatcherClosers[cluster.URI.String()]; ok {


So as I understand, the current implementation assumes that the watcher goroutine loop ends after the cert expires, right? How does this happen? It's not clear for me just from looking at the code. 🤔

* Add headless watcher to tshd daemon service. * Add SendPendingHeadlessAuthentication rpc to tshd events service. * Add UpdateHeadlessAuthenticationState rpc to the daemon service.

ravicious

Looks good though obv I didn't test it, I'm leaving that for the second part which implements the UI for it.

ravicious · 2023-07-18T13:55:08Z

+	// Stop and restart the watcher twice to simulate logout + login + relogin. Ensure the watcher catches events.
+
+	err = daemonService.StopHeadlessWatcher(cluster.URI.String())
+	require.NoError(t, err)
+	err = daemonService.StartHeadlessWatcher(cluster.URI.String())
+	require.NoError(t, err)
+	err = daemonService.StartHeadlessWatcher(cluster.URI.String())
+	require.NoError(t, err)


It'd be better to use actual login/logout handlers, but the last time I tried to do that I realized there's no way to get a password for a user out of those test helpers.

gzdunek · 2023-07-18T17:30:12Z

+// startHeadlessWatcher starts a background process to watch for pending headless
+// authentications for this cluster.
+func (s *Service) startHeadlessWatcher(cluster *clusters.Cluster) error {
+	if _, ok := s.headlessWatcherClosers[cluster.URI.String()]; ok {


And what is going to happen when the headless auth request is created and my certs are expired? Can a watcher on an expired client receive it? (I don't have much knowledge about watchers)

public-teleport-github-review-bot · 2023-07-19T03:15:44Z

@Joerger See the table below for backport results.

Branch	Result
branch/v12	Failed
branch/v13	Failed

ravicious · 2023-07-19T16:33:30Z

+	if err := s.StopHeadlessWatcher(uri); err != nil {
+		return trace.Wrap(err)
+	}


ClusterLogout needs to ignore NotFound from StopHeadlessWatcher.

ClusterLogout can be called on a cluster with expired certs which doesn't have a watcher active.

Will fix this in the other PR, thanks

* Implement headless watcher backend for Teleport Connect. * Add headless watcher to tshd daemon service. * Add SendPendingHeadlessAuthentication rpc to tshd events service. * Add UpdateHeadlessAuthenticationState rpc to the daemon service. * Address comments.

* * Enable headless authentication event watch. (#28234) * Add WatchPendingHeadlessAuthentications rpc. * Fix headless authentication matching logic for watcher (#28843) * Fix headless authentication matching logic for watcher and add test. * Move hasWatchPermissionForKind to a separate function. * Clean up hasWatchPermissionForKind. * Cleanup test code with suggestions from review. * Refactor Gateway Cert Reissuer and tshd events client (#28782) * - Move tshd events client into the daemon service. - Replace gatway cert reissuer with a more reusable retryWithRelogin method. * Resolve comments. * Teleport Connect headless watcher (#28844) * Implement headless watcher backend for Teleport Connect. * Add headless watcher to tshd daemon service. * Add SendPendingHeadlessAuthentication rpc to tshd events service. * Add UpdateHeadlessAuthenticationState rpc to the daemon service. * Address comments. * Tune Headless Watcher retry logic in Teleport Connect (#29410) * Reduce headless watcher max backoff period to 90s; Propogate watcher error properly; Don't retry on not implemented error. * Stop watcher if it wasn't stopped already. * Implement headless watcher approval logic in the Electron App. (#29097) * Fix uncaught merge conflict. * Fix call count race condition; Fix grpc server stop race condition; Make timeout less aggressive. (#29880)

Joerger added backport/branch/v12 labels Jul 8, 2023

Joerger requested a review from ravicious July 8, 2023 03:24

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from f8223aa to 6d4fc09 Compare July 10, 2023 20:22

Joerger changed the base branch from master to joerger/refactor-db-cert-reissuer July 10, 2023 20:28

Joerger marked this pull request as ready for review July 11, 2023 01:31

github-actions Bot requested review from kimlisa and ryanclark July 11, 2023 01:32

github-actions Bot added size/lg ui labels Jul 11, 2023

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 2b6aaff to 3213af8 Compare July 11, 2023 01:34

Joerger force-pushed the joerger/refactor-db-cert-reissuer branch from 96bbc52 to 6b709aa Compare July 11, 2023 01:35

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 3213af8 to 7877dff Compare July 11, 2023 01:36

ravicious requested review from gzdunek and removed request for kimlisa and ryanclark July 11, 2023 13:59

ravicious reviewed Jul 11, 2023

View reviewed changes

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 4a08360 to b6b639b Compare July 11, 2023 20:11

Joerger force-pushed the joerger/refactor-db-cert-reissuer branch from 260d440 to 95f1532 Compare July 11, 2023 20:22

Joerger requested a review from ravicious July 11, 2023 20:23

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from b6b639b to 1c5ffe2 Compare July 11, 2023 20:34

Joerger mentioned this pull request Jul 11, 2023

[v13] Add Headless Polling to Teleport Connect #28975

Merged

Base automatically changed from joerger/refactor-db-cert-reissuer to master July 11, 2023 21:12

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch 2 times, most recently from 44082a6 to 2d2abe1 Compare July 12, 2023 00:33

gzdunek reviewed Jul 12, 2023

View reviewed changes

ravicious reviewed Jul 12, 2023

View reviewed changes

Comment thread lib/teleterm/daemon/daemon_headless_watcher.go Outdated

Comment thread lib/teleterm/daemon/daemon_headless_watcher.go Outdated

ravicious reviewed Jul 13, 2023

View reviewed changes

Joerger changed the title ~~Implement Teleport Connect headless watcher~~ Teleport Connect headless watcher Jul 14, 2023

Joerger mentioned this pull request Jul 14, 2023

Teleport Connect headless approval #29097

Merged

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 5b896d5 to 18b1d52 Compare July 14, 2023 01:34

ravicious reviewed Jul 14, 2023

View reviewed changes

Comment thread lib/teleterm/clusters/cluster_headless.go Outdated

Implement headless watcher backend for Teleport Connect.

6df43c7

* Add headless watcher to tshd daemon service. * Add SendPendingHeadlessAuthentication rpc to tshd events service. * Add UpdateHeadlessAuthenticationState rpc to the daemon service.

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 06ae6c7 to 6df43c7 Compare July 17, 2023 19:06

Joerger requested review from gzdunek and ravicious July 17, 2023 19:08

ravicious approved these changes Jul 18, 2023

View reviewed changes

greedy52 approved these changes Jul 18, 2023

View reviewed changes

Comment thread integration/teleterm_test.go

Comment thread integration/teleterm_test.go

Comment thread integration/teleterm_test.go Outdated

Comment thread lib/teleterm/daemon/daemon_headless_watcher.go

gzdunek approved these changes Jul 18, 2023

View reviewed changes

Address comments.

6b44714

Joerger force-pushed the joerger/teleport-connect-headless-watcher branch from 9ff95c8 to 6b44714 Compare July 19, 2023 02:39

Joerger enabled auto-merge July 19, 2023 02:39

Joerger added this pull request to the merge queue Jul 19, 2023

Merged via the queue into master with commit 2099ccc Jul 19, 2023

Joerger deleted the joerger/teleport-connect-headless-watcher branch July 19, 2023 03:14

ravicious reviewed Jul 19, 2023

View reviewed changes

ravicious mentioned this pull request Aug 28, 2023

Update headless modal to show both Reject and Cancel #31069

Merged

Conversation

Joerger commented Jul 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravicious left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gzdunek Jul 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Joerger Jul 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gzdunek Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Joerger Jul 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ravicious left a comment

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!

gzdunek Jul 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

public-teleport-github-review-bot Bot commented Jul 19, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Joerger commented Jul 8, 2023 •

edited

Loading

gzdunek Jul 12, 2023 •

edited

Loading

Joerger Jul 13, 2023 •

edited

Loading

gzdunek Jul 18, 2023 •

edited

Loading

Joerger Jul 19, 2023 •

edited

Loading

gzdunek Jul 18, 2023 •

edited

Loading