Fix broken CI and IaC, add GitOps delivery, sample app and architecture docs - #3
Open
Mpurushotham wants to merge 14 commits into
Open
Fix broken CI and IaC, add GitOps delivery, sample app and architecture docs#3Mpurushotham wants to merge 14 commits into
Mpurushotham wants to merge 14 commits into
Conversation
The Terraform tree could not be validated, let alone applied. Several
modules had HCL that never parsed, and the ones that did parse described
infrastructure that could not have worked.
Parse failures:
- monitoring: semicolons used as attribute separators in the dashboard
widget list. This is what failed the Checkov CI job, since Checkov could
not parse the directory at all.
- waf: `override_action { none {} }` and `action { block {} }` written as
single-line blocks with a nested block inside. Never caught because the
file lived under security/ where nothing validated it.
Broken references:
- prod referenced ../../modules/guardduty and ../../modules/waf, neither of
which existed; the sources were under security/. Moved both into
terraform/modules/ and parameterized them by environment.
Networking that could not have worked:
- vpc created a VPC and four subnets with no internet gateway, no NAT, and
no route tables, so nothing had a path off the VPC. Added IGW, per-AZ NAT
with an optional single-NAT cost mode, per-AZ private route tables, S3 and
DynamoDB gateway endpoints, flow logs, default SG lockdown, and the
kubernetes.io subnet discovery tags the AWS load balancer controller needs.
- eks put nodes in those unrouted private subnets behind a private-only API
endpoint, so the node group could never have registered. Added the IAM OIDC
provider without which no IRSA role can be assumed, secrets envelope
encryption, control plane and node security groups, a launch template that
requires IMDSv2 with a hop limit of 1, managed addons, and API-mode access
config.
Log delivery that would have failed at apply:
- s3 pointed each bucket's access logging at itself, and had no policy for
any log producer. CloudTrail rejects trail creation outright when the
destination bucket policy does not already grant it access. Added a
dedicated access-log bucket and opt-in policy statements for cloudtrail,
config and alb.
- cloudtrail created a CloudWatch log group but never attached it to the
trail, so every CIS metric filter and alarm built on that group silently
matched nothing.
- kms granted only logs and s3, so CloudTrail, Config, flow logs and EKS
could not use the key.
ECS bugs that would have failed at apply or at runtime:
- HTTPS listener declared with no certificate_arn, which the API rejects.
- ALB could egress to the tasks but the task security group had no matching
ingress, so every health check would have failed.
- task definition referenced a Secrets Manager ARN with a wildcard account.
- access log bucket and awslogs region were hardcoded.
Also pinned the AWS provider in every module so CI validates the same
version that deploys, replaced untyped variables with typed and documented
ones, and set GuardDuty's high-severity filter to NOOP rather than ARCHIVE,
which was suppressing exactly the findings it was meant to surface.
All 14 modules and all 3 environments now pass terraform validate, and
terraform fmt -check -recursive is clean.
Every scheduled Security Scanning run had been failing. Root causes, one per job: - IaC Scan (tfsec): aquasecurity/tfsec-action@v1.0.0 no longer runs. tfsec is end of life and was folded into Trivy. Replaced with a Trivy config scan that uploads SARIF and then gates on CRITICAL/HIGH separately, so a findings report is published even when the gate fails. - IaC Scan (Checkov): could not parse terraform/ because of the monitoring module syntax error fixed in the previous commit. - Kubernetes Manifest Scan: referenced fairwindsops/polaris/.github/actions/polaris@master, which does not resolve, so the job died during "Set up job" without running a step. Replaced with pinned kubeconform and Polaris binaries. - CodeQL (javascript): the matrix asked for a language this repository does not contain, so the analyze step had no database to work with. Other latent breakage: - compliance-report.yml built its report with a quoted heredoc, so every $(...) and $(date) was written to the report literally rather than evaluated. Values are now computed into variables first. - container-build.yml signed steps.build.outputs.digest from a push:false build, where the digest is empty, so image signing could never have produced anything valid. It also built the image twice. Now it builds once, scans, publishes, and signs and attests the digest it published. - eks-deploy.yml reported failures via issues.createComment using context.issue.number, which is undefined on workflow_dispatch and workflow_call, so the failure notification itself threw. It also passed --values for per-environment files that do not exist, and asserted on deployment/app while the chart creates <release>-app. - ecs-deploy.yml "rolled back" by force-redeploying the same broken task definition. It now captures the previous revision first and rolls back to it, and its health check fails when running count is below desired instead of only printing the number. - devsecops-pipeline.yml re-ran GitLeaks, Semgrep, Checkov and tfsec already covered by security-scan.yml, authenticated with long-lived AWS_ACCESS_KEY_ID secrets while every other workflow used OIDC, and built a Dockerfile at the repository root that does not exist. It is now an orchestrator: test, then build via the reusable container workflow, then deploy. Added: application tests (22, covering input validation, path traversal rejection and that the interactive docs stay disabled), ruff and bandit configuration, a tflint job, a Helm lint job, an actionlint job so this class of bug is caught in future, and the .github/dependabot.yml that was missing from main. Also modernised the app: Pydantic v1 @validator to @field_validator, and fixed CORS treating an unset ALLOWED_ORIGINS as the single origin "" rather than as no origins. actionlint is clean across all workflows; ruff, bandit and pytest pass.
Helm chart: the chart shipped only deployment.yaml, so `helm upgrade` produced a Deployment and nothing else. The eks-deploy workflow then looked for a Service that was never created, and asserted on deployment/app while the template named it <release>-app. Added service, serviceaccount, configmap, hpa, pdb, ingress and networkpolicy templates, a _helpers.tpl so every resource derives its name consistently, and the values-dev/staging/prod.yaml overlays the deploy workflow references. Notable choices: - replicas is omitted from the Deployment when the HPA is enabled, so Helm and the autoscaler stop fighting over the count on every upgrade. - selector labels exclude version, since selectors are immutable once the Deployment exists. - the NetworkPolicy denies egress to 169.254.169.254, so a compromised pod cannot read the node's IAM credentials from IMDS. - no CPU limit: the request already guarantees the share, and a limit only adds throttling before saturation. All three overlays render and lint clean. IaC scanning: Trivy reported four distinct rules across 18 findings. Fixed rather than suppressed: - AWS-0136, SNS topic on an AWS-managed key: security findings now ride on the customer-managed key, whose policy and rotation we control. - AWS-0054, plaintext HTTP listener: the ECS module had an optional certificate that silently degraded the listener to HTTP wherever it was left unset. certificate_arn is now required and validated, port 80 exists only to redirect to 443. - AWS-0345, s3:* in the IAM permission boundary: a boundary is a ceiling, so the wildcard let any role under it delete buckets and rewrite bucket policies, including the ones holding CloudTrail evidence. Enumerated the needed actions and added an explicit deny on audit-log tampering across S3, CloudTrail, Config, GuardDuty and Security Hub. - AWS-0104, unrestricted egress: added interface VPC endpoints for ECR, STS, logs, Secrets Manager, SSM, ELB and EKS, then scoped ECS task and EKS control plane egress to the VPC CIDR and the S3 prefix list. One AWS-0104 instance remains, recorded in .trivyignore.yaml with a written justification: EKS nodes keep 443 egress for third-party registries and Sigstore, which no AWS endpoint fronts. Also pinned the Polaris example pod off :latest, which is mutable and which Polaris counts as having no tag at all. Verified locally: 17 terraform configurations validate, fmt clean, trivy config reports zero CRITICAL/HIGH, kubeconform passes 16 resources, Polaris scores 92 against a threshold of 85, helm lint and actionlint clean.
Installs ArgoCD onto EKS via the Helm provider and hands ongoing delivery to
git. Terraform owns exactly one Application, "root", pointing at
gitops/bootstrap; everything after that is discovered from the repository, so
adding a workload is a file, not a terraform run.
Structure:
- gitops/bootstrap app-of-apps root, ordered with sync waves so the
AppProject exists before anything references it
- gitops/projects the platform AppProject
- gitops/applications one Application per environment
Choices worth stating:
- The AppProject pins sourceRepos to an explicit allow-list. ArgoCD defaults
to '*', which would let anyone able to create an Application pull manifests
from a repository nobody reviews.
- RBAC policy.default is role:readonly. ArgoCD ships an admin default, and
the controller holds cluster-admin on the cluster it manages, so an
over-permissive default there is an over-permissive default on everything.
- Anonymous access is disabled explicitly.
- dev and staging sync automatically with prune and selfHeal, so drift is
corrected without a human and the blast radius is one namespace.
- prod does neither, and tracks a release tag rather than a branch. Merging
to main reaches dev and staging immediately; production moves only when
someone cuts a tag. selfHeal stays off there too, because reverting an
emergency manual change should be a decision rather than a reflex.
- The IRSA trust policy is scoped to system:serviceaccount:argocd:argocd-*
rather than the whole namespace.
- The admin password is never read into Terraform state; the module outputs
the kubectl command to retrieve it instead.
The chart version is pinned so a sync cannot silently upgrade the delivery
control plane itself.
Adds app/ as a pinned copy of aws-containers/retail-store-sample-app v1.6.2 (MIT-0, commit 1a28474). Provenance and refresh instructions are in app/VENDOR.md. Why this app: it is five services in four languages with MySQL, DynamoDB, Redis and RabbitMQ dependencies, which forces the network policy, security group and IRSA work to be correct rather than theoretical. A single first-party service exercised almost none of it. Scanner scope is deliberate. Third-party code does not gate this repository's CI: .semgrepignore excludes app/, pip-audit is scoped to src/, and CodeQL analyses Python only. It is still gated where it matters, since any image built from it goes through the same Trivy and Grype checks as first-party code. Reasoning in ADR 0008. Documentation added: - docs/problem-statement.md the five recurring failures this platform addresses, six use cases mapped to the mechanism that solves each, and an explicit list of what it does not solve. That last section includes the fact that the production OIDC trust policy currently accepts any branch. - docs/adr/0001-0008 decision records covering OIDC over static keys, running both ECS and EKS, GitOps with ArgoCD, the state backend bootstrap, the tfsec to Trivy migration, mandatory TLS, network egress scoping, and vendoring the sample. Each states the problem, the decision, what it costs, and where relevant what was rejected and why. - docs/diagrams/architecture.md platform overview, network topology, delivery paths and control coverage as mermaid diagrams. - docs/scenarios/01-04 four end-to-end walkthroughs, each with the failure modes stated rather than left to be discovered. ADR 0005 records a change that cannot be made from inside the repository: the branch protection required-check contexts still name the removed tfsec job, so merges stay blocked until they are updated.
Adds .mcp.json with seven MCP servers so the platform can be inspected directly rather than through relayed console output. Every server that can reach the account is pinned read-only, via whichever switch that server exposes: READ_OPERATIONS_ONLY and REQUIRE_MUTATION_CONSENT for aws-api, ALLOW_WRITE=false for ecs, --readonly for iam. Answering "why is this unhealthy" should not be able to change the thing being asked about; provisioning stays behind Terraform and a reviewed plan. AWS_PROFILE is default, which resolves to user/muktha-aws. Nothing here uses root. Three findings worth recording, all verified rather than assumed: - awslabs.terraform-mcp-server is yanked on PyPI, superseded by hashicorp/terraform-mcp-server. Switched to HashiCorp's Docker image, pinned to 1.1.0. - awslabs.aws-diagram-mcp-server is yanked with no successor. Omitted rather than replaced, since the diagrams in docs/diagrams/ are mermaid and render natively. - awslabs.ecs-mcp-server installs a console script called ecs-mcp-server, not one named after the package, so `uvx awslabs.ecs-mcp-server` exits 1 silently having found nothing to run. Fixed with --from. A PyPI 200 only proves the name exists, so each server was smoke-tested by sending a real initialize request over stdio. All seven return a serverInfo result. --help is not a usable check here: several of these servers do not implement it and exit non-zero. Setup, credential requirements and the verification command are in docs/mcp-servers.md.
Building and scanning the image locally showed the container gate could never have passed. python:3.12-slim carries 44 CRITICAL/HIGH OS CVEs, every one of them with no fixed version available (perl-base, curl, gzip, libxml2 and friends), and the gate ran with ignore-unfixed: false. A gate nobody can pass does not make anything safer. It gets bypassed, or the severity threshold gets lowered until it goes quiet, and either way the control is gone. The gate now uses ignore-unfixed: true so it fails on findings a developer can act on. Unfixed CVEs are still reported: the SARIF step above it scans at CRITICAL,HIGH,MEDIUM with ignore-unfixed: false, so they stay visible in code scanning, and the base image is refreshed through the Dockerfile bump Dependabot raises. With the gate made actionable, 7 real CVEs surfaced and are now fixed: python-multipart 0.0.9 -> 0.0.32 CVE-2024-53981, CVE-2026-24486, CVE-2026-42561, CVE-2026-53539 starlette 0.37.2 -> 1.3.1 CVE-2024-47874, CVE-2026-48818, CVE-2026-54283 (via fastapi) fastapi 0.111.0 -> 0.140.8 uvicorn 0.30.1 -> 0.51.0 pydantic 2.7.4 -> 2.13.4 boto3 1.34.131 -> 1.43.57 Worth noting for the open Dependabot PR: it proposes python-multipart 0.0.27, which still carries CVE-2026-53539. The floor is 0.0.30, so that PR would not have closed the advisory it was raised for. Verified after the bump: the image rebuilds, trivy reports zero actionable CRITICAL/HIGH, and all 22 tests pass against the new versions. README updated with a documentation index, the certificate_arn requirement, and the GitOps delivery flow that replaced the direct helm upgrade.
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 35235625 | Triggered | Generic Password | 0acd449 | app/.github/workflows/e2e-test.yml | View secret |
| 35235624 | Triggered | Generic Password | a58ab1e | src/app/tests/test_app.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secrets safely. Learn here the best practices.
- Revoke and rotate these secrets.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
The first CI run on this branch failed during "Set up job", which is what GitHub reports when an action reference cannot be resolved. Two were wrong: - aquasecurity/trivy-action@0.33.1 — the tag is v0.33.1. I dropped the v prefix. Now pinned to v0.36.0, the current release. - sigstore/cosign-installer@v4 — this project publishes v4.1.2 and v4.1.1 but no moving v4 tag, so the major-version shorthand that works for most actions does not work here. Pinned to v4.1.2. Both were my error: I checked that the packages and releases existed but assumed the ref format rather than resolving it. Adds scripts/verify-action-refs.sh, which resolves every `uses:` reference against the GitHub API, and runs it in the workflow-lint job. actionlint validates syntax but not whether a pinned ref exists, and a bad ref produces no useful error — the job simply dies before running a step. All 24 references now resolve.
tflint found 30 issues on the first CI run. Eleven were in the vendored app/ tree, which is not ours to restyle; leaving them would have trained everyone to ignore the tool. CI now runs tflint with --chdir=terraform, and .tflint.hcl records why. The rest were real, and several pointed at wiring bugs rather than style: - aws-config accepted kms_key_arn and never used it, so Config snapshots were landing under the bucket default rather than the key the module was handed. Now passed to the delivery channel as s3_kms_key_arn. - monitoring accepted eks_cluster_name and never used it, so the dashboard showed ECS utilisation only and nothing about the Kubernetes nodes. Added the node CPU and memory widget it was clearly meant to have. - monitoring also accepted kms_key_arn but owns no log group, so the argument was noise. Removed, along with the call sites. - cloudtrail accepted s3_bucket_arn but the bucket policy lives in modules/s3. Removed. - argocd accepted cluster_name and never used it. Removed. - organizations declared root_email_prefix and org_domain for member account creation it does not do. Removed rather than left as a misleading interface, with a comment explaining that account creation is deliberately a manual step because it is effectively irreversible. - Typed the remaining untyped variables across aws-config, ecr, iam and organizations. tflint now reports zero issues.
The build and plan jobs failed with "Could not assume role with OIDC: Request ARN is invalid" after retrying twelve times over two and a half minutes. The cause was not the role or the trust policy: this repository has no secrets set at all, so AWS_ACCOUNT_ID rendered empty and the ARN came out as arn:aws:iam:::role/dev-cicd-deploy-role. That error message points nowhere near the actual problem, and it will recur for anyone cloning this repository, since the AWS side cannot work until bootstrap has run. Both workflows now start with an aws-preflight job that checks whether AWS_ACCOUNT_ID is set. When it is not, the jobs needing live credentials skip, and the run summary states what to do: run scripts/bootstrap.sh, apply terraform/environments/dev, then set the secret. Validation, formatting, linting and the application tests are unaffected and still gate the pull request, because none of them need an AWS account. Also wires TF_VAR_certificate_arn from an ACM_CERTIFICATE_ARN secret. That variable lost its default when the plaintext listener was removed, so plan would otherwise fail on a missing required variable once credentials exist.
|
You are seeing this message because GitHub Code Scanning has recently been set up for this repository, or this pull request contains the workflow file for the Code Scanning tool. What Enabling Code Scanning Means:
For more information about GitHub Code Scanning, check out the documentation. |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Semgrep OSS found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Checkov reported 15 findings. Fixed rather than suppressed where the finding described a real weakness: - CKV_AWS_274: the Config auto-remediation role carried AdministratorAccess, assumable by ssm.amazonaws.com with no condition. That made the remediation path a privilege escalation route: anything able to invoke SSM automation inherited unrestricted access to the account. Replaced with the seven S3 actions AWS-DisableS3BucketPublicReadWrite actually calls, plus an aws:SourceAccount condition on the trust policy to close the confused-deputy hole. - CKV_AWS_338: raised VPC flow log, WAF log and EKS control plane log retention from 90 days to a year. These are the records an investigation reads; ninety days is shorter than most breaches take to surface. Documented as accepted, each at the resource rather than in a global list: - ECS application log retention stays at 30 days. It is stdout, not audit evidence, and the security record already lives in CloudTrail and the ALB access logs. - The EKS node ingress range spans 1025-65535 because kubelet and extension API servers bind ephemeral ports. It is reachable only from the control plane security group, never from a CIDR. - IAM policies keeping Resource "*" do so for calls that do not accept a resource ARN (ecr:GetAuthorizationToken, the Describe and List calls), or because the remediation handlers act on whichever resource a finding names. Both roles sit under the permission boundary, which denies IAM mutation and audit-log tampering outright. - CloudTrail SNS notification, S3 event notifications and the GuardDuty graph check, each with the reason it does not apply here. Also fixes the Kubernetes Manifest Scan job, which 404'd installing Polaris. The release tag carries a v prefix but the asset filename does not and repeats the version: v10.2.1/polaris_10.2.1_linux_amd64.tar.gz. Same class of mistake as the trivy-action tag, and the reason scripts/verify-action-refs.sh now exists. Adds .gitguardian.yaml scoping secret scanning away from the vendored tree. The two findings there are AWS_ACCESS_KEY_ID=key and AWS_SECRET_ACCESS_KEY=dummy in the sample's cart service, which DynamoDB Local requires to be set but ignores the value of. GitLeaks, which gates this repository's CI, already reports no leaks across the full history. Verified locally: fmt, validate, tflint, checkov, trivy, actionlint, action ref resolution, helm lint and template, kubeconform, polaris, gitleaks, pytest, ruff, bandit and pip-audit all pass.
Comment on lines
+280
to
+315
| resource "aws_iam_role_policy" "config_remediation" { | ||
| # checkov:skip=CKV_AWS_355: the remediation acts on whichever bucket the | ||
| # Config finding names, which is not knowable when the policy is written. | ||
| # checkov:skip=CKV_AWS_289: the permissive-sounding actions here are the | ||
| # minimum AWS-DisableS3BucketPublicReadWrite needs to restore a public access | ||
| # block. This replaced an AdministratorAccess attachment, so the wildcard | ||
| # resource on seven scoped S3 actions is a large reduction, not an expansion. | ||
| name = "${var.environment}-config-remediation-policy" | ||
| role = aws_iam_role.config_remediation.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Sid = "RestoreS3PublicAccessBlock" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:PutBucketPublicAccessBlock", | ||
| "s3:GetBucketAcl", | ||
| "s3:PutBucketAcl", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketLocation", | ||
| "s3:ListAllMyBuckets", | ||
| ] | ||
| Resource = "*" | ||
| }, | ||
| { | ||
| Sid = "ReportRemediationOutcome" | ||
| Effect = "Allow" | ||
| Action = ["config:PutEvaluations", "ssm:GetAutomationExecution"] | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
Comment on lines
+280
to
+315
| resource "aws_iam_role_policy" "config_remediation" { | ||
| # checkov:skip=CKV_AWS_355: the remediation acts on whichever bucket the | ||
| # Config finding names, which is not knowable when the policy is written. | ||
| # checkov:skip=CKV_AWS_289: the permissive-sounding actions here are the | ||
| # minimum AWS-DisableS3BucketPublicReadWrite needs to restore a public access | ||
| # block. This replaced an AdministratorAccess attachment, so the wildcard | ||
| # resource on seven scoped S3 actions is a large reduction, not an expansion. | ||
| name = "${var.environment}-config-remediation-policy" | ||
| role = aws_iam_role.config_remediation.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Sid = "RestoreS3PublicAccessBlock" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:PutBucketPublicAccessBlock", | ||
| "s3:GetBucketAcl", | ||
| "s3:PutBucketAcl", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketLocation", | ||
| "s3:ListAllMyBuckets", | ||
| ] | ||
| Resource = "*" | ||
| }, | ||
| { | ||
| Sid = "ReportRemediationOutcome" | ||
| Effect = "Allow" | ||
| Action = ["config:PutEvaluations", "ssm:GetAutomationExecution"] | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
Comment on lines
+280
to
+315
| resource "aws_iam_role_policy" "config_remediation" { | ||
| # checkov:skip=CKV_AWS_355: the remediation acts on whichever bucket the | ||
| # Config finding names, which is not knowable when the policy is written. | ||
| # checkov:skip=CKV_AWS_289: the permissive-sounding actions here are the | ||
| # minimum AWS-DisableS3BucketPublicReadWrite needs to restore a public access | ||
| # block. This replaced an AdministratorAccess attachment, so the wildcard | ||
| # resource on seven scoped S3 actions is a large reduction, not an expansion. | ||
| name = "${var.environment}-config-remediation-policy" | ||
| role = aws_iam_role.config_remediation.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Sid = "RestoreS3PublicAccessBlock" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:PutBucketPublicAccessBlock", | ||
| "s3:GetBucketAcl", | ||
| "s3:PutBucketAcl", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketLocation", | ||
| "s3:ListAllMyBuckets", | ||
| ] | ||
| Resource = "*" | ||
| }, | ||
| { | ||
| Sid = "ReportRemediationOutcome" | ||
| Effect = "Allow" | ||
| Action = ["config:PutEvaluations", "ssm:GetAutomationExecution"] | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
Comment on lines
+280
to
+315
| resource "aws_iam_role_policy" "config_remediation" { | ||
| # checkov:skip=CKV_AWS_355: the remediation acts on whichever bucket the | ||
| # Config finding names, which is not knowable when the policy is written. | ||
| # checkov:skip=CKV_AWS_289: the permissive-sounding actions here are the | ||
| # minimum AWS-DisableS3BucketPublicReadWrite needs to restore a public access | ||
| # block. This replaced an AdministratorAccess attachment, so the wildcard | ||
| # resource on seven scoped S3 actions is a large reduction, not an expansion. | ||
| name = "${var.environment}-config-remediation-policy" | ||
| role = aws_iam_role.config_remediation.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Sid = "RestoreS3PublicAccessBlock" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:PutBucketPublicAccessBlock", | ||
| "s3:GetBucketAcl", | ||
| "s3:PutBucketAcl", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketLocation", | ||
| "s3:ListAllMyBuckets", | ||
| ] | ||
| Resource = "*" | ||
| }, | ||
| { | ||
| Sid = "ReportRemediationOutcome" | ||
| Effect = "Allow" | ||
| Action = ["config:PutEvaluations", "ssm:GetAutomationExecution"] | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
Comment on lines
+280
to
+315
| resource "aws_iam_role_policy" "config_remediation" { | ||
| # checkov:skip=CKV_AWS_355: the remediation acts on whichever bucket the | ||
| # Config finding names, which is not knowable when the policy is written. | ||
| # checkov:skip=CKV_AWS_289: the permissive-sounding actions here are the | ||
| # minimum AWS-DisableS3BucketPublicReadWrite needs to restore a public access | ||
| # block. This replaced an AdministratorAccess attachment, so the wildcard | ||
| # resource on seven scoped S3 actions is a large reduction, not an expansion. | ||
| name = "${var.environment}-config-remediation-policy" | ||
| role = aws_iam_role.config_remediation.id | ||
|
|
||
| policy = jsonencode({ | ||
| Version = "2012-10-17" | ||
| Statement = [ | ||
| { | ||
| Sid = "RestoreS3PublicAccessBlock" | ||
| Effect = "Allow" | ||
| Action = [ | ||
| "s3:GetBucketPublicAccessBlock", | ||
| "s3:PutBucketPublicAccessBlock", | ||
| "s3:GetBucketAcl", | ||
| "s3:PutBucketAcl", | ||
| "s3:GetBucketPolicyStatus", | ||
| "s3:GetBucketLocation", | ||
| "s3:ListAllMyBuckets", | ||
| ] | ||
| Resource = "*" | ||
| }, | ||
| { | ||
| Sid = "ReportRemediationOutcome" | ||
| Effect = "Allow" | ||
| Action = ["config:PutEvaluations", "ssm:GetAutomationExecution"] | ||
| Resource = "*" | ||
| } | ||
| ] | ||
| }) | ||
| } |
Comment on lines
+263
to
+273
| resource "aws_vpc_security_group_ingress_rule" "node_from_cluster" { | ||
| # checkov:skip=CKV_AWS_25: The range spans 1025-65535 because kubelet and | ||
| # extension API servers bind ephemeral ports, which EKS requires. It is | ||
| # reachable only from the control plane security group, not from any CIDR. | ||
| security_group_id = aws_security_group.node.id | ||
| description = "Kubelet and extension API traffic from the control plane" | ||
| referenced_security_group_id = aws_security_group.cluster.id | ||
| from_port = 1025 | ||
| to_port = 65535 | ||
| ip_protocol = "tcp" | ||
| } |
Comment on lines
+263
to
+273
| resource "aws_vpc_security_group_ingress_rule" "node_from_cluster" { | ||
| # checkov:skip=CKV_AWS_25: The range spans 1025-65535 because kubelet and | ||
| # extension API servers bind ephemeral ports, which EKS requires. It is | ||
| # reachable only from the control plane security group, not from any CIDR. | ||
| security_group_id = aws_security_group.node.id | ||
| description = "Kubelet and extension API traffic from the control plane" | ||
| referenced_security_group_id = aws_security_group.cluster.id | ||
| from_port = 1025 | ||
| to_port = 65535 | ||
| ip_protocol = "tcp" | ||
| } |
Comment on lines
+17
to
+37
| resource "aws_guardduty_detector" "main" { | ||
| # checkov:skip=CKV2_AWS_3: the graph check looks for the detector and a | ||
| # Config recorder in one module. They are deliberately separate modules here; | ||
| # both are instantiated by every environment. | ||
| enable = true | ||
| finding_publishing_frequency = var.finding_publishing_frequency | ||
|
|
||
| tags = { Environment = var.environment } | ||
|
|
||
| datasources { | ||
| s3_logs { enable = true } | ||
| kubernetes { | ||
| audit_logs { enable = true } | ||
| } | ||
| malware_protection { | ||
| scan_ec2_instance_with_findings { | ||
| ebs_volumes { enable = true } | ||
| } | ||
| } | ||
| } | ||
| } |
Comment on lines
9
to
+80
| resource "aws_s3_bucket" "main" { | ||
| bucket = var.bucket_name | ||
| force_destroy = false | ||
| tags = { Environment = var.environment } | ||
| # checkov:skip=CKV2_AWS_62: event notifications are not configured. These are | ||
| # log archive buckets written by AWS services; object-level reaction is | ||
| # handled by Config rules and GuardDuty S3 protection, not by a notification | ||
| # fan-out that nothing currently consumes. | ||
| bucket = var.bucket_name | ||
| force_destroy = false | ||
| object_lock_enabled = var.object_lock_enabled | ||
| tags = { Environment = var.environment } | ||
| } |
Comment on lines
9
to
+80
| resource "aws_s3_bucket" "main" { | ||
| bucket = var.bucket_name | ||
| force_destroy = false | ||
| tags = { Environment = var.environment } | ||
| # checkov:skip=CKV2_AWS_62: event notifications are not configured. These are | ||
| # log archive buckets written by AWS services; object-level reaction is | ||
| # handled by Config rules and GuardDuty S3 protection, not by a notification | ||
| # fan-out that nothing currently consumes. | ||
| bucket = var.bucket_name | ||
| force_destroy = false | ||
| object_lock_enabled = var.object_lock_enabled | ||
| tags = { Environment = var.environment } | ||
| } |
Semgrep reported 85 findings, all in this repository's own workflows rather
than the vendored tree. Fixed rather than suppressed:
- run-shell-injection (10, ERROR). ecs-deploy and eks-deploy interpolated
${{ inputs.* }} directly into run: blocks. GitHub substitutes those before
the shell parses the line, so a value containing shell metacharacters is
executed rather than read. These workflows are callable via
workflow_dispatch, so the inputs are attacker-influenced for anyone with
write access. Every interpolation now arrives through env: and is
referenced as a quoted shell variable.
- secrets-inherit (3, ERROR). The pipeline passed `secrets: inherit` to
three called workflows, handing over every repository secret including
ones they have no use for. The reusable workflows now declare the single
secret they accept, and callers pass it by name.
- github-actions-mutable-action-tag (66, WARNING). Actions were pinned to
moving tags such as @v5, so the code executing with this repository's OIDC
role could change without any commit here. All 66 references are now
pinned to full commit SHAs with the tag retained as a trailing comment, so
the version stays readable and Dependabot can still bump them.
- dependabot-missing-cooldown (4). Added a 7-day cooldown, 14 for majors, so
an automated bump is not the delivery mechanism for a package compromised
and yanked within days of publication.
- gha-workflow-env-secret (1). The ACM certificate ARN sat in workflow-level
env, which exposes it to every job in the file. Scoped to the four steps
that plan or apply.
- curl-pipe-bash (1). scripts/setup.sh still installed tfsec, which is end of
life, by piping a remote script into bash. Removed; Trivy is what CI runs.
scripts/verify-action-refs.sh now resolves SHA pins as commits rather than
refs, so it keeps working against the new format.
Semgrep now reports zero findings. Also verified: fmt, validate, tflint,
checkov, trivy, actionlint, ref resolution, helm, kubeconform, polaris,
gitleaks, pytest, ruff, bandit, pip-audit and shellcheck all pass.
Adds ADR 0009 covering SHA-pinning of GitHub Actions: what a moving tag actually exposes given these workflows hold an OIDC role that can push images and run Terraform, why the trailing version comment matters for Dependabot, and why pinning and the Dependabot cooldown address different halves of the same problem rather than substituting for each other. Updates ADR 0005 now that the branch protection contexts have been changed from the dead tfsec check to the Trivy one. It also records the two other reasons merges were blocked, neither of which is visible in the repository's files: GitHub had auto-disabled the Security Scanning and Compliance Report workflows for inactivity, and enforce_admins combined with a single-owner CODEOWNERS means a pull request that owner authors cannot be approved or admin-merged.
Bootstrap has run, so AWS_ACCOUNT_ID is now set as a repository secret. That
alone would have made things worse: the preflight only checked whether the
secret existed, so setting it flipped the build and deploy jobs from a clean
skip to a red AssumeRole failure, because dev-cicd-deploy-role does not
exist until terraform apply creates it.
The preflight now attempts the role assumption itself with
continue-on-error, and reports configured=true only when the role is
genuinely reachable. The job summary distinguishes the two cases: secret
missing, versus secret present but the role not yet created.
This means the secret can be set at any point in the sequence without
turning CI red, which matters because bootstrap and apply are separate steps
that can be days apart.
Bootstrap created, all outside Terraform by design (see ADR 0004):
devsecops-aws-tfstate-{dev,staging,prod}
terraform-state-lock (DynamoDB, PAY_PER_REQUEST, SSE-KMS)
GitHub OIDC provider (already present, left as-is)
This was referenced Jul 30, 2026
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
2 participants
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.
Why
Every scheduled workflow run on
mainwas failing, and the Terraform tree could not be validated, let alone applied. Both older PRs were blocked as a result.What was actually broken
Terraform would not parse.
modules/monitoringused semicolons as HCL attribute separators, andsecurity/wafhad nested blocks written on one line — never caught, because that file lived outside any validated path.environments/prodreferenced../../modules/guarddutyand../../modules/waf, neither of which existed.The network could not have worked. The VPC module created a VPC and four subnets with no internet gateway, no NAT and no route tables. EKS then placed nodes in those unrouted subnets behind a private-only API endpoint, so the node group could never have registered. There was no OIDC provider either, so no IRSA role could ever be assumed.
Controls that were on but not connected. CloudTrail created a CloudWatch log group and never attached it to the trail, so every CIS metric filter and alarm built on it silently matched nothing. The GuardDuty filter was set to
ARCHIVEon high-severity findings, suppressing exactly what it existed to surface.Four CI jobs, four separate causes. tfsec's action is end-of-life and no longer runs; Checkov could not parse the directory; the Polaris action reference does not resolve, so that job died during Set up job; and CodeQL was asked for
javascript, which this repository does not contain.Three things blocked merges, none visible in the files. The required status check named the now-deleted tfsec job; GitHub had auto-disabled the Security Scanning and Compliance Report workflows for repository inactivity; and
enforce_adminscombined with a single-owner CODEOWNERS makes an owner-authored PR unapprovable.Security findings fixed, not suppressed
CKV_AWS_274aws:SourceAccountguard.run-shell-injection×10${{ inputs.* }}interpolated straight intorun:blocks in the deploy workflows. Now passed throughenv:.secrets-inherit×3secrets: inherithanded every repository secret to called workflows. Now named explicitly.mutable-action-tag×66AWS-0345s3:*in the IAM permission boundary let any role under it delete the buckets holding CloudTrail evidence. Enumerated, plus an explicit audit-tamper deny.AWS-0054certificate_arnis now required.python-multipart,starletteand transitivelyfastapi.Two judgement calls worth reviewing
The image scan gate could never have passed.
python:3.12-slimcarries 44 CRITICAL/HIGH OS CVEs, every one with no fix available, and the gate ranignore-unfixed: false. A gate nobody can pass gets bypassed or has its threshold lowered until it goes quiet. It now fails on findings a developer can act on, and still reports the rest to code scanning.Dependabot PR #2 would not have fixed what it was raised for. It proposes
python-multipart0.0.27, which still carries CVE-2026-53539; the floor is 0.0.30. This PR goes to 0.0.32.What this adds
ArgoCD with app-of-apps GitOps · a complete Helm chart (it previously shipped only
deployment.yaml, so the deploy workflow looked for a Service that never existed) · the AWS retail-store sample vendored atv1.6.2inapp/· 9 ADRs, a problem statement, mermaid diagrams and 4 scenario walkthroughs · 7 read-only AWS Labs MCP servers ·scripts/verify-action-refs.sh, becauseactionlintdoes not check that a pinned ref exists upstream.Verified locally
terraform fmt/validate(18 configs) ·tflint·checkov·trivy(IaC + image) ·actionlint· action-ref resolution ·semgrep(0 findings) ·helm lint+ template ×3 ·kubeconform·polaris·gitleaks·pytest(22) ·ruff·bandit·pip-audit·shellcheck.Remaining, and needing you
GitGuardian Security Checksfails on 2 findings —AWS_ACCESS_KEY_ID=key/AWS_SECRET_ACCESS_KEY=dummyin the vendored sample's cart service, which DynamoDB Local requires to be set but ignores the value of..gitguardian.yamlscopes them out, but the GitHub App reads its config from the GitGuardian dashboard rather than the repository, so they need ignoring there. It is not a required check and does not block the merge. GitLeaks, which does gate CI, reports no leaks across the full history.This PR cannot be merged as configured. All five required checks pass, but
enforce_adminsis on and.github/CODEOWNERSnames@Mpurushothamfor every path, so the sole code owner cannot approve their own PR and--adminis disabled too. Merging needs one of: a second reviewer with write access, temporarily unsettingenforce_admins, or narrowingrequire_code_owner_reviews. That is a policy decision, so I have not touched it.