Skip to content

fix(ci): P0 — publish templates/images only from master via protected environments + OIDC - #130

Open
lmeyerov wants to merge 9 commits into
dev/agent-skills-review-planfrom
dev/secure-publish-workflow
Open

fix(ci): P0 — publish templates/images only from master via protected environments + OIDC#130
lmeyerov wants to merge 9 commits into
dev/agent-skills-review-planfrom
dev/secure-publish-workflow

Conversation

@lmeyerov

@lmeyerov lmeyerov commented Oct 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Stacked on #129 (agent skills). Review that one first, or retarget this PR to master after #129 merges.
Operator setup is required before merge (environments, AWS OIDC role, DockerHub token; then key rotation). Steps: DEVELOP.md "One-time setup"; tracking and decisions: internal issue graphistry/graphistry#3610.

Problem (P0)

publish.yml uploaded the public CloudFormation templates to s3://graph-app-kit-repo-public/templates/latest/ from any branch, either by adding the publish label to a PR or by dispatching the workflow manually. These are the templates customers deploy from the README "Launch Stack" buttons. The upload used long-lived AWS keys stored as repo secrets, with no permissions: limits and actions referenced by tag (checkout@v3). dockerhubpublish.yml had the same any-branch exposure for the DockerHub token.

Fix

publish.yml

  • Triggers: push to master (only when templates or the workflow change) and workflow_dispatch. The PR labeled and schedule triggers are removed.
  • The job runs only when github.ref == 'refs/heads/master', inside the publish-templates environment (required reviewers, master-only).
  • AWS access is a GitHub OIDC role-to-assume; no stored keys. If AWS_S3_PUBLISH_ROLE_ARN is unset, the job fails closed.
  • permissions: {}; the job gets only contents: read + id-token: write.
  • Actions pinned to commit SHAs, persist-credentials: false, job-level concurrency keyed by ref.
  • Bucket and region fixed in the file; no ${{ }} interpolation in run:.

dockerhubpublish.yml

  • Master dispatch only, in the dockerhub-publish environment. This is how graphistry CI already drives it (base.yml dispatches on master with cuda_versions '["13","12"]' and polls the run); the workflow name, dispatch trigger, and master ref are preserved for that chain.
  • The tag-push trigger is removed: its pattern never matched a real tag (0 push runs in 133), and a tag path would let anyone who can create tags reach the token.
  • cuda_versions entries are validated (N or N.N), so a newline can't inject into GITHUB_ENV; default is now ["13","12"] to match current graphistry-forge-base tags.
  • Same permission, pinning, and env-var hygiene as publish.yml.

ci.yml / scan.yml declare explicit least-privilege permissions:, so the repo's default token can later be switched to read-only.

DEVELOP.md: ordered one-time setup runbook (IAM trust/permission policies included) and the graphistry→graph-app-kit release chain. CHANGELOG.md updated.

Verification

  • actionlint 1.7.7 passes on all changed workflows; .agents/skills/review/scripts/anti_pattern_scan.sh reports 0 signals on this range (on master it flags AP020/023/024/025/026 on publish.yml).
  • Action SHAs checked against their tags with gh api / git ls-remote; inputs checked against each action's action.yml at the pinned SHA.
  • Upload writes the same 4 S3 keys as before; bucket/region match every Launch Stack URL; s3:PutObject suffices (small single-part uploads, SSE-S3, no ACL).
  • Not exercised live: the OIDC publish and environment gates need the operator setup. The first approved run after merge is the live test; the next graphistry ci-labeled run is the live test of the DockerHub chain.

Review (.agents/skills/review, fixes=inline)

4 waves until convergence, isolated per-dimension subagents (spec, correctness, supply chain, operability), per-finding adversarial verification; later findings verified with gh api.

Wave New IMPORTANT Fixed
1 8 tag path to DockerHub token; IAM trust window / setup order; concurrency race; GITHUB_ENV injection; runbook gaps
2 3 self-approval path to master (runbook); forge-base tag docs; "test" dispatch would republish
3 2 read-only default and retirement ordering
4 0 suggestions only

Post-review: research of graphistry's CI/CD found the automated dispatch chain and a validation bug that would have failed it; fixed in bc8c03b. Verdict triage at important: PASS.

