Skip to content
Merged
Show file tree
Hide file tree
Changes from 6 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
22 changes: 21 additions & 1 deletion packages/core/src/awsService/ec2/sshKeyPair.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,13 @@
* Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved.
* SPDX-License-Identifier: Apache-2.0
*/
import { fs } from '../../shared'
import { fs, globals } from '../../shared'
import { ToolkitError } from '../../shared/errors'
import { tryRun } from '../../shared/utilities/pathFind'
import { Timeout } from '../../shared/utilities/timeoutUtils'
import { findAsync } from '../../shared/utilities/collectionUtils'
import { RunParameterContext } from '../../shared/utilities/processUtils'
import path from 'path'

type sshKeyType = 'rsa' | 'ed25519'

Expand All @@ -28,10 +29,25 @@ export class SshKeyPair {
}

public static async getSshKeyPair(keyPath: string, lifetime: number) {
SshKeyPair.assertValidKeypath(
keyPath,
`ec2: unable to generate key outside of global storage in path ${keyPath}`
)
await SshKeyPair.generateSshKeyPair(keyPath)
return new SshKeyPair(keyPath, lifetime)
}

private static isValidKeyPath(keyPath: string): boolean {
const relative = path.relative(globals.context.globalStorageUri.fsPath, keyPath)
return relative !== undefined && !relative.startsWith('..') && !path.isAbsolute(relative)
}

private static assertValidKeypath(keyPath: string, message: string): void | never {
if (!SshKeyPair.isValidKeyPath(keyPath)) {
throw new ToolkitError(message)
}
}

public static async generateSshKeyPair(keyPath: string): Promise<void> {
const keyGenerated = await this.tryKeyTypes(keyPath, ['ed25519', 'rsa'])
if (!keyGenerated) {
Expand Down Expand Up @@ -72,6 +88,10 @@ export class SshKeyPair {
}

public async delete(): Promise<void> {
SshKeyPair.assertValidKeypath(
this.keyPath,
`ec2: keyPath became invalid after creation, not deleting key at ${this.keyPath}`
)
await fs.delete(this.keyPath)
await fs.delete(this.publicKeyPath)

Expand Down
14 changes: 9 additions & 5 deletions packages/core/src/test/awsService/ec2/model.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,8 @@ import { ToolkitError } from '../../../shared/errors'
import { IAM } from 'aws-sdk'
import { SshKeyPair } from '../../../awsService/ec2/sshKeyPair'
import { DefaultIamClient } from '../../../shared/clients/iamClient'
import path from 'path'
import { fs, globals } from '../../../shared'

describe('Ec2ConnectClient', function () {
let client: Ec2ConnectionManager
Expand Down Expand Up @@ -132,17 +134,19 @@ describe('Ec2ConnectClient', function () {
it('calls the sdk with the proper parameters', async function () {
const sendCommandStub = sinon.stub(SsmClient.prototype, 'sendCommandAndWait')

sinon.stub(SshKeyPair, 'generateSshKeyPair')
sinon.stub(SshKeyPair.prototype, 'getPublicKey').resolves('test-key')

const testSelection = {
instanceId: 'test-id',
region: 'test-region',
}
const mockKeys = await SshKeyPair.getSshKeyPair('fakeDir', 30000)
await client.sendSshKeyToInstance(testSelection, mockKeys, 'test-user')
const temporaryDirectory = path.join(globals.context.globalStorageUri.fsPath, 'ModelTests')
Copy link
Contributor

@nkomonen-amazon nkomonen-amazon Oct 19, 2024

Choose a reason for hiding this comment

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

In tests, globalStorageUri is stubbed with a temp dir in globalSetup.test.ts, so you may be able to use the default value

But there is a chance that it is only cleared on the "after" instead of "afterEach", and may cause issues in tests:

await testUtil.deleteTestTempDirs()

So maybe you could add:

await fs.delete(globals.context.globalStorageUri.fsPath, { recursive: true, force: true })

around here:

await globals.globalState.clear()

which I think we should have been doing in the first place

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I move the temp keys into the globalStorage itself, then is there any reason I need this to clear on afterEach? I would think deleting the temp dir on after should be sufficient.

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do not clear globalStorageUri after each test we'll probably have a collision somewhere later.

Lets say you had a test that created an ssh key, then the following test assumed that since it is a new test the globalStorage will not have a key by default. The initial test state bled over in to the next and left some unexpected artifacts

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 see. Was there any reason we didn't want to do this before? Is it expensive/slow to delete all its contents for every test?

Copy link
Contributor

@justinmk3 justinmk3 Oct 21, 2024

Choose a reason for hiding this comment

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

We haven't used it much in tests before, so it was just an oversight.

await fs.mkdir(temporaryDirectory)

const keys = await SshKeyPair.getSshKeyPair(path.join(temporaryDirectory, 'key'), 30000)
await client.sendSshKeyToInstance(testSelection, keys, 'test-user')
sinon.assert.calledWith(sendCommandStub, testSelection.instanceId, 'AWS-RunShellScript')
sinon.restore()

await fs.delete(temporaryDirectory, { force: true, recursive: true })
})
})

Expand Down
13 changes: 11 additions & 2 deletions packages/core/src/test/awsService/ec2/sshKeyPair.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import * as sinon from 'sinon'
import * as path from 'path'
import * as os from 'os'
import { SshKeyPair } from '../../../awsService/ec2/sshKeyPair'
import { createTestWorkspaceFolder, installFakeClock } from '../../testUtil'
import { installFakeClock } from '../../testUtil'
import { InstalledClock } from '@sinonjs/fake-timers'
import { ChildProcess } from '../../../shared/utilities/processUtils'
import { fs, globals } from '../../../shared'
Expand All @@ -21,7 +21,10 @@ describe('SshKeyUtility', async function () {
let clock: InstalledClock

before(async function () {
temporaryDirectory = (await createTestWorkspaceFolder()).uri.fsPath
// Setup a temporary directory inside of globalStorage since keys need to be inside globalStorage
temporaryDirectory = path.join(globals.context.globalStorageUri.fsPath, 'SshKeyUtilityTests')
await fs.mkdir(temporaryDirectory)

keyPath = path.join(temporaryDirectory, 'testKeyPair')
clock = installFakeClock()
})
Expand Down Expand Up @@ -134,6 +137,12 @@ describe('SshKeyUtility', async function () {
assert(keyPair.isDeleted())
})

it('does not allow writing keys to non-global storage', async function () {
await assert.rejects(async () => await SshKeyPair.getSshKeyPair('~/.ssh/someKey', 2000))

await assert.rejects(async () => await SshKeyPair.getSshKeyPair('/a/path/that/isnt/real/key', 2000))
})

describe('isDeleted', async function () {
it('returns false if key files exist', async function () {
assert.strictEqual(await keyPair.isDeleted(), false)
Expand Down
Loading