Restrict workspace state on Windows, where the declared modes do nothing

src/state-store.js

@@ -4,6 +4,7 @@ import { homedir } from 'node:os';
 import { dirname, join } from 'node:path';
 import { randomBytes } from 'node:crypto';
 import { StateStoreError } from './errors.js';
+import { protectDirectoryForCurrentUser } from './windows-acl.js';
 
 const LOCK_STALE_MS = 5 * 60_000;
 const LOCK_WAIT_MS = 30_000;
@@ -123,6 +124,13 @@ export async function atomicWrite(path, content) {
 }
 
 async function secureDirectory(path) {
+  const root = stateRoot();
+  await mkdir(root, { recursive: true, mode: 0o700 });
+  await chmod(root, 0o700);
+  // Windows accepts both of the above and honours neither. The root carries an
+  // inheritable ACL instead, so every directory and file created below it is restricted
+  // without a call of its own.
+  await protectDirectoryForCurrentUser(root);
   await mkdir(path, { recursive: true, mode: 0o700 });
   await chmod(path, 0o700);
 }

src/state-store.test.js

@@ -2,16 +2,27 @@ import { afterEach, beforeEach, describe, expect, it } from 'vitest';
 import { mkdtemp, readFile, rm, stat, writeFile } from 'node:fs/promises';
 import { tmpdir } from 'node:os';
 import { join } from 'node:path';
+import { run } from './process.js';
+import { resetWindowsAclCache } from './windows-acl.js';
 import { readWorkspaceSecret, readWorkspaceState, withWorkspaceLock, workspaceStateDirectory, writeWorkspaceSecret, writeWorkspaceState } from './state-store.js';
 
