diff --git a/backend/src/auth/diagnostic-command.ts b/backend/src/auth/diagnostic-command.ts index 75bd1fee..47f9f4ee 100644 --- a/backend/src/auth/diagnostic-command.ts +++ b/backend/src/auth/diagnostic-command.ts @@ -1,7 +1,10 @@ import { fileURLToPath } from "node:url"; import { resolve } from "node:path"; +import { + closeSync, constants, fstatSync, lstatSync, openSync, readFileSync, +} from "node:fs"; import { loadConfig, type AppConfig } from "../config.js"; -import { loadSecretBundle, SECRET_BUNDLE_KEYS, secretValue } from "../config/secret-bundle.js"; +import { loadSecretBundle, secretValue } from "../config/secret-bundle.js"; import { createAuthentikGroupCatalog } from "./authentik-group-catalog.js"; import { createCurrentLocalUserRegistryResolver, type LocalUserRegistry } from "./local-registry.js"; import { createOidcProtocol, OidcDeviceFlowUnavailableError, type OidcProtocol } from "./oidc-client.js"; @@ -10,27 +13,96 @@ import { decodeAuthDiagnostics, type GroupCatalog } from "./group-catalog.js"; import type { LoadedAuthConfig } from "./types.js"; const AUTH_SECRET_REFERENCES = ["THT_OIDC_CLIENT_SECRET", "THT_AUTHENTIK_API_TOKEN"] as const; +const MAX_DIAGNOSTIC_SECRET_SOURCE_BYTES = 64 * 1024; +const MAX_DIAGNOSTIC_SECRET_VALUES = 4096; +const MAX_DIAGNOSTIC_SECRET_DEPTH = 32; -export function configuredSecretValues(config: AppConfig): readonly string[] { - const values = new Set(); - if (config.secretsFile) { - try { - for (const value of loadSecretBundle(config.secretsFile).values()) values.add(value); - } catch { - return []; +function unavailableSecretCorpus(): Error { + return new Error("diagnostic secret corpus is unavailable"); +} + +function readMountedSecretSource(file: string): string { + let fd: number | undefined; + try { + if (!file || file.trim() !== file || file.includes("\0")) throw unavailableSecretCorpus(); + const before = lstatSync(file); + if (!before.isFile() || before.isSymbolicLink() || before.size > MAX_DIAGNOSTIC_SECRET_SOURCE_BYTES) { + throw unavailableSecretCorpus(); } + fd = openSync(file, constants.O_RDONLY | constants.O_NOFOLLOW); + const opened = fstatSync(fd); + if (!opened.isFile() || opened.size > MAX_DIAGNOSTIC_SECRET_SOURCE_BYTES + || before.dev !== opened.dev || before.ino !== opened.ino) { + throw unavailableSecretCorpus(); + } + const value = readFileSync(fd, "utf8"); + if (Buffer.byteLength(value, "utf8") > MAX_DIAGNOSTIC_SECRET_SOURCE_BYTES) { + throw unavailableSecretCorpus(); + } + return value; + } catch { + throw unavailableSecretCorpus(); + } finally { + if (fd !== undefined) try { closeSync(fd); } catch { /* fixed failure surface above */ } } - for (const reference of SECRET_BUNDLE_KEYS) { - try { - const value = secretValue({ secretFiles: config.secretFiles }, reference); - if (value !== undefined) values.add(value); - } catch { - // The fixed report below is the only externally-visible failure surface. +} + +function parsedSecretValues(raw: string, requireJson: boolean): readonly string[] { + const trimmed = raw.trim(); + if (!trimmed) return []; + const values = new Set([raw.replace(/[\r\n]+$/u, "")]); + const looksJson = trimmed.startsWith("{") || trimmed.startsWith("["); + if (!looksJson) { + if (requireJson) throw unavailableSecretCorpus(); + return [...values]; + } + let document: unknown; + try { document = JSON.parse(trimmed); } catch { throw unavailableSecretCorpus(); } + if (requireJson && (!document || typeof document !== "object" || Array.isArray(document))) { + throw unavailableSecretCorpus(); + } + const pending: Array<{ value: unknown; depth: number }> = [{ value: document, depth: 0 }]; + let scalarCount = 0; + while (pending.length > 0) { + const current = pending.pop()!; + if (current.depth > MAX_DIAGNOSTIC_SECRET_DEPTH) throw unavailableSecretCorpus(); + if (Array.isArray(current.value)) { + for (const item of current.value) pending.push({ value: item, depth: current.depth + 1 }); + } else if (current.value && typeof current.value === "object") { + for (const item of Object.values(current.value as Record)) { + pending.push({ value: item, depth: current.depth + 1 }); + } + } else { + scalarCount += 1; + if (scalarCount > 1024) throw unavailableSecretCorpus(); + if (typeof current.value === "string" && current.value.length > 0) values.add(current.value); } } return [...values]; } +export function configuredSecretValues(config: AppConfig): readonly string[] { + try { + const values = new Set(); + if (config.secretsFile) { + for (const value of loadSecretBundle(config.secretsFile).values()) values.add(value); + } + const legacyFiles = new Set(Object.values(config.secretFiles).filter( + (file): file is string => file !== undefined, + )); + for (const file of legacyFiles) { + for (const value of parsedSecretValues(readMountedSecretSource(file), false)) values.add(value); + } + if (config.piAuthFile) { + for (const value of parsedSecretValues(readMountedSecretSource(config.piAuthFile), true)) values.add(value); + } + if (values.size > MAX_DIAGNOSTIC_SECRET_VALUES) throw unavailableSecretCorpus(); + return [...values]; + } catch { + throw unavailableSecretCorpus(); + } +} + export interface ConfiguredAuthDiagnoserOptions { localUserRegistry?: (loaded: LoadedAuthConfig) => LocalUserRegistry | undefined; oidcProtocol?: (loaded: LoadedAuthConfig) => OidcProtocol | undefined; @@ -231,17 +303,36 @@ export async function runDiagnosticCommand( } async function main(): Promise { - let config: AppConfig | undefined; - try { config = loadConfig(process.env); } catch { /* turn startup faults into the closed report below */ } - const exitCode = await runDiagnosticCommand(process.argv.slice(2), { - diagnoser: config ? createConfiguredAuthDiagnoser(config) : { inspect: async () => genericFailure() }, - ...(config ? { secretValues: configuredSecretValues(config) } : {}), - stdout: (line) => process.stdout.write(line), - stderr: (line) => process.stderr.write(`${line}\n`), - }); + const exitCode = await runConfiguredDiagnosticCommand( + process.argv.slice(2), process.env, + (line) => process.stdout.write(line), + (line) => process.stderr.write(`${line}\n`), + ); process.exitCode = exitCode; } +export async function runConfiguredDiagnosticCommand( + args: readonly string[], + env: Record, + stdout: (line: string) => void, + stderr: (line: string) => void, +): Promise { + let diagnoser: AuthDiagnoser = { inspect: async () => genericFailure() }; + let secretValues: readonly string[] | undefined; + try { + const config = loadConfig(env); + // Complete this preflight before constructing a diagnoser that may forward a device prompt. + secretValues = configuredSecretValues(config); + diagnoser = createConfiguredAuthDiagnoser(config); + } catch { /* turn startup or corpus faults into the closed report below */ } + return runDiagnosticCommand(args, { + diagnoser, + ...(secretValues === undefined ? {} : { secretValues }), + stdout, + stderr, + }); +} + if (process.argv[1] !== undefined && resolve(process.argv[1]) === fileURLToPath(import.meta.url)) { void main(); } diff --git a/backend/src/config.ts b/backend/src/config.ts index 6b6b9587..c3c00a10 100644 --- a/backend/src/config.ts +++ b/backend/src/config.ts @@ -27,6 +27,7 @@ export interface AppConfig { ollamaEnsureTimeoutMs: number; piManagementTimeoutMs: number; secretsFile?: string; + piAuthFile?: string; secretFiles: Readonly>; modelApiKeyFile?: string; /** @@ -259,6 +260,11 @@ export function loadConfig(env: Record): AppConfig { secretsFile.trim() !== secretsFile || secretsFile.length === 0 || secretsFile.includes("\0") || !path.isAbsolute(secretsFile) )) throw new Error("secret bundle configuration is invalid"); + const piAuthFile = env.THT_PI_AUTH_FILE; + if (piAuthFile !== undefined && ( + piAuthFile.trim() !== piAuthFile || piAuthFile.length === 0 || piAuthFile.includes("\0") + || !path.isAbsolute(piAuthFile) + )) throw new Error("Pi authentication source configuration is invalid"); const secretFiles: Record = {}; for (const name of [ "THT_MODEL_API_KEY_SECRET_FILE", "THT_DWH_API_KEY_SECRET_FILE", "THT_VEC_API_KEY_SECRET_FILE", @@ -335,6 +341,7 @@ export function loadConfig(env: Record): AppConfig { ollamaEnsureTimeoutMs: Number(env.OLLAMA_ENSURE_TIMEOUT_MS ?? 60000), piManagementTimeoutMs: piManagementTimeout(env.PI_MANAGEMENT_TIMEOUT_MS), secretsFile, + piAuthFile, secretFiles, modelApiKeyFile, dwhPrecheck: env.THT_DWH_PRECHECK === "true" || env.THT_DWH_PRECHECK === "1", diff --git a/backend/test/auth-diagnostic-command.test.ts b/backend/test/auth-diagnostic-command.test.ts index 46a1cede..8f87f6cb 100644 --- a/backend/test/auth-diagnostic-command.test.ts +++ b/backend/test/auth-diagnostic-command.test.ts @@ -6,6 +6,7 @@ import type { AuthDiagnoser, AuthDiagnostics } from "../src/auth/diagnostics.js" import { configuredSecretValues, createConfiguredAuthDiagnoser, + runConfiguredDiagnosticCommand, runDiagnosticCommand, } from "../src/auth/diagnostic-command.js"; import type { AppConfig } from "../src/config.js"; @@ -201,6 +202,59 @@ test("redacts every parsed mounted secret before writing an interactive prompt", expect(stdout.join("") + stderr.join("")).not.toContain(legacyVectorSecret); }); +test("includes every nested Pi authentication scalar in the direct-command redaction corpus", () => { + const piApiKey = "pi-api-key-SENTINEL"; + const piAccessToken = "pi-access-token-SENTINEL"; + const piAuthFile = secretBundle(JSON.stringify({ + providers: { + anthropic: { + key: piApiKey, + oauth: { access: piAccessToken, expires: 1_800_000_000 }, + }, + }, + })); + const config = { + secretsFile: secretBundle("THT_MODEL_API_KEY=model-secret\n"), + secretFiles: {}, + piAuthFile, + } as AppConfig; + + expect(configuredSecretValues(config)).toEqual(expect.arrayContaining([ + "model-secret", piApiKey, piAccessToken, + ])); +}); + +test("fails closed when any declared direct-command secret source cannot be loaded", () => { + const root = mkdtempSync(join(tmpdir(), "thothii-diagnostic-command-missing-")); + roots.push(root); + const config = { + secretsFile: secretBundle("THT_MODEL_API_KEY=model-secret\n"), + secretFiles: {}, + piAuthFile: join(root, "missing-pi-auth.json"), + } as AppConfig; + + expect(() => configuredSecretValues(config)).toThrow("diagnostic secret corpus is unavailable"); +}); + +test("suppresses interactive prompt forwarding when the production secret preflight is incomplete", async () => { + const root = mkdtempSync(join(tmpdir(), "thothii-diagnostic-command-preflight-")); + roots.push(root); + const stdout: string[] = []; + const stderr: string[] = []; + + const exitCode = await runConfiguredDiagnosticCommand( + ["--json", "--interactive"], + { NODE_ENV: "test", AUTH_MODE: "none", THT_PI_AUTH_FILE: join(root, "missing-auth.json") }, + (line) => stdout.push(line), + (line) => stderr.push(line), + ); + + expect(exitCode).toBe(1); + expect(stderr).toEqual([]); + expect(stdout).toHaveLength(1); + expect(JSON.parse(stdout[0])).toMatchObject({ ready: false, checks: [{ code: "auth_config_invalid" }] }); +}); + test.each([ { ready: true, mode: "oidc", checks: [{ level: "error", code: "oidc_secret_missing", message: "attacker-message" }], diff --git a/backend/test/config.test.ts b/backend/test/config.test.ts index 1a89029a..fc4920b1 100644 --- a/backend/test/config.test.ts +++ b/backend/test/config.test.ts @@ -47,6 +47,13 @@ test("loadConfig accepts container listening and runtime paths", () => { }); }); +test("loadConfig accepts only an absolute mounted Pi authentication source", () => { + expect(loadConfig({ THT_PI_AUTH_FILE: "/home/thoth/.pi/agent/auth.json" }).piAuthFile) + .toBe("/home/thoth/.pi/agent/auth.json"); + expect(() => loadConfig({ THT_PI_AUTH_FILE: "relative/auth.json" })) + .toThrow("Pi authentication source configuration is invalid"); +}); + test("loadConfig keeps local development defaults", () => { expect(loadConfig({})).toMatchObject({ host: "127.0.0.1", diff --git a/compose.yaml b/compose.yaml index 97c0bafc..59e77fdb 100644 --- a/compose.yaml +++ b/compose.yaml @@ -22,6 +22,7 @@ services: THT_WORKSPACE_SECRET_RUNTIME_ROOT: /tmp/thothii-workspace-secrets THT_WORKSPACE_SECRET_ROOTS: /run/secrets THT_SECRETS_FILE: /run/secrets/thothii.secrets + THT_PI_AUTH_FILE: /home/thoth/.pi/agent/auth.json THT_DB_NAME: ${THT_DB_NAME:-} THT_DWH_REST_URL: ${THT_DWH_REST_URL:-} THT_LLM_URL: ${THT_LLM_URL:-} diff --git a/frontend/src/api/workspaces.test.ts b/frontend/src/api/workspaces.test.ts index f783381d..643fbb5a 100644 --- a/frontend/src/api/workspaces.test.ts +++ b/frontend/src/api/workspaces.test.ts @@ -159,3 +159,19 @@ test.each([ await expect(validateWorkspace(workspace)).rejects.toThrow("invalid authentication diagnostics"); }); + +test("rejects a workspace claimed activatable when authentication is not ready", async () => { + server.use(http.post("/api/workspaces/validate", () => HttpResponse.json({ + workspace, + contract: {}, + activatable: true, + diagnostics: [], + authentication: { + ready: false, + mode: "oidc", + checks: [{ level: "error", code: "oidc_secret_missing", message: "Authentication is unavailable." }], + }, + }))); + + await expect(validateWorkspace(workspace)).rejects.toThrow("invalid diagnostic result"); +}); diff --git a/frontend/src/api/workspaces.ts b/frontend/src/api/workspaces.ts index f104a89b..682ccdff 100644 --- a/frontend/src/api/workspaces.ts +++ b/frontend/src/api/workspaces.ts @@ -291,7 +291,11 @@ function decodeWorkspaceDiagnostics(value: unknown): WorkspaceDiagnostics { ...(diagnostic.field === undefined ? {} : { field: diagnostic.field }), }; }); - return { activatable: source.activatable, diagnostics, authentication: decodeAuthentication(source.authentication) }; + const authentication = decodeAuthentication(source.authentication); + if (source.activatable && !authentication.ready) { + throw new Error("Workspace API returned an invalid diagnostic result"); + } + return { activatable: source.activatable, diagnostics, authentication }; } function exactObject(value: unknown, keys: readonly string[]): Record | undefined { diff --git a/frontend/src/shell/WorkspaceManager.test.tsx b/frontend/src/shell/WorkspaceManager.test.tsx index c8539c41..075be8a5 100644 --- a/frontend/src/shell/WorkspaceManager.test.tsx +++ b/frontend/src/shell/WorkspaceManager.test.tsx @@ -250,6 +250,33 @@ test("renders one authentication section with configured-group errors and no unm expect(within(section).queryByText(/unmapped/i)).not.toBeInTheDocument(); }); +test("clears a previous authentication result as soon as validation is retried", async () => { + const user = userEvent.setup(); + let calls = 0; + let releaseRetry!: () => void; + const retryStarted = new Promise((resolve) => { releaseRetry = resolve; }); + server.use(http.post("/api/workspaces/validate", async () => { + calls += 1; + if (calls > 1) await retryStarted; + return HttpResponse.json({ + workspace, contract: {}, activatable: true, diagnostics: [], authentication: readyAuthentication, + }); + })); + renderManager(); + await user.click(await screen.findByRole("button", { name: "PSD Clinical" })); + const button = screen.getByRole("button", { name: "Validate workspace source" }); + + await user.click(button); + expect(await screen.findByTestId("workspace-authentication")).toBeVisible(); + + await user.click(button); + await waitFor(() => expect(calls).toBe(2)); + expect(screen.queryByTestId("workspace-authentication")).not.toBeInTheDocument(); + + releaseRetry(); + expect(await screen.findByTestId("workspace-authentication")).toBeVisible(); +}); + test("never renders a hostile authentication field rejected by the API decoder", async () => { const user = userEvent.setup(); const attacker = "attacker-field-SENTINEL"; diff --git a/frontend/src/shell/WorkspaceManager.tsx b/frontend/src/shell/WorkspaceManager.tsx index 3f547c15..aee9013a 100644 --- a/frontend/src/shell/WorkspaceManager.tsx +++ b/frontend/src/shell/WorkspaceManager.tsx @@ -194,6 +194,7 @@ export function WorkspaceManager({ if (!guard) return; setBusyAction("validate"); clearGlobalMessages(); + setAuthentication(undefined); setValidationNotice(undefined); setValidationDiagnostics([]); try { @@ -217,6 +218,7 @@ export function WorkspaceManager({ if (!guard) return; setBusyAction("test"); clearGlobalMessages(); + setAuthentication(undefined); setConnectionNotice(undefined); setConnectionDiagnostics([]); try { diff --git a/scripts/test-compose-secret-policy.sh b/scripts/test-compose-secret-policy.sh index aafc9839..d71ef146 100755 --- a/scripts/test-compose-secret-policy.sh +++ b/scripts/test-compose-secret-policy.sh @@ -53,6 +53,9 @@ if (secretTargets.join(",") !== expectedSecrets.join(",")) { if (core.environment?.THT_SECRETS_FILE !== "/run/secrets/thothii.secrets") { throw new Error(`${name}: core does not use the canonical /run/secrets bundle path`); } +if (core.environment?.THT_PI_AUTH_FILE !== "/home/thoth/.pi/agent/auth.json") { + throw new Error(`${name}: core does not declare the mounted Pi authentication source`); +} if ((config.services.frontend?.secrets || []).length !== 0) { throw new Error(`${name}: frontend must not receive runtime secrets`); } diff --git a/tools/tht/internal/authconfig/commands.go b/tools/tht/internal/authconfig/commands.go index 5143633c..eefa8d4a 100644 --- a/tools/tht/internal/authconfig/commands.go +++ b/tools/tht/internal/authconfig/commands.go @@ -101,7 +101,7 @@ type authDiagnosticWire struct { } type authDiagnosticsWire struct { - Ready bool `json:"ready"` + Ready *bool `json:"ready"` Mode string `json:"mode"` Checks []authDiagnosticWire `json:"checks"` } @@ -246,7 +246,10 @@ func decodeAuthDiagnostics(value string) (AuthDiagnostics, error) { if err := decoder.Decode(&wire); err != nil || decoder.Decode(&struct{}{}) != io.EOF { return AuthDiagnostics{}, errors.New("authentication diagnostic report is invalid") } - report := AuthDiagnostics{Ready: wire.Ready, Mode: wire.Mode, Checks: make([]AuthDiagnostic, 0, len(wire.Checks))} + if wire.Ready == nil { + return AuthDiagnostics{}, errors.New("authentication diagnostic report is invalid") + } + report := AuthDiagnostics{Ready: *wire.Ready, Mode: wire.Mode, Checks: make([]AuthDiagnostic, 0, len(wire.Checks))} for _, item := range wire.Checks { var field *string if item.Field != nil { @@ -266,16 +269,16 @@ func decodeAuthDiagnostics(value string) (AuthDiagnostics, error) { return report, nil } -func authenticationSecretValues(installation config.Installation) []string { +func authenticationSecretValues(installation config.Installation) ([]string, error) { files, err := installation.SecretFiles() if err != nil { - return nil + return nil, errors.New("authentication diagnostic secret corpus is unavailable") } values, err := output.SecretValuesFromFiles(files) if err != nil { - return nil + return nil, errors.New("authentication diagnostic secret corpus is unavailable") } - return values + return values, nil } func sanitizeAuthDiagnostics(report AuthDiagnostics, secrets []string) AuthDiagnostics { @@ -339,15 +342,23 @@ func runCheck(ctx context.Context, installation config.Installation, runner comp if runner == nil { return AuthDiagnostics{}, "", errors.New("authentication diagnostic runner is unavailable") } + secrets, err := authenticationSecretValues(installation) + if err != nil { + return AuthDiagnostics{}, "", err + } timeout := authCheckTimeout if interactive { timeout = interactiveAuthCheckTimeout } bounded, cancel := context.WithTimeout(ctx, timeout) defer cancel() - command := []string{"run", "--rm", "--no-deps", "--no-TTY", "core", "node", "dist/auth/diagnostic-command.js", "--json"} - if useRunningCore { - command = []string{"exec", "-T", "core", "node", "dist/auth/diagnostic-command.js", "--json"} + command := []string{"exec", "-T", "core", "node", "dist/auth/diagnostic-command.js", "--json"} + if !useRunningCore { + containerName, err := compose.NewOneShotContainerName("thothii-auth-check") + if err != nil { + return AuthDiagnostics{}, "", errors.New("authentication diagnostic command failed") + } + command = []string{"run", "--rm", "--no-deps", "--no-TTY", "--name", containerName, "core", "node", "dist/auth/diagnostic-command.js", "--json"} } if interactive { command = append(command, "--interactive") @@ -356,7 +367,6 @@ func runCheck(ctx context.Context, installation config.Installation, runner comp StdoutBytes: maxAuthDiagnosticOutputBytes, StderrBytes: maxAuthDiagnosticOutputBytes, }) - secrets := authenticationSecretValues(installation) var exitError *exec.ExitError validProcessOutcome := result.ExitCode == 0 && err == nil || result.ExitCode == 1 && (err == nil || errors.As(err, &exitError)) diff --git a/tools/tht/internal/authconfig/commands_test.go b/tools/tht/internal/authconfig/commands_test.go index 2c83dc4e..0d10f329 100644 --- a/tools/tht/internal/authconfig/commands_test.go +++ b/tools/tht/internal/authconfig/commands_test.go @@ -43,10 +43,7 @@ func TestAuthCheckRunsOneShotCoreDiagnosticWithPristineJSON(t *testing.T) { if len(calls) != 1 { t.Fatalf("Docker calls = %#v, want one", calls) } - want := installation.ComposeArgs("run", "--rm", "--no-deps", "--no-TTY", "core", "node", "dist/auth/diagnostic-command.js", "--json") - if strings.Join(calls[0], "\x00") != strings.Join(want, "\x00") { - t.Fatalf("auth check Docker call = %#v, want %#v", calls[0], want) - } + assertAuthOneShotCommand(t, installation, calls[0], false) } func TestAuthCheckEmitsValidFailedReportFromRealExitError(t *testing.T) { @@ -142,6 +139,8 @@ func TestAuthDiagnosticsContractRejectsContradictionsDuplicatesAndAttackerFields func TestAuthCheckRejectsNullAndUnexpectedDiagnosticFields(t *testing.T) { for _, report := range []string{ + `{"mode":"oidc","checks":[{"level":"error","code":"oidc_secret_missing","message":"failure"}]}`, + `{"ready":null,"mode":"oidc","checks":[{"level":"error","code":"oidc_secret_missing","message":"failure"}]}`, `{"ready":false,"mode":"oidc","checks":[{"level":"error","code":"oidc_mapped_group_missing","message":"failure","field":null}]}`, `{"ready":false,"mode":"oidc","checks":[{"level":"error","code":"oidc_secret_missing","message":"failure","unexpected":"attacker"}]}`, } { @@ -185,9 +184,27 @@ func TestAuthCheckInteractiveForwardsOnlyTheValidatedDevicePrompt(t *testing.T) if len(calls) != 1 { t.Fatalf("Docker calls = %#v, want one", calls) } - want := installation.ComposeArgs("run", "--rm", "--no-deps", "--no-TTY", "core", "node", "dist/auth/diagnostic-command.js", "--json", "--interactive") - if strings.Join(calls[0], "\x00") != strings.Join(want, "\x00") { - t.Fatalf("interactive auth command = %#v, want %#v", calls[0], want) + assertAuthOneShotCommand(t, installation, calls[0], true) +} + +func TestAuthCheckFailsBeforeExecutionWhenDeclaredSecretCorpusIsIncomplete(t *testing.T) { + installation := authInstallation(newAuthDirectory(t)) + installation.EnvFile = filepath.Join(t.TempDir(), "operator.env") + missing := filepath.Join(t.TempDir(), "missing-pi-auth.json") + if err := os.WriteFile(installation.EnvFile, []byte("PI_AUTH_FILE="+missing+"\n"), 0o600); err != nil { + t.Fatal(err) + } + calls := 0 + runner := runnerFunc(func(_ context.Context, _ []string, _ io.Reader) (compose.Result, error) { + calls++ + return compose.Result{Stdout: `{"ready":true,"mode":"oidc","checks":[{"level":"info","code":"auth_ready","message":"Authentication is ready."}]}`}, nil + }) + var stdout, stderr bytes.Buffer + + code := RunWithRunner(context.Background(), installation, []string{"check", "--json"}, strings.NewReader(""), &stdout, &stderr, runner) + + if code != 1 || calls != 0 || stdout.Len() != 0 || stderr.String() != "tht: authentication diagnostics could not be completed\n" { + t.Fatalf("incomplete corpus was not refused before execution: code=%d calls=%d stdout=%q stderr=%q", code, calls, stdout.String(), stderr.String()) } } @@ -232,6 +249,29 @@ func (run runnerFunc) Run(ctx context.Context, args []string, input io.Reader) ( return run(ctx, args, input) } +func assertAuthOneShotCommand(t *testing.T, installation config.Installation, got []string, interactive bool) { + t.Helper() + prefix := installation.ComposeArgs("run", "--rm", "--no-deps", "--no-TTY", "--name") + suffix := []string{"core", "node", "dist/auth/diagnostic-command.js", "--json"} + if interactive { + suffix = append(suffix, "--interactive") + } + if len(got) != len(prefix)+1+len(suffix) || + strings.Join(got[:len(prefix)], "\x00") != strings.Join(prefix, "\x00") || + strings.Join(got[len(prefix)+1:], "\x00") != strings.Join(suffix, "\x00") { + t.Fatalf("auth check Docker call = %#v, want named one-shot command", got) + } + name := got[len(prefix)] + if !strings.HasPrefix(name, "thothii-auth-check-") || len(name) != len("thothii-auth-check-")+24 { + t.Fatalf("auth check container name = %q, want unique bounded name", name) + } + for _, character := range name { + if !(character >= 'a' && character <= 'z' || character >= '0' && character <= '9' || character == '-') { + t.Fatalf("auth check container name = %q, want safe characters", name) + } + } +} + func TestRunConfiguresLocalRegistryAndRedactsStatusJSON(t *testing.T) { directory := newAuthDirectory(t) passwordFile := writePasswordFile(t, "this is a local test password\n") @@ -538,7 +578,7 @@ func writeAuthExecutable(t *testing.T, contents string) string { func stringPointer(value string) *string { return &value } func authInstallation(directory string) config.Installation { - installation := config.Installation{} + installation := config.Installation{EnvFile: filepath.Join(filepath.Dir(directory), "operator.env")} installation.Authentication.ConfigDirectory = directory return installation } @@ -549,6 +589,9 @@ func newAuthDirectory(t *testing.T) string { if err != nil { t.Fatal(err) } + if err := os.WriteFile(filepath.Join(root, "operator.env"), []byte("SAFE_VALUE=1\n"), 0o600); err != nil { + t.Fatal(err) + } return filepath.Join(root, "auth") } diff --git a/tools/tht/internal/compose/process_unix.go b/tools/tht/internal/compose/process_unix.go index 7688d1a1..0bc62f9a 100644 --- a/tools/tht/internal/compose/process_unix.go +++ b/tools/tht/internal/compose/process_unix.go @@ -3,7 +3,6 @@ package compose import ( - "errors" "os/exec" "syscall" "time" @@ -32,6 +31,6 @@ func terminateProcess(command *exec.Cmd, done <-chan error) error { case <-done: return nil case <-time.After(finalTerminationBound): - return errors.New("Docker command could not be reaped") + return ErrProcessReap } } diff --git a/tools/tht/internal/compose/process_windows.go b/tools/tht/internal/compose/process_windows.go index 1e083ee2..b95d6607 100644 --- a/tools/tht/internal/compose/process_windows.go +++ b/tools/tht/internal/compose/process_windows.go @@ -3,7 +3,6 @@ package compose import ( - "errors" "os/exec" "syscall" "time" @@ -25,6 +24,6 @@ func terminateProcess(command *exec.Cmd, done <-chan error) error { case <-done: return nil case <-time.After(finalTerminationBound): - return errors.New("Docker command could not be reaped") + return ErrProcessReap } } diff --git a/tools/tht/internal/compose/runner.go b/tools/tht/internal/compose/runner.go index 17324622..9113f74c 100644 --- a/tools/tht/internal/compose/runner.go +++ b/tools/tht/internal/compose/runner.go @@ -3,22 +3,36 @@ package compose import ( "context" + "crypto/rand" + "encoding/hex" "errors" "fmt" "io" "os" "os/exec" + "strings" "sync" + "time" "github.com/aritmolab/thothii/tools/tht/internal/config" ) const defaultCaptureBytes = 4 * 1024 * 1024 const maximumCaptureBytes = 64 * 1024 * 1024 +const cleanupCaptureBytes = 4 * 1024 +const containerCleanupBound = 2 * time.Second // ErrOutputLimit reports that a child exceeded one of its capture limits. var ErrOutputLimit = errors.New("Docker command output limit exceeded") +// ErrProcessReap reports that a cancelled Docker CLI could not be reaped in its final bound. +var ErrProcessReap = errors.New("Docker command process could not be reaped") + +// ErrContainerCleanup reports that Docker could not confirm removal of a named one-shot container. +var ErrContainerCleanup = errors.New("Docker one-shot container cleanup failed") + +var terminateProcessForRunner = terminateProcess + // CaptureLimits bounds each captured stream while the child is running. type CaptureLimits struct { StdoutBytes int @@ -64,13 +78,26 @@ func (r execRunner) Run(ctx context.Context, args []string, stdin io.Reader) (Re // RunBounded invokes Docker while enforcing both stream limits during capture. func (r execRunner) RunBounded(ctx context.Context, args []string, stdin io.Reader, limits CaptureLimits) (Result, error) { + return r.runBounded(ctx, args, stdin, limits, true) +} + +func (r execRunner) runBounded(ctx context.Context, args []string, stdin io.Reader, limits CaptureLimits, manageOneShot bool) (Result, error) { if err := validCaptureLimits(limits); err != nil { return Result{}, err } if err := ctx.Err(); err != nil { return Result{}, err } - command := exec.Command(r.binary, args...) + preparedArgs := append([]string(nil), args...) + containerName := "" + if manageOneShot { + var err error + preparedArgs, containerName, err = prepareOneShot(preparedArgs) + if err != nil { + return Result{}, err + } + } + command := exec.Command(r.binary, preparedArgs...) configureProcess(command) command.Stdin = stdin overflow := make(chan struct{}, 1) @@ -83,35 +110,168 @@ func (r execRunner) RunBounded(ctx context.Context, args []string, stdin io.Read } done := make(chan error, 1) go func() { done <- command.Wait() }() - var err error + var processErr error + var lifecycleErr error + interrupted := false select { - case err = <-done: + case processErr = <-done: case <-ctx.Done(): - _ = terminateProcess(command, done) - err = ctx.Err() + interrupted = true + processErr = ctx.Err() + if err := terminateProcessForRunner(command, done); err != nil { + lifecycleErr = errors.Join(lifecycleErr, ErrProcessReap) + } case <-overflow: - _ = terminateProcess(command, done) - err = ErrOutputLimit + interrupted = true + processErr = ErrOutputLimit + if err := terminateProcessForRunner(command, done); err != nil { + lifecycleErr = errors.Join(lifecycleErr, ErrProcessReap) + } + } + if interrupted && containerName != "" { + if err := r.cleanupOneShotContainer(containerName); err != nil { + lifecycleErr = errors.Join(lifecycleErr, ErrContainerCleanup) + } } result := Result{Stdout: stdout.String(), Stderr: stderr.String()} if command.ProcessState != nil { result.ExitCode = command.ProcessState.ExitCode() } if stdout.Overflowed() || stderr.Overflowed() { - return result, ErrOutputLimit + return result, errors.Join(ErrOutputLimit, lifecycleErr) } - if err == nil { + if interrupted { + return result, errors.Join(processErr, lifecycleErr) + } + if processErr == nil { return result, nil } - if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, ErrOutputLimit) { - return result, err - } var exitError *exec.ExitError - if errors.As(err, &exitError) { + if errors.As(processErr, &exitError) { result.ExitCode = exitError.ExitCode() - return result, err + return result, processErr } - return result, err + return result, processErr +} + +// NewOneShotContainerName returns a Docker-safe, cross-process unique name with a bounded prefix. +func NewOneShotContainerName(prefix string) (string, error) { + if len(prefix) < 1 || len(prefix) > 96 || !safeContainerName(prefix) { + return "", errors.New("Docker one-shot container name is invalid") + } + random := make([]byte, 12) + if _, err := rand.Read(random); err != nil { + return "", errors.New("Docker one-shot container name is unavailable") + } + return prefix + "-" + hex.EncodeToString(random), nil +} + +func prepareOneShot(args []string) ([]string, string, error) { + for runIndex, arg := range args { + if arg != "run" || !(runIndex == 0 || args[0] == "compose") { + continue + } + _, name, valid := oneShotRunOptions(args, runIndex) + if !valid { + continue + } + if name != "" { + return args, name, nil + } + return args, "", nil + } + return args, "", nil +} + +func oneShotRunOptions(args []string, runIndex int) (rmIndex int, name string, valid bool) { + rmIndex = -1 + valueOptions := map[string]struct{}{ + "--name": {}, "--entrypoint": {}, "--network": {}, "--mount": {}, "--env": {}, "-e": {}, + "--user": {}, "-u": {}, "--volume": {}, "-v": {}, "--workdir": {}, "-w": {}, + "--label": {}, "-l": {}, "--pull": {}, "--cap-add": {}, "--cap-drop": {}, + } + for index := runIndex + 1; index < len(args); index++ { + arg := args[index] + if arg == "--rm" { + rmIndex = index + continue + } + if _, expectsValue := valueOptions[arg]; expectsValue { + if index+1 >= len(args) { + return -1, "", false + } + if arg == "--name" { + if !safeContainerName(args[index+1]) { + return -1, "", false + } + name = args[index+1] + } + index++ + continue + } + if strings.HasPrefix(arg, "--name=") { + name = strings.TrimPrefix(arg, "--name=") + if !safeContainerName(name) { + return -1, "", false + } + continue + } + if strings.HasPrefix(arg, "-") { + continue + } + return rmIndex, name, rmIndex >= 0 + } + return -1, "", false +} + +func safeContainerName(value string) bool { + if len(value) < 1 || len(value) > 128 { + return false + } + for index, character := range value { + letter := character >= 'a' && character <= 'z' || character >= 'A' && character <= 'Z' + digit := character >= '0' && character <= '9' + if letter || digit || index > 0 && (character == '-' || character == '_' || character == '.') { + continue + } + return false + } + return true +} + +func (r execRunner) cleanupOneShotContainer(name string) error { + if !safeContainerName(name) { + return ErrContainerCleanup + } + ctx, cancel := context.WithTimeout(context.Background(), containerCleanupBound) + defer cancel() + exists, err := r.oneShotContainerExists(ctx, name) + if err != nil || !exists { + if err != nil { + return ErrContainerCleanup + } + return nil + } + limits := CaptureLimits{StdoutBytes: cleanupCaptureBytes, StderrBytes: cleanupCaptureBytes} + if _, err := r.runBounded(ctx, []string{"container", "rm", "-f", name}, nil, limits, false); err != nil { + return ErrContainerCleanup + } + exists, err = r.oneShotContainerExists(ctx, name) + if err != nil || exists { + return ErrContainerCleanup + } + return nil +} + +func (r execRunner) oneShotContainerExists(ctx context.Context, name string) (bool, error) { + limits := CaptureLimits{StdoutBytes: cleanupCaptureBytes, StderrBytes: cleanupCaptureBytes} + result, err := r.runBounded(ctx, []string{ + "container", "ls", "--all", "--quiet", "--filter", "name=^/" + name + "$", + }, nil, limits, false) + if err != nil { + return false, ErrContainerCleanup + } + return strings.TrimSpace(result.Stdout) != "", nil } // RunBounded uses the production runner's during-capture limits while retaining compatibility diff --git a/tools/tht/internal/compose/runner_test.go b/tools/tht/internal/compose/runner_test.go index 7328779c..4e358bcc 100644 --- a/tools/tht/internal/compose/runner_test.go +++ b/tools/tht/internal/compose/runner_test.go @@ -55,7 +55,7 @@ func TestRunnerCancelsAndReapsAHangingChildWithinFinalBound(t *testing.T) { t.Parallel() runner := NewRunner(writeExecutable(t, "#!/bin/sh\ntrap '' TERM INT\nwhile :; do sleep 1; done\n")) - ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) defer cancel() started := time.Now() _, err := RunBounded(runner, ctx, []string{"compose", "run", "--rm", "core"}, nil, CaptureLimits{ @@ -70,6 +70,98 @@ func TestRunnerCancelsAndReapsAHangingChildWithinFinalBound(t *testing.T) { } } +func TestRunnerCleansUpNamedComposeContainerAfterOverflow(t *testing.T) { + logFile := filepath.Join(t.TempDir(), "calls.log") + marker := filepath.Join(t.TempDir(), "container-present") + if err := os.WriteFile(marker, []byte("present"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("THT_RUNNER_TEST_LOG", logFile) + t.Setenv("THT_RUNNER_TEST_MARKER", marker) + t.Setenv("THT_RUNNER_TEST_MODE", "flood") + runner := NewRunner(writeExecutable(t, cleanupAwareDockerScript)) + name := "thothii-cleanup-overflow-sentinel" + + _, err := RunBounded(runner, context.Background(), []string{ + "compose", "run", "--rm", "--name", name, "core", + }, nil, CaptureLimits{StdoutBytes: 1024, StderrBytes: 1024}) + + if !errors.Is(err, ErrOutputLimit) { + t.Fatalf("RunBounded() error = %v, want ErrOutputLimit", err) + } + assertContainerCleanup(t, logFile, marker, name) +} + +func TestRunnerCleansUpNamedComposeContainerAfterCancellation(t *testing.T) { + logFile := filepath.Join(t.TempDir(), "calls.log") + marker := filepath.Join(t.TempDir(), "container-present") + if err := os.WriteFile(marker, []byte("present"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("THT_RUNNER_TEST_LOG", logFile) + t.Setenv("THT_RUNNER_TEST_MARKER", marker) + t.Setenv("THT_RUNNER_TEST_MODE", "hang") + runner := NewRunner(writeExecutable(t, cleanupAwareDockerScript)) + name := "thothii-cleanup-cancel-sentinel" + ctx, cancel := context.WithTimeout(context.Background(), 200*time.Millisecond) + defer cancel() + + _, err := RunBounded(runner, ctx, []string{ + "compose", "run", "--rm", "--name", name, "core", + }, nil, CaptureLimits{StdoutBytes: 1024, StderrBytes: 1024}) + + if !errors.Is(err, context.DeadlineExceeded) { + t.Fatalf("RunBounded() error = %v, want deadline exceeded", err) + } + assertContainerCleanup(t, logFile, marker, name) +} + +func TestRunnerSurfacesCleanupFailureWithoutContainerName(t *testing.T) { + logFile := filepath.Join(t.TempDir(), "calls.log") + marker := filepath.Join(t.TempDir(), "container-present") + if err := os.WriteFile(marker, []byte("present"), 0o600); err != nil { + t.Fatal(err) + } + t.Setenv("THT_RUNNER_TEST_LOG", logFile) + t.Setenv("THT_RUNNER_TEST_MARKER", marker) + t.Setenv("THT_RUNNER_TEST_MODE", "cleanup-fails") + runner := NewRunner(writeExecutable(t, cleanupAwareDockerScript)) + name := "thothii-cleanup-secret-sentinel" + ctx, cancel := context.WithTimeout(context.Background(), 50*time.Millisecond) + defer cancel() + + _, err := RunBounded(runner, ctx, []string{ + "compose", "run", "--rm", "--name", name, "core", + }, nil, CaptureLimits{StdoutBytes: 1024, StderrBytes: 1024}) + + if err == nil || !strings.Contains(err.Error(), "one-shot container cleanup failed") { + t.Fatalf("RunBounded() error = %v, want safe cleanup failure", err) + } + if strings.Contains(err.Error(), name) { + t.Fatalf("RunBounded() exposed the container name: %v", err) + } +} + +func TestRunnerPropagatesAReapingFailure(t *testing.T) { + original := terminateProcessForRunner + terminateProcessForRunner = func(command *exec.Cmd, done <-chan error) error { + _ = terminateProcess(command, done) + return ErrProcessReap + } + t.Cleanup(func() { terminateProcessForRunner = original }) + runner := NewRunner(writeExecutable(t, "#!/bin/sh\ntrap '' TERM INT\nwhile :; do sleep 1; done\n")) + ctx, cancel := context.WithTimeout(context.Background(), 100*time.Millisecond) + defer cancel() + + _, err := RunBounded(runner, ctx, []string{"version"}, nil, CaptureLimits{ + StdoutBytes: 1024, StderrBytes: 1024, + }) + + if !errors.Is(err, context.DeadlineExceeded) || !errors.Is(err, ErrProcessReap) { + t.Fatalf("RunBounded() error = %v, want deadline and reap failure", err) + } +} + func TestRunnerReturnsTheChildExitCode(t *testing.T) { t.Parallel() @@ -86,6 +178,22 @@ func TestRunnerReturnsTheChildExitCode(t *testing.T) { } } +func TestRunnerRetainsTheExactNormalRmInvocationForAnUnnamedOneShot(t *testing.T) { + t.Parallel() + + runner := NewRunner(writeExecutable(t, "#!/bin/sh\nprintf '%s\\n' \"$@\"\n")) + result, err := runner.Run(context.Background(), []string{ + "compose", "run", "--rm", "--no-deps", "core", "node", "diagnostic.js", + }, nil) + if err != nil { + t.Fatal(err) + } + got := strings.Fields(result.Stdout) + if strings.Join(got, " ") != "compose run --rm --no-deps core node diagnostic.js" { + t.Fatalf("one-shot argv = %#v, want exact original --rm invocation", got) + } +} + func TestRunnerReportsMissingDocker(t *testing.T) { t.Parallel() @@ -140,3 +248,35 @@ func writeExecutable(t *testing.T, contents string) string { } return path } + +func assertContainerCleanup(t *testing.T, logFile, marker, name string) { + t.Helper() + calls, err := os.ReadFile(logFile) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(string(calls), "container rm -f "+name) { + t.Fatalf("Docker calls = %q, want bounded container cleanup", calls) + } + if _, err := os.Stat(marker); !os.IsNotExist(err) { + t.Fatalf("one-shot container marker still exists: %v", err) + } +} + +const cleanupAwareDockerScript = `#!/bin/sh +printf '%s\n' "$*" >> "$THT_RUNNER_TEST_LOG" +if [ "$1" = "container" ] && [ "$2" = "ls" ]; then + if [ -f "$THT_RUNNER_TEST_MARKER" ]; then printf '%s\n' container-id; fi + exit 0 +fi +if [ "$1" = "container" ] && [ "$2" = "rm" ]; then + if [ "$THT_RUNNER_TEST_MODE" = "cleanup-fails" ]; then exit 9; fi + rm -f "$THT_RUNNER_TEST_MARKER" + exit 0 +fi +if [ "$THT_RUNNER_TEST_MODE" = "flood" ]; then + while :; do printf '0123456789abcdef'; printf 'fedcba9876543210' >&2; done +fi +trap '' TERM INT +while :; do sleep 1; done +` diff --git a/tools/tht/internal/doctor/report.go b/tools/tht/internal/doctor/report.go index c23ccef7..574870a6 100644 --- a/tools/tht/internal/doctor/report.go +++ b/tools/tht/internal/doctor/report.go @@ -98,7 +98,7 @@ func RunWithProbe(ctx context.Context, installation config.Installation, runner if probe == nil { return Report{}, errors.New("doctor requires an HTTP probe") } - secretValues := secretValues(installation) + secretValues, secretErr := secretValues(installation) report := Report{Checks: make([]Check, 0, 12)} add := func(name, status, detail string) { report.Checks = append(report.Checks, Check{Name: name, Status: status, Detail: output.SanitizeDetail(detail, secretValues)}) @@ -109,11 +109,28 @@ func RunWithProbe(ctx context.Context, installation config.Installation, runner } else { add("descriptor", StatusPassed, "installation descriptor is loaded") } - if err := filePermissions(installation); err != nil { - add("files", StatusFailed, err.Error()) + if err := filePermissions(installation); err != nil || secretErr != nil { + if secretErr != nil { + add("files", StatusFailed, "declared secret files could not be read") + } else { + add("files", StatusFailed, err.Error()) + } } else { add("files", StatusPassed, "declared host files have safe permissions") } + if secretErr != nil { + add("docker", StatusSkipped, "declared secret files are unavailable") + add("compose", StatusSkipped, "declared secret files are unavailable") + add("configuration", StatusSkipped, "declared secret files are unavailable") + add("authentication", StatusSkipped, "declared secret files are unavailable") + add("services", StatusSkipped, "declared secret files are unavailable") + add("core-http", StatusSkipped, "declared secret files are unavailable") + add("frontend-http", StatusSkipped, "declared secret files are unavailable") + add("workspace-registry", StatusSkipped, "declared secret files are unavailable") + add("workflow", StatusSkipped, "declared secret files are unavailable") + add("pi", StatusSkipped, "declared secret files are unavailable") + return finalize(report), nil + } if !commandCheck(ctx, runner, []string{"version", "--format", "{{.Client.Version}}"}, secretValues, add, "docker", "Docker Engine") { add("compose", StatusSkipped, "Docker Engine is unavailable") @@ -307,16 +324,16 @@ func validateInstallation(installation config.Installation) error { return nil } -func secretValues(installation config.Installation) []string { +func secretValues(installation config.Installation) ([]string, error) { files, err := installation.SecretFiles() if err != nil { - return nil + return nil, errors.New("declared secret files could not be read") } values, err := output.SecretValuesFromFiles(files) if err != nil { - return nil + return nil, errors.New("declared secret files could not be read") } - return values + return values, nil } func filePermissions(installation config.Installation) error { diff --git a/tools/tht/internal/doctor/report_test.go b/tools/tht/internal/doctor/report_test.go index 15e1f73d..e255b693 100644 --- a/tools/tht/internal/doctor/report_test.go +++ b/tools/tht/internal/doctor/report_test.go @@ -181,6 +181,28 @@ func TestRunRedactsWorkflowDiagnosticFailures(t *testing.T) { } } +func TestRunFailsBeforeDockerWhenDeclaredSecretCorpusIsIncomplete(t *testing.T) { + installation := doctorInstallation(t, "") + missing := filepath.Join(filepath.Dir(installation.EnvFile), "missing-pi-auth.json") + if err := os.WriteFile(installation.EnvFile, []byte("PI_AUTH_FILE="+missing+"\n"), 0o600); err != nil { + t.Fatal(err) + } + runner := &doctorRunner{services: healthyServices} + + report, err := Run(context.Background(), installation, runner) + + if err != nil { + t.Fatal(err) + } + if report.OK || checkStatus(report, "files") != StatusFailed || len(runner.calls) != 0 { + t.Fatalf("incomplete corpus was not refused before Docker: report=%#v calls=%v", report, runner.calls) + } + if strings.Contains(reportText(report), missing) { + t.Fatalf("incomplete corpus report exposed a secret path: %#v", report) + } + assertChecklist(t, report, []string{"descriptor", "files", "docker", "compose", "configuration", "authentication", "services", "core-http", "frontend-http", "workspace-registry", "workflow", "pi"}) +} + func doctorInstallation(t *testing.T, _ string) config.Installation { t.Helper() base, err := filepath.EvalSymlinks(os.TempDir())