Bastion #34

rcketscientist · 2020-10-23T03:15:06Z

I want to change the lazy jumpHost to three values and just properly check them, but the basics should hold.

rcketscientist · 2020-10-27T00:38:46Z

This should be ready for review. Could likely use another cleanup pass, but the logic is stable in my limited use case at least.

joelspadin · 2020-10-27T05:41:30Z

Thanks! Will take a look when I get some time.

joelspadin

Thanks for this! It looks pretty good, but I have a few recommendations to help keep the code easy to understand.

joelspadin · 2020-11-01T16:34:12Z

ssh/src/extension.ts


-		conn.on('x11', handleX11);
+		if (preferConfig) {


Would it be possible to pull this SSH config handling out into a separate function that returns destinationOptions and jumpOptions? createForwardedDisplay is getting rather long, and this seems like it's mostly about parsing the file rather than creating the connection.

It's also currently difficult to understand the error handling behavior here since the try block is so long. Something like this would be much easier to read:

try { const parsedConfig = parseSshConfig(/* whatever args it needs */); destinationOptions = parsedConfig.destinationOptions; jumpOptions = parsedConfig.jumpOptions; } catch (err) { logger.log(`Failed to process SSH config. ...`); }

joelspadin · 2020-11-01T16:36:27Z

ssh/src/extension.ts

@@ -27,15 +31,18 @@ interface ConnectOptions {
 const BASE_PORT = 6000;

 const logger = new Logger('Remote X11 (SSH)');
-let conn: Client;
+let destination: Client;
+let jump: Client;


These should probably be typed as Client | undefined since they're not immediately initialized. (This was an error in my original code too.)

Typescript should handle that already (maybe needs strict), but fine by me

joelspadin · 2020-11-01T16:44:35Z

ssh/src/extension.ts

+				// display.
+				createForwardingShell(jump, false);
+
+				jump.forwardOut('127.0.0.1', 12345, destinationOptions.host, destinationOptions.port, (err, stream) => {


Is there any specific meaning to the port 12345 (other than the fact that it was listed in ssh2's documentation)? If this is a specific port reserved for forwarding connections, I'd like to have it defined as a constant so it's not a magic number.

If it doesn't matter what port is used here, then it would be better to pick an arbitrary, unused port instead of using a fixed number that might already be in use. Based on https://stackoverflow.com/questions/28050171/nodejs-random-free-tcp-ports, you might be able to use port 0 for that. If that doesn't work with ssh2, there are libraries like https://www.npmjs.com/package/get-port which might work, or you could just make a setting so you can change it if it conflicts with something.

Nope, copied as you mentioned...forgot to TODO that.

joelspadin · 2020-11-01T16:48:07Z

ssh/src/config.ts

+}
+
+export function getSshConfig(): string {
+	return getConfig('SSH.sshConfig', DefaultSshConfig);


If you just wrap the result of this with resolveHome (as is done in getPrivateKey), then resolveHome doesn't need to be exported, and you can simplify calls to getSshConfig.

joelspadin · 2020-11-01T16:51:55Z

ssh/src/extension.ts

+			logger.log(`Attempting to parse ${sshConfig} for ssh configuration of ${host}`);
+
+			try {
+				const data = fs.readFileSync(resolveHome(getSshConfig()))


I think this can just be simplified to

const data = fs.readFileSync(sshConfig);

Also, it probably doesn't matter because there isn't much else the code could be doing while waiting on IO here, and you can't easily use await inside a promise callback like this, but consider using fs.promises.readFile.

Pretty sure fs can throw for a number of reasons and the goal was zero side effects, so I basically wanted the logic to branch off "try to load config" and if that failed operate reliably as before.

joelspadin · 2020-11-01T17:09:21Z

ssh/src/extension.ts

+				const configProxy = hostConfig.find((line:any) => line.param === 'ProxyCommand');
+
+				destinationOptions = {
+					username: configUser ? configUser.value : options.username,


Can simplify these using optional chaining and null coalescing.

{ username: configUser?.value ?? options.username }

Mostly stuck in older versions of TS, haven't had a chance to leverage the chaining yet.

joelspadin · 2020-11-01T17:18:45Z

ssh/src/extension.ts

+
+							const hostAndPort = args[forwardArg + 1].split(':');
+							// %h is the config host, %p is the config port
+							destinationOptions.host = hostAndPort[0] === '%h' ? destinationOptions.host : hostAndPort[0];


Would be nice to include a check like

const hostAndPortStr = args[forwardArg + 1]; const hostAndPort = hostAndPortStr.split(':'); if (hostAndPort.length < 2) { throw new Error(`Invalid destination "${hostAndPortStr}". Expected a string of the form "host:port"`); }

Yah, good point. Since that is a strict argument I skipped the check, but we're the ones parsing so we'll need to be explicit.

joelspadin · 2020-11-01T17:21:39Z

ssh/src/extension.ts

+							const jumpHostBlock = config.find((line:any) => line.param === 'Host' && line.value === jumpHostArg);
+							if (jumpHostBlock) {
+								const jumpHostNameNode = jumpHostBlock.config.find((line:any) => line.param === 'HostName');
+								const jumpHost: string = jumpHostNameNode ? jumpHostNameNode.value : options.host;


Can also simplify these with optional chaining and null coalescing.

joelspadin · 2020-11-01T17:34:16Z

ssh/src/extension.ts

+	}
+}
+
+function createForwardingShell(conn: Client, shouldGetDisplay: boolean): Promise<string> {


In places that call this, it's not really obvious what the second parameter does:

createForwardingShell(conn, true);

Something like this would make it much more clear:

function createForwardingShell(conn: client, { getDisplay }: { getDisplay: boolean }) { //... } createForwardingShell(conn, { getDisplay: true });

joelspadin · 2020-11-01T17:35:26Z

ssh/src/extension.ts

 		});
 	});
 }

-function createForwardingShell(conn: Client): Promise<string> {
+function createForwardingShellNoDisplay(conn: Client): Promise<string> {


This appears to be unused now?

rcketscientist · 2020-11-02T13:59:13Z

Other than dropping the try, I think that all makes sense. It may be some time before I can gather all those. Feel free to tweak as desired if you'd like to merge in the meantime

joelspadin · 2020-11-03T00:53:17Z

Other than dropping the try, I think that all makes sense.

Did I leave in a comment about getting rid of the try? If so, ignore that. I had originally written something like that but then realized you were intentionally falling back to regular settings if parsing SSH config failed. I intended to replace that with my comment about splitting the parsing out so the try block is short and easy to follow.

rcketscientist · 2020-11-05T14:05:28Z

Ironically, we just switched to 2fa, rendering all this useless for us. I can't really justify finishing the PR now, sorry. I'll answer any questions you may have.

Anthony Mandra added 2 commits October 22, 2020 15:09

functional hard coded bastion

ba0a01b

functional ssh-config parse

adfa16c

rcketscientist mentioned this pull request Oct 23, 2020

Bastion (jump host) support? #31

Open

Anthony Mandra added 5 commits October 23, 2020 11:36

bug fixes, but remote x11 not picking up ssh now

8ada42a

can't seem to connect any longer

31e25a2

function hop and x11

d57f432

cleanup

7ca74e8

this should work, needs cleanup

ce76cf1

lose the jump display noise

f3b7cbf

joelspadin suggested changes Nov 1, 2020

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bastion #34

Bastion #34

rcketscientist commented Oct 23, 2020

rcketscientist commented Oct 27, 2020

joelspadin commented Oct 27, 2020

joelspadin left a comment

joelspadin Nov 1, 2020

joelspadin Nov 1, 2020

rcketscientist Nov 2, 2020

joelspadin Nov 1, 2020

rcketscientist Nov 2, 2020

joelspadin Nov 1, 2020

joelspadin Nov 1, 2020

rcketscientist Nov 2, 2020

joelspadin Nov 1, 2020

rcketscientist Nov 2, 2020

joelspadin Nov 1, 2020

rcketscientist Nov 2, 2020

joelspadin Nov 1, 2020

joelspadin Nov 1, 2020

joelspadin Nov 1, 2020

rcketscientist commented Nov 2, 2020

joelspadin commented Nov 3, 2020

rcketscientist commented Nov 5, 2020

Bastion #34

Are you sure you want to change the base?

Bastion #34

Conversation

rcketscientist commented Oct 23, 2020

rcketscientist commented Oct 27, 2020

joelspadin commented Oct 27, 2020

joelspadin left a 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

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

rcketscientist commented Nov 2, 2020

joelspadin commented Nov 3, 2020

rcketscientist commented Nov 5, 2020