Merge pull request 'mssql: the query runs as a read-only login, one line, no variables; sqlcmd is installed (hq #193)' (#210) from fix/193-mssql-reads-as-a-reader into main

modules/mssql/Dockerfile

@@ -20,7 +20,20 @@ COPY . .
 RUN node /app/node_modules/typescript/bin/tsc client.ts index.ts tools/index.ts provisioner/index.ts \
       --module NodeNext --moduleResolution NodeNext --target ES2022 --outDir dist
 
+# **sqlcmd, which this module's client drives, has to be here** — it never was, so every tool failed
+# with `spawn sqlcmd ENOENT`. go-sqlcmd is one static binary; fetched at a pinned release and checked
+# against its digest, so a build that receives anything else stops here.
+FROM ${BUILD_BASE} AS sqlcmd
+ARG SQLCMD_VERSION=v1.10.0
+ARG SQLCMD_SHA256=92516d98c63d99b0994de5b61350c91f6915f9b76f139a59039fbcb225c2e987
+RUN apt-get update && apt-get install -y --no-install-recommends curl ca-certificates bzip2 \
+      && curl -fsSL -o /tmp/sqlcmd.tar.bz2 \
+           "https://github.com/microsoft/go-sqlcmd/releases/download/${SQLCMD_VERSION}/sqlcmd-linux-amd64.tar.bz2" \
+      && echo "${SQLCMD_SHA256}  /tmp/sqlcmd.tar.bz2" | sha256sum -c - \
+      && tar -xjf /tmp/sqlcmd.tar.bz2 -C /usr/local/bin sqlcmd
+
 FROM ${RUNTIME_BASE}
+COPY --from=sqlcmd /usr/local/bin/sqlcmd /usr/local/bin/sqlcmd
 COPY --from=build /app/modules/mssql/dist /app/modules/mssql/dist
 # Every serve-time entrypoint, loaded by the runtime in serve mode: tools and events serve, and a
 # provider's provisioner runs its reconcile loop in the same process, with the broker connected —

modules/mssql/client.ts

@@ -29,11 +29,40 @@ export interface MssqlConn {
   readonly port: number;
   readonly user: string;
   readonly password: string;
+  /**
+   * The read-only login's password, which the mesh mints for this module (`own-secrets.reader`).
+   * Absent when the mesh has not delivered it: then a caller's statement is refused, never run as
+   * the administrator (novox/hq issue 193).
+   */
+  readonly readerPassword?: string;
+}
+
+/**
+ * The login a caller's statement runs as (novox/hq issue 193). It may connect to every database and
+ * read every table, and holds no other permission. A statement cannot climb out of a login the way it
+ * could out of a transaction wrapped around it as text, and the administrator — who can run programs
+ * on the server — never runs a caller's text.
+ */
+export const READER = "mesh_mssql_reader";
+
+/** Who a sqlcmd invocation logs in as, and whether the text is a caller's rather than the module's. */
+interface Invocation {
+  readonly user: string;
+  readonly password: string;
+  /**
+   * A caller's text: sqlcmd substitutes no `$(NAME)` in it, which would read this process's
+   * environment — the administrator's password among it. (Its own commands are kept out by the
+   * caller's text never beginning a line; see readOnlyQuery.)
+   */
+  readonly caller: boolean;
 }
 
 export class MssqlClient {
   constructor(private readonly conn: MssqlConn) {}
 
+  /** The reader is made once per process: idempotent, and repeating it re-sets a rotated password. */
+  private readerReady?: Promise<void>;
+
   /**
    * Build from the module's resolved environment. Reads MESH_MSSQL_* first (the documented
    * names), falling back to the MESH_PROVISION_* keys the manifest already sets on the provisioner
@@ -48,7 +77,9 @@ export class MssqlClient {
     if (!host || !password) {
       throw new Error("mssql host or admin password is not set — mssql's own code cannot reach the server");
     }
-    return new MssqlClient({ host, port, user, password });
+    const readerPassword = env.MESH_MSSQL_READER_PASSWORD ??
+      readSecretFile(env.MESH_MSSQL_READER_PASSWORD_FILE);
+    return new MssqlClient({ host, port, user, password, readerPassword });
   }
 
   get host(): string {
@@ -86,7 +117,12 @@ export class MssqlClient {
   }
 
   /** The one execution boundary: invoke `sqlcmd` and return its concatenated stdout. */
-  private async sqlcmd(sql: string, database: string, variables: Record<string, string> = {}): Promise<string> {
+  private async sqlcmd(
+    sql: string,
+    database: string,
+    variables: Record<string, string> = {},
+    as: Invocation = { user: this.conn.user, password: this.conn.password, caller: false },
+  ): Promise<string> {
     // `-h -1` drops the column-header rule; `-y 0`/`-Y 0` lift the display-width cap so a long
     // JSON document is not truncated; `-W` trims trailing whitespace so the JSON chunks rejoin
     // cleanly. sqlcmd from the mssql-tools ships in the runtime container, the way `psql` ships
@@ -95,8 +131,9 @@ export class MssqlClient {
       "sqlcmd",
       [
         "-S", `${this.conn.host},${this.conn.port}`,
-        "-U", this.conn.user,
+        "-U", as.user,
         "-d", database,
+        ...(as.caller ? ["-x"] : []),
         "-C",
         "-b",
         "-h", "-1",
@@ -107,7 +144,7 @@ export class MssqlClient {
       ],
       // `variables` reach sqlcmd as environment variables, which it substitutes as `$(NAME)` scripting
       // variables: a value that must not appear on argv, or in the message of a failed command.
-      { env: { ...process.env, ...variables, SQLCMDPASSWORD: this.conn.password }, maxBuffer: 16 << 20 },
+      { env: { ...process.env, ...variables, SQLCMDPASSWORD: as.password }, maxBuffer: 16 << 20 },
     );
     return stdout;
   }
@@ -225,16 +262,76 @@ export class MssqlClient {
     }));
   }
 
-  /** Run a read-only SELECT against a named database, for the mssql_query tool. */
-  async readOnlyQuery(database: string, sql: string): Promise<QueryResult> {
-    // The read-only guarantee is a wrapping transaction that is always rolled back: any write the
-    // statement attempts is undone. The rows are rendered by FOR JSON inside query().
-    const rows = await this.query(
-      `BEGIN TRANSACTION;\n${stripTrailingSemis(sql)}\nFOR JSON PATH, INCLUDE_NULL_VALUES;\nROLLBACK;`,
-      database,
+  /**
+   * Make the read-only login, idempotently, with the password the mesh minted for it: it may connect
+   * to every database and read every table, and is taken out of the administrators' role should
+   * anyone have put it there. Run as the administrator, because only it can make a login.
+   */
+  async ensureReader(): Promise<void> {
+    const password = this.conn.readerPassword;
+    if (!password) throw readerMissing();
+    const logins = await this.query(
+      `SELECT 1 AS ok FROM sys.server_principals WHERE name = ${literal(READER)}`,
     );
-    return { command: sql.trimStart().split(/\s+/)[0]?.toUpperCase() ?? "", rows };
+    if (logins.length === 0) {
+      await this.exec(
+        `CREATE LOGIN ${ident(READER)} WITH PASSWORD = ${literal(password)}, CHECK_POLICY = OFF`,
+      );
+    } else {
+      await this.exec(`ALTER LOGIN ${ident(READER)} WITH PASSWORD = ${literal(password)}`);
+      await this.exec(`ALTER LOGIN ${ident(READER)} ENABLE`);
+    }
+    await this.exec(
+      `IF IS_SRVROLEMEMBER('sysadmin', ${literal(READER)}) = 1 ` +
+        `ALTER SERVER ROLE sysadmin DROP MEMBER ${ident(READER)}`,
+    );
+    await this.exec(`GRANT CONNECT ANY DATABASE TO ${ident(READER)}`);
+    await this.exec(`GRANT SELECT ALL USER SECURABLES TO ${ident(READER)}`);
   }
+
+  /**
+   * Run a caller's SELECT against a named database as the read-only login, for the mssql_query tool
+   * (novox/hq issue 193). Read-only by the login, not by a transaction wrapped around the text; the
+   * rows are rendered by FOR JSON. Never as the administrator: without the reader's password the call
+   * is refused.
+   */
+  async readOnlyQuery(database: string, sql: string): Promise<QueryResult> {
+    const password = this.conn.readerPassword;
+    if (!password) throw readerMissing();
+    // **One line, refused otherwise.** sqlcmd reads a line that BEGINS with `:` or `!!` as its own
+    // command rather than SQL, and `:!!` starts a program in this container, which holds the
+    // administrator's password. Its switch for refusing those (-X) makes it ignore -Q in the
+    // version shipped here, so instead no line of a caller's text can begin one: the text follows
+    // this module's own on the first line, and a line break in it is refused. Proven on a throwaway
+    // server: the same text at the start of a line ran a program; mid-line it is a syntax error.
+    if (/[\r\n]/.test(sql)) {
+      throw new Error(
+        "mssql_query: the statement must be one line — sqlcmd takes a line beginning with ':' or " +
+          "'!!' as a command of its own, which can start a program (novox/hq issue 193)",
+      );
+    }
+    this.readerReady ??= this.ensureReader().catch((err) => {
+      this.readerReady = undefined; // asked again next call, not failed for the process's life
+      throw err;
+    });
+    await this.readerReady;
+    const stdout = await this.sqlcmd(
+      `SET NOCOUNT ON; ${stripTrailingSemis(sql)}\nFOR JSON PATH, INCLUDE_NULL_VALUES;`,
+      database,
+      {},
+      { user: READER, password, caller: true },
+    );
+    const command = /^\s*([A-Za-z]+)/.exec(sql)?.[1]?.toUpperCase() ?? "";
+    return { command, rows: parseJsonRows(stdout) };
+  }
+}
+
+function readerMissing(): Error {
+  return new Error(
+    "the read-only login's password was not delivered (own-secrets.reader, " +
+      "MESH_MSSQL_READER_PASSWORD_FILE), so the statement is refused rather than run as the " +
+      "administrator (novox/hq issue 193)",
+  );
 }
 
 /** Generate a URL-safe password. */

modules/mssql/module.json

@@ -38,7 +38,8 @@
   },
   "own-secrets": {
     "sa": "${dir:state}/sa.secret",
-    "broker": "${dir:mesh-state}/broker"
+    "broker": "${dir:mesh-state}/broker",
+    "reader": "${dir:state}/reader.secret"
   },
   "resources": [
     {
@@ -101,13 +102,15 @@
       "volumes": [
         "${dir:mesh-state}/broker:/run/secrets/broker:ro",
         "${dir:grants}:/var/lib/mssql/grants:ro",
-        "${dir:state}/sa.secret:/run/secrets/sa:ro"
+        "${dir:state}/sa.secret:/run/secrets/sa:ro",
+        "${dir:state}/reader.secret:/run/secrets/reader:ro"
       ],
       "env": {
         "MESH_PROVISION_MSSQL": "mssql://sa@mssql:1433/master",
         "MESH_PROVISION_PASSWORD_FILE": "/run/secrets/sa",
         "MESH_BROKER_FILE": "/run/secrets/broker",
-        "MESH_RECEIVES": "/var/lib/mssql/grants/mesh.json"
+        "MESH_RECEIVES": "/var/lib/mssql/grants/mesh.json",
+        "MESH_MSSQL_READER_PASSWORD_FILE": "/run/secrets/reader"
       },
       "artifact": "runtime"
     }

modules/mssql/package.json

@@ -1,9 +1,13 @@
 {
   "name": "@novox/module-mssql",
   "version": "0.1.0",
-  "description": "mssql — provides the mesh mssql-database interface. Its client, provisioner, tools and events live here (novox/hq ADR 0039).",
+  "description": "mssql \u2014 provides the mesh mssql-database interface. Its client, provisioner, tools and events live here (novox/hq ADR 0039).",
   "type": "module",
   "private": true,
+  "scripts": {
+    "build": "tsc client.ts index.ts tools/index.ts provisioner/index.ts --module NodeNext --moduleResolution NodeNext --target ES2022 --outDir dist",
+    "test": "npm run build && node --test --experimental-strip-types 'test/*.test.ts'"
+  },
   "dependencies": {
     "@novox/mesh-sdk": "^0.1.1"
   },

modules/mssql/test/reader.test.ts

@@ -0,0 +1,96 @@
+// What holds mssql_query to being read-only (novox/hq issue 193): a caller's statement runs as the
+// reader login and never as the administrator, with sqlcmd's variable substitution off, on one line
+// that follows the module's own — a line break is refused before sqlcmd starts — and with no
+// transaction wrapped around it as text. Without the reader's password the statement is refused.
+//
+// sqlcmd is a fake on PATH that records each call's login, flags and text. That the reader cannot
+// write is the server's to enforce and was proven against a real server; this holds the module to
+// asking for it. Run against the compiled module (npm test builds first), the way the runtime loads it.
+
+import { test, before, after } from "node:test";
+import assert from "node:assert/strict";
+import { chmod, mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+import { MssqlClient, READER } from "../dist/client.js";
+
+let dir: string;
+let log: string;
+const originalPath = process.env.PATH;
+
+before(async () => {
+  dir = await mkdtemp(join(tmpdir(), "mssql-reader-"));
+  log = join(dir, "calls.jsonl");
+  await writeFile(join(dir, "sqlcmd"), `#!/usr/bin/env node
+const fs = require("node:fs");
+const args = process.argv.slice(2);
+const at = (flag) => args[args.indexOf(flag) + 1];
+fs.appendFileSync(${JSON.stringify(log)}, JSON.stringify({
+  user: at("-U"), database: at("-d"), sql: at("-Q"), noVariables: args.includes("-x"),
+  password: process.env.SQLCMDPASSWORD,
+}) + "\\n");
+const sql = at("-Q");
+if (/FROM sys.server_principals/.test(sql)) process.stdout.write("");
+else if (/FOR JSON/.test(sql)) process.stdout.write('[{"name":"alpha","n":1}]\\n');
+`);
+  await chmod(join(dir, "sqlcmd"), 0o755);
+  process.env.PATH = `${dir}:${originalPath}`;
+});
+
+after(async () => {
+  process.env.PATH = originalPath;
+  await rm(dir, { recursive: true, force: true });
+});
+
+async function calls(): Promise<Record<string, unknown>[]> {
+  const text = await readFile(log, "utf8").catch(() => "");
+  await writeFile(log, "");
+  return text.split("\n").filter(Boolean).map((line) => JSON.parse(line));
+}
+
+const conn = { host: "127.0.0.1", port: 1433, user: "sa", password: "admin-secret" };
+
+test("a caller's statement runs as the reader, without variables, on the module's first line", async () => {
+  const client = new MssqlClient({ ...conn, readerPassword: "reader-secret" });
+  const result = await client.readOnlyQuery("inventory", "SELECT '$(SQLCMDPASSWORD)' AS p");
+
+  const asked = (await calls()).at(-1)!;
+  assert.equal(asked.user, READER, "the statement never runs as the administrator");
+  assert.equal(asked.password, "reader-secret");
+  assert.equal(asked.noVariables, true, "no $(NAME) is substituted in a caller's text");
+  const [first] = String(asked.sql).split("\n");
+  assert.ok(first.startsWith("SET NOCOUNT ON; SELECT '$(SQLCMDPASSWORD)'"), "the caller's text never begins a line");
+  assert.doesNotMatch(String(asked.sql), /BEGIN TRANSACTION|ROLLBACK/, "no transaction wrapped around it as text");
+  assert.deepEqual(result.rows, [{ name: "alpha", n: 1 }]);
+  assert.equal(result.command, "SELECT");
+});
+
+test("a line break in a caller's statement is refused before sqlcmd starts", async () => {
+  const client = new MssqlClient({ ...conn, readerPassword: "reader-secret" });
+  for (const sql of ["SELECT 1\n:!! id", "SELECT 1\r\n:!! id", "SELECT 1\r:!! id"]) {
+    await assert.rejects(client.readOnlyQuery("inventory", sql), /must be one line/);
+  }
+  assert.deepEqual(await calls(), []);
+});
+
+test("the reader is made as the administrator, kept out of sysadmin, and granted only reading", async () => {
+  const client = new MssqlClient({ ...conn, readerPassword: "reader-secret" });
+  await client.readOnlyQuery("inventory", "SELECT 1 AS x");
+  await client.readOnlyQuery("inventory", "SELECT 2 AS x");
+
+  const made = await calls();
+  const asAdmin = made.filter((c) => c.user === "sa").map((c) => String(c.sql));
+  assert.ok(asAdmin.some((s) => s.startsWith(`CREATE LOGIN [${READER}]`)));
+  assert.ok(asAdmin.some((s) => /ALTER SERVER ROLE sysadmin DROP MEMBER/.test(s)));
+  assert.ok(asAdmin.includes(`GRANT CONNECT ANY DATABASE TO [${READER}]`));
+  assert.ok(asAdmin.includes(`GRANT SELECT ALL USER SECURABLES TO [${READER}]`));
+  assert.equal(asAdmin.filter((s) => s.startsWith("CREATE LOGIN")).length, 1, "made once, not per call");
+  assert.equal(made.filter((c) => c.user === READER).length, 2);
+});
+
+test("without the reader's password the statement is refused, and nothing runs as the administrator", async () => {
+  const client = new MssqlClient(conn);
+  await assert.rejects(client.readOnlyQuery("inventory", "SELECT 1"), /refused rather than run as the administrator/);
+  assert.deepEqual(await calls(), []);
+});

modules/mssql/tools/index.ts

@@ -16,7 +16,7 @@ export function getMssqlTools(mssql: MssqlClient): ToolDefinition[] {
     },
     {
       name: "mssql_query",
-      description: "Run a read-only SELECT against a named database (wrapped in a rolled-back transaction).",
+      description: "Run a read-only SELECT against a named database, as a login that can read every table and change nothing.",
       input: {
         database: { type: "string", description: "the database to query" },
         sql: { type: "string", description: "a single SELECT statement" },