Skip to content

feat(helm): add nullify-k8s-readonly-access chart for managed EKS scanning - #61

Merged
tim-thacker-nullify merged 18 commits into
fix/helm-publish-every-versionfrom
feat/helm-readonly-access-chart
Sep 17, 2026
Merged

tim-thacker-nullify merged 18 commits into
fix/helm-publish-every-versionfrom
feat/helm-readonly-access-chart

Conversation

@tim-thacker-nullify

@tim-thacker-nullify tim-thacker-nullify commented Sep 14, 2026 •

Copy link
Copy Markdown
Member

Claude

Customers had no way to install the in-cluster RBAC for the managed EKS scan: it is not in the collector chart, and no manifest exists. Merge after #60, which this is stacked on; the old release workflow would unpublish the collector chart when it released this one.

  • New chart helm-charts/nullify-k8s-readonly-access 0.1.0. ClusterRole nullify-readonly grants list only, on exactly the 26 kinds the managed scanner lists. ClusterRoleBinding binds it to Group groupName (default nullify-readonly).
  • Guards: an empty groupName, or a system: group in groupName or extraSubjects, fails the render; extraRules accepts only get/list/watch.
  • grantSecretsRead and grantConfigMapsRead default to true. Their comments, NOTES and README say that setting either to false currently fails the strict scan for the whole cluster, until the Nullify-side change ships.
  • extraRules, extraSubjects, labels, annotations.
  • Chart README covers the access entry, Helm, Flux (HelmRepository/HelmRelease and GitRepository/Kustomization) and kubectl installs, and a kubectl auth can-i list <res> --as nullify-verify --as-group nullify-readonly verify loop. It warns about AmazonEKSAdminViewPolicy (subresources via *, so pods/exec on EKS <= 1.34) and says AmazonEKSViewPolicy fails the scan.
  • manifests/nullify-readonly-rbac.yaml: the default render without Helm's bookkeeping labels. The CI job rbac-manifest (scripts/check-rbac-manifest.sh) renders the chart and diffs, and release needs it.
  • CI cases: ci/defaults.yaml, ci/customised.yaml, and four tests/helm/.../must-fail values files.
  • Root README: quick-start row, a short managed-scan section with a collector comparison, and structure/docs entries.

Deliberately not granted: /version. The default system:public-info-viewer binding already serves it to every authenticated principal, and the chart grants list on these 26 kinds and nothing else.

Nothing was rendered locally; CI on this PR runs lint, render, the must-fail cases and the manifest diff.

Collapsed stack

Absorbed after passing adversarial review: #66, #71, #76.

Merge order (required)

🤖 Generated with Claude Code


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.

…nning

New chart nullify-k8s-readonly-access 0.1.0: a ClusterRole with list only on
the 26 kinds the managed EKS scanner lists, bound to a Group (default
nullify-readonly). Renders fail for an empty or system: group and for
non-read extraRules verbs. grantSecretsRead and grantConfigMapsRead default to
true because the strict scanner fails the cluster without them today.

manifests/nullify-readonly-rbac.yaml is the default render without Helm's
bookkeeping labels, and a CI job diffs the two. The chart README covers Helm,
Flux and kubectl installs, impersonation-based verification, and why
AmazonEKSAdminViewPolicy and AmazonEKSViewPolicy are not the recommended path.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@tim-thacker-nullify
tim-thacker-nullify force-pushed the feat/helm-readonly-access-chart branch from 31d9112 to a2e6145 Compare September 14, 2026 15:41
tim-thacker-nullify and others added 4 commits September 15, 2026 01:46
… nothing

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ra rules

- extraSubjects: any name starting "system:" (trimmed, case-insensitive)
  fails whatever its kind; kinds are limited to User, Group and
  ServiceAccount; ServiceAccounts in kube-system, kube-public and
  kube-node-lease fail.
- extraRules: reject wildcard apiGroups or resources, the exec, attach,
  portforward, proxy and log subresources, and nonResourceURLs other than
  /version.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The strict managed scan records a ServerVersion() failure and fails the
