Spawn gateway CLI client directly by ravicious · Pull Request #26441 · gravitational/teleport

ravicious · 2023-05-17T13:19:31Z

What

This PR makes it so that when the user clicks "Run" next to a CLI command in a DocumentGateway tab, we spawn that process directly rather than spawning a shell first and then writing the CLI command to its PTY.

The number of additions is actually half as big, I had to update some proto messages.

Why

This fixes two problems:

The previous approach allowed for command injection. The database name (targetSubresourceName) field could be set to something like nonexistent-database; touch /tmp/whoops and upon clicking "Run" an arbitrary command could have been executed.
- This PR makes it so that args are passed as actual args array to the spawn function so it's not possible to inject commands.
- This also lets us remove initCommand from ShellCommand which will be done in a separate PR.
Previously, any tab opened through the Run button would be degraded to a local terminal after app restart. Now those tabs will be restored properly.

How

The What section explains the general approach. One problem to solve here was that on app restart, we cannot spawn the CLI client until we know that the gateway is ready. This is dealt with by storing targetUri and targetUser on the new doc and waiting for the gateway with these params to be created.

When it comes to connections, gateway CLI client tabs are treated as regular terminal tabs here and are not added to connections.

What isn't addressed

This PR doesn't address a situation in which the user doesn't have the CLI client but clicks "Run" anyway. Handling this is a little more complex than I assumed and will be done in a separate PR that is in the works.

I noticed that we pass both argsList and args to the PtyProcess constructor. While TypeScript allows that, it is a bit confusing when inspecting the actual values received in the constructor.

ravicious · 2023-05-17T14:01:56Z

Just adding some comments to the old code.

ravicious · 2023-05-17T14:06:50Z

Rust & Go made me write code like this. 😭 It doesn't create any intermediate data structures unlike [...this.state.gateways.entries()].find(…).

Why not

