From fc349e634cbbc51df5ce7e70d84d71f06d31e0af Mon Sep 17 00:00:00 2001 From: mptyl Date: Wed, 5 Aug 2026 12:09:32 +0200 Subject: [PATCH] fix: close server bypass edge cases --- docs/install/server.md | 8 +- scripts/test-server-operator-permissions.sh | 19 +- scripts/test-verify-workspace-install-docs.sh | 37 +++- scripts/verify-workspace-install-docs.sh | 183 ++++++++++++------ tools/thothctl/cmd/thothctl/main.go | 2 +- tools/thothctl/cmd/thothctl/main_test.go | 76 +++++--- tools/thothctl/internal/output/sanitize.go | 20 ++ .../thothctl/internal/serverops/operations.go | 23 +-- .../internal/serverops/operations_test.go | 76 +++++--- 9 files changed, 296 insertions(+), 148 deletions(-) diff --git a/docs/install/server.md b/docs/install/server.md index 01cd8467..a0d0b458 100644 --- a/docs/install/server.md +++ b/docs/install/server.md @@ -60,9 +60,12 @@ write access to runtime bind trees. Create explicit directories. `source` contains the clone; `operator` contains untracked path-only configuration; the three writable trees are bind-mounted into `core`; `secrets` contains regular -files only. Backups are separate from live data. +files only. Backups are separate from live data. Reset the account home explicitly because +distribution `useradd` defaults may otherwise leave `/srv/thothii` non-traversable by +`thothii-ops`. ```sh +sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii/source sudo install -d -o 10001 -g thothii-ops -m 2770 /srv/thothii/operator sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii/secrets @@ -72,7 +75,8 @@ sudo install -d -o 10001 -g 10001 -m 0750 /srv/thothii/workspace-registry sudo install -d -o root -g root -m 0700 /srv/thothii-backups ``` -The human operator can write only `operator`; setgid keeps generated files in `thothii-ops`. +Verify `/srv/thothii` is owned by `10001:thothii-ops` with mode `2750`. The human operator can +traverse the parent but can write only `operator`; setgid keeps generated files in `thothii-ops`. `source`, `secrets`, and all runtime bind trees remain non-group-writable. Do not make `/srv/thothii` a shared application directory. diff --git a/scripts/test-server-operator-permissions.sh b/scripts/test-server-operator-permissions.sh index 67aa14a7..4b8ab817 100755 --- a/scripts/test-server-operator-permissions.sh +++ b/scripts/test-server-operator-permissions.sh @@ -7,12 +7,15 @@ image='golang:1.26.5-bookworm@sha256:1ecb7edf62a0408027bd5729dfd6b1b8766e578e8df docker run --rm --volume "$root:/repository:ro" "$image" /bin/bash -ceu ' groupadd --gid 10001 thothii -useradd --uid 10001 --gid 10001 --no-create-home --shell /usr/sbin/nologin thothii +useradd --uid 10001 --gid 10001 --home-dir /srv/thothii --create-home --shell /usr/sbin/nologin thothii +# Reproduce the conservative home mode permitted by the documented useradd sequence. +chmod 0700 /srv/thothii groupadd --gid 20001 operator-primary groupadd --gid 20002 thothii-ops groupadd --gid 20003 docker useradd --uid 20001 --gid 20001 --groups 20002,20003 --create-home --shell /bin/bash operator +install -d -o 10001 -g 20002 -m 2750 /srv/thothii install -d -o 10001 -g 20002 -m 2750 /srv/thothii/source install -d -o 10001 -g 20002 -m 2770 /srv/thothii/operator install -d -o 10001 -g 20002 -m 2750 /srv/thothii/secrets @@ -52,8 +55,10 @@ sed -i "s#replace-me#/srv/thothii/source/ThothII#" /srv/thothii/operator/thothii --output /srv/thothii/operator/connector-secrets.server.yaml test -r /srv/thothii/secrets/dwh-password if (printf tamper >> /srv/thothii/secrets/dwh-password) 2>/dev/null; then exit 41; fi -if touch /srv/thothii/source/operator-must-not-write 2>/dev/null; then exit 42; fi -if touch /srv/thothii/data/operator-must-not-write 2>/dev/null; then exit 43; fi +for protected in /srv/thothii /srv/thothii/source /srv/thothii/secrets \ + /srv/thothii/data /srv/thothii/pi-state /srv/thothii/workspace-registry; do + if touch "$protected/operator-must-not-write" 2>/dev/null; then exit 42; fi +done THT_THOTHCTL_OUTPUT_DIRECTORY=/srv/thothii/operator/build-output \ /srv/thothii/source/ThothII/scripts/build-thothctl.sh if THT_THOTHCTL_OUTPUT_DIRECTORY=relative-output \ @@ -70,11 +75,15 @@ rm -f "$root_output_error" test "$(stat -c %u:%g /srv/thothii/operator/connector-secrets.server.yaml)" = 20001:20002 test "$(stat -c %a /srv/thothii/operator/connector-secrets.server.yaml)" = 660 +test "$(stat -c %u:%g /srv/thothii)" = 10001:20002 +test "$(stat -c %a /srv/thothii)" = 2750 test "$(stat -c %u:%g /srv/thothii/operator/build-output/thothctl-linux-amd64)" = 20001:20002 test "$(stat -c %a /srv/thothii/operator/build-output/thothctl-linux-amd64)" = 750 test -f /srv/thothii/operator/start.marker -test ! -e /srv/thothii/source/operator-must-not-write -test ! -e /srv/thothii/data/operator-must-not-write +for protected in /srv/thothii /srv/thothii/source /srv/thothii/secrets \ + /srv/thothii/data /srv/thothii/pi-state /srv/thothii/workspace-registry; do + test ! -e "$protected/operator-must-not-write" +done test "$(cat /srv/thothii/secrets/dwh-password)" = operator-readable-secret ' diff --git a/scripts/test-verify-workspace-install-docs.sh b/scripts/test-verify-workspace-install-docs.sh index 01d46510..c1546d1b 100755 --- a/scripts/test-verify-workspace-install-docs.sh +++ b/scripts/test-verify-workspace-install-docs.sh @@ -36,6 +36,10 @@ for fixture in \ done server_guide="$root/docs/install/server.md" +grep -Eq '^sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii$' "$server_guide" || { + echo "server operations guide does not set the parent traversal boundary" >&2 + exit 1 +} for required in \ 'thothii-ops' \ 'THT_BACKUP_ROOT=/srv/thothii-backups' \ @@ -101,7 +105,8 @@ sed '/^case "\$mode" in/,$d' "$root/scripts/verify-workspace-install-docs.sh" >" source "$verifier_functions" adapted_reorder="$negative_root/caddy-adapted-reorder.json" -node - "$adapted_reorder" <<'NODE' +adapted_bypass="$negative_root/caddy-adapted-bypass.json" +node - "$adapted_reorder" "$adapted_bypass" <<'NODE' const fs = require("fs"); const publicHeaders = [ "X-Thoth-Principal-Issuer", "X-Thoth-Principal-Subject", @@ -123,6 +128,12 @@ const document = {routes: [{handle: [ {handler: "reverse_proxy", upstreams: [{dial: "127.0.0.1:8080"}]}, ]}]}; fs.writeFileSync(process.argv[2], JSON.stringify(document)); +const frontend = {handler: "reverse_proxy", upstreams: [{dial: "127.0.0.1:8080"}]}; +const validChain = [...publicHeaders, ...trustedHeaders].map(clear).concat(auth, frontend); +fs.writeFileSync(process.argv[3], JSON.stringify({routes: [ + {handle: validChain}, + {handle: [frontend]}, +]})); NODE adapted_output="$negative_root/caddy-adapted-output" set +e @@ -135,6 +146,16 @@ if [[ $adapted_status -eq 0 ]] || ! grep -Fq "Caddy adapted identity clears must exit 1 fi +set +e +verify_caddy_adapted_identity_order "$adapted_bypass" >"$adapted_output" 2>&1 +adapted_status=$? +set -e +if [[ $adapted_status -eq 0 ]] || ! grep -Fq "Caddy adapted frontend path bypasses complete authentication contract" "$adapted_output"; then + echo "Caddy additional direct frontend route fixture was not rejected correctly" >&2 + cat "$adapted_output" >&2 + exit 1 +fi + negative_failures=0 expect_guide_rejected() { local label="$1" validator="$2" source_guide="$3" relative_path="$4" @@ -180,6 +201,9 @@ switch (mutation) { case "server-host-loopback": changed += "\nFor host-gateway, keep the external service listening on 127.0.0.1.\n"; break; + case "server-parent-traversal": + changed = original.replace("sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii\n", ""); + break; case "server-raw-remove": changed += "\n```sh\ndocker rm thothii-core thothii-frontend\n```\n"; break; @@ -218,6 +242,9 @@ switch (mutation) { changed = changed.slice(0, frontendAt) + clear + "\n" + clear + changed.slice(frontendAt + clear.length); break; } + case "nginx-additional-bypass": + changed = original.replace(" location / {", " location /bypass {\n proxy_pass http://127.0.0.1:8080;\n }\n\n location / {"); + break; case "caddy-no-auth": changed = original.replace("forward_auth auth-gateway:4180 {", "# forward authentication omitted"); break; @@ -333,6 +360,10 @@ expect_guide_rejected \ "server host-gateway loopback listener" verify_server_guide \ "$root/docs/install/server.md" docs/install/server.md server-host-loopback \ "server host-gateway guidance assumes a host loopback listener" +expect_guide_rejected \ + "server parent traversal boundary" verify_server_guide \ + "$root/docs/install/server.md" docs/install/server.md server-parent-traversal \ + "server installation guide does not set parent traversal boundary" expect_guide_rejected \ "server raw container removal" verify_server_guide \ "$root/docs/install/server.md" docs/install/server.md server-raw-remove \ @@ -377,6 +408,10 @@ expect_guide_rejected \ "Nginx admin clear moved out of auth scope" verify_reverse_proxy_nginx_guide \ "$root/docs/install/reverse-proxy-nginx.md" docs/install/reverse-proxy-nginx.md nginx-admin-clear-wrong-scope \ "Nginx auth location does not clear inbound admin identity" +expect_guide_rejected \ + "Nginx additional frontend bypass location" verify_reverse_proxy_nginx_guide \ + "$root/docs/install/reverse-proxy-nginx.md" docs/install/reverse-proxy-nginx.md nginx-additional-bypass \ + "Nginx frontend upstream location bypasses complete authentication contract" expect_guide_rejected \ "Caddy identity without authentication" verify_reverse_proxy_caddy_guide \ "$root/docs/install/reverse-proxy-caddy.md" docs/install/reverse-proxy-caddy.md caddy-no-auth \ diff --git a/scripts/verify-workspace-install-docs.sh b/scripts/verify-workspace-install-docs.sh index 9ae61cc1..e38576aa 100755 --- a/scripts/verify-workspace-install-docs.sh +++ b/scripts/verify-workspace-install-docs.sh @@ -547,6 +547,10 @@ verify_server_guide() { "docker compose down --volumes" \ "reverse-proxy-nginx.md" \ "reverse-proxy-caddy.md" + if ! grep -Eq '^sudo install -d -o 10001 -g thothii-ops -m 2750 /srv/thothii$' "$guide"; then + echo "server installation guide does not set parent traversal boundary" >&2 + return 1 + fi node - "$guide" <<'NODE' const fs = require("fs"); const source = fs.readFileSync(process.argv[2], "utf8"); @@ -658,19 +662,49 @@ const identities = [ ["admin", "Is-Admin", "thoth_is_admin", "x_thoth_is_admin"], ]; function escaped(value) { return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); } -function directiveBlock(text, marker) { - const start = text.indexOf(marker); - if (start < 0) throw new Error(`Nginx proxy lacks scoped block: ${marker}`); - const opening = text.indexOf("{", start); - let depth = 0; - for (let index = opening; index < text.length; index++) { - if (text[index] === "{") depth++; - if (text[index] === "}" && --depth === 0) return text.slice(opening + 1, index); +function nginxLocations(text) { + const locations = []; + const pattern = /\blocation\s+([^\n{]+)\{/g; + for (const match of text.matchAll(pattern)) { + const opening = match.index + match[0].lastIndexOf("{"); + let depth = 0; + let closing = -1; + for (let index = opening; index < text.length; index++) { + if (text[index] === "{") depth++; + if (text[index] === "}" && --depth === 0) { + closing = index; + break; + } + } + if (closing < 0) throw new Error(`Nginx proxy has unterminated location: ${match[1].trim()}`); + locations.push({selector: match[1].trim(), body: text.slice(opening + 1, closing)}); + } + return locations; +} +const locations = nginxLocations(block); +const authLocations = locations.filter((location) => location.selector === "= /_authenticate"); +if (authLocations.length !== 1) { + throw new Error("Nginx proxy must define exactly one authentication location"); +} +const authLocation = authLocations[0].body; +const frontendLocations = locations.filter((location) => + /proxy_pass\s+http:\/\/127\.0\.0\.1:8080\s*;/.test(location.body)); +if (frontendLocations.length === 0) { + throw new Error("Nginx proxy lacks a frontend upstream location"); +} +for (const location of locations) { + const upstreams = [...location.body.matchAll(/proxy_pass\s+([^;]+);/g)].map((match) => match[1].trim()); + for (const upstream of upstreams) { + if (location.selector === "= /_authenticate" && upstream === "http://auth-gateway:4180/verify") continue; + if (upstream === "http://127.0.0.1:8080") continue; + throw new Error(`Nginx location proxies to an unreviewed upstream: ${upstream}`); + } +} +for (const frontendLocation of frontendLocations) { + if (!/auth_request\s+\/_authenticate\s*;/.test(frontendLocation.body)) { + throw new Error("Nginx frontend upstream location bypasses complete authentication contract"); } - throw new Error(`Nginx proxy has unterminated scoped block: ${marker}`); } -const authLocation = directiveBlock(block, "location = /_authenticate {"); -const frontendLocation = directiveBlock(block, "location / {"); for (const [label, publicName, variable, upstream] of identities) { const trustedName = publicName === "Is-Admin" ? "Is-Admin" : publicName; const publicClear = new RegExp(`proxy_set_header\\s+X-Thoth-${escaped(publicName)}\\s+"";`); @@ -684,18 +718,20 @@ for (const [label, publicName, variable, upstream] of identities) { if (authTrustedAt < 0) { throw new Error(`Nginx auth location does not clear inbound trusted ${label} identity`); } - const frontendPublicAt = frontendLocation.search(publicClear); - if (frontendPublicAt < 0) { - throw new Error(`Nginx frontend location does not clear inbound ${label} identity`); - } - const normalizedFrontend = frontendLocation.replace(/\s+/g, " "); - const captureAt = normalizedFrontend.search(new RegExp(`auth_request_set\\s+\\$${variable}\\s+\\$upstream_http_${upstream};`)); - if (captureAt < 0) { - throw new Error(`Nginx frontend location does not capture authenticated ${label} identity`); - } - const mapAt = normalizedFrontend.search(new RegExp(`proxy_set_header\\s+${escaped(trustedHeader)}\\s+\\$${variable};`)); - if (mapAt < 0) { - throw new Error(`Nginx frontend location does not map authenticated ${label} identity`); + for (const frontendLocation of frontendLocations) { + const frontendPublicAt = frontendLocation.body.search(publicClear); + if (frontendPublicAt < 0) { + throw new Error(`Nginx frontend location does not clear inbound ${label} identity`); + } + const normalizedFrontend = frontendLocation.body.replace(/\s+/g, " "); + const captureAt = normalizedFrontend.search(new RegExp(`auth_request_set\\s+\\$${variable}\\s+\\$upstream_http_${upstream};`)); + if (captureAt < 0) { + throw new Error(`Nginx frontend location does not capture authenticated ${label} identity`); + } + const mapAt = normalizedFrontend.search(new RegExp(`proxy_set_header\\s+${escaped(trustedHeader)}\\s+\\$${variable};`)); + if (mapAt < 0) { + throw new Error(`Nginx frontend location does not map authenticated ${label} identity`); + } } if (new RegExp(`auth_request_set\\s+\\$${variable}|proxy_set_header\\s+${escaped(trustedHeader)}\\s+\\$${variable};`).test(authLocation)) { throw new Error(`Nginx auth location performs a forbidden ${label} capture or mapping`); @@ -801,22 +837,6 @@ function frontendUpstream(handler) { return handler?.handler === "reverse_proxy" && (handler.upstreams || []).some((upstream) => upstream.dial === "127.0.0.1:8080"); } -function findHandlerArray(value) { - if (!value || typeof value !== "object") return null; - if (Array.isArray(value)) { - if (value.some(authUpstream) && value.some(frontendUpstream)) return value; - for (const child of value) { - const found = findHandlerArray(child); - if (found) return found; - } - return null; - } - for (const child of Object.values(value)) { - const found = findHandlerArray(child); - if (found) return found; - } - return null; -} function collectTrustedSets(value, collected = new Map()) { if (!value || typeof value !== "object") return collected; if (value.handler === "headers") { @@ -827,36 +847,73 @@ function collectTrustedSets(value, collected = new Map()) { for (const child of Object.values(value)) collectTrustedSets(child, collected); return collected; } -const handlers = findHandlerArray(document); -if (!handlers) throw new Error("Caddy adapted config lacks the ordered auth/frontend handler chain"); -const authAt = handlers.findIndex(authUpstream); -const frontendAt = handlers.findIndex(frontendUpstream); -if (authAt < 0 || frontendAt <= authAt) throw new Error("Caddy adapted auth/frontend handler order is invalid"); const expectedClears = [...publicHeaders, ...trustedHeaders]; -for (const header of expectedClears) { - const clearAt = handlers.findIndex((handler) => - handler?.handler === "headers" && (handler.request?.delete || []).includes(header)); - if (clearAt < 0 || clearAt >= authAt) { - throw new Error("Caddy adapted identity clears must execute before authentication"); +function validateAuthenticatedMappings(auth) { + const successResponse = (auth.handle_response || []).find((response) => + (response.match?.status_code || []).map(Number).includes(2)); + if (!successResponse) throw new Error("Caddy adapted identity mapping is not restricted to auth 2xx"); + const mappings = collectTrustedSets(successResponse); + for (let index = 0; index < trustedHeaders.length; index++) { + const replacement = mappings.get(trustedHeaders[index]); + const expected = `{http.reverse_proxy.header.${publicHeaders[index]}}`; + if (!Array.isArray(replacement) || replacement.length !== 1 || replacement[0] !== expected) { + throw new Error(`Caddy adapted authenticated mapping is invalid for ${trustedHeaders[index]}`); + } } } -const auth = handlers[authAt]; -const successResponse = (auth.handle_response || []).find((response) => - (response.match?.status_code || []).map(Number).includes(2)); -if (!successResponse) throw new Error("Caddy adapted identity mapping is not restricted to auth 2xx"); -const mappings = collectTrustedSets(successResponse); -for (let index = 0; index < trustedHeaders.length; index++) { - const replacement = mappings.get(trustedHeaders[index]); - const expected = `{http.reverse_proxy.header.${publicHeaders[index]}}`; - if (!Array.isArray(replacement) || replacement.length !== 1 || replacement[0] !== expected) { - throw new Error(`Caddy adapted authenticated mapping is invalid for ${trustedHeaders[index]}`); +function validateFrontendPath(handlers) { + let authAt = -1; + for (let index = handlers.length - 1; index >= 0; index--) { + if (authUpstream(handlers[index])) { + authAt = index; + break; + } + } + if (authAt < 0) { + throw new Error("Caddy adapted frontend path bypasses complete authentication contract"); + } + for (const header of expectedClears) { + const clearAt = handlers.findIndex((handler) => + handler?.handler === "headers" && (handler.request?.delete || []).includes(header)); + if (clearAt < 0 || clearAt >= authAt) { + throw new Error("Caddy adapted identity clears must execute before authentication"); + } + } + validateAuthenticatedMappings(handlers[authAt]); + for (let index = 0; index < handlers.length; index++) { + if (index !== authAt && collectTrustedSets(handlers[index]).size !== 0) { + throw new Error("Caddy adapted config maps trusted identity outside auth success"); + } } } -for (let index = 0; index < handlers.length; index++) { - if (index === authAt) continue; - if (collectTrustedSets(handlers[index]).size !== 0) { - throw new Error("Caddy adapted config maps trusted identity outside auth success"); +let frontendPaths = 0; +function walk(value, inherited = []) { + if (!value || typeof value !== "object") return; + if (Array.isArray(value)) { + for (const child of value) walk(child, inherited); + return; } + if (frontendUpstream(value)) { + frontendPaths++; + validateFrontendPath(inherited); + } + if (Array.isArray(value.handle)) { + const previous = []; + for (const handler of value.handle) { + walk(handler, [...inherited, ...previous]); + previous.push(handler); + } + for (const [key, child] of Object.entries(value)) { + if (key !== "handle") walk(child, inherited); + } + return; + } + const childContext = authUpstream(value) ? [...inherited, value] : inherited; + for (const child of Object.values(value)) walk(child, childContext); +} +walk(document); +if (frontendPaths === 0) { + throw new Error("Caddy adapted config lacks a frontend handler path"); } NODE } diff --git a/tools/thothctl/cmd/thothctl/main.go b/tools/thothctl/cmd/thothctl/main.go index e0fb69dd..7fb6772e 100644 --- a/tools/thothctl/cmd/thothctl/main.go +++ b/tools/thothctl/cmd/thothctl/main.go @@ -175,7 +175,7 @@ func serverOperationFailure(stderr io.Writer, err error, secretValues []string) message := output.Sanitize(err.Error(), secretValues) var operationErr *serverops.OperationError if errors.As(err, &operationErr) && operationErr.Detail() != "" { - detail := output.Sanitize(operationErr.Detail(), secretValues) + detail := output.SanitizeDetail(operationErr.Detail(), secretValues) fmt.Fprintf(stderr, "thothctl: %s: %s\n", message, detail) } else { fmt.Fprintf(stderr, "thothctl: %s\n", message) diff --git a/tools/thothctl/cmd/thothctl/main_test.go b/tools/thothctl/cmd/thothctl/main_test.go index cbdeba96..5ecf540a 100644 --- a/tools/thothctl/cmd/thothctl/main_test.go +++ b/tools/thothctl/cmd/thothctl/main_test.go @@ -104,33 +104,59 @@ func TestRunSessionsMigrateRequiresExplicitConfirmationBeforeDocker(t *testing.T assertDockerNotInvoked(t, fixture) } -func TestRunSessionsMigrateReportsSanitizedStageAndExitClass(t *testing.T) { - fixture := newCLIFixture(t, "MIGRATION_PASSWORD_FILE=%s\n") - fixture.setProfile(t, "server") - secretPath := filepath.Join(fixture.root, "migration-password") - if err := os.WriteFile(secretPath, []byte("migration-secret-value"), 0o600); err != nil { - t.Fatal(err) - } - fixture.setEnvironment(t, secretPath) - t.Setenv("THOTHCTL_FAKE_CONFIG", `{"services":{"core":{"image":"thothii-core:local"},"session-migrate":{"image":"thothii-core:local"}}}`) - t.Setenv("THOTHCTL_FAKE_MIGRATION_FAILURE", "TLS rejected migration-secret-value") - t.Setenv("THOTHCTL_FAKE_MIGRATION_EXIT", "23") - var stdout, stderr bytes.Buffer +func TestRunSessionsMigrateRedactsCompleteDetailBeforeDisplayBound(t *testing.T) { + longSecret := "long-secret-" + strings.Repeat("s", 700) + for _, spec := range []struct { + name string + secret string + failure string + leakedProbe string + }{ + { + name: "secret longer than display limit", + secret: longSecret, + failure: longSecret + " rejected by TLS", + leakedProbe: longSecret[:64], + }, + { + name: "secret crossing display boundary", + secret: "boundary-secret-value", + failure: strings.Repeat("p", 500) + "boundary-secret-value rejected", + leakedProbe: "boundary-sec", + }, + } { + t.Run(spec.name, func(t *testing.T) { + fixture := newCLIFixture(t, "MIGRATION_PASSWORD_FILE=%s\n") + fixture.setProfile(t, "server") + secretPath := filepath.Join(fixture.root, "migration-password") + if err := os.WriteFile(secretPath, []byte(spec.secret), 0o600); err != nil { + t.Fatal(err) + } + fixture.setEnvironment(t, secretPath) + t.Setenv("THOTHCTL_FAKE_CONFIG", `{"services":{"core":{"image":"thothii-core:local"},"session-migrate":{"image":"thothii-core:local"}}}`) + t.Setenv("THOTHCTL_FAKE_MIGRATION_FAILURE", spec.failure) + t.Setenv("THOTHCTL_FAKE_MIGRATION_EXIT", "23") + var stdout, stderr bytes.Buffer - code := run(context.Background(), []string{ - "--installation", fixture.installationPath, "sessions", "migrate", "--yes", - }, &stdout, &stderr) + code := run(context.Background(), []string{ + "--installation", fixture.installationPath, "sessions", "migrate", "--yes", + }, &stdout, &stderr) - if code != 1 { - t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) - } - for _, required := range []string{"stage=session-migration", "class=nonzero-exit", "TLS rejected [REDACTED]"} { - if !strings.Contains(stderr.String(), required) { - t.Errorf("stderr = %q, missing %q", stderr.String(), required) - } - } - if strings.Contains(stderr.String(), "migration-secret-value") { - t.Fatalf("stderr exposed secret: %q", stderr.String()) + if code != 1 { + t.Fatalf("exit = %d, stderr = %q", code, stderr.String()) + } + for _, required := range []string{"stage=session-migration", "class=nonzero-exit", "[REDACTED]"} { + if !strings.Contains(stderr.String(), required) { + t.Errorf("stderr = %q, missing %q", stderr.String(), required) + } + } + if strings.Contains(stderr.String(), spec.secret) || strings.Contains(stderr.String(), spec.leakedProbe) { + t.Fatalf("stderr exposed secret or prefix: %q", stderr.String()) + } + if stderr.Len() > 640 { + t.Fatalf("stderr exceeded bounded display: %d bytes", stderr.Len()) + } + }) } } diff --git a/tools/thothctl/internal/output/sanitize.go b/tools/thothctl/internal/output/sanitize.go index 6295715e..9c360fa0 100644 --- a/tools/thothctl/internal/output/sanitize.go +++ b/tools/thothctl/internal/output/sanitize.go @@ -18,6 +18,8 @@ const maxSecretSourceFiles = 32 const maxSecretSourceBytes = 256 * 1024 +const maxDiagnosticDetailBytes = 512 + // Sanitize redacts common credential fields and every supplied secret value. func Sanitize(text string, secretValues []string) string { text = credentialField.ReplaceAllString(text, "${1}[REDACTED]") @@ -31,6 +33,24 @@ func Sanitize(text string, secretValues []string) string { return text } +// SanitizeDetail redacts the complete subprocess detail before normalizing and bounding the text +// that may be displayed at the CLI boundary. +func SanitizeDetail(text string, secretValues []string) string { + detail := strings.Join(strings.Fields(Sanitize(text, secretValues)), " ") + if len(detail) <= maxDiagnosticDetailBytes { + return detail + } + var bounded strings.Builder + for _, character := range detail { + encoded := string(character) + if bounded.Len()+len(encoded) > maxDiagnosticDetailBytes { + break + } + bounded.WriteString(encoded) + } + return bounded.String() +} + // SecretValuesFromFiles reads non-empty secret-file contents without exposing them to callers. func SecretValuesFromFiles(paths []string) ([]string, error) { if len(paths) > maxSecretSourceFiles { diff --git a/tools/thothctl/internal/serverops/operations.go b/tools/thothctl/internal/serverops/operations.go index ed2d77ff..67cbed10 100644 --- a/tools/thothctl/internal/serverops/operations.go +++ b/tools/thothctl/internal/serverops/operations.go @@ -45,8 +45,8 @@ const ( ExitClassInvocation ExitClass = "invocation-failure" ) -// OperationError reports only allowlisted operation metadata from Error. Detail remains bounded -// and is exposed separately so the CLI can redact declared secrets before displaying it. +// OperationError reports only allowlisted operation metadata from Error. Complete subprocess +// detail is exposed separately so the CLI can redact it before applying its display bound. type OperationError struct { stage Stage class ExitClass @@ -384,24 +384,7 @@ func runDocker(ctx context.Context, runner Runner, stage Stage, args []string) ( if strings.TrimSpace(detail) == "" { detail = err.Error() } - return result, &OperationError{stage: stage, class: class, detail: boundedDetail(detail)} + return result, &OperationError{stage: stage, class: class, detail: detail} } return result, nil } - -func boundedDetail(detail string) string { - detail = strings.Join(strings.Fields(detail), " ") - const maximumBytes = 512 - if len(detail) <= maximumBytes { - return detail - } - var bounded strings.Builder - for _, character := range detail { - encoded := string(character) - if bounded.Len()+len(encoded) > maximumBytes { - break - } - bounded.WriteString(encoded) - } - return bounded.String() -} diff --git a/tools/thothctl/internal/serverops/operations_test.go b/tools/thothctl/internal/serverops/operations_test.go index 11126a9f..d1f6906f 100644 --- a/tools/thothctl/internal/serverops/operations_test.go +++ b/tools/thothctl/internal/serverops/operations_test.go @@ -131,38 +131,52 @@ func TestMigrateSessionsFailsClosedBeforeMutation(t *testing.T) { } } -func TestMigrateSessionsReturnsTypedBoundedRunFailure(t *testing.T) { - installation := testInstallation(t) - secret := "database-password-in-stderr" - configCalls := 0 - runner := &fakeRunner{run: func(args []string) (compose.Result, error) { - switch { - case contains(args, "ps", "--all"): - return compose.Result{Stdout: `[]`}, nil - case contains(args, "config", "--format", "json"): - configCalls++ - return compose.Result{Stdout: `{"services":{"core":{"image":"thothii-core:local"},"session-migrate":{"image":"thothii-core:local"}}}`}, nil - case contains(args, "run", "--rm", "--no-deps", "--no-TTY", "session-migrate"): - return compose.Result{Stderr: "TLS connection for " + secret + ": " + strings.Repeat("x", 2048), ExitCode: 23}, errors.New("exit status 23") - default: - t.Fatalf("unexpected Docker invocation: %#v", args) - return compose.Result{}, nil - } - }} +func TestMigrateSessionsPreservesCompleteFailureDetailBehindTypedMetadata(t *testing.T) { + longSecret := "long-secret-" + strings.Repeat("s", 700) + for _, spec := range []struct { + name string + secret string + stderr string + }{ + {name: "secret longer than display limit", secret: longSecret, stderr: longSecret + " rejected"}, + {name: "secret crossing display boundary", secret: "boundary-secret-value", stderr: strings.Repeat("p", 500) + "boundary-secret-value rejected"}, + } { + t.Run(spec.name, func(t *testing.T) { + installation := testInstallation(t) + configCalls := 0 + runner := &fakeRunner{run: func(args []string) (compose.Result, error) { + switch { + case contains(args, "ps", "--all"): + return compose.Result{Stdout: `[]`}, nil + case contains(args, "config", "--format", "json"): + configCalls++ + return compose.Result{Stdout: `{"services":{"core":{"image":"thothii-core:local"},"session-migrate":{"image":"thothii-core:local"}}}`}, nil + case contains(args, "run", "--rm", "--no-deps", "--no-TTY", "session-migrate"): + return compose.Result{Stderr: spec.stderr, ExitCode: 23}, errors.New("exit status 23") + default: + t.Fatalf("unexpected Docker invocation: %#v", args) + return compose.Result{}, nil + } + }} - _, err := MigrateSessions(context.Background(), installation, runner, true) - var operationErr *OperationError - if !errors.As(err, &operationErr) { - t.Fatalf("MigrateSessions() error = %T %v, want OperationError", err, err) - } - if operationErr.Stage() != StageSessionMigration || operationErr.Class() != ExitClassNonzero { - t.Fatalf("operation error = %#v", operationErr) - } - if detail := operationErr.Detail(); !strings.Contains(detail, secret) || len(detail) > 512 { - t.Fatalf("bounded detail length=%d value=%q", len(detail), detail) - } - if configCalls != 2 { - t.Fatalf("config calls = %d", configCalls) + _, err := MigrateSessions(context.Background(), installation, runner, true) + var operationErr *OperationError + if !errors.As(err, &operationErr) { + t.Fatalf("MigrateSessions() error = %T %v, want OperationError", err, err) + } + if operationErr.Stage() != StageSessionMigration || operationErr.Class() != ExitClassNonzero { + t.Fatalf("operation error = %#v", operationErr) + } + if strings.Contains(operationErr.Error(), spec.secret[:12]) { + t.Fatalf("typed metadata exposed secret prefix: %q", operationErr.Error()) + } + if detail := operationErr.Detail(); detail != spec.stderr || !strings.Contains(detail, spec.secret) { + t.Fatalf("detail was truncated before redaction: length=%d", len(detail)) + } + if configCalls != 2 { + t.Fatalf("config calls = %d", configCalls) + } + }) } }