From c83f48b821a19fdb25375affcf005c2088f3a2af Mon Sep 17 00:00:00 2001 From: jochen Date: Wed, 21 Oct 2026 20:21:56 +0200 Subject: [PATCH] Make link-key secrets 40 characters and keep every mc argument's secret out of its errors (issue 1766) --- modules/minio/client.ts | 41 +++++++++++++++++++++--- modules/minio/links.ts | 8 ++++- modules/minio/test/links.test.ts | 55 ++++++++++++++++++++++++++++++++ 3 files changed, 98 insertions(+), 6 deletions(-) diff --git a/modules/minio/client.ts b/modules/minio/client.ts index af29f17a..374f4a76 100644 --- a/modules/minio/client.ts +++ b/modules/minio/client.ts @@ -256,11 +256,22 @@ export class MinioClient { if (argv.some((a) => a.includes(this.rootPassword))) { throw new Error("refused to run mc: its command line would carry the root password (novox/hq issue 282)"); } - const { stdout } = await execFileAsync(this.mcBin, argv, { - timeout: 30_000, - env: { ...process.env, MC_HOST_mesh: mcHost(this.baseUrl, this.rootUser, this.rootPassword) }, - }); - return stdout.trim(); + const mcHostValue = mcHost(this.baseUrl, this.rootUser, this.rootPassword); + try { + const { stdout } = await execFileAsync(this.mcBin, argv, { + timeout: 30_000, + env: { ...process.env, MC_HOST_mesh: mcHostValue }, + }); + return stdout.trim(); + } catch (err) { + // A fresh error with only the redacted message: execFile's own carries the whole command line, and + // its cmd, stdout and stderr properties would carry it on (issue 1766). + const known = [mcHostValue, this.rootPassword, encodeURIComponent(this.rootPassword), ...secretArgs(argv)]; + const e = err as { stderr?: unknown; message?: string }; + const reason = typeof e.stderr === "string" && e.stderr.trim() !== "" ? e.stderr.trim() : (e.message ?? String(err)); + const shown = argv.filter((a) => a !== this.mcConfigDir).join(" "); + throw new Error(redactSecrets(`mc ${shown} failed: ${reason}`, known)); + } } // --- SigV4 request plumbing --------------------------------------------- @@ -360,6 +371,26 @@ function uriEncode(str: string, encodeSlash: boolean): string { return out; } +/** The values that follow --secret-key and --access-key in an argument list. */ +function secretArgs(argv: string[]): string[] { + const out: string[] = []; + argv.forEach((a, i) => { if ((a === "--secret-key" || a === "--access-key") && i + 1 < argv.length) out.push(argv[i + 1]); }); + return out; +} + +/** + * The one redaction every `mc` failure goes through (issue 1766): the token after --secret-key or + * --access-key (as a separate word or after "="), any MC_HOST_ value, and each known secret, wherever they + * appear in the text, become "[redacted]". + */ +export function redactSecrets(text: string, known: string[] = []): string { + let out = text + .replace(/(--(?:secret|access)-key(?:=|\s+))('[^']*'|"[^"]*"|\S+)/g, "$1[redacted]") + .replace(/(MC_HOST_\w+=)\S+/g, "$1[redacted]"); + for (const k of known) if (k !== "") out = out.split(k).join("[redacted]"); + return out; +} + /** * The MC_HOST_ URL `mc` reads an alias from: the endpoint with the credentials in its userinfo, * each percent-encoded so a reserved character in either cannot end it early. diff --git a/modules/minio/links.ts b/modules/minio/links.ts index b870f0df..305f6b04 100644 --- a/modules/minio/links.ts +++ b/modules/minio/links.ts @@ -36,6 +36,9 @@ export function linkPolicy(bucket: string): string { }); } +/** A link key's secret: 40 hex characters, minio's limit for a secret key (issue 1766). */ +const LINK_SECRET = /^[0-9a-f]{40}$/; + export function linkSecretPath(dir: string, bucket: string): string { return join(dir, `${bucket}.secret`); } @@ -47,9 +50,12 @@ export async function ensureLinkKey(admin: LinkAdmin, dir: string, bucket: strin const path = linkSecretPath(dir, bucket); mkdirSync(dir, { recursive: true, mode: 0o700 }); let secret = existsSync(path) ? readFileSync(path, "utf8").trim() : ""; + // A stored secret of the wrong shape (an earlier version made 64 characters; minio takes at most 40) is + // replaced, and its key made again (issue 1766). + if (!LINK_SECRET.test(secret)) secret = ""; if (secret !== "" && (await admin.accessKeyExists(id))) return; if (secret === "") { - secret = randomBytes(32).toString("hex"); + secret = randomBytes(20).toString("hex"); writeFileSync(path, secret + "\n", { mode: 0o600 }); chmodSync(path, 0o600); } diff --git a/modules/minio/test/links.test.ts b/modules/minio/test/links.test.ts index 62de8e75..4c0bd840 100644 --- a/modules/minio/test/links.test.ts +++ b/modules/minio/test/links.test.ts @@ -115,3 +115,58 @@ test("the link key id is the one the object-store tool derives (cmd/object-store // 1516/AC1 assert.equal(linkKeyId("brother"), "link0b4bd77cec705cf5"); }); + +// Issue 1766: a link key's secret is minio's length, and no mc failure carries an argument's secret. + +import { chmodSync, writeFileSync, mkdirSync } from "node:fs"; +import { MinioClient } from "../client.ts"; + +test("a link key's secret is 40 characters, and a stored 64-character secret is replaced", async () => { + // 1766/AC1 + const { minio, dir } = setup(); + await ensureLinkKey(minio, dir, "fresh"); + assert.equal(minio.keys.get(linkKeyId("fresh"))!.secret.length, 40); + assert.match(minio.keys.get(linkKeyId("fresh"))!.secret, /^[0-9a-f]{40}$/); + + mkdirSync(dir, { recursive: true }); + const old = "ab".repeat(32); + writeFileSync(linkSecretPath(dir, "old"), old + "\n"); + await ensureLinkKey(minio, dir, "old"); + const made = minio.keys.get(linkKeyId("old"))!.secret; + assert.equal(made.length, 40); + assert.notEqual(made, old); + assert.equal(readFileSync(linkSecretPath(dir, "old"), "utf8").trim(), made); +}); + +function failingMc(): { client: MinioClient; root: string } { + const d = mkdtempSync(join(process.env.TMPDIR ?? tmpdir(), "minio-mc-")); + const bin = join(d, "mc"); + // Says its arguments and its alias on stderr, then fails, the way a refused command does. + writeFileSync(bin, '#!/bin/sh\necho "args: $*" >&2\necho "env: MC_HOST_mesh=$MC_HOST_mesh" >&2\nexit 1\n'); + chmodSync(bin, 0o755); + const root = "RootPass-s3cret/ok"; + const client = new MinioClient({ endpoint: "http://127.0.0.1:9000", rootUser: "root", rootPassword: root, region: "us-east-1", mcBin: bin, mcConfigDir: d }); + return { client, root }; +} + +test("an mc failure's message holds no --secret-key value and no MC_HOST value", async () => { + // 1766/AC2 + const { client, root } = failingMc(); + const secret = "SuperSecretValue0123456789"; + const err: any = await client.createAccessKey("b", "ACCESSKEYID1234567", secret).then(() => null, (e) => e); + assert.ok(err, "the failure is raised"); + for (const text of [String(err.message), String(err.stack ?? ""), JSON.stringify(Object.getOwnPropertyNames(err).map((k) => err[k]))]) { + assert.ok(!text.includes(secret), "no secret key"); + assert.ok(!text.includes("ACCESSKEYID1234567"), "no access key"); + assert.ok(!text.includes(root) && !text.includes(encodeURIComponent(root)), "no root password"); + assert.ok(!text.includes("root:"), "no MC_HOST value"); + } + assert.match(err.message, /\[redacted\]/); +}); + +test("a consumer key's create passes the minted secret unchanged", async () => { + // 1766/AC3 + const s = setup(); + await s.h.create(provision); + assert.equal(s.minio.keys.get("brother-key")!.secret, "consumer-secret"); +}); -- 2.54.0