Fix PR #10 review findings for lab operations - #11
donny-devops wants to merge 3 commits into
Conversation
Add the missing privileged-container module and unique assessments so the advertised 11-module course is complete. Bind lab ports to localhost, add a hardened Compose topology, and replace broken exploit stubs with detection helpers. CI runs offline pytest plus script syntax checks so the lab-validate job stays green. Co-authored-by: Adonis Jimenez <donny-devops@users.noreply.github.com>
Add the missing Attack scenario heading so catalog checks pass, and point the lab landing page at the standalone hardened Compose file. Co-authored-by: Adonis Jimenez <donny-devops@users.noreply.github.com>
Strip UTF-8 BOMs from helper scripts, copy catalog fixtures into the validation image, interpolate Compose env overrides, and tighten published-port detection plus course examples so checks stay green. Co-authored-by: Adonis Jimenez <donny-devops@users.noreply.github.com>
ECC Tools / Security EvidenceCommit: Security scanner evidence required (action_required) Detected 1 security-sensitive predictive risk signal(s) without scanner evidence. Mode: enforce Findings:
Touched security-sensitive paths:
Expected evidence:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Risk TaxonomyCommit: PR taxonomy review recommended (neutral) Detected 3 PR taxonomy bucket(s): Security Evidence, CI/CD Recommendation, PR Review/Salvage Evidence. Scanned 78 changed file(s). Roadmap taxonomy buckets: Security EvidenceSecurity-sensitive changes should carry explicit scanner, code-scanning, or focused regression evidence. Signals:
Paths:
CI/CD RecommendationCI, dependency, coverage, and contract signals should be routed into follow-up checks or verification work. Signals:
Paths:
PR Review/Salvage EvidencePR review, stale-closure, and triage automation should carry stale-closure, reviewer-thread, and reopen-flow evidence so useful closed work can still be salvaged. Signals:
Paths:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Reference Set ReadinessCommit: Reference set readiness gaps detected (neutral) Reference evidence present for 0/7 areas (0%) across 78 changed file(s). This check is based on files changed in this PR. Repository-level readiness is still reported by
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / Hosted Promotion ReadinessCommit: Hosted promotion readiness passed (success) No hosted promotion evidence gaps detected across 78 changed file(s); 0 corpus scenarios had matching evidence. This check compares PR file changes against the evaluator/RAG promotion corpus in No evaluator corpus scenarios matched this PR. Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Config AuditCommit: No changed-config issues detected (success) Scanned 1 config file(s) present at this commit across 1 changed config path(s) and found no issues in the supported security rules. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
ECC Tools / PR Harness AuditCommit: No harness issues detected (success) Scanned 1 changed config file(s) and found no harness issues. Changed config files:
Check publication was denied or unavailable. An app owner must enable Checks: read and write, and the installation owner must approve the updated permission. |
There was a problem hiding this comment.
This PR successfully applies review findings from PR #10, addressing security and configuration improvements for the Docker hacking lab. The changes include:
Key improvements:
- Published ports now bind to localhost with proper
LAB_BIND_ADDRESSinterpolation - Environment variables use proper shell interpolation patterns (
${VAR:-default}) - UTF-8 BOM detection tests added for Compose files and scripts
- Comprehensive health checks added for all services
- Lab validation and misconfiguration detection tooling implemented
Verification:
- Test suite validates both insecure baseline and hardened configurations
- Port binding restrictions properly enforced
- Example secrets correctly identified as documentation placeholders
- No actual security vulnerabilities present in production code
The educational lab properly maintains intentional insecurities (Docker socket mount, privileged containers, host root mounts) with clear documentation, while also providing hardened alternatives for comparison. All changes appear ready for merge.
You can now have the agent implement changes and create commits directly on your pull request's source branch. Simply comment with /q followed by your request in natural language to ask the agent to make changes.
There was a problem hiding this comment.
🟡 Changes recommended
Critical detector and validation-image issues remain, along with additional correctness and configuration findings.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
This follow-up operationalizes the Docker security lab with course content, Compose configurations, defensive tooling, validation, and documentation updates.
Changes:
- Adds 11 course modules and 22 assessments.
- Adds insecure and hardened Compose topologies with detection and validation tooling.
- Updates tests, CI, documentation, helper scripts, and lab infrastructure.
File summaries
| File | Summary |
|---|---|
tests/test_scripts.py |
Tests scripts, detectors, BOMs, seccomp, and landing pages. |
tests/test_course_catalog.py |
Validates course inventory and uniqueness. |
tests/test_compose.py |
Tests Compose contracts. |
tests/README.md |
Documents test usage. |
tests/conftest.py |
Defines shared test paths and constants. |
SUPPORT.md |
Provides support instructions. |
solutions/seccomp-restricted.json |
Provides a seccomp allow-list profile. |
solutions/README.md |
Documents defensive solutions. |
solutions/detect_misconfigurations.py |
Analyzes Compose misconfigurations. |
SKILL.md |
Updates learning objectives. |
ROADMAP.md |
Updates project milestones. |
requirements-dev.txt |
Defines development dependencies. |
README.md |
Updates quickstart and course overview. |
pytest.ini |
Configures pytest. |
Makefile |
Adds validation and Compose targets. |
lab/www/index.php |
Provides the PHP lab landing page. |
lab/www/index.html |
Provides the static lab landing page. |
lab/validate.sh |
Runs offline validation. |
lab/README.md |
Documents lab operations. |
lab/Dockerfile |
Builds the validation image. |
docker-compose.yml |
Defines the insecure teaching topology. |
docker-compose.hardened.yml |
Defines the standalone hardened topology. |
course/README.md |
Documents the course catalog. |
course/quizzes/shared-kernel.md |
Adds the shared-kernel quiz. |
course/quizzes/shared-kernel-final.md |
Adds the shared-kernel final assessment. |
course/quizzes/privileged-containers.md |
Adds the privileged-containers quiz. |
course/quizzes/privileged-containers-final.md |
Adds the privileged-containers final assessment. |
course/quizzes/orchestration.md |
Adds the orchestration quiz. |
course/quizzes/orchestration-final.md |
Adds the orchestration final assessment. |
course/quizzes/lsm.md |
Adds the LSM quiz. |
course/quizzes/lsm-final.md |
Adds the LSM final assessment. |
course/quizzes/kernel-vulns.md |
Adds the kernel-vulnerabilities quiz. |
course/quizzes/kernel-vulns-final.md |
Adds the kernel-vulnerabilities final assessment. |
course/quizzes/insecure-images.md |
Adds the insecure-images quiz. |
course/quizzes/insecure-images-final.md |
Adds the insecure-images final assessment. |
course/quizzes/insecure-config.md |
Adds the insecure-configuration quiz. |
course/quizzes/insecure-config-final.md |
Adds the insecure-configuration final assessment. |
course/quizzes/image-tampering.md |
Adds the image-tampering quiz. |
course/quizzes/image-tampering-final.md |
Adds the image-tampering final assessment. |
course/quizzes/exposed-apis.md |
Adds the exposed-APIs quiz. |
course/quizzes/exposed-apis-final.md |
Adds the exposed-APIs final assessment. |
course/quizzes/dos.md |
Adds the DoS quiz. |
course/quizzes/dos-final.md |
Adds the DoS final assessment. |
course/quizzes/container-escape.md |
Adds the container-escape quiz. |
course/quizzes/container-escape-final.md |
Adds the container-escape final assessment. |
course/modules/11-lsm.md |
Adds the LSM and seccomp module. |
course/modules/10-insecure-images.md |
Adds the insecure-images module. |
course/modules/09-orchestration.md |
Adds the orchestration module. |
course/modules/08-shared-kernel.md |
Adds the shared-kernel module. |
course/modules/07-kernel-vulns.md |
Adds the kernel-vulnerabilities module. |
course/modules/06-dos.md |
Adds the DoS module. |
course/modules/05-insecure-config.md |
Adds the insecure-configuration module. |
course/modules/04-image-tampering.md |
Adds the image-tampering module. |
course/modules/03-container-escape.md |
Adds the container-escape module. |
course/modules/02-exposed-apis.md |
Adds the exposed-APIs module. |
course/modules/01-privileged-containers.md |
Adds the privileged-containers module. |
COURSE_SUMMARY.md |
Summarizes the course. |
CONTRIBUTING.md |
Documents contribution guidance. |
CODEOWNERS.md |
Documents repository ownership. |
CHANGELOG.md |
Records release changes. |
attacker-tools/supply_chain.sh |
Inspects image references. |
attacker-tools/runtime_defense.sh |
Compares runtime posture. |
attacker-tools/registry_attack.sh |
Checks registry exposure. |
attacker-tools/README.md |
Documents defensive helpers. |
attacker-tools/pivot_network.sh |
Maps lab network position. |
attacker-tools/inspect_internals.sh |
Inspects kernel isolation. |
attacker-tools/exploit_tcp_daemon.sh |
Detects Engine TCP listeners. |
attacker-tools/escape_socket.sh |
Detects Docker socket exposure. |
attacker-tools/escape_hostmount.sh |
Detects host mounts. |
attacker-tools/enumerate.sh |
Performs read-only enumeration. |
attacker-tools/arp_spoof.sh |
Adds network inspection. |
ARCHITECTURE.md |
Documents system architecture. |
.gitignore |
Adds development exclusions. |
.github/workflows/ci.yml |
Adds offline CI validation. |
.github/PULL_REQUEST_TEMPLATE.md |
Adds the pull request checklist. |
.github/dependabot.yml |
Configures dependency updates. |
.github/CODEOWNERS |
Defines canonical ownership. |
.env.example |
Provides Compose environment defaults. |
Review details
Suppressed comments (9)
.env.example:4
- With the documented
cp .env.example .envflow, this variable is loaded for both Compose files.COMPOSE_PROJECT_NAMEtakes precedence over each file's top-levelname, so the hardened file usesdocker-hacking-labinstead ofdocker-hacking-lab-hardened, contradicting the teardown instructions; the block is also duplicated. Remove the shared project-name override (and keep one copy of the remaining settings), or provide per-stack env files.
COMPOSE_PROJECT_NAME=docker-hacking-lab
.github/PULL_REQUEST_TEMPLATE.md:10
- This checklist still calls the hardened file an “overlay”, but the repository now documents it as a standalone Compose project and explicitly warns not to merge it with the insecure file. Keeping “overlay” here can lead contributors to apply the wrong Compose invocation; use “hardened Compose” instead.
- Hardening overlay / detection scripts stay defensive.
ARCHITECTURE.md:28
- This absolute statement conflicts with the Compose contract documented in
README.md:5andlab/README.md:3:LAB_BIND_ADDRESScan intentionally override the loopback default. Qualify this as the default bind address so the architecture/security documentation does not claim that non-loopback overrides are impossible.
- Host publishes bind to `127.0.0.1` only.
Makefile:20
- The guard checks only for the Docker executable, not the Compose plugin. On a host with Docker installed but no
docker compose, this target enters the first branch and fails instead of using its documented fallback; checkdocker compose versionas the CI workflow does.
@if command -v docker >/dev/null 2>&1; then \
attacker-tools/registry_attack.sh:5
- The README says these helpers can run on the host, but the default
registry:5000name resolves only from a container on the Compose network. On the host, the insecure registry is reachable at127.0.0.1:5000; this default makescurlfail and prints “good if intended,” hiding the unauthenticated-registry finding. Support the host address or require/documentREGISTRY=127.0.0.1:5000there, and distinguish an unreachable target from an authenticated one.
REGISTRY="${REGISTRY:-registry:5000}"
course/modules/04-image-tampering.md:19
- The image is built as
app:1.0, but the next commands useregistry.example/app:1.0. Without adocker tag(or a registry-qualified build tag),docker pushand thedocker image inspectlookup fail because that local reference does not exist. Build the registry-qualified reference before pushing.
course/modules/11-lsm.md:48 Seccomp:in/proc/self/statusandno-new-privilegesare separate controls; this Compose file enables the latter but does not configure a custom seccomp profile. Comparing them as though they were the same signal can mislead students about which control is active. Clarify that the seccomp mode is checked separately andno-new-privilegesis an additional control.
docker-compose.hardened.yml:67- The standard
php:7.4-apacheimage does not providepidofby default, so this shell healthcheck exits with command-not-found even when Apache is serving traffic.vuln-webwill therefore remain unhealthy in the hardened stack; use a probe available in the image or install the utility in a derived image.
docker-compose.yml:61 - The standard
php:7.4-apacheimage does not providepidofby default, so this shell healthcheck exits with command-not-found even when Apache is serving traffic.vuln-webwill therefore remain unhealthy in the teaching stack; use a probe available in the image or install the utility in a derived image.
- Files reviewed: 77/78 changed files
- Comments generated: 6
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| WORKDIR /lab | ||
| COPY requirements-dev.txt /lab/requirements-dev.txt | ||
| RUN pip install --no-cache-dir -r requirements-dev.txt |
| def _ip_is_loopback(value: str) -> bool: | ||
| text = value.strip().lower().strip("[]") | ||
| return any(marker in text for marker in LOOPBACK_MARKERS) |
| if _ip_is_loopback(text): | ||
| return False |
| docker images --format '{{.Repository}}:{{.Tag}} {{.Digest}}' | awk ' | ||
| $2 == "<none>" { print "[unpinned] " $1; next } | ||
| { print "[pinned] " $1 " " $2 } | ||
| ' |
| ## Noncompliant | ||
|
|
||
| ```bash | ||
| docker build -t app:latest . |
| COMPOSE_PROJECT_NAME=docker-hacking-lab | ||
| POSTGRES_USER=admin | ||
| POSTGRES_PASSWORD=lab-only-password | ||
| POSTGRES_DB=corporate_secrets | ||
| LAB_BIND_ADDRESS=127.0.0.1 |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Closes #7.
Follow-up to PR #10 that applies Copilot/Amazon Q review findings. The repo rejects extra pushes to an existing
donny-devops/**PR ref, so this branch carries the original catalog/CI work plus the review fixes.Review fixes
COURSE_SUMMARY.mdandlab/wwwintolab/Dockerfileat the paths pytest expectsPOSTGRES_*andLAB_BIND_ADDRESSin both Compose fileshost_ip/[::]published ports; keep loopback defaultssubArchestosubArchitecturesdown -vcompose-configchains with&&--pidexample, seccomp vs LSM,REGISTRY=registry:5000, digest classification, no mutatingtouchVerify
python3 -m pip install -r requirements-dev.txt make test python3 solutions/detect_misconfigurations.py docker-compose.yml --expect-findings python3 solutions/detect_misconfigurations.py docker-compose.hardened.ymlLocal
lab-validate: 50 passed.