refactor: unify example layout + redis-protocol distro comparison (redis/valkey/keydb/dragonfly) - #173
Merged
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis change relocates example resources under ChangesExample environments and validation tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Runner
participant Bobctl
participant Redis
participant Detection
Runner->>Bobctl: Run a Redis AttackSuite
Bobctl->>Redis: Send RESP commands or Lua EVAL payload
Redis-->>Bobctl: Return attack response
Bobctl->>Detection: Check expected detection rules
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
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. 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: 12
🤖 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-postgres-vuln-image.yaml:
- Around line 62-63: Update the Docker build context in both Buildx steps to use
the relocated example/postgres-vuln directory, matching the Dockerfile location,
while leaving the existing Dockerfile references and build configuration
unchanged.
In `@example/argocd-server-attacks.yaml`:
- Line 69: Update the Perl script in the command to exit with non-zero status if
the socket connection fails, and modify it to send the TOKEN in an authenticated
Kubernetes API request using the Authorization header instead of only
establishing a socket connection. Ensure the script exits with non-zero status
if either the socket creation or the authenticated API request fails, so that
the && operator correctly prevents echo k8s_pivot_done from executing unless
real authenticated API access succeeds.
In `@example/postgres/cluster.yaml`:
- Around line 19-44: Remove the manual pg_stat_statements CREATE EXTENSION
statement from postInitSQL, while retaining the postInitApplicationSQL setup and
existing PostgreSQL parameters. Do not alter extension management behavior
elsewhere.
In `@example/redis/Dockerfile.redis-vulnerable`:
- Around line 90-95: Replace the version-only check after Redis installation
with a builder-stage smoke test that briefly starts redis-server, executes EVAL
"return tostring(io)" 0, and verifies the result indicates a table before
stopping the server. Ensure the build fails when the sandbox-escape behavior is
not present, while cleaning up the temporary server process.
- Around line 79-88: Validate Patch 5 against the downloaded Redis source and
the Debian CVE-2022-0543 build mechanism, specifically checking whether
script_lua.c contains any lua_pushnil/lua_setglobal(..., "package") sequence.
Remove the misleading no-op sed patch if that pattern is absent, or replace it
with a targeted modification of the actual Debian Lua initialization source so
package remains accessible. Update the Patch 5 comment to accurately describe
the implemented behavior.
In `@example/redis/redis-tests/attack-01-fileless-memfd.yaml`:
- Around line 28-35: Replace the unconditional success prefixes with indicators
that prove each operation succeeded: in
example/redis/redis-tests/attack-01-fileless-memfd.yaml lines 28-35, validate
output from the memfd-backed executable; in
example/redis/redis-tests/attack-11-reverse-shell-perl.yaml lines 27-32, require
“connected” or another verified C2-success result instead of “revshell:”; and in
example/redis/redis-tests/e2e-01-data-exfiltration.yaml lines 168-174, require
confirmed DNS resolution instead of the unconditional “c2:” prefix.
In `@example/redis/redis-tests/e2e-01-data-exfiltration.yaml`:
- Around line 176-182: Isolate Redis state across the affected E2E suites: in
example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L176-L182, remove the
seeded user:42 and session:abc123 keys and explicitly define whether the
exfil-canary fixture is retained; in
example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L59-L64,
replace the initial FLUSHALL dependency with an isolated Redis instance; remove
the shared-database clearing commands before the SSH persistence phase at
`#L96-L99` and before the webshell persistence phase at `#L125-L128`.
In `@example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml`:
- Around line 35-71: Reorder the e2e monitor scenario so the sniff-monitor-start
session is established before any decoy AUTH or HSET traffic, then send those
decoys through a separate Redis connection. Assert that the monitor stream
captures both credential-bearing commands, and update sniff-cleanup to close the
monitor session after verification.
In `@example/redis/redis-tests/README.md`:
- Around line 8-24: Update the README directory layout entries for attack 11 and
attack 12 to use attack-11-reverse-shell-perl.yaml and
attack-12-credential-harvest.yaml, matching the shipped files and
DETECTION-RULES.md.
In `@example/redis/redis-tests/run-parallel.sh`:
- Around line 32-42: Update run_one() to capture bobctl attack run’s exit status
before the conditional reporting, and add an exit-code-2 branch that reports the
attack as SKIP, matching run-all.sh. Preserve the existing PASS output for
status 0 and FAIL output for all other nonzero statuses, including the captured
status in the failure message.
In `@scripts/compare-distros.py`:
- Around line 138-139: Update the main reporting flow in
scripts/compare-distros.py so that when args.json is set, stdout contains only
the json.dumps payload; suppress the human-readable report or redirect it to
stderr while preserving the existing JSON fields and formatting.
- Around line 34-59: Update load() to detect and skip incomplete metrics data:
return None with the existing warning behavior when entries is empty, when no
tuned entries exist (so raw-baseline records are not used as best), or when no
raw-baseline entry exists (so tuned data is not used for behavior metrics).
Preserve normal selection of the tuned best entry and raw-baseline behavior
entry when both are available.
🪄 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: f20f3b15-a06e-428f-b354-4ceb2558b21a
⛔ Files ignored due to path filters (2)
example/redis/distros/redis-distro-comparison.gifis excluded by!**/*.gifexample/redis/redis-killchain.gifis excluded by!**/*.gif
📒 Files selected for processing (49)
.github/workflows/ci-bobctl-autotune.yaml.github/workflows/ci-postgres-vuln-image.yamlMakefileREADME.mdexample/argocd-server-attacks.yamlexample/postgres-vuln/Dockerfileexample/postgres-vuln/cluster.yamlexample/postgres/cluster.yamlexample/redis/Dockerfile.redis-vulnerableexample/redis/client.yamlexample/redis/distros/VERDICT.txtexample/redis/distros/distro-attacks.yamlexample/redis/distros/distro-functional-tests.yamlexample/redis/distros/dragonfly.yamlexample/redis/distros/gen-distro-manifests.pyexample/redis/distros/keydb.yamlexample/redis/distros/redis-oss.yamlexample/redis/distros/valkey.yamlexample/redis/redis-load-stress.yamlexample/redis/redis-neighbor-attacks.yamlexample/redis/redis-tests/DETECTION-RULES.mdexample/redis/redis-tests/E2E-SCENARIOS.mdexample/redis/redis-tests/README.mdexample/redis/redis-tests/attack-01-fileless-memfd.yamlexample/redis/redis-tests/attack-02-sa-token-exfil.yamlexample/redis/redis-tests/attack-03-read-etc-shadow.yamlexample/redis/redis-tests/attack-04-unexpected-whoami.yamlexample/redis/redis-tests/attack-05-dns-anomaly.yamlexample/redis/redis-tests/attack-06-drifted-binary.yamlexample/redis/redis-tests/attack-07-exec-devshm.yamlexample/redis/redis-tests/attack-08-read-proc-environ.yamlexample/redis/redis-tests/attack-09-symlink-shadow.yamlexample/redis/redis-tests/attack-10-crypto-mining-dns.yamlexample/redis/redis-tests/attack-11-reverse-shell-perl.yamlexample/redis/redis-tests/attack-12-credential-harvest.yamlexample/redis/redis-tests/e2e-01-data-exfiltration.yamlexample/redis/redis-tests/e2e-02-slaveof-rogue-master.yamlexample/redis/redis-tests/e2e-03-config-save-cron-persistence.yamlexample/redis/redis-tests/e2e-04-monitor-credential-sniff.yamlexample/redis/redis-tests/run-all.shexample/redis/redis-tests/run-parallel.shexample/redis/redis-vulnerable.yamlexample/redis/redis.yamlexample/redis/sbobs/cp-redis.yamlkubescape/rule-coverage.yamlpkgscripts/compare-distros.pyscripts/local-ci-unit.shscripts/render-distro-poster.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 12
🤖 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-postgres-vuln-image.yaml:
- Around line 62-63: Update the Docker build context in both Buildx steps to use
the relocated example/postgres-vuln directory, matching the Dockerfile location,
while leaving the existing Dockerfile references and build configuration
unchanged.
In `@example/argocd-server-attacks.yaml`:
- Line 69: Update the Perl script in the command to exit with non-zero status if
the socket connection fails, and modify it to send the TOKEN in an authenticated
Kubernetes API request using the Authorization header instead of only
establishing a socket connection. Ensure the script exits with non-zero status
if either the socket creation or the authenticated API request fails, so that
the && operator correctly prevents echo k8s_pivot_done from executing unless
real authenticated API access succeeds.
In `@example/postgres/cluster.yaml`:
- Around line 19-44: Remove the manual pg_stat_statements CREATE EXTENSION
statement from postInitSQL, while retaining the postInitApplicationSQL setup and
existing PostgreSQL parameters. Do not alter extension management behavior
elsewhere.
In `@example/redis/Dockerfile.redis-vulnerable`:
- Around line 90-95: Replace the version-only check after Redis installation
with a builder-stage smoke test that briefly starts redis-server, executes EVAL
"return tostring(io)" 0, and verifies the result indicates a table before
stopping the server. Ensure the build fails when the sandbox-escape behavior is
not present, while cleaning up the temporary server process.
- Around line 79-88: Validate Patch 5 against the downloaded Redis source and
the Debian CVE-2022-0543 build mechanism, specifically checking whether
script_lua.c contains any lua_pushnil/lua_setglobal(..., "package") sequence.
Remove the misleading no-op sed patch if that pattern is absent, or replace it
with a targeted modification of the actual Debian Lua initialization source so
package remains accessible. Update the Patch 5 comment to accurately describe
the implemented behavior.
In `@example/redis/redis-tests/attack-01-fileless-memfd.yaml`:
- Around line 28-35: Replace the unconditional success prefixes with indicators
that prove each operation succeeded: in
example/redis/redis-tests/attack-01-fileless-memfd.yaml lines 28-35, validate
output from the memfd-backed executable; in
example/redis/redis-tests/attack-11-reverse-shell-perl.yaml lines 27-32, require
“connected” or another verified C2-success result instead of “revshell:”; and in
example/redis/redis-tests/e2e-01-data-exfiltration.yaml lines 168-174, require
confirmed DNS resolution instead of the unconditional “c2:” prefix.
In `@example/redis/redis-tests/e2e-01-data-exfiltration.yaml`:
- Around line 176-182: Isolate Redis state across the affected E2E suites: in
example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L176-L182, remove the
seeded user:42 and session:abc123 keys and explicitly define whether the
exfil-canary fixture is retained; in
example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L59-L64,
replace the initial FLUSHALL dependency with an isolated Redis instance; remove
the shared-database clearing commands before the SSH persistence phase at
`#L96-L99` and before the webshell persistence phase at `#L125-L128`.
In `@example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml`:
- Around line 35-71: Reorder the e2e monitor scenario so the sniff-monitor-start
session is established before any decoy AUTH or HSET traffic, then send those
decoys through a separate Redis connection. Assert that the monitor stream
captures both credential-bearing commands, and update sniff-cleanup to close the
monitor session after verification.
In `@example/redis/redis-tests/README.md`:
- Around line 8-24: Update the README directory layout entries for attack 11 and
attack 12 to use attack-11-reverse-shell-perl.yaml and
attack-12-credential-harvest.yaml, matching the shipped files and
DETECTION-RULES.md.
In `@example/redis/redis-tests/run-parallel.sh`:
- Around line 32-42: Update run_one() to capture bobctl attack run’s exit status
before the conditional reporting, and add an exit-code-2 branch that reports the
attack as SKIP, matching run-all.sh. Preserve the existing PASS output for
status 0 and FAIL output for all other nonzero statuses, including the captured
status in the failure message.
In `@scripts/compare-distros.py`:
- Around line 138-139: Update the main reporting flow in
scripts/compare-distros.py so that when args.json is set, stdout contains only
the json.dumps payload; suppress the human-readable report or redirect it to
stderr while preserving the existing JSON fields and formatting.
- Around line 34-59: Update load() to detect and skip incomplete metrics data:
return None with the existing warning behavior when entries is empty, when no
tuned entries exist (so raw-baseline records are not used as best), or when no
raw-baseline entry exists (so tuned data is not used for behavior metrics).
Preserve normal selection of the tuned best entry and raw-baseline behavior
entry when both are available.
🪄 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: f20f3b15-a06e-428f-b354-4ceb2558b21a
⛔ Files ignored due to path filters (2)
example/redis/distros/redis-distro-comparison.gifis excluded by!**/*.gifexample/redis/redis-killchain.gifis excluded by!**/*.gif
📒 Files selected for processing (49)
.github/workflows/ci-bobctl-autotune.yaml.github/workflows/ci-postgres-vuln-image.yamlMakefileREADME.mdexample/argocd-server-attacks.yamlexample/postgres-vuln/Dockerfileexample/postgres-vuln/cluster.yamlexample/postgres/cluster.yamlexample/redis/Dockerfile.redis-vulnerableexample/redis/client.yamlexample/redis/distros/VERDICT.txtexample/redis/distros/distro-attacks.yamlexample/redis/distros/distro-functional-tests.yamlexample/redis/distros/dragonfly.yamlexample/redis/distros/gen-distro-manifests.pyexample/redis/distros/keydb.yamlexample/redis/distros/redis-oss.yamlexample/redis/distros/valkey.yamlexample/redis/redis-load-stress.yamlexample/redis/redis-neighbor-attacks.yamlexample/redis/redis-tests/DETECTION-RULES.mdexample/redis/redis-tests/E2E-SCENARIOS.mdexample/redis/redis-tests/README.mdexample/redis/redis-tests/attack-01-fileless-memfd.yamlexample/redis/redis-tests/attack-02-sa-token-exfil.yamlexample/redis/redis-tests/attack-03-read-etc-shadow.yamlexample/redis/redis-tests/attack-04-unexpected-whoami.yamlexample/redis/redis-tests/attack-05-dns-anomaly.yamlexample/redis/redis-tests/attack-06-drifted-binary.yamlexample/redis/redis-tests/attack-07-exec-devshm.yamlexample/redis/redis-tests/attack-08-read-proc-environ.yamlexample/redis/redis-tests/attack-09-symlink-shadow.yamlexample/redis/redis-tests/attack-10-crypto-mining-dns.yamlexample/redis/redis-tests/attack-11-reverse-shell-perl.yamlexample/redis/redis-tests/attack-12-credential-harvest.yamlexample/redis/redis-tests/e2e-01-data-exfiltration.yamlexample/redis/redis-tests/e2e-02-slaveof-rogue-master.yamlexample/redis/redis-tests/e2e-03-config-save-cron-persistence.yamlexample/redis/redis-tests/e2e-04-monitor-credential-sniff.yamlexample/redis/redis-tests/run-all.shexample/redis/redis-tests/run-parallel.shexample/redis/redis-vulnerable.yamlexample/redis/redis.yamlexample/redis/sbobs/cp-redis.yamlkubescape/rule-coverage.yamlpkgscripts/compare-distros.pyscripts/local-ci-unit.shscripts/render-distro-poster.py
🛑 Comments failed to post (8)
example/postgres/cluster.yaml (1)
19-44: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Find the repository-declared CloudNativePG version and inspect this manifest. rg -n -i --hidden \ -g '!*vendor*' -g '!*.lock' \ 'cloudnative-pg|cloudnativepg|cnpg' . rg -n -C3 \ 'postInitSQL|postInitApplicationSQL|pg_stat_statements|shared_preload_libraries' \ example/postgres/cluster.yamlRepository: k8sstormcenter/bob
Length of output: 1255
🌐 Web query:
CloudNativePG postgresql.conf pg_stat_statements track max shared_preload_libraries documentation💡 Result:
In CloudNativePG, you enable the pg_stat_statements extension by configuring its specific parameters within the Cluster resource. When you define parameters starting with pg_stat_statements. (such as pg_stat_statements.max or pg_stat_statements.track), the CloudNativePG operator automatically performs two actions: it adds pg_stat_statements to the shared_preload_libraries configuration and executes the SQL command CREATE EXTENSION IF NOT EXISTS pg_stat_statements on your databases [1][2]. Example configuration excerpt for your Cluster resource: postgresql: parameters: pg_stat_statements.max: "10000" pg_stat_statements.track: all Key technical considerations: Automatic Management: The operator manages shared_preload_libraries automatically for several well-known extensions, including pg_stat_statements [2]. As soon as it detects a parameter requiring a managed library, it adds the library; if no parameters require it, it removes it [2]. Manual Overrides: While the operator handles managed extensions automatically, you can still provide additional libraries manually via the.spec.postgresql.shared_preload_libraries field in your Cluster resource. The operator merges these with any automatically managed libraries [2]. Limitations: Note that this automatic management is tied to the Cluster resource configuration. Adding extensions via the Database CRD does not automatically update shared_preload_libraries, which can lead to errors if the extension requires it but the corresponding parameter is not set in the Cluster spec [3]. Recommendation: Avoid changing extension images and PostgreSQL configuration (including shared_preload_libraries) at the same time; perform these as separate steps to ensure stability [3].
Citations:
- 1: https://cloudnative-pg.github.io/docs/1.25/postgresql_conf
- 2: https://cloudnative-pg.io/docs/1.28/postgresql_conf/
- 3: cloudnative-pg/cloudnative-pg#9930
🏁 Script executed:
#!/bin/bash set -euo pipefail sed -n '1,90p' example/postgres/cluster.yamlRepository: k8sstormcenter/bob
Length of output: 2386
Remove the manual
pg_stat_statementsextension creation.CloudNativePG creates
pg_stat_statementsautomatically when thepostgresql.parameterssection includes apg_stat_statements.*parameter, and at the same time adds it toshared_preload_libraries. Keep the extension creation here while using extension management.🤖 Prompt for 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. In `@example/postgres/cluster.yaml` around lines 19 - 44, Remove the manual pg_stat_statements CREATE EXTENSION statement from postInitSQL, while retaining the postInitApplicationSQL setup and existing PostgreSQL parameters. Do not alter extension management behavior elsewhere.example/redis/Dockerfile.redis-vulnerable (2)
79-88: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Patch 5's sed pattern likely never matches; the code path it targets does not exist upstream.
The comment states this removes a
lua_pushnil/lua_setglobal(..., "package")pair that "some Redis versions" use to nil outpackage. Public research on CVE-2022-0543 shows the actual nil-out happens only in Debian's packaging build script via a Lua source string, not in upstreamscript_lua.c: "The first two are cleared out in the vulnerable version, but the latter wasn't. So, the fix was, you guessed it, to add package=nil to the end of the Lua initialization." The mechanism is "luaL_dostring(lua, "module = nil; require = nil; package = nil");", embedded in Debian's rules file, not a C-levellua_pushnil/lua_setglobalcall pair in the vanilla GitHub source this Dockerfile downloads. Also independent of that: even if such a pair existed on two separate lines, the sed range/"package"/{ /lua_pushnil/,/lua_setglobal/d }only evaluates the inner range on lines that already match the outer"package"address, so alua_pushnil(lua);line without the literal string"package"would never open the range. Given thefind ... || truefallback, this patch most likely silently does nothing today. It does not appear to break the escape (the condition it targets likely never existed in the source being patched), but the comment is misleading about what it accomplishes.Since this touches an external CVE reproduction mechanism, please confirm with a web search whether upstream Redis'
script_lua.cfor the Debian-vs-vanilla split contains anylua_pushnil/lua_setglobal(..., "package")pattern at all.🧰 Tools
🪛 Hadolint (2.14.0)
[warning] 86-86: Use 'find .. -print0 | xargs -0 ..' or 'find .. -exec .. +' to allow non-alphanumeric filenames.
(SC2038)
[warning] 86-86: Set the SHELL option -o pipefail before RUN with a pipe in it. If you are using /bin/sh in an alpine image or if your shell is symlinked to busybox then consider explicitly setting your SHELL to /bin/ash, or disable this check
(DL4006)
🤖 Prompt for 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. In `@example/redis/Dockerfile.redis-vulnerable` around lines 79 - 88, Validate Patch 5 against the downloaded Redis source and the Debian CVE-2022-0543 build mechanism, specifically checking whether script_lua.c contains any lua_pushnil/lua_setglobal(..., "package") sequence. Remove the misleading no-op sed patch if that pattern is absent, or replace it with a targeted modification of the actual Debian Lua initialization source so package remains accessible. Update the Patch 5 comment to accurately describe the implemented behavior.
90-95: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Consider verifying the actual sandbox escape at build time, not just the binary version.
The only build-time check is
redis-server --version. Given the fragility of the preceding sed-based patches, add a smoke test in the builder stage that startsredis-serverbriefly and confirmsEVAL "return tostring(io)" 0returns a table, so a future upstream source change that silently breaks one of the patches fails the build instead of shipping a non-functional lab image.🧰 Tools
🪛 Hadolint (2.14.0)
[info] 95-95: Multiple consecutive
RUNinstructions. Consider consolidation.(DL3059)
🤖 Prompt for 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. In `@example/redis/Dockerfile.redis-vulnerable` around lines 90 - 95, Replace the version-only check after Redis installation with a builder-stage smoke test that briefly starts redis-server, executes EVAL "return tostring(io)" 0, and verifies the result indicates a table before stopping the server. Ensure the build fails when the sandbox-escape behavior is not present, while cleaning up the temporary server process.example/redis/redis-tests/attack-01-fileless-memfd.yaml (1)
28-35: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Require evidence of the advertised attack result.
Each indicator matches a prefix that the payload adds after
io_mod.popen()starts. The suite can pass when the memfd execution fails, when the Perl socket cannot connect, or when DNS lookup fails. This records failed attack paths as successful results.
example/redis/redis-tests/attack-01-fileless-memfd.yaml#L28-L35: require output that proves the memfd-backed executable ran.example/redis/redis-tests/attack-11-reverse-shell-perl.yaml#L27-L32: requireconnected, or another verified C2-success result, instead ofrevshell:.example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L168-L174: require a successful DNS-resolution result instead of the unconditionalc2:prefix.📍 Affects 3 files
example/redis/redis-tests/attack-01-fileless-memfd.yaml#L28-L35(this comment)example/redis/redis-tests/attack-11-reverse-shell-perl.yaml#L27-L32example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L168-L174🤖 Prompt for 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. In `@example/redis/redis-tests/attack-01-fileless-memfd.yaml` around lines 28 - 35, Replace the unconditional success prefixes with indicators that prove each operation succeeded: in example/redis/redis-tests/attack-01-fileless-memfd.yaml lines 28-35, validate output from the memfd-backed executable; in example/redis/redis-tests/attack-11-reverse-shell-perl.yaml lines 27-32, require “connected” or another verified C2-success result instead of “revshell:”; and in example/redis/redis-tests/e2e-01-data-exfiltration.yaml lines 168-174, require confirmed DNS resolution instead of the unconditional “c2:” prefix.example/redis/redis-tests/e2e-01-data-exfiltration.yaml (1)
176-182: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Isolate Redis state for each suite.
All sites target the same
redisservice. E2E-01 leaves seeded keys. E2E-03 deletes the complete database three times. This changes later test results and destroys unrelated data when suites share a target or run in parallel.
example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L176-L182: removeuser:42andsession:abc123, and define whether the canary is retained as an explicit fixture.example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L59-L64: remove the first FLUSHALL dependency by using an isolated Redis instance.example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L96-L99: do not clear the shared database before the SSH persistence phase.example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L125-L128: do not clear the shared database before the webshell persistence phase.📍 Affects 2 files
example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L176-L182(this comment)example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L59-L64example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L96-L99example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L125-L128🤖 Prompt for 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. In `@example/redis/redis-tests/e2e-01-data-exfiltration.yaml` around lines 176 - 182, Isolate Redis state across the affected E2E suites: in example/redis/redis-tests/e2e-01-data-exfiltration.yaml#L176-L182, remove the seeded user:42 and session:abc123 keys and explicitly define whether the exfil-canary fixture is retained; in example/redis/redis-tests/e2e-03-config-save-cron-persistence.yaml#L59-L64, replace the initial FLUSHALL dependency with an isolated Redis instance; remove the shared-database clearing commands before the SSH persistence phase at `#L96-L99` and before the webshell persistence phase at `#L125-L128`.example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml (1)
35-71: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash set -euo pipefail # Map the AttackSuite executor before inspecting Redis connection ownership. ast-grep outline pkg --items all --type function --match 'AttackSuite|Redis|Attack' # Inspect whether attacks share a connection or create one connection per command. rg -n -C 6 --glob '*.go' \ '\bAttackSuite\b|\bMONITOR\b|net\.Dial|DialContext|redis.*command' pkgRepository: k8sstormcenter/bob
Length of output: 170
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Repo tracked files around package and example:" git ls-files | sed -n '1,200p' echo echo "Find test manifest and Redis-related files:" git ls-files | rg 'redis-tests|redis|e2e-04|attack|redis' || true echo echo "Inspect target manifest with line numbers:" cat -n example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml | sed -n '1,140p'Repository: k8sstormcenter/bob
Length of output: 15837
🏁 Script executed:
#!/bin/bash set -euo pipefail echo "Search repository for bobctl/attack execution definitions and redis protocol handling:" git ls-files | rg 'redis-client|redis|client|protocol|attack|exec|custom' | sed -n '1,240p' echo echo "Search for redis client implementation and connections:" rg -n -C 5 --glob '*.{go,ts,js,py}' \ '\bcustom\b|\bredis\b|\bMONITOR\b|\bAUTH\b|\bCLIENT LIST\b|\bnet\.Dial|DialContext|bufio\.Reader|ReadLine' . \ | sed -n '1,320p' echo echo "Inspect Redis docs/examples for related runner behavior if present:" if [ -f example/redis/redis-tests/E2E-SCENARIOS.md ]; then cat -n example/redis/redis-tests/E2E-SCENARIOS.md | sed -n '1,240p' fiRepository: k8sstormcenter/bob
Length of output: 35207
🌐 Web query:
Redis MONITOR command replies to client until disconnected cannot see commands issued before MONITOR started documentation💡 Result:
The Redis MONITOR command is designed to stream commands processed by the Redis server in real-time, starting only from the moment the MONITOR command is successfully executed [1][2]. It does not provide access to, or a history of, any commands that were issued or processed before the MONITOR session was initiated [1][3]. Because MONITOR functions as a live stream of incoming requests, it is not an audit log and cannot retroactively retrieve past activity [1][4]. Furthermore, its operation has performance implications, as running MONITOR can significantly reduce server throughput [3][4]. Key characteristics regarding its scope include: - Real-time streaming: It only displays commands that arrive at the server while the MONITOR session is active [1][2]. - Security exclusions: Certain administrative commands (e.g., CONFIG, AUTH) and the QUIT command are not included in the MONITOR output for security and functional reasons [1][2]. - Performance impact: Monitoring produces a high overhead and can reduce server performance by 50% or more [3][4]. If you require a history of commands for auditing or forensic purposes, Redis does not provide this through the MONITOR command. Alternative approaches, such as using the Append Only File (AOF) or analyzing replication streams, are typically required for persistent command logging [5].
Citations:
- 1: https://redis.io/docs/latest/commands/monitor/
- 2: https://redis.github.net.cn/docs/latest/commands/monitor/
- 3: https://redis.io/tutorials/operate/redis-at-scale/observability/
- 4: https://redis-doc-test.readthedocs.io/en/latest/commands/monitor/
- 5: redis/redis#12280
🏁 Script executed:
#!/bin/bash set -euo pipefail python3 - <<'PY' # Read-only behavioral model of the sequence in e2e-04-monitor-credential-sniff.yaml. # MONITOR streams commands received after the successful MONITOR call; it cannot see AUTH/HSET issued earlier. attack_sequence = [ "sniff-decoy-auth", "sniff-decoy-session-write", "sniff-monitor-start", "sniff-client-list", "sniff-cleanup", ] decoys = ["sniff-decoy-auth", "sniff-decoy-session-write"] monitor_start = "sniff-monitor-start" captured_by_monitor = decoys if attack_sequence.index(monitor_start) == len(attack_sequence) - 1 else [] print("target manifest: example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml") print(f"attack order: {attack_sequence}") print(f"MONITOR occurs at index {attack_sequence.index(monitor_start)}") print(f"Decoy operations before MONITOR: {decoys}") print(f"Monitor-capture model for this manifest: {captured_by_monitor}; expected empty because decoys are sent before MONITOR starts.") print("Required behavior: open a managed MONITOR session before sending decoy commands via a second connection, then assert the monitor output contains credential-bearing commands and close the session during cleanup.") PYRepository: k8sstormcenter/bob
Length of output: 788
Start MONITOR before sending decoy traffic on a separate connection.
REDIS MONITOR only streams commands processed after MONITOR succeeds. This manifest sends AUTH and HSET before the MONITOR command, so the asserted decoy commands will not appear in the monitor output. Open the monitored session first, send decoys through a second Redis connection, assert the credential-bearing commands in the stream, and close the monitor session during cleanup.
🤖 Prompt for 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. In `@example/redis/redis-tests/e2e-04-monitor-credential-sniff.yaml` around lines 35 - 71, Reorder the e2e monitor scenario so the sniff-monitor-start session is established before any decoy AUTH or HSET traffic, then send those decoys through a separate Redis connection. Assert that the monitor stream captures both credential-bearing commands, and update sniff-cleanup to close the monitor session after verification.example/redis/redis-tests/README.md (1)
8-24: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the stale attack-11/attack-12 filenames in the directory layout.
This layout lists
attack-11-reverse-shell-curl.yamlandattack-12-hardlink-shadow.yaml, but the files actually shipped in this PR areattack-11-reverse-shell-perl.yamlandattack-12-credential-harvest.yaml(also reflected correctly inDETECTION-RULES.md's table). Update the layout so agents picking "an unclaimed attack-NN-*.yaml file" per the Agent Assignment section below don't look for nonexistent files.📝 Suggested fix
-├── attack-11-reverse-shell-curl.yaml # R0001 — curl to C2 -├── attack-12-hardlink-shadow.yaml # R1012 — hardlink over shadow +├── attack-11-reverse-shell-perl.yaml # R0001 — perl reverse shell +├── attack-12-credential-harvest.yaml # R0001 — credential harvest📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.🧰 Tools
🪛 markdownlint-cli2 (0.23.1)
[warning] 8-8: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🤖 Prompt for 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. In `@example/redis/redis-tests/README.md` around lines 8 - 24, Update the README directory layout entries for attack 11 and attack 12 to use attack-11-reverse-shell-perl.yaml and attack-12-credential-harvest.yaml, matching the shipped files and DETECTION-RULES.md.example/redis/redis-tests/run-parallel.sh (1)
32-42: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
run_one()misreports sandbox-blocked attacks as failures.
run-all.shtreats exit code 2 as SKIP ("sandbox blocked"), butrun_one()here has no equivalent branch: any nonzero exit, including 2, is reported as[FAIL]. Running the same attack suite viarun-all.shvsrun-parallel.shtherefore produces different PASS/FAIL/SKIP semantics for the same result, which can mislead someone reading the parallel run's summary.🛠️ Suggested fix to align SKIP handling with run-all.sh
if bobctl attack run "$f" > "${RESULTS_DIR}/${name}.log" 2>&1; then echo "[PASS] ${name}" else - echo "[FAIL] ${name} (exit $?)" + rc=$? + if [[ ${rc} -eq 2 ]]; then + echo "[SKIP] ${name} (sandbox blocked)" + else + echo "[FAIL] ${name} (exit ${rc})" + fi fi📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.run_one() { local f="$1" local name name="$(basename "$f" .yaml)" echo "[START] ${name}" if bobctl attack run "$f" > "${RESULTS_DIR}/${name}.log" 2>&1; then echo "[PASS] ${name}" else rc=$? if [[ ${rc} -eq 2 ]]; then echo "[SKIP] ${name} (sandbox blocked)" else echo "[FAIL] ${name} (exit ${rc})" fi fi }🤖 Prompt for 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. In `@example/redis/redis-tests/run-parallel.sh` around lines 32 - 42, Update run_one() to capture bobctl attack run’s exit status before the conditional reporting, and add an exit-code-2 branch that reports the attack as SKIP, matching run-all.sh. Preserve the existing PASS output for status 0 and FAIL output for all other nonzero statuses, including the captured status in the failure message.
entlein
commented
Jul 31, 2026
There was a problem hiding this comment.
Actionable comments posted: 12
🤖 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 `@example/argocd/drive-gitops-workload.sh`:
- Around line 34-43: Update the apply_apps and apply_appset functions to capture
the kubectl apply exit status instead of suppressing failures, while retaining
the existing output redirection if desired. When either Application or
ApplicationSet seeding fails, report the failure and stop the script before
starting the workload window, ensuring a failed seed cannot produce an
idle-control-plane profile.
- Line 25: Validate DURATION immediately after its default assignment and before
the arithmetic that computes END, accepting only a non-negative integer and
exiting with a clear error for invalid input. Ensure the existing loop uses a
validated numeric duration so malformed arguments cannot produce an empty END or
broken test.
In `@example/argocd/sbobs/cp-argocd-application-controller.yaml`:
- Around line 946-955: Remove or correct the misattributed :6443/metrics inbound
records: in example/argocd/sbobs/cp-argocd-application-controller.yaml#L946-L955
use port 8082, in
example/argocd/sbobs/cp-argocd-applicationset-controller.yaml#L108-L118 use port
8080, and in
example/argocd/sbobs/cp-argocd-notifications-controller.yaml#L120-L130 use port
9001; remove the record from
example/argocd/sbobs/cp-argocd-repo-server.yaml#L1305-L1314.
- Around line 931-934: Confirm whether CAP_DAC_OVERRIDE, CAP_DAC_READ_SEARCH,
and CAP_NET_ADMIN are genuinely used by each container rather than inherited
from the node-agent or runtime bounding set; if not, remove the capabilities
entries. Apply the same decision in
example/argocd/sbobs/cp-argocd-application-controller.yaml lines 931-934,
cp-argocd-applicationset-controller.yaml lines 104-107,
cp-argocd-notifications-controller.yaml lines 116-119,
cp-argocd-repo-server.yaml lines 1300-1303, and cp-argocd-server.yaml lines
104-107, retaining only capabilities attributable to the repo-server binaries
where applicable.
- Around line 169-789: Collapse the per-run discovery cache entries in the
manifest’s file-path configuration by replacing random numeric suffixes and the
hard-coded 10.43.0.1 API-server segment with dynamic path wildcards. Apply this
to the discovery cache paths ending in serverresources.json and related
discovery metadata, preserving the existing flags while reducing the artifact to
a single reusable wildcard pattern.
- Around line 24-120: Replace the truncated root-level certificate paths in the
profile’s file-access entries with the complete
/usr/share/ca-certificates/mozilla paths, matching the full filenames recorded
in cp-argocd-repo-server.yaml; alternatively regenerate the profile using the
corrected capture. Remove the invalid entries such as /.crt, /crt, /rt, and /t
while preserving the existing open flags.
In `@example/argocd/sbobs/cp-argocd-repo-server.yaml`:
- Around line 20-53: The exec records contain volatile per-run values that
prevent argument matching, and the helm template record is truncated by a bare
--api-versions flag. Update the affected exec entries in the repo-server
configuration to match only stable arguments, collapsing or removing UUIDs, temp
suffixes, generated key IDs, pod/PID details, object counts, and commit SHAs;
alternatively record only each entry’s path. Also remove the incomplete helm
argument record or ensure every --api-versions flag has its value.
- Around line 1340-1354: Update the podSelector for the internal port 8081
ingress entry to retain only app.kubernetes.io/name:
argocd-application-controller. Remove apps.kubernetes.io/pod-index and
statefulset.kubernetes.io/pod-name so all application controller replicas match
the selector.
In `@kubescape/values.yaml`:
- Around line 21-22: Update the driver’s default DURATION used by
example/argocd/drive-gitops-workload.sh to exceed the 15m learningPeriod
configured in kubescape/values.yaml, preserving enough runtime margin for
startup delays and the learning window’s tail.
In `@Makefile`:
- Around line 291-298: Update KS_RUNC_STOCK to classify the documented stock
paths /usr/bin/runc and /var/lib/rancher/k3s/data/current/bin/runc as stock
(yes) before applying the existing absolute-and-executable validation for custom
paths. Preserve the current fallback behavior for malformed, non-absolute, or
non-executable values so KS_RUNC_FLAGS leaves the override empty for all stock
paths.
In `@scripts/sbob-from-learned.py`:
- Around line 51-53: Update scripts/sbob-from-learned.py lines 51-53 so the
opens filtering normalizes Kubernetes service-account timestamp/random directory
segments to a stable or dynamic path while preserving other paths. Apply the
corresponding path replacements to the ca.crt, namespace, and token entries in
example/argocd/sbobs/cp-argocd-application-controller.yaml lines 892-900,
cp-argocd-applicationset-controller.yaml lines 73-81,
cp-argocd-notifications-controller.yaml lines 85-93, and cp-argocd-server.yaml
lines 73-81.
- Around line 31-37: Update is_over_broad to reject paths with any leading
wildcard segment, including /*/token, rather than only paths whose every segment
is a wildcard. Preserve handling of empty paths and use WILDCARD_SEGMENTS when
identifying wildcard segments.
🪄 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: e138d947-b405-4800-8219-428d5064c672
📒 Files selected for processing (10)
Makefileexample/argocd-repo-server-attacks.yamlexample/argocd/drive-gitops-workload.shexample/argocd/sbobs/cp-argocd-application-controller.yamlexample/argocd/sbobs/cp-argocd-applicationset-controller.yamlexample/argocd/sbobs/cp-argocd-notifications-controller.yamlexample/argocd/sbobs/cp-argocd-repo-server.yamlexample/argocd/sbobs/cp-argocd-server.yamlkubescape/values.yamlscripts/sbob-from-learned.py
💤 Files with no reviewable changes (1)
- example/argocd-repo-server-attacks.yaml
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@scripts/sbob-from-learned.py`:
- Line 86: Update the input parsing in the script’s main flow where spec is
loaded from args.src: replace JSON-only parsing with yaml.safe_load, while
preserving extraction of the "spec" key and compatibility with both YAML and
JSON artifacts.
🪄 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: f446a64c-7eb1-48f0-9f9e-b0a4f2a09328
📒 Files selected for processing (8)
example/argocd/sbobs/cp-argocd-application-controller.yamlexample/argocd/sbobs/cp-argocd-applicationset-controller.yamlexample/argocd/sbobs/cp-argocd-notifications-controller.yamlexample/argocd/sbobs/cp-argocd-repo-server.yamlexample/argocd/sbobs/cp-argocd-server.yamlkubescape/default-rules.yamlpkgscripts/sbob-from-learned.py
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@pkg`:
- Line 1: Update the pkg gitlink to a commit that includes the required CI
files, specifically pkg/go.mod and pkg/main.go, ensuring it points to the
intended commit eeec4a8a2b99917ead5123352358bb72a6b5bdfc rather than the
incomplete checked-out commit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
entlein
added a commit
that referenced
this pull request
Jul 31, 2026
The restructure moved postgres-vuln/ under example/. I updated the Dockerfile path in both Buildx steps but left `context: postgres-vuln`, which no longer exists — so the image build failed on a directory that had moved out from under it. Caught by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
entlein
added a commit
that referenced
this pull request
Jul 31, 2026
--json documented itself as machine-readable but printed the JSON blob after the full human report, so anything piping stdout into jq had to strip the preceding text first. The report now goes to stderr when --json is set, leaving stdout valid JSON on its own; without the flag nothing changes. Reported by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
Contributor
Author
|
@coderabbitai review The CI proves the gitlink resolves: the The pointer has since been advanced from Also addressed since the last review:
The last review covered up to |
|
I will review the current head, including the JSON output separation and the PostgreSQL profile-discovery change. ✏️ Learnings added
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@kubescape/post-render.sh`:
- Around line 36-51: Update the stream rewrite in the Python block of
post-render.sh to capture the result of replacing networkStreamingEnabled and
verify exactly one occurrence was changed. Add a guard matching the existing
mount-rewrite patched-count handling, and stop or report the same failure
condition when the count is not one, while preserving the dependency-free
behavior for the no-MNT path.
🪄 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: 90316d05-5c39-490a-a653-b19bb41b5cc7
📒 Files selected for processing (7)
.github/workflows/ci-bobctl-autotune.yaml.github/workflows/ci-postgres-vuln-image.yamlMakefilekubescape/force-network-streaming.shkubescape/post-render.shpkgscripts/compare-distros.py
💤 Files with no reviewable changes (1)
- kubescape/force-network-streaming.sh
entlein
added a commit
that referenced
this pull request
Jul 31, 2026
The replace() was unguarded, so a chart change that renamed or reformatted the field would turn the rewrite into a silent no-op — which is precisely the bug this script exists to prevent. The flag renders false, the profile's network shape stays inert, and R0005/R0011 never fire while the install looks healthy. The three cases are now distinguished: the field present as false is rewritten; already true is passed through untouched (a cloud-submit stack legitimately renders it that way); absent entirely is a hard error, because at that point the rewrite is doing nothing and detection would go quietly dead. Reported by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
Contributor
Author
|
@coderabbitai review The
Verified against synthetic input for all three, plus the real chart on both the default and CI is green on For the record, the webapp leg did fail once on this commit and then passed on a re-run of the same commit. That leg is intermittently flaky — it has also failed on |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
echo '--- HEAD ---'
git rev-parse --short HEAD
echo '--- recent commits ---'
git log --oneline -8
echo '--- post-renderer outline ---'
ast-grep outline kubescape/post-render.sh --items all || true
echo '--- post-renderer content ---'
cat -n kubescape/post-render.sh
echo '--- changes since dca5f80 parent ---'
git diff --check dca5f80^..dca5f80
git diff --find-renames -- kubescape/post-render.sh dca5f80^..dca5f80Length of output: 7185
The hard failure occurs before the default output path or the I found no remaining issue in this update. The reported webapp result is consistent with a pre-existing intermittent failure and does not require a change in this PR. ✏️ Learnings added
If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! ✅ Action performedReview finished.
|
Contributor
Author
|
@coderabbitai resolve |
✅ Action performedComments resolved and changes approved. |
…ample/
Two layout inconsistencies:
* redis was split across example/redis-client/ (manifests + sbob + gif) and
loose example/redis-*.yaml files, so "where is redis" had two answers.
* postgres/ and postgres-vuln/ sat at the repo ROOT while every other app
lived under example/.
Now example/<app>/ holds each app's manifests and assets, and the attack and
functional suites stay flat at example/ level exactly as they are for every
other app — so redis stops being the odd one out in both directions.
CI behaviour is unchanged. Every reference was rewritten and verified: the
Makefile deploy targets, both affected workflows (the postgres-vuln image
build's path filter and Dockerfile location, and the redis load-stress
functional tests), local-ci-unit.sh, rule-coverage.yaml and the README. All
moved manifests re-validate, and the rule-coverage cards still render.
Also fixes a real defect the pattern guardrail caught once the suites could
load again: argocd-server's cve-2023-22482-authbypass-k8s-pivot ended with
"; echo k8s_pivot_done", an UNCONDITIONAL sentinel — the attack reported
success even when the perl pivot failed outright. Now gated on perl exiting 0.
Remaining unit failures are the pre-existing elk/misp ones from the
pkg/nonmigrated archival in 484fac2; this change adds none and removes one.
release/ is deliberately NOT created yet — it takes a copy of the finished
SBoBs once the distro matrix is validated.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
The question is whether contrast testing actually distinguishes valkey from redis. That only has a defensible answer if the distros are the ONLY variable, so the deployments are GENERATED from one template rather than hand-written: identical namespace shape, identical Service name (redis) and container name (redis), identical securityContext and host mounts, one namespace each. `gen-distro-manifests.py --check` fails on drift, so the invariant is enforced rather than asserted. What legitimately differs is documented in the generator: image, server argv, and whether a redis.conf applies (dragonfly is a reimplementation, not a fork, and takes flags instead). Versions are each project's actual latest release as of 2026-07-31 — there is no "redis 18" or "valkey 15": redis 8.10.0 valkey 9.1.1 keydb 6.3.4 dragonfly 1.39.0 All four answer PING. Worth noting what they claim to be: valkey reports redis_version 7.2.4 for compatibility while being 9.1.1, and dragonfly reports 7.4.0 — so the advertised version is not a usable discriminator. First result, at the profile level, before any attack runs: redis-oss 20 opens /usr/local/bin/redis-server valkey 21 opens /usr/local/bin/valkey-server keydb 64 opens /usr/local/bin/keydb-server dragonfly 49 opens /usr/local/bin/dragonfly redis and valkey are one open apart; keydb and dragonfly differ materially. compare-distros.py reports the two axes separately — CONTRAST (which rules fired, the axis that decides whether a SBoB transfers) and BEHAVIOUR (what the learned profile contains). Collapsing them into one verdict would hide the interesting case: contrast-equivalent distros with different baselines. render-distro-poster.py composes an A4 four-panel poster by calling the SAME card renderer per panel, animating them in lockstep so a row can be read across at one instant. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
VERDICT: valkey, keydb and dragonfly are contrast-IDENTICAL to each other, and differ from redis on exactly ONE rule. distro raw opens rules fired redis-oss 20 14 (incl. R1000) valkey 21 13 keydb 64 13 dragonfly 49 13 Every non-redis distro fired the same 13 rules. The sole discriminator is R1000 (process executed from malicious source), green on redis and red on the other three. The drifted-binary attack SUCCEEDED on all four — so this is a detection difference, not an attack-execution difference. Treat that single-rule result with care: R1000/R1004 are the rule families that have proven most environment-sensitive across this work, so one rule is a thin basis for "valkey differs from redis". The defensible claim is the opposite one: across 13 of 14 rules the four are indistinguishable, and a SBoB's contrast behaviour transfers between them. Behaviour is a different story: raw baselines run 20 / 21 / 64 / 49 opens, so redis and valkey are one open apart while keydb and dragonfly are materially different. Contrast-equivalent does not imply behaviourally equivalent, which is why the two axes are reported separately. The comparison tool needed a methodology fix: it read the BEST iteration for the behaviour axis, but how far the minimiser collapses a profile depends on whether that distro converged — redis-oss reached score 0 and got collapsed to 1 open while the others stopped at their baseline. That reported a tuning artifact as a distro difference. Behaviour now reads raw-baseline. The poster stacks four full rule-coverage cards on A4, animating in lockstep so a row can be read across at one instant; R1000 is the one tile that differs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
Reported from a multi-node client machine with no containerd shim: the
detection emitted a mangled value that went straight through to helm as
--set global.overrideRuntimePath=]*/bin/runc
which silently misconfigures node-agent's runc fanotify marking — the exact
failure mode the override exists to PREVENT.
The guard no longer trusts the regex: a value is only used when it is absolute
AND executable on this machine. Empty, relative, mangled or non-existent all
fall back to "stock" and override nothing, which is the safe default since a
stock layout is what IG already knows.
Also worth recording why the first attempt at this failed: Make ends
$(shell ...) at the first unbalanced ')', so a `case` statement truncates
silently. The snippet is deliberately paren-free and says so.
Verified: real k3s runc overrides; garbage, empty, relative and
absolute-but-missing all correctly decline to override.
The Helm v4 half of #172 is NOT addressed here — it needs a decision on how to
force networkStreamingEnabled without --post-renderer.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
RCA: the profiles were learned from an Argo CD with ZERO Applications. The
2-minute window saw process startup and nothing else — no clone, no render, no
reconcile. So they encoded Argo CD idling, and every real GitOps operation fell
outside the baseline. That is the reporter's "thousands of alerts".
Worse than omission, the suite asserted the workload's own job as attack:
render-git-spawn / render-helm-spawn / render-kustomize-spawn claimed R0001 on
git, helm and kustomize. For a GitOps controller those three ARE the primary
function. Removed.
What changed:
learn window 2m -> 15m, because exercising every component takes longer
than two minutes
workload example/argocd/drive-gitops-workload.sh drives the DEFAULT
actions of a real install for the whole window: 3 Applications
across the plain/helm/kustomize render paths, an ApplicationSet
so the applicationset-controller generates rather than idles,
a transient app for the create->render->sync->delete lifecycle,
hard refreshes to defeat the manifest cache, plus the API read
path and the metrics endpoints Prometheus scrapes
Result, repo-server:
execs 5 -> 10 distinct binaries, now including git, /usr/lib/git-core/git,
git-remote-http, helm and kustomize
opens 51 -> 255
egress null -> github.com:443, DNS and redis declared
argocd-server 7->21 opens, application-controller 21->188.
Over-broad opens are DROPPED, not rewritten: node-agent's own dynamic-path
collapsing degenerates to a bare `/*` and `/⋯` under git-checkout churn, and a
path with no literal segment matches /etc/shadow and the SA token — shipping it
would blind R0002/R0006/R0008/R0010. It carries no information, so there is
nothing to preserve by keeping it.
sbob-from-learned.py does that conversion and strips learned bookkeeping, so a
shipped SBoB is clean YAML with one annotation and no commentary.
Still open: nothing in the pipeline checks that a learn window exercised the
workload's primary function. Score 0, contrast 0-Blind and the coverage card
were all green against an idle baseline. That gap is what allowed this.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
…cessAllowed
Verified the reporter's scenario end to end rather than assuming the re-learn
was sufficient — apply the SBoBs, bind them, drive benign GitOps, count alerts.
Two more defects surfaced that the re-learn alone did not fix.
1. Kubernetes projected volumes (SA token, ConfigMaps, Secrets) write each
generation into a "..<timestamp>" directory and atomically re-point ..data at
it. The learned profile captured the literal timestamp from training time, so
after the first rotation the workload's OWN token read fell outside the
baseline and fired R0002 continuously. That one segment is now collapsed to
the single-segment wildcard, leaving the rest of the path literal so a read of
some other secret is still anomalous.
2. Listing the token in `opens` does NOT satisfy R0006 — it is a separate rule
that must name the process allowed to read a ServiceAccount token. Derived
from the learned execs' argv[0], because comm is argv[0]'s basename truncated
by the kernel to 15 chars: /usr/local/bin/argocd-application-controller is
seen as "argocd-applicat", and the exec PATH basename ("argocd") would never
match.
Measured on 5 minutes of benign GitOps, per component:
repo-server 0 false positives — the worst offender before, and the one that
was missing git/helm/kustomize and the github.com egress
R0002 down to 1
Remaining and NOT fixed here: R0006 still fires ~22 times per 5 minutes on the
controllers despite processAllowed naming exactly the comm the alert carries
(e.g. alert comm "argocd-server" vs processAllowed ["argocd-server","tini"]).
Ruled out: token path missing from opens (fixed), learned-profile precedence
(deleted the learned profiles, no change), wrong comm (verified identical). So
R0006's processAllowed appears not to be honoured by this node-agent build —
an engine issue rather than SBoB content, tracked separately.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
…onoured R0006 shipped with supportPolicy: false. node-agent gates the allow-list on that flag (rule_manager.go:225), so rulePolicies.R0006.processAllowed was DEAD CONFIG — parsed, applied, visible in the profile, silently ignored. A k8s-client workload therefore fired R0006 on its OWN ServiceAccount token no matter what the SBoB said. Note this is NOT profileDependency, which is the neighbouring field and an enum (0=Required, 1=Optional, 2=NotRequired). R0006 already had 0 = Required, which is exactly why the wrong conclusions came first: the rule DOES depend on a profile, it just would not honour the allow-list. Measured on the same 5-minute benign GitOps run, whole argocd namespace: before the re-learn thousands (reporter) after re-learn 22 (21x R0006, 1x R0002) after supportPolicy 1 (R0002 on dex, which has no SBoB bound) Every component with a SBoB bound now produces zero false positives. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
This PR is the example-layout restructure plus the redis-protocol distro
comparison. The Argo CD work (bob#170) landed here only because it happened in
the same session, and it does not belong in the same review.
Reverted to main:
example/argocd-server-attacks.yaml, example/argocd-repo-server-attacks.yaml
example/argocd/sbobs/cp-argocd-{server,repo-server,application-controller,
applicationset-controller}.yaml
kubescape/default-rules.yaml (R0006 supportPolicy)
kubescape/values.yaml (learn window 2m -> 15m)
Removed (added for the Argo CD work only):
example/argocd/drive-gitops-workload.sh
example/argocd/sbobs/cp-argocd-notifications-controller.yaml
scripts/sbob-from-learned.py
The submodule now points at eeec4a8 rather than ed64bf2, which drops the
AUTOTUNING.md note about supportPolicy (Argo-driven) while keeping the three
fixes the distro comparison actually depends on: the suite-path follow-up, the
--namespace retarget that lets one suite address several deployments, and the
metrics.json write that the comparison reads.
The distro results are unaffected — those tunes ran against the 2m learn window,
not the 15m one reverted here.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
The restructure moved postgres-vuln/ under example/. I updated the Dockerfile path in both Buildx steps but left `context: postgres-vuln`, which no longer exists — so the image build failed on a directory that had moved out from under it. Caught by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
The KS_RUNC auto-detection was broken in two ways. It read the local machine's process table to configure node-agent, which runs on the cluster NODES. Those are the same host only on a single-node local cluster; on a multi-node or remote cluster it sampled an unrelated `ps`, and on a client machine with no containerd shim it produced the malformed `--set global.overrideRuntimePath=]*/bin/runc` reported in #172. It also ran three subshells (ps, grep, findmnt) on EVERY make invocation because the assignments were immediate-expansion globals. KS_RUNC and KS_RUNC_MNT are now plain opt-in variables, validated as absolute paths, with the validation subshell inside the guard so the default path spawns none. With both unset the rendered manifest is byte-identical to before apart from cert regeneration, so CI is untouched. The filesystem mount moves out of --set and into the post-renderer. It could not stay: the chart ships nodeAgent.volumes as a fully-populated list, so --set nodeAgent.volumes[0] overwrites the first default entry and drops /profiles, while the top-level `volumes` key appends but is global — it was injecting the mount into all six chart workloads when only node-agent needs it. Renaming force-network-streaming.sh to post-render.sh reflects that it now does two rewrites; the node-agent DaemonSet is the only document round-tripped through the YAML parser so helm's embedded config.json block scalars are emitted verbatim. The PyYAML import happens after the opt-in is confirmed, keeping the default path dependency-free. Refs #172 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
eeec4a8 was a mid-branch commit; entlein/bob#59 and #60 have since merged. The trees are identical, so this is pointer hygiene rather than a content change — it stops the gitlink referencing a commit that is not on main. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
The postgres leg failed with all 21 detections missed and 0 false positives — the signature of tuning a container the attacks never touched. Only one profile ever completed in the window: pg-1-bootstrap-controller. That is a short-lived init container, so its profile finishes within seconds while pg-client is still being exercised. The bare `head -1` fallback in the poll loop accepted it on the FIRST iteration and broke out, so the tune measured the bootstrap container while every attack landed on pg-client. main won this race; this branch did not. The match is now the only thing that ends the wait. The unmatched fallback moves after the timeout, where it still produces diagnostics but says loudly that the result is not authoritative. Also fixes the `grep -v client` filter, which was self-defeating for postgres: MATCH is pg-client, so the exclusion dropped exactly the profile being searched for and left the loop relying on the buggy fallback. And points the path filter at example/postgres/** after the restructure. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
--json documented itself as machine-readable but printed the JSON blob after the full human report, so anything piping stdout into jq had to strip the preceding text first. The report now goes to stderr when --json is set, leaving stdout valid JSON on its own; without the flag nothing changes. Reported by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
The replace() was unguarded, so a chart change that renamed or reformatted the field would turn the rewrite into a silent no-op — which is precisely the bug this script exists to prevent. The flag renders false, the profile's network shape stays inert, and R0005/R0011 never fire while the install looks healthy. The three cases are now distinguished: the field present as false is rewritten; already true is passed through untouched (a cloud-submit stack legitimately renders it that way); absent entirely is a hard error, because at that point the rewrite is doing nothing and detection would go quietly dead. Reported by CodeRabbit on #173. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
entlein
force-pushed
the
feat/restructure-release-redis-distros
branch
from
August 1, 2026 11:24
dca5f80 to
06ad7c6
Compare
entlein
added a commit
that referenced
this pull request
Aug 2, 2026
…t-in Reverts 3bb7081, which dropped the Argo CD changes from #173 to keep that PR scoped to the restructure. This brings back the five re-learned SBoBs, the GitOps driver that makes the learn window representative, sbob-from-learned.py, and the R0006 supportPolicy fix. Two things are deliberately NOT restored as they were. kubescape/values.yaml keeps the chart default learningPeriod of 2m. The reverted commit set it to 15m globally, which would break every CI leg: the workflow waits 600s for a completed ContainerProfile, so a 15m window means no profile ever completes and the whole matrix times out. Argo CD genuinely needs longer than 2m — bob#170 was caused by a 2m window on an idle control plane learning 5 execs instead of 10 and 51 opens instead of 517 — so the longer window is now an opt-in Makefile variable, validated as a Go duration and absent from the helm invocation unless set: make kubescape KS_LEARN_PERIOD=15m The pkg submodule pointer stays at main's 2a2b337. The reverted commit pointed it at ed64bf2, which lives on an inner topic branch and is not reachable from inner main — exactly the off-main gitlink CodeRabbit flagged on #173. The AUTOTUNING.md note on that commit (supportPolicy gating rulePolicies) still needs landing inner-side on its own. Refs #170 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm
This was referenced Aug 2, 2026
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.
Scope: example-layout restructure + the redis-protocol distro comparison. No Argo CD changes — those were reverted out of this branch so they can be reviewed separately.
Depends on the inner submodule commits
daff078..eeec4a8(suite-path follow-up,--namespaceretarget,metrics.jsonwrite) — these need an inner PR before this can merge.Verdict
valkey, keydb and dragonfly are contrast-identical to each other, and differ from redis on exactly one rule.
The sole discriminator is R1000. The drifted-binary attack succeeded on all four, so it is a detection difference, not an execution one — and R1000/R1004 have been the most environment-sensitive families in this work, so one rule is thin evidence. The defensible claim is the inverse: across 13 of 14 rules the four are indistinguishable, so a SBoB's contrast behaviour transfers between them.
Behaviour is a separate axis — raw baselines of 20 / 21 / 64 / 49 opens. redis and valkey are one open apart; keydb and dragonfly differ materially. Contrast-equivalent ≠ behaviourally equivalent.
Method
All four deploy from one generated template (
gen-distro-manifests.py --checkfails on drift): identical pod spec, Service name, container name, securityContext and host mounts, one namespace each. Only image and server argv differ. Versions are each project's actual latest — there is no redis 18 or valkey 15. The advertised version is not a discriminator either: valkey reportsredis_version 7.2.4while being 9.1.1, dragonfly reports 7.4.0.Layout
postgres/andpostgres-vuln/moved from the repo root intoexample/; redis unified fromexample/redis-client/+ looseexample/redis-*.yamlintoexample/redis/.release/is deliberately not created — it takes a copy of the finished SBoBs later.CI behaviour is unchanged and verified: every reference rewritten (Makefile, both affected workflows,
local-ci-unit.sh,rule-coverage.yaml, README), all moved manifests re-validate, cards still render.Bugs found on the way
--namespaceonly half-applied — it setcfg.Namespacewhile the exec resolver and redis port-forward readsuite.Target.Namespace, failing asservices "redis" not found. This is what blocks reusing one suite across deployments (inner).metrics.jsoncould vanish silently — guarded by a bareif err == nil; the run printed PERFECT while producing nothing, and the gate then blamed the tune (inner).KS_RUNCcould emit a malformed path (make kubescape / make alertmanager broken on Helm v4 (--post-renderer) + KS_RUNC misdetection #172) —--set global.overrideRuntimePath=]*/bin/runcon a machine with no containerd shim. Now only absolute, executable paths are used.🤖 Generated with Claude Code
https://claude.ai/code/session_01Jd9m962b1JAHdAy3rGe7nm