diff --git a/.gitattributes b/.gitattributes index 6313b56c57..08d76f5cdb 100644 --- a/.gitattributes +++ b/.gitattributes @@ -1 +1,3 @@ * text=auto eol=lf +pnpm-lock.yaml linguist-generated=true +flake.lock linguist-generated=true diff --git a/.github/actions/setup/action.yml b/.github/actions/setup/action.yml index 27a655b4ae..f980648f76 100644 --- a/.github/actions/setup/action.yml +++ b/.github/actions/setup/action.yml @@ -10,11 +10,15 @@ runs: run: git config --global url."https://github.com/".insteadOf "git@github.com:" - uses: pnpm/action-setup@ea17c68df8912ef543352723c149a84f56e3d413 # v6.1.0 + with: + # Caches the pnpm store, keyed by lockfile hash with a prefix fallback, + # so a dependency bump reuses the rest of the store. setup-node's own + # pnpm cache restores on an exact hash only. + cache: true - uses: actions/setup-node@820762786026740c76f36085b0efc47a31fe5020 # v7.0.0 with: node-version: "22" - cache: "pnpm" - name: Install dependencies shell: bash diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index fdcbfcff5e..6301c1bb6f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -48,6 +48,11 @@ jobs: name: Windows, electron-version: "latest", } + - { + os: windows-11-arm, + name: Windows ARM64, + electron-version: "latest", + } - { os: macos-15, name: macOS, electron-version: "latest" } steps: @@ -62,6 +67,7 @@ jobs: shell: bash env: CI: true + EXPECTED_ARCH: ${{ runner.arch }} test-integration: name: Integration Test (${{ matrix.name }}, VS Code ${{ matrix.vscode-version }}) diff --git a/.vscodeignore b/.vscodeignore index 93ceb676ab..0997b1288f 100644 --- a/.vscodeignore +++ b/.vscodeignore @@ -62,4 +62,4 @@ AGENTS.md # Storybook .storybook/** storybook-static/** -**/*.stories.* \ No newline at end of file +**/*.stories.* diff --git a/CHANGELOG.md b/CHANGELOG.md index b1c4b35055..a8ce8c5ff6 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,10 @@ replay the buffered connection logs instead. Close codes never reached the reconnect logic, so these closes retried forever. Server-initiated normal closes (`1000`/`1001`) keep reconnecting. +- Repair permissions on the Windows SSH config files the extension generates, + so connections stop failing with "Bad owner or permissions". Only you, + SYSTEM, and Administrators keep access to them. The extension leaves your own + SSH config untouched and needs no admin rights. ## [v1.16.3](https://github.com/coder/vscode-coder/releases/tag/v1.16.3) 2026-09-14 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index ff4ed30b67..18bf8c53fa 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -87,6 +87,41 @@ command. Coder Remote periodically reads the `network-info-dir + "/" + matchingSSHPID` file to display network information. +### Windows SSH config permissions + +Windows files inherit their permissions from the directory they live in, so a +config the extension generates under `%APPDATA%\coder.coder-remote\ssh` can end +up readable by other accounts. OpenSSH rejects such a file with "Bad owner or +permissions" and skips the whole `Include`, which blocks every Coder host, not +just the one it came from. + +Before each managed write, `src/remote/windowsAcl.ts` locks the directory down +and lets its files inherit from it: + +| Step | Command | +| -------------------------------------------------------- | ---------------------------------------------------- | +| Read the current user's SID | `whoami.exe /user /fo csv /nh` | +| Clear the directory's own grants | `icacls.exe /reset` | +| Grant that user, SYSTEM, and Administrators full control | `icacls.exe /inheritance:r /grant:r ` | +| Clear each `*.conf` file so it inherits the directory | `icacls.exe /reset` | + +Resetting every `*.conf` file, not only the one being written, also repairs +files left behind by other deployments and editors. + +Worth knowing: + +- Like VS Code, the code checks exit codes but never reads ACLs back. It needs + no script, native module, ownership change, or elevation, and it leaves the + user's own SSH config alone. +- Links and non-files are rejected before the repair, because inheritable + grants reach children even without `/T`. That stops mistakes, not an attacker + racing the check. +- The repair is not atomic: a failure after `/reset` can leave the directory + with its parent's grants. + +`windowsAcl.native.test.ts` drives the real `icacls.exe`, `whoami.exe`, and +OpenSSH. Run it unelevated as well as in CI to catch privilege assumptions. + ## Other features The extension provides several sidebar panels: @@ -289,7 +324,8 @@ When updating the minimum Node.js version, update these files: Some dependencies are not directly used in the source but are required anyway. -- `bufferutil` and `utf-8-validate` are peer dependencies of `ws`. +- `bufferutil` and `utf-8-validate` are peer dependencies of `ws`. Their source + builds are off, so Windows on ARM64 uses their JavaScript fallback. - `ua-parser-js` and `dayjs` are used by the Coder API client. The coder client is vendored from coder/coder. Pin it to a release tag in diff --git a/pnpm-workspace.yaml b/pnpm-workspace.yaml index f0647df913..f29929bb40 100644 --- a/pnpm-workspace.yaml +++ b/pnpm-workspace.yaml @@ -32,13 +32,14 @@ dedupePeers: true allowBuilds: "@vscode/vsce-sign": true - bufferutil: true + # Only win32-arm64 lacks a prebuild, and its JavaScript fallback is fine. + bufferutil: false electron: true esbuild: true keytar: false odiff-bin: true unrs-resolver: true - utf-8-validate: true + utf-8-validate: false overrides: "@vscode-elements/elements": ^2.5.1 diff --git a/src/remote/remote.ts b/src/remote/remote.ts index 9c18dad9a9..ce0321c1f1 100644 --- a/src/remote/remote.ts +++ b/src/remote/remote.ts @@ -67,6 +67,7 @@ import { sshSupportsSetEnv, type SshProperties, } from "./sshSupport"; +import { createManagedPermissions } from "./windowsAcl"; import { WorkspaceStateMachine } from "./workspaceStateMachine"; import type { Api } from "coder/site/src/api/api"; @@ -929,6 +930,8 @@ export class Remote { const coderConfig = new SshConfig( this.pathResolver.getSshConfigPath(safeHostname, hostEditorId(sshHost)), this.logger, + undefined, + createManagedPermissions(), ); // Options the user set themselves win the merge below, so they are exempt diff --git a/src/remote/sshConfig.ts b/src/remote/sshConfig.ts index e6fe17914f..8a95ff53cf 100644 --- a/src/remote/sshConfig.ts +++ b/src/remote/sshConfig.ts @@ -1,6 +1,7 @@ import { mkdir, readFile, + readdir, rename, stat, unlink, @@ -34,10 +35,20 @@ export interface SshValues { SetEnv?: string; } +/** + * Restricts the Coder-managed config directory and the files it generates. + * A config without one is not Coder-managed, so it is written untouched. + */ +export interface ManagedPermissions { + prepareDirectory(directory: string): Promise; + secure(filePath: string): Promise; +} + /** Injectable for tests. */ export interface FileSystem { mkdir: typeof mkdir; readFile: typeof readFile; + readdir: typeof readdir; rename: typeof rename; stat: typeof stat; unlink: typeof unlink; @@ -47,6 +58,7 @@ export interface FileSystem { const defaultFileSystem: FileSystem = { mkdir, readFile, + readdir, rename, stat, unlink, @@ -299,15 +311,19 @@ export class SshConfig { private readonly fileSystem: FileSystem; private readonly logger: Logger; private raw: string | undefined; + /** Marks this file as Coder-managed; absent for the user's own config. */ + private readonly permissions: ManagedPermissions | undefined; constructor( filePath: string, logger: Logger, fileSystem: FileSystem = defaultFileSystem, + permissions?: ManagedPermissions, ) { this.filePath = filePath; this.logger = logger; this.fileSystem = fileSystem; + this.permissions = permissions; } async load() { @@ -442,39 +458,99 @@ export class SshConfig { /** Atomically write raw via a temp file. */ private async save(): Promise { - // Preserve the existing file mode. - const existingMode = await this.fileSystem - .stat(this.filePath) - .then((stat) => stat.mode) - .catch((ex: NodeJS.ErrnoException) => { - if (ex.code === "ENOENT") { - return 0o600; - } - throw ex; - }); - await this.fileSystem.mkdir(path.dirname(this.filePath), { + const existingMode = await this.getFileMode(); + const fileName = path.basename(this.filePath); + const dirName = path.dirname(this.filePath); + await this.fileSystem.mkdir(dirName, { mode: 0o700, recursive: true, }); - const fileName = path.basename(this.filePath); - const dirName = path.dirname(this.filePath); + // Must come before any file reset or temporary write in this directory. + await this.permissions?.prepareDirectory(dirName); + await this.repairIncludedFiles(dirName); const tempPath = tempFilePath( `${dirName}/.${fileName}`, "vscode-coder-tmp", ); + await this.writeTemp(tempPath, existingMode); + await this.repairPermissions(tempPath); + await this.replaceWithTemp(tempPath); + } + + /** Preserve the existing file mode, defaulting to owner-only access. */ + private async getFileMode(): Promise { + try { + return (await this.fileSystem.stat(this.filePath)).mode; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") { + return 0o600; + } + throw error; + } + } + + /** Repair every direct Include match; one unsafe sibling blocks every host. */ + private async repairIncludedFiles(dirName: string): Promise { + if (!this.permissions) return; + const entries = await this.fileSystem + .readdir(dirName, { withFileTypes: true }) + .catch((error: unknown) => { + this.logger.warn( + "Failed to enumerate Coder-managed SSH config files", + error, + ); + return []; + }); + for (const entry of entries) { + if (!entry.name.toLowerCase().endsWith(SSH_CONFIG_EXT)) continue; + const filePath = path.join(dirName, entry.name); + // On Windows, fopen fails on a directory, so OpenSSH aborts the whole + // Include. No ACL change fixes that, so report it instead. + if (!entry.isFile()) { + throw new Error( + `SSH config entry ${filePath} is not a regular file. Move or rename it so it no longer matches *.conf, then reconnect.`, + ); + } + await this.repairPermissions(filePath); + } + } + + /** Create the temporary file exclusively, leaving any preexisting path alone. */ + private async writeTemp(tempPath: string, mode: number): Promise { try { await this.fileSystem.writeFile(tempPath, this.getRaw(), { - mode: existingMode, encoding: "utf-8", + flag: "wx", + mode, }); } catch (err) { + // On EEXIST this write did not create the path, so it must not delete it. + if ((err as NodeJS.ErrnoException).code !== "EEXIST") { + await this.discardTemp(tempPath); + } throw new Error( `Failed to write temporary SSH config file at ${tempPath}: ${err instanceof Error ? err.message : String(err)}. ` + `Please check your disk space, permissions, and that the directory exists.`, { cause: err }, ); } + } + + /** Log a repair failure without preventing an SSH connection attempt. */ + private async repairPermissions(filePath: string): Promise { + try { + await this.permissions?.secure(filePath); + } catch (error) { + this.logger.warn( + "Failed to repair SSH config permissions", + filePath, + error, + ); + } + } + /** Replace the destination atomically, cleaning up if the rename fails. */ + private async replaceWithTemp(tempPath: string): Promise { try { await renameWithRetry( (src, dest) => this.fileSystem.rename(src, dest), @@ -493,6 +569,7 @@ export class SshConfig { } } + /** Attempt cleanup without hiding the original write or rename failure. */ private async discardTemp(tempPath: string): Promise { try { await this.fileSystem.unlink(tempPath); diff --git a/src/remote/windowsAcl.ts b/src/remote/windowsAcl.ts new file mode 100644 index 0000000000..4684872a9c --- /dev/null +++ b/src/remote/windowsAcl.ts @@ -0,0 +1,129 @@ +import { execFile } from "node:child_process"; +import { lstat, readdir } from "node:fs/promises"; +import * as path from "node:path"; +import { promisify } from "node:util"; + +import { wrapError } from "../error/errorUtils"; + +import type { ManagedPermissions } from "./sshConfig"; + +const EXECUTE = promisify(execFile); + +/** Protect the Coder-managed SSH directory and repair its files by inheritance. */ +export const WINDOWS_ACL: ManagedPermissions = { prepareDirectory, secure }; + +/** Use DACL repair on Windows; other platforms rely on the file mode. */ +export function createManagedPermissions(): ManagedPermissions | undefined { + return process.platform === "win32" ? WINDOWS_ACL : undefined; +} + +/** Resolve a system tool without searching PATH or the working directory. */ +export function system32(name: string): string { + const systemRoot = process.env.SystemRoot; + if (!systemRoot || !isFullyQualifiedWindowsPath(systemRoot)) { + throw new Error("SystemRoot must be a fully qualified Windows path"); + } + return path.win32.join(systemRoot, "System32", name); +} + +/** Grant inheritable full control to the user, SYSTEM, and Administrators only. */ +async function prepareDirectory(target: string): Promise { + const directory = path.win32.normalize(target); + try { + checkPath(directory); + if (!(await lstat(directory)).isDirectory()) { + throw new Error("Expected a Coder-managed directory without links"); + } + // Inheritable grants reach children even without /T, so vet them first. + for (const entry of await readdir(directory)) { + try { + await checkRegularFile(path.win32.join(directory, entry)); + } catch (error) { + // Another window's temporary config can be renamed away mid-scan. + if ((error as NodeJS.ErrnoException).code !== "ENOENT") { + throw error; + } + } + } + const sid = await currentUserSid(); + // /grant:r alone leaves other trustees' explicit grants and denies. + await run("icacls.exe", [directory, "/reset"]); + await run("icacls.exe", [ + directory, + "/inheritance:r", + "/grant:r", + `*${sid}:(OI)(CI)F`, + "*S-1-5-18:(OI)(CI)F", // SYSTEM + "*S-1-5-32-544:(OI)(CI)F", // Administrators + ]); + } catch (error) { + throw wrapError( + "prepare SSH config directory permissions for", + directory, + error, + ); + } +} + +/** Drop a file's own permissions so it inherits the directory's. */ +async function secure(target: string): Promise { + const file = path.win32.normalize(target); + try { + await checkRegularFile(file); + await run("icacls.exe", [file, "/reset"]); + } catch (error) { + throw wrapError("repair SSH config permissions for", file, error); + } +} + +/** Accept drive-qualified or UNC paths without wildcards or control characters. */ +function isFullyQualifiedWindowsPath(target: string): boolean { + if (/[\0\r\n*?]/.test(target)) { + return false; + } + const normalized = path.win32.normalize(target); + return ( + /^[A-Za-z]:\\/.test(normalized) || + /^\\\\[^\\]+\\[^\\]+(?:\\|$)/.test(normalized) + ); +} + +/** Reject ambiguous paths and characters that can expand an icacls target. */ +function checkPath(target: string): void { + if (!isFullyQualifiedWindowsPath(target)) { + throw new Error( + "Expected a fully qualified Windows path without wildcards or control characters", + ); + } +} + +/** Reject links and non-files, which share their ACL with another target. */ +async function checkRegularFile(target: string): Promise { + checkPath(target); + const stat = await lstat(target); + if (!stat.isFile() || stat.nlink !== 1) { + throw new Error( + `Expected a regular file with no links: ${target}. Move or rename linked and non-file entries out of the Coder-managed SSH directory, then reconnect.`, + ); + } +} + +/** Read the SID, not the localized account name, from whoami's CSV output. */ +async function currentUserSid(): Promise { + const { stdout } = await run("whoami.exe", ["/user", "/fo", "csv", "/nh"]); + const sid = /,"(S-\d+(?:-\d+)+)"\s*$/.exec(stdout)?.[1]; + if (!sid) { + throw new Error("Could not read the current Windows user SID"); + } + return sid; +} + +/** Run a system tool without a shell, bounding its time and output. */ +function run(name: string, args: string[]) { + return EXECUTE(system32(name), args, { + windowsHide: true, + timeout: 10_000, + maxBuffer: 64 * 1024, + encoding: "utf8", + }); +} diff --git a/test/env-check.ts b/test/env-check.ts new file mode 100644 index 0000000000..9dc55224b5 --- /dev/null +++ b/test/env-check.ts @@ -0,0 +1,14 @@ +import assert from "node:assert/strict"; + +/** Fails the run when the test runtime is not the architecture CI expects. */ +export default function checkEnv(): void { + const expected = process.env.EXPECTED_ARCH?.toLowerCase(); + if (!expected) { + return; + } + assert.equal( + process.arch, + expected, + `Test runtime architecture is ${process.arch}, but CI expects ${expected}`, + ); +} diff --git a/test/unit/remote/sshConfig.test.ts b/test/unit/remote/sshConfig.test.ts index ce70715f74..a711653dfb 100644 --- a/test/unit/remote/sshConfig.test.ts +++ b/test/unit/remote/sshConfig.test.ts @@ -1,6 +1,7 @@ import { vol } from "memfs"; import * as fsPromises from "node:fs/promises"; import * as os from "node:os"; +import * as path from "node:path"; import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; import { @@ -8,13 +9,17 @@ import { parseCoderSshOptions, parseSshConfig, SshConfig, + type ManagedPermissions, type SshValues, validateDeploymentSshOptions, } from "@/remote/sshConfig"; import { createMockLogger } from "../../mocks/testHelpers"; -vi.mock("node:fs/promises", async () => (await import("memfs")).fs.promises); +vi.mock("node:fs/promises", async () => { + const fs = (await import("memfs")).fs.promises; + return { ...fs, readdir: vi.fn(fs.readdir) }; +}); vi.mock("node:os", async (importOriginal) => { const actual = await importOriginal(); return { ...actual, homedir: vi.fn(() => "/Path/To/UserHomeDir") }; @@ -22,6 +27,10 @@ vi.mock("node:os", async (importOriginal) => { const homeDir = "/Path/To/UserHomeDir"; const sshFilePath = "/Path/To/UserHomeDir/.sshConfigDir/sshConfigFile"; +const MANAGED_SSH_DIR = path.join(homeDir, ".sshConfigDir"); +const MANAGED_SSH_FILE_PATH = path.join(MANAGED_SSH_DIR, "deployment.conf"); +const TEMP_MARKER = "vscode-coder-tmp"; +const FAILURE = new Error("permission repair failed"); const hostname = "dev.coder.com"; const fileHeader = `# Coder workspace hosts. Do not edit; the Coder extension rewrites this file # on every connection. Override options with the "coder.sshConfig" setting.`; @@ -83,6 +92,8 @@ const includeBlock = renderIncludeBlock(includeDir); const mockLogger = createMockLogger(); const readConfig = () => fsPromises.readFile(sshFilePath, "utf-8"); +const tempFiles = () => + Object.keys(vol.toJSON()).filter((file) => file.includes(TEMP_MARKER)); async function loadSshConfig( contents?: string, @@ -116,6 +127,8 @@ async function updateInclude( beforeEach(() => { vol.reset(); + vi.clearAllMocks(); + vi.mocked(fsPromises.readdir).mockReset(); vi.mocked(os.homedir).mockReturnValue(homeDir); }); @@ -378,6 +391,21 @@ describe("persistence", () => { ); }); + it("does not remove a preexisting exclusive temp path", async () => { + vi.spyOn(crypto, "randomUUID").mockReturnValue( + "00000000-0000-0000-0000-000000000000", + ); + const tempPath = + "/Path/To/UserHomeDir/.sshConfigDir/.sshConfigFile.vscode-coder-tmp-00000000"; + vol.fromJSON({ [tempPath]: "unrelated temp" }); + const sshConfig = await loadSshConfig(); + + await expect(sshConfig.update(BASE_SSH_VALUES)).rejects.toThrow( + "Failed to write temporary SSH config file", + ); + expect(await fsPromises.readFile(tempPath, "utf-8")).toBe("unrelated temp"); + }); + it("wraps rename failures and removes the temporary file", async () => { const sshConfig = await loadSshConfig("Host initial"); const error = Object.assign(new Error("EXDEV"), { code: "EXDEV" }); @@ -410,6 +438,165 @@ describe("persistence", () => { }); }); +describe("managed config permissions", () => { + const sibling = path.join(MANAGED_SSH_DIR, "sibling.conf"); + const upperCase = path.join(MANAGED_SSH_DIR, "upper.CONF"); + const siblings = { [sibling]: "Host sibling", [upperCase]: "Host upper" }; + const existing = { [MANAGED_SSH_FILE_PATH]: "Host original" }; + const anyTempFile = expect.stringContaining(TEMP_MARKER); + + /** Repair that fails for the paths the test names. */ + function permissions(...failures: readonly string[]): ManagedPermissions { + return { + prepareDirectory: vi.fn(() => Promise.resolve()), + secure: vi.fn((filePath: string) => + failures.some((name) => filePath.includes(name)) + ? Promise.reject(FAILURE) + : Promise.resolve(), + ), + }; + } + + function updateManaged( + acl: ManagedPermissions, + files: Record, + ) { + vol.fromJSON(files); + return new SshConfig( + MANAGED_SSH_FILE_PATH, + mockLogger, + fsPromises, + acl, + ).update(BASE_SSH_VALUES); + } + + const securedPaths = (acl: ManagedPermissions) => + vi.mocked(acl.secure).mock.calls.flat(); + + interface RepairCase { + name: string; + files: Record; + failures: readonly string[]; + secured: unknown[]; + warnings: number; + } + it.each([ + { + name: "rewriting an existing deployment", + files: { ...existing, ...siblings }, + failures: [], + // The file being replaced is repaired too: the rename needs access to it. + secured: [MANAGED_SSH_FILE_PATH, sibling, upperCase, anyTempFile], + warnings: 0, + }, + { + name: "adding the first file of a new deployment", + files: siblings, + failures: [], + secured: [sibling, upperCase, anyTempFile], + warnings: 0, + }, + { + name: "a sibling and the temp file cannot be repaired", + files: { ...existing, ...siblings }, + failures: ["sibling.conf", TEMP_MARKER], + secured: [MANAGED_SSH_FILE_PATH, sibling, upperCase, anyTempFile], + warnings: 2, + }, + ])( + "repairs every Include match and the temp file when $name", + async ({ files, failures, secured, warnings }) => { + const acl = permissions(...failures); + + await updateManaged(acl, files); + + expect(acl.prepareDirectory).toHaveBeenCalledWith(MANAGED_SSH_DIR); + expect(securedPaths(acl)).toEqual(secured); + expect( + await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8"), + ).toContain("ProxyCommand some-command-here"); + expect(await fsPromises.readFile(sibling, "utf-8")).toBe("Host sibling"); + expect(mockLogger.warn).toHaveBeenCalledTimes(warnings); + expect(tempFiles()).toEqual([]); + }, + ); + + it("repairs the temp file before it replaces the config", async () => { + const acl = permissions(); + let destinationDuringRepair: string | undefined; + vi.mocked(acl.secure).mockImplementation(async (filePath) => { + if (filePath.includes(TEMP_MARKER)) { + destinationDuringRepair = await fsPromises.readFile( + MANAGED_SSH_FILE_PATH, + "utf-8", + ); + } + }); + + await updateManaged(acl, existing); + + // Undefined would mean the temp file was never repaired at all. + expect(destinationDuringRepair).toBe("Host original"); + }); + + it("writes and repairs nothing when directory preparation fails", async () => { + const acl = permissions(); + vi.mocked(acl.prepareDirectory).mockRejectedValueOnce(FAILURE); + + await expect( + updateManaged(acl, { ...existing, ...siblings }), + ).rejects.toThrow(FAILURE); + + expect(acl.secure).not.toHaveBeenCalled(); + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf8")).toBe( + "Host original", + ); + expect(tempFiles()).toEqual([]); + }); + + it("preserves the config and reports a directory matching the Include", async () => { + const directory = path.join(MANAGED_SSH_DIR, "directory.conf"); + + await expect( + updateManaged(permissions(), { ...existing, [directory]: null }), + ).rejects.toThrow(`SSH config entry ${directory} is not a regular file`); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toBe( + "Host original", + ); + expect(tempFiles()).toEqual([]); + }); + + it("warns and still writes the config when enumeration fails", async () => { + const error = new Error("cannot enumerate"); + vi.mocked(fsPromises.readdir).mockRejectedValueOnce(error); + + await updateManaged(permissions(), existing); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toContain( + "ProxyCommand some-command-here", + ); + expect(mockLogger.warn).toHaveBeenCalledWith( + "Failed to enumerate Coder-managed SSH config files", + error, + ); + }); + + it("neither repairs nor enumerates without injected permissions", async () => { + vol.fromJSON(existing); + + await new SshConfig(MANAGED_SSH_FILE_PATH, mockLogger, fsPromises).update( + BASE_SSH_VALUES, + ); + + expect(await fsPromises.readFile(MANAGED_SSH_FILE_PATH, "utf-8")).toContain( + "ProxyCommand some-command-here", + ); + expect(fsPromises.readdir).not.toHaveBeenCalled(); + expect(mockLogger.warn).not.toHaveBeenCalled(); + }); +}); + describe("parseSshConfig", () => { interface ParseSshConfigCase { name: string; diff --git a/test/unit/remote/windowsAcl.native.test.ts b/test/unit/remote/windowsAcl.native.test.ts new file mode 100644 index 0000000000..5a48f5ea06 --- /dev/null +++ b/test/unit/remote/windowsAcl.native.test.ts @@ -0,0 +1,221 @@ +import { execFile as execFileCallback } from "node:child_process"; +import { randomUUID } from "node:crypto"; +import * as fs from "node:fs/promises"; +import * as os from "node:os"; +import * as path from "node:path"; +import { promisify } from "node:util"; +import { describe, expect, it, onTestFinished } from "vitest"; + +import { SshConfig, type SshValues } from "@/remote/sshConfig"; +import { system32, WINDOWS_ACL } from "@/remote/windowsAcl"; + +import { createMockLogger } from "../../mocks/testHelpers"; + +import type { Logger } from "@/logging/logger"; + +const execFile = promisify(execFileCallback); +const GUESTS_SID = "S-1-5-32-546"; +const GUESTS_ALIAS = "BG"; + +// Real icacls and ssh, no mocks. Needs the Windows OpenSSH client. Run it +// without elevation as well as in CI to catch privilege assumptions. +describe.runIf(process.platform === "win32")( + "Windows SSH config ACL repair", + () => { + it( + "repairs the directory and every fragment so OpenSSH accepts them", + { timeout: 45_000 }, + async () => { + const { root, logger } = await nativeFixture(); + const directory = path.join(root, "coder.coder-remote", "ssh"); + const userConfig = path.join(root, "config"); + const current = path.join(directory, "current.conf"); + const other = path.join(directory, "other.conf"); + const host = "coder-acl-current"; + await fs.mkdir(directory, { recursive: true }); + await fs.writeFile( + userConfig, + `Include "${directory.replaceAll("\\", "/")}/*.conf"\n`, + ); + await fs.writeFile(current, renderConfig(host, "unsafe-user")); + await fs.writeFile(other, renderConfig("coder-acl-other", "other")); + + // A fragment Guests can reach makes OpenSSH refuse the whole include. + // One inherits the grant, the other blocks inheritance and keeps its + // own, which the directory repair alone would miss. + await grant(directory, GUESTS_SID, "(OI)(CI)(F)"); + await grant(current, GUESTS_SID, "(F)"); + await execFile(system32("icacls.exe"), [other, "/inheritancelevel:d"]); + await grant(other, GUESTS_SID, "(F)"); + const rootBefore = await savedAcl(root); + const userConfigBefore = await savedAcl(userConfig); + await expect(resolve(userConfig, host)).rejects.toMatchObject({ + stderr: expect.stringContaining("Bad owner or permissions"), + }); + + await managed(current, logger).update(sshValues(host, "repaired")); + + expect(await resolve(userConfig, host)).toContain( + "proxycommand repaired", + ); + expect(await savedAcl(directory)).not.toContain(GUESTS_ALIAS); + for (const fragment of [current, other]) { + const acl = await savedAcl(fragment); + expect(acl).toContain("A;ID;"); + expect(acl).not.toContain(GUESTS_ALIAS); + } + expect(logger.warn).not.toHaveBeenCalled(); + expect(await savedAcl(root)).toBe(rootBefore); + expect(await savedAcl(userConfig)).toBe(userConfigBefore); + + // /inheritance:r keeps a later parent grant out of the directory. + const repaired = await savedAcl(directory); + await grant(root, GUESTS_SID, "(OI)(CI)(F)"); + expect(await savedAcl(root)).not.toBe(rootBefore); + expect(await savedAcl(directory)).toBe(repaired); + + // Reconnecting still rewrites the file the repair locked down. + await managed(current, logger).update(sshValues(host, "rewritten")); + expect(await resolve(userConfig, host)).toContain( + "proxycommand rewritten", + ); + expect(await savedAcl(directory)).toBe(repaired); + expect(logger.warn).not.toHaveBeenCalled(); + }, + ); + + interface UnsafeEntryCase { + name: string; + /** Adds the unsafe entry and names extra paths that must not change. */ + create: (directory: string, root: string) => Promise; + } + it.each([ + { + name: "a hard link to a file outside it", + create: async (directory, root) => { + const external = path.join(root, "external.conf"); + await fs.writeFile(external, "# external"); + await fs.link(external, path.join(directory, "linked.conf")); + return [external]; + }, + }, + { + name: "a directory matching *.conf", + create: async (directory) => { + await fs.mkdir(path.join(directory, "folder.conf")); + return []; + }, + }, + ])( + "refuses to write when the managed directory holds $name", + async ({ create }) => { + const { root, logger } = await nativeFixture(); + const directory = path.join(root, "managed"); + const current = path.join(directory, "current.conf"); + await fs.mkdir(directory); + await fs.writeFile(current, "# unchanged"); + const watched = [ + directory, + current, + ...(await create(directory, root)), + ]; + await grant(directory, GUESTS_SID, "(OI)(CI)(F)"); + const before = await Promise.all(watched.map(savedAcl)); + + await expect( + managed(current, logger).update(sshValues("coder-unsafe", "unsafe")), + ).rejects.toThrow(); + + expect(await Promise.all(watched.map(savedAcl))).toEqual(before); + expect(await fs.readFile(current, "utf8")).toBe("# unchanged"); + expect( + (await fs.readdir(directory)).filter((name) => + name.includes("vscode-coder-tmp"), + ), + ).toEqual([]); + }, + ); + + it.each([ + { + name: "a directory junction", + create: async (root: string) => { + const target = path.join(root, "junction target"); + const candidate = path.join(root, "junction"); + await fs.mkdir(target); + await fs.symlink(target, candidate, "junction"); + return { candidate, unchanged: target }; + }, + }, + { + name: "a regular file", + create: async (root: string) => { + const candidate = path.join(root, "not a directory"); + await fs.writeFile(candidate, "unchanged"); + return { candidate, unchanged: candidate }; + }, + }, + ])("rejects $name as the managed directory", async ({ create }) => { + const { root } = await nativeFixture(); + const { candidate, unchanged } = await create(root); + const before = await savedAcl(unchanged); + + await expect(WINDOWS_ACL.prepareDirectory(candidate)).rejects.toThrow(); + expect(await savedAcl(unchanged)).toBe(before); + }); + }, +); + +const managed = (filePath: string, logger: Logger) => + new SshConfig(filePath, logger, undefined, WINDOWS_ACL); + +async function nativeFixture() { + const root = await fs.mkdtemp(path.join(os.tmpdir(), "coder acl é space-")); + onTestFinished(() => fs.rm(root, { recursive: true, force: true })); + return { root, logger: createMockLogger() }; +} + +function sshValues(host: string, proxyCommand: string): SshValues { + return { + Host: host, + ProxyCommand: proxyCommand, + ConnectTimeout: "0", + StrictHostKeyChecking: "no", + UserKnownHostsFile: "NUL", + LogLevel: "ERROR", + ServerAliveInterval: "10", + ServerAliveCountMax: "3", + }; +} + +const renderConfig = (host: string, proxyCommand: string) => + `Host ${host}\n ProxyCommand ${proxyCommand}\n`; + +/** Dump a path's ACL so a test can compare it before and after a repair. */ +async function savedAcl(pathname: string): Promise { + const backup = path.join(os.tmpdir(), `coder-acl-${randomUUID()}.txt`); + try { + await execFile(system32("icacls.exe"), [pathname, "/save", backup]); + return await fs.readFile(backup, "utf16le"); + } finally { + await fs.rm(backup, { force: true }); + } +} + +/** Resolve a host through the user's config to prove OpenSSH reads the include. */ +async function resolve(userConfig: string, host: string): Promise { + const { stdout } = await execFile(system32("OpenSSH/ssh.exe"), [ + "-G", + "-F", + userConfig, + host, + ]); + return stdout; +} + +const grant = (pathname: string, sid: string, permissions: string) => + execFile(system32("icacls.exe"), [ + pathname, + "/grant", + `*${sid}:${permissions}`, + ]); diff --git a/test/unit/remote/windowsAcl.test.ts b/test/unit/remote/windowsAcl.test.ts new file mode 100644 index 0000000000..24f9314e43 --- /dev/null +++ b/test/unit/remote/windowsAcl.test.ts @@ -0,0 +1,206 @@ +import { promisify } from "node:util"; +import { beforeEach, describe, expect, it, onTestFinished, vi } from "vitest"; + +import { WINDOWS_ACL } from "@/remote/windowsAcl"; + +import type { Stats } from "node:fs"; + +type FileStat = Pick; + +const { execute, lstat, readdir } = vi.hoisted(() => ({ + execute: + vi.fn<(command: string, args: string[]) => Promise<{ stdout: string }>>(), + lstat: vi.fn<(target: string) => Promise>(), + readdir: vi.fn<() => Promise>(), +})); + +vi.mock("node:child_process", () => ({ + execFile: Object.assign(vi.fn(), { [promisify.custom]: execute }), +})); +vi.mock("node:fs/promises", () => ({ lstat, readdir })); + +const DIRECTORY = "C:\\Users\\coder\\AppData\\Roaming\\coder.coder-remote\\ssh"; +const FILE = `${DIRECTORY}\\file.conf`; +const ICACLS = "C:\\Windows\\System32\\icacls.exe"; +const WHOAMI = "C:\\Windows\\System32\\whoami.exe"; +const SID = "S-1-5-21-1-2-3-1001"; +const WHOAMI_CSV = `"account","${SID}"\r\n`; + +const DIR: FileStat = { + isDirectory: () => true, + isFile: () => false, + nlink: 1, +}; +const FILE_STAT: FileStat = { + isDirectory: () => false, + isFile: () => true, + nlink: 1, +}; +const LINKED: FileStat = { ...FILE_STAT, nlink: 2 }; + +/** Each run as one line, so a test can assert the whole transcript at once. */ +const commandLines = () => + execute.mock.calls.map(([command, args]) => `${command} ${args.join(" ")}`); + +const ranIcacls = () => commandLines().some((line) => line.startsWith(ICACLS)); + +beforeEach(() => { + execute.mockReset().mockResolvedValue({ stdout: WHOAMI_CSV }); + lstat + .mockReset() + .mockImplementation((target) => + Promise.resolve(target === DIRECTORY ? DIR : FILE_STAT), + ); + readdir.mockReset().mockResolvedValue(["file.conf"]); + vi.stubEnv("SystemRoot", "C:\\Windows"); + onTestFinished(() => { + vi.unstubAllEnvs(); + }); +}); + +describe("WINDOWS_ACL", () => { + it("restricts the directory, then resets files to inherit from it", async () => { + await WINDOWS_ACL.prepareDirectory(DIRECTORY.replaceAll("\\", "/")); + await WINDOWS_ACL.secure(FILE); + + expect(commandLines()).toEqual([ + `${WHOAMI} /user /fo csv /nh`, + `${ICACLS} ${DIRECTORY} /reset`, + `${ICACLS} ${DIRECTORY} /inheritance:r /grant:r *${SID}:(OI)(CI)F ` + + "*S-1-5-18:(OI)(CI)F *S-1-5-32-544:(OI)(CI)F", + `${ICACLS} ${FILE} /reset`, + ]); + }); + + it("ignores an entry that disappears between readdir and lstat", async () => { + readdir.mockResolvedValue(["file.conf", "file.conf.tmp"]); + lstat.mockImplementation((target) => { + if (target === DIRECTORY) { + return Promise.resolve(DIR); + } + if (target.endsWith(".tmp")) { + return Promise.reject( + Object.assign(new Error("no such file"), { code: "ENOENT" }), + ); + } + return Promise.resolve(FILE_STAT); + }); + + await expect( + WINDOWS_ACL.prepareDirectory(DIRECTORY), + ).resolves.toBeUndefined(); + expect(ranIcacls()).toBe(true); + }); + + interface RejectCase { + name: string; + error: string; + arrange?: () => void; + call: () => Promise; + /** Extra assertion for rows that must stop before a specific step. */ + assert?: () => void; + } + it.each([ + { + name: "a relative directory", + error: "fully qualified", + call: () => WINDOWS_ACL.prepareDirectory("relative"), + }, + { + name: "a wildcard file path", + error: "fully qualified", + call: () => WINDOWS_ACL.secure(`${DIRECTORY}\\*.conf`), + }, + { + name: "a relative SystemRoot", + error: "SystemRoot must be", + arrange: () => vi.stubEnv("SystemRoot", "Windows"), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a missing SystemRoot", + error: "SystemRoot must be", + arrange: () => vi.stubEnv("SystemRoot", undefined), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a linked directory, before enumerating it", + error: "directory without links", + arrange: () => lstat.mockResolvedValue(FILE_STAT), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + assert: () => expect(readdir).not.toHaveBeenCalled(), + }, + { + name: "a linked child, before permissions can propagate", + error: "regular file with no links", + arrange: () => + lstat.mockResolvedValueOnce(DIR).mockResolvedValueOnce(LINKED), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "a linked file", + error: "regular file with no links", + arrange: () => lstat.mockResolvedValue(LINKED), + call: () => WINDOWS_ACL.secure(FILE), + }, + { + name: "whoami output with no SID", + error: "Could not read the current Windows user SID", + arrange: () => execute.mockResolvedValue({ stdout: '"account","nope"' }), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + { + name: "whoami output with an extra column", + error: "Could not read the current Windows user SID", + arrange: () => + execute.mockResolvedValue({ stdout: `${WHOAMI_CSV},"extra"` }), + call: () => WINDOWS_ACL.prepareDirectory(DIRECTORY), + }, + ])("rejects $name without changing permissions", async (testCase) => { + testCase.arrange?.(); + await expect(testCase.call()).rejects.toThrow(testCase.error); + expect(ranIcacls()).toBe(false); + testCase.assert?.(); + }); + + it("propagates an icacls failure without running the next command", async () => { + execute.mockImplementation((command) => + command === ICACLS + ? Promise.reject(new Error("access denied")) + : Promise.resolve({ stdout: WHOAMI_CSV }), + ); + + await expect(WINDOWS_ACL.prepareDirectory(DIRECTORY)).rejects.toThrow( + "access denied", + ); + await expect(WINDOWS_ACL.secure(FILE)).rejects.toThrow("access denied"); + + // The failing /reset must not be followed by a grant on a still-open directory. + expect(commandLines().some((line) => line.includes("/grant:r"))).toBe( + false, + ); + }); + + it.each([ + "C:\\Users\\coder\\config", + "C:/Users/coder/config", + "\\\\server\\share\\config", + ])("accepts the fully qualified path %s", async (target) => { + await expect(WINDOWS_ACL.secure(target)).resolves.toBeUndefined(); + }); + + it.each([ + "\\\\server", + "\\config", + "C:config", + "config", + "C:\\*.conf", + "C:\\file?.conf", + "C:\\file\r.conf", + "C:\\file\0.conf", + "C:\\file\n.conf", + ])("rejects the path %j", async (target) => { + await expect(WINDOWS_ACL.secure(target)).rejects.toThrow("fully qualified"); + expect(ranIcacls()).toBe(false); + }); +}); diff --git a/vitest.config.mts b/vitest.config.mts index 88ec0b90d9..9f1cbdd8ba 100644 --- a/vitest.config.mts +++ b/vitest.config.mts @@ -12,6 +12,7 @@ const testTimeout = process.platform === "win32" ? 10_000 : 5_000; export default defineConfig({ test: { testTimeout, + globalSetup: "./test/env-check.ts", projects: [ { extends: true,