Skip to content

Teleport Connect headless approval - Skip Confirmation#29875

Merged
Joerger merged 4 commits intomasterfrom
joerger/headless-approval-teleport-connect-skip-confirm
Aug 14, 2023
Merged

Teleport Connect headless approval - Skip Confirmation#29875
Joerger merged 4 commits intomasterfrom
joerger/headless-approval-teleport-connect-skip-confirm

Conversation

@Joerger
Copy link
Copy Markdown
Contributor

@Joerger Joerger commented Aug 1, 2023

Updates Teleport Connect headless approval flow to skip the confirmation step (Approve/Cancel) and go straight to prompting for MFA in the background when the headless.skipConfirm config option is set to true.

Docs will be added in a follow up PR.

@Joerger
Copy link
Copy Markdown
Contributor Author

Joerger commented Aug 1, 2023

@gzdunek @ravicious Any idea why I am getting process is not defined errors? Shouldn't process.env be accessible globally?

@Joerger Joerger force-pushed the joerger/headless-approval-teleport-connect-skip-confirm branch from 21e0415 to ffd0692 Compare August 1, 2023 20:05
@Joerger Joerger force-pushed the joerger/headless-approval-teleport-connect-skip-confirm branch from ffd0692 to c15597c Compare August 1, 2023 20:30
@gzdunek
Copy link
Copy Markdown
Contributor

gzdunek commented Aug 2, 2023

Shouldn't process.env be accessible globally?

No, it is a Node.js thing, and it is not passed to the renderer (or, in other words, the browser window).
There are ways to pass a value from the main process to the renderer, for example via our RuntimeSettings, but I'm wondering if it is a best idea to configure the app by the env var. IMO it is rather unusual for desktop apps, and I think it is better to add a config entry for this.
You can take a look at Configuration and appConfigSchema.ts to see how we configure other things.

@ravicious ravicious added size/sm and removed size/lg labels Aug 2, 2023
@Joerger Joerger marked this pull request as draft August 3, 2023 17:34
@Joerger Joerger force-pushed the joerger/headless-approval-teleport-connect-skip-confirm branch from b32bd00 to b9cd730 Compare August 9, 2023 20:56
Copy link
Copy Markdown
Contributor Author

@Joerger Joerger Aug 9, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ravicious @gzdunek For some reason, this OnApprove call results in an infinite loop, sending countless headless approval requests to the tshd daemon and overloading the yubikey. I have no idea why this would be an infinite loop while the on button press below works perfectly fine. Any ideas?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because the body of a component function, HeadlessPrompt, is executed on each render and a component gets rendered each time you update it's state. So you end up with an infinite loop.

Try this patch:

Patch
diff --git a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
index 08da0c1aa6..fc73d9b1c3 100644
--- a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
+++ b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import React, { useRef } from 'react';
+import React, { useRef, useEffect } from 'react';
 
 import { useAsync } from 'shared/hooks/useAsync';
 
@@ -69,6 +69,12 @@ export function HeadlessAuthentication(props: HeadlessAuthenticationProps) {
     }
   }
 
+  useEffect(() => {
+    if (props.skipConfirm && updateHeadlessStateAttempt.status === '') {
+      handleHeadlessApprove();
+    }
+  }, []);
+
   return (
     <HeadlessPrompt
       cluster={cluster}
diff --git a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
index f1da2f1bd8..b0294cb162 100644
--- a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
+++ b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
@@ -52,13 +52,9 @@ export function HeadlessPrompt({
   updateHeadlessStateAttempt,
   onCancel,
 }: HeadlessPromptProps) {
-  const [waitForMfa, setWaitForMfa] = useState(false);
-
-  // skip to MFA confirmation step.
-  if (skipConfirm) {
-    setWaitForMfa(true);
-    onApprove();
-  }
+  // skipConfirm automatically attempts to approve a headless auth attempt,
+  // so let's show waitForMfa from the very beginning in that case.
+  const [waitForMfa, setWaitForMfa] = useState(skipConfirm);
 
   return (
     <DialogConfirmation

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I suspected it was something like that but had no idea how to fix it, thanks!

Copy link
Copy Markdown
Member

@ravicious ravicious left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should be good to go after implementing the changes I mentioned.

Comment thread web/packages/teleterm/src/services/config/appConfigSchema.ts Outdated
Comment thread web/packages/teleterm/src/services/config/appConfigSchema.ts Outdated
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's because the body of a component function, HeadlessPrompt, is executed on each render and a component gets rendered each time you update it's state. So you end up with an infinite loop.

Try this patch:

Patch
diff --git a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
index 08da0c1aa6..fc73d9b1c3 100644
--- a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
+++ b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessAuthentication.tsx
@@ -14,7 +14,7 @@
  * limitations under the License.
  */
 
-import React, { useRef } from 'react';
+import React, { useRef, useEffect } from 'react';
 
 import { useAsync } from 'shared/hooks/useAsync';
 
@@ -69,6 +69,12 @@ export function HeadlessAuthentication(props: HeadlessAuthenticationProps) {
     }
   }
 
+  useEffect(() => {
+    if (props.skipConfirm && updateHeadlessStateAttempt.status === '') {
+      handleHeadlessApprove();
+    }
+  }, []);
+
   return (
     <HeadlessPrompt
       cluster={cluster}
diff --git a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
index f1da2f1bd8..b0294cb162 100644
--- a/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
+++ b/web/packages/teleterm/src/ui/HeadlessAuthn/HeadlessPrompt/HeadlessPrompt.tsx
@@ -52,13 +52,9 @@ export function HeadlessPrompt({
   updateHeadlessStateAttempt,
   onCancel,
 }: HeadlessPromptProps) {
-  const [waitForMfa, setWaitForMfa] = useState(false);
-
-  // skip to MFA confirmation step.
-  if (skipConfirm) {
-    setWaitForMfa(true);
-    onApprove();
-  }
+  // skipConfirm automatically attempts to approve a headless auth attempt,
+  // so let's show waitForMfa from the very beginning in that case.
+  const [waitForMfa, setWaitForMfa] = useState(skipConfirm);
 
   return (
     <DialogConfirmation

@Joerger Joerger force-pushed the joerger/headless-approval-teleport-connect-skip-confirm branch from 885be29 to c64fdd1 Compare August 10, 2023 18:51
@Joerger Joerger marked this pull request as ready for review August 10, 2023 18:52
@github-actions github-actions Bot requested review from rudream and ryanclark August 10, 2023 18:52
Copy link
Copy Markdown
Contributor

@gzdunek gzdunek left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested, works fine.

@Joerger Joerger added this pull request to the merge queue Aug 14, 2023
Merged via the queue into master with commit d7ed907 Aug 14, 2023
@Joerger Joerger deleted the joerger/headless-approval-teleport-connect-skip-confirm branch August 14, 2023 19:49
@public-teleport-github-review-bot
Copy link
Copy Markdown

@Joerger See the table below for backport results.

Branch Result
branch/v13 Failed

Joerger added a commit that referenced this pull request Aug 15, 2023
* Support TELEPORT_HEADLESS_SKIP_CONFIRM env flag in teleport connect.

* Replace reject button with cancel button that persists through approval state. This makes it possible to reject a headless authentication when it skips the initial confirmation step.

* Add headlessSkipConfirm config option.

* Apply changes from CR.
github-merge-queue Bot pushed a commit that referenced this pull request Aug 16, 2023
* Support TELEPORT_HEADLESS_SKIP_CONFIRM env flag in teleport connect.

* Replace reject button with cancel button that persists through approval state. This makes it possible to reject a headless authentication when it skips the initial confirmation step.

* Add headlessSkipConfirm config option.

* Apply changes from CR.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants