Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

SSH: Support executing commands and interactive shell with port forwarding #50

Merged
merged 6 commits into from
Mar 2, 2024

Conversation

GGonryun
Copy link
Contributor

@GGonryun GGonryun commented Mar 1, 2024

This PR enables

  • interactive shells & port forwarding: p0 ssh <instance> -L 56789:80
  • executing commands & port forwarding: p0 ssh <instance> sleep 10 -L 56789:80
  • performing a port forward without allowing commands: p0 ssh <instance> -NL 56789:80

In order to support this behavior a new package ps-tree was added to ensure that grandchild processes spawned by the AWS CLI are cleaned up.

Solves:
https://linear.app/p0-security/issue/ENG-1651/support-p0-ssh-n
https://linear.app/p0-security/issue/ENG-1653/p0-cli-ssh-support-executing-commands-with-port-forwarding

@GGonryun GGonryun changed the title WIP: alternative solution to SSH live sessions while port forwarding SSH: Support executing commands and interactive shell with port forwarding Mar 1, 2024
@GGonryun GGonryun requested review from gergas3 and nbrahms March 1, 2024 21:46
@GGonryun GGonryun self-assigned this Mar 1, 2024
@GGonryun GGonryun marked this pull request as ready for review March 1, 2024 21:47
Copy link
Contributor

@nbrahms nbrahms left a comment

Choose a reason for hiding this comment

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

Can you remove the package-lock.json please?

];
};

const createSsmCommands = (args: Omit<SsmArgs, "requestId">): string[][] => {
Copy link
Contributor

Choose a reason for hiding this comment

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

More stable I think as

Suggested change
const createSsmCommands = (args: Omit<SsmArgs, "requestId">): string[][] => {
const createSsmCommands = (args: Omit<SsmArgs, "requestId">): {
interactiveShellCommand: string[];
portForwardingCommand: string[];
} => {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed to {shellCommand, subCommand} because the shell command sometimes wont be interactive. It can be the portforwarding command for example p0 ssh <instance> -NL <ports>

...credential,
},
stdio,
}) as ChildProcessByStdio<
Copy link
Contributor

Choose a reason for hiding this comment

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

The type casting here is a little sus. It would be nicer to use typescript overloading. You could get away with just the two variants that you actually used:

const spawnChildProcess = (credential: AwsCredentials, command: string[], stdio: [StdioNull, StdioPipe, StdioPipe]): ChildProcessByStdio<null, Readable, Readable>;
const spawnChildProcess = (credential: AwsCredentials, command: string[], stdio: [StdioNull, StdioNull, StdioNull]): ChildProcessByStdio<null, null, null>;
const spanwChildProcess = (credential: AwsCredentials, command: string[], stdio: [StdioNull, StdioNull | StdioPipe, StdioNull | StdioPipe]): ChildProcessByStdio<null, Readable, Readable> | ChildProcessByStdio<null, null, null> => {
  ...
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! This is much neater.

src/plugins/aws/ssm/index.ts Outdated Show resolved Hide resolved
Copy link
Contributor Author

@GGonryun GGonryun left a comment

Choose a reason for hiding this comment

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

Can you remove the package-lock.json please?

I removed it and I reran yarn add ps-tree because I had previously installed it with npm.

...credential,
},
stdio,
}) as ChildProcessByStdio<
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TIL! This is much neater.

];
};

const createSsmCommands = (args: Omit<SsmArgs, "requestId">): string[][] => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I renamed to {shellCommand, subCommand} because the shell command sometimes wont be interactive. It can be the portforwarding command for example p0 ssh <instance> -NL <ports>

Copy link
Contributor

@nbrahms nbrahms left a comment

Choose a reason for hiding this comment

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

Lookin' good 👌

@GGonryun GGonryun merged commit 2d16b75 into main Mar 2, 2024
3 checks passed
@GGonryun GGonryun deleted the wip/live-session branch March 2, 2024 00:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants