-
Notifications
You must be signed in to change notification settings - Fork 421
Improve firewall to avoid affecting existing iptables rules #328
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
8d3bd66
to
024a339
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks awesome! Thanks for your patience. Excited to get this in.
.devcontainer/init-firewall.sh
Outdated
else | ||
echo "Firewall verification passed - able to reach https://api.github.com as expected" | ||
# Fallback GitHub IPs |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want this? when would we expect the call to /meta to fail? should we checkfail instead of falling back?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got rate limited today. Maybe something like this? #448
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we want this? when would we expect the call to /meta to fail? should we checkfail instead of falling back?
removed
e979f0c
to
b827318
Compare
Please re-request review whenever you're ready 🙏 |
This is looking good. Can you clarify what specific functionality is enabled by the vscode / cursor whitelisting? I don't have a don't have a ton of experience with devcontainers. |
e9d111c
to
4cb64db
Compare
Whitelisting the VSCode and Cursor domains in the devcontainer configuration allows marketplace access from within the containerized development environment. Specifically, when running Claude-code in a remote environment and connecting through VSCode or Cursor, these whitelisted domains (such as marketplace.visualstudio.com and update.code.visualstudio.com) enable the editor to:
Without these whitelisted hosts in your devcontainer.json configuration, the firewall blocks access and prevents the IDE from functioning as intended. For context, I work at Gitpod, where we develop remote environments https://www.gitpod.io/blog/introducing-gitpod-flex |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This commit makes it so that even if we don't pass the eg connectivity checks, the command will succeed (with a warning). This is not what we want for this script.
@@ -33,20 +33,27 @@ execute_cmd() { | |||
return 0 | |||
} | |||
|
|||
# Add IP to allowed list with deduplication |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
duplicate commment
fi | ||
|
||
# Try with iptables but don't fail the script if it doesn't work | ||
if iptables -A CLAUDE_OUTPUT -d "$ip" -j ACCEPT 2>/dev/null; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the failure mode you're trying to address here?
|
||
if [ -n "$download_url" ]; then | ||
log "Found Azure IP ranges download URL: $download_url" | ||
azure_ip_json=$(fetch_with_retry "$download_url") | ||
azure_ip_json=$(fetch_with_retry "$download_url") || true |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is the point of this?
This patch enhances the firewall configuration by adopting a more targeted approach with custom chains (CLAUDE_INPUT, CLAUDE_OUTPUT, CLAUDE_FORWARD) rather than flushing all existing IPtables rules. This strategy prevents disruption to other services, such as Docker, that depend on iptables. The patch additionally introduces support for VS Code and Cursor by incorporating necessary domains and Azure IP ranges for the VS Code Marketplace.
Further improvements include enhanced error handling, more thorough logging, detection of network interfaces, and support for AWS S3 IP ranges. The script now gracefully manages failures instead of exiting completely, making it more resilient in diverse environments.