cluster, so a cluster without the default system:public-info-viewer binding
could not be scanned. The raw manifest carries the same rule, and the
manifest check fails if either side drops it. NOTES lists the egress IPs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ctor Namespace

- Helm 3.2+ adopts the manifest's objects once they carry Helm ownership
  metadata, so moving to Helm no longer needs a delete and a scan outage.
- List Nullify egress IPs per region for the EKS public endpoint.
- The collector no longer creates a Namespace (#63).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tim-thacker-nullify and others added 2 commits September 15, 2026 02:41
…est check

helm-charts/nullify-k8s-readonly-access/ci/pre-publish.sh runs
scripts/check-rbac-manifest.sh. The #65 follow-up's publish script runs a
chart's pre-publish hook before packaging it and skips the chart when the hook
fails, so manifest drift blocks the release and its tag whatever the release
job's needs.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…immed group

- extraRules resources: any subresource other than status and scale is
  rejected, so CRD subresources such as KubeVirt's virtualmachineinstances/console
  and /vnc/screenshot no longer render. New must-fail cases cover both.
- The ClusterRoleBinding subject and NOTES use the trimmed groupName the guard
  checks, not the raw value.
- The Helm adoption steps take the release name and storage namespace as
  variables, with the Flux HelmRelease values (flux-system unless
  storageNamespace is set) alongside the Helm CLI ones.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- .helmignore excludes ci/, so editing the pre-publish hook or a CI values file
  no longer changes the packaged contents of a published version (which forced a
  version bump) and the hook script is not shipped to users.
- scripts/test-helm-charts.sh reads ci/*.yaml from the chart directory, so lint
  and render coverage is unchanged.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@dnjg

dnjg commented Sep 15, 2026

Copy link
Copy Markdown
Member

Has conflicts

The managed EKS scan now lists Jobs to attribute CronJob-run pods to their
CronJob. Without the grant the list is skipped and those pods key on the Job.

- ClusterRole and the rendered manifest gain a batch/jobs list rule (kept identical,
  as check-rbac-manifest.sh requires).
- README kinds table and verify loop include jobs; note that a denied Jobs list
  is skipped rather than failing the scan.
- values.yaml extraRules example uses cronjobs, since jobs is now granted.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
- Manifest header, chart README and root README said 26 kinds; batch/jobs makes it 27.
  CI's manifest check ignores comments, so it did not catch the header.
- ci/customised.yaml extraRules adds only cronjobs; jobs is now a default grant.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tim-thacker-nullify added a commit that referenced this pull request Sep 15, 2026
… site (#60)

* fix(helm): publish every chart version instead of replacing the Pages site

The release workflow packaged one hard-coded chart into an empty directory and
deployed it as the whole Pages site, so each release unpublished every earlier
version. scripts/publish-helm-repo.sh now re-downloads every version in the
live index (digest-checked), packages each chart under helm-charts/, refuses to
republish an existing version with different contents, and merges the index.

Charts are linted and rendered per chart on pull requests, the release job
needs that validation, and tagging moves into the release job as
<chart>-v<version> after the deploy. auto-tag.yml, the stale
aws-integration-setup/docs/index.yaml and update-helm-repo.sh are removed.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* ci(helm): dry-run the repository build on pull requests

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): check the index against tags and keep charts independent when publishing

- Refuse to build when a <chart>-v<version> tag (or legacy collector tag
  v0.2.0) has no version in the fetched index; recover those versions from
  their GitHub release assets only with HELM_REPO_RECOVER_FROM_RELEASES=true,
  digest-checked against the asset's recorded sha256.
- Fetch index.yaml with a cache-busting query and Cache-Control: no-cache.
- Write release.txt with every chart version at HEAD the site serves, so
  tagging no longer depends on the version being new to the live index.
- Skip a chart that fails its tests or changes an already-published version,
  with an ::error:: annotation and skipped.txt, instead of failing every
  chart; PUBLISH_STRICT=true keeps the hard failure for per-chart validation.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* ci(helm): release only from main, retry tagging, pin actions and gate per chart

- release runs only on refs/heads/main in the github-pages environment;
  dispatches from other refs run the dry run instead.
- Workflow-level concurrency no longer groups non-PR runs; release uses its
  own pages group with cancel-in-progress false.
- release runs when a single chart's validate leg fails; the publish script
  re-tests each chart and skips only the failing one, and the job still
  fails at the end listing skipped charts.
- validate checks that a chart's published version is unchanged or bumped.
- Tagging creates any missing tag, release or release asset for every
  published chart version at HEAD, so a re-run after a failed tag step
  finishes the job.
- workflow_dispatch input recover_from_releases.
- Pin every action by commit SHA; pass matrix.chart through env.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): verify recovered chart assets, tag before deploying and gate on pre-publish hooks
- Recovery re-packages CHARTS_DIR/<chart> from the tag's tree and requires the
  release asset to have the same contents, and the tag's commit to be on main.
  Legacy v0.2.0 is pinned to sha256 19feed16...d8a4, checked on recovery and
  on every kept index entry.
- Only <chart>-v<version> tags of charts in helm-charts/ or the index are
  cross-checked, so an unrelated tag cannot block every run.
- The release job creates tags and releases before deploy-pages, so every
  served version is tagged and a stale CDN index fails the cross-check.
- Drop the index cache-busting query and headers; Pages ignores both.
- Run <chart>/ci/pre-publish.sh, when present, before packaging; a failing
  hook skips that chart whatever the workflow's needs.
- Skip a chart version whose release already has a tgz asset with a different
  digest (PUBLISH_CHECK_RELEASE_ASSETS); the tag step re-checks before deploy.
- release.txt carries each version's tag target: HEAD for a new version, else
  the last commit that changed the chart directory or manifests/.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): restore tagged-but-unpublished chart versions automatically

- Every workflow job sets HELM_REPO_RECOVER_FROM_RELEASES=true, so a version
  tagged before a failed deploy is restored by the next release on main, and
  validate reports it as a ::warning:: instead of failing. Recovery still
  requires the tag on main, the asset to match the chart re-packaged from the
  tag, and legacy 0.2.0's pinned digest; a version failing any check stops the
  build. The recover_from_releases dispatch input is removed; the github-pages
  environment approval is unchanged.
- A new version whose release already has a tgz asset is compared by contents
  instead of by the digest of a fresh helm package, which never matches, and the
  site then serves the asset's bytes. An asset that cannot be downloaded or does
  not match its recorded digest still skips the chart.
- Recovery finds the chart directory at the tag by Chart.yaml name instead of
  assuming the directory is named after the chart.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): warn instead of failing on a tag with no release asset

A <chart>-v<version> tag whose release or asset does not exist made
recover_from_release die, so every PR's validate leg, publish-dry-run and every
release run failed - including before the step that would have created the
release. Creating the nullify-k8s-readonly-access-v0.1.0 tag would have wedged
all Helm CI.

- a missing release, or a release without that asset, is a ::warning:: and the
  version is dropped from the tagged cross-check; nothing was ever published
  for it, and the release job publishes it from main if HEAD carries it
- an asset whose release records no digest is verified against the tag's tree
  instead of dying
- a digest mismatch, a failed download and a failed content check still fail

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): verify hand-made tags before deploy and use a REST-only release lookup

A <chart>-v<version> tag created without a release skipped the ancestry and
chart-tree checks entirely (recover_from_release returned before
verify_against_tag_tree ran), and the release job's --verify-tag path accepted
any pre-existing tag regardless of what commit it named. Together these bound
main's package to whatever commit a hand-made tag pointed at, with CI green.
gh 2.100.0 also reports every failure of the published-release lookup
(5xx, rate limit, reset) as "release not found", so a transient error took the
same warn-and-drop path as a real absence and could drop a live version.

- recover_from_release now runs the same on-main/chart/version checks in the
  no-release and no-asset branches, factored into verify_tag_chart_version;
  the result only changes the warning's wording, so a tag with no release
  still never wedges validate.
- the release lookup is now a REST-only gh api call; only a genuine HTTP 404
  means "no release", anything else fails the build loudly.
- the "Ensure a tag and release" step refuses to attach a release to a
  pre-existing tag whose tree (chart dir + manifests/) differs from the
  commit it intends to publish, instead of trusting --verify-tag's mere
  tag-exists check.

Procedurally: never hand-create <chart>-v* tags; the release job creates them
at HEAD. nullify-k8s-readonly-access-v0.1.0 is created by #61's first release.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): verify hand-made tags before every release attach, not just create

The tag step only checked a pre-existing tag's tree (and never its ancestry
on main) in the branch that creates a fresh release; a release hand-made in
the GitHub UI with a missing asset skipped verification entirely and got
main's package uploaded to it. git ls-remote --exit-code also treated any
non-zero exit as "no tag", so an unreachable remote fell through to
gh release create --target, which GitHub ignores once a tag exists.

- Determine tag existence once via git ls-remote, failing loudly on any exit
  code other than 0 (tag exists) or 2 (no tag).
- Whenever the tag exists, require both its commit is an ancestor of main and
  its tree matches $target before any upload or create, whether or not a
  release already exists.
- Scope the tree comparison to helm-charts/<name> plus only the manifest
  files that chart's own README references, instead of the whole manifests/
  directory, so one chart's manifest change can no longer fail another
  chart's tag check.
- publish-helm-repo.sh: have verify_tag_chart_version return a distinct exit
  status for "not on main" so the three callers stop collapsing it into "or
  its tree is not <name> <version>".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

* fix(helm): gate hand-made-tag verification to attach paths only

The ancestry and tree check ran on every has_tag=true release loop
iteration, before checking whether the release already has the asset.
An unbumped edit that doesn't change the packaged chart (a helmignored
ci/ file, a Chart.yaml comment, a manifest comment) moves the "last
change" commit forward while leaving the tag's already-uploaded asset
untouched, so the check failed and blocked every chart's Pages deploy,
telling the operator to delete a customer-pinned tag.

- Move the ancestry/tree check into verify_hand_made_tag and call it
  only immediately before gh release upload and gh release create
  --verify-tag, never on the already-has-the-asset path.
- Reword the git merge-base and tree-diff errors to stop suggesting
  tag deletion, and distinguish a bad-ref merge-base exit from "not an
  ancestor".
- Join the extracted manifest list with spaces instead of newlines so
  ::error:: annotations aren't truncated at the first manifest.
- publish-helm-repo.sh: give verify_tag_chart_version a distinct exit
  code for an unshallow/fetch failure so a network error inside the
  $(...) call no longer surfaces as "has no Chart.yaml named ... at
  version ...".

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
tim-thacker-nullify and others added 3 commits September 15, 2026 20:53
release job needs rbac-manifest (per #61) and tolerates a single
chart's failing validate leg (per #60/fix/helm-publish-every-version),
plus requires rbac-manifest to succeed outright since it isn't
per-chart.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
@tim-thacker-nullify

Copy link
Copy Markdown
Member Author

@dnjg The conflict is gone. This branch is MERGEABLE / CLEAN against fix/helm-publish-every-version (resolved in e6e3fee).

tim-thacker-nullify and others added 2 commits September 17, 2026 17:20
list returns Secret values, so the chart rejects grantSecretsRead and extraRules that name secrets.

Co-authored-by: Cursor <cursoragent@cursor.com>
…e gate

The managed scan stores name, type, key count and age, and collectSecrets
already exists, so the chart must not claim values leak or that the option
is unshipped. Release stays per-chart via the pre-publish hook.

Co-authored-by: Cursor <cursoragent@cursor.com>
@tim-thacker-nullify
tim-thacker-nullify merged commit 0c22648 into fix/helm-publish-every-version Sep 17, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants