Enable ConPTY by default in terminals on Windows (#44468) (#44743)

* Add `terminal.windowsUseConpty` config option * Calculate `windowsPty` options * Pass `useConpty` to node-pty * Pass `windowsPty` to xterm * Fix test * Replace `terminal.windowsUseConpty` with `terminal.windowsBackend`, pass boolean all the way through * Wait for pty processes to exit before closing the app * Simplify `Array.from` * `GRACEFUL_KILL_MESSAGE` -> `TERMINATE_MESSAGE` * Adjust callsites to async `dispose` * Add `winpty` to ignored words in `cspell.json` (cherry picked from commit 981aed6)
gravitational · Jul 30, 2024 · c2090c9 · c2090c9
1 parent 78389d4
commit c2090c9
Show file tree

Hide file tree

Showing 28 changed files with 311 additions and 37 deletions.
diff --git a/docs/cspell.json b/docs/cspell.json
@@ -951,6 +951,7 @@
     "winadj",
     "windowsaccountname",
     "windowsdesktop",
+    "winpty",
     "winscp",
     "winserver",
     "workgroups",

diff --git a/docs/pages/connect-your-client/teleport-connect.mdx b/docs/pages/connect-your-client/teleport-connect.mdx
@@ -419,6 +419,7 @@ Below is the list of the supported config properties.
 | `theme`                       | `system`                                                                                                             | Color theme for the app. Available modes: `light`, `dark`, `system`.   |
 | `terminal.fontFamily`         | `Menlo, Monaco, monospace` on macOS<br/>`Consolas, monospace` on Windows<br/>`'Droid Sans Mono', monospace` on Linux | Font family for the terminal.                                          |
 | `terminal.fontSize`           | 15                                                                                                                   | Font size for the terminal.                                            |
+| `terminal.windowsBackend` | `auto` | `auto` uses modern [ConPTY](https://devblogs.microsoft.com/commandline/windows-command-line-introducing-the-windows-pseudo-console-conpty/) system if available, which requires Windows 10 (19H1) or above. Set to `winpty` to use winpty even if ConPTY is available. |
 | `usageReporting.enabled`      | `false`                                                                                                              | Enables collecting anonymous usage data (see [Telemetry](#telemetry)). |
 | `keymap.tab1` - `keymap.tab9` | `Command+1` - `Command+9` on macOS <br/> `Ctrl+1` - `Ctrl+9` on Windows<br/>`Alt+1` - `Alt+9` on Linux               | Shortcut to open tab 1–9.                                              |
 | `keymap.closeTab`             | `Command+W` on macOS<br/>`Ctrl+W` on Windows/Linux                                                                   | Shortcut to close a tab.                                               |
@@ -431,6 +432,7 @@ Below is the list of the supported config properties.
 | `keymap.openProfiles`         | `Command+I` on macOS<br/>`Ctrl+I` on Windows/Linux                                                                   | Shortcut to open the profile selector.                                 |
 | `keymap.openSearchBar`        | `Command+K` on macOS<br/>`Ctrl+K` on Windows/Linux                                                                   | Shortcut to open the search bar.                                       |
 | `headless.skipConfirm` | false | Skips the confirmation prompt for Headless WebAuthn approval and instead prompts for WebAuthn immediately. |
+| `ssh.noResume`                | false                                                                                                                | Disables SSH connection resumption.                                                                        |
 
 <Admonition
   type="note"

diff --git a/web/packages/teleterm/src/mainProcess/mainProcess.ts b/web/packages/teleterm/src/mainProcess/mainProcess.ts
@@ -44,6 +44,7 @@ import {
   ChildProcessAddresses,
   MainProcessIpc,
   RendererIpc,
+  TERMINATE_MESSAGE,
 } from 'teleterm/mainProcess/types';
 import { getAssetPath } from 'teleterm/mainProcess/runtimeSettings';
 import { RootClusterUri } from 'teleterm/ui/uri';
@@ -141,7 +142,11 @@ export default class MainProcess {
       terminateWithTimeout(this.tshdProcess, 10_000, () => {
         this.gracefullyKillTshdProcess();
       }),
-      terminateWithTimeout(this.sharedProcess),
+      terminateWithTimeout(this.sharedProcess, 5_000, process =>
+        // process.kill doesn't allow running a cleanup code in the child process
+        // on Windows
+        process.send(TERMINATE_MESSAGE)
+      ),
       this.agentRunner.killAll(),
     ]);
   }

diff --git a/web/packages/teleterm/src/mainProcess/types.ts b/web/packages/teleterm/src/mainProcess/types.ts
@@ -277,3 +277,12 @@ export enum MainProcessIpc {
 export enum WindowsManagerIpc {
   SignalUserInterfaceReadiness = 'windows-manager-signal-user-interface-readiness',
 }
+
+/**
+ * A custom message to gracefully quit a process.
+ * It is sent to the child process with `process.send`.
+ *
+ * We need this because `process.kill('SIGTERM')` doesn't work on Windows,
+ * so we couldn't run any cleanup logic.
+ */
+export const TERMINATE_MESSAGE = 'TERMINATE_MESSAGE';
diff --git a/web/packages/teleterm/src/preload.ts b/web/packages/teleterm/src/preload.ts
@@ -73,7 +73,14 @@ async function getElectronGlobals(): Promise<ElectronGlobals> {
     credentials.shared,
     runtimeSettings,
     {
-      noResume: mainProcessClient.configService.get('ssh.noResume').value,
+      ssh: {
+        noResume: mainProcessClient.configService.get('ssh.noResume').value,
+      },
+      terminal: {
+        windowsBackend: mainProcessClient.configService.get(
+          'terminal.windowsBackend'
+        ).value,
+      },
     }
   );
   const {

diff --git a/web/packages/teleterm/src/services/config/appConfigSchema.ts b/web/packages/teleterm/src/services/config/appConfigSchema.ts
@@ -22,6 +22,9 @@ import { Platform } from 'teleterm/mainProcess/types';
 
 import { createKeyboardShortcutSchema } from './keyboardShortcutSchema';
 
+// When adding a new config property, add it to the docs too
+// (teleport-connect.mdx#configuration).
+
 export type AppConfigSchema = ReturnType<typeof createAppConfigSchema>;
 export type AppConfig = z.infer<AppConfigSchema>;
 
@@ -54,6 +57,12 @@ export const createAppConfigSchema = (platform: Platform) => {
       .max(256)
       .default(15)
       .describe('Font size for the terminal.'),
+    'terminal.windowsBackend': z
+      .enum(['auto', 'winpty'])
+      .default('auto')
+      .describe(
+        '`auto` uses modern ConPTY system if available, which requires Windows 10 (19H1) or above. Set to `winpty` to use winpty even if ConPTY is available.'
+      ),
     'usageReporting.enabled': z
       .boolean()
       .default(false)

diff --git a/web/packages/teleterm/src/services/pty/fixtures/mocks.ts b/web/packages/teleterm/src/services/pty/fixtures/mocks.ts
@@ -20,6 +20,7 @@ import { IPtyProcess } from 'teleterm/sharedProcess/ptyHost';
 import {
   PtyProcessCreationStatus,
   PtyServiceClient,
+  WindowsPty,
 } from 'teleterm/services/pty';
 
 export class MockPtyProcess implements IPtyProcess {
@@ -29,7 +30,7 @@ export class MockPtyProcess implements IPtyProcess {
 
   resize() {}
 
-  dispose() {}
+  async dispose() {}
 
   onData() {
     return () => {};
@@ -64,10 +65,12 @@ export class MockPtyServiceClient implements PtyServiceClient {
   createPtyProcess(): Promise<{
     process: IPtyProcess;
     creationStatus: PtyProcessCreationStatus;
+    windowsPty: WindowsPty;
   }> {
     return Promise.resolve({
       process: new MockPtyProcess(),
       creationStatus: PtyProcessCreationStatus.Ok,
+      windowsPty: undefined,
     });
   }
 }
diff --git a/web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.test.ts b/web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.test.ts
@@ -47,7 +47,7 @@ describe('getPtyProcessOptions', () => {
 
       const { env } = getPtyProcessOptions(
         makeRuntimeSettings(),
-        { noResume: false },
+        { ssh: { noResume: false }, windowsPty: { useConpty: true } },
         cmd,
         processEnv
       );
@@ -76,7 +76,7 @@ describe('getPtyProcessOptions', () => {
 
       const { env } = getPtyProcessOptions(
         makeRuntimeSettings(),
-        { noResume: false },
+        { ssh: { noResume: false }, windowsPty: { useConpty: true } },
         cmd,
         processEnv
       );
@@ -103,7 +103,7 @@ describe('getPtyProcessOptions', () => {
 
       const { args } = getPtyProcessOptions(
         makeRuntimeSettings(),
-        { noResume: true },
+        { ssh: { noResume: true }, windowsPty: { useConpty: true } },
         cmd,
         processEnv
       );

diff --git a/web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.ts b/web/packages/teleterm/src/services/pty/ptyHost/buildPtyOptions.ts
@@ -25,18 +25,24 @@ import { assertUnreachable } from 'teleterm/ui/utils';
 import {
   PtyCommand,
   PtyProcessCreationStatus,
-  SshOptions,
   TshKubeLoginCommand,
+  SshOptions,
+  WindowsPty,
 } from '../types';
 
 import {
   resolveShellEnvCached,
   ResolveShellEnvTimeoutError,
 } from './resolveShellEnv';
 
+type PtyOptions = {
+  ssh: SshOptions;
+  windowsPty: Pick<WindowsPty, 'useConpty'>;
+};
+
 export async function buildPtyOptions(
   settings: RuntimeSettings,
-  sshOptions: SshOptions,
+  options: PtyOptions,
   cmd: PtyCommand
 ): Promise<{
   processOptions: PtyProcessOptions;
@@ -68,7 +74,7 @@ export async function buildPtyOptions(
       return {
         processOptions: getPtyProcessOptions(
           settings,
-          sshOptions,
+          options,
           cmd,
           combinedEnv
         ),
@@ -79,10 +85,12 @@ export async function buildPtyOptions(
 
 export function getPtyProcessOptions(
   settings: RuntimeSettings,
-  sshOptions: SshOptions,
+  options: PtyOptions,
   cmd: PtyCommand,
   env: typeof process.env
 ): PtyProcessOptions {
+  const useConpty = options.windowsPty?.useConpty;
+
   switch (cmd.kind) {
     case 'pty.shell': {
       // Teleport Connect bundles a tsh binary, but the user might have one already on their system.
@@ -104,6 +112,7 @@ export function getPtyProcessOptions(
         cwd: cmd.cwd,
         env: { ...env, ...cmd.env },
         initMessage: cmd.initMessage,
+        useConpty,
       };
     }
 
@@ -129,6 +138,7 @@ export function getPtyProcessOptions(
         path: settings.defaultShell,
         args: isWindows ? powershellCommandArgs : bashCommandArgs,
         env: { ...env, KUBECONFIG: getKubeConfigFilePath(cmd, settings) },
+        useConpty,
       };
     }
 
@@ -140,7 +150,7 @@ export function getPtyProcessOptions(
       const args = [
         `--proxy=${cmd.rootClusterId}`,
         'ssh',
-        ...(sshOptions.noResume ? ['--no-resume'] : []),
+        ...(options.ssh.noResume ? ['--no-resume'] : []),
         '--forward-agent',
         loginHost,
       ];
@@ -149,6 +159,7 @@ export function getPtyProcessOptions(
         path: settings.tshd.binaryPath,
         args,
         env,
+        useConpty,
       };
     }
 
@@ -159,6 +170,7 @@ export function getPtyProcessOptions(
         path: cmd.path,
         args: cmd.args,
         env: { ...env, ...cmd.env },
+        useConpty,
       };
     }
 

diff --git a/web/packages/teleterm/src/services/pty/ptyHost/ptyHostClient.ts b/web/packages/teleterm/src/services/pty/ptyHost/ptyHostClient.ts
@@ -41,6 +41,7 @@ export function createPtyHostClient(
         args: ptyOptions.args,
         path: ptyOptions.path,
         env: Struct.fromJson(ptyOptions.env),
+        useConpty: ptyOptions.useConpty,
       });
 
       if (ptyOptions.cwd) {

diff --git a/web/packages/teleterm/src/services/pty/ptyHost/ptyProcess.ts b/web/packages/teleterm/src/services/pty/ptyHost/ptyProcess.ts
@@ -47,7 +47,7 @@ export function createPtyProcess(
       exchangeEventsStream.resize(columns, rows);
     },
 
-    dispose(): void {
+    async dispose(): Promise<void> {
       exchangeEventsStream.dispose();
     },
 

diff --git a/web/packages/teleterm/src/services/pty/ptyHost/windowsPty.test.ts b/web/packages/teleterm/src/services/pty/ptyHost/windowsPty.test.ts
@@ -0,0 +1,61 @@
+/**
+ * Teleport
+ * Copyright (C) 2024 Gravitational, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import { makeRuntimeSettings } from 'teleterm/mainProcess/fixtures/mocks';
+
+import { getWindowsPty } from './windowsPty';
+
+test.each([
+  {
+    name: 'uses conpty on supported Windows version',
+    platform: 'win32' as const,
+    osVersion: '10.0.22621',
+    terminalOptions: { windowsBackend: 'auto' as const },
+    expected: { useConpty: true, buildNumber: 22621 },
+  },
+  {
+    name: 'uses winpty on unsupported Windows version',
+    platform: 'win32' as const,
+    osVersion: '10.0.18308',
+    terminalOptions: { windowsBackend: 'auto' as const },
+    expected: { useConpty: false, buildNumber: 18308 },
+  },
+  {
+    name: 'uses winpty when Windows version is supported, but conpty is disabled in options',
+    platform: 'win32' as const,
+    osVersion: '10.0.22621',
+    terminalOptions: { windowsBackend: 'winpty' as const },
+    expected: { useConpty: false, buildNumber: 22621 },
+  },
+  {
+    name: 'undefined on non-Windows OS',
+    platform: 'darwin' as const,
+    osVersion: '23.5.0',
+    terminalOptions: { windowsBackend: 'auto' as const },
+    expected: undefined,
+  },
+])('$name', ({ platform, osVersion, terminalOptions, expected }) => {
+  const pty = getWindowsPty(
+    makeRuntimeSettings({
+      platform,
+      osVersion,
+    }),
+    terminalOptions
+  );
+  expect(pty).toEqual(expected);
+});
diff --git a/web/packages/teleterm/src/services/pty/ptyHost/windowsPty.ts b/web/packages/teleterm/src/services/pty/ptyHost/windowsPty.ts
@@ -0,0 +1,49 @@
+/**
+ * Teleport
+ * Copyright (C) 2024 Gravitational, Inc.
+ *
+ * This program is free software: you can redistribute it and/or modify
+ * it under the terms of the GNU Affero General Public License as published by
+ * the Free Software Foundation, either version 3 of the License, or
+ * (at your option) any later version.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU Affero General Public License for more details.
+ *
+ * You should have received a copy of the GNU Affero General Public License
+ * along with this program.  If not, see <http://www.gnu.org/licenses/>.
+ */
+
+import { RuntimeSettings } from 'teleterm/mainProcess/types';
+
+import { TerminalOptions, WindowsPty } from '../types';
+
+export const WIN_BUILD_STABLE_CONPTY = 18309;
+
+export function getWindowsPty(
+  runtimeSettings: RuntimeSettings,
+  terminalOptions: TerminalOptions
+): WindowsPty {
+  if (runtimeSettings.platform !== 'win32') {
+    return undefined;
+  }
+
+  const buildNumber = getWindowsBuildNumber(runtimeSettings.osVersion);
+  const useConpty =
+    terminalOptions.windowsBackend === 'auto' &&
+    buildNumber >= WIN_BUILD_STABLE_CONPTY;
+  return {
+    useConpty,
+    buildNumber,
+  };
+}
+
+function getWindowsBuildNumber(osVersion: string): number {
+  const parsedOsVersion = /(\d+)\.(\d+)\.(\d+)/g.exec(osVersion);
+  if (parsedOsVersion?.length === 4) {
+    return parseInt(parsedOsVersion[3]);
+  }
+  return 0;
+}