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.

Sign up for GitHub

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

Jump to bottom

Finer-grained firewall: allow specific hosts, in particular to SSH #22586

Closed
wants to merge 25 commits into from
Closed

Finer-grained firewall: allow specific hosts, in particular to SSH #22586

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
6c43d68
Finer-grained firewall: allow specific hosts, allow to disable the ss…
P-E-Meunier Feb 9, 2017
5da578d
Finer-grained firewall rules, second attempt.
P-E-Meunier Feb 9, 2017
2060825
Replacing "if" by "optionalString"
P-E-Meunier Feb 9, 2017
c55880b
Hopefully better style.
P-E-Meunier Feb 9, 2017
4f203bd
Even more style
P-E-Meunier Feb 9, 2017
8aeaf75
Firewall: A few examples
P-E-Meunier Feb 9, 2017
5677c04
Merge branch 'master' of git://github.com/NixOS/nixpkgs
P-E-Meunier Feb 9, 2017
20dc8f1
modules/sshd: firewallOpenPorts -> openInFirewall
P-E-Meunier Feb 9, 2017
0872e73
sshd: did not compile
P-E-Meunier Feb 9, 2017
a3133f9
Merge branch 'master' of git://github.com/NixOS/nixpkgs
P-E-Meunier Feb 10, 2017
d503e79
Firewall: address families (incorrect firewall code)
P-E-Meunier Feb 10, 2017
27ce317
Firewall: string -> str
P-E-Meunier Feb 11, 2017
db8d11e
top-level/rust-packages: update to 2017-02-13
P-E-Meunier Feb 13, 2017
9215385
Merge branch 'master' of git://github.com/NixOS/nixpkgs
P-E-Meunier Feb 13, 2017
77ee320
Finer-grained firewall: allow specific hosts, allow to disable the ss…
P-E-Meunier Feb 9, 2017
ce8f425
Finer-grained firewall rules, second attempt.
P-E-Meunier Feb 9, 2017
fdac4be
Replacing "if" by "optionalString"
P-E-Meunier Feb 9, 2017
e036c45
Hopefully better style.
P-E-Meunier Feb 9, 2017
9504ce9
Even more style
P-E-Meunier Feb 9, 2017
7c6b061
Firewall: A few examples
P-E-Meunier Feb 9, 2017
ea14eac
modules/sshd: firewallOpenPorts -> openInFirewall
P-E-Meunier Feb 9, 2017
7277ca0
sshd: did not compile
P-E-Meunier Feb 9, 2017
b2d7740
Firewall: address families (incorrect firewall code)
P-E-Meunier Feb 10, 2017
72c75e7
Firewall: string -> str
P-E-Meunier Feb 11, 2017
d30cd1b
Merge github.com:P-E-Meunier/nixpkgs
P-E-Meunier Feb 13, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 31 additions & 0 deletions nixos/modules/services/networking/firewall.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,21 @@ let
) cfg.allowedUDPPortRanges
}

# Accept packets on the extra allowed host+port combination.
${concatMapStrings (allowed:
if (allowed.useIPv4 || allowed.usedIPv6) then (
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

allowed.useIpv6 not allowed.usedIpv6. If you use an enum for the address family you can drop this if clause.

Copy link
Member

@edolstra edolstra Feb 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parentheses around "if" conditions are unnecessary, this isn't C.

(if allowed.useIPv4 then (
if allowed.useIPv6 then ("ip46tables -A nixos-fw ") else ("iptables -A nixos-fw ")
) else (
"ip6tables -A nixos-fw "
))
+ (if (allowed.source != "") then (" --source " + allowed.source) else "")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use optionalString and string interpolation in all instances instead of if clauses and +

ex.:

${optionalString (allowed.source != "") " --source ${allowed.source}"}

+ (if (allowed.protocol != "") then (" -p " + allowed.protocol) else "")
+ (if (allowed.dport != 0) then (" --dport " + toString allowed.dport) else "")
+ " -j nixos-fw-accept\n"
) else "") cfg.extraAllowed
}

# Accept IPv4 multicast. Not a big security risk since
# probably nobody is listening anyway.
#iptables -A nixos-fw -d 224.0.0.0/4 -j nixos-fw-accept
Expand Down Expand Up @@ -493,6 +508,22 @@ in
'';
};

