Phase 0: fix silently wrong assessments, recover lost work, tighten CI - #21
Open
carlosinfantes wants to merge 4 commits into
Open
carlosinfantes wants to merge 4 commits into
carlosinfantes wants to merge 4 commits into
Conversation
Five defects that made assessments report incomplete or wrong results:
- run_analyzer derived the scope key from the class name ("IAMAnalyzer" ->
"iamanalyzer"), which never matched the scope map, so --scope skipped
every native analyzer and still reported COMPLETED with no findings.
Analyzers now declare `name` and `scopes` explicitly, and BaseAnalyzer
rejects a subclass that does not.
- The network analyzer read fl["ResourceType"], a field DescribeFlowLogs
does not return, so any account with flow logs raised KeyError and lost
every network finding.
- The Prowler OCSF parser read `status` (always "New") instead of
`status_code`, so passing checks became findings, including CRITICAL
ones. It also read the check ID, compliance mappings and resource ARN
from the wrong fields.
- Prowler ran five EKS check IDs that do not exist. Prowler exits 1 on
unknown IDs, so the next image built from main would have failed every
scan. Use the real IDs and pin prowler==5.42.0.
- The native puller crashed on GuardDuty float scores (DynamoDB rejects
floats) after writing an arbitrary subset of the batch, and its
FINDINGS_TABLE was missing from the CDK environment, so it always wrote
to the dev table.
Also restores the Prowler --scope filtering lost in the v0.3.0 reconcile,
and writes the expiresAt TTL attribute that all three tables are
configured for but no writer ever set, so data was kept forever.
The account-ID pre-commit hook matched the last group of a UUID and had no
placeholders for tests; it now skips digits adjacent to a hyphen and allows
the documented example IDs.
Tests: 276, up from 219, including the first moto-backed ones.
- invalidate_endpoint built its key without the profile, so it never matched the key resolve_api_endpoint writes and the refresh after a connection error kept using the stale endpoint. Both now call one cache_key() helper. - The CLI declared Python 3.9 support but fails to import on it, and its own dependencies require 3.10; say 3.10 in pyproject, install.sh and the README. - Lambda assets shipped whatever sat in the source directory, including locally generated cloudsecure-report.* files with real account data, and __pycache__ that changes the asset hash on every build. All assets now go through pythonLambdaCode(), which excludes them; the report_generator template still ships. - The README onboarding command was missing the required CloudSecureAccountId parameter, so it always failed. CI: - Add a CLI job (Python 3.10 and 3.13) with AWS access disabled, so a unit test can no longer reach a real account. - Add cdk synth to the CDK job: compiling does not catch wiring errors. - Measure coverage over all nine packages instead of five (the partial list reported 73% where the real figure is 54%) and fail under 50%. - publish-cli now runs the tests and refuses to publish when the tag does not match __version__: 0.3.0 was published with a failing test. - publish-prowler validates every check ID against the pinned Prowler before building the image. - Update the action majors and move the CDK job to Node 24: Node 20 is removed from the runners on 2026-09-23. - Declare jinja2, which report_generator imports.
The v0.3.0 squash resolved conflicts by taking whole files from one side, which silently reverted two changes that had already been merged: - docs/architecture.html lost the SVG alignment fix from #10. - CONTRIBUTING.md lost the "Testing & Validation" and "Commits" sections from #18 and went back to the manual docker layer build. CONTRIBUTING comes back from #18, plus what is true today: Podman, the layer caveat on non-Linux hosts, the CLI and synth jobs, and a note to prefer moto where the AWS response shape is what the code depends on. Documentation now matches the code: - The README no longer claims NIST/ISO/GDPR/SOC2 mappings (only CIS 1.4 references exist, and they do not reach the report), no longer says missing security services are reported as findings, and states that AI synthesis currently falls back because the model is end-of-life. - IMPLEMENTATION.md opens with the real state instead of a table of six sprints at 100%, and its test figures match reality (302 tests, 54% coverage, not "38 tests, 93%"). The sprint checklists keep their history: they record what was built, not what works.
infrastructure/test held the commented-out "SQS Queue Created" stub from cdk init, so the CDK job asserted nothing. Assert instead what silently breaks at runtime: every findings writer gets FINDINGS_TABLE, every CloudSecure function gets ASSESSMENTS_TABLE, and the Bedrock model comes from the environment. A missing environment variable is not a deploy error — the handler falls back to a dev table name — so only a synth-time assertion catches it.
This branch has not been deployed
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.
Follow-up to the September 2026 audit. This is the stabilisation batch: defects that made assessments report wrong or incomplete results, plus the CI that let them through green.
What was broken
--scopeskipped every native analyzer (base.pyderived"iamanalyzer"from the class name, the map had"iam_analyzer")assess --scope iamreturned zero native findings and still reported COMPLETEDfl["ResourceType"], a fieldDescribeFlowLogsdoes not returnstatus(always"New") instead ofstatus_codemainwould have failed 100% of scansFINDINGS_TABLEwas missing from the CDK environmentexpiresAtwas never writteninvalidate_endpointbuilt a key without the profilecloudsecure-report.*from the source directoryAlso restores the Prowler
--scopefiltering, thearchitecture.htmlSVG fix and the CONTRIBUTING sections that the v0.3.0 squash silently reverted.CI
The suite was green while all of the above was broken, because no test used a real AWS response shape and half the packages were not measured.
AWS_CONFIG_FILE=/dev/null: a unit test was making realDescribeStackscalls.cdk synthin CI, and real CDK assertions replacing thecdk initplaceholder that asserted nothing.publish-cliruns the tests and refuses a tag that does not match__version__: 0.3.0 was published with a failing test.publish-prowlervalidates every check ID against the pinned Prowler before building.Docs
README and IMPLEMENTATION.md now match the code: no NIST/ISO/GDPR/SOC2 mapping claims, no "missing services reported as findings", AI synthesis marked as falling back, and the real test numbers.
Verification
276 Lambda tests, 26 CLI tests, 11 CDK tests,
cdk synthon dev/prod/skipProwler, ruff and black clean. Not deployed to any account.Not in this PR
Known and tracked, needs design work: Step Functions failure handling (assessments stuck RUNNING), the 256 KiB payload ceiling, the end-of-life Bedrock model,
ReadOnlyAccessin the onboarding role, and the shared layer exported across stacks.