-
Notifications
You must be signed in to change notification settings - Fork 28
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
Add permanent kill switch and redirect stderr and stdout for up check #41
base: master
Are you sure you want to change the base?
Conversation
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.
Thanks so much for this valuable contribution. ❤️
Left some comments for minor changes, and a question or two that I'm unsure about.
Please also bump the version in the Dockerfile to 0.5.0 in all these places:
❯ ag 0.4
test/docker_secrets_test/test.sh:3:#docker build -t walt3rl/proton-privoxy:0.4.2-dev ../..
Dockerfile:3:LABEL version="0.4.2"
test/docker_secrets_test/docker-compose.yml:4: image: walt3rl/proton-privoxy:0.4.2-dev
.github/workflows/docker.yml:39: walt3rl/proton-privoxy:0.4.2
iif lo accept | ||
|
||
# Allow new, established, and related traffic | ||
ct state new,established,related accept |
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.
Why do we want to allow incoming connections?
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.
The first line can be removed; I left it for future testing purposes.
The second line is to initially connect with privoxy; otherwise no incoming connection can be made except outgoing from within the proxy. Try replacing with iif eth0 skuid privoxyu accept
, but I think the functionality won't change significantly.
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.
I understand what this does, but not clear on why we would and to allow incoming connections at all. The connection to setup the VPN is outgoing, as is all connections using the VPN, right?
In fact, shouldn't the new
on this line be added to the corresponding output
rule? Otherwise no new connection can be made from this container.
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.
Yes, outgoing connections can already be made as proton0 allows it, but to connect to privoxy as a proxy incoming connection of some form has to be allowed; otherwise, it's not a proxy, but a direct VPN connection from within. I recommend testing your edge cases and letting me know.
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.
Right. That makes sense, but it still seems odd that all incoming connections are allowed, and no outgoing ones. I'll test it out a bit.
Co-authored-by: Walter <[email protected]>
linux-cli-community may be unmantained. Resolves #5 by adding package
iptables
.Adds experimental kill-switch separate from the cli to container. Permanent kill switch is set to default. The killswitch is ip-agnostic relying on processes and interfaces(eth0 and proton0) and supports ipv6 from using nftables.
Below was the initial nfttable configure file but took space using sets even when size of 65535 is sufficient above 128, it would hinder configurability. Privoxy also recommends not to run as root. Finally manually enabling the cli killswitch may cause unexpected results.
Not tested in swarm mode.
Minor change was from seeing #36 and preventing some redundant output.