Deferred: docker login after build; unused version input; upgrade/pin actions in ci.yml/scan.yml; pre-existing scan.yml sarif_file bug (workflow disabled).

🤖 Generated with Claude Code

lmeyerov and others added 9 commits October 18, 2026 18:05
@lmeyerov @claude
fix(ci): restrict template + image publishing to master via protected…
b63e958 
… environments and OIDC

P0: publish.yml uploaded the public CloudFormation templates
(templates/latest/, what customers deploy) from any branch when a PR was
labeled `publish` or the workflow was dispatched, using long-lived AWS keys.

publish.yml:
- triggers: push to master (template/workflow paths) + workflow_dispatch;
  drop PR `labeled` and schedule triggers
- job guarded by `github.ref == 'refs/heads/master'` and the protected
  `publish-templates` environment (required reviewers, master-only)
- AWS via GitHub OIDC (`role-to-assume` from environment var
  AWS_S3_PUBLISH_ROLE_ARN; fails closed if unset); no stored keys
- `permissions: {}` default, job gets contents:read + id-token:write
- actions pinned to commit SHAs, persist-credentials: false, concurrency
  group, bucket/region fixed in-file, no `${{ }}` in run scripts

dockerhubpublish.yml (same any-branch exposure for the DockerHub token):
- only master or tags; `dockerhub-publish` environment; permissions;
  SHA pins; env vars instead of `${{ }}` interpolation in run scripts

DEVELOP.md documents the IAM trust/permission policies, environment setup
and retiring/rotating the old AWS keys and DockerHub token.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
fix(ci): job-level, ref-keyed concurrency for template publish
44c8860 
Review W1-C1: workflow-level group let a skipped non-master dispatch cancel a
pending master publish run.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
fix(ci): DockerHub publish only via master dispatch; validate cuda_ve…
bb936b9 
…rsions

Review W1-SC1/SC4: the tag-push trigger never matched a real tag, and keeping a
tag path would let any tag creator reach the DockerHub token through the
environment's tag policy. Validate matrix values before writing GITHUB_ENV so a
newline in the dispatch input cannot inject env vars.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
docs: ordered publish setup runbook (lock envs first, verify, then re…
2be73df 
…tire keys)

Review W1-SC2/SC3/O1-O8/S1-S5: environments are auto-created unprotected, so
lock them before creating the IAM role and before merge; replace (not move)
the DockerHub token; verify before deleting old keys; CHANGELOG states the
operator dependency; refresh stale DockerHub section.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
docs+ci: close self-approval path to master; fix DockerHub release docs
f44900e 
Review W2: with default write GITHUB_TOKEN and 'Allow GitHub Actions to
approve pull requests' on, a writer's workflow can approve its own PR and
reach master, defeating the master-only gates. Runbook now disables that and
sets read-only default permissions; ci.yml/scan.yml declare explicit
permissions so they keep working. Also: release tag must match a forge-base
tag, push only the release tag, name DOCKERHUB_TOKEN, and don't 'test' the
DockerHub path with a republish.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
docs: runbook ordering for read-only default + credential retirement;…
da21a05 
… audit other secrets

Review W3: switching the default token to read-only before merge breaks
master's scan.yml, so do it after merge (the approval checkbox is the actual
control and stays pre-merge). Retire old DockerHub creds right after the
template publish verifies instead of waiting for a release. Disable admin
bypass on the environments. Flag the unused GRAPHISTRY_GHA_CI_* repo secrets
for owner review.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
docs: wave-4 runbook polish (two reviewers, post-merge read-only defa…
fd49fbf 
…ult, no-bypass)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@lmeyerov @claude
fix(ci): accept graphistry CI's CUDA values (13/12) in dockerhubpubli…
bc8c03b 
…sh; document the chain

graphistry/graphistry base.yml dispatches this workflow on master with
cuda_versions '["13", "12"]' and polls it for 30 min; the N.N-only check
would have failed every such run, and the stale '["12.8", "11.8"]' default
matches no current forge-base tag. Validation now accepts N or N.N (still
rejects newlines). DEVELOP.md describes the graphistry-driven path as the
normal one.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
@lmeyerov @claude
docs: generic repo-secret guidance; specifics tracked internally
ba87f14 
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@lmeyerov