Skip to content

fix: SSHConfig: atomically write ssh config #511

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.

Sign up for GitHub

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

Merged
merged 5 commits into from
May 23, 2025
Merged

fix: SSHConfig: atomically write ssh config #511

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
f9943cb
fix: SSHConfig: atomically write ssh config
johnstcn May 20, 2025
1925567
preserve existing mode on config file if present
johnstcn May 21, 2025
4997f50
adjust tempfile naming
johnstcn May 21, 2025
b61468b
escape dots in sshTempFilePathExpr
johnstcn May 21, 2025
8ac1e78
improve error message on failure to write/rename
johnstcn May 21, 2025
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
Next Next commit
fix: SSHConfig: atomically write ssh config
  • Loading branch information
@johnstcn
johnstcn committed May 21, 2025
commit f9943cb2df7c6fb16692bdd137d50302fbde423e
37 changes: 29 additions & 8 deletions src/sshConfig.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ const mockFileSystem = {
readFile: vi.fn(),
mkdir: vi.fn(),
writeFile: vi.fn(),
rename: vi.fn(),
}

afterEach(() => {
Expand Down Expand Up @@ -38,7 +39,12 @@ Host coder-vscode--*
# --- END CODER VSCODE ---`

expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(
expect.stringContaining(sshFilePath),
expectedOutput,
expect.anything(),
)
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("creates a new file and adds the config", async () => {
Expand All @@ -65,7 +71,12 @@ Host coder-vscode.dev.coder.com--*
# --- END CODER VSCODE dev.coder.com ---`

expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(
expect.stringContaining(sshFilePath),
expectedOutput,
expect.anything(),
)
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("adds a new coder config in an existent SSH configuration", async () => {
Expand Down Expand Up @@ -100,10 +111,11 @@ Host coder-vscode.dev.coder.com--*
UserKnownHostsFile /dev/null
# --- END CODER VSCODE dev.coder.com ---`

expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringContaining(sshFilePath), expectedOutput, {
encoding: "utf-8",
mode: 384,
})
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("updates an existent coder config", async () => {
Expand Down Expand Up @@ -164,10 +176,11 @@ Host coder-vscode.dev-updated.coder.com--*
Host *
SetEnv TEST=1`

expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringContaining(sshFilePath), expectedOutput, {
encoding: "utf-8",
mode: 384,
})
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("does not remove deployment-unaware SSH config and adds the new one", async () => {
Expand Down Expand Up @@ -209,10 +222,11 @@ Host coder-vscode.dev.coder.com--*
UserKnownHostsFile /dev/null
# --- END CODER VSCODE dev.coder.com ---`

expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringContaining(sshFilePath), expectedOutput, {
encoding: "utf-8",
mode: 384,
})
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("it does not remove a user-added block that only matches the host of an old coder SSH config", async () => {
Expand Down Expand Up @@ -243,10 +257,11 @@ Host coder-vscode.dev.coder.com--*
UserKnownHostsFile /dev/null
# --- END CODER VSCODE dev.coder.com ---`

expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringContaining(sshFilePath), expectedOutput, {
encoding: "utf-8",
mode: 384,
})
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("throws an error if there is a missing end block", async () => {
Expand Down Expand Up @@ -517,10 +532,11 @@ Host afterconfig
LogLevel: "ERROR",
})

expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, {
expect(mockFileSystem.writeFile).toBeCalledWith(expect.stringContaining(sshFilePath), expectedOutput, {
encoding: "utf-8",
mode: 384,
})
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})

it("override values", async () => {
Expand Down Expand Up @@ -561,5 +577,10 @@ Host coder-vscode.dev.coder.com--*
# --- END CODER VSCODE dev.coder.com ---`

expect(mockFileSystem.readFile).toBeCalledWith(sshFilePath, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(sshFilePath, expectedOutput, expect.anything())
expect(mockFileSystem.writeFile).toBeCalledWith(
expect.stringContaining(sshFilePath),
expectedOutput,
expect.anything(),
)
expect(mockFileSystem.rename).toBeCalledWith(expect.stringContaining(sshFilePath + "."), sshFilePath)
})
9 changes: 7 additions & 2 deletions src/sshConfig.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { mkdir, readFile, writeFile } from "fs/promises"
import { mkdir, readFile, writeFile, rename } from "fs/promises"
import path from "path"
import { countSubstring } from "./util"

Expand All @@ -23,12 +23,14 @@ export interface FileSystem {
readFile: typeof readFile
mkdir: typeof mkdir
writeFile: typeof writeFile
rename: typeof rename
}

const defaultFileSystem: FileSystem = {
readFile,
mkdir,
writeFile,
rename,
}

// mergeSSHConfigValues will take a given ssh config and merge it with the overrides
Expand Down Expand Up @@ -224,10 +226,13 @@ export class SSHConfig {
mode: 0o700, // only owner has rwx permission, not group or everyone.
recursive: true,
})
return this.fileSystem.writeFile(this.filePath, this.getRaw(), {
const randSuffix = Math.random().toString(36).substring(8)
const tempFilePath = `${this.filePath}.${randSuffix}`
await this.fileSystem.writeFile(tempFilePath, this.getRaw(), {
Copy link
Member

Choose a reason for hiding this comment

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

I am probably overthinking this, but if there is somehow a file with this suffix already, would we want to error instead of overwrite? We could use flag: "wx" on the writeFile call to error if the path already exists.

Vanishingly unlikely, but maybe there could be a freak collision where a user has a ~/.ssh/config.conf and the random suffix was also conf.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. We could just write to /tmp
  2. We could instead name it something like config.vscode-coder.${timestamp}.${randSuffix} to minimize the likelihood of collisions

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I like 2 personally because I think it gives us a better chance to get the temp files for debugging, with /tmp they could be long gone.

But I could see an argument for /tmp to avoid "clutter".

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about a ULID? It would be sufficiently random and you could sort them by time to determine write order when debugging.

Copy link
Member

Choose a reason for hiding this comment

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

If we want to guarantee atomicity, same folder is best (/tmp for instance could be on another filesystem).

To hide clutter in the case of error, I'd suggest a . prefix on the filename, perhaps .tmp.[filename].[rand]. At this point guaranteeing uniqueness beyond this would seem like overkill.

I.e. my suggestion is just /home/user/.ssh/config -> /home/user/.ssh/.tmp.config.XXXXXX.

One use-case that won't work here is if /home/user/.ssh/config is bind-mounted. In that situation we either error out or try to write the file directly. Erroring is obviously safest so perhaps we go with that.

Copy link
Member Author

@johnstcn johnstcn May 21, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

What about a ULID?

I like the idea in theory, but it would be another import.

Copy link
Member

Choose a reason for hiding this comment

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

Forgot to write down one more thing, we should also respect the existing file modes and permissions and make sure they're copied over.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done. I've also adjusted the tempfile naming convention to ~/.ssh/.config.vscode-coder-tmp.abc123 to make it abundantly clear that

  1. This refers to ~/.ssh/config
  2. It was created by vscode-coder

mode: 0o600, // owner rw
encoding: "utf-8",
})
await this.fileSystem.rename(tempFilePath, this.filePath)
}

public getRaw() {
Expand Down