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 6 commits
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
92 changes: 92 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 @@ -200,6 +200,24 @@ let
ip6tables -A nixos-fw -d fe80::/64 -p udp --dport 546 -j nixos-fw-accept
''}


# Accept packets on the extra allowed host+port combination.
${concatMapStrings (allowed:
concatMapStrings (addressFamily:
concatMapStrings (protocol:
(if addressFamily == "IPv4" then "iptables -w -A nixos-fw "
else "ip6tables -w -A nixos-fw ")
+ (lib.optionalString (allowed.sourceAddress != null) (" --source " + allowed.sourceAddress))
+ (lib.optionalString (allowed.destAddress != null) (" --dest " + allowed.destAddress))
+ (lib.optionalString (protocol != null) (" -p " + protocol))
+ (lib.optionalString (allowed.dport != null) (" --dport " + toString allowed.dport))
+ (lib.optionalString (allowed.sport != null) (" --sport " + toString allowed.sport))
+ " -j nixos-fw-accept\n"
) (if allowed.protocols != null then allowed.protocols else [null])
) allowed.addressFamily
) cfg.extraAllowed
}

${cfg.extraCommands}

# Reject/drop everything else.
Expand Down Expand Up @@ -493,6 +511,80 @@ in
'';
};

networking.firewall.extraAllowed = mkOption {
type = types.listOf (types.submodule { options = {
sourceAddress = mkOption {
type = types.nullOr types.string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is the choice of types.string deliberate? Usually str is preferred for plain strings.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not. Why is str preferred? What's the difference?

Copy link
Contributor

@joachifm joachifm Feb 10, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

string has merge semantics that may differ from what you expect. If two modules declare values for an option foo, the final value is the concatenation of the two values, so if module A sets it to "foo" and B sets it to "bar" it'd end up as "foobar" in the final config object.

Copy link

Choose a reason for hiding this comment

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

Thanks for the explanation, I just fixed it!

default = null;
example = "198.51.100.0";
description =
''
Source of the packets. According to the iptables manual,
DNS host names might be used here, but their use is
discouraged (in particular because this delegates the
security rule to DNS servers). DNS host names are resolved
only when the rule is loaded.
'';
};
destAddress = mkOption {
type = types.nullOr types.string;
default = null;
example = "198.51.100.0";
description =
''
Destination of the packets. Same comment as for the
`sourceAddress` parameter.
'';
};
protocols = mkOption {
type = types.nullOr (types.listOf (types.enum
[ "tcp" "udp" "icmp" "udplite" "esp" "ah" "sctp" ]
));
default = null;
example = [ "tcp" ];
description = "Protocol used.";
};
sport = mkOption {
type = types.nullOr types.string;
default = null;
example = "32219";
description =
''
Source port of the packet. This might be either an
integer port number, or the name of a protocol, such
as "http" or "ssh".
'';
};
dport = mkOption {
type = types.nullOr types.string;
default = null;
example = "22";
description =
''
Destination port of the packet. This might be either an
integer port number, or the name of a protocol, such as
"http" or "ssh".
'';
};
addressFamily = mkOption {
type = types.listOf (types.enum [ "ipv4" "ipv6" ]);
default = ["ipv4"];
description =
''
The address family of the sourceAddress and destAddress
parameters. If these addresses use a DNS lookup (despite
recommendations), make sure the host has addresses in
this family.
'';
};
};});
default = [ ];
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