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

fix(ec2): give warning when OpenSSH client is outdated. #6018

Closed
wants to merge 12 commits into from
33 changes: 33 additions & 0 deletions packages/core/src/shared/extensions/ssh.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import * as vscode from 'vscode'
import * as path from 'path'
import * as nls from 'vscode-nls'
import * as semver from 'semver'
import fs from '../fs/fs'
import { getLogger } from '../logger'
import { ChildProcess } from '../utilities/processUtils'
Expand Down Expand Up @@ -148,3 +149,35 @@ export async function startVscodeRemote(

await new ProcessClass(vscPath, ['--folder-uri', workspaceUri]).run()
}
/**
* Uses provided sshPath to get version of OpenSSH. If sshPath is not OpenSSH, returns undefined.
* @param sshPath
* @returns
*/
export async function getSshVersion(sshPath: string): Promise<string | undefined> {
const result = await new ChildProcess(sshPath, ['-V'], { collect: true }).run()

return parseSshVersion(result.stdout === '' ? result.stderr : result.stdout)
}

export async function ensureSshVersionGte(sshPath: string, minVersion: string): Promise<void | never> {
const sshVersion = await getSshVersion(sshPath)
if (sshVersion && semver.lt(sshVersion, minVersion)) {
const msg = `SSH version ${sshVersion} is not supported, please upgrade OpenSSH to ${minVersion} or higher`
getLogger().error(msg)
throw new Error(msg)
}
}

function parseSshVersion(output: string): string | undefined {
const match = output.match(/OpenSSH_(\d+)\.(\d+)/)
Copy link
Contributor

@justinmk3 justinmk3 Nov 14, 2024

Choose a reason for hiding this comment

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

Instead of all this, could we we just triy to connect to the remote and surface any error messages (plus ssh version) on failure? We already do that for the "terminal" feature, right? So for vscode, we could do that as a pre-step, to really check that things are working.

An extra benefit is that this gives a much better experience if there is an ssh issue, compared to checking vscode-remote's ssh window.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The error is coming from the connect script, i.e. the child process that launches the new instance of VSCode so I am not sure how to surface it. Especially since the child process is successfully opening VSCode, but then the newly opened VSCode instance throws an error. We would have to route that error back to this instance of VSCode somehow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, when we invoke code with a remote URL, vscode invokes ssh implicitly for us. But what I'm thinking is that we can manually invoke ssh ourselves, before invoking code, and just run a trivial command on the remote like sh -c 'true'. That gives a true picture of whether ssh can work.

If that fails, we can report/log the ssh output and that gives us valuable troubleshooting info. Without needing to check for specific ssh versions. This is also future-proof for any changes we make to our .ssh/config.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ahhh, I see. That's a good idea. I'll work on that in a fresh PR and link it here.

if (!match) {
getLogger().warn(`ssh: failed to parse SSH version: ${output}`)
return undefined
}

const major = parseInt(match[1], 10)
const minor = parseInt(match[2], 10)

return `${major}.${minor}.0`
}
11 changes: 11 additions & 0 deletions packages/core/src/shared/remoteSession.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,7 @@ import { findSshPath, getVscodeCliPath } from './utilities/pathFind'
import { IamClient } from './clients/iamClient'
import { IAM } from 'aws-sdk'
import { getIdeProperties } from './extensionUtilities'
import { ensureSshVersionGte } from './extensions/ssh'

const policyAttachDelay = 5000

Expand Down Expand Up @@ -99,6 +100,16 @@ export async function ensureDependencies(): Promise<Result<DependencyPaths, Canc
return await handleMissingTool(tools)
}

if (tools.isOk()) {
const sshPath = tools.unwrap().ssh
// Pre 7.6 does not support accept-new as StrictHostKeyChecking
try {
await ensureSshVersionGte(sshPath, '7.6.0')
} catch (e) {
return Result.err(e as Error)
}
}

return tools
}

Expand Down
40 changes: 39 additions & 1 deletion packages/core/src/test/shared/extensions/ssh.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,12 @@
*/
import * as assert from 'assert'
import { ChildProcess } from '../../../shared/utilities/processUtils'
import { startSshAgent } from '../../../shared/extensions/ssh'
import { ensureSshVersionGte, getSshVersion, startSshAgent } from '../../../shared/extensions/ssh'
import { createExecutableFile, createTestWorkspaceFolder } from '../../testUtil'
import { isWin } from '../../../shared/vscode/env'
import path from 'path'
import { fs } from '../../../shared'
import { WorkspaceFolder } from 'vscode'

describe('SSH Agent', function () {
it('can start the agent on windows', async function () {
Expand All @@ -29,3 +34,36 @@ describe('SSH Agent', function () {
assert.strictEqual(await getStatus(), 'Running')
})
})

describe('SSH Versioning', function () {
let tempDir: WorkspaceFolder

before(async function () {
tempDir = await createTestWorkspaceFolder()
})

after(async function () {
await fs.delete(tempDir.uri.fsPath, { force: true, recursive: true })
})

it('gets ssh version from path', async function () {
const testSshVersion = async (sshName: string, sshOutput: string, expectedVersion: string) => {
const sshPath = path.join(tempDir.uri.fsPath, `${sshName}${isWin() ? '.cmd' : ''}`)
await createExecutableFile(sshPath, `echo "${sshOutput}"`)
const version = await getSshVersion(sshPath)
assert.strictEqual(version, expectedVersion)
}

await testSshVersion('ssh', 'OpenSSH_9.7p1, LibreSSL 3.3.6', '9.7.0')
await testSshVersion('ssh2', 'OpenSSH_6.6.1p1, OpenSSL 1.0.1e-fips 11 Feb 2013', '6.6.0')
await testSshVersion('ssh3', 'OpenSSH_7.4p1, OpenSSL 1.0.1e-fips 11 Feb 2013', '7.4.0')
})

it('asserts version is above threshold', async function () {
const sshPath = path.join(tempDir.uri.fsPath, `ssh3${isWin() ? '.cmd' : ''}`)
await createExecutableFile(sshPath, `echo "'OpenSSH_9.7p1, LibreSSL 3.3.6'"`)
await assert.rejects(async () => await ensureSshVersionGte(sshPath, '9.10'))
await assert.doesNotReject(async () => await ensureSshVersionGte(sshPath, '9.7.0'))
await assert.doesNotReject(async () => await ensureSshVersionGte(sshPath, '9.2.0'))
})
})
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
{
"type": "Bug Fix",
"description": "ec2/codecatalyst remote connect: warn user when OpenSSH client is outdated"
}
Loading