feat: make PQC validation fail closed and AWS-tested#1
Conversation
Independent Cursor reviewI ran a read-only implementation review of Verdict: PASS. No material release blockers found. The review independently confirmed:
The follow-up review also confirmed that the public documentation now defines the repository's legacy shorthand, and that renamed workflow display names, job IDs, artifact names, and AWS role-session names do not break in-repository dependencies or change the credential boundary. What the “janitor” is“Janitor” is shorthand for the cleanup safety net implemented by Normal cleanup happens inside each live test and again in a post-test cleanup job that runs even after failure. If a runner is canceled, crashes, or disappears before either path finishes, the hourly cleanup safety net runs from a separate protected GitHub environment. It only targets resources tagged as QuantumForge integration-test resources whose It is not a general AWS cleanup process and does not target production or unrelated resources. Non-blocking suggestions
The Cursor run was review-only. It made no file changes, commits, AWS calls, or external mutations. |
17407ba to
1bc987a
Compare
IAwiz87
left a comment
There was a problem hiding this comment.
Reviewed end-to-end with direct test execution, not just trusting the PR description — all claims verified: terraform fmt clean, opa test 42/42, terraform test 2/2 + 6/6 + 5/5, CycloneDX CBOM (10 components), Conftest hybrid-pass (8/8) and classical-fail (exactly 2 expected failures), fail-closed census behavior on empty/malformed plans, and the AWS provider 6.1.0→6.2.0 justification cross-checked against HashiCorp's own withdrawal notice (hashicorp/terraform-provider-aws#43213).
Confirmed improvements: fixes a real fail-open bug in the old census workflow (silently reported "compliant" on plan failure); meaningfully tightens OIDC/id-token: write exposure so PR-triggered runs can no longer assume the AWS role; least-privilege IAM boundary with tag-gated mutations and an explicit anti-tamper Deny; a properly account/tag-scoped sandbox cleanup script; a real exception lifecycle wired into the policy deny logic; and a risk-scoring change (impact as a co-primary dimension) that lines up with the roadmap. Nice work.
Non-blocking items for a follow-up, not this PR (see inline comments):
enable_pqc_kms_signing's default flipstrue→false— confirm intentional.- Tag-key list asymmetry between KMS key creation and ACM certificate creation in the IAM boundary.
try_delete()in the cleanup script swallows all errors silently — fine given the end-of-script verification, but worth a debug log line eventually.ROADMAP.mdis substantially rewritten rather than incrementally updated — worth a maintainer pass to confirm the new framing is the one to keep.- Two prior capabilities quietly disappear rather than carrying forward: PR-comment posting from the old census workflow, and the previously-disabled CMVP status-change issue-creation stub. Confirm you're fine losing both.
None of the above block merge — approving.
| description = "Whether to provision the reference ML-DSA KMS signing key." | ||
| type = bool | ||
| default = true | ||
| default = false |
There was a problem hiding this comment.
Default flips from true to false, not just a rename from enable_hybrid_pqc_kms. Confirm this behavior change (KMS signing key no longer provisioned by default) is intentional.
| "aws:RequestTag/project": "quantumforge", | ||
| "aws:RequestTag/environment": "integration-test" | ||
| }, | ||
| "ForAllValues:StringEquals": { |
There was a problem hiding this comment.
ACM certificate creation's required tag-key list omits owner (and the quantumforge:* keys) that the KMS key creation list requires above. Confirm this asymmetry is intentional.
| ' | ||
| } | ||
|
|
||
| try_delete() { |
There was a problem hiding this comment.
try_delete() swallows all command errors, not just "already deleted" ones. The end-of-script remaining-resource check catches aggregate failures, but you lose the specific error reason if something unexpected breaks here.
| @@ -1,109 +1,66 @@ | |||
| # QuantumForge Roadmap | |||
There was a problem hiding this comment.
This replaces the roadmap doc wholesale rather than updating it incrementally — worth a maintainer pass to confirm the new prioritization (discovery → governance → gated platform modules) is the direction you want, versus merging with the original framing.
Summary
This PR turns QuantumForge's AWS post-quantum cryptography (PQC) path from a syntax-validated proof of concept into a credential-free tested and live-validated reference implementation. Missing, malformed, or contradictory inputs now stop the assessment instead of producing an empty or compliant result.
1. AWS provider compatibility
~> 6.2, which contains ML-DSA support for AWS Key Management Service (KMS).2. Terminology and standards
modules/hybrid-pqc-kmstomodules/pqc-kms-signing; ML-DSA is pure post-quantum signing, not a classical/PQC hybrid signature.movedblock.HTTPS). Network Load Balancer (NLB)TLSsupport remains a separate implementation.3. Credential-free Terraform tests
Adds native
mock_provider "aws"tests for:Pull-request tests clear AWS credential variables and disable EC2 metadata access.
4. Fail-closed CI
contents: read; checkout does not persist the GitHub token and no PR-executed job receives OpenID Connect (OIDC) authority.assessment_complete,no_assets_found,collection_failed, andnot_assessed.5. Real AWS ML-DSA lifecycle
Adds an explicitly authorized, account-guarded temporary KMS test deployment that:
ML_DSA_65key;PendingDeletionin the expected seven-day window.6. Real ALB PQ-TLS runtime validation
Adds a temporary ALB test deployment and capable OpenSSL client that proves:
X25519MLKEM768;X25519;ELBSecurityPolicy-TLS13-1-2-Res-PQ-2025-09;Live tests are manual,
main-only jobs inside protected GitHub environments, which restrict approval, branch access, and variables. They use job-scoped GitHub OIDC instead of stored AWS access keys. Cleanup is layered: process traps, a post-test job that runs even after failure, and an hourly cleanup workflow that removes expired tagged test resources if a runner is canceled or crashes. The workflow filename and environment retain the legacy internal name “janitor.”7. Scoring, governance, and evidence
8. Discovery before more deployment modules
unknownas a first-class assessment outcome.Verification
Credential-free and static
terraform fmt -check -recursivecheck-jsonschema0.37.4: canonical and scoring-enriched inventories passedLive AWS sandbox
Executed locally in an isolated
us-east-1sandbox on 2026-07-20:ML_DSA_65sign/verify: passedPendingDeletionX25519MLKEM768X25519Raw sandbox evidence is intentionally not committed because the private source material contains ephemeral resource identifiers. The sanitized bundles contain only the allowlisted runtime assertions.
Upstream Actions status
At PR creation, GitHub reported all three workflows already present on the upstream default branch as
disabled_manually. No check suite or status context was created for this PR. A maintainer must enable GitHub Actions before hosted CI can run; the verification above is from the exact pushed commit.Migration and operator notes
enable_hybrid_pqc_kms→enable_pqc_kms_signing.module.hybrid_pqc_kms[0]tomodule.pqc_kms_signing[0].quantumforge-aws-sandboxand the protected expired-resource cleanup environmentquantumforge-aws-sandbox-janitor, plus exact OIDC trust subjects and repository variablesQUANTUMFORGE_AWS_ROLE_ARN/QUANTUMFORGE_AWS_ACCOUNT_ID.QUANTUMFORGE_EVIDENCE_BUCKETand the rendered IAM/Object Lock templates underdocs/aws/.us-east-1; the hourly expired-resource cleanup workflow bounds ordinary orphan duration, but operators should keep an account budget alert.