From 0f141454823c3678490cfb0294ca09340e8e2dae Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 21 Sep 2026 10:55:58 +0200 Subject: [PATCH 1/3] fix(auth): emit environment as "" so the token envelope is consumable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `ct auth token` exists 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 JSON object to be a string. `environment` was the one field that could be null, and it is null in the common case — no `--env` passed. The external provider then fails with a message about JSON types that names neither this command nor the field, so the one credential helper in the system broke in the least diagnosable way available, on the default invocation. Every other field was already a string. Now this one is too. --- src/application/operations/auth.ts | 14 ++++++++++++-- tests/auth-token-command.test.ts | 21 +++++++++++++++++++++ 2 files changed, 33 insertions(+), 2 deletions(-) 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/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([]); + }); +}); From ea5d94fe129e84e2ea155610a9530469d77c8bb3 Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 21 Sep 2026 10:55:58 +0200 Subject: [PATCH 2/3] fix(auth): raise the login-handshake hourly cap to 1000 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The hourly cap was doing the rate limiting, and it is the wrong knob for it. MIN_INTERVAL_MS already caps sustained traffic at 20 handshakes a minute however hard anything loops, which is the conservative end of what a ChurchTools instance tolerates (operator estimate: 20-30 requests a minute, possibly 40). Anything the spacing permits is a rate the instance is fine with, so a cap that binds first is not protecting anything — it just fails commands that were never the problem. And it bound first by a wide margin. The gate sits in performLogin, so it covers every ct command, and there is no session cache off macOS, so on CI each invocation costs a handshake. 120/hour is an average of 2 a minute: a tenth of what the spacing already allows. 1000 sits 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 spacing is unchanged at 3s: a login handshake is heavier than a plain read, so the bottom of the tolerated band is the right default, and it costs CI almost nothing because a ct command usually takes longer than 3s anyway. --- docs/opentofu-migration.md | 14 ++++++++------ src/auth/loginThrottle.ts | 37 +++++++++++++++++++++++++++---------- 2 files changed, 35 insertions(+), 16 deletions(-) diff --git a/docs/opentofu-migration.md b/docs/opentofu-migration.md index 98974fc..d7da283 100644 --- a/docs/opentofu-migration.md +++ b/docs/opentofu-migration.md @@ -170,7 +170,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 +183,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/auth/loginThrottle.ts b/src/auth/loginThrottle.ts index 645bf10..be6e3f7 100644 --- a/src/auth/loginThrottle.ts +++ b/src/auth/loginThrottle.ts @@ -36,22 +36,39 @@ 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, and the only one that scales with how fast a caller goes: 3s means at most + * 20 handshakes a minute no matter how hard anything 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 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 this is NOT the rate limiter — the spacing + * above is. {@link MIN_INTERVAL_MS} already caps sustained traffic 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 is by definition a rate the instance + * is fine with, so an hourly cap that binds FIRST 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. * - * 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. + * So 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. */ -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. */ From d8ee7ebb4d98ff11d0c0334589a6dd3ff1a7646b Mon Sep 17 00:00:00 2001 From: Felix Kotschenreuther Date: Mon, 21 Sep 2026 11:02:08 +0200 Subject: [PATCH 3/3] docs(auth): be honest about what the spacing actually bounds MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two review follow-ups, both documentation; no behaviour changes. The rationale added for raising MAX_PER_HOUR claimed MIN_INTERVAL_MS caps traffic at 20 handshakes a minute "no matter how hard anything loops". The module header says the opposite 30 lines above, and it is right: the counter is read-modify-written without a lock, so parallel callers sleep the same wait together and fire in lockstep. Measured it — 10 concurrent acquires over 40 rounds produced 400 handshakes and recorded 40, under-reporting by exactly the parallelism factor. That matters for the number chosen: because the count under-reports by N, N parallel runaways reach the cap after ~MAX_PER_HOUR rounds regardless of N, so 120 -> 1000 stretches a concurrent runaway from ~6 minutes of hammering to ~50. The trade is still the right one — 120 demonstrably failed ordinary serial CI pipelines, a certain cost to every user against an uncertain one to a misconfigured few — but it is a trade, and the comment now says so instead of resting on a bound that only holds serially. Also states the property the token-envelope fix exists to create in the migration guide's contract list: every value is a string, and environment is "" rather than null on the default invocation. A provider author reading that page could not previously tell what the un-flagged envelope looks like, which is precisely the case that was broken. --- docs/opentofu-migration.md | 4 ++++ src/auth/loginThrottle.ts | 40 +++++++++++++++++++++++++------------- 2 files changed, 31 insertions(+), 13 deletions(-) diff --git a/docs/opentofu-migration.md b/docs/opentofu-migration.md index d7da283..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 diff --git a/src/auth/loginThrottle.ts b/src/auth/loginThrottle.ts index be6e3f7..a58c132 100644 --- a/src/auth/loginThrottle.ts +++ b/src/auth/loginThrottle.ts @@ -39,10 +39,15 @@ import { hostSlug } from "../permissions/catalog-store.js"; /** * Minimum spacing between two handshakes against one host. Waited out, never an error. * - * This is the real brake, and the only one that scales with how fast a caller goes: 3s means at most - * 20 handshakes a minute no matter how hard anything 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. + * 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. @@ -51,12 +56,12 @@ export const MIN_INTERVAL_MS = 3_000; /** * Handshakes per rolling hour per host before `ct` refuses to add to the pile. * - * Deliberately far above anything legitimate, because this is NOT the rate limiter — the spacing - * above is. {@link MIN_INTERVAL_MS} already caps sustained traffic 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 is by definition a rate the instance - * is fine with, so an hourly cap that binds FIRST is not protecting the instance — it is just - * failing commands that were never the problem. + * 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 @@ -64,9 +69,18 @@ export const MIN_INTERVAL_MS = 3_000; * 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 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. + * 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 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 = 1000; const HOUR_MS = 60 * 60 * 1000;