Re: [PR] fix(runtime): allow bounded Apple Git startup [maka]

["Astro-Han (via GitHub)" <gi...@apache.org>]

Astro-Han commented on code in PR #5266:
URL: https://github.com/apache/maka/pull/5266#discussion_r4175351321


##########
packages/runtime/src/sandbox/macos-command-paths.ts:
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { execFile } from 'node:child_process';
+import { existsSync, realpathSync, statSync } from 'node:fs';
+import { homedir } from 'node:os';
+import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path';
+
+import { isReadOnlyPermissionProfile, type PermissionProfile } from '@maka/core/permission-profile';
+
+const XCODE_SELECT_TIMEOUT_MS = 1_000;
+const CODESIGN_TIMEOUT_MS = 1_000;
+
+interface MacosDeveloperCommandResult {
+  status: number | null;
+  stdout?: string;
+}
+
+export type MacosDeveloperCommandRunner = (
+  executable: string,
+  args: readonly string[],
+  options: {
+    encoding?: 'utf8';
+    timeout: number;
+    stdio: ['ignore', 'pipe', 'ignore'] | 'ignore';
+  },
+) => MacosDeveloperCommandResult | Promise<MacosDeveloperCommandResult>;
+
+export interface MacosCommandPaths {
+  executableRoots: readonly string[];
+}
+
+export interface MacosDeveloperPathOptions {
+  developerDir?: string;
+  homeDir?: string;
+  selectDeveloperDir?: () => string | undefined | Promise<string | undefined>;
+  validateAppleBinary?: (path: string) => boolean | Promise<boolean>;
+  runCommand?: MacosDeveloperCommandRunner;
+}
+
+/** Resolve only the dynamic-library directories used by the selected Apple toolchain. */
+export async function resolveMacosDeveloperExecutableRoots(
+  options: MacosDeveloperPathOptions = {},
+): Promise<readonly string[]> {
+  const runCommand = options.runCommand ?? runDeveloperCommand;
+  const selected =
+    options.developerDir?.trim() ||
+    (await (options.selectDeveloperDir ?? (() => readSelectedDeveloperDirectory(runCommand)))());
+  if (!selected || !isAbsolute(selected)) return [];
+
+  let developerRoot: string;
+  try {
+    developerRoot = realpathSync(selected);
+  } catch {
+    return [];
+  }
+
+  const homeRoot = canonicalDirectory(options.homeDir ?? homedir());
+  if (developerRoot === '/' || (homeRoot && isPathWithin(developerRoot, homeRoot))) return [];
+
+  const libraryRoot = canonicalDirectory(join(developerRoot, 'usr', 'lib'));
+  if (!libraryRoot || !isPathWithin(libraryRoot, developerRoot)) return [];
+  const xcrunLibrary = canonicalRegularFile(join(libraryRoot, 'libxcrun.dylib'));
+  if (!xcrunLibrary || !isPathWithin(xcrunLibrary, libraryRoot)) return [];
+  if (
+    !(await (options.validateAppleBinary ?? ((path) => validateAppleBinary(path, runCommand)))(
+      xcrunLibrary,
+    ))
+  ) {
+    return [];
+  }
+
+  if (basename(developerRoot) === 'CommandLineTools') return [libraryRoot];

Review Comment:
   P2 (medium confidence, unverified on macOS). The only new grant is `file-read*`/`file-map-executable` on `<developer>/usr/lib` (plus `SharedFrameworks` for Xcode). That is enough for the `/usr/bin/git` shim to load `libxcrun.dylib`. But xcrun then execs `<developer>/usr/bin/git`. Under workspace-write, Seatbelt reads are limited to readable roots (`macos-seatbelt.ts:463-480`), plus system defaults (`/usr`, `/System`, `/bin`, `/etc`, ...; `:41-47`, `:51-61`). Nothing covers `<developer>/usr/bin`, `usr/libexec/git-core` (helpers), or `usr/share/git-core`. Apple Git's system config (`.../usr/share/git-core/gitconfig`, which sets `credential.helper=osxkeychain`) and the `git init` templates live there. Git treats an unreadable config file as fatal (`unknown error occurred while reading the configuration files`), so `git status` could still fail one step after this fix. CI has no macOS runner, and every reported local smoke run hit `sandbox_apply` exit 71. So `starts Apple Git with the
  selected Apple toolchain` (`macos-seatbelt-smoke.test.ts:160`) has never passed anywhere. Please run that smoke on a real Mac, not inside another sandbox, for both CLT and Xcode. If it fails, add read-only (not `file-map-executable`) roots for `usr/bin`, `usr/libexec/git-core` and `usr/share/git-core`. If it passes, please post the output so the PR's core claim is backed by evidence.



##########
packages/runtime/src/sandbox/macos-command-paths.ts:
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { execFile } from 'node:child_process';
+import { existsSync, realpathSync, statSync } from 'node:fs';
+import { homedir } from 'node:os';
+import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path';
+
+import { isReadOnlyPermissionProfile, type PermissionProfile } from '@maka/core/permission-profile';
+
+const XCODE_SELECT_TIMEOUT_MS = 1_000;
+const CODESIGN_TIMEOUT_MS = 1_000;
+
+interface MacosDeveloperCommandResult {
+  status: number | null;
+  stdout?: string;
+}
+
+export type MacosDeveloperCommandRunner = (
+  executable: string,
+  args: readonly string[],
+  options: {
+    encoding?: 'utf8';
+    timeout: number;
+    stdio: ['ignore', 'pipe', 'ignore'] | 'ignore';
+  },
+) => MacosDeveloperCommandResult | Promise<MacosDeveloperCommandResult>;
+
+export interface MacosCommandPaths {
+  executableRoots: readonly string[];
+}
+
+export interface MacosDeveloperPathOptions {
+  developerDir?: string;
+  homeDir?: string;
+  selectDeveloperDir?: () => string | undefined | Promise<string | undefined>;
+  validateAppleBinary?: (path: string) => boolean | Promise<boolean>;
+  runCommand?: MacosDeveloperCommandRunner;
+}
+
+/** Resolve only the dynamic-library directories used by the selected Apple toolchain. */
+export async function resolveMacosDeveloperExecutableRoots(
+  options: MacosDeveloperPathOptions = {},
+): Promise<readonly string[]> {
+  const runCommand = options.runCommand ?? runDeveloperCommand;
+  const selected =
+    options.developerDir?.trim() ||
+    (await (options.selectDeveloperDir ?? (() => readSelectedDeveloperDirectory(runCommand)))());
+  if (!selected || !isAbsolute(selected)) return [];
+
+  let developerRoot: string;
+  try {
+    developerRoot = realpathSync(selected);
+  } catch {
+    return [];
+  }
+
+  const homeRoot = canonicalDirectory(options.homeDir ?? homedir());
+  if (developerRoot === '/' || (homeRoot && isPathWithin(developerRoot, homeRoot))) return [];
+
+  const libraryRoot = canonicalDirectory(join(developerRoot, 'usr', 'lib'));
+  if (!libraryRoot || !isPathWithin(libraryRoot, developerRoot)) return [];
+  const xcrunLibrary = canonicalRegularFile(join(libraryRoot, 'libxcrun.dylib'));
+  if (!xcrunLibrary || !isPathWithin(xcrunLibrary, libraryRoot)) return [];
+  if (
+    !(await (options.validateAppleBinary ?? ((path) => validateAppleBinary(path, runCommand)))(
+      xcrunLibrary,
+    ))
+  ) {
+    return [];
+  }
+
+  if (basename(developerRoot) === 'CommandLineTools') return [libraryRoot];
+
+  if (basename(developerRoot) !== 'Developer' || basename(dirname(developerRoot)) !== 'Contents') {

Review Comment:
   P3. A `DEVELOPER_DIR` that points at the bundle root (`/Applications/Xcode.app`, which `xcode-select -s` also accepts) fails at `:77` because `usr/lib` is missing, so no roots are granted. It fails closed, so this is not a regression, but Git stays broken for that setup and nothing tells the user why. Consider normalising `*.app` to `*.app/Contents/Developer` before the layout checks, or at least logging the rejected path.



##########
packages/runtime/src/sandbox/macos-command-paths.ts:
##########
@@ -0,0 +1,183 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+import { execFile } from 'node:child_process';
+import { existsSync, realpathSync, statSync } from 'node:fs';
+import { homedir } from 'node:os';
+import { basename, dirname, isAbsolute, join, relative, sep } from 'node:path';
+
+import { isReadOnlyPermissionProfile, type PermissionProfile } from '@maka/core/permission-profile';
+
+const XCODE_SELECT_TIMEOUT_MS = 1_000;
+const CODESIGN_TIMEOUT_MS = 1_000;
+
+interface MacosDeveloperCommandResult {
+  status: number | null;
+  stdout?: string;
+}
+
+export type MacosDeveloperCommandRunner = (
+  executable: string,
+  args: readonly string[],
+  options: {
+    encoding?: 'utf8';
+    timeout: number;
+    stdio: ['ignore', 'pipe', 'ignore'] | 'ignore';
+  },
+) => MacosDeveloperCommandResult | Promise<MacosDeveloperCommandResult>;
+
+export interface MacosCommandPaths {
+  executableRoots: readonly string[];
+}
+
+export interface MacosDeveloperPathOptions {
+  developerDir?: string;
+  homeDir?: string;
+  selectDeveloperDir?: () => string | undefined | Promise<string | undefined>;
+  validateAppleBinary?: (path: string) => boolean | Promise<boolean>;
+  runCommand?: MacosDeveloperCommandRunner;
+}
+
+/** Resolve only the dynamic-library directories used by the selected Apple toolchain. */
+export async function resolveMacosDeveloperExecutableRoots(
+  options: MacosDeveloperPathOptions = {},
+): Promise<readonly string[]> {
+  const runCommand = options.runCommand ?? runDeveloperCommand;
+  const selected =
+    options.developerDir?.trim() ||
+    (await (options.selectDeveloperDir ?? (() => readSelectedDeveloperDirectory(runCommand)))());
+  if (!selected || !isAbsolute(selected)) return [];
+
+  let developerRoot: string;
+  try {
+    developerRoot = realpathSync(selected);
+  } catch {
+    return [];
+  }
+
+  const homeRoot = canonicalDirectory(options.homeDir ?? homedir());
+  if (developerRoot === '/' || (homeRoot && isPathWithin(developerRoot, homeRoot))) return [];
+
+  const libraryRoot = canonicalDirectory(join(developerRoot, 'usr', 'lib'));
+  if (!libraryRoot || !isPathWithin(libraryRoot, developerRoot)) return [];
+  const xcrunLibrary = canonicalRegularFile(join(libraryRoot, 'libxcrun.dylib'));
+  if (!xcrunLibrary || !isPathWithin(xcrunLibrary, libraryRoot)) return [];
+  if (
+    !(await (options.validateAppleBinary ?? ((path) => validateAppleBinary(path, runCommand)))(
+      xcrunLibrary,
+    ))
+  ) {
+    return [];
+  }
+
+  if (basename(developerRoot) === 'CommandLineTools') return [libraryRoot];
+
+  if (basename(developerRoot) !== 'Developer' || basename(dirname(developerRoot)) !== 'Contents') {
+    return [];
+  }
+
+  const contentsRoot = dirname(developerRoot);
+  const sharedFrameworks = canonicalDirectory(join(contentsRoot, 'SharedFrameworks'));
+  if (!sharedFrameworks || !isPathWithin(sharedFrameworks, contentsRoot)) return [];
+  return [libraryRoot, sharedFrameworks];
+}
+
+export async function resolveMacosCommandPaths(
+  profile: PermissionProfile,
+  env: Readonly<Record<string, string | undefined>>,
+  options: Omit<MacosDeveloperPathOptions, 'developerDir' | 'homeDir'> = {},
+): Promise<MacosCommandPaths> {
+  // Runtime roots are an implementation allowance for writable command
+  // sessions. They must not silently widen a restricted read-only profile.
+  if (profile.type === 'managed' && isReadOnlyPermissionProfile(profile)) {
+    return { executableRoots: [] };
+  }
+  return {
+    executableRoots: await resolveMacosDeveloperExecutableRoots({
+      developerDir: env.DEVELOPER_DIR,
+      homeDir: env.HOME,
+      ...options,
+    }),
+  };
+}
+
+async function readSelectedDeveloperDirectory(
+  runCommand: MacosDeveloperCommandRunner,
+): Promise<string | undefined> {
+  const result = await runCommand('/usr/bin/xcode-select', ['-p'], {
+    encoding: 'utf8',
+    timeout: XCODE_SELECT_TIMEOUT_MS,
+    stdio: ['ignore', 'pipe', 'ignore'],
+  });
+  return result.status === 0 ? result.stdout?.trim() || undefined : undefined;
+}
+
+async function validateAppleBinary(
+  path: string,
+  runCommand: MacosDeveloperCommandRunner,
+): Promise<boolean> {
+  const result = await runCommand(
+    '/usr/bin/codesign',
+    ['--verify', '--strict', '-R=anchor apple', path],
+    {
+      timeout: CODESIGN_TIMEOUT_MS,
+      stdio: 'ignore',
+    },
+  );
+  return result.status === 0;
+}
+
+const runDeveloperCommand: MacosDeveloperCommandRunner = (executable, args, options) =>
+  new Promise((resolve) => {
+    execFile(
+      executable,
+      [...args],
+      { encoding: 'utf8', timeout: options.timeout, maxBuffer: 64 * 1024 },

Review Comment:
   P3. `execFile`'s `timeout` sends `killSignal` (default `SIGTERM`), and the callback only fires on `close`. So the 1s bound holds only if the child exits on SIGTERM. Pass `killSignal: 'SIGKILL'` to make the bound unconditional. Two smaller points: the probes don't receive the tool's `ctx.abortSignal`, so cancelling a command still waits up to about 2s. The runner's `stdio` option is accepted by the type (`:41`) but ignored here, so codesign's stderr is buffered into `maxBuffer` rather than discarded. That is harmless, but the contract is misleading.



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@maka.apache.org
For additional commands, e-mail: commits-help@maka.apache.org