From 400b2f9696b62821b3f4db8bb8df0b745bc0670a Mon Sep 17 00:00:00 2001 From: jochens Date: Fri, 2 Oct 2026 00:25:22 +0200 Subject: [PATCH] mssql: the query runs as a read-only login, one line, no variables; sqlcmd is installed (hq #193) Proven on a throwaway server: as the administrator a caller's $(SQLCMDPASSWORD) returned the sa password, and a line beginning ':!!' ran a program in the tools container. The statement now runs as mesh_mssql_reader (CONNECT ANY DATABASE, SELECT ALL USER SECURABLES), with substitution off (-x), after the module's own text on the first line, and a line break is refused. go-sqlcmd v1.10.0 is installed at a pinned digest: the image never had sqlcmd, so every mssql tool failed with spawn sqlcmd ENOENT. --- modules/mssql/Dockerfile | 13 ++++ modules/mssql/client.ts | 121 +++++++++++++++++++++++++++--- modules/mssql/module.json | 9 ++- modules/mssql/package.json | 6 +- modules/mssql/test/reader.test.ts | 96 ++++++++++++++++++++++++ modules/mssql/tools/index.ts | 2 +- 6 files changed, 230 insertions(+), 17 deletions(-) create mode 100644 modules/mssql/test/reader.test.ts diff --git a/modules/mssql/Dockerfile b/modules/mssql/Dockerfile index 20e878e..07ac078 100644 --- a/modules/mssql/Dockerfile +++ b/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 — diff --git a/modules/mssql/client.ts b/modules/mssql/client.ts index e3304ac..5bbb78a 100644 --- a/modules/mssql/client.ts +++ b/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; + /** * 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 = {}): Promise { + private async sqlcmd( + sql: string, + database: string, + variables: Record = {}, + as: Invocation = { user: this.conn.user, password: this.conn.password, caller: false }, + ): Promise { // `-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 { - // 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 { + 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 { + 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. */ diff --git a/modules/mssql/module.json b/modules/mssql/module.json index f6a2137..0b86a84 100644 --- a/modules/mssql/module.json +++ b/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" } diff --git a/modules/mssql/package.json b/modules/mssql/package.json index 31eeb79..3a9be3b 100644 --- a/modules/mssql/package.json +++ b/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" }, diff --git a/modules/mssql/test/reader.test.ts b/modules/mssql/test/reader.test.ts new file mode 100644 index 0000000..2d7c7e7 --- /dev/null +++ b/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[]> { + 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(), []); +}); diff --git a/modules/mssql/tools/index.ts b/modules/mssql/tools/index.ts index 8b91d2e..d48bf1f 100644 --- a/modules/mssql/tools/index.ts +++ b/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" },