+const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`;
+
 describe('workspace state store', () => {
   let root;
   beforeEach(async () => {
     root = await mkdtemp(join(tmpdir(), 'workspace-state-test-'));
     process.env.OPENCHAMBER_WORKSPACE_STATE_DIR = root;
+    if (process.platform === 'win32') {
+      // A profile's temporary directory is already private, so a store created there
+      // would look protected whether or not the code protects anything. This one is
+      // opened to everyone first, and the test can then observe it being closed.
+      await run(icacls, [root, '/grant', '*S-1-1-0:(OI)(CI)F', '/q']);
+    }
   });
   afterEach(async () => {
     delete process.env.OPENCHAMBER_WORKSPACE_STATE_DIR;
+    resetWindowsAclCache();
     await rm(root, { recursive: true, force: true });
   });
 
@@ -21,13 +32,17 @@ describe('workspace state store', () => {
     await writeWorkspaceSecret(id, 'endpoint-token', 'secret');
     expect(await readWorkspaceState(id)).toMatchObject({ lifecycle: 'ready' });
     expect(await readWorkspaceSecret(id, 'endpoint-token')).toBe('secret');
-    // POSIX modes are how this store restricts its state and secrets, and Windows does
-    // not implement them: `chmod` is close to a no-op there and every file reports 0o666.
-    // Asserting the modes on Windows would only restate that, so the check is skipped —
-    // but the protection genuinely is absent there, standing only on ACLs inherited from
-    // wherever the data directory happens to live. Enforcing it explicitly on Windows is
-    // outstanding work, not a platform difference that can be waved through.
-    if (process.platform !== 'win32') {
+    // Each platform is asked about the mechanism that actually restricts the store there:
+    // POSIX modes where they are implemented, and the inherited ACL on Windows, which
+    // accepts a chmod and honours none of it.
+    if (process.platform === 'win32') {
+      const secret = join(workspaceStateDirectory(id), 'secrets', 'endpoint-token');
+      const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`;
+      const { stdout } = await run(icacls, [secret]);
+      const granted = stdout.split(/\r?\n/).filter((line) => line.includes(':(')).join('\n');
+      expect(granted).not.toMatch(/Everyone|BUILTIN\\Users|Authenticated Users/i);
+      expect(granted).toContain(process.env.USERNAME);
+    } else {
       expect((await stat(workspaceStateDirectory(id))).mode & 0o777).toBe(0o700);
       expect((await stat(join(workspaceStateDirectory(id), 'state.json'))).mode & 0o777).toBe(0o600);
       expect((await stat(join(workspaceStateDirectory(id), 'secrets', 'endpoint-token'))).mode & 0o777).toBe(0o600);

src/windows-acl.js

@@ -0,0 +1,127 @@
+import { execFile } from 'node:child_process';
+import { promisify } from 'node:util';
+import { StateStoreError } from './errors.js';
+
+/**
+ * Setting an access list is an operating-system primitive, not a provider command, so it
+ * runs through `child_process` rather than the shared runner. The runner carries argument
+ * redaction and provider attribution that mean nothing here, and — the reason that
+ * matters — provider tests replace it wholesale, which would leave the state store unable
+ * to write a file in any suite that mocks a container CLI.
+ */
+const exec = promisify(execFile);
+
+/**
+ * The state store declares `0o700` directories and `0o600` files, and Windows implements
+ * neither — `chmod` is close to a no-op there and every file reports `0o666`. Under the
+ * default data directory the store is nonetheless private, because it inherits the
+ * profile's permissions; move it to a second drive, a shared folder, or anywhere else an
+ * operator points `OPENCHAMBER_WORKSPACE_STATE_DIR` and it inherits that location's
+ * instead. What the store holds is container endpoint tokens, so the protection has to be
+ * stated rather than inherited.
+ *
+ * Windows states it with an ACL, which Node cannot set, so this shells out to `icacls`
+ * once per state root: inheritance is removed and full control granted to this account,
+ * SYSTEM, and Administrators — the last two by SID, because their names are localised and
+ * `Administrators` does not exist on a Ukrainian or German install.
+ */
+
+const SYSTEM_SID = '*S-1-5-18';
+const ADMINISTRATORS_SID = '*S-1-5-32-544';
+
+const protectedRoots = new Set();
+let accountSid;
+
+/**
+ * Both of these are named by absolute path rather than looked up on PATH. Git for Windows
+ * ships a POSIX `whoami` that takes no `/user`, and it usually comes first — the same way
+ * its `tar` shadows the system one and misreads `C:\…` as a remote host. Whichever tool
+ * answers should not depend on what else happens to be installed.
+ */
+function system32(binary) {
+  return `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\${binary}`;
+}
+
+export function resetWindowsAclCache() {
+  protectedRoots.clear();
+  accountSid = undefined;
+}
+
+/** Parses `whoami /user /fo csv /nh`, which prints `"DOMAIN\user","S-1-5-21-…"`. */
+export function parseAccountSid(output) {
+  const match = /S-1-[0-9-]+/.exec(String(output ?? ''));
+  return match ? match[0] : null;
+}
+
+async function currentAccountSid() {
+  if (accountSid) return accountSid;
+  let result;
+  try {
+    result = await exec(system32('whoami.exe'), ['/user', '/fo', 'csv', '/nh'], { timeout: 15_000, windowsHide: true });
+  } catch (error) {
+    throw new StateStoreError('Unable to identify the current Windows account, so workspace state cannot be restricted to it.', { cause: error });
+  }
+  const sid = parseAccountSid(result.stdout);
+  if (!sid) throw new StateStoreError('Windows reported no security identifier for the current account, so workspace state cannot be restricted to it.');
+  accountSid = sid;
+  return sid;
+}
+
+/**
+ * Lists the principals holding an entry on a path. `icacls` puts the path and the first
+ * entry on one line and indents the rest:
+ *
+ *     C:\…\workspaces Everyone:(OI)(CI)(F)
+ *                     NT AUTHORITY\SYSTEM:(I)(OI)(CI)(F)
+ *                     DESKTOP-CP9J2I4\Bohdan Triapitsyn:(I)(OI)(CI)(F)
+ *
+ * The path has to come off first: it contains the drive's colon, and a principal may
+ * contain spaces, so there is no separator that tells them apart on that first line.
+ */
+export function parseGrantedPrincipals(output, path) {
+  const principals = [];
+  const prefix = String(path ?? '');
+  for (const raw of String(output ?? '').split(/\r?\n/)) {
+    const line = prefix && raw.toLowerCase().startsWith(prefix.toLowerCase()) ? raw.slice(prefix.length) : raw;
+    const match = /^\s*(\S.*?):\([A-Z]+\)/.exec(line);
+    if (match) principals.push(match[1].trim());
+  }
+  return principals;
+}
+
+/**
+ * Restricts a directory and everything created beneath it to this account. Children
+ * inherit the entries, so this runs once per root rather than once per write.
+ *
+ * Neither switch does the whole job on its own. `/inheritance:r` drops what the parent
+ * contributed, and `/grant:r` replaces the permissions of the principals it names — but
+ * an entry someone added explicitly for a third party is touched by neither and simply
+ * stays. So the list is emptied first and rebuilt second, rather than granting over
+ * whatever was already there; the owner may always rewrite its own list, which is why the
+ * directory does not lock this process out in between.
+ */
+export async function protectDirectoryForCurrentUser(path, { platform = process.platform } = {}) {
+  if (platform !== 'win32') return false;
+  if (protectedRoots.has(path)) return false;
+  const sid = await currentAccountSid();
+  const icacls = system32('icacls.exe');
+  try {
+    const options = { timeout: 30_000, windowsHide: true };
+    await exec(icacls, [path, '/inheritance:r', '/q'], options);
+    const { stdout } = await exec(icacls, [path], options);
+    for (const principal of parseGrantedPrincipals(stdout, path)) {
+      await exec(icacls, [path, '/remove:g', principal, '/q'], options);
+    }
+    await exec(icacls, [
+      path,
+      '/grant:r', `*${sid}:(OI)(CI)F`,
+      '/grant:r', `${SYSTEM_SID}:(OI)(CI)F`,
+      '/grant:r', `${ADMINISTRATORS_SID}:(OI)(CI)F`,
+      '/q',
+    ], options);
+  } catch (error) {
+    throw new StateStoreError(`Unable to restrict workspace state to the current account: ${path}`, { cause: error });
+  }
+  protectedRoots.add(path);
+  return true;
+}

src/windows-acl.test.js

@@ -0,0 +1,90 @@
+import { afterEach, describe, expect, it } from 'vitest';
+import { mkdtemp, rm, writeFile } from 'node:fs/promises';
+import { tmpdir } from 'node:os';
+import { join } from 'node:path';
+import { parseAccountSid, parseGrantedPrincipals, protectDirectoryForCurrentUser, resetWindowsAclCache } from './windows-acl.js';
+import { run } from './process.js';
+
+const icacls = `${process.env.SystemRoot || 'C:\\Windows'}\\System32\\icacls.exe`;
+
+describe('windows state protection', () => {
+  const created = [];
+
+  /** The principal lines of an access list, joined so a test can match across them. */
+  async function granted(path) {
+    const { stdout } = await run(icacls, [path]);
+    return stdout.split(/\r?\n/).filter((line) => line.includes(':(')).join('\n');
+  }
+
+  /** A directory anyone on the machine can read, so removing that grant is observable. */
+  async function openDirectory() {
+    const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-'));
+    created.push(directory);
+    await run(icacls, [directory, '/grant', '*S-1-1-0:(OI)(CI)F', '/q']);
+    return directory;
+  }
+  afterEach(async () => {
+    resetWindowsAclCache();
+    await Promise.all(created.splice(0).map((path) => rm(path, { recursive: true, force: true })));
+  });
+
+  it('reads every principal out of a list, including one whose name has a space', () => {
+    const path = 'C:\\Users\\BOHDAN~1\\AppData\\Local\\Temp\\acl-probe';
+    const output = [
+      `${path} Everyone:(OI)(CI)(F)`,
+      '                                             NT AUTHORITY\\SYSTEM:(I)(OI)(CI)(F)',
+      '                                             BUILTIN\\Administrators:(I)(OI)(CI)(F)',
+      '                                             DESKTOP-CP9J2I4\\Bohdan Triapitsyn:(I)(OI)(CI)(F)',
+      '',
+      'Successfully processed 1 files; Failed processing 0 files',
+    ].join('\r\n');
+    expect(parseGrantedPrincipals(output, path)).toEqual([
+      'Everyone',
+      'NT AUTHORITY\\SYSTEM',
+      'BUILTIN\\Administrators',
+      'DESKTOP-CP9J2I4\\Bohdan Triapitsyn',
+    ]);
+  });
+
+  it('reads the account identifier out of what whoami prints', () => {
+    expect(parseAccountSid('"CORP\\\\yulia","S-1-5-21-1004336348-1177238915-682003330-512"\r\n'))
+      .toBe('S-1-5-21-1004336348-1177238915-682003330-512');
+    expect(parseAccountSid('')).toBeNull();
+  });
+
+  it('does nothing where POSIX modes already restrict the store', async () => {
+    const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-'));
+    created.push(directory);
+    expect(await protectDirectoryForCurrentUser(directory, { platform: 'linux' })).toBe(false);
+  });
+
+  it.runIf(process.platform === 'win32')('removes a grant that was there before it, not only inherited ones', async () => {
+    const directory = await openDirectory();
+    // %TEMP% is already private on a normal profile, so a directory made there cannot
+    // show whether anything was actually taken away. This one is opened to Everyone
+    // first — an explicit entry, which `/grant:r` alone would leave untouched.
+    expect(await granted(directory)).toMatch(/Everyone/i);
+
+    expect(await protectDirectoryForCurrentUser(directory)).toBe(true);
+
+    expect(await granted(directory)).not.toMatch(/Everyone/i);
+    expect(await granted(directory)).toContain(process.env.USERNAME);
+  });
+
+  it.runIf(process.platform === 'win32')('passes the restriction down to files created afterwards', async () => {
+    const directory = await openDirectory();
+    await protectDirectoryForCurrentUser(directory);
+    const secret = join(directory, 'endpoint-token');
+    await writeFile(secret, 'secret');
+
+    expect(await granted(secret)).not.toMatch(/Everyone/i);
+    expect(await granted(secret)).toContain(process.env.USERNAME);
+  });
+
+  it.runIf(process.platform === 'win32')('asks the operating system once per root, not once per write', async () => {
+    const directory = await mkdtemp(join(tmpdir(), 'workspace-acl-test-'));
+    created.push(directory);
+    expect(await protectDirectoryForCurrentUser(directory)).toBe(true);
+    expect(await protectDirectoryForCurrentUser(directory)).toBe(false);
+  });
+});