diff --git a/.changeset/major-parts-remain.md b/.changeset/major-parts-remain.md new file mode 100644 index 0000000000..1ad744aae3 --- /dev/null +++ b/.changeset/major-parts-remain.md @@ -0,0 +1,5 @@ +--- +"@tailor-platform/sdk-plugin-setup": patch +--- + +`tailor setup ci env --check` now warns when `TAILOR_PLATFORM_MACHINE_USER_CLIENT_SECRET` is set on the repository or the organization, where any workflow on any branch can read it and the GitHub Environment's protection rules do not guard it, and prints the `gh` commands that move it onto the environment. The check still passes. With `--json`, each entry's `exposedIn` lists where it was found outside its environment. diff --git a/packages/sdk-plugin-setup/src/commands.ts b/packages/sdk-plugin-setup/src/commands.ts index 4e762b68d9..f6c666d0f6 100644 --- a/packages/sdk-plugin-setup/src/commands.ts +++ b/packages/sdk-plugin-setup/src/commands.ts @@ -56,7 +56,7 @@ const envCommand = defineAppCommand({ }), check: arg(z.boolean().default(false), { description: - "Check with gh which of them are registered in GitHub, and print gh commands for the missing ones; fails when a required one is missing", + "Check with gh which of them are registered in GitHub, and print gh commands for the missing ones and for a machine user client secret to move onto its environment; fails when a required one is missing", }), }), run: (args) => { diff --git a/packages/sdk-plugin-setup/src/env-check.test.ts b/packages/sdk-plugin-setup/src/env-check.test.ts index 648f9e43cb..e5039e05fa 100644 --- a/packages/sdk-plugin-setup/src/env-check.test.ts +++ b/packages/sdk-plugin-setup/src/env-check.test.ts @@ -11,6 +11,7 @@ import type { GitRunner } from "./git"; const CLIENT_ID = "TAILOR_PLATFORM_MACHINE_USER_CLIENT_ID"; const CLIENT_SECRET = "TAILOR_PLATFORM_MACHINE_USER_CLIENT_SECRET"; const WORKSPACE_ID = "TAILOR_PLATFORM_WORKSPACE_ID"; +const SLACK_BOT_TOKEN = "TAILOR_SLACK_BOT_TOKEN"; type Entries = { secrets?: readonly string[]; variables?: readonly string[] }; @@ -23,6 +24,9 @@ type FakeGitHub = Entries & { responses?: Record; }; +/** Repository state plus the secrets of the `stg` environment. */ +type ClientSecretPlacement = FakeGitHub & { environmentSecrets?: readonly string[] }; + const ok = (stdout: string): GhOutput => ({ status: 0, stdout, stderr: "" }); const httpError = (code: number, message: string): GhOutput => ({ status: 1, @@ -184,12 +188,17 @@ describe("checkEnv", () => { checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh }); expect(output.stderr()).toMatch(/TAILOR_SLACK_BOT_TOKEN \(secret\): optional, not set/); - expect(output.stderr()).toMatch(/All required secrets and variables are registered/); + expect(output.stderr().split("\n")).toContain(` ✔ ${CLIENT_SECRET} (secret)`); + expect(output.stderr()).not.toContain("⚠"); + expect(output.stdout()).not.toContain("gh secret delete"); + expect(output.stderr().split("\n").at(-1)).toBe( + "All required secrets and variables are registered.", + ); }); test.each([ - ["repository", { secrets: [CLIENT_SECRET] }], - ["organization", { organization: { secrets: [CLIENT_SECRET] } }], + ["repository", { secrets: [SLACK_BOT_TOKEN] }], + ["organization", { organization: { secrets: [SLACK_BOT_TOKEN] } }], ] as const)( "counts a secret set on the %s as registered, since the workflow reads it from there", (scope, github) => { @@ -197,15 +206,127 @@ describe("checkEnv", () => { using output = captureOutput(); const gh = fakeGh({ ...github, - environments: { stg: { variables: [CLIENT_ID, WORKSPACE_ID] } }, + environments: { stg: { secrets: [CLIENT_SECRET], variables: [CLIENT_ID, WORKSPACE_ID] } }, + }); + + checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh }); + + expect(output.stderr()).toContain(`✔ ${SLACK_BOT_TOKEN} (secret, set on the ${scope})`); + }, + ); + + const UNGUARDED = "so the environment's protection rules do not guard it"; + const STG_SUMMARY = + "All required secrets and variables are registered, but any workflow on any branch can read " + + `${CLIENT_SECRET} outside the protection rules of environment "stg".`; + const SET = `gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`; + const DELETE = `# gh secret delete ${CLIENT_SECRET} --repo=acme/app`; + const HOW_TO_REUSE = "# How to get: the value set on the"; + const howToReuse = (scope: string) => + `${HOW_TO_REUSE} ${scope} (GitHub does not show secret values, so use the copy you stored)`; + + test.each([ + [ + "only on the repository", + { secrets: [CLIENT_SECRET] }, + `(secret, set on the repository): any workflow on any branch can read it from the repository, ${UNGUARDED}; ` + + "set it on the environment and delete it from the repository", + [howToReuse("repository"), SET, DELETE], + ], + [ + "only on the organization", + { organization: { secrets: [CLIENT_SECRET] } }, + `(secret, set on the organization): any workflow on any branch can read it from the organization, ${UNGUARDED}; ` + + "set it on the environment and have an organization owner stop sharing it with this repository", + [howToReuse("organization"), SET], + ], + [ + "on the repository and the organization", + { secrets: [CLIENT_SECRET], organization: { secrets: [CLIENT_SECRET] } }, + "(secret, set on the repository): any workflow on any branch can read it from the repository and the " + + `organization, ${UNGUARDED}; set it on the environment, delete it from the repository, and have an ` + + "organization owner stop sharing it with this repository", + [howToReuse("repository"), SET, DELETE], + ], + [ + "on the environment and also on the repository", + { secrets: [CLIENT_SECRET], environmentSecrets: [CLIENT_SECRET] }, + "(secret): set on the environment and also on the repository, where any workflow on any branch can " + + `read it, ${UNGUARDED}; delete it from the repository`, + [DELETE], + ], + [ + "on the environment and also on the organization", + { organization: { secrets: [CLIENT_SECRET] }, environmentSecrets: [CLIENT_SECRET] }, + "(secret): set on the environment and also on the organization, where any workflow on any branch can " + + `read it, ${UNGUARDED}; have an organization owner stop sharing it with this repository`, + [], + ], + ] as const)( + "warns, without failing, about the client secret set %s, and prints the gh commands that fix it", + (_, { environmentSecrets = [], ...github }: ClientSecretPlacement, line, commands) => { + using tmp = setup(); + using output = captureOutput(); + const gh = fakeGh({ + ...github, + environments: { + stg: { secrets: environmentSecrets, variables: [CLIENT_ID, WORKSPACE_ID] }, + }, }); checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh }); - expect(output.stderr()).toContain(`✔ ${CLIENT_SECRET} (secret, set on the ${scope})`); + const stderr = output.stderr().split("\n"); + expect(stderr).toContain(` ⚠ ${CLIENT_SECRET} ${line}`); + expect(output.stderr()).not.toContain(`✔ ${CLIENT_SECRET}`); + expect(stderr.at(-1)).toBe(STG_SUMMARY); + + const stdout = output.stdout().split("\n"); + expect( + stdout.filter((text) => text === SET || text === DELETE || text.startsWith(HOW_TO_REUSE)), + ).toEqual(commands); }, ); + test("moves a repository client secret to every environment reading it, deleting it once", () => { + using tmp = setup(target("branch", "my-app", "stg"), target("tag", "my-app", "production")); + using output = captureOutput(); + const gh = fakeGh({ + secrets: [CLIENT_SECRET], + environments: { + stg: { variables: [CLIENT_ID, WORKSPACE_ID] }, + production: { variables: [CLIENT_ID, WORKSPACE_ID] }, + }, + }); + + checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh }); + + const lines = output.stdout().split("\n"); + expect(lines).toContain(`gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`); + expect(lines).toContain(`gh secret set ${CLIENT_SECRET} --env=production --repo=acme/app`); + expect(lines.filter((line) => line.includes("gh secret delete"))).toEqual([ + `# gh secret delete ${CLIENT_SECRET} --repo=acme/app`, + ]); + expect(output.stderr().split("\n").at(-1)).toBe( + "All required secrets and variables are registered, but any workflow on any branch can read " + + `${CLIENT_SECRET} outside the protection rules of environments "stg" and "production".`, + ); + }); + + test("warns about a repository client secret for an environment that does not exist yet, creating it in the commands", () => { + using tmp = setup(); + using output = captureOutput(); + const gh = fakeGh({ secrets: [CLIENT_SECRET], variables: [CLIENT_ID, WORKSPACE_ID] }); + + checkEnv({ outputDir: tmp.dir, gitRunner: origin, ghRunner: gh }); + + expect(output.stderr()).toContain(`⚠ ${CLIENT_SECRET} (secret, set on the repository)`); + expect(output.stdout()).toContain('gh api -X PUT "repos/acme/app/environments/stg" --silent'); + expect(output.stdout().split("\n")).toContain( + `gh secret set ${CLIENT_SECRET} --env=stg --repo=acme/app`, + ); + }); + test("skips organization-level entries for a repository a user owns", () => { using tmp = setup(); using _output = captureOutput(); @@ -387,6 +508,7 @@ describe("checkEnv", () => { using output = captureOutput(); using _json = vi.spyOn(logger, "jsonMode", "get").mockReturnValue(true); const gh = fakeGh({ + secrets: [CLIENT_SECRET], environments: { stg: { secrets: [CLIENT_SECRET], variables: [CLIENT_ID] } }, }); @@ -409,6 +531,12 @@ describe("checkEnv", () => { registered: true, foundIn: "environment", otherTypeIn: null, + exposedIn: [], + }), + expect.objectContaining({ + name: CLIENT_SECRET, + foundIn: "environment", + exposedIn: ["repository"], }), expect.objectContaining({ name: WORKSPACE_ID, registered: false, foundIn: null }), ]), diff --git a/packages/sdk-plugin-setup/src/env-check.ts b/packages/sdk-plugin-setup/src/env-check.ts index 66519a3d0d..e20c0e887a 100644 --- a/packages/sdk-plugin-setup/src/env-check.ts +++ b/packages/sdk-plugin-setup/src/env-check.ts @@ -1,6 +1,7 @@ import { logger } from "@tailor-platform/sdk/cli"; import { describeGroup, + ENVIRONMENT_PREFERRED_SECRETS, type EnvRequirement, type EnvironmentRequirements, loadEnvironmentRequirements, @@ -21,6 +22,8 @@ type Scope = "environment" | "repository" | "organization"; type ScopedNames = { scope: Scope; names: Names }; +type SharedScope = Exclude; + type RequirementCheck = EnvRequirement & { /** Whether the workflow finds a value, counting the deprecated secret fallback. */ registered: boolean; @@ -28,6 +31,8 @@ type RequirementCheck = EnvRequirement & { foundIn: Scope | null; /** Scope holding an entry of the same name but the other type; null when none does. */ otherTypeIn: Scope | null; + /** Scopes outside the environment holding a secret that belongs on it; empty for other entries. */ + exposedIn: SharedScope[]; }; type EnvironmentCheck = Omit & { @@ -44,11 +49,21 @@ function checkRequirement(requirement: EnvRequirement, scopes: ScopedNames[]): R const otherTypeIn = foundIn === null ? scopeHolding(OTHER_TYPE[requirement.type]) : null; const fallback = requirement.type === "variable" && SECRET_FALLBACK_VARIABLES.has(requirement.name); + const checksEnvironment = scopes.some(({ scope }) => scope === "environment"); + const belongsOnEnvironment = + checksEnvironment && + requirement.type === "secret" && + ENVIRONMENT_PREFERRED_SECRETS.has(requirement.name); return { ...requirement, registered: foundIn !== null || (fallback && otherTypeIn !== null), foundIn, otherTypeIn, + exposedIn: belongsOnEnvironment + ? scopes.flatMap(({ scope, names }) => + scope !== "environment" && names.secret.has(requirement.name) ? [scope] : [], + ) + : [], }; } @@ -244,7 +259,26 @@ function report(checks: EnvironmentCheck[], fullName: string, repository: Reposi } printReport(checks, fullName, repository); if (missing > 0) throw new Error(`${summary} Set them with the gh commands printed above.`); - logger.success("All required secrets and variables are registered."); + const exposedGroups = checks.filter(({ requirements }) => + requirements.some((check) => check.exposedIn.length > 0), + ); + if (exposedGroups.length === 0) { + logger.success("All required secrets and variables are registered."); + return; + } + const secrets = new Set( + exposedGroups.flatMap(({ requirements }) => + requirements.filter((check) => check.exposedIn.length > 0).map((check) => check.name), + ), + ); + const environments = exposedGroups.flatMap(({ environment }) => + environment === null ? [] : [`"${environment}"`], + ); + logger.warn( + "All required secrets and variables are registered, but any workflow on any branch can read " + + `${andList.format([...secrets])} outside the protection rules of ` + + `${environments.length === 1 ? "environment" : "environments"} ${andList.format(environments)}.`, + ); } function printReport(checks: EnvironmentCheck[], fullName: string, repository: Repository): void { @@ -257,13 +291,31 @@ function printReport(checks: EnvironmentCheck[], fullName: string, repository: R logger.log(` ${statusLine(check)}`); } } - const unset = checks + const toSet = checks .map((check) => ({ ...check, - requirements: check.requirements.filter((r) => r.foundIn === null), + requirements: check.requirements.flatMap((r): EnvRequirement[] => { + if (r.foundIn === null) return [r]; + if (r.foundIn === "environment" || r.exposedIn.length === 0) return []; + return [ + { + ...r, + howTo: `the value set on the ${r.foundIn} (GitHub does not show secret values, so use the copy you stored)`, + }, + ]; + }), })) .filter((check) => check.requirements.length > 0); - if (unset.length > 0) logger.out(renderGhCommands(unset, repository)); + const toDelete = [ + ...new Set( + checks.flatMap(({ requirements }) => + requirements.filter((r) => r.exposedIn.includes("repository")).map((r) => r.name), + ), + ), + ]; + if (toSet.length > 0 || toDelete.length > 0) { + logger.out(renderGhCommands(toSet, repository, toDelete)); + } } function statusLine(check: RequirementCheck): string { @@ -275,9 +327,30 @@ function statusLine(check: RequirementCheck): string { if (check.registered && check.foundIn === null) { return `⚠ ${label}: set as a secret, a deprecated fallback; set it as a variable and delete the secret`; } + if (check.exposedIn.length > 0) return `⚠ ${label}: ${exposureWarning(check)}`; if (check.registered) return `✔ ${label}`; const status = check.required ? `✖ ${label}: missing` : `- ${label}: optional, not set`; if (check.otherTypeIn === null) return status; const other = OTHER_TYPE[check.type]; return `${status}; a ${other} of this name is set on the ${check.otherTypeIn}, but the workflow reads a ${check.type}`; } + +const andList = new Intl.ListFormat("en", { type: "conjunction" }); + +const REMOVE_FROM: Record = { + repository: "delete it from the repository", + organization: "have an organization owner stop sharing it with this repository", +}; + +function exposureWarning({ foundIn, exposedIn }: RequirementCheck): string { + const places = andList.format(exposedIn.map((scope) => `the ${scope}`)); + const fixes = [ + ...(foundIn === "environment" ? [] : ["set it on the environment"]), + ...exposedIn.map((scope) => REMOVE_FROM[scope]), + ]; + const exposure = + foundIn === "environment" + ? `set on the environment and also on ${places}, where any workflow on any branch can read it` + : `any workflow on any branch can read it from ${places}`; + return `${exposure}, so the environment's protection rules do not guard it; ${andList.format(fixes)}`; +} diff --git a/packages/sdk-plugin-setup/src/env.ts b/packages/sdk-plugin-setup/src/env.ts index e6d8cc20c1..e1eaa22f45 100644 --- a/packages/sdk-plugin-setup/src/env.ts +++ b/packages/sdk-plugin-setup/src/env.ts @@ -49,6 +49,9 @@ const CLIENT_SECRET: EnvRequirement = { description: "Client secret of the same platform machine user", howTo: `${MACHINE_USER_HOW_TO}; use its client secret`, }; +/** Secrets that belong on the environment, since its protection rules do not guard them elsewhere. */ +export const ENVIRONMENT_PREFERRED_SECRETS: ReadonlySet = new Set([CLIENT_SECRET.name]); + const WORKSPACE_ID: EnvRequirement = { name: "TAILOR_PLATFORM_WORKSPACE_ID", type: "variable", @@ -285,14 +288,17 @@ export function describeGroup(group: EnvironmentRequirements): string { } /** - * Render `gh` commands that create each missing environment and set its secrets and variables. + * Render `gh` commands that create each missing environment and set its secrets and variables, + * followed by commented-out commands that delete the given repository secrets. * @param environments - Requirements grouped by environment; `exists: true` skips creating it * @param repository - Target repository; when null, gh resolves it from the current directory + * @param repositorySecretsToDelete - Repository secrets to delete once the environments hold them * @returns Shell script text */ export function renderGhCommands( environments: (EnvironmentRequirements & { exists?: boolean })[], repository: Repository | null = null, + repositorySecretsToDelete: string[] = [], ): string { const repoPath = repository ? `${repository.owner}/${repository.name}` : "{owner}/{repo}"; const hostPrefix = repository?.host ? `${repository.host}/` : ""; @@ -320,6 +326,15 @@ export function renderGhCommands( } return lines.join("\n"); }); + if (repositorySecretsToDelete.length > 0) { + blocks.push( + [ + "# Repository secrets that any workflow on any branch can read. Delete each once every", + "# environment and workflow that reads it has its own; GitHub cannot restore a deleted secret.", + ...repositorySecretsToDelete.map((name) => `# gh secret delete ${name}${repoFlag}`), + ].join("\n"), + ); + } const header = [ "# Run these one at a time: each `gh ... set` prompts for its value.", ...(repository diff --git a/packages/sdk/docs/github-actions.md b/packages/sdk/docs/github-actions.md index e51cac7d97..4312383697 100644 --- a/packages/sdk/docs/github-actions.md +++ b/packages/sdk/docs/github-actions.md @@ -709,7 +709,11 @@ keeps them alongside that environment's `TAILOR_PLATFORM_WORKSPACE_ID` variable. You can also set the client secret as a repository-level secret and the client ID as a repository-level variable if every target shares one machine user, but then any workflow on any branch can read them, so the environment's -protection rules no longer guard your deploys. +protection rules no longer guard your deploys, and `tailor setup ci env --check` +warns about the client secret (see +[Checking what is registered](#checking-what-is-registered)). To share one machine +user and keep that protection, set the same client secret on each environment +instead. ### Setting secrets and variables @@ -784,13 +788,25 @@ to manage those settings in Terraform instead. environment and for the repository entries of the autofix workflow, which of these entries are registered. It fails when a required entry is missing, reports missing optional entries without failing, and prints -the `gh` commands that set only the missing entries, with where each value comes -from. It changes nothing, and it checks names only, since GitHub never returns -secret values. +the `gh` commands that set the missing entries, with where each value comes +from, and that move the client secret onto its environment (see below). It +changes nothing, and it checks names only, since GitHub never returns secret +values. - An entry set on the repository, or on the organization and shared with the repository, counts as registered, because the workflow reads it from there when the environment does not set it. The report says where each entry was found. +- `TAILOR_PLATFORM_MACHINE_USER_CLIENT_SECRET` set on the repository or the + organization is reported with ⚠, even when the environment also sets it: any + workflow on any branch can read it there, so the environment's protection rules + do not guard it. The check still passes. The printed commands set it on the + environment, with the value you set on the repository or organization, and list + a commented-out `gh secret delete` for the repository copy: run it once every + environment and workflow that reads that secret has its own. An organization + owner has to stop sharing an organization secret with the repository. The + warning stops once the client secret is set only on the environments; when + every target shares one machine user, set the same client secret on each of + them. - An entry registered as the other kind, such as `TAILOR_PLATFORM_WORKSPACE_ID` set as a secret, is reported as missing, because the workflow does not read it. `TAILOR_PLATFORM_MACHINE_USER_CLIENT_ID` set as a secret still counts, with a