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

Enable conntrack in FORWARD #29

Merged
merged 1 commit into from
Dec 3, 2020
Merged

Conversation

keachi
Copy link
Collaborator

@keachi keachi commented Nov 29, 2020

This PR will enable conntrack for forwarding packages. This is mostly used for router traffic.
This extends #14.

duritong
duritong previously approved these changes Nov 29, 2020
@duritong
Copy link
Collaborator

@nbarrientos any reason why you removed the FORWARD conntrack in #14 or just missed it? Mainly, as you added explicit tests to not contain the forward conntrack rules by default.

@nbarrientos
Copy link
Collaborator

Hi @duritong

any reason why you removed the FORWARD conntrack in #14 or just missed it? Mainly, as you added explicit tests to not contain the forward conntrack rules by default.

It was on purpose. Perhaps the most prominent use-case for this module is machines where all the traffic they see on their interfaces is for the host itself (pure servers), hence having rules added to FORWARD by default didn't sound like a good default. To me people using the module on boxes that route traffic is a more marginal use case that could be covered by either rules added externally by the administrator via nftables::rule or perhaps by a module parameter as you're proposing, however I wouldn't set true as default value for it. If this patch is accepted we'll certainly set it centrally to false in our deployment. My opinion, of course :)

@keachi
Copy link
Collaborator Author

keachi commented Nov 30, 2020

Hi @nbarrientos,
Enabling conntrack doesn't allow any traffic, except there was a rule which enabled the "new" connection. But if conntrack is missing, a lot of connections won't work as expected, and even more if we will change the rules to only allow ct state new in the future.
In my opinion the benefit is bigger if we allow traffic, which is already established, than the user must allow it explicitly.

@nbarrientos
Copy link
Collaborator

Could you please provide an example of the connections that won't work as expected if forwarded traffic part of an established connection is not allowed in a typical server environment?

@duritong
Copy link
Collaborator

duritong commented Nov 30, 2020

I agree with @nbarrientos that in typical server environments FORWARD is most likely not relevant and it actually follows our principle of the core design of the module, that by default it should be as locked down as possible. So having something in place that might eventually be used is not really following that principle.

So imho what should happen: By default it should not be allowed, since no forward is needed by default. If we add rules to forward, then connection tracking should be set up, so that established flows work.

But maybe @keachi you have an example in mind, where it makes sense.

@keachi
Copy link
Collaborator Author

keachi commented Dec 2, 2020

We don't open anything with it, as there is now rule which allows new traffic. But I changed the fwd_conntrack to false by default.

@keachi keachi merged commit e3c56ff into voxpupuli:master Dec 3, 2020
@keachi keachi deleted the fwd_conntrack branch December 3, 2020 08:48
@traylenator traylenator added the enhancement New feature or request label Dec 10, 2020
figless pushed a commit to figless/puppet-nftables that referenced this pull request Aug 25, 2021
e3c56ff Merge pull request voxpupuli#29 from keachi/fwd_conntrack
24a5a2a Enable conntrack in FORWARD

git-subtree-dir: code
git-subtree-split: e3c56ff
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants