Merge pull request 'Make minio's link-key secrets 40 characters and keep every mc argument's secret out of its errors (issue 1766)' (#422) from fix/minio-link-key-length into main

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, {
+    const mcHostValue = mcHost(this.baseUrl, this.rootUser, this.rootPassword);
-      timeout: 30_000,
+    try {
-      env: { ...process.env, MC_HOST_mesh: mcHost(this.baseUrl, this.rootUser, this.rootPassword) },
+      const { stdout } = await execFileAsync(this.mcBin, argv, {
-    });
+        timeout: 30_000,
-    return stdout.trim();
+        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_<alias> 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.

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);
   }

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");
+});