gravitational / teleport

The easiest, and most secure way to access and protect all of your infrastructure.
https://goteleport.com
GNU Affero General Public License v3.0
17.69k stars 1.77k forks source link

Use correct `PATH` when resolving db CLI clients in Connect #28525

Open ravicious opened 1 year ago

ravicious commented 1 year ago

Problem

While investigating #28115, we found an issue with how paths to database CLI clients are resolved.

To create a database gateway, Connect calls tsh daemon. tsh daemon in turn uses dbcmd which calls os/exec.LookPath to resolve database CLI clients.

However, tsh daemon in spawned with env vars inherited from the Electron app. On macOS, if an app is launched from the desktop (desktop icon or Spotlight), the path for it is set to /usr/bin:/bin:/usr/sbin:/sbin.

All of this means that when evaluating dbcmd to get a command for a database gateway, tsh daemon will pretty much never see the db CLI clients that the user has installed on their system. Those clients are typically added to PATH that's defined in the startup files of whatever shell is used by the user.

This means that if the app is launched from Spotlight (or otherwise not through the terminal, which is how typically people launch apps on macOS), every single is*Available method from dbcmd.CLICommandBuilder returns false. At the moment this has probably only two consequences:

Solution

We had this exact problem before with terminal sessions. We fixed it by doing the same thing as VSCode – spawning a shell and reading env vars from it.

Now, calling resolveShellEnv takes a while and how long it takes is entirely dependent on what the user has in their shell startup files. As such, we probably don't want to put it in the critical path leading to the start of the app.

All we really need is for tsh daemon to have a correct PATH set when evaluating CLI clients. We could check if os.Setenv has the desired effect. If so, the best place to update PATH would be just before creating the first gateway during the lifespan of the daemon.

The problem with doing this on the JS side is that we have to either:

I wonder if it would be possible to completely reuse the logic from resolveShellEnv.ts in Go, complete with launching a Node process. Then we could pick only PATH out of the returned result.

ravicious commented 7 months ago

This doesn't seem to be a problem on Windows, where the app always has the "correct" environment vars set by the OS, no matter how it's launched.