fix: enhance OpenVPN image security and privacy #16
Conversation
📝 WalkthroughWalkthroughThis PR updates CI, the container image, the entrypoint comment, and documentation. The GitHub Actions workflow adds a weekly (Mon 00:00 UTC) Trivy image scan of ghcr.io/blinklabs-io/openvpn:main, extracts the first tag from metadata, emits SARIF, uploads the SARIF to GitHub Security, and grants security-events: write. The Dockerfile switches from ubuntu:24.04 to debian:bookworm-slim, installs openvpn/iptables with --no-install-recommends, purges cron/rsyslog, removes /var/log, and adds a HEALTHCHECK that pgrep's openvpn. bin/entrypoint adds a comment documenting a VPN kill-switch; forwarding rules remain functionally the same. README gains a Privacy and Security Recommendations section and updates the description. Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches✅ Passed checks (2 passed)
✨ Finishing touches🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
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 |
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
README.md (1)
16-52: Add language to fenced config block to satisfy markdownlintThe new recommendations section looks solid. To address MD040 and improve readability, add a language to the OpenVPN config block, e.g.:
-``` +```conf # Disable logging log /dev/null verb 0Dockerfile (1)
1-14: Consider--no-install-recommendsto reduce image footprint and attack surfaceThe Debian base + explicit OpenVPN/iptables install looks good. To minimize extra packages and potential vulnerabilities, consider:
- && apt-get install -y openvpn iptables \ + && apt-get install -y --no-install-recommends openvpn iptables \You can keep the purge/clean steps as-is.
bin/entrypoint (1)
21-24: Kill-switch iptables rules are sensible; consider conntrack match for modern stacksThe default DROP on FORWARD plus explicit tun↔NAT rules give a reasonable container-level kill-switch when NAT is enabled.
As a small modernization and to better align with nftables-backed iptables, consider using
conntrackinstead of the legacystatematch:-iptables -A FORWARD -i ${NAT_DEVICE} -o tun+ -m state --state RELATED,ESTABLISHED -j ACCEPT +iptables -A FORWARD -i ${NAT_DEVICE} -o tun+ -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPTThis keeps behavior while matching current iptables conventions.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/publish.yml(3 hunks)Dockerfile(1 hunks)README.md(2 hunks)bin/entrypoint(1 hunks)
🧰 Additional context used
🪛 GitHub Check: CodeQL
.github/workflows/publish.yml
[warning] 57-57: Unpinned tag for a non-immutable Action in workflow
Unpinned 3rd party Action 'publish' step Uses Step uses 'aquasecurity/trivy-action' with ref 'master', not a pinned commit hash
🪛 markdownlint-cli2 (0.18.1)
README.md
22-22: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
🔇 Additional comments (4)
README.md (1)
3-3: Tagline update is clear and accurateThe new description correctly reflects the image’s purpose and focus on privacy.
Dockerfile (1)
15-15: HEALTHCHECK is reasonable for monitoring OpenVPNUsing
pgrep -f openvpnas a healthcheck is simple and aligns with the single-process model. This is fine given OpenVPN isexec’d from the entrypoint..github/workflows/publish.yml (2)
8-10: Scheduled weekly run is appropriate for recurring security scansThe Monday 00:00 UTC schedule is a good cadence to keep images and vulnerability data fresh without overloading CI.
20-20:security-events: writeis needed for SARIF uploadsGranting
security-events: writeis correct and scoped narrowly to what the CodeQL upload step requires.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
wolf31o2
force-pushed
the
security-improvements
branch
from
ada1eda to
547609c
Compare
wolf31o2
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
Dockerfile (1)
15-15: Refine healthcheck to avoid false positives.The
pgrep -fflag searches the full command line, which could match unintended processes that have "openvpn" in their arguments (e.g., a script namedcheck-openvpn-config.sh).Apply this diff to match only the openvpn process name:
-HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD pgrep -f openvpn || exit 1 +HEALTHCHECK --interval=30s --timeout=10s --start-period=5s --retries=3 CMD pgrep openvpn || exit 1.github/workflows/publish.yml (1)
56-67: Note: Scanning occurs after image push.The vulnerability scan runs after the image has already been pushed to both Docker Hub and GHCR. While this provides visibility into vulnerabilities via the GitHub Security tab, it doesn't prevent vulnerable images from being published. Consider adding a pre-push scan step with severity gating if you want to block vulnerable images from being published.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/publish.yml(3 hunks)Dockerfile(1 hunks)README.md(2 hunks)bin/entrypoint(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- README.md
🔇 Additional comments (4)
bin/entrypoint (1)
21-24: LGTM! Kill-switch implementation is correct.The VPN kill-switch correctly implements a default-deny forwarding policy with explicit allow rules for VPN-related traffic only. The rule order ensures that NAT masquerading is configured first, then the DROP policy is enforced with specific exceptions for tun interfaces.
.github/workflows/publish.yml (2)
8-10: LGTM! Weekly scanning schedule improves security posture.The weekly cron schedule ensures regular vulnerability scanning of the image, which is essential for detecting newly discovered vulnerabilities in dependencies.
20-20: LGTM! Required permission for SARIF upload.The
security-events: writepermission is correctly added to enable SARIF results upload to the GitHub Security tab.Dockerfile (1)
7-8: The OpenVPN version reference in the Dockerfile is accurate.Verification confirms that Debian Bookworm repositories provide OpenVPN version
2.6.3-1+deb12u3, which matches the comment's reference to "latest 2.6.x." The documentation is correct, and no update is needed.
Signed-off-by: Chris Gianelloni <wolf31o2@blinklabs.io>
wolf31o2
force-pushed
the
security-improvements
branch
from
547609c to
fc6fa67
Compare
Summary by cubic
Hardened the OpenVPN image for better security and privacy. Adds a kill switch, healthcheck, a slimmer base image, and automated vulnerability scans.
New Features
Dependencies
Written for commit fc6fa67. Summary will update automatically on new commits.
Summary by CodeRabbit
New Features
Documentation
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.