Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

Move weave-npc checks to top of FORWARD chain #3210

Merged
merged 3 commits into from
Jan 14, 2018
Merged

Conversation

bboreham
Copy link
Contributor

@bboreham bboreham commented Jan 2, 2018

We want our DROP rule to be executed ahead of any ACCEPT rules which might have been added by another program on the host.

Since #3204 gave us the framework to ensure we are recreating every rule, just inserting them all at the top, in order, works nicely.

I also moved the NFLOG and DROP rules from FORWARD to WEAVE-NPC to simplify the logic about which order they come in.

Extend kubernetes test to include the circumstances that trigger the issue.

Fixes #3209

@bboreham bboreham force-pushed the issues/3209-npc-rules-first branch 3 times, most recently from 1a543c6 to 0952194 Compare January 3, 2018 12:05
@bboreham
Copy link
Contributor Author

bboreham commented Jan 3, 2018

https://circleci.com/gh/weaveworks/weave/9969 is a demonstration of the problem
https://circleci.com/gh/weaveworks/weave/9968 is the same test but without the two factors that trigger it - Kubernetes 1.8.5 and --cluster-cidr argument to kube-proxy

@bboreham bboreham force-pushed the issues/3209-npc-rules-first branch from bb7de39 to 553433c Compare January 5, 2018 14:03
@bboreham bboreham changed the title WIP: Extend kubernetes test to include virtual IP Move weave-npc checks to top of FORWARD chain Jan 5, 2018
@bboreham bboreham requested a review from brb January 5, 2018 15:33
@brb
Copy link
Contributor

brb commented Jan 6, 2018

I like the idea of the chain simplification, but as discussed on Slack, moving -j DROP to the WEAVE-NPC chain creates a time window for allowing a traffic which should not be allowed by NetworkPolicy, because the weave-npc process clears the chain each time it starts.

However, we have another time window due to -A WEAVE-NPC -m set ! --match-set weave-local-pods dst -j ACCEPT. But this can be fixed by replacing the rule with -A WEAVE-NPC -m set --match-set weave-local-pods src ! --match-set weave-local-pods dst -j ACCEPT.

I suggest to create another level of indirection - a new chain WEAVE-NPC-FOOBAR which would contain -j WEAVE-NPC, "nflog" and -j DROP rules, and insert -j WEAVE-NPC-FOOBAR at the very top of "filter/FORWARD -o weave".

@bboreham
Copy link
Contributor Author

bboreham commented Jan 8, 2018

But this can be fixed by replacing the rule with -A WEAVE-NPC -m set --match-set weave-local-pods src ! --match-set weave-local-pods dst -j ACCEPT.

The idea of this rule was to allow non-pod traffic; that change would disallow non-pod traffic originating on this machine. Maybe that's a niche case.

net/bridge.go Outdated
}
if err = ipt.AppendUnique("filter", "FORWARD", "-o", config.WeaveBridgeName, "-j", "DROP"); err != nil {
// Use insert rather than append to try to get ahead of any ACCEPT rules which would defeat our policies
if err = insertUnique(ipt, "filter", "FORWARD", 1, "-o", config.WeaveBridgeName, "-j", "WEAVE-NPC"); err != nil {

This comment was marked as abuse.

@brb
Copy link
Contributor

brb commented Jan 11, 2018

This needs rebasing after #3204 has been merged. Happy to do if you are busy.

@bboreham bboreham force-pushed the issues/3209-npc-rules-first branch from 553433c to 5a13611 Compare January 11, 2018 15:29
We want our DROP rule to be executed ahead of any ACCEPT rules which
might have been added by another program on the system.
@bboreham
Copy link
Contributor Author

Rebased and simplified.

@brb brb added this to the 2.2 milestone Jan 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants