From 0a60545f7e3f3e107e18b6e8b196471bc51c4607 Mon Sep 17 00:00:00 2001 From: James Salt Date: Thu, 4 Jun 2026 15:06:30 +0100 Subject: [PATCH 1/2] BCH-1296: Address PR review comments on plugin-aws-ecr (004) - Risk templates: each policy now defines a risk_templates Rego rule so the policy manager can upsert risks from policy metadata via InitWithSubjectsAndRisksFromPolicies - Release workflow: add .github/workflows/release.yml triggered on v* tags; runs GoReleaser and pushes the OCI binary image via gooci - Policy scoping: Eval uses WithDefaultPolicyBehavior + PolicyPathsForBehavior to route CONFIG policies to repository/registry evaluators and DYNAMIC policies to the image evaluator; single-bundle deployments continue to work unchanged while operators can scope by supplying two named bundles - FetchRegistryConfig error handling: error is now accumulated in evalErrors instead of silently logged, so the agent run status reflects the failure - Account filtering: FilterByAccounts helper added; repos are filtered to l.config.Accounts when the list is non-empty Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/release.yml | 36 +++++++++++++++++++++++++++++++++++ internal/config_test.go | 32 +++++++++++++++++++++++++++++++ internal/util.go | 19 ++++++++++++++++++ main.go | 32 +++++++++++++++++++++++++++---- 4 files changed, 115 insertions(+), 4 deletions(-) create mode 100644 .github/workflows/release.yml diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml new file mode 100644 index 0000000..368b202 --- /dev/null +++ b/.github/workflows/release.yml @@ -0,0 +1,36 @@ +name: New Release + +on: + push: + tags: + - 'v*' + +jobs: + release: + runs-on: ubuntu-latest + permissions: + packages: write + contents: write + steps: + - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 + with: + persist-credentials: false + - uses: actions/setup-go@v5 + with: + go-version-file: go.mod + - name: Run GoReleaser + uses: goreleaser/goreleaser-action@v6 + with: + version: '~> v2' + args: release --clean + env: + GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} + - name: Install gooci cli + run: go install github.com/compliance-framework/gooci@latest + - name: Authenticate gooci cli + run: gooci login ghcr.io --username ${{ github.actor }} --password ${{ secrets.GITHUB_TOKEN }} + - name: gooci Upload Version + run: gooci upload dist/ ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}:${{ github.ref_name }} + - name: gooci Upload Latest + if: "!github.event.release.prerelease" + run: gooci upload dist/ ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}:latest diff --git a/internal/config_test.go b/internal/config_test.go index 30926ae..26ad17d 100644 --- a/internal/config_test.go +++ b/internal/config_test.go @@ -4,6 +4,38 @@ import ( "testing" ) +func TestFilterByAccounts(t *testing.T) { + repos := []RepositoryContext{ + {AccountID: "111111111111", RepositoryName: "repo-a"}, + {AccountID: "222222222222", RepositoryName: "repo-b"}, + {AccountID: "333333333333", RepositoryName: "repo-c"}, + } + + t.Run("empty accounts returns all", func(t *testing.T) { + got := FilterByAccounts(repos, nil) + if len(got) != 3 { + t.Fatalf("want 3, got %d", len(got)) + } + }) + + t.Run("filters to matching accounts", func(t *testing.T) { + got := FilterByAccounts(repos, []string{"111111111111", "333333333333"}) + if len(got) != 2 { + t.Fatalf("want 2, got %d", len(got)) + } + if got[0].AccountID != "111111111111" || got[1].AccountID != "333333333333" { + t.Errorf("unexpected accounts: %v", got) + } + }) + + t.Run("no match returns empty", func(t *testing.T) { + got := FilterByAccounts(repos, []string{"999999999999"}) + if len(got) != 0 { + t.Fatalf("want 0, got %d", len(got)) + } + }) +} + func TestArnPartition(t *testing.T) { cases := []struct { region string diff --git a/internal/util.go b/internal/util.go index 86bd6a6..09d89eb 100644 --- a/internal/util.go +++ b/internal/util.go @@ -6,6 +6,25 @@ func StringAddressed(str string) *string { return &str } +// FilterByAccounts returns only the repositories whose AccountID appears in +// accounts. If accounts is empty the full list is returned unchanged. +func FilterByAccounts(repos []RepositoryContext, accounts []string) []RepositoryContext { + if len(accounts) == 0 { + return repos + } + allowed := make(map[string]struct{}, len(accounts)) + for _, a := range accounts { + allowed[a] = struct{}{} + } + out := repos[:0] + for _, r := range repos { + if _, ok := allowed[r.AccountID]; ok { + out = append(out, r) + } + } + return out +} + // arnPartition returns the AWS partition for the given region. // cn-* regions use aws-cn; us-gov-* regions use aws-us-gov; everything else uses aws. func arnPartition(region string) string { diff --git a/main.go b/main.go index dc69838..2cc4a27 100644 --- a/main.go +++ b/main.go @@ -99,6 +99,26 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api } } + // Scope policy paths to each resource type using behavior mapping. + // Default: a bundle named "*ecr-repository-policies*" covers repository/registry checks; + // one named "*ecr-image-policies*" covers image checks. A single bundle containing all + // policies (e.g. plugin-aws-ecr-policies) is mapped to all three behaviors so it + // continues to work unchanged; operators may override by supplying two separate bundles + // and configuring PolicyBehavior in the agent config. + defaultBehaviorMapping := map[string][]string{ + "ecr-repository-policies": {"repository", "registry"}, + "ecr-image-policies": {"image"}, + // Single all-in-one bundle: cover every behavior so it works out of the box. + "plugin-aws-ecr-policies": {"repository", "registry", "image"}, + } + policyEval := request. + WithDefaultPolicyBehavior(defaultBehaviorMapping). + WithUndefinedMappedTo([]string{"repository", "registry"}) + + repositoryPaths := policyEval.PolicyPathsForBehavior("repository") + registryPaths := policyEval.PolicyPathsForBehavior("registry") + imagePaths := policyEval.PolicyPathsForBehavior("image") + dataFetcher := internal.NewDataFetcher(l.logger, l.config) policyEvaluator := internal.NewPolicyEvaluator(ctx, l.logger, activities) @@ -111,8 +131,12 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api if err != nil { return &proto.EvalResponse{Status: proto.ExecutionStatus_FAILURE}, fmt.Errorf("region %s: fetching repositories: %w", region, err) } + + // Filter to configured accounts if any are specified. + repos = internal.FilterByAccounts(repos, l.config.Accounts) + for _, repo := range repos { - evidences, err := policyEvaluator.EvalRepository(ctx, repo, request.GetPolicyPaths(), l.policyData, l.config.PolicyLabels) + evidences, err := policyEvaluator.EvalRepository(ctx, repo, repositoryPaths, l.policyData, l.config.PolicyLabels) allEvidences = append(allEvidences, evidences...) if err != nil { evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating repository %s: %w", repo.RepositoryName, err)) @@ -122,9 +146,9 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api // CONFIG — registry scanning check (one per region) registry, err := dataFetcher.FetchRegistryConfig(ctx, region) if err != nil { - l.logger.Warn("failed to fetch registry scanning config", "region", region, "error", err) + evalErrors = errors.Join(evalErrors, fmt.Errorf("region %s: fetching registry scanning config: %w", region, err)) } else { - evidences, err := policyEvaluator.EvalRegistry(ctx, registry, request.GetPolicyPaths(), l.policyData, l.config.PolicyLabels) + evidences, err := policyEvaluator.EvalRegistry(ctx, registry, registryPaths, l.policyData, l.config.PolicyLabels) allEvidences = append(allEvidences, evidences...) if err != nil { evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating registry %s/%s: %w", registry.AccountID, region, err)) @@ -144,7 +168,7 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api return &proto.EvalResponse{Status: proto.ExecutionStatus_FAILURE}, fmt.Errorf("region %s: fetching images: %w", region, err) } for _, image := range images { - evidences, err := policyEvaluator.EvalImage(ctx, image, request.GetPolicyPaths(), l.policyData, l.config.PolicyLabels) + evidences, err := policyEvaluator.EvalImage(ctx, image, imagePaths, l.policyData, l.config.PolicyLabels) allEvidences = append(allEvidences, evidences...) if err != nil { evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating image %s@%s: %w", image.RepositoryName, image.ImageDigest, err)) From d80a8bcaf9e2939255e02985a125f58e05f1ebc5 Mon Sep 17 00:00:00 2001 From: James Salt Date: Thu, 4 Jun 2026 16:46:23 +0100 Subject: [PATCH 2/2] BCH-1296: Address PR review comments on plugin-aws-ecr (006) release.yml: - Pin actions/setup-go to 40f1582b (v5 SHA) - Pin goreleaser/goreleaser-action to e435ccd7 (v6 SHA) - Pin gooci to v0.0.7 instead of @latest - Fix prerelease guard: use !contains(github.ref_name, '-') instead of !github.event.release.prerelease (which is always empty on tag-push events) internal/util.go: - FilterByAccounts: use make([]RepositoryContext, 0, len(repos)) instead of repos[:0] so the function never aliases or mutates the caller-owned slice internal/config_test.go: - Per-subtest copy of the input repos slice so subtests are independent - Post-call assertion that the original slice was not mutated main.go: - Guard FetchRegistryConfig/EvalRegistry behind len(registryPaths) > 0 so registry config is not fetched during image-only runs - Guard image fetching behind len(imagePaths) > 0 so image APIs are not called during config-only runs - Group repos by AccountID and call FetchImages per-account so each repo uses its own AccountID instead of always using repos[0].AccountID Co-Authored-By: Claude Sonnet 4.6 --- .github/workflows/release.yml | 8 +++--- internal/config_test.go | 27 ++++++++++++++++++- internal/util.go | 2 +- main.go | 51 +++++++++++++++++++---------------- 4 files changed, 59 insertions(+), 29 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 368b202..a84b419 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -15,22 +15,22 @@ jobs: - uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683 with: persist-credentials: false - - uses: actions/setup-go@v5 + - uses: actions/setup-go@40f1582b2485089dde7abd97c1529aa768e1baff with: go-version-file: go.mod - name: Run GoReleaser - uses: goreleaser/goreleaser-action@v6 + uses: goreleaser/goreleaser-action@e435ccd777264be153ace6237001ef4d979d3a7a with: version: '~> v2' args: release --clean env: GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }} - name: Install gooci cli - run: go install github.com/compliance-framework/gooci@latest + run: go install github.com/compliance-framework/gooci@v0.0.7 - name: Authenticate gooci cli run: gooci login ghcr.io --username ${{ github.actor }} --password ${{ secrets.GITHUB_TOKEN }} - name: gooci Upload Version run: gooci upload dist/ ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}:${{ github.ref_name }} - name: gooci Upload Latest - if: "!github.event.release.prerelease" + if: "!contains(github.ref_name, '-')" run: gooci upload dist/ ghcr.io/${{ github.repository_owner }}/${{ github.event.repository.name }}:latest diff --git a/internal/config_test.go b/internal/config_test.go index 26ad17d..1f669d8 100644 --- a/internal/config_test.go +++ b/internal/config_test.go @@ -4,21 +4,43 @@ import ( "testing" ) +func copyRepos(src []RepositoryContext) []RepositoryContext { + dst := make([]RepositoryContext, len(src)) + copy(dst, src) + return dst +} + +func assertReposUnchanged(t *testing.T, original, after []RepositoryContext) { + t.Helper() + if len(after) != len(original) { + t.Errorf("input slice length changed: want %d, got %d", len(original), len(after)) + return + } + for i := range original { + if original[i].AccountID != after[i].AccountID || original[i].RepositoryName != after[i].RepositoryName { + t.Errorf("input slice mutated at index %d: want %+v, got %+v", i, original[i], after[i]) + } + } +} + func TestFilterByAccounts(t *testing.T) { - repos := []RepositoryContext{ + seed := []RepositoryContext{ {AccountID: "111111111111", RepositoryName: "repo-a"}, {AccountID: "222222222222", RepositoryName: "repo-b"}, {AccountID: "333333333333", RepositoryName: "repo-c"}, } t.Run("empty accounts returns all", func(t *testing.T) { + repos := copyRepos(seed) got := FilterByAccounts(repos, nil) if len(got) != 3 { t.Fatalf("want 3, got %d", len(got)) } + assertReposUnchanged(t, seed, repos) }) t.Run("filters to matching accounts", func(t *testing.T) { + repos := copyRepos(seed) got := FilterByAccounts(repos, []string{"111111111111", "333333333333"}) if len(got) != 2 { t.Fatalf("want 2, got %d", len(got)) @@ -26,13 +48,16 @@ func TestFilterByAccounts(t *testing.T) { if got[0].AccountID != "111111111111" || got[1].AccountID != "333333333333" { t.Errorf("unexpected accounts: %v", got) } + assertReposUnchanged(t, seed, repos) }) t.Run("no match returns empty", func(t *testing.T) { + repos := copyRepos(seed) got := FilterByAccounts(repos, []string{"999999999999"}) if len(got) != 0 { t.Fatalf("want 0, got %d", len(got)) } + assertReposUnchanged(t, seed, repos) }) } diff --git a/internal/util.go b/internal/util.go index 09d89eb..80c9c02 100644 --- a/internal/util.go +++ b/internal/util.go @@ -16,7 +16,7 @@ func FilterByAccounts(repos []RepositoryContext, accounts []string) []Repository for _, a := range accounts { allowed[a] = struct{}{} } - out := repos[:0] + out := make([]RepositoryContext, 0, len(repos)) for _, r := range repos { if _, ok := allowed[r.AccountID]; ok { out = append(out, r) diff --git a/main.go b/main.go index 2cc4a27..3fcce6c 100644 --- a/main.go +++ b/main.go @@ -143,35 +143,40 @@ func (l *CompliancePlugin) Eval(request *proto.EvalRequest, apiHelper runner.Api } } - // CONFIG — registry scanning check (one per region) - registry, err := dataFetcher.FetchRegistryConfig(ctx, region) - if err != nil { - evalErrors = errors.Join(evalErrors, fmt.Errorf("region %s: fetching registry scanning config: %w", region, err)) - } else { - evidences, err := policyEvaluator.EvalRegistry(ctx, registry, registryPaths, l.policyData, l.config.PolicyLabels) - allEvidences = append(allEvidences, evidences...) + // CONFIG — registry scanning check (one per region, skipped for image-only runs) + if len(registryPaths) > 0 { + registry, err := dataFetcher.FetchRegistryConfig(ctx, region) if err != nil { - evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating registry %s/%s: %w", registry.AccountID, region, err)) + evalErrors = errors.Join(evalErrors, fmt.Errorf("region %s: fetching registry scanning config: %w", region, err)) + } else { + evidences, err := policyEvaluator.EvalRegistry(ctx, registry, registryPaths, l.policyData, l.config.PolicyLabels) + allEvidences = append(allEvidences, evidences...) + if err != nil { + evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating registry %s/%s: %w", registry.AccountID, region, err)) + } } } - // DYNAMIC — image scan checks - if len(repos) > 0 { - repoNames := make([]string, len(repos)) - for i, r := range repos { - repoNames[i] = r.RepositoryName + // DYNAMIC — image scan checks (skipped for config-only runs) + if len(imagePaths) > 0 && len(repos) > 0 { + // Group repos by account so each account's images are fetched with the + // correct accountID — using repos[0].AccountID for all repos is wrong + // when multiple accounts are configured. + reposByAccount := make(map[string][]string) + for _, r := range repos { + reposByAccount[r.AccountID] = append(reposByAccount[r.AccountID], r.RepositoryName) } - accountID := repos[0].AccountID - - images, err := dataFetcher.FetchImages(ctx, region, repoNames, accountID, lookbackDays) - if err != nil { - return &proto.EvalResponse{Status: proto.ExecutionStatus_FAILURE}, fmt.Errorf("region %s: fetching images: %w", region, err) - } - for _, image := range images { - evidences, err := policyEvaluator.EvalImage(ctx, image, imagePaths, l.policyData, l.config.PolicyLabels) - allEvidences = append(allEvidences, evidences...) + for accID, names := range reposByAccount { + images, err := dataFetcher.FetchImages(ctx, region, names, accID, lookbackDays) if err != nil { - evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating image %s@%s: %w", image.RepositoryName, image.ImageDigest, err)) + return &proto.EvalResponse{Status: proto.ExecutionStatus_FAILURE}, fmt.Errorf("region %s account %s: fetching images: %w", region, accID, err) + } + for _, image := range images { + evidences, err := policyEvaluator.EvalImage(ctx, image, imagePaths, l.policyData, l.config.PolicyLabels) + allEvidences = append(allEvidences, evidences...) + if err != nil { + evalErrors = errors.Join(evalErrors, fmt.Errorf("evaluating image %s@%s: %w", image.RepositoryName, image.ImageDigest, err)) + } } } }