Resolve shell env #718

gzdunek · 2022-04-04T20:13:53Z

The implementation is mostly based on vsocde solution (I've put link in the code).

I'm not particularly happy with the error handling. Reading env is not critical to the terminal work, so we should just show the notification and continue without env. Unfortunately, there is no way to trigger a notification from the main process and the only place to do so is the useDocumentTerminal. I think we should somehow allow showing notifications from the main process.

Closes https://github.com/gravitational/webapps.e/issues/194

gzdunek · 2022-04-04T20:14:52Z

packages/teleterm/src/services/pty/resolveShellEnv.ts

+
+  return new Promise<typeof process.env>((resolve, reject) => {
+    const command = `'${process.execPath}' -p '"${mark}" + JSON.stringify(process.env) + "${mark}"'`;
+    const shellArgs = shell === '/bin/tcsh' ? ['-ic'] : ['-ilc'];


Does it have to be interactive? 🤔

Apparently if you don't specify -i, then bash for example won't read your ~/.bashrc.

https://unix.stackexchange.com/questions/277312/is-the-shell-created-by-bash-i-c-command-interactive

Matter of fact, we should perhaps link to this stackexchange post in case anyone has the same question in the future. ;)

Thanks, done :)

ravicious · 2022-04-05T16:33:40Z

packages/teleterm/src/services/pty/resolveShellEnv.ts

+
+  return new Promise<typeof process.env>((resolve, reject) => {
+    const command = `'${process.execPath}' -p '"${mark}" + JSON.stringify(process.env) + "${mark}"'`;
+    const shellArgs = shell === '/bin/tcsh' ? ['-ic'] : ['-ilc'];


Apparently if you don't specify -i, then bash for example won't read your ~/.bashrc.

https://unix.stackexchange.com/questions/277312/is-the-shell-created-by-bash-i-c-command-interactive

ravicious · 2022-04-05T16:34:47Z

packages/teleterm/src/services/pty/resolveShellEnv.ts

+ SOFTWARE.
+ */
+
+//Based on https://github.com/microsoft/vscode/blob/main/src/vs/platform/shell/node/shellEnv.ts


Suggested change

//Based on https://github.com/microsoft/vscode/blob/main/src/vs/platform/shell/node/shellEnv.ts

// Based on https://github.com/microsoft/vscode/blob/1.66.0/src/vs/platform/shell/node/shellEnv.ts

ravicious · 2022-04-05T16:47:13Z

packages/teleterm/src/services/pty/resolveShellEnv.ts

+  const env = {
+    ...process.env,
+    ELECTRON_RUN_AS_NODE: '1',
+    ELECTRON_NO_ATTACH_CONSOLE: '1',
+  };


Don't we need to do the same thing that vscode does?

AFAIK ELECTRON_RUN_AS_NODE needs to be added to child env because otherwise the flag -p on line 86 would have no effect. But I don't think we should always pass these two to the shells that we open. As far as I understand, vscode returns them from this function only if they were initially in process.env.

Good point, done

ravicious · 2022-04-05T17:08:59Z

packages/teleterm/src/services/pty/ptyService.ts

  const env = {
+    ...process.env,
    TELEPORT_HOME: settings.tshd.homeDir,
  };

+  const localShellEnv = {
+    ...env,
+    ...(await resolveShellEnvCached(settings.defaultShell)),
+  };


I'm not sure if this should be this way but I'm running out of time for today, I'll check it tomorrow morning + I have a solution for setting the correct cwd instead of opening the shell in /.

Okay, so I think the priority should be this:

TELEPORT_HOME

resolveShellEnvCached

process.env

What was clear to you but took a second for me is that obviously we want resolveShellEnvCached to override whatever path is set in process.env, because that's the problem we're solving here – launching the app from outside of the terminal doesn't set path correctly.

Now, I wonder what we should do about TELEPORT_HOME. The current code is going to override it with whatever comes from user's env. Regular Teleport users probably won't have this env var set anyway, but powerusers might in which case they might not expect Teleterm to use it and it'll mess with their tsh directory. OTOH if always override TELEPORT_HOME with settings.tshd.homeDir, users won't be able to easily change that. But for the preview release that should be okay? If someone really needs that they can probably symlink ~/Library/Application\ Support/teleterm/tsh to whatever they want. 🤔

Now that I think about it, we probably don't even need to include stuff from process.env when launching a shell. resolveShellEnvCached already gives us all the env vars we need. That being said, it's hard for me to say if this is going to have any unforeseen consequences, so I cannot really recommend that we do it.

I don't have any strong opinion on envs priority, but it seems ok to override TELEPORT_HOME with settings.tshd.homeDir.
Regarding process.env from what I see all of its properties are overridden by the resolved shell, but I'm not sure if we should remove it, I think I'd leave it as it is.

ravicious · 2022-04-06T08:43:23Z

The way to set correct cwd is this:

diff --git a/packages/teleterm/src/services/pty/ptyProcess.ts b/packages/teleterm/src/services/pty/ptyProcess.ts
index 43384017..a58b0eca 100644
--- a/packages/teleterm/src/services/pty/ptyProcess.ts
+++ b/packages/teleterm/src/services/pty/ptyProcess.ts
@@ -43,7 +43,9 @@ class PtyProcess extends EventEmitter {
       cols,
       rows,
       name: 'xterm-color',
-      cwd: this.options.cwd || process.cwd(),
+      // HOME should be always defined. But just in case it isn't let's use the cwd from process.
+      // https://unix.stackexchange.com/questions/123858
+      cwd: this.options.cwd || process.env['HOME'] || process.cwd(),
       env: {
         ...process.env,
         ...this.options.env,

ravicious · 2022-04-06T09:18:13Z

It might be a good idea to cut a new demo build after merging this, I imagine this is a huge issue if you don't know that starting the app from the terminal fixes this.

gzdunek · 2022-04-06T16:01:19Z

@ravicious what about using our resolved env here?
cwd: this.options.cwd || this.options.env['HOME'] || process.cwd()

ravicious

I think there's just one small change we should introduce and then it's ready to go.

what about using our resolved env here?
cwd: this.options.cwd || this.options.env['HOME'] || process.cwd()

Good point, idk how I missed that!

ravicious · 2022-04-07T08:42:37Z

packages/teleterm/src/services/pty/ptyService.ts

  const env = {
+    ...process.env,
    TELEPORT_HOME: settings.tshd.homeDir,
  };


This is left here so that we can pass "regular" env to pty.tsh-login, right? Is there any specific reason we need to do that? For pty.tsh-login, we're running tsh ssh underneath, so in case it interacts with any other tool, I suspect the user would like it to have the correct env.

So unless I'm missing something with regards to pty.tsh-login, I think we should always pass the resolved shell env.

I was wondering if we need to pass that resolved env for a remote session, but you're right, it is better to always have correct env. Fix pushed.

@smallinsky

* Limit navigation capabilities to reduce attack surface At the moment we don't create new windows nor navigate away from the rendered app, so we can just block everything. * Update to electron@13.6.9 (#703) * Use x64 arch when building & packaging Teleterm Our build system doesn't support arm64 for Mac releases yet (see issue gravitational/teleport#4226 for more information). Because of that, for the preview release we're likely going to have only the x64 version of Teleterm. This means that the shipped version of tsh should also be the x64 version. I tried to change electron-builder's config to use x64 for macOS, but the config options don't seem to work. I tried `mac.defaultArch` as well as changing `mac.target` in various ways but `electron-builder install-app-deps` just doesn't pick up those options. Both were set through `packages/teleterm/package.json` * Add `Notifications` component and service * Show errors in `ClusterResources`' tables using standard `Danger` labels * Use `Notifications` error in `syncRootCluster()` and `removeGateway()` * Do not block app rendering when initializing function fails * Fix accessing `serversSyncStatus` Map in `clustersService` * Revert "Use x64 arch when building & packaging Teleterm" This reverts commit 276e9a9. Turns out that for development, we need to use arm64 version of native deps. The build server is going to use x64 anyway, as per the reverted commit, but when making manual demo builds, we'll have to remember to use x64 for Teleterm and tsh. * Submit modals' forms on `Enter` press * Remove global `keyDown` handler from `KeyboardArrowsNavigation` as it blocked submitting forms * Use teleterm/logger in runtimeSettings (#716) The one from shared/libs/logger calls `window`, which doesn't exist in the context of Electron main process. * Improve Teleterm README (#719) * Mention that `yarn build-term` needs to be run first before attempting to run the app in dev mode (already had a couple of people who had problems with setting up the app because they didn't run this first). * Mention the assumption about both repos living in the same folder. * Move the architecture diagram to the end of the file. Most people reading the README are not doing it for the diagram, but build instructions. * Explain when gRPC files need to be recompiled. * Prevent crash when network or cluster is offline (#712) * Simplify the db connection tab (#720) As described in gravitational/webapps.e#177, we want to replace the db tab with just two sections: * "Connect with CLI" which will show the command to use in terminal * "Connect with GUI" which links to our documentation After gravitational/teleport#11720 gets merged, the "Connect with CLI" section will be massively simplified: it'll be basically just something like: psql postgres://localhost:12345 Moreover, @smallinsky suggested that tsh should be responsible for creating those CLI connection commands. We should also do this in the future as it'll let us support new protocols as soon as they land in tsh, without us having to touch Teleterm codebase, for the most part. * Resolve shell env (#718) Co-authored-by: Grzegorz Zdunek <gzdunek@users.noreply.github.com> Co-authored-by: Grzegorz Zdunek <grzegorz.zdunek@goteleport.com>

gzdunek commented Apr 4, 2022

View reviewed changes

gzdunek requested a review from ravicious April 4, 2022 20:15

ravicious reviewed Apr 5, 2022

View reviewed changes

gzdunek force-pushed the gzdunek/resolve-shell-env branch from a97a3bf to ec50797 Compare April 6, 2022 16:00

ravicious reviewed Apr 7, 2022

View reviewed changes

Resolve shell env

8feb1ea

gzdunek force-pushed the gzdunek/resolve-shell-env branch from ec50797 to 8feb1ea Compare April 7, 2022 08:59

ravicious approved these changes Apr 7, 2022

View reviewed changes

gzdunek merged commit f20ced2 into master Apr 7, 2022

gzdunek deleted the gzdunek/resolve-shell-env branch April 7, 2022 10:04

ravicious pushed a commit that referenced this pull request Apr 26, 2022

Resolve shell env (#718)

f7f4fb6

ravicious mentioned this pull request Apr 26, 2022

[v9] Backport Teleport Connect (3 of 5) #764

Merged

ravicious pushed a commit that referenced this pull request Apr 27, 2022

Resolve shell env (#718)

2b9b309

This was referenced Jun 29, 2023

Update Teleport Connect MongoDB database connection function to use mongosh instead of deprecated mongo gravitational/teleport#28115

Closed

Use correct PATH when resolving db CLI clients in Connect gravitational/teleport#28525

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve shell env #718

Resolve shell env #718

gzdunek commented Apr 4, 2022

gzdunek Apr 4, 2022 •

edited

Loading

ravicious Apr 5, 2022

ravicious Apr 6, 2022

gzdunek Apr 6, 2022

ravicious Apr 5, 2022

ravicious Apr 5, 2022

gzdunek Apr 6, 2022

ravicious Apr 5, 2022

gzdunek Apr 6, 2022

ravicious Apr 5, 2022

ravicious Apr 6, 2022

gzdunek Apr 6, 2022

ravicious commented Apr 6, 2022

ravicious commented Apr 6, 2022

gzdunek commented Apr 6, 2022 •

edited

Loading

ravicious left a comment

ravicious Apr 7, 2022

gzdunek Apr 7, 2022

	//Based on https://github.com/microsoft/vscode/blob/main/src/vs/platform/shell/node/shellEnv.ts
	// Based on https://github.com/microsoft/vscode/blob/1.66.0/src/vs/platform/shell/node/shellEnv.ts

Resolve shell env #718

Resolve shell env #718

Conversation

gzdunek commented Apr 4, 2022

gzdunek Apr 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravicious commented Apr 6, 2022

ravicious commented Apr 6, 2022

gzdunek commented Apr 6, 2022 • edited Loading

ravicious left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gzdunek Apr 4, 2022 •

edited

Loading

gzdunek commented Apr 6, 2022 •

edited

Loading