networking.firewall.extraAllowed = mkOption {
type = types.listOf (types.submodule { options = {
source = mkOption { type = types.string; default = ""; example = "example.com"; description = "Source of the packets"; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is sourceAddress better here? destinationAddress could be also added. I would also mention that it is possible to specifiy a subnet here.

protocol = mkOption { type = types.enum ["tcp" "udp" "icmp"]; default = ""; example = "tcp"; description = "Protocol used."; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does an empty default do? Note that "" does not match (or should not match, I didn't test) the valid values in the enum. A better type might be nullOr (listOf (enum ["tcp" "udp" "icmp"])), where null means any protocol. This also allows specifying multiple protocols (e.g. ["tcp" "udp"]).

dport = mkOption { type = types.int; default = 0; example = 22; description = "Destination port of the packet."; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I find destinationPort better here. The nixos module should be easier to use then iptables.
sourcePort might be also worth adding.

useIPv4 = mkOption { type = types.bool; default = true; description = "Use the IPv4 address of this host."; };
useIPv6 = mkOption { type = types.bool; default = true; description = "Use the IPv6 address of this host."; };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would use allowIPv4 or allowIPv6 instead of use or make it an enum called addressFamily with the options "ipv6", "ipv4" and "all".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does "Use the IPv4/IPv6 address of this host" mean? Do you mean "Accept IPv4/IPv6 traffic from this host"?

Copy link
Member

@edolstra edolstra Feb 9, 2017 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In fact it's unclear what "host" means. Should that be "from this source"? Can source be a CIDR prefix?

};});
default = [];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

default = [ ];

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I changed that, thanks.

example = [ { source = "example.com"; protocol = "tcp"; dport = 22; } ];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From the iptables manpage:

Hostnames will be resolved once only, before the rule is submitted to the kernel.  Please note that
specifying any name to be resolved with a remote query such as DNS is a really bad idea.

So in order not to give people bad ideas, it would be better to use an IP address in the example. (See https://tools.ietf.org/search/rfc5737)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had not realized that, but this felt wrong indeed.

The reason for the useIPv4 and useIPv6 is that the iptables rules introduced by this pull request might match addresses specified in either IPv4 or IPv6.

Since some hosts don't have an IPv6 yet, or don't have an IPv4 anymore, we need a way to tell whether to call iptables or ip6tables.

description =
''
Additional exceptions in the firewall.
'';
};

};


Expand Down
12 changes: 11 additions & 1 deletion nixos/modules/services/networking/ssh/sshd.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,16 @@ in
'';
};

firewallOpenPorts = mkOption {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

=> openInFirewall?

type = types.bool;
default = true;
description = ''
Specifies whether the ports the SSH daemon listens on must
be enabled in the firewall. Remember to open at least some
of them.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a great change (imho) - related to #19504

For example, enabling a module should not open firewall ports by default.

I don't think we should immediately use default = false but we should make it clear in the description that this will probably happen in the future and that one shouldn't enable this option (i.e. use the firewall module instead).

Additional information can be found in the related issue: #19504

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, as far as I'm concerned, I appreciate the fact that nixops doesn't lock me out of my machine on the first run.

Even though nixops is a great tool after some time, it is not totally easy to master at first (potentially due to lack of documentation on how it works conceptually). I remember learning both nix and nixops at the same time, it wasn't very "fun" (I'm having a lot of fun using it now, though).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@P-E-Meunier Yeah, that's why changing the default is indeed really problematic...

However using services.openssh.listenAddresses with a different port than services.openssh.ports will still lock you out (but obviously you already know that).

Another possible solution would be to use your new option for allowing the (IP, Port) combinations from services.openssh.listenAddresses as well, which would still be against the convention but at least improve the current situation.

@edolstra, @Mic92, @wkennington, #19504 - Any opinions on that?

We could also start using the "services" solution (#19504 (comment) from @Mic92) which would also greatly improve our current situation (imho) but probably require some additional thinking/discussion.

Imho it would also be a good idea to move that option over to the firewall module (e.g. networking.firewall.allowSSH (or allowServices = [ ... ], allowPorts = [ service.openssh.ports ]) - Could probably even be the best solution (idk)...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific as to why it might be useful to combine services.openssh.listenAddresses with this PR?
I must confess I don't really understand how to use services.openssh.listenAddresses: it sends RST packets (revealing that there's an SSH server running on the machine), and it is so easy to lock yourself out by just testing things, without any real way to go back.

In my situation (VPS + nixops), this PR would have enabled me to just logged via a "web console", and type "iptables -F" to re-enable everything, whereas listenAddresses forced me to write a new ssh configuration in the web console (with an incorrect keyboard layout and a terrible lag), and then to try and understand how to start an SSH server in the command line just be able to run nixops again.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you be more specific as to why it might be useful to combine services.openssh.listenAddresses with this PR?

We don't necessarily have to combine it with this PR, but Imho it is a good idea to consider this since you made related modifications inside the openssh module (and at least we have to get the description right!).

Imust confess I don't really understand how to use services.openssh.listenAddresses

It doesn't modify the firewall (atm) so you have to do that manually (I thought this is how you've locked yourself out in the first place).

By default OpenSSH will listen on port 22 on all IP addresses of all interfaces, with this option you could e.g. limit that to [::1]:22 (yes, that example doesn't make much sense...).

it sends RST packets (revealing that there's an SSH server running on the machine)

I assume that RST packet (TCP reset) came from the firewall. Then you should only get those for existing connections (I guess that terminated your running SSH session).

"iptables -F" to re-enable everything, whereas listenAddresses

The change is intended to prevent a user from getting into that situation (not to fix a "broken" firewall) 😉

'';
};

listenAddresses = mkOption {
type = with types; listOf (submodule {
options = {
Expand Down Expand Up @@ -308,7 +318,7 @@ in

};

networking.firewall.allowedTCPPorts = cfg.ports;
networking.firewall.allowedTCPPorts = if cfg.firewallOpenPorts then cfg.ports else [];

security.pam.services.sshd =
{ startSession = true;
Expand Down