From 3b9681a63aab5fdf01a3df1efe4d248bbf2ed87d Mon Sep 17 00:00:00 2001 From: mptyl Date: Sun, 9 Aug 2026 19:27:52 +0200 Subject: [PATCH] fix: harden evidence secret file handoff --- backend/src/workspaces/bindings.ts | 23 +++++++------- backend/test/workspaces-bindings.test.ts | 28 +++++++++++++---- .../tests/test_registry_evidence_config.py | 31 +++++++++++++++++++ harness/tht/config.py | 25 ++++++++++----- 4 files changed, 83 insertions(+), 24 deletions(-) diff --git a/backend/src/workspaces/bindings.ts b/backend/src/workspaces/bindings.ts index a5f01015..19c070c7 100644 --- a/backend/src/workspaces/bindings.ts +++ b/backend/src/workspaces/bindings.ts @@ -60,18 +60,18 @@ function isInside(path: string, root: string): boolean { return pathRelative !== "" && !pathRelative.startsWith("..") && !isAbsolute(pathRelative); } -function isSafeSecretFile(path: string, secretRoots: readonly string[]): boolean { - if (!isAbsolute(path)) return false; +function safeSecretFilePath(path: string, secretRoots: readonly string[]): string | undefined { + if (!isAbsolute(path)) return undefined; try { const resolvedPath = realpathSync(path); const resolvedRoots = secretRoots.map((root) => realpathSync(root)); - if (!resolvedRoots.some((root) => isInside(resolvedPath, root))) return false; - if (!statSync(resolvedPath).isFile()) return false; + if (!resolvedRoots.some((root) => isInside(resolvedPath, root))) return undefined; + if (!statSync(resolvedPath).isFile()) return undefined; accessSync(resolvedPath, constants.R_OK); - return true; + return resolvedPath; } catch { - return false; + return undefined; } } @@ -132,11 +132,12 @@ export function resolveBinding( const value = env[variable.name]; const present = value !== undefined && value.trim() !== ""; - const safe = !variable.secret || (present && isSafeSecretFile(value, secretRoots)); + const safePath = variable.secret && present ? safeSecretFilePath(value, secretRoots) : undefined; + const safe = !variable.secret || safePath !== undefined; if ((required.has(variable.suffix) && !present) || (present && !safe)) { missing.push(variable.name); } - if (present && safe) values[variable.name] = value; + if (present && safe) values[variable.name] = variable.secret ? safePath! : value; } return { transport: selectedTransport, values, missing }; @@ -166,11 +167,11 @@ export function resolveEvidenceBinding( for (const variable of variables) { const value = env[variable.name]; const present = value !== undefined && value.trim() !== ""; - const safe = present && isSafeSecretFile(value, secretRoots); - if ((required.has(variable.suffix) && !present) || (present && !safe)) { + const safePath = present ? safeSecretFilePath(value, secretRoots) : undefined; + if ((required.has(variable.suffix) && !present) || (present && safePath === undefined)) { missing.push(variable.name); } - if (safe) values[variable.name] = value; + if (safePath !== undefined) values[variable.name] = safePath; } return { values, missing }; } diff --git a/backend/test/workspaces-bindings.test.ts b/backend/test/workspaces-bindings.test.ts index 9875445c..5b219915 100644 --- a/backend/test/workspaces-bindings.test.ts +++ b/backend/test/workspaces-bindings.test.ts @@ -1,4 +1,4 @@ -import { chmodSync, mkdirSync, mkdtempSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; +import { chmodSync, mkdirSync, mkdtempSync, realpathSync, rmSync, symlinkSync, writeFileSync } from "node:fs"; import { tmpdir } from "node:os"; import { join } from "node:path"; import { afterEach, expect, test } from "vitest"; @@ -153,7 +153,7 @@ test("resolves direct bindings from the stable workspace namespace", () => { values: { THT_WS_PSD_CLINICAL_DWH_HOST: "dwh.internal", THT_WS_PSD_CLINICAL_DWH_PORT: "5432", - THT_WS_PSD_CLINICAL_DWH_PASSWORD_FILE: password.path, + THT_WS_PSD_CLINICAL_DWH_PASSWORD_FILE: realpathSync(password.path), }, }); }); @@ -304,10 +304,26 @@ test("requires only a safe HTTP signed-URL file and never reads its contents", ( [variable]: signed.path, [evidenceVariable("ACCESS_KEY_FILE")]: signed.path, }, [signed.root]); - expect(resolved).toEqual({ values: { [variable]: signed.path }, missing: [] }); + expect(resolved).toEqual({ values: { [variable]: realpathSync(signed.path) }, missing: [] }); expect(JSON.stringify(resolved)).not.toContain("CANARY-SIGNED-URL-CONTENT"); }); +test("canonicalizes an in-root Evidence symlink before passing it to the harness", () => { + const signed = secretPath("evidence-signed-target"); + const link = join(signed.root, "signed-urls-link"); + symlinkSync(signed.path, link); + const source = withEvidence({ + type: "http", + uris: ["https://evidence.example.test/guide.md"], + authentication: "signed_urls_file", + }); + const variable = evidenceVariable("SIGNED_URLS_FILE"); + + expect(resolveEvidenceBinding(source, { [variable]: link }, [signed.root])).toEqual({ + values: { [variable]: realpathSync(signed.path) }, missing: [], + }); +}); + test("requires S3 access and secret files together while accepting an optional safe session token", () => { const access = secretPath("evidence-access"); const secret = secretPath("evidence-secret"); @@ -327,9 +343,9 @@ test("requires S3 access and secret files together while accepting an optional s }, [access.root]).missing).toEqual([evidenceVariable("SECRET_KEY_FILE")]); expect(resolveEvidenceBinding(source, env, [access.root, secret.root, token.root])).toEqual({ values: { - [evidenceVariable("ACCESS_KEY_FILE")]: access.path, - [evidenceVariable("SECRET_KEY_FILE")]: secret.path, - [evidenceVariable("SESSION_TOKEN_FILE")]: token.path, + [evidenceVariable("ACCESS_KEY_FILE")]: realpathSync(access.path), + [evidenceVariable("SECRET_KEY_FILE")]: realpathSync(secret.path), + [evidenceVariable("SESSION_TOKEN_FILE")]: realpathSync(token.path), }, missing: [], }); diff --git a/harness/tests/test_registry_evidence_config.py b/harness/tests/test_registry_evidence_config.py index 3f7db24a..f972974b 100644 --- a/harness/tests/test_registry_evidence_config.py +++ b/harness/tests/test_registry_evidence_config.py @@ -223,6 +223,37 @@ def test_signed_http_rejects_reordered_extra_mismatch_userinfo_and_duplicate_pro assert_no_canaries(caught.value) +def test_s3_rejects_inline_credentials_and_never_discloses_them(tmp_path): + path = write_config(tmp_path, { + "type": "s3", "bucket": "clinical-evidence", + "access_key": ACCESS_CANARY, + "secret_key": SECRET_CANARY, + "session_token": TOKEN_CANARY, + }) + + with pytest.raises(ConfigError) as caught: + load_config(path) + assert "file" in str(caught.value).lower() + assert_no_canaries(caught.value) + assert_no_canaries("".join(traceback.format_exception(caught.value))) + + +def test_s3_scalar_secret_files_are_bounded(tmp_path): + access = tmp_path / "oversized-access-key" + access.write_bytes(b"A" * (64 * 1024 + 1)) + secret = tmp_path / "secret-key" + secret.write_text("bounded-secret") + path = write_config(tmp_path, { + "type": "s3", "bucket": "clinical-evidence", + "access_key_file": str(access), "secret_key_file": str(secret), + }) + + with pytest.raises(ConfigError) as caught: + load_config(path) + assert "secret file" in str(caught.value).lower() + assert "bounded-secret" not in str(caught.value) + + def test_s3_ambient_and_static_file_credentials_are_secret_typed(tmp_path): ambient = load_config(write_config(tmp_path, { "type": "s3", "bucket": "clinical-evidence", "prefix": "published/", diff --git a/harness/tht/config.py b/harness/tht/config.py index 040bdf1f..6916d13d 100644 --- a/harness/tht/config.py +++ b/harness/tht/config.py @@ -49,13 +49,17 @@ def _expand_env(value: Any) -> Any: _MAX_SIGNED_URL_FILE_BYTES = 1024 * 1024 -def _resolve_http_signed_url_files(value: Any) -> Any: +def _resolve_evidence_secret_files(value: Any) -> Any: """Resolve only signed HTTP URL arrays, keeping their values out of public errors.""" if isinstance(value, dict): resolved = { - key: _resolve_http_signed_url_files(item) + key: _resolve_evidence_secret_files(item) for key, item in value.items() } + if resolved.get("type") == "s3" and any( + name in resolved for name in ("access_key", "secret_key", "session_token") + ): + raise ConfigError("S3 Evidence credentials require *_file references") if resolved.get("type") != "http" or "signed_urls_file" not in resolved: return resolved if "urls" in resolved: @@ -94,10 +98,13 @@ def _resolve_http_signed_url_files(value: Any) -> Any: resolved["urls"] = parsed return resolved if isinstance(value, list): - return [_resolve_http_signed_url_files(item) for item in value] + return [_resolve_evidence_secret_files(item) for item in value] return value +_MAX_SCALAR_SECRET_FILE_BYTES = 64 * 1024 + + def _resolve_secret_files(value: Any) -> Any: if isinstance(value, dict): resolved = {key: _resolve_secret_files(item) for key, item in value.items()} @@ -109,9 +116,13 @@ def _resolve_secret_files(value: Any) -> Any: raise ConfigError(f"{secret_name} and {file_name} are mutually exclusive") path = Path(resolved.pop(file_name)) try: - secret = path.read_text() - except (OSError, UnicodeError) as exc: - raise ConfigError(f"Cannot read secret file: {path}") from exc + with path.open("rb") as stream: + payload = stream.read(_MAX_SCALAR_SECRET_FILE_BYTES + 1) + if len(payload) > _MAX_SCALAR_SECRET_FILE_BYTES: + raise OSError + secret = payload.decode("utf-8") + except (OSError, UnicodeError): + raise ConfigError(f"Cannot read secret file: {path}") from None if not secret or any(char.isspace() for char in secret) or "\x00" in secret: raise ConfigError(f"Invalid secret file: {path}") resolved[secret_name] = secret @@ -532,7 +543,7 @@ def load_config(path: Path) -> Config: raise ConfigError(f"Configurazione YAML non valida: {path}") from exc if not isinstance(raw, dict): raise ConfigError(f"Configurazione non valida (atteso un mapping YAML): {path}") - expanded = _resolve_secret_files(_resolve_http_signed_url_files(_expand_env(raw))) + expanded = _resolve_secret_files(_resolve_evidence_secret_files(_expand_env(raw))) _validate_internal_embedding_contract(expanded, path) _validate_internal_vector_contract(expanded, path) translated, used_legacy = translate_legacy_config(expanded)