-
Notifications
You must be signed in to change notification settings - Fork 81
fix(engine): probe real containment in platform_support() #752
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.
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
Open
Carlos Alexandro Becker (caarlos0)
wants to merge
8
commits into
microsoft:main
Choose a base branch
from
caarlos0:plat-support-improments
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
Show all changes
8 commits
Select commit
Hold shift + click to select a range
555ff29
fix(engine): probe real containment in platform_support()
caarlos0 3a352a0
Merge branch 'main' into plat-support-improments
caarlos0 63c16af
fix(sdk): honor experimental backends in the shared platform gate
caarlos0 a63524e
style(sdk): restore CRLF line endings in platform.ts
caarlos0 82d3197
fix(bwrap): collect probe output via files so the deadline actually hβ¦
caarlos0 c002170
fix(bwrap): cap the probe output read
caarlos0 a00b428
fix(bwrap): reap the timed-out probe without blocking
caarlos0 a1f38e1
Merge remote-tracking branch 'origin/main' into plat-support-improvemβ¦
caarlos0 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -24,6 +24,77 @@ function getSdkPackageRoot(): string { | |
| } | ||
| } | ||
|
|
||
| /** | ||
| * Query Windows Registry for a value | ||
| * @param key - Registry key path (e.g., "HKLM\\Software\\...") | ||
| * @param valueName - Name of the value to query | ||
| * @returns The registry value as a string, or null if not found | ||
| */ | ||
| function queryWindowsRegistry(key: string, valueName: string): string | null { | ||
| try { | ||
| const command = `reg query "${key}" /v "${valueName}"`; | ||
| const output = execSync(command, { encoding: 'utf-8', stdio: 'pipe' }); | ||
|
|
||
| // Parse output - format is: | ||
| // HKEY_LOCAL_MACHINE\... | ||
| // ValueName REG_SZ Value | ||
| const lines = output.split('\n'); | ||
| for (const line of lines) { | ||
| if (line.includes(valueName)) { | ||
| // Extract value after REG_SZ or REG_DWORD | ||
| const match = line.match(/REG_\w+\s+(.+)/); | ||
| if (match) { | ||
| return match[1].trim(); | ||
| } | ||
| } | ||
| } | ||
| return null; | ||
| } catch { | ||
| return null; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * Result of querying the host's Windows build number, or `null` when the | ||
| * registry value is missing or unparseable. | ||
| */ | ||
| type WindowsBuild = { major: number } | null; | ||
|
|
||
| /** | ||
| * Default implementation that reads `CurrentBuild` from the registry. | ||
| * Replaceable via {@link _setWindowsBuildQuery} in tests so we can exercise | ||
| * the `processcontainer` build floor deterministically. | ||
| */ | ||
| function defaultWindowsBuildQuery(): WindowsBuild { | ||
| const registryPath = 'HKLM\\Software\\Microsoft\\Windows NT\\CurrentVersion'; | ||
| const currentBuild = queryWindowsRegistry(registryPath, 'CurrentBuild'); | ||
| if (!currentBuild) { | ||
| return null; | ||
| } | ||
| const major = parseInt(currentBuild, 10); | ||
| if (isNaN(major)) { | ||
| return null; | ||
| } | ||
| return { major }; | ||
| } | ||
|
|
||
| let windowsBuildQuery: () => WindowsBuild = defaultWindowsBuildQuery; | ||
|
|
||
| /** @internal Test-only: override the Windows build lookup. */ | ||
| export function _setWindowsBuildQuery(fn: (() => WindowsBuild) | null): void { | ||
| windowsBuildQuery = fn ?? defaultWindowsBuildQuery; | ||
| } | ||
|
|
||
| /** | ||
| * Minimum Windows build the `processcontainer` backend supports β 26100 | ||
| * (Windows 11 24H2). This is the product floor documented in the README and in | ||
| * `docs/process-container/os-version-support.md`. | ||
| * | ||
| * Mirrors `MIN_WINDOWS_BUILD` in `src/core/mxc_engine/src/platform.rs` β keep | ||
| * both in sync. | ||
| */ | ||
| const MIN_PROCESSCONTAINER_BUILD = 26100; | ||
|
|
||
| let windowsSandboxAvailableCache: boolean | undefined; | ||
|
|
||
| /** | ||
|
|
@@ -222,12 +293,40 @@ function computeSupport(): PlatformSupport { | |
| return support; | ||
| } | ||
|
|
||
| support.isSupported = true; | ||
| support.availableMethods = ['processcontainer']; | ||
| // The host build is the real gate on Windows: below the product floor | ||
| // `processcontainer` fails at spawn rather than at detection. An unreadable | ||
| // registry leaves the build unknown, which is treated as modern so a | ||
| // detection failure never declares a supported host unsupported. | ||
| const build = windowsBuildQuery(); | ||
| const methods: ContainmentBackend[] = []; | ||
| if (!build || build.major >= MIN_PROCESSCONTAINER_BUILD) { | ||
| methods.push('processcontainer'); | ||
| } | ||
| // Windows Sandbox has its own, lower floor, so a host below the | ||
| // processcontainer floor may still have it. Both it and IsolationSession are | ||
| // reported when present, but they are experimental-only backends reached by | ||
| // explicit opt-in, so they cannot carry `isSupported` β that flag is what | ||
| // guards the default `processcontainer` spawn. | ||
| if (isWindowsSandboxAvailable()) { | ||
| support.availableMethods.push('windows_sandbox'); | ||
| methods.push('windows_sandbox'); | ||
| } | ||
| support.availableMethods = methods; | ||
| // Runs before the verdict below so `isolation_session`, which only the probe | ||
| // can report, is counted among the alternatives on a below-floor host. | ||
| populateIsolationFromProbe(support); | ||
|
|
||
| if (!support.availableMethods.includes('processcontainer')) { | ||
| const alternatives = | ||
| support.availableMethods.length > 0 | ||
| ? ` (experimental backends available: ${support.availableMethods.join(', ')})` | ||
| : ''; | ||
| support.reason = | ||
| `Windows build ${build?.major} is below ${MIN_PROCESSCONTAINER_BUILD}, ` + | ||
| `the minimum supported build (Windows 11 24H2)${alternatives}`; | ||
| return support; | ||
| } | ||
|
|
||
| support.isSupported = true; | ||
| return support; | ||
| } | ||
|
|
||
|
|
@@ -462,9 +561,116 @@ export function _probeBubblewrap(): BubblewrapProbe { | |
| reason: `Bubblewrap (bwrap) ${version.join('.')} is too old; version ${minVersion} or newer is required`, | ||
| }; | ||
| } | ||
| // A new enough `bwrap` still cannot sandbox if the host forbids it, and | ||
| // `--version` never creates a namespace, so ask it to build a real one. | ||
| const sandbox = bwrapSandboxRunner(); | ||
| if (!sandbox.ok) { | ||
| return { | ||
| available: false, | ||
| reason: `Bubblewrap (bwrap) ${version.join('.')} is installed but cannot create a sandbox on this host: ${sandbox.detail}`, | ||
| }; | ||
| } | ||
| return { available: true }; | ||
| } | ||
|
|
||
| /** | ||
| * Arguments for a minimal end-to-end containment probe. | ||
| * | ||
| * `bwrap --version` only prints a banner β it never creates a namespace β so | ||
| * it passes on hosts where unprivileged user namespaces are disabled | ||
| * (`kernel.unprivileged_userns_clone=0`) or where AppArmor denies `bwrap` | ||
| * (Ubuntu 23.10+), both of which then fail at every spawn. | ||
| * | ||
| * The shape mirrors a real run: the same namespaces the Bubblewrap backend | ||
| * unshares, plus `--proc` / `--dev`, and `--clearenv` so the payload is | ||
| * resolved through `execvp`'s built-in `/bin:/usr/bin` default rather than the | ||
| * caller's `PATH`. Binds use `--ro-bind-try` on the few directories a shell | ||
| * needs β binding `/` instead would make the probe fail on any host with an | ||
| * awkward submount, since `bwrap` treats a failed submount remount as fatal. | ||
| * | ||
| * Kept in step with the engine's `BWRAP_PROBE_ARGS` | ||
| * (`src/core/mxc_engine/src/platform.rs`), which is pinned against the | ||
| * production argument builder by a unit test. | ||
| */ | ||
| const BWRAP_PROBE_ARGS = [ | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. suggestion (non-blocking): The Rust vector is pinned against production arguments, but this TypeScript copy is protected only by a βkeep in stepβ comment. A future namespace or mount change can silently make Node detection weaker than actual execution. |
||
| '--unshare-user', | ||
| '--unshare-pid', | ||
| '--unshare-ipc', | ||
| '--unshare-uts', | ||
| '--unshare-net', | ||
| '--ro-bind-try', | ||
| '/bin', | ||
| '/bin', | ||
| '--ro-bind-try', | ||
| '/usr/bin', | ||
| '/usr/bin', | ||
| '--ro-bind-try', | ||
| '/lib', | ||
| '/lib', | ||
| '--ro-bind-try', | ||
| '/lib64', | ||
| '/lib64', | ||
| '--ro-bind-try', | ||
| '/usr/lib', | ||
| '/usr/lib', | ||
| '--ro-bind-try', | ||
| '/usr/lib64', | ||
| '/usr/lib64', | ||
| '--proc', | ||
| '/proc', | ||
| '--dev', | ||
| '/dev', | ||
| '--clearenv', | ||
| '--', | ||
| 'sh', | ||
| '-c', | ||
| 'exit 0', | ||
| ]; | ||
|
|
||
| /** Outcome of the sandbox probe; `detail` is empty when `ok`. */ | ||
| export type BubblewrapSandboxProbe = { ok: boolean; detail: string }; | ||
|
|
||
| /** | ||
| * Run {@link BWRAP_PROBE_ARGS}, reporting bwrap's own diagnostic on failure. | ||
| * | ||
| * Replaceable in unit tests via {@link _setBwrapSandboxRunner}, so the | ||
| * version-gate tests can drive `_probeBubblewrap` on a host without `bwrap`. | ||
| */ | ||
| function defaultBwrapSandboxRunner(): BubblewrapSandboxProbe { | ||
| try { | ||
| execFileSync('bwrap', BWRAP_PROBE_ARGS, { | ||
| stdio: ['ignore', 'ignore', 'pipe'], | ||
| timeout: BWRAP_VERSION_TIMEOUT_MS, | ||
|
caarlos0 marked this conversation as resolved.
|
||
| }); | ||
| return { ok: true, detail: '' }; | ||
| } catch (error) { | ||
| return { ok: false, detail: bwrapFailureDetail(error) }; | ||
| } | ||
| } | ||
|
|
||
| let bwrapSandboxRunner: () => BubblewrapSandboxProbe = defaultBwrapSandboxRunner; | ||
|
|
||
| /** @internal Test-only: override the Bubblewrap sandbox probe. */ | ||
| export function _setBwrapSandboxRunner(fn: (() => BubblewrapSandboxProbe) | null): void { | ||
| bwrapSandboxRunner = fn ?? defaultBwrapSandboxRunner; | ||
| } | ||
|
|
||
| /** Reduce a failed bwrap run to a single length-capped line for a `reason`. */ | ||
| function bwrapFailureDetail(error: unknown): string { | ||
| const MAX_LEN = 200; | ||
| const { stderr } = (error ?? {}) as { stderr?: Buffer | string }; | ||
| const line = (stderr?.toString() ?? '') | ||
| .split('\n') | ||
| .map((l) => l.trim()) | ||
| .find((l) => l.length > 0); | ||
| if (!line) { | ||
| return 'it failed with no diagnostic output'; | ||
| } | ||
| // Spread so the cap counts code points and never splits a surrogate pair. | ||
| const chars = [...line]; | ||
| return chars.length > MAX_LEN ? `${chars.slice(0, MAX_LEN).join('')}β¦` : line; | ||
| } | ||
|
|
||
| /** | ||
| * Check if the macOS sandbox is available. `/usr/bin/sandbox-exec` is part | ||
| * of the macOS base install and present on every shipping version of macOS, | ||
|
|
||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (non-blocking, tests): This parameter threading repairs the case where the shared platform check rejects an explicitly selected experimental backend on a below-26100 host. Current tests validate platform output and state-aware envelopes separately, but do not execute a state-aware phase through this resolution path.
Could we add a build-22631 test using a fake executor that calls
provisionSandboxorstartSandboxfor an experimental backend and verifies resolution does not throw the default platform-support error?