fix(toolkit): attach lifetime to generated ssh keys. #5578
Conversation
|
This pull request implements a feature or fix, so it must include a changelog entry. See CONTRIBUTING.md#changelog for instructions. |
Great idea. Would also be a good time to add a EC2-specific section here: aws-toolkit-vscode/docs/arch_features.md Line 14 in 791ae55 That way, we have a high-level description of the steps that happen. |
| @@ -5,19 +5,32 @@ | |||
| import * as fs from 'fs-extra' | |||
There was a problem hiding this comment.
If it's not much trouble, it would be helpful to remove this and use our fs.ts module instead. We want to eliminate 'fs-extra'
There was a problem hiding this comment.
I couldn't figure out how to set file permission (fs.chmod equivalent) through fs.ts. While ssh-keygen seems to do this for me locally, it fails in CI without this. Should I keep fs-extra for this until a chmod equivalent exists in fs.ts?
There was a problem hiding this comment.
how to set file permission (
fs.chmodequivalent) throughfs.ts
would you mind adding a stub for chmod in fs.ts? it can just do nothing for if (isWeb()). and for non-web, it can use node's chmod (from 'fs', instead of 'fs-extra)
this could be a followup PR. for now in this PR, if it's easy, try using 'fs' instead of 'fs-extra'.
|
/retryBuilds |
|
|
||
| public async delete(): Promise<void> { | ||
| await fs.delete(this.publicKeyPath) | ||
| await fs.delete(this.keyPath) |
There was a problem hiding this comment.
might be a good idea to delete the private key first, in case deleting the public key fails (which is less important)
Co-authored-by: Justin M. Keyes <[email protected]>
| } | ||
|
|
||
| public isDeleted(): boolean { | ||
| return this.deleted |
There was a problem hiding this comment.
can this extra state be avoided? usually better to check "reality", i.e. check the filesystem. after all, we're already storing the filepath.
intermediate state can drift, and leads to "coherency" bugs. even very simple state.
There was a problem hiding this comment.
So in that case, replace the isDeleted() method with one that checks the actual filesystem?
Problem
For connecting VSCode to EC2 instance, we generate an ssh key pair on disk. This results in writing the private ssh key to VSCode global storage, allowing the key to be potentially reused by other users on the same machine.
Solution
Attach a lifetime to any key pair generated such that they wipe from disk after X seconds. Value is currently set is 30 seconds to allow connection to reliably establish. Also, change file permissions to read/write owner only and change behavior to overwrite existing keys.
Unable to test file permissions, due to VSCode file system unable to provide us with enough detail. Only provides whether it is readonly or not (somewhat unreliably: microsoft/vscode-discussions#673). VSCode file system is what is used in
fs.tsimplementation.This is in-line with how ec2 instance connect works: https://github.com/aws/aws-ec2-instance-connect-cli/blob/master/ec2instanceconnectcli/EC2InstanceConnectKey.py
License: I confirm that my contribution is made under the terms of the Apache 2.0 license.