diff --git a/docs/opentofu-migration.md b/docs/opentofu-migration.md index 98974fc..e689e89 100644 --- a/docs/opentofu-migration.md +++ b/docs/opentofu-migration.md @@ -156,6 +156,10 @@ The contract: - the credential goes to **stdout and nothing else does** — every message, warning and Keychain prompt is on stderr, so `$(ct auth token --raw)` is safe; +- **every value in the object is a string**, so `data "external"` can consume it + as-is. `environment` is `""` — not `null` — when no `--env` was passed, which + is the default invocation; a `null` there fails inside the external provider + with a message about JSON types that names neither the command nor the field; - a failure writes **nothing** to stdout and exits non-zero, with the remedy named (`ct auth login --env `); - printing to a **terminal is refused** unless `--allow-tty` — a credential in @@ -170,7 +174,7 @@ _is_ needed, a cross-process brake keeps it from becoming a burst: - handshakes against one host are spaced at least 3s apart (waited out, not an error); -- more than 120 in a rolling hour is refused, naming when the window frees up — +- more than 1000 in a rolling hour is refused, naming when the window frees up — that is a runaway loop, and hammering a throttled instance only lengthens the outage for everyone on it; - `CT_NO_LOGIN_THROTTLE=1` disables it, for a CI job that knows it runs alone. @@ -183,11 +187,13 @@ same instant are spaced no better than not at all. **The brake sits in the login handshake, so it covers every `ct` command**, not just `ct auth token` — and on Linux and Windows there is no session cache, so -there each invocation is one handshake. That is why the hourly cap is 120 rather -than a number sized for a credential helper alone: a pipeline should never reach -it, while a runaway loop passes it in about six minutes. A CI job that runs more -`ct` invocations than that against one host in an hour should set -`CT_NO_LOGIN_THROTTLE=1`. +there each invocation is one handshake. + +The 3s spacing is the part that protects the instance: it caps sustained traffic +at 20 handshakes a minute however hard anything loops, which is the conservative +end of what ChurchTools takes. The hourly cap is only a backstop for a process +that has been hammering for an actual hour, so it is set well above anything the +spacing permits — a pipeline of any plausible size never reaches it. A CI job otherwise needs none of this: it passes the token explicitly from a GitHub secret, which is already storage-free. This path exists for local diff --git a/src/application/operations/auth.ts b/src/application/operations/auth.ts index a3c8342..abd0524 100644 --- a/src/application/operations/auth.ts +++ b/src/application/operations/auth.ts @@ -170,7 +170,17 @@ export interface AuthTokenRequest { export interface AuthTokenResult { operation: "auth"; action: "token"; - environment: string | null; + /** + * The selected environment, or `""` when none was — deliberately NOT `null`. + * + * This command's whole purpose is to be read by another tool, and the tool it was written for + * reads it through Terraform's `data "external"`, which requires EVERY value in the object to be + * a string. A `null` here fails inside the external provider with a message about JSON types that + * names neither this command nor the field, so the one credential helper in the system breaks in + * the least diagnosable way available. Every other field is already a string; this one is the + * exception that made the envelope unusable. + */ + environment: string; host: string; cookie: string; csrfToken: string; @@ -244,7 +254,7 @@ export async function runAuthToken( return { operation: "auth", action: "token", - environment: project.environment, + environment: project.environment ?? "", host: project.host, cookie: session.cookie, csrfToken: session.csrfToken, diff --git a/src/auth/loginThrottle.ts b/src/auth/loginThrottle.ts index 645bf10..a58c132 100644 --- a/src/auth/loginThrottle.ts +++ b/src/auth/loginThrottle.ts @@ -36,22 +36,53 @@ import { homedir } from "node:os"; import { join } from "node:path"; import { hostSlug } from "../permissions/catalog-store.js"; -/** Minimum spacing between two handshakes against one host. Waited out, never an error. */ +/** + * Minimum spacing between two handshakes against one host. Waited out, never an error. + * + * This is the real brake for a SEQUENCE of invocations, and the only one that scales with how fast a + * caller goes: 3s means at most 20 handshakes a minute however hard one caller loops. That is the + * conservative end of what an instance takes — a login handshake is heavier than a plain read, so + * sitting at the bottom of the band is the right default rather than the top of it. + * + * It bounds a sequence, NOT a simultaneous burst — see "What it does not do" above. N processes that + * read the file at the same moment compute the same wait, sleep it together and fire in lockstep, so + * the ceiling the spacing enforces is 20*N a minute, and the recorded count grows by roughly one per + * round no matter how many fired. That is the case {@link MAX_PER_HOUR} is the only bound on. + * + * It is also close to free for CI, which is the case that pays it: a `ct plan` takes longer than 3s + * on its own, so the wait is usually already elapsed by the time the next invocation asks. + */ export const MIN_INTERVAL_MS = 3_000; /** * Handshakes per rolling hour per host before `ct` refuses to add to the pile. * - * Sized for a CI pipeline, not for `ct auth token` alone, because the gate sits in - * `CtClient.performLogin` and therefore covers EVERY ct command. On Linux and Windows there is no - * session cache at all (`sessionStore` is Keychain-only by design), so on CI each invocation costs - * one handshake: at 20/hour a pipeline whose 21st `ct plan`/`ct get`/`ct apply` ran inside the hour - * would start failing on ct's own error, somewhere it had always worked. + * Deliberately far above anything legitimate, because for a serial caller this is NOT the rate + * limiter — the spacing above is. {@link MIN_INTERVAL_MS} already caps one looping caller at 20 + * handshakes a minute, which sits at the conservative end of what a ChurchTools instance tolerates + * (operator estimate: 20-30 requests a minute, possibly 40). Anything the spacing permits serially + * is by definition a rate the instance is fine with, so an hourly cap that binds FIRST on that path + * is not protecting the instance — it is just failing commands that were never the problem. + * + * It bound first for a long time. The gate sits in `CtClient.performLogin`, so it covers EVERY ct + * command, and on Linux and Windows there is no session cache at all (`sessionStore` is + * Keychain-only by design) — so on CI each invocation costs one handshake. The original 20/hour + * failed a pipeline's 21st `ct plan`; even 120/hour is an average of 2 a minute, a tenth of what the + * spacing already allows. + * + * So on the serial path this is only the backstop for a process that has been hammering for an + * actual hour: 1000 is just under the ~1200 the spacing physically permits, so it never binds on a + * pipeline of any plausible size, while a genuine runaway still stops rather than running all day. * - * The number that matters for the instance is the SPACING above — it already caps a runaway loop at - * 20 handshakes a minute — so this is the backstop for a loop that keeps going, not the primary - * brake. A loop reaches 120 in about six minutes; a real pipeline does not reach it at all. + * The concurrent path is the one this number is a real trade on, and it is a trade made with open + * eyes. Because the count under-reports by the parallelism factor, N parallel runaways reach the cap + * after ~MAX_PER_HOUR rounds of 3s regardless of N — so raising 120 -> 1000 stretches how long they + * hammer before ct refuses from ~6 minutes to ~50. That is accepted because the alternative, 120, + * demonstrably failed ordinary serial CI pipelines, which is a certain cost paid by every user + * against an uncertain one paid by a misconfigured few; the spacing still holds each individual + * process to 20/min throughout, and an operator who runs ct fanned out wide should be setting + * CT_NO_LOGIN_THROTTLE=1 and rate-limiting at the orchestrator instead. */ -export const MAX_PER_HOUR = 120; +export const MAX_PER_HOUR = 1000; const HOUR_MS = 60 * 60 * 1000; /** What a throttle needs from its caller. Injected so a client in a test never touches the disk. */ diff --git a/tests/auth-token-command.test.ts b/tests/auth-token-command.test.ts index bbcbbae..7fc5f7a 100644 --- a/tests/auth-token-command.test.ts +++ b/tests/auth-token-command.test.ts @@ -129,3 +129,24 @@ describe("ct auth token", () => { expect(stderr.join("")).toMatch(/fresh login handshake/); }); }); + +/** + * The envelope is read by `data "external"` (terraform-provider-churchtools), which requires EVERY + * value in the JSON object to be a string. A single `null` fails inside the external provider with + * a message about JSON types that names neither this command nor the field — so the one credential + * helper in the system breaks in the least diagnosable way available. + */ +describe('ct auth token — an envelope `data "external"` can consume', () => { + it("reports no environment as an empty string, never null", async () => { + await run([]); + const parsed = JSON.parse(stdout[0]!) as Record; + expect(parsed.environment).toBe(""); + }); + + it("emits an object whose values are all strings", async () => { + await run([]); + const parsed = JSON.parse(stdout[0]!) as Record; + const nonStrings = Object.entries(parsed).filter(([, value]) => typeof value !== "string"); + expect(nonStrings).toEqual([]); + }); +});