for (const gateway of this.state.gateways.values()) {

?

edit: ohhhh you were talking about the approach of the loop not the code style lol

ravicious · 2023-05-17T14:09:40Z

In a situation where DocumentGatewayCliClient times out when waiting for the gateway to be created, it updates the status to error. When the gateway ends up being created, we want to update the state back to connecting when we attempt to connect with the client.

This is what this call accomplishes.

ravicious · 2023-05-17T14:16:08Z

-    let title = `${command} · ${doc.targetUser}@${doc.targetName}`;
-
-    if (doc.targetSubresourceName) {
-      title += `/${doc.targetSubresourceName}`;


I've decided to remove targetSubresourceName from the title. Technically it was possible to set the subresource name to foo, create a CLI client tab connected to foo, change the subresource name to bar, restart the app and have the tab now connect to bar while the title said foo.

Working around this wouldn't be possible without bigger changes to the app. But even more so, in many CLI clients it's possible to change the current database from within the CLI anyway.

gzdunek · 2023-05-18T09:16:19Z

+  // target, but the user might still want to inspect the output.
+  if (gateway || hasRenderedTerminal) {
+    if (!hasRenderedTerminal) {
+      setHasRenderedTerminal(true);


I'm not sure about calling setState in render, it looks "hacky". SO says that is not a good practice https://stackoverflow.com/a/35296725.
I know we want to avoid useEffect when possible, but it seems to me that it is a completely valid use case:

useEffect(() => { if (gateway) { setHasRenderedTerminal(true); }}, [gateway]);

AFAIK calling setState with the same value doesn't cause a re-render.

I'm not sure about calling setState in render

I think this use case falls under Adjusting some state when a prop changes. gateway is not prop in the traditional sense here but it does come from outside of the component.

Ok, I didn't know it is allowed :p

gzdunek · 2023-05-18T10:08:27Z

+      </StyledText>
+    )}
+
+    <ButtonPrimary onClick={openConnection}>Open the connection</ButtonPrimary>


WDYT about disabling the button while waiting for a db connection?
Or even showing it only after timeout?

IMO the current behavior with the button always active is slightly confusing.

WDYT about disabling the button while waiting for a db connection?

What if you know that the connection won't be opened because the tab with the gateway is not present? I'd rather let the user click the button immediately. Also, clicking this button, even repeatedly, is not going to cause any side effects besides switching to the gateway tab.

All the "waiting" here is fake after all.

avatus · 2023-05-18T18:35:55Z

+          A connection to <strong>{doc.targetName}</strong> as{' '}
+          <strong>{doc.targetUser}</strong> has not been opened up within{' '}
+          {TIMEOUT_SECONDS} seconds.


I think it'd be better to have a more explicit error message here. Something like

Error connecting to {targetName} as {targetUser}: Connection timed out after {timeout_seconds} seconds.

I won't die on this hill tho. I remember talk about changing the way our error messages sound, but I don't remember if it was supposed to be more or less "machine-y".

The message you proposed is more direct and uses the active voice which I like. However, in this particular case there's no connection that times out and there's no error. As I mentioned in the previous comment, all waiting here is fake – we assume that when the DocumentGatewayCliClient is open, there's DocumentGateway open as well. When the user restarts the app, we wait for DocumentGateway to create the gateway before DocumentGatewayCliClient attempts to connect to the gateway.

What's more, it might take DocumentGateway more than 10 seconds to open the gateway, in that case DocumentGatewayCliClient is going to switched from the "timed out" state to starting a terminal session automatically.

With that in mind, I don't know how I could transform the existing message from the passive voice to the active voice.

ravicious · 2023-05-22T08:23:34Z

I'll see how much of a hassle it is to backport this to v12 and v11. The issue with the command injection is not as relevant in v11 and v12 since those versions have the command bar. But backporting this will improve the gateway CLI client experience in all three versions and reduce discrepancies between the versions.

public-teleport-github-review-bot · 2023-05-22T09:00:24Z

@ravicious See the table below for backport results.

Branch	Result
branch/v11	Failed
branch/v12	Create PR
branch/v13	Create PR

espadolini · 2023-05-22T13:51:58Z

@ravicious Could you change the number and the field name of cli_command before backporting? As it is, this is a breaking change for the teleterm API, which - while we don't document it being stable across versions, we also never actually document as not being stable.

ravicious · 2023-05-22T14:08:46Z

@espadolini Sure thing, I'll make a PR to address this. I'm waiting for #26581 before backporting anyway.

What would be the best place to document that this API is not stable? How did you notice that I made a breaking change, as in is there a check that I could avoid tripping up in the future?

espadolini · 2023-05-22T16:02:25Z

I noticed because I was running buf breaking on recent PRs and almost all failed the check because they did not yet contiain the change in this PR.

As part of #26688 I'll have buf breaking run as part of CI, so that we'll automatically reject any change in the protos that results in a breaking change in the wire format.

As far as where the indication that this API is subject to breakage should be added, maybe at the (proto) package level or maybe at the service level, I'd say.

ravicious · 2023-05-22T16:59:18Z

Addressed the breaking change in protos in #26705.

* Return os.exec.Cmd as gateway CLI command * Remove separate Props type from DocumentTerminal * Refactor Kind type exported from documentsService * Export makeRuntimeSettings from MainProcess mock * PtyProcess: Join args in logger name * ptyHostService: Pass ptyOptions explicitly instead of using spread I noticed that we pass both argsList and args to the PtyProcess constructor. While TypeScript allows that, it is a bit confusing when inspecting the actual values received in the constructor. * Add empty DocumentGatewayCliClient * Start terminal from DocumentGatewayCliClient * Add waiting state for DocumentGatewayCliClient * Remove targetSubresourceName from DocumentGatewayCliClient title

ravicious added 10 commits May 17, 2023 16:12

Return os.exec.Cmd as gateway CLI command

7e25fc1

Remove separate Props type from DocumentTerminal

96480fe

Refactor Kind type exported from documentsService

2732e60

Export makeRuntimeSettings from MainProcess mock

3cf43e9

PtyProcess: Join args in logger name

8f5f14b

ptyHostService: Pass ptyOptions explicitly instead of using spread

fe30ce9

I noticed that we pass both argsList and args to the PtyProcess constructor. While TypeScript allows that, it is a bit confusing when inspecting the actual values received in the constructor.

Add empty DocumentGatewayCliClient

d321dc4

Start terminal from DocumentGatewayCliClient

ee20692

Add waiting state for DocumentGatewayCliClient

d78d691

Remove targetSubresourceName from DocumentGatewayCliClient title

da3e408

ravicious force-pushed the ravicious/db-repl branch from 4ec7ba5 to da3e408 Compare May 17, 2023 14:13

ravicious commented May 17, 2023

View reviewed changes

ravicious marked this pull request as ready for review May 17, 2023 14:26

github-actions Bot added size/lg ui labels May 17, 2023

github-actions Bot requested review from gzdunek and ryanclark May 17, 2023 14:26

ravicious removed the request for review from ryanclark May 17, 2023 14:26

ravicious assigned avatus and unassigned avatus May 17, 2023

ravicious requested a review from avatus May 17, 2023 14:42

ryanclark approved these changes May 17, 2023

View reviewed changes

gzdunek reviewed May 18, 2023

View reviewed changes

avatus approved these changes May 18, 2023

View reviewed changes

ravicious mentioned this pull request May 19, 2023

Capture the error if the gateway CLI client executable is not found #26581

Merged

ravicious requested a review from gzdunek May 19, 2023 14:55

gzdunek approved these changes May 19, 2023

View reviewed changes

ravicious added backport/branch/v13 labels May 22, 2023

ravicious added this pull request to the merge queue May 22, 2023

github-merge-queue Bot removed this pull request from the merge queue due to failed status checks May 22, 2023

ravicious added this pull request to the merge queue May 22, 2023

Merged via the queue into master with commit 2d9a755 May 22, 2023

ravicious deleted the ravicious/db-repl branch May 22, 2023 08:58

ravicious mentioned this pull request May 22, 2023

Preserve backwards compatibility of teleport.lib.teleterm.v1.Gateway #26705

Merged

ravicious mentioned this pull request May 23, 2023

[v13] Spawn gateway CLI client directly #26751

Merged

ravicious mentioned this pull request May 23, 2023

[v12] Spawn gateway CLI client directly #26752

Merged

ravicious mentioned this pull request May 23, 2023

[v11] Spawn gateway CLI client directly #26755

Merged

This was referenced May 24, 2023

Remove initCommand from DocumentPtySession and PtyProcess #26837

Merged

Teleport Connect leaves orphaned processes #27125

Closed

ravicious mentioned this pull request Jun 2, 2023

Document lack of stability of lib/teleterm gRPC API #27275

Merged

This was referenced Mar 27, 2024

Error reported attempting to connect to MongoDB resources via Teleport Connect #39836

Closed

Use dbcmd.WithPrintFormat only for preview of teleterm gateway command #39906

Merged

Conversation

ravicious commented May 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

Why

How

What isn't addressed

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ryanclark May 17, 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!

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!

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!

Choose a reason for hiding this comment

Uh oh!

ravicious commented May 22, 2023

Uh oh!

Uh oh!

public-teleport-github-review-bot Bot commented May 22, 2023

Uh oh!

espadolini commented May 22, 2023

Uh oh!

ravicious commented May 22, 2023

Uh oh!

espadolini commented May 22, 2023

Uh oh!

ravicious commented May 22, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

ravicious commented May 17, 2023 •

edited

Loading

ryanclark May 17, 2023 •

edited

Loading