BCH-1296: AWS ECR compliance plugin#1
Merged
Merged
Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
📝 WalkthroughWalkthroughAdds a new plugin-aws-ecr: project scaffolding (CI, Makefile, goreleaser, module), config parsing, ECR data fetcher (repos/registry/images/scan findings), OPA-based PolicyEvaluator, plugin wiring (Configure/Init/Eval), and comprehensive unit tests and docs. ChangesAWS ECR Plugin
Sequence Diagram(s)sequenceDiagram
participant Client as Runner Framework
participant Plugin as CompliancePlugin
participant Fetcher as DataFetcher
participant Evaluator as PolicyEvaluator
participant API as ApiHelper
Client->>Plugin: Configure + Init
Client->>Plugin: Eval(regions, policies)
loop Per Region
Plugin->>Fetcher: FetchRepositories(region)
Fetcher-->>Plugin: []RepositoryContext
Plugin->>Evaluator: EvalRepository(...policies)
Evaluator-->>Plugin: []Evidence
Plugin->>Fetcher: FetchRegistryConfig(region)
Fetcher-->>Plugin: RegistryContext
Plugin->>Evaluator: EvalRegistry(...policies)
Evaluator-->>Plugin: []Evidence
Plugin->>Fetcher: FetchImages(region, repos, lookbackDays)
Fetcher-->>Plugin: []ImageContext
Plugin->>Evaluator: EvalImage(...policies)
Evaluator-->>Plugin: []Evidence
end
Plugin->>API: CreateEvidence(allEvidence)
API-->>Plugin: Success
Plugin-->>Client: EvalResponse(OK)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/ci.yml:
- Around line 1-12: Add an explicit least-privilege permissions block to the
workflow named "CI" by defining minimal workflow-level permissions (for example
read-only for contents) instead of relying on default GITHUB_TOKEN scopes;
update the top-level YAML (near the existing name/on/jobs keys) to include a
permissions mapping so the job "test" runs with only the necessary rights.
Ensure the permissions block is at workflow root and scoped conservatively
(e.g., contents: read) to follow least-privilege principles.
In `@examples/agent-config.yaml`:
- Around line 2-16: The current example can accidentally define two plugin
entries; update the snippet so users edit the same plugin block instead of
adding a second item: show a single plugin with name "plugin-aws-ecr" and
clearly indicate switching the source.type (and its associated keys: for local
use keep path, for OCI use image) within that same block, or alternatively
provide a complete second, self-contained example that includes the entire
plugin entry for the OCI variant so users won’t end up with two plugin items;
reference the "plugin-aws-ecr" block and the "source.type", "path", and "image"
fields when making the change.
In `@go.mod`:
- Line 3: Update the pinned Go toolchain and the golang.org/x/* module versions
in go.mod: change the go directive from "go 1.26.1" to "go 1.26.2" and bump the
module versions for golang.org/x/crypto, golang.org/x/net, and golang.org/x/sys
to patched minimums (at least v0.52.0 for x/crypto, v0.55.0 for x/net, and
v0.45.0 for x/sys); then run "go mod tidy" and "go mod vendor" (if vendoring)
and re-run tests to ensure compatibility, and if you choose to defer upgrading,
add a documented risk acceptance note referencing govulncheck/go vuln output in
the repository.
In `@internal/config_test.go`:
- Around line 48-52: The test currently indexes cfg.Accounts without checking
its length which can panic; before the loop over want, add a length assertion
(e.g., if got, want := len(cfg.Accounts), len(want); if got != want {
t.Fatalf("accounts: want %d entries, got %d", want, got) }) or a bound check
ensuring len(cfg.Accounts) >= len(want) so the subsequent loop that compares
cfg.Accounts[i] to want[i] cannot panic; update the test around the want :=
[]string... and loop to perform this guard using t.Fatalf/t.Errorf as
appropriate.
In `@internal/data_test.go`:
- Around line 266-314: Modify the test(s) (e.g., TestFetchImages_WithinLookback
and the similar case around lines 447-470) so DescribeImageScanFindings returns
ImageScanStatus.Status = COMPLETE but with
ImageScanFindings.FindingSeverityCounts = nil; then assert that the resulting
image record (from f.FetchImages) has HasSeverityData == false and that
FindingsHigh/FindingsCritical remain zero (or unchanged) rather than being
treated as populated. Update the mock.describeImageScanFindings closure to omit
FindingSeverityCounts and add the explicit assertions against
img.HasSeverityData, img.FindingsHigh, and img.FindingsCritical after calling
FetchImages.
In `@internal/data.go`:
- Around line 362-366: The code currently sets imgCtx.HasSeverityData=true
whenever findingsOut.ImageScanFindings is non-nil and imgCtx.ScanStatus ==
"COMPLETE", even if FindingSeverityCounts is nil; update the condition to
require findingsOut.ImageScanFindings.FindingSeverityCounts != nil before
assigning counts and setting HasSeverityData. Concretely, in the block
referencing findingsOut.ImageScanFindings, imgCtx.ScanStatus, and
FindingSeverityCounts, only read counts and set imgCtx.FindingsCritical,
imgCtx.FindingsHigh and imgCtx.HasSeverityData when FindingSeverityCounts is
non-nil (and safely handle missing keys if needed).
In `@internal/eval.go`:
- Around line 225-231: The emitted evidence metadata currently sets
proto.Link{Href: "https://github.com/container-solutions/plugin-aws-ecr", ...}
which points to the old repo; update the Href in that proto.Link to
"https://github.com/compliance-framework/plugin-aws-ecr" so the evidence Links
field in the code that constructs the metadata (see the struct initialization
with Title, Type, Links and the proto.Link literal) references the correct
repository.
- Around line 127-132: inventoryID is not globally unique because it omits
image.Region and uses a truncated digest; change the construction of inventoryID
(where componentID, digestShort, and inventoryID are defined) to include
image.Region and the full image.ImageDigest (keep digestShort only for titles if
desired) so the identifier becomes e.g.
"aws-ecr-image/{AccountID}/{Region}/{RepositoryName}/{FullDigest}" ensuring you
update the same pattern in the other block referenced (the later block that also
computes inventoryID).
In `@Makefile`:
- Around line 17-18: The run target currently assumes ../agent/dist/concom and
./.config/config.yaml exist; add explicit preflight checks in the run recipe to
test that ../agent/dist/concom exists and is executable and that
./.config/config.yaml exists (and is readable), printing clear, actionable error
messages and exiting non-zero if either check fails before invoking
../agent/dist/./concom agent --config ./.config/config.yaml; reference the
Makefile target name run and the two paths so reviewers can locate and verify
the checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7e7c9a5b-f599-45dd-a11f-ed1177ec96af
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (14)
.github/workflows/ci.yml.gitignore.goreleaser.yamlMakefileREADME.mdexamples/agent-config.yamlgo.modinternal/config.gointernal/config_test.gointernal/data.gointernal/data_test.gointernal/eval.gointernal/util.gomain.go
- ci.yml: add workflow-level permissions: contents: read - Makefile: preflight checks on run target for agent binary and config - go.mod: bump go directive to 1.26.3; x/crypto→0.52.0, x/net→0.55.0, x/sys→0.45.0 - config_test.go: guard account-slice length before index loop - data.go: only set HasSeverityData when FindingSeverityCounts is non-nil - data_test.go: add TestFetchImages_ScanCompleteNilCounts - eval.go: fix repo URL to compliance-framework org; include region + full digest in image inventoryID - examples/agent-config.yaml: single plugin block with inline source.type comment Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.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.
Summary
plugin-aws-ecr, a CCF compliance plugin for AWS Elastic Container RegistryChecks implemented
CONFIG (per repository)
ecr_require_scan_on_push—imageScanningConfiguration.scanOnPushmust be trueecr_require_tag_immutability—imageTagImmutabilitymust be IMMUTABLEecr_require_encryption— encryption type must be in approved listecr_require_lifecycle_policy— lifecycle policy must be configuredecr_deny_public_access— resource policy must not grantPrincipal: "*"Allowecr_require_tags— required tag keys must be presentecr_require_registry_scanning— account-level scan mode must be in approved listDYNAMIC (per image digest, 90-day window)
ecr_require_image_scan_complete— scan status must be COMPLETEecr_require_no_critical_image_findings— CRITICAL finding count must be 0ecr_require_no_high_image_findings— HIGH finding count must be ≤ thresholdecr_require_scan_findings_retrievable— scan findings with severity data must be accessibleTest plan
make testpasses (Go unit tests)make buildproduces a valid binarylocal-dev/envs/aws-ecragainst a dev AWS accountlocal-dev/e2e/ecr-repository— see that repo for full instructions🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation
Tests
Chores