Repository navigation
fix(terraform): reconcile K8s scan-mode RBAC and address #81 review feedback - #98
Merged
tim-thacker-nullify merged 2 commits intoSep 15, 2026
Conversation
…81 Applies dnjg's review feedback from ncc-cloud-connector #81 to #78's collapsed branch. - Align the in-cluster collector's Kubernetes RBAC (Helm chart and the Terraform k8s-resources module, v5 and v6) with the managed scan's ClusterRole and with collect.go, so switching scan mode no longer silently changes which kinds get scanned (N78-2, N78-3). - Document why both scan modes list Secrets without reading values: the managed scan reads Kubernetes' server-side Table representation, the in-cluster collector redacts before upload (N78-2). - Fix the documented terraform import address for the EKS access entry, covering the root module's count index, a direct module call and the multi-cluster example (N78-4). - Cite the platform monorepo's iac/network/vpc module as the source of the hardcoded Nullify egress IPs, in both the module comment and the README (N78-5). Serving the list from an API is left as a product follow-up, not built here. - Reword the ARN partition validation errors to say only the commercial aws partition is supported, instead of reading like a malformed-ARN typo (N78-6). - Add terraform test assertions on the KMS, S3 access point and readonly aws_iam_policy_document statement blocks: the existing mock_provider fakes every document's computed json, so no test previously asserted an actual IAM action or resource (N78-8). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
whiskeylotus's review on ncc #58 (W58-3) asks for a job that verifies the CloudFormation template and both Terraform trees keep granting the same Allow/Deny action sets for the customer readonly role -- nothing currently checks that the three independently maintained templates stay identical. Adds scripts/check_iam_policy_drift.py, comparing the CloudFormation template's ReadOnlyAccessPolicy(+2)/DenyActionsPolicy statements against readonly_policy_part1/part2/deny_actions_policy in both aws-integration-setup/terraform and terraform-v6, and wires it as a new job beside the existing terraform-validate matrix. Verified locally: passes against the current templates (437 Allow, 45 Deny actions matching on both trees) and correctly fails when an action is injected into one side only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
tim-thacker-nullify
marked this pull request as ready for review
September 15, 2026 06:09
tim-thacker-nullify
merged commit Sep 15, 2026
1d5cd17
into
fix/terraform-kms-image-region
30 checks passed
tim-thacker-nullify
added a commit
that referenced
this pull request
Sep 15, 2026
- Revert the Helm collector clusterrole.yaml to #78's version: #98 mutated the already-published chart 0.2.0's RBAC without a version bump, which would republish 0.2.0 with different RBAC and a new digest on merge. - Revert #98's collector-role edits in both k8s-resources/main.tf trees (v5, v6) to #78's version, removing the three-file merge conflict with #58/#99/#63. Collector RBAC belongs to #58/#99 (chart 0.3.0). - Add batch/jobs (list) to managed-scan RBAC in both trees, matching Helm and the in-cluster collector role, so CronJob pod ownership attribution doesn't silently lose data on managed-scan clusters. Update the pinned k8s_resources.tftest.hcl assertions for the new rule. - Correct README:113's Secrets claim (no server-side Table fetch exists; values are redacted after collection, not before) and README:89's false "same kind list" claim for the published 0.2.0 chart. - Rewrite check_iam_policy_drift.py to compare whole statements (effect, action, resources, condition) instead of a bare union of action names, and fix the actions= regex matching inside not_actions=. - Add scripts/** to terraform-validate.yml's path filters so editing the drift script alone triggers the drift job. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Sep 15, 2026
tim-thacker-nullify
added a commit
that referenced
this pull request
Sep 17, 2026
… look up clusters in their own region (#78) * feat: grant wafv2:Get*, ec2:GetEbsEncryptionByDefault and the missing Kubernetes list kinds The AWS collectors and the managed Kubernetes scanner read resources the connector never granted, so those reads fail closed or record a permission gap. - Add `wafv2:Get*` and `ec2:GetEbsEncryptionByDefault` to all three AWS templates (CloudFormation, terraform, terraform-v6), keeping the Allow sets byte-identical. - Reconcile the ClusterRole in both k8s-resources terraform modules and the Helm chart to the exact kinds the collector Lists: core configmaps, secrets, resourcequotas, limitranges; discovery.k8s.io endpointslices; all four admissionregistration.k8s.io kinds; storage.k8s.io csidrivers; node.k8s.io runtimeclasses; scheduling.k8s.io priorityclasses; flowcontrol.apiserver.k8s.io flowschemas; networking.k8s.io ingressclasses; certificates.k8s.io certificatesigningrequests. - Drop core `endpoints`, which every manifest granted and the collector never lists (it reads discovery.k8s.io/endpointslices instead). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> * fix(aws): grant the 45 read actions the CSPM collectors call  The connector role never granted 45 of the AWS read actions the collectors issue. Each call returns AccessDenied, the collector tolerates it and leaves the field nil, and the CSPM rule that reads the field fires on every resource: kms:GetKeyRotationStatus on every customer-managed key, ecr:GetLifecyclePolicy on every repository, sns:GetTopicAttributes on every topic. sqs, codebuild, config and opensearch drop their whole inventory instead. - part1: athena:GetWorkGroup, backup:Get*, codebuild:BatchGet*, codebuild:List*, config:Describe*, dynamodb:GetResourcePolicy, ec2:GetManagedPrefixListEntries, ec2:GetSnapshotBlockPublicAccessState, ecr:GetLifecyclePolicy, ecr:GetRepositoryPolicy, es:Describe*, es:List*, glue:Get*, iam:GenerateCredentialReport, kms:GetKeyPolicy, kms:GetKeyRotationStatus - part2: s3:GetAccountPublicAccessBlock, s3:GetBucket*, s3:GetEncryptionConfiguration, s3:GetLifecycleConfiguration, s3:GetReplicationConfiguration, sns:GetTopicAttributes, sqs:GetQueueAttributes, ssm:Describe*, ssm:GetDocument - s3:GetBucketLocation dropped as redundant under the new s3:GetBucket* - terraform-v6/data.tf gains the trailing newline it was missing - applied identically to the CloudFormation template and both Terraform modules; part1 is at 5921/6144 chars and part2 at 5593/6144, so neither needs a Part3 The 45 were derived from every *<pkg>.<Operation>Input literal in context/internal/cloudintegrations/awsintegration/processors, mapped through the IAM prefix and action renames (opensearch -> es, s3 GetBucketEncryption -> s3:GetEncryptionConfiguration, API Gateway -> apigateway:GET). A Go test in that package now re-derives the list on every run and asserts this template grants it, which is the third source check_cloud_connector_iam_drift.py cannot supply: it compares the three templates to each other, so a grant missing from all three was green. * fix(aws): scope the connector role to configuration reads only Four wildcard grants reached customer data, not just cloud configuration: wafv2:Get* covers GetSampledRequests (live HTTP requests with Cookie and Authorization headers), glue:Get* covers GetConnection (JDBC PASSWORD) and GetTable*/GetPartitions, codebuild:BatchGet* and backup:Get* both expand well past the single call each collector makes. - Replace those four with the nine actions the collectors actually call. - Deny 55 data-plane actions the Allow wildcards still reached: raw object and message content (s3-outposts:GetObject, logs:StartLiveTail, states:DescribeExecution, cognito-sync:QueryRecords, aps:QueryMetrics, rum:GetAppMonitorData, ssm:ListCommandInvocations, support:DescribeCases), cleartext credentials (lightsail:GetRelationalDatabaseMasterUserPassword, storagegateway:DescribeChapCredentials), end-user PII (cognito-idp:ListUsers and ListUsersInGroup), and the credential oracles transfer:TestIdentityProvider and verifiedpermissions:IsAuthorizedWithToken. - Delete the 12 Allow entries the new Deny statement shadowed, rather than leaving the same action in both lists. - Delete 7 Allow entries already subsumed by a sibling wildcard. - Bump the Helm chart to 0.3.0 so tenants can actually pull the new ClusterRole; helm-release.yml keys the published version off Chart.yaml, so the RBAC added in the previous commit was unreachable at 0.2.0. Verified against the collectors on monorepo ea4b7c0: 240/241 AWS calls remain covered (the one exception, sts:AssumeRole, is test-only and is the role's trust policy, not a grant). No collector calls any denied action. Real IAM-counted headroom on readonly-part1 goes 223 -> 326 chars; the drift gate under-reports by 15 chars per statement because document_size() omits "Condition". Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126R5BKvayttBu8nKmFZBVR * fix(aws): deny the data reads a second audit found still reachable An adversarial pass over the remaining Allow wildcards found reach the first narrowing missed. None of it is called by any collector. - Deny 22 actions still reachable through a wildcard: Lake Formation row data (GetWorkUnitResults, GetWorkUnits), live stream samples (kinesisanalytics:DiscoverInputSchema), Lightsail container application logs, support case attachment bytes, a plaintext OAuth client secret (cognito-idp:DescribeUserPoolClient), usable AppSync API keys (appsync:ListApiKeys), notification event bodies, and the WorkMail / IdentityStore / AppStream / Rekognition / SSO / WorkSpaces user directories. - Drop 11 Allow entries instead of shadowing them: amplify:GetWebhook (the URL embeds a bearer token and POSTing it starts a build, a write the Deny statement cannot reach), account:GetContactInformation / GetAlternateContact / GetPrimaryEmail and one:ListUsers (named-person PII), cloudtrail:LookupEvents (per-event username and source IP), and the four write actions hiding behind read-sounding names -- cloudformation:Detect* (AWS classes it Write; it schedules drift detection and mutates stack state), cloudwatch:GenerateQuery, logs:StopLiveTail, logs:StopQuery. - Drop glue:Describe*, which matches no real Glue action, and one of the two copies of notifications:List*. Verified with a from-scratch IAM evaluator: 232/232 collector actions still allowed, every action above resolves to explicitDeny or implicitDeny, and no action appears in both Allow and Deny. readonly-part1 headroom 223 -> 545. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126R5BKvayttBu8nKmFZBVR * fix(k8s): drop the unused get verb and harden the terraform collector Under an assume-breach model the collector's RBAC and pod spec are what an attacker inherits, so both are cut to what the code actually needs. - Drop `get` from all 38 grants. `collect.go` has no `.Get(` call on any kind, including CRDs; all five list sites go through `.List`, so `get` was dead weight in the ClusterRole a compromised image would hold. - Grant `nonResourceURLs: ["/version"]`. `Discovery().ServerVersion()` is called and was never granted; it works today only because EKS, GKE and AKS bind `system:public-info-viewer` to `system:authenticated`, and under `Strict: true` a cluster without that default fails collection outright. - Give the terraform CronJob the security context the Helm chart already sets (no privilege escalation, read-only root filesystem, run as non-root, drop ALL capabilities). Terraform-deployed collectors ran with none of it. - Add the trailing newline `terraform-v6/modules/k8s-resources/main.tf` was missing, the same defect this branch already fixed in its `data.tf`. All three manifests stay byte-identical in what they grant: 38 resources at `list`, plus `/version` at `get`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126R5BKvayttBu8nKmFZBVR * fix(aws): cut the role to the actions the collectors call The readonly policy was AWS's SecurityAudit shape minus a deny list, so it granted ~300 services the collectors never touch. If Nullify is compromised the attacker inherits this role in every onboarded account, and that unused surface is pure attacker capability with no scanning value. Rebuild the two readonly policies as an explicit allow list: 433 account-wide Allow entries -> 107. - Drop 134 services outright, none of which any collector calls: sdb, lakeformation, lightsail, support, workmail, rekognition, sso, sso-directory, identitystore, route53domains, storagegateway, servicecatalog, waf, machinelearning, the mediapackage family and the rest. - Drop cloudformation, elasticbeanstalk, amplify and serverlessrepo, which have no processor at all and leak DescribeStacks parameters, Beanstalk plaintext env config, Amplify basicAuthCredentials and presigned source archives. - Drop `iam:Simulate*`, whose only use to an attacker holding the IAM graph is testing escalation paths offline without generating AccessDenied in CloudTrail. - Narrow 9 families whose wildcard mixes configuration reads with a data or secret sibling: `lambda:Get*` (GetLayerVersion returns a presigned code URL), `s3:List*` (ListBucket enumerates object keys), `guardduty:Get*` (GetFindings), `appsync:List*` (ListApiKeys), `cognito-idp:Describe*`/`List*`, `cloudtrail:Get*`, `ssm:List*`, `states:Describe*`. - Keep `ec2:Describe*` and `iam:Get*`/`List*` as wildcards deliberately. Their families are inventory metadata, enumerating them is brittle, and `iam:Get*`/`List*` also cover the OIDC-provider reads in admin/internal/cloud/cloudpolicies that a processors-only derivation misses. Verified against 234 required actions drawn from the collectors, the admin preflight and cloudpolicies: all 234 still resolve to allowed, zero regressions. Every dropped item was confirmed to have no caller anywhere in context, admin, cspm or scpm. readonly-part1 falls from 5599 to 1355 of 6144 chars. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0126R5BKvayttBu8nKmFZBVR * fix(aws): enumerate ec2 and iam instead of wildcarding them `ec2:Describe*` and `iam:Get*`/`List*` were kept as wildcards on the argument that both families are inventory metadata. That is wrong for ec2 and too generous for iam. - `ec2:Describe*` covers 192 actions; the collectors call 24. The unused remainder includes `DescribeVpnConnections`, which returns `CustomerGatewayConfiguration` with the IPsec pre-shared key in plaintext -- a live credential into the customer's on-prem network -- plus `DescribeInstanceAttribute` (UserData), `DescribeClientVpnConnections` (client identities and source IPs) and `DescribeTags` (owner emails). - `iam:Get*`/`List*` cover 67; the collectors and admin's OIDC resolver call 25. The remainder includes `GetAccountAuthorizationDetails`, which returns the entire IAM graph in one call, `GetServiceLastAccessedDetails`, which tells an attacker which grants are stale enough to abuse unnoticed, and `GetSAMLProvider`/`GetLoginProfile` for phishing the workforce. Allow 107 -> 152: enumeration is longer than the wildcards it replaces but grants far less. - Deny 15 further actions as defence in depth against a future re-broadening, matching the discipline already applied to Cognito and WorkMail. - Deny `apigateway:GET` on `/apikeys*` and `/usageplans*` via a second, resource-scoped statement. API Gateway exposes one HTTP-verb-shaped action covering its whole control plane, including `GET /apikeys?includeValues=true`; path ARNs are the only way to separate the five calls the collectors make from plaintext API key values. 234/234 required actions still resolve to allowed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): correct two invalid action names and a neptune coverage gap IAM accepts a misspelled action in a policy and simply never matches it, so both of these were silent. - `medialive:GetInputDeviceThumbnail` is not an IAM action; the real one is `DescribeInputDeviceThumbnail`. Introduced in 3a1a4fa. Inert today -- nothing grants `medialive:*` any more -- but a dead Deny that a future broad grant would sail straight past. - `neptune:` is not a valid IAM prefix. Neptune's control plane is served by the RDS API and authorised with `rds:` actions, so `neptune:Describe*` and `neptune:List*` granted nothing. Drop both. - That mattered: enumerating the policy mapped `neptune.ListTagsForResource` to the dead `neptune:List*` rather than `rds:ListTagsForResource`, and the earlier `rds:List*` wildcard was gone, so the collector's Neptune tag read would have failed. Grant `rds:ListTagsForResource`. The three Describe calls were already covered by `rds:Describe*`. Every action string in all three templates was checked against AWS's policy generator dataset: 296/297 valid before this commit, and the one miss is fixed here. 231/231 required actions resolve to allowed with Neptune mapped to `rds:`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): grant KMS key/* for alias ARNs, pin the collector image and look up clusters in their own region - KMS policy grants kms_key_arn and key/* in the same Nullify account and region; validation accepts key (incl. mrk-) and alias ARNs, like the CloudFormation template. - k8s-resources collector_image defaults to public.ecr.aws/w4o2j2x4/integrations:k8s-collector-3.45.0 instead of the unowned Docker Hub nullify/k8s-collector:latest. - New nullify_s3_access_point_arn grants s3:PutObject/PutObjectAcl on <ap>/object/k8s-collector/*. - v6 reads each EKS cluster in the region from its ARN; v5 fails fast with a precondition and accepts eks_oidc_issuer_urls for other regions. - multi-cluster example: region-correct lookups and exactly two clusters. - Root drops the unused kubernetes provider and the dead cronjob_schedule pass-through; cross-variable validations become preconditions so the declared Terraform >= 1.5 floor holds. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(aws): fix Terraform README drift and document KMS, access point and region handling - k8s-resources file is providers.tf; basic quick start copies its own tfvars; collector_image is a k8s-resources variable, not a root one. - Module sources point at Nullify-Platform/nullify-cloud-connector. - v6 README no longer claims data.aws_eks_cluster_auth was removed. - Requirements (Terraform >= 1.5) and upgrade notes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: validate terraform-v6 and run each AWS row at its declared Terraform floor Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(aws): add Terraform modules for managed EKS scan access - modules/eks-managed-scan-access (v5 and v6): one STANDARD access entry per cluster, rbac by default (group nullify-readonly) or AmazonEKSAdminViewPolicy at cluster scope as a warned opt-in. Preconditions reject CONFIG_MAP clusters and cross-account clusters (v5: clusters outside the provider region). Entries are replaced when the role's unique_id changes. A check block warns when Nullify's regional egress IPs are missing from publicAccessCidrs; the module never manages them and outputs a merge command instead. - k8s-resources: enable_collector (default true, moved blocks keep state) and enable_managed_scan_rbac, a list-only nullify-readonly ClusterRole on the 26 scanned kinds plus get /version, bound to the group. - nullify-aws-integration: role_unique_id output. - terraform test suites with mocked providers for all three modules. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * feat(aws): wire managed EKS scan into the root and examples - Root: enable_managed_scan, managed_scan_cluster_arns, nullify_region, managed_scan_authorization (default rbac), managed_scan_kubernetes_group, and managed-scan outputs. v5 root requires AWS provider ~> 5.33. - New examples/managed-scan: one cluster, access entry plus the list-only ClusterRole, manage_rbac = false for GitOps-applied RBAC. - multi-cluster-complete: scan_mode collector (default) | managed | both. v5 uses one access-entry module per regional provider, v6 one instance. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(aws): document the managed EKS scan modules, RBAC, endpoint allowlist and upgrade notes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: validate the managed-scan module and examples and run terraform test with mocked providers Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): convert publicAccessCidrs to a list before building the update command terraform test caught concat() rejecting the set returned by data.aws_eks_cluster, which would fail every plan where a Nullify egress IP is missing. The test mocks aws_iam_policy_document with a valid policy so provider validation accepts the mocked JSON. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci: run the terraform test suites at the lowest version that can run them The modules declare required_version >= 1.5, but the validate rows only fmt and validate there, which never evaluate preconditions or check blocks, and the test job pinned 1.10.5 for every row. - terraform-test gains a version dimension: 1.8.5 and 1.10.5 - 1.8.5 is the floor for the suite itself: mock_provider is 1.7+, and the tests use strcontains, which is 1.8+ Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): name each collector's cluster and narrow the KMS grant Round-1 review of the managed EKS scan stack. - k8s-resources sets CLUSTER_NAME on the CronJob from a new cluster_name variable, so collectors stop sharing the k8s-collector/default-name-data.json upload key; a precondition requires it while enable_collector is true, and the multi-cluster examples pass each cluster's own name - eks-managed-scan-access clears kubernetes_groups with [] rather than null: the attribute is Optional+Computed, so null kept the stale group forever - nullify-aws-integration derives key/* only for an alias ARN; a key ARN is granted alone, as the CloudFormation template already does - collector_image pins k8s-collector-3.46.0, the build the Helm chart's k8s-collector-latest tag resolves to - README documents that the clusters must exist before the first plan, with the -target two-step, and the cluster_arns message says the same - docs describe the RBAC requirement instead of pointing at a chart, a manifest and a tag that exist on no merged branch - the RBAC test pins the exact kind set per API group, not just the counts Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(terraform): note the cluster_name requirement in the upgrade notes Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): require KMS for the collector and narrow the key/* grant to cross-account Both published collector images refuse to upload without KMS encryption, so a Terraform deployment that left kms_key_arn empty collected the cluster and then failed every upload. cluster_name had two undocumented requirements that fail silently server-side. scan_mode = "both" registered one cluster twice. - k8s-resources: kms_key_arn required while enable_collector is true, with allow_unencrypted_upload as the only opt-out and the only setter of ALLOW_UNENCRYPTED_UPLOAD - cluster_name docs and precondition state the exact-name and registered-cluster requirements; README example uses a real-looking name - nullify-aws-integration: derive key/* for alias and key ARNs alike, only when the ARN's account is not the caller's - examples/multi-cluster-complete: reject scan_mode = "both" - use data.aws_partition for constructed ARNs, and the cluster ARN's own partition for the EKS access policy ARN - declare Terraform >= 1.8 and validate at 1.8.5, the version the test suites run - correct the upgrade note that told customers to delete an object their role is denied s3:DeleteObject* on Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): drop the unencrypted-upload opt-out and reject collector + managed scan together allow_unencrypted_upload could not work against Nullify's bucket: an empty kms_key_arn also sets enable_kms_access = false, so the role gets no KMS statement, and the bucket's SSE-KMS default encryption still requires kms:GenerateDataKey on Nullify's key. The variable traded the collector's self-explaining error for an opaque S3 AccessDenied, so it is removed and kms_key_arn is unconditionally required when enable_collector is true. - k8s-resources: remove allow_unencrypted_upload and the ALLOW_UNENCRYPTED_UPLOAD env block; the KMS precondition is now kms_key_arn != "" alone - k8s-resources: precondition rejecting enable_collector together with enable_managed_scan_rbac, matching the example's scan_mode = "both" rejection - nullify-aws-integration: kms_key_arn description now states the cross-account rule the locals actually implement - terraform/README.md: fold #88's bare-key-form note into the rewritten KMS section so merging both PRs cannot leave two near-duplicate paragraphs - multi-cluster-complete: comment out the placeholder KMS ARN and shape it so the variable regex rejects it if pasted as-is - CI: validate rows for the four remaining modules/ directories Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): move the collector/managed-scan duplicate guard to the access entry and drop phantom upgrade notes The precondition guarding against duplicate cluster registration lived on k8s-resources' ClusterRole, so authorization = admin_view_policy (which creates no Kubernetes objects) never triggered it: a direct caller running the collector and a managed scan on one cluster in that mode still plans clean. The same guard also rejected a legitimate configuration -- staging the ClusterRole ahead of a cutover with no access entry behind it yet. - Move the guard to eks-managed-scan-access's access entry (created in every authorization mode) via a new collector_cluster_arns input, keyed on cluster name; remove the now-redundant precondition from k8s-resources' ClusterRole so staging is allowed again. - Wire collector_cluster_arns through the root configs and the multi-cluster-complete examples in both terraform/ and terraform-v6/. - Add terraform test rows for both directions: rejected under admin_view_policy, and unaffected when the collector cluster differs. - Drop three Upgrade Notes bullets that describe variables (allow_unencrypted_upload, enable_managed_scan_rbac, scan_mode) absent from origin/main and both release tags -- there is no upgrade path they describe. - Add the missing Upgrade Notes section to terraform-v6/README.md with the real (non-phantom) bullets, mirrored from v5. - Fix the KMS placeholder ARN in the three remaining tfvars.example files to match the corrected multi-cluster-complete one (R3-8): the key is in Nullify's account, not the customer's. - Remove the four terraform-validate rows for module directories already covered end-to-end by the terraform-test job, and correct the comment justifying them (both matrices are hand-maintained, not auto-discovered). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): compare full cluster ARNs in the collector duplicate guard and root-derive it correctly The guard added in the previous round compared bare cluster names, so two clusters sharing a name in different regions/accounts collided with no override in the v6 root. It also derived collector_cluster_arns from the IRSA trust list, which is a prerequisite for a collector, not evidence one is deployed. - Compare full cluster ARNs instead of names in eks-managed-scan-access (both trees); add a same-name-different-region test row in both. - Add a root collector_cluster_arns override variable (both trees) so root users can correct the IRSA-trust-list proxy and decouple a collector-to-managed-scan cutover from revoking IRSA trust. - Reword the guard's error message to cover the Helm/manifest paths its own variable description already names. - Add ARN-shape validation to collector_cluster_arns, matching cluster_arns. - Remove the dead collector_cluster_arns wiring in both multi-cluster examples: scan_mode's mutual exclusivity makes it always []. - Fix terraform-v6/README.md still documenting the enable_collector + enable_managed_scan_rbac rejection this PR's base commit removed; add the missing collector_cluster_arns doc and Kubernetes-provider-removal upgrade note to match the v5 tree. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(terraform): tell collector-cutover instructions to stop the collector first collector_cluster_arns docs told users to override the guard during a collector-to-managed-scan cutover but never said to disable the collector first, so following them literally leaves the collector uploading while the cluster also gets managed-scan access -- the duplicate registration the guard exists to prevent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): drop collector ClusterRole grants the collector never lists whiskeylotus's review on #58 (W58-2) flagged that the collector ClusterRole grants 12 kinds main's collect.go never lists: jobs is kept since the monorepo's fix/k8s-deployment-matching-review stack (#12807, which #12830 sits on) starts collecting Jobs owned by a CronJob; cronjobs, horizontalpodautoscalers, poddisruptionbudgets, ingressclasses, storageclasses, csidrivers, runtimeclasses, priorityclasses, flowschemas, certificatesigningrequests and customresourcedefinitions are dropped -- no open collector change reads any of them. Verified against context/internal/cloudintegrations/k8sintegration/ collect/collect.go on both the monorepo's main and fix/k8s-deployment-matching-review branches. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): drop collector ClusterRole grants the collector never lists (#99) whiskeylotus's review on #58 (W58-2) flagged that the collector ClusterRole grants 12 kinds main's collect.go never lists: jobs is kept since the monorepo's fix/k8s-deployment-matching-review stack (#12807, which #12830 sits on) starts collecting Jobs owned by a CronJob; cronjobs, horizontalpodautoscalers, poddisruptionbudgets, ingressclasses, storageclasses, csidrivers, runtimeclasses, priorityclasses, flowschemas, certificatesigningrequests and customresourcedefinitions are dropped -- no open collector change reads any of them. Verified against context/internal/cloudintegrations/k8sintegration/ collect/collect.go on both the monorepo's main and fix/k8s-deployment-matching-review branches. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): reconcile K8s scan-mode RBAC and address #81 review feedback (#98) * fix(terraform): reconcile K8s scan-mode RBAC and fix review nits from #81 Applies dnjg's review feedback from ncc-cloud-connector #81 to #78's collapsed branch. - Align the in-cluster collector's Kubernetes RBAC (Helm chart and the Terraform k8s-resources module, v5 and v6) with the managed scan's ClusterRole and with collect.go, so switching scan mode no longer silently changes which kinds get scanned (N78-2, N78-3). - Document why both scan modes list Secrets without reading values: the managed scan reads Kubernetes' server-side Table representation, the in-cluster collector redacts before upload (N78-2). - Fix the documented terraform import address for the EKS access entry, covering the root module's count index, a direct module call and the multi-cluster example (N78-4). - Cite the platform monorepo's iac/network/vpc module as the source of the hardcoded Nullify egress IPs, in both the module comment and the README (N78-5). Serving the list from an API is left as a product follow-up, not built here. - Reword the ARN partition validation errors to say only the commercial aws partition is supported, instead of reading like a malformed-ARN typo (N78-6). - Add terraform test assertions on the KMS, S3 access point and readonly aws_iam_policy_document statement blocks: the existing mock_provider fakes every document's computed json, so no test previously asserted an actual IAM action or resource (N78-8). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * ci(terraform): add an IAM policy drift check across CFN and Terraform whiskeylotus's review on ncc #58 (W58-3) asks for a job that verifies the CloudFormation template and both Terraform trees keep granting the same Allow/Deny action sets for the customer readonly role -- nothing currently checks that the three independently maintained templates stay identical. Adds scripts/check_iam_policy_drift.py, comparing the CloudFormation template's ReadOnlyAccessPolicy(+2)/DenyActionsPolicy statements against readonly_policy_part1/part2/deny_actions_policy in both aws-integration-setup/terraform and terraform-v6, and wires it as a new job beside the existing terraform-validate matrix. Verified locally: passes against the current templates (437 Allow, 45 Deny actions matching on both trees) and correctly fails when an action is injected into one side only. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): collector chart namespace, service account and default-value fixes (#63) * fix(helm): collector chart namespace, service account and default-value fixes - Remove the pre-install namespace hook and the bitnami/kubectl Job; the namespace now comes from --create-namespace, and a namespace helper falls back to the release namespace when serviceAccount.namespace is empty (default stays nullify for the IRSA trust). - Use the service-account-name helper in the CronJob and ServiceAccount. - Default clusterName, s3.bucket and kms.keyArn to "" and fail on empty or placeholder values, on a KMS alias instead of a key ARN, on a region that differs from the key's, and on a missing EKS role-arn or GKE role ARN. - Remove collector.dataCollection, which the binary never read. - Fix the labels helper joining custom labels onto the previous line, and apply nodeSelector/tolerations to the collector pod. - kubeVersion >=1.21, chart 0.4.0, and README/NOTES corrections. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * test(helm): expect the role-arn guard for the collector defaults case Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): give the exact fix in the collector's KMS alias and region errors - An alias keyArn still fails, now saying to use the key ARN from the Nullify configure page and to contact Nullify if the page shows an alias. - The region mismatch error explains that --reuse-values from 0.2.0 carries us-east-1, and to upgrade with --reset-then-reuse-values or set the region. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): accept every AWS partition and allow opting out of the collector role-arn guard - The EKS role-arn guard accepts arn:aws, arn:aws-us-gov and arn:aws-cn. - serviceAccount.requireRoleArn (default true) lets EKS Pod Identity, instance-profile and kube2iam/kiam installs render without IRSA; an empty role-arn annotation is no longer rendered. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(helm): document the alias migration, requireRoleArn and --reuse-values upgrades Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): accept KMS alias ARNs in the collector and keep the role ARN commercial - collector.kms.keyArn accepts arn:aws:kms:<region>:<account>:alias/<name> and key ARNs including multi-Region key/mrk-..., matching the connector CloudFormation AllowedPattern. The region default and check take the region from either form. - NOTES warns when an alias is used: IAM matches the key ARN, so the role needs the connector stack version that also grants key/*; the Terraform module grants only the given ARN. - The role-arn guard is back to arn:aws:iam:: only; Nullify's bucket and key are commercial-partition. serviceAccount.requireRoleArn=false stays the escape hatch. ci/eks-govcloud-role.yaml is removed. - CI: ci/eks-kms-alias.yaml and ci/eks-kms-mrk.yaml must render; a bare alias, a GovCloud alias ARN, an alias region mismatch and a GovCloud role ARN must fail. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): keep ci/ out of the collector chart package - .helmignore excludes ci/ from 0.4.0, so a later edit to a CI values file does not change a published version's contents and force a version bump. - Added with the 0.4.0 bump because 0.2.0 is already published; the file changes the package contents. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * docs(helm): state the KMS alias requirement instead of pointing at the current template - NOTES, README and values.yaml say what an alias in collector.kms.keyArn needs: the IAM role's KMS statement must cover the resolved key, for example arn:aws:kms:<region>:<account-id>:key/*. - CloudFormation: connector releases that include PR #64 grant key/* in the account and region of NullifyKMSKeyArn; the template on main grants only the ARN given. Terraform: the module accepts an alias; check the module version's KMS statement covers key/*. A key ARN works with every version. - New README section "KMS alias ARNs" holds the detail; the table, 0.4.0 upgrade notes and troubleshooting link to it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(helm): refuse to unpublish live chart versions and document the merge order main's helm-release.yml rebuilds the Pages site from the collector chart alone, so merging this chain before the publish rework unpublishes collector 0.2.0 and any other version the live index serves. - the release job compares the live index against the single package it would deploy and fails, naming the required merge order, when it would drop one - README gains a maintainer section stating that order and why Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): finish W58-2 in both collector-role trees #99 dropped the 11 never-collected kinds (cronjobs, HPA, PDB, ingressclasses, storageclasses, csidrivers, runtimeclasses, priorityclasses, flowschemas, certificatesigningrequests, CRDs) from the Helm ClusterRole only. The Terraform collector roles in both trees still granted them, so a k8s-resources install of #58 kept read access to kinds no collector reads, and the two collector roles differed by those 11 kinds. jobs is kept. Helm (0.3.0), TF v5 and TF v6 now grant identical kinds and verbs. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(terraform): describe today's Secrets handling and list batch/jobs in managed-scan RBAC - Drop the server-side Table fetch claim and the per-cluster opt-out claim: neither exists in the deployed collector or managed scan. - Kinds-read row: identical from k8s-resources and chart 0.3.0; 0.2.0 grants extra kinds. - Add the batch/jobs row managed_scan.tf already grants. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(terraform): match #58's networking rule formatting and state chart 0.2.0's kinds - k8s-resources main.tf (v5, v6): write the networking.k8s.io resources list the way #58 does, so a squash merge of #58 does not conflict on a formatting-only hunk. - README kinds-read row: identical from chart 0.4.0 (0.3.0 is never published); 0.2.0 grants a different set, not a superset. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(aws): drop AdminView and tighten secret-shaped IAM RBAC is the only EKS authorization mode. Scope GetDocument, deny UserData and EventBridge target Input, pin KMS key/* to alias ARNs, and allow ECR image pull for scanning. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(aws): pin ssm:GetDocument to the Session Manager document partition Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): compare partition-scoped IAM ARNs in the CFN/Terraform drift check Fn: :Sub and the Terraform partition interpolation are the same GetDocument grant; hashing the raw CFN dict crashed the check after that action was scoped. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(aws): deny SSM document bodies and pin the collector image DescribeDocument and DescribeAutomationExecutions return runbook content and automation InputParameters; deny-wins over ssm:Describe*. Helm now defaults to the same k8s-collector-3.46.0 tag as Terraform. * fix(k8s): drop secrets list so Secret values cannot be read Kubernetes list on secrets returns .data; omit the grant from collector and managed-scan ClusterRoles. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Cursor <cursoragent@cursor.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes dnjg's review feedback on #81 (#81 (comment)), stacked on #78 (
fix/terraform-kms-image-region).Re-verified each item against code at #78's head (
fa505f0bb5) and against the platform monorepo'scontext/internal/cloudintegrations/k8sintegration/collect/collect.gobefore changing anything. N78-1 and N78-7 were already addressed on #78 and needed no further work.terraform importaddress missing[0]Per Tim's decision: Kubernetes scans keep
liston Secrets in both modes. The managed scan requests Secrets as a server-side Table (metadata, type, key count -- never values); the in-cluster collector redacts values before upload. The fix reconciles the RBAC kind sets on that basis rather than dropping the Secrets grant.Changes:
helm-charts/nullify-k8s-collector/templates/clusterrole.yaml) and the Terraformk8s-resourcescollector ClusterRole (v5 and v6) now list exactly the kindscollect.golists -- same set as the managed-scan ClusterRole inmanaged_scan.tf. Previously the collector grantedjobs,cronjobs,customresourcedefinitions(Helm) and additionallystorageclasses,horizontalpodautoscalers,poddisruptionbudgets(Terraform) thatcollect.gonever reads, and was missingconfigmaps,secrets,resourcequotas,limitranges,endpointslicesand the four admission kinds that it does.terraform importaddress shapes:module.eks_managed_scan_access[0]....through the root, unindexed for a direct module call,_primary[0]/_secondary[0]in the multi-cluster example.eks-managed-scan-access/main.tf(v5 and v6) and the README citeiac/network/vpc(aws_eip.nat) in the platform monorepo as the source of the egress IPs, and note they only change if Nullify recreates a NAT gateway.eks-managed-scan-accessvariables, v5 and v6) now say only the commercialawspartition is supported, instead of reading like a malformed-ARN typo for GovCloud/China ARNs.aws_iam_policy_document.*.statementblocks (KMS, S3 access point, readonly) for the KMS, S3 and readonly policies: the existingmock_providerfakes every document's computedjsonto{"Statement":[]}, so no prior test asserted an actual IAM action or resource.scripts/check_iam_policy_drift.pyplus aterraform-validate.ymljob comparing the CloudFormation template's readonly/deny statements against both Terraform trees'data.tf. This answers whiskeylotus's W58-3 on feat: grant wafv2:Get*, ec2:GetEbsEncryptionByDefault and the missing Kubernetes list kinds #58: that PR doesn't touch this workflow, so the job lands here beside the matrix it depends on.🤖 Generated with Claude Code