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

Firewall: Add support for arbitrary rules for input, output and forward #12940

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@kampfschlaefer
Contributor

kampfschlaefer commented Feb 11, 2016

This extends the firewall to support arbitrary rules for the INPUT, OUTPUT and
FORWARD chains. All the rules are placed in nixos-fw-[input|output|forward]
chains. It also adds support to define the default policies of the chains.

Logging works as special target. When hits for a rule should be logged, this
rule has to be defined twice, once with the LOG target and once with the actual
action. Or you just rely on logRejectedPackets.

The old nixos-fw chain is deleted first. This is only really needed when
switching to this new feature-branch and activiting the config the first time.
But for this one case its needed as otherwise the accept and refuse chains
can't be deleted. Also delete the invalid chain to allow reload to work.

For debugging its now possible to run only the ipv6 tests or only the ipv4
tests. Maybe this should even be extracted into a parameter before including
this in nixos.

@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Feb 11, 2016

By analyzing the blame information on this pull request, we identified @wkennington, @edolstra and @bluescreen303 to be potential reviewers

mention-bot commented Feb 11, 2016

By analyzing the blame information on this pull request, we identified @wkennington, @edolstra and @bluescreen303 to be potential reviewers

@fpletz

This comment has been minimized.

Show comment
Hide comment
@fpletz

fpletz Feb 12, 2016

Member

Thanks a lot for this! I'm willing to review & test this soon if nobody beats me to it.

Member

fpletz commented Feb 12, 2016

Thanks a lot for this! I'm willing to review & test this soon if nobody beats me to it.

Show outdated Hide outdated nixos/modules/services/networking/firewall.nix
default = null;
type = types.nullOr (
types.addCheck types.str (
v: v == "ACCEPT" || v == "DROP" || v == "REJECT"

This comment has been minimized.

@copumpkin

copumpkin Feb 12, 2016

Member

why not an enum here? Also, isn't LOG an option sometimes?

@copumpkin

copumpkin Feb 12, 2016

Member

why not an enum here? Also, isn't LOG an option sometimes?

This comment has been minimized.

@kampfschlaefer

kampfschlaefer Feb 12, 2016

Contributor

yeah, enum sounds like the word I didn't remember when writing this ;-)

LOG would be the action to just log, nothing else, right?

Also I think having DROP and REJECT both could be seen as superfluous as the actual action depends on the global rejectPackets option. (Current difference between DROP and REJECT is that the first sends to nixos-fw-refuse while the later sends to nixos-fw-log-refuse.
Maybe these choices here shouldn't be these abstract names but the real chain names (without the nixos-fw-prefix)?

@kampfschlaefer

kampfschlaefer Feb 12, 2016

Contributor

yeah, enum sounds like the word I didn't remember when writing this ;-)

LOG would be the action to just log, nothing else, right?

Also I think having DROP and REJECT both could be seen as superfluous as the actual action depends on the global rejectPackets option. (Current difference between DROP and REJECT is that the first sends to nixos-fw-refuse while the later sends to nixos-fw-log-refuse.
Maybe these choices here shouldn't be these abstract names but the real chain names (without the nixos-fw-prefix)?

This comment has been minimized.

@kampfschlaefer

kampfschlaefer Feb 12, 2016

Contributor

@copumpkin

Also, isn't LOG an option sometimes?

What would you think is better:

rule = [
  { fromInterface = "eth0"; target = "LOG"; log-prefix="bla: "; log-level = "info"; }
  { fromInterface = "eth0"; target = "ACCEPT"; }
];

with two rule-entries to explicitly LOG and ACCEPT the connection/packet? Or

rule = [
  { fromInterface = "eth0"; target = "ACCEPT"; log-prefix="bla: "; log-level = "info"; }
];

to have one rule-entry and create two iptables rules with log and accept implicit. Is there a need for logging without any special ACCEPT/DROP/REJECT handling?

@kampfschlaefer

kampfschlaefer Feb 12, 2016

Contributor

@copumpkin

Also, isn't LOG an option sometimes?

What would you think is better:

rule = [
  { fromInterface = "eth0"; target = "LOG"; log-prefix="bla: "; log-level = "info"; }
  { fromInterface = "eth0"; target = "ACCEPT"; }
];

with two rule-entries to explicitly LOG and ACCEPT the connection/packet? Or

rule = [
  { fromInterface = "eth0"; target = "ACCEPT"; log-prefix="bla: "; log-level = "info"; }
];

to have one rule-entry and create two iptables rules with log and accept implicit. Is there a need for logging without any special ACCEPT/DROP/REJECT handling?

@abbradar

This comment has been minimized.

Show comment
Hide comment
@abbradar

abbradar Feb 12, 2016

Member

I vote for transparent conversion of allowed ports into rules -- I like them for simplicity (and no need for one to understand iptables).

Member

abbradar commented Feb 12, 2016

I vote for transparent conversion of allowed ports into rules -- I like them for simplicity (and no need for one to understand iptables).

@kampfschlaefer kampfschlaefer changed the base branch from release-15.09 to master Sep 28, 2016

@kampfschlaefer kampfschlaefer changed the title from [WIP, RFC] Firewall: Add support for rules for input, output and forward to Firewall: Add support for arbitrary rules for input, output and forward Sep 29, 2016

@Mic92

This comment has been minimized.

Show comment
Hide comment
@Mic92

Mic92 Oct 20, 2016

Contributor

(triage)

Contributor

Mic92 commented Oct 20, 2016

(triage)

@spacekitteh

This comment has been minimized.

Show comment
Hide comment
@spacekitteh

spacekitteh Nov 4, 2016

Contributor

@grahamc security

Contributor

spacekitteh commented Nov 4, 2016

@grahamc security

firewall: Add support for arbitrary rules for iptables
This extends the firewall to support arbitrary rules for the INPUT, OUTPUT and
FORWARD chains. All the rules are placed in nixos-fw-[input|output|forward]
chains. It also adds support to define the default policies of the chains.

Logging works as special target. When hits for a rule should be logged, this
rule has to be defined twice, once with the LOG target and once with the actual
action. Or you just rely on logRejectedPackets.

The old nixos-fw chain is deleted first. This is only really needed when
switching to this new feature-branch and activiting the config the first time.
But for this one case its needed as otherwise the accept and refuse chains
can't be deleted. Also delete the invalid chain to allow reload to work.

For debugging its now possible to run only the ipv6 tests or only the ipv4
tests. Maybe this should even be extracted into a parameter before including
this in nixos.
@primeos

Hey, thank you very much for the PR, it contains some awesome stuff (imho) 😄

However, imho it's a bit too late to get this right for 17.3 and would suggest to aim for 17.9 instead (unfortunately at least I don't have the time for a proper review atm and it's a pretty complex change anyway).

There is also an issue (and PR as I just noticed) for switching to nftables and I'd personally aim for creating an abstraction layer (should require nixifying the rules, but even that's probably not enough...) so that we can use iptables, nftables, etc. as "back-ends" (I'll probably create an issue for that, but that has to wait for at least one weak as I don't have the time atm).

(Please note that the review is incomplete, just wanted to note some stuff until I or someone else has the time to do a proper review.)

FORWARD = "nixos-fw-forward";
PREROUTING = "PREROUTING";
POSTROUTING = "POSTROUTING";
};

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member

It would be great if you could document these new rules at the beginning of the file.

@primeos

primeos Feb 12, 2017

Member

It would be great if you could document these new rules at the beginning of the file.

getTargetForPolicy = policy:
if policy == "ACCEPT"
then "nixos-fw-accept"
else "nixos-fw-refuse";

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member

This seems a bit odd (without looking at the rest). If I hear policy I'd expect the following:

Only built-in (non-user-defined) chains can have policies, and neither built-in nor user-defined chains can be policy targets.

@primeos

primeos Feb 12, 2017

Member

This seems a bit odd (without looking at the rest). If I hear policy I'd expect the following:

Only built-in (non-user-defined) chains can have policies, and neither built-in nor user-defined chains can be policy targets.

ip46tables -D INPUT -j nixos-fw-input 2> /dev/null || true
ip46tables -D OUTPUT -j nixos-fw-output 2> /dev/null || true
ip46tables -D FORWARD -j nixos-fw-forward 2> /dev/null || true
for chain in nixos-fw nixos-fw-accept nixos-fw-input nixos-fw-output nixos-fw-forward nixos-fw-accept nixos-fw-log-refuse nixos-fw-refuse __invalid_chain__ FW_REFUSE; do

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member
  • nixos-fw-accept appears twice
  • You can drop FW_REFUSE (see 460b43d (was after your PR))
  • I don't really like the following name: __invalid_chain__ (that's just my personal opinion tho)
@primeos

primeos Feb 12, 2017

Member
  • nixos-fw-accept appears twice
  • You can drop FW_REFUSE (see 460b43d (was after your PR))
  • I don't really like the following name: __invalid_chain__ (that's just my personal opinion tho)
# Accept packets from established or related connections.
ip46tables -A nixos-fw-input -m conntrack --ctstate ESTABLISHED,RELATED -j nixos-fw-accept
ip46tables -A nixos-fw-forward -m conntrack --ctstate ESTABLISHED,RELATED -j nixos-fw-accept
ip46tables -A nixos-fw-output -m conntrack --ctstate ESTABLISHED,RELATED -j nixos-fw-accept

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member

I wouldn't touch forward by default (probably set the policy to DROP - but that could break some setups).

I assume new connections are handled below, if not we need the connection state NEW as well.

@primeos

primeos Feb 12, 2017

Member

I wouldn't touch forward by default (probably set the policy to DROP - but that could break some setups).

I assume new connections are handled below, if not we need the connection state NEW as well.

input = mkOption {
default = "DROP";
type = types.enum targetTypes;
description = "";

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member

An empty description seems a bit odd, even if we probably don't need it.

@primeos

primeos Feb 12, 2017

Member

An empty description seems a bit odd, even if we probably don't need it.

'';
};
networking.firewall.rules = mkOption {

This comment has been minimized.

@primeos

primeos Feb 12, 2017

Member

Nixifying the FW rules seems like a great idea (imho).
However #22586 does that as well (IIRC), i.e. we have to compare/merge them.

@primeos

primeos Feb 12, 2017

Member

Nixifying the FW rules seems like a great idea (imho).
However #22586 does that as well (IIRC), i.e. we have to compare/merge them.

@ocharles

This comment has been minimized.

Show comment
Hide comment
@ocharles

ocharles Feb 12, 2017

Member

As this has been open for a year (and @kampfschlaefer would certainly like at least a resolution on this PR), ccould we put it in the 17.09 milestone?

Member

ocharles commented Feb 12, 2017

As this has been open for a year (and @kampfschlaefer would certainly like at least a resolution on this PR), ccould we put it in the 17.09 milestone?

@primeos

This comment has been minimized.

Show comment
Hide comment
@primeos

primeos Feb 26, 2017

Member

Please don't merge this - it conflict's with #22586 and there hasn't been any discussion yet!

Just wanted to make sure anyone is aware of this as I can't see how we (currently) can merge them both. I encourage anyone to compare them and decide which one we use (or we could probably use parts from both). I don't really have much time ATM but I created the following issue to share some of my thought/ideas: #23181

Member

primeos commented Feb 26, 2017

Please don't merge this - it conflict's with #22586 and there hasn't been any discussion yet!

Just wanted to make sure anyone is aware of this as I can't see how we (currently) can merge them both. I encourage anyone to compare them and decide which one we use (or we could probably use parts from both). I don't really have much time ATM but I created the following issue to share some of my thought/ideas: #23181

@kampfschlaefer kampfschlaefer deleted the kampfschlaefer:feature/improve_firewalling branch Aug 29, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment