Skip to content
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

Vulnerability: Don't add generic ACCEPT rules to the filter chain #112

Merged
merged 1 commit into from
Feb 15, 2021

Conversation

SerialVelocity
Copy link
Contributor

#106 added these rules under the assumption that Kilo should automatically allow traffic going from/to the pods.

What this actually did was break all firewalls that are "accept explicit, drop implicit" style and allow all traffic through the firewall.

If a user has configured their firewall to be drop by default, they should add rules like these manually if they want to allow all traffic.

This also breaks network policy controllers that are written/configured this way.

@squat
Copy link
Owner

squat commented Feb 15, 2021

thanks for raising this @SerialVelocity. This point of view sounds totally reasonable to me. This was introduced to mirror the behavior of Flannel (https://github.com/coreos/flannel/blob/a20edd2cc99b5dbcc2e9b612e8b263b4be92e868/network/iptables.go#L83-L84). I wonder if it's worth raising an issue there as well.

@SerialVelocity
Copy link
Contributor Author

This was originally added to Flannel because of the docker change that broke users so to try and keep backwards compatibility, they decided to add those rules.

Flannel actually added a flag to disable this feature because it broke another set of people:
flannel-io/flannel#938

Since Kilo doesn't need to be backwards compatible with Docker 1.13, I would suggest having users add these rules themselves if they need it and not touch the filter FORWARD chain as you currently don't add any rules there (especially since Docker is deprecated in Kubernetes).

@squat
Copy link
Owner

squat commented Feb 15, 2021

ACK thanks for the context, I wasn't aware of the Docker deal

@squat squat merged commit 4ae1ccf into squat:master Feb 15, 2021
@squat
Copy link
Owner

squat commented Feb 15, 2021

🎉 thanks @SerialVelocity for bringing this to light! 🌮

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants