From 0d0c15b4b8b97d24aaccad7cf8f2719f475b2aac Mon Sep 17 00:00:00 2001 From: mptyl Date: Mon, 17 Aug 2026 22:15:41 +0200 Subject: [PATCH] fix(auth): complete Task 13 deployment review --- backend/test/auth.test.ts | 33 +++------- scripts/test-unified-compose.sh | 12 +++- scripts/unified-deployment-smoke.sh | 6 +- tools/tht/internal/backup/create.go | 38 +++++++---- tools/tht/internal/backup/create_test.go | 73 +++++++++++++++++++-- tools/tht/internal/backup/preflight.go | 7 ++ tools/tht/internal/backup/preflight_test.go | 54 +++++++++++++++ tools/tht/internal/backup/restore_host.go | 2 +- tools/tht/internal/backup/restore_test.go | 13 ++++ tools/tht/internal/pi/update.go | 7 +- tools/tht/internal/pi/update_test.go | 33 ++++++++++ 11 files changed, 228 insertions(+), 50 deletions(-) diff --git a/backend/test/auth.test.ts b/backend/test/auth.test.ts index dcbd1ac1..3bd5332c 100644 --- a/backend/test/auth.test.ts +++ b/backend/test/auth.test.ts @@ -8,33 +8,18 @@ import { expandLocalHome, localPrincipal, upstreamPrincipal } from "../src/auth/ import { buildApp } from "../src/app.js"; import { loadConfig } from "../src/config.js"; -test("server smoke trusted claims transform through nginx to a non-admin principal", () => { +test("server smoke rejects retired trusted claims under OIDC authentication", () => { const smoke = readFileSync("../scripts/unified-deployment-smoke.sh", "utf8"); - const nginx = readFileSync("../docker/nginx.conf.template", "utf8"); - const helper = smoke.match(/task13_server_auth_headers\(\) \{([\s\S]*?)\n\}/)?.[1] ?? ""; - const trusted = Object.fromEntries( - [...helper.matchAll(/-H '([^:']+): ([^']+)'/g)].map((match) => [match[1].toLowerCase(), match[2]]), - ); - const normalized: Record = {}; - for (const [header, suffix] of [ - ["x-thoth-principal-issuer", "principal_issuer"], - ["x-thoth-principal-subject", "principal_subject"], - ["x-thoth-principal-display-name", "principal_display_name"], - ["x-thoth-is-admin", "is_admin"], + for (const header of [ + "x-thoth-trusted-principal-issuer", + "x-thoth-trusted-principal-subject", + "x-thoth-trusted-principal-display-name", + "x-thoth-trusted-is-admin", ]) { - expect(nginx).toContain(`$http_x_thoth_trusted_${suffix}`); - const value = trusted[`x-thoth-trusted-${header.slice("x-thoth-".length)}`]; - if (value !== undefined) normalized[header] = value; + expect(smoke).toContain(`-H '${header}:`); } - - expect(upstreamPrincipal(normalized)).toEqual({ - issuer: "task13-proxy", - subject: "task13-user", - displayName: "Task 13 User", - roles: ["user"], - permissions: ["session.use"], - isAdmin: false, - }); + expect(smoke).toContain('[[ "$trusted_header_status" == 401 ]]'); + expect(smoke).toContain("server accepted retired trusted identity headers"); }); test("local mode resolves a stable local principal", async () => { diff --git a/scripts/test-unified-compose.sh b/scripts/test-unified-compose.sh index 41f6651c..e5e835c0 100755 --- a/scripts/test-unified-compose.sh +++ b/scripts/test-unified-compose.sh @@ -68,11 +68,20 @@ for (const serviceName of ["embedding", "embedding-model-init"]) { throw new Error(`${serviceName} image must be pinned by version and digest`); } } -for (const serviceName of ["qdrant", "embedding", "embedding-model-init"]) { +for (const serviceName of ["embedding", "embedding-model-init"]) { if ((config.services[serviceName].ports || []).length !== 0) { throw new Error(`${serviceName} must not publish a host port`); } } +const qdrantPorts = config.services.qdrant.ports || []; +if (profile === "local") { + if (qdrantPorts.length !== 1 || qdrantPorts[0].host_ip !== "127.0.0.1" + || Number(qdrantPorts[0].published) !== 6333 || Number(qdrantPorts[0].target) !== 6333) { + throw new Error("local qdrant may publish only 127.0.0.1:6333"); + } +} else if (qdrantPorts.length !== 0) { + throw new Error("server qdrant must not publish a host port"); +} if ((config.services.qdrant.expose || []).join(",") !== "6333") throw new Error("qdrant must expose only 6333"); if ((config.services.embedding.expose || []).join(",") !== "11434") throw new Error("embedding must expose only 11434"); if (!config.services.qdrant.healthcheck) throw new Error("qdrant must define a healthcheck"); @@ -155,6 +164,7 @@ assert_remote_required() { THT_WORKSPACE_GIT_REMOTE=https://git.example.invalid/platform/thoth-workspaces.git \ PI_AUTH_FILE=/dev/null \ THT_SECRETS_FILE=/dev/null \ +THT_AUTH_CONFIG_ROOT=/tmp/thothii-auth \ docker compose -f compose.yaml config --format json >"$tmp/base.json" node - "$tmp/base.json" <<'NODE' const fs = require("fs"); diff --git a/scripts/unified-deployment-smoke.sh b/scripts/unified-deployment-smoke.sh index 636d40b2..8370e1a2 100755 --- a/scripts/unified-deployment-smoke.sh +++ b/scripts/unified-deployment-smoke.sh @@ -428,7 +428,7 @@ EOF chmod 0600 "$TASK13_OIDC_KEY" chmod 0644 "$TASK13_OIDC_CERT" cat >"$TASK13_OIDC_SERVER" <<'EOF' -import { createPublicKey, generateKeyPairSync } from "node:crypto"; +import { generateKeyPairSync } from "node:crypto"; import { readFileSync } from "node:fs"; import https from "node:https"; @@ -436,7 +436,7 @@ const origin = "https://task13-fake-oidc:9443"; const issuer = `${origin}/application/o/task13/`; const expectedToken = process.env.TASK13_AUTHENTIK_API_TOKEN; const { publicKey } = generateKeyPairSync("rsa", { modulusLength: 2048 }); -const jwk = { ...createPublicKey(publicKey).export({ format: "jwk" }), kid: "task13", use: "sig", alg: "RS256" }; +const jwk = { ...publicKey.export({ format: "jwk" }), kid: "task13", use: "sig", alg: "RS256" }; const send = (response, status, body) => { const payload = JSON.stringify(body); response.writeHead(status, { "content-type": "application/json", "content-length": Buffer.byteLength(payload) }); @@ -1104,7 +1104,7 @@ task13_assert_server_runtime() { status="$TASK13_TMP/server-auth-status.json" task13_run_logged "server static OIDC status" "$TASK13_THT" --installation "$TASK13_INSTALLATION" auth status --json "$TASK13_THT" --installation "$TASK13_INSTALLATION" auth status --json >"$status" - node -e 'const value=JSON.parse(require("fs").readFileSync(process.argv[1], "utf8")); if(value.mode!=="oidc"||typeof value.configRevision!=="string"||value.configRevision.length!==64) process.exit(1)' "$status" \ + node -e 'const value=JSON.parse(require("fs").readFileSync(process.argv[1], "utf8")); if(value.mode!=="oidc"||!/^sha256:[0-9a-f]{64}$/.test(value.configRevision)) process.exit(1)' "$status" \ || task13_fail "server static OIDC status was not valid" diagnostics="$TASK13_TMP/server-auth-diagnostics.json" task13_run_logged "server live OIDC diagnostics" "$TASK13_THT" --installation "$TASK13_INSTALLATION" auth check --json diff --git a/tools/tht/internal/backup/create.go b/tools/tht/internal/backup/create.go index 86acf36e..1d7f9ce9 100644 --- a/tools/tht/internal/backup/create.go +++ b/tools/tht/internal/backup/create.go @@ -394,7 +394,8 @@ type renderedCompose struct { Name string `json:"name"` } `json:"volumes"` Services map[string]struct { - Image string `json:"image"` + Image string `json:"image"` + ContainerName string `json:"container_name"` } `json:"services"` } @@ -474,18 +475,13 @@ func imageIdentities(ctx context.Context, installation config.Installation, runn if err != nil { return nil, err } - matching := make([]composeImageIdentity, 0, len(inspected)) - for _, item := range inspected { - if item.Service == "" || item.Service == name { - matching = append(matching, item) - } + expectedContainer := rendered.Services[name].ContainerName + if expectedContainer == "" { + expectedContainer = installation.ProjectName() + "-" + name + "-1" } - if len(matching) > 1 { - return nil, errors.New("Docker Compose returned invalid image identities") - } - id := "" - if len(matching) == 1 { - id = matching[0].ID + id, err := selectComposeImageIdentity(name, expectedContainer, inspected) + if err != nil { + return nil, err } images = append(images, ImageIdentity{Service: name, Reference: rendered.Services[name].Image, ID: id}) } @@ -500,7 +496,7 @@ type composeImageIdentity struct { func decodeComposeImageIdentities(value string) ([]composeImageIdentity, error) { trimmed := strings.TrimSpace(value) - if trimmed == "" { + if trimmed == "" || trimmed == "null" { return nil, nil } var identities []composeImageIdentity @@ -530,6 +526,22 @@ func decodeComposeImageIdentities(value string) ([]composeImageIdentity, error) return identities, nil } +func selectComposeImageIdentity(service string, expectedContainer string, identities []composeImageIdentity) (string, error) { + if len(identities) == 0 { + return "", nil + } + matching := make([]composeImageIdentity, 0, len(identities)) + for _, item := range identities { + if item.Service == service || item.Service == "" && item.ContainerName == expectedContainer { + matching = append(matching, item) + } + } + if len(matching) != 1 { + return "", errors.New("Docker Compose returned invalid image identities") + } + return matching[0].ID, nil +} + func installationRunning(ctx context.Context, installation config.Installation, runner archiveRunner) (bool, error) { result, err := runner.Run(ctx, installation.ComposeArgs("ps", "--all", "--format", "json"), nil) if err != nil { diff --git a/tools/tht/internal/backup/create_test.go b/tools/tht/internal/backup/create_test.go index 2d9bd9f1..67957540 100644 --- a/tools/tht/internal/backup/create_test.go +++ b/tools/tht/internal/backup/create_test.go @@ -23,7 +23,7 @@ import ( var requiredTestVolumes = []string{"settings", "pi-state", "workspace-registry", "workspace-secrets", "sessions", "qdrant-data", "embedding-models"} -func TestDecodeComposeImageIdentitiesAcceptsArrayAndStreamingJSON(t *testing.T) { +func TestDecodeComposeImageIdentitiesAcceptsNonEmptyArrayAndStreamingJSON(t *testing.T) { for name, input := range map[string]string{ "array": `[{"ContainerName":"project-core-1","ID":"sha256:core"},{"ContainerName":"project-frontend-1","ID":"sha256:frontend"}]`, "streaming": "{\"Service\":\"core\",\"ID\":\"sha256:core\"}\n{\"Service\":\"frontend\",\"ID\":\"sha256:frontend\"}\n", @@ -41,11 +41,72 @@ func TestDecodeComposeImageIdentitiesAcceptsArrayAndStreamingJSON(t *testing.T) } func TestDecodeComposeImageIdentitiesAcceptsNoContainerForProfiledService(t *testing.T) { - for _, input := range []string{"", "[]"} { - identities, err := decodeComposeImageIdentities(input) - if err != nil || len(identities) != 0 { - t.Fatalf("decodeComposeImageIdentities(%q) = %#v, %v", input, identities, err) - } + for name, input := range map[string]string{ + "empty output": "", + "empty array": "[]", + "null": "null", + } { + t.Run(name, func(t *testing.T) { + identities, err := decodeComposeImageIdentities(input) + if err != nil || len(identities) != 0 { + t.Fatalf("decodeComposeImageIdentities(%q) = %#v, %v", input, identities, err) + } + }) + } +} + +func TestDecodeComposeImageIdentitiesRejectsMalformedOrIncompleteOutput(t *testing.T) { + for name, input := range map[string]string{ + "malformed JSON": "[", + "scalar JSON": "true", + "empty object": "{}", + "null array element": "[null]", + "missing image ID": `[{"Service":"core"}]`, + "trailing document": "null\n{}", + "trailing malformed bytes": `null garbage`, + } { + t.Run(name, func(t *testing.T) { + if identities, err := decodeComposeImageIdentities(input); err == nil { + t.Fatalf("decodeComposeImageIdentities(%q) = %#v, nil; want error", input, identities) + } + }) + } +} + +func TestSelectComposeImageIdentityUsesExactComposeContainerWhenServiceIsAbsent(t *testing.T) { + identities := []composeImageIdentity{ + {ContainerName: "project-core-1", ID: "sha256:core"}, + {ContainerName: "project-llm", ID: "sha256:shared-image"}, + } + id, err := selectComposeImageIdentity("core", "project-core-1", identities) + if err != nil { + t.Fatal(err) + } + if id != "sha256:core" { + t.Fatalf("selected image ID = %q, want sha256:core", id) + } +} + +func TestSelectComposeImageIdentityDoesNotFailOpen(t *testing.T) { + for name, identities := range map[string][]composeImageIdentity{ + "unrelated container only": {{ContainerName: "project-llm", ID: "sha256:shared-image"}}, + "duplicate service": { + {Service: "core", ID: "sha256:first"}, + {Service: "core", ID: "sha256:second"}, + }, + } { + t.Run(name, func(t *testing.T) { + if id, err := selectComposeImageIdentity("core", "project-core-1", identities); err == nil { + t.Fatalf("selectComposeImageIdentity() = %q, nil; want error", id) + } + }) + } +} + +func TestSelectComposeImageIdentityAcceptsSemanticallyEmptyOutput(t *testing.T) { + id, err := selectComposeImageIdentity("workspace-maintenance", "project-workspace-maintenance-1", nil) + if err != nil || id != "" { + t.Fatalf("selectComposeImageIdentity() = %q, %v; want empty identity", id, err) } } diff --git a/tools/tht/internal/backup/preflight.go b/tools/tht/internal/backup/preflight.go index e5658b26..a8169582 100644 --- a/tools/tht/internal/backup/preflight.go +++ b/tools/tht/internal/backup/preflight.go @@ -572,6 +572,13 @@ func validateVolumeTar(ctx context.Context, member *zip.File) error { return fmt.Errorf("read volume archive: %w", err) } name := strings.TrimSuffix(header.Name, "/") + if name == "." { + if header.Typeflag != tar.TypeDir { + return errors.New("volume archive root marker is not a directory") + } + continue + } + name = strings.TrimPrefix(name, "./") if _, err := validateArchiveMemberPath(name); err != nil { return fmt.Errorf("volume archive path is unsafe: %w", err) } diff --git a/tools/tht/internal/backup/preflight_test.go b/tools/tht/internal/backup/preflight_test.go index 8b9f6f18..c3505f74 100644 --- a/tools/tht/internal/backup/preflight_test.go +++ b/tools/tht/internal/backup/preflight_test.go @@ -233,6 +233,60 @@ func TestPreflightRejectsTraversalAndSymlinkInsideVolumeTar(t *testing.T) { } } +func TestPreflightAcceptsCanonicalTarRootDirectoryAndRelativeMembers(t *testing.T) { + installation := preflightTestInstallation(t) + var payload strings.Builder + writer := tar.NewWriter(&stringWriter{value: &payload}) + for _, header := range []tar.Header{ + {Name: "./", Mode: 0o755, Typeflag: tar.TypeDir}, + {Name: "./payload", Mode: 0o600, Size: 1, Typeflag: tar.TypeReg}, + } { + if err := writer.WriteHeader(&header); err != nil { + t.Fatal(err) + } + if header.Size > 0 { + if _, err := writer.Write([]byte("x")); err != nil { + t.Fatal(err) + } + } + } + if err := writer.Close(); err != nil { + t.Fatal(err) + } + archive := filepath.Join(t.TempDir(), "canonical-volume.zip") + writePreflightArchive(t, archive, preflightArchiveSpec{entries: []preflightArchiveEntry{{ + path: "volumes/sessions.tar", body: []byte(payload.String()), kind: EntryVolume, + }}}) + result, err := Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true}, permissivePreflightDependencies()) + if err != nil { + t.Fatal(err) + } + result.CloseArchive() +} + +func TestPreflightRejectsNonDirectoryTarRootMarker(t *testing.T) { + installation := preflightTestInstallation(t) + var payload strings.Builder + writer := tar.NewWriter(&stringWriter{value: &payload}) + header := tar.Header{Name: ".", Mode: 0o600, Size: 1, Typeflag: tar.TypeReg} + if err := writer.WriteHeader(&header); err != nil { + t.Fatal(err) + } + if _, err := writer.Write([]byte("x")); err != nil { + t.Fatal(err) + } + if err := writer.Close(); err != nil { + t.Fatal(err) + } + archive := filepath.Join(t.TempDir(), "unsafe-root-volume.zip") + writePreflightArchive(t, archive, preflightArchiveSpec{entries: []preflightArchiveEntry{{ + path: "volumes/sessions.tar", body: []byte(payload.String()), kind: EntryVolume, + }}}) + if _, err := Preflight(context.Background(), installation, PreflightRequest{Archive: archive, Confirm: true}, permissivePreflightDependencies()); err == nil { + t.Fatal("Preflight accepted a non-directory TAR root marker") + } +} + func TestPreflightRejectsArchivesThatExceedConfiguredProcessingLimits(t *testing.T) { installation := preflightTestInstallation(t) tests := []struct { diff --git a/tools/tht/internal/backup/restore_host.go b/tools/tht/internal/backup/restore_host.go index 495161b3..e970992b 100644 --- a/tools/tht/internal/backup/restore_host.go +++ b/tools/tht/internal/backup/restore_host.go @@ -285,7 +285,7 @@ func safeRestoreParent(target string) bool { func volumeRestoreCommand(volume string) []string { return []string{ - "run", "--rm", "--network", "none", "--mount", "type=volume,src=" + volume + ",dst=/target", + "run", "--rm", "--interactive", "--network", "none", "--mount", "type=volume,src=" + volume + ",dst=/target", helperImage, "sh", "-ceu", "rm -rf -- /target/* /target/.[!.]* /target/..?*; tar --numeric-owner -C /target -xf -", } diff --git a/tools/tht/internal/backup/restore_test.go b/tools/tht/internal/backup/restore_test.go index 3f9cde8e..8dae6f3d 100644 --- a/tools/tht/internal/backup/restore_test.go +++ b/tools/tht/internal/backup/restore_test.go @@ -30,6 +30,19 @@ func TestRestorePublicPathUsesConcreteProductionPreflight(t *testing.T) { } } +func TestVolumeRestoreCommandKeepsTarInputOpenWithoutTTY(t *testing.T) { + args := volumeRestoreCommand("project_sessions") + wantPrefix := []string{"run", "--rm", "--interactive", "--network", "none"} + if len(args) < len(wantPrefix) || !equalStrings(args[:len(wantPrefix)], wantPrefix) { + t.Fatalf("volumeRestoreCommand() prefix = %q, want %q", args, wantPrefix) + } + for _, arg := range args { + if arg == "--tty" || arg == "-t" { + t.Fatalf("volumeRestoreCommand() requests a TTY: %q", args) + } + } +} + func TestRestoreRestoresVerifiedVolumesInManifestOrderBeforeAuthenticationReset(t *testing.T) { installation := preflightTestInstallation(t) archive := filepath.Join(t.TempDir(), "restore-volumes.zip") diff --git a/tools/tht/internal/pi/update.go b/tools/tht/internal/pi/update.go index 50533f47..84785ed1 100644 --- a/tools/tht/internal/pi/update.go +++ b/tools/tht/internal/pi/update.go @@ -477,7 +477,8 @@ func setMaintenance(ctx context.Context, runner Runner, enabled bool) error { if enabled { path = "activate" } - args := []string{"exec", "-T", "core", "curl", "-fsS", "-X", "POST", "http://127.0.0.1:8787/internal/maintenance/" + path} + args := append([]string{"exec", "-T", "core", "curl", "-fsS"}, internalIdentityHeaders...) + args = append(args, "-X", "POST", "http://127.0.0.1:8787/internal/maintenance/"+path) result, err := runCompose(ctx, runner, args...) status, valid := parseMaintenanceStatus(result.Stdout) if err == nil && valid && status.Active == enabled && status.Admissions == 0 && !status.RecoveryRequired { @@ -517,7 +518,9 @@ func parseMaintenanceStatus(value string) (MaintenanceState, bool) { } func MaintenanceStatus(ctx context.Context, runner Runner) (MaintenanceState, error) { - result, err := runCompose(ctx, runner, "exec", "-T", "core", "curl", "-fsS", "http://127.0.0.1:8787/internal/maintenance/status") + args := append([]string{"exec", "-T", "core", "curl", "-fsS"}, internalIdentityHeaders...) + args = append(args, "http://127.0.0.1:8787/internal/maintenance/status") + result, err := runCompose(ctx, runner, args...) if err != nil { return MaintenanceState{}, commandError("maintenance status check", result, err) } diff --git a/tools/tht/internal/pi/update_test.go b/tools/tht/internal/pi/update_test.go index 8b28434f..79ef8426 100644 --- a/tools/tht/internal/pi/update_test.go +++ b/tools/tht/internal/pi/update_test.go @@ -779,6 +779,39 @@ func TestUpdateRequiresConfirmationAndDrainsActiveSessions(t *testing.T) { assertCalled(t, fake.calls, "/internal/maintenance/deactivate") } +func TestMaintenanceControlUsesExactLoopbackOperatorIdentity(t *testing.T) { + fake := newFakeRunner() + if err := setMaintenance(context.Background(), fake, true); err != nil { + t.Fatal(err) + } + fake.maintenance = true + if _, err := MaintenanceStatus(context.Background(), fake); err != nil { + t.Fatal(err) + } + for _, path := range []string{"/internal/maintenance/activate", "/internal/maintenance/status"} { + found := false + for _, call := range fake.calls { + if !strings.Contains(call, path) { + continue + } + found = true + for _, header := range []string{ + "x-thoth-principal-issuer: tht", + "x-thoth-principal-subject: tht-maintenance", + "x-thoth-principal-display-name: Tht maintenance", + "x-thoth-is-admin: 1", + } { + if !strings.Contains(call, header) { + t.Fatalf("maintenance call %q lacks %q", call, header) + } + } + } + if !found { + t.Fatalf("maintenance call %q was not made", path) + } + } +} + func TestRollbackRestoresInterruptedOrPreviouslyRecordedState(t *testing.T) { fake := newFakeRunner() statePath := filepath.Join(t.TempDir(), "state.json")