Skip to content

sign and detect tampering#809

Open
entlein wants to merge 2 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-tamper-detection
Open

sign and detect tampering#809
entlein wants to merge 2 commits into
kubescape:mainfrom
k8sstormcenter:upstream-pr/sbob-tamper-detection

Conversation

@entlein
Copy link
Copy Markdown

@entlein entlein commented May 15, 2026

This PR needs to be reviewed together with the original signature PR

It builds upon the previous PRs and adds

  • cmd/sign-object/: standalone binary an admin uses to sign a profile against a cosign key.

(This one has no sister PR in storage)

Signed-off-by: entlein <einentlein@gmail.com>
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 15, 2026

Warning

Rate limit exceeded

@entlein has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 59 minutes before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ab970d43-f79c-4d95-9d29-595986b45937

📥 Commits

Reviewing files that changed from the base of the PR and between cc59fa0 and 9ed25cf.

📒 Files selected for processing (25)
  • cmd/sign-object/Dockerfile
  • cmd/sign-object/main.go
  • pkg/exporters/alert_manager.go
  • pkg/objectcache/containerprofilecache/tamper_alert.go
  • pkg/objectcache/containerprofilecache/tamper_alert_test.go
  • pkg/signature/annotations.go
  • pkg/signature/cluster_flow_test.go
  • pkg/signature/cluster_scenario_test.go
  • pkg/signature/cosign_adapter.go
  • pkg/signature/cosign_adapter_test.go
  • pkg/signature/interface.go
  • pkg/signature/profiles/adapter_test.go
  • pkg/signature/profiles/applicationprofile_adapter.go
  • pkg/signature/profiles/empty_typemeta_test.go
  • pkg/signature/profiles/networkneighborhood_adapter.go
  • pkg/signature/profiles/networkneighborhood_adapter_test.go
  • pkg/signature/profiles/rules_adapter.go
  • pkg/signature/profiles/rules_adapter_test.go
  • pkg/signature/profiles/seccompprofile_adapter.go
  • pkg/signature/sign.go
  • pkg/signature/sign_test.go
  • pkg/signature/signer.go
  • pkg/signature/verifier.go
  • pkg/signature/verify.go
  • pkg/signature/verify_test.go
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

…state

Signed-off-by: entlein <einentlein@gmail.com>
@entlein
Copy link
Copy Markdown
Author

entlein commented May 16, 2026

@coderabbitai review

Collective re-review request after applying your atomic-split advisory + rabbit-feedback fixes across the full SBOB upstream PR set. Each PR is now scoped to a single tier per your dependency map:

Tier 0 (opens):

Tier 2 (execs):

Tier 3 (network):

Tier 4 (tamper/sign):

Umbrellas:

Please confirm atomicity per your earlier advisory and flag any remaining cross-PR coupling we missed.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 16, 2026

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Gather PR details for `#809` and related PRs to assess atomicity and coupling
gh pr view 809 --repo kubescape/node-agent --json title,body,files,labels,state,baseRefName,headRefName 2>/dev/null | jq '{title, state, baseRefName, headRefName, labels: [.labels[].name], files: [.files[].path]}'

echo "--- PR `#808` files ---"
gh pr view 808 --repo kubescape/node-agent --json title,files 2>/dev/null | jq '{title, files: [.files[].path]}'

echo "--- PR `#811` files ---"
gh pr view 811 --repo kubescape/node-agent --json title,files 2>/dev/null | jq '{title, files: [.files[].path]}'

echo "--- PR `#810` files ---"
gh pr view 810 --repo kubescape/node-agent --json title,files 2>/dev/null | jq '{title, files: [.files[].path]}'

Length of output: 7360


@entlein: Re-review triggered. ✅ Here is the atomicity assessment for PR #809 and the coupling analysis across the tier set.


Atomicity Assessment — PR #809

✅ Clean / Atomic surfaces

Surface Files Assessment
pkg/signature/** (21 files) cosign adapter, sign/verify, profile adapters, signer/verifier interfaces, cluster tests Exclusively in #809 — fully atomic
cmd/sign-object/ Dockerfile, main.go Exclusively in #809 — fully atomic, standalone binary
pkg/exporters/alert_manager.go alert routing for tamper events Exclusively in #809 — atomic

⚠️ Cross-PR Coupling Detected

Hard file overlap between #808 and #809:

File In #808 In #809
pkg/objectcache/containerprofilecache/tamper_alert.go
pkg/objectcache/containerprofilecache/tamper_alert_test.go

Both PRs are in Tier 4 and both modify the exact same two files. This creates:

  1. Hard merge-order dependency — whichever lands second must absorb the other's changes to these files without a clean rebase surface.
  2. Logical split ambiguity#808's scope is projection overlay + tamper infrastructure, while #809's is cosign sign/verify + tamper detection. The tamper_alert files bridge both concerns, but they can't atomically belong to two PRs simultaneously.

Recommendation: Decide ownership of tamper_alert.{go,_test.go}:

  • If tamper_alert.go contains only the structural tamper hook (non-signature logic), move it entirely into #808 and have #809 only call into it.
  • If it contains cosign-verification-driven alert logic, move it entirely into #809 and have #808 leave a stub or interface boundary.

Either way, only one PR should own the file; the other should depend on the merged result.


Tier Structure — Remaining Cross-PR Coupling Map

graph TD
    subgraph Tier0["Tier 0 (opens)"]
        S323["storage#323\npath-wildcards core"]
        NA811["node-agent#811\nwas_path_opened CEL\n✅ atomic (2 files, clean)"]
    end
    subgraph Tier2["Tier 2 (execs)"]
        S322["storage#322 CompareExecArgs"]
        S325["storage#325 CollapseConfig CRD"]
        NA805["node-agent#805 resolveExecPath"]
        NA807["node-agent#807 R0040 args"]
        NA812["node-agent#812 ExecsByPath"]
    end
    subgraph Tier3["Tier 3 (network)"]
        S324["storage#324 networkmatch"]
        NA806["node-agent#806 network CEL"]
    end
    subgraph Tier4["Tier 4 (tamper/sign)"]
        NA808["node-agent#808\nprojection + tamper infra"]
        NA809["node-agent#809\ncosign sign/verify\n⚠️ tamper_alert overlap"]
        NA808 -->|"tamper_alert.go\nOVERLAP"| NA809
    end
    subgraph Umbrella["Umbrellas"]
        S326["storage#326"]
        NA810["node-agent#810\nabsorbs all ✅"]
    end
    Tier0 --> Tier2
    Tier2 --> Tier4
    Tier3 --> Tier4
Loading

All other tier boundaries appear clean — no file overlaps detected outside the #808#809 pair. The umbrella PR #810 correctly absorbs all files from both.

✅ Actions performed

Full review triggered.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: No status

Development

Successfully merging this pull request may close these issues.

2 participants