diff --git a/.github/actionlint.yaml b/.github/actionlint.yaml index 3a842c6..a2d0b71 100644 --- a/.github/actionlint.yaml +++ b/.github/actionlint.yaml @@ -1,8 +1,7 @@ # actionlint needs to be told about self-hosted runner labels, or every # `runs-on:` referring to one is an error and the real findings drown in noise. # -# mdb-dev newdev EKS pod, IRSA into the build/ECR account -# mdb-prod newprod EKS pod +# mdb-dev, mdb-prod the org's self-hosted runner pools self-hosted-runner: labels: - mdb-dev diff --git a/.github/workflows/notify-startup-failure.yml b/.github/workflows/notify-startup-failure.yml index f818a78..d9cc65b 100644 --- a/.github/workflows/notify-startup-failure.yml +++ b/.github/workflows/notify-startup-failure.yml @@ -19,9 +19,10 @@ # - malformed YAML, an unknown `on:` filter, a bad matrix expression # # Its consequence is the worst-shaped one available: a merge to a deploy branch -# reports nothing, so the branch is believed deployed and is not. Two pushes to -# auth `staging` sat undeployed for ten hours this way, and the first thing the -# channel heard was the RECOVERY message from the run that fixed it. +# reports nothing, so the branch is believed deployed and is not. It has already +# happened to a consuming repo, whose deploy branch sat undeployed for hours across +# two merges, and where the first thing the engineering channel heard about it was +# the RECOVERY message from the run that fixed it. # # So this workflow watches from outside, on a schedule. # diff --git a/.github/workflows/pr-env-comment.yml b/.github/workflows/pr-env-comment.yml index e0817b9..89eedc9 100644 --- a/.github/workflows/pr-env-comment.yml +++ b/.github/workflows/pr-env-comment.yml @@ -1,41 +1,48 @@ # Reusable workflow: post (and keep updating) one comment telling reviewers where -# a pull request's environment is and how to sign in. +# a pull request's environment is. # # Every repo that deploys a PR environment wants this and only one had it, so the # reviewer's first move on the other four was to reconstruct a hostname from a # namespace convention or ask someone. The mechanism belongs here. # -# THE FACTS DO NOT. This repo is public and has to stay public (a public caller -# may only consume public reusable workflows, and cowork, cowork-server, and anton -# are public callers), so anything hard-coded here is world-readable. The account -# you sign in as, the Secret and Secrets Manager bundle holding its password, the -# namespace convention, and the internal host pattern are not credentials, but -# together they are a free recon package: which username to spray, and exactly -# what to reach for once inside a cluster. +# IT CARRIES NO CREDENTIAL DETAIL. Not the password (obviously), but also not the +# account name, not the Secret and key holding the password, and not the command +# that reads it. That is a deliberate narrowing, and the reasoning is worth keeping +# because the convenience argument is seductive: # -# So every one of those arrives as an INPUT from the private caller. This file -# renders a table and maintains one marker-keyed comment; it names nothing of ours. -# A reusable that cannot be described without naming our infrastructure has not -# earned promotion and stays in the repo that owns it. +# A `kubectl get secret` line grants nobody any capability. Anyone who can run it +# can already read every Secret in that namespace. So it helps only the people who +# least need help, and it pays for that by publishing the account name, the Secret +# name, and the key into a permanent, org-wide, searchable surface — a free map of +# what to grab, for whoever eventually gets cluster read. # -# Layout is deliberate: the two things a reviewer actually wants — who to sign in -# as, and the one command that prints the password — come FIRST, before any links -# or caveats. The previous version led with four paragraphs of rationale and put -# the account halfway down. +# Credentials live in the access-controlled runbook, which is one place to update and +# one place to audit. This comment links it. `access-doc-url` is that link. # -# It never posts the password itself. A PR comment is permanent, org-wide, and -# un-redactable, and the credential is one `kubectl` away for anyone who can reach -# the cluster; printing it here would put a live admin password in the timeline of -# every PR forever. +# THE FACTS STAY WITH THE CALLER. This repo is public and has to stay public (a +# public caller may only consume public reusable workflows, and cowork, +# cowork-server, and anton are public callers), so anything hard-coded here is +# world-readable. Hosts, namespace conventions, and the runbook link all arrive as +# INPUTS. A reusable that cannot be described without naming our infrastructure has +# not earned promotion and stays in the repo that owns it. # -# k8s-secret-ok: the `kubectl get secret` this renders is a command for a human to -# run, assembled from caller inputs. This workflow never reads the value. +# On a PUBLIC repo, `access-doc-url` and `notes` are dropped: both describe internals +# by nature. `links` are NOT dropped, because only the caller knows what a link points +# at — an internal dev hostname is worth suppressing, a build artifact on a run that +# is already public in the Actions tab is not. Composing the links is the caller's +# job, so judging them is too. # -# The calling job must grant `pull-requests: write`, and no job here declares it: a -# called workflow can never exceed its caller's grant and that cap is enforced when -# the file LOADS, so a scope named here would reject the run of any caller that -# does not grant it — including, in a shared pipeline, the push callers that never -# comment on anything. +# ON PERMISSIONS. The `comment` job DECLARES `pull-requests: write` rather than +# inheriting it, and that is deliberate after getting it wrong once: a job that +# declares nothing is invisible to the permission gate, so a caller that forgets the +# grant fails at RUN time with an opaque `HTTP 403` on the first `gh api` call +# instead of failing the lint. Declaring it means every caller must grant it, which +# is exactly the relationship the gate verifies. +# +# The consequence is that this workflow must be called from the pull-request pipeline +# DIRECTLY, not from a shared build-and-deploy reusable that push pipelines also +# call — otherwise those callers would have to grant a PR scope they never use, on +# the release path. name: PR environment comment @@ -43,37 +50,31 @@ on: workflow_call: inputs: env-name: - description: "The environment's name, e.g. pr-auth-123. Shown as the heading." + description: "What this comment is about — an environment name, a build kind. Shown in the heading." type: string required: true + heading: + description: >- + Heading text, with the env-name appended. Defaults to the per-PR + environment wording; a caller posting something else (installer + artifacts, say) supplies its own. + type: string + default: "PR environment is up" links: description: >- JSON array of {"label","url"} objects, rendered as a table in order. e.g. '[{"label":"Console","url":"https://..."}]' type: string default: "[]" - login-email: - description: "Account to sign in as. Omitted from the comment when empty." - type: string - default: "" - login-note: - description: "One short line about that account (realm, roles it carries)." - type: string - default: "" - password-namespace: - description: "Namespace for the rendered password command. Defaults to env-name." - type: string - default: "" - password-secret: - description: "Secret holding the password. No command is rendered when empty." - type: string - default: "" - password-key: - description: "Key within that Secret." - type: string - default: "" - password-source-note: - description: "Where the same value comes from upstream, e.g. a Secrets Manager bundle name." + access-doc-url: + description: >- + Link to the runbook covering how to sign in: which account, and how to + obtain its password. + + A LINK, and never the method. Anything that lets a reader obtain a + credential is the credential, and a PR comment is a permanent, searchable, + un-redactable place to put one. Keep the method behind whatever already + guards it. type: string default: "" notes: @@ -96,21 +97,21 @@ defaults: jobs: comment: runs-on: ${{ inputs.runs-on }} + permissions: + contents: read + pull-requests: write # the rolling comment steps: - - name: Post the environment access details + - name: Post the environment details env: GH_TOKEN: ${{ github.token }} PR: ${{ github.event.pull_request.number }} ENV_NAME: ${{ inputs.env-name }} LINKS: ${{ inputs.links }} - LOGIN_EMAIL: ${{ inputs.login-email }} - LOGIN_NOTE: ${{ inputs.login-note }} - PW_NS: ${{ inputs.password-namespace || inputs.env-name }} - PW_SECRET: ${{ inputs.password-secret }} - PW_KEY: ${{ inputs.password-key }} - PW_SOURCE: ${{ inputs.password-source-note }} + DOC_URL: ${{ inputs.access-doc-url }} NOTES: ${{ inputs.notes }} MARKER_KEY: ${{ inputs.marker }} + HEADING: ${{ inputs.heading }} + IS_PRIVATE: ${{ github.event.repository.private }} run: | set -euo pipefail @@ -119,28 +120,21 @@ jobs: exit 1 fi + # Fail closed: `private` is present on a pull_request payload, and if it + # ever is not, the safe reading is "assume public". + if [ "${IS_PRIVATE:-false}" != "true" ]; then + LINKS="[]" + DOC_URL="" + NOTES="" + echo "Public repository: posting the environment name only." + fi + MARKER="" { echo "${MARKER}" - echo "## PR environment is up · \`${ENV_NAME}\`" + echo "## ${HEADING} · \`${ENV_NAME}\`" echo - # The two things a reviewer came for, first. - if [ -n "$LOGIN_EMAIL" ]; then - echo -n "**Sign in as** \`${LOGIN_EMAIL}\`" - [ -n "$LOGIN_NOTE" ] && echo -n " — ${LOGIN_NOTE}" - echo - echo - fi - if [ -n "$PW_SECRET" ] && [ -n "$PW_KEY" ]; then - echo "**Password** (not in this comment on purpose, it is a live credential and a PR comment is permanent):" - echo - echo '```bash' - echo "kubectl -n ${PW_NS} get secret ${PW_SECRET} -o jsonpath='{.data.${PW_KEY}}' | base64 -d; echo" - echo '```' - echo - fi - if [ "$(jq 'length' <<< "${LINKS:-[]}")" != "0" ]; then echo "| | |" echo "| :--- | :--- |" @@ -148,16 +142,21 @@ jobs: echo fi - if [ -n "$NOTES" ] || [ -n "$PW_SOURCE" ]; then + if [ -n "$DOC_URL" ]; then + echo "**Signing in:** [how to get an account and its password](${DOC_URL}). Deliberately not in this comment — a PR comment is permanent and world-visible to the org." + echo + fi + + if [ -n "$NOTES" ]; then echo "
More" echo - [ -n "$PW_SOURCE" ] && { echo "${PW_SOURCE}"; echo; } - [ -n "$NOTES" ] && { echo "${NOTES}"; echo; } + echo "${NOTES}" + echo echo "
" echo fi - echo "Rebuilt on every push to this PR. Torn down when the PR closes or loses its \`deploy\` label." + echo "Updated on every push to this PR." } > body.md # One rolling comment per PR rather than a new one per push. diff --git a/.github/workflows/workflow-lint.yml b/.github/workflows/workflow-lint.yml index 0df52e7..4d30bc3 100644 --- a/.github/workflows/workflow-lint.yml +++ b/.github/workflows/workflow-lint.yml @@ -10,8 +10,8 @@ # permissions The one check neither tool does: a called workflow may never # declare a permission its caller lacks. Blocking. See # scripts/workflow_graph.py for why this is not redundant — -# short version, it was verified against a tree that had already -# broken production and neither tool said a word. +# short version, it was verified against a real tree that had +# already broken a deploy branch, and neither tool said a word. # zizmor The established Actions SECURITY auditor (template injection, # credential persistence, unpinned actions, excessive permissions). # Advisory by default, because pointing it at existing pipelines diff --git a/README.md b/README.md index e01dcfa..c272a9c 100644 --- a/README.md +++ b/README.md @@ -156,9 +156,9 @@ Blind spot to know about: it can only read local (`./.github/workflows/...`) cal ```yaml - uses: mindsdb/github-actions/k8s-secret@main with: - namespace: pr-auth-123 - secret: keycloak-secrets - key: PRIVATE_CLIENT_SECRET + namespace: pr--123 + secret: + key: env-var: PR_KEYCLOAK_CLIENT_SECRET ``` @@ -168,7 +168,7 @@ There is no blanket answer, and "prefer Kubernetes because it is the source of t | The value is | Source | Why | | --- | --- | --- | -| Ephemeral and namespace-local (a per-PR environment's own credentials) | **Kubernetes**, via `k8s-secret` | No GitHub Environment can exist for `pr-auth-204`, so a copy is impossible; the namespace is the only source there is | +| Ephemeral and namespace-local (a per-PR environment's own credentials) | **Kubernetes**, via `k8s-secret` | No GitHub Environment can exist for `pr--204`, so a copy is impossible; the namespace is the only source there is | | A permanent environment's real credential (prod/staging DB, Stripe live, prod vendor tokens) | **GitHub Environment** | A job can only read it by declaring `environment: prod`, and that environment requires a reviewer. A k8s Secret has no such gate — *any* job on a runner with cluster read can fetch it, unreviewed | | Needed to reach the cluster, or used on a GitHub-hosted runner | **GitHub secret** | Package-install tokens, the ArgoCD token that creates the namespace, Snyk, Slack. Reading these from the cluster is circular | @@ -189,7 +189,7 @@ What actually goes wrong with a GitHub secret is different and has a cheaper fix # `pull-requests: write`; the push callers must not have to. uses: mindsdb/github-actions/.github/workflows/pr-env-comment.yml@main with: - env-name: pr-auth-${{ github.event.pull_request.number }} + env-name: pr--${{ github.event.pull_request.number }} login-email: someone@example.com password-secret: some-secret password-key: SOME_KEY diff --git a/k8s-secret/action.yml b/k8s-secret/action.yml index 289f0a7..603e6cb 100644 --- a/k8s-secret/action.yml +++ b/k8s-secret/action.yml @@ -5,10 +5,10 @@ # Both patterns are in use across these repos. The honest comparison: # # A GitHub Environment secret is auto-masked, needs no cluster, and is invisible -# to fork PRs. But it is a SECOND COPY of a value whose source of truth is AWS -# Secrets Manager (see auth's scripts/secrets/create-aws-secrets.py), duplicated -# per environment, rotated by hand in lockstep — and a reference to a secret that -# no longer exists resolves to the EMPTY STRING rather than failing. That is a +# to fork PRs. But it is a SECOND COPY of a value whose source of truth is a +# secrets manager that already populates the cluster, duplicated per environment +# and rotated by hand in lockstep — and a reference to a secret that no longer +# exists resolves to the EMPTY STRING rather than failing. That is a # real failure mode, not a theoretical one: a renamed token left a config job # authenticating with "" and reporting a vendor 401 instead of "the secret is # gone". diff --git a/scripts/workflow_graph.py b/scripts/workflow_graph.py index 6388e2a..9371707 100644 --- a/scripts/workflow_graph.py +++ b/scripts/workflow_graph.py @@ -31,9 +31,9 @@ That failure mode is uniquely bad. The run has zero jobs, so the pipeline's own terminal notify job cannot fire, and nothing anywhere says the push did not -deploy. It cost this repo two undeployed merges to ``staging`` and ten hours of -serving the previous image, and the first thing the engineering channel heard -about it was the *recovery* message from the run that fixed it. +deploy. It has already cost a consuming repo two undeployed merges to its deploy +branch, where the first thing the engineering channel heard about it was the +*recovery* message from the run that fixed it. No existing tool catches it, which is the only reason this file exists. Verified against the failing tree: ``actionlint`` reports nothing (it does not resolve the @@ -52,10 +52,18 @@ within the calling job's effective grant which in practice means a job in a shared reusable declares only what ALL its -callers grant and INHERITS anything only one of them needs. Inheriting is not a -loss of precision, it is the only thing that composes: the PR caller grants -``pull-requests: write`` and the comment posts, the push callers grant -``contents: read`` and the same job runs without the scope it never needed. +callers grant. Anything only ONE caller needs does not belong in a shared reusable +at all: move that job into the caller that has the grant. + +Do not reach for "just inherit it" — that was tried and it is a trap. A job with no +``permissions:`` block declares nothing for this check to compare, so it is +invisible here, and a caller that forgot the grant fails at RUN time with an opaque +``HTTP 403`` on its first API call rather than failing the lint. Declaring the scope +is what makes the relationship checkable at all. + +The blind spot that remains: this check can see what a job DECLARES, never what a +job NEEDS. A job that quietly requires a scope nobody granted is not detectable +statically, which is the second reason to declare rather than inherit. Remote callees (``uses: org/repo/.github/workflows/x.yml@ref``) cannot be read from here and are reported as unchecked rather than assumed fine. @@ -97,7 +105,7 @@ # What the workflow token carries when nothing declares anything. This is the # repo's "Read repository contents and packages permissions" setting; confirm with -# gh api repos/mindsdb/auth/actions/permissions/workflow +# gh api repos///actions/permissions/workflow DEFAULT_GRANTS: dict[str, dict[str, str]] = { "read": {"contents": "read", "packages": "read"}, "write": {scope: "write" for scope in SCOPES}, @@ -138,8 +146,10 @@ def __str__(self) -> str: f"but {where} declares {self.scope}: {self.declared}\n" f" call chain: {chain}\n" f" every run of {self.caller} would be rejected as a startup_failure, with no job to report it.\n" - f" fix: drop the `permissions:` block in the callee so it inherits the caller's ceiling, " - f"or grant {self.scope}: {self.declared} on `{self.caller_job}` if every caller should hold it." + f" fix: grant {self.scope}: {self.declared} on `{self.caller_job}` if EVERY caller should hold it, " + f"or move that callee job into the one caller that needs it. Do not just delete the callee's " + f"`permissions:` block: it silences this check without giving the job the scope, and the failure " + f"comes back at run time as an opaque 403." ) @@ -207,14 +217,26 @@ def level_of(grants: dict[str, str], scope: str) -> str: return grants.get(scope, "none") +# A workflow that must keep its own run tree says so, with a reason, the same way +# the secret-hygiene rule is opted out of. Written into the file rather than a +# central list, because the reason belongs next to the thing it excuses. +RUN_TREE_EXEMPT = "run-tree-ok:" + +EXEMPT: set[str] = set() + + def load_workflows(workflow_dir: Path) -> dict[str, dict]: """Parse every workflow, keyed by the `./.github/workflows/x.yml` form callers use.""" workflows: dict[str, dict] = {} + EXEMPT.clear() for path in sorted(workflow_dir.glob("*.y*ml")): - with path.open(encoding="utf-8") as handle: - parsed = yaml.safe_load(handle) + raw = path.read_text(encoding="utf-8") + parsed = yaml.safe_load(raw) if isinstance(parsed, dict): - workflows[f"{LOCAL_PREFIX}{path.name}"] = parsed + key = f"{LOCAL_PREFIX}{path.name}" + workflows[key] = parsed + if RUN_TREE_EXEMPT in raw: + EXEMPT.add(key) return workflows @@ -386,7 +408,7 @@ def check_run_trees( by_event: dict[str, list[str]] = {} for key, workflow in workflows.items(): - if not is_entry_point(workflow): + if not is_entry_point(workflow) or key in EXEMPT: continue for event in event_keys(workflow): by_event.setdefault(event, []).append(key) @@ -400,7 +422,10 @@ def check_run_trees( f" Fold the side ones into the event's pipeline as `workflow_call` jobs, so one run " f"shows everything that happened. A job that should not pay for itself on every run " f"gates on a path check; a job behind an `environment:` approval needs an ungated plan " - f"job before it, since an approval blocks a job from starting." + f"job before it, since an approval blocks a job from starting.\n" + f" If a workflow genuinely has to keep its own run tree (a vendor OIDC claim bound to its " + f"filename, a release-train hook on someone else's event), add a `run-tree-ok: ` " + f"comment to it." ) if not allow_external_reusables: diff --git a/tests/test_workflow_graph.py b/tests/test_workflow_graph.py index ca381ae..15213cd 100644 --- a/tests/test_workflow_graph.py +++ b/tests/test_workflow_graph.py @@ -1,11 +1,10 @@ """Unit tests for the workflow permission gate (``scripts/workflow_graph.py``). -The gate exists because of a real incident in ``mindsdb/auth``: -``config-apply.yml``'s ``detect`` and ``plan`` jobs declared a ``pull-requests`` -scope that the staging and prod push callers did not grant, GitHub rejected those -runs at file load with ``startup_failure`` and zero jobs, and two merges to -``staging`` sat undeployed for ten hours with nothing said — the terminal notify -job cannot fire in a run that never started. +The gate exists because of a real failure in a consuming repo: a shared reusable's +jobs declared a ``pull-requests`` scope that the push callers did not grant, GitHub +rejected those runs at file load with ``startup_failure`` and zero jobs, and merges +to a deploy branch went undeployed with nothing said — the terminal notify job +cannot fire in a run that never started. These tests pin the rule and, at the bottom, replay that exact shape, so the class cannot come back the next time someone adds a scope to a shared reusable, in any @@ -342,7 +341,8 @@ def test_the_finding_says_what_would_happen_and_how_to_fix_it(self, tmp_path): message = str(violations[0]) assert "startup_failure" in message assert "no job to report it" in message - assert "inherits the caller's ceiling" in message + assert "if EVERY caller should hold it" in message + assert "Do not just delete" in message, "the wrong fix has to be named as wrong" def test_inheriting_in_the_callee_is_what_resolves_it(self, tmp_path): """The fix that composes: the callee stops naming the scope only one caller grants.""" @@ -426,6 +426,30 @@ def test_an_uncalled_reusable_is_a_note_not_an_error(self, tmp_path): assert errors == [] assert len(notes) == 1 and "nothing in this repo calls" in notes[0] + def test_a_workflow_can_be_exempted_with_a_written_reason(self, tmp_path): + """Some duplication is deliberate: a vendor OIDC claim bound to a filename, or a + release-train hook that fires on another pipeline's event. The exemption is a + comment in the file, so the reason lives next to the thing it excuses.""" + write(tmp_path, "prod.yml", {"on": {"push": {"branches": ["main"]}}, "jobs": {"a": {"runs-on": "x"}}}) + (tmp_path / "hook.yml").write_text( + "# run-tree-ok: fires on the release PR merging, not on this pipeline's behalf\n" + "name: Hook\non:\n push:\n branches: [main]\njobs:\n b:\n runs-on: x\n", + encoding="utf-8", + ) + errors, _ = gate.check_run_trees(gate.load_workflows(tmp_path)) + assert errors == [] + + def test_an_unexcused_duplicate_is_still_an_error_alongside_an_exempt_one(self, tmp_path): + write(tmp_path, "prod.yml", {"on": {"push": {"branches": ["main"]}}, "jobs": {"a": {"runs-on": "x"}}}) + write(tmp_path, "other.yml", {"on": {"push": {"branches": ["main"]}}, "jobs": {"c": {"runs-on": "x"}}}) + (tmp_path / "hook.yml").write_text( + "# run-tree-ok: deliberate\nname: Hook\non:\n push:\n branches: [main]\njobs:\n b:\n runs-on: x\n", + encoding="utf-8", + ) + errors, _ = gate.check_run_trees(gate.load_workflows(tmp_path)) + assert len(errors) == 1 + assert "hook.yml" not in errors[0] + def test_a_library_repo_can_silence_the_orphan_note(self, tmp_path): """This repo's reusables are called from other repos, by design.""" write(tmp_path, "orphan.yml", {"on": {"workflow_call": None}, "jobs": {"a": {"runs-on": "x"}}})