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

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

wants to merge 25 commits into from

Conversation

P-E-Meunier
Copy link
Contributor

@P-E-Meunier P-E-Meunier commented Feb 9, 2017

Motivation for this change

I'm running nixos on a VPS, which I intend to use as a public-facing machine. Because my SSH setup is not particularly simple (I'm running OpenSSH on some port, and providing a different service using the SSH protocol on port 22), I'd like to connect to OpenSSH only through a proxy.

I tried to use openssh.listenAdresses, but locked myself out while trying (which was not pleasant to repair), and the resulting OpenSSH was sending REJECT packets anyway when nmapping my machine.

This change allows to allow specific hosts on specific ports. It also allows one to disable the OpenSSH ports from the outside. There is certainly a small risk of being locked out, but it is repaired with a single iptables -F.

Things done
  • Tested using sandboxing
    (nix.useSandbox on NixOS,
    or option build-use-sandbox in nix.conf
    on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • Linux
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Fits CONTRIBUTING.md.

@mention-bot
Copy link

@P-E-Meunier, thanks for your PR! By analyzing the history of the files in this pull request, we identified @edolstra, @wkennington and @primeos to be potential reviewers.

protocol = mkOption { type = types.enum ["tcp" "udp" "icmp"]; default = ""; example = "tcp"; description = "Protocol used."; };
dport = mkOption { type = types.int; default = 0; example = 22; description = "Destination port of the packet."; };
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.

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

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?

@@ -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

@edolstra edolstra Feb 9, 2017

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.

Copy link
Member

@Mic92 Mic92 left a comment

Choose a reason for hiding this comment

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

I see the need of the added options here. I added some comments regarding naming options.

protocol = mkOption { type = types.enum ["tcp" "udp" "icmp"]; default = ""; example = "tcp"; description = "Protocol used."; };
dport = mkOption { type = types.int; default = 0; example = 22; description = "Destination port of the packet."; };
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".

@@ -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.

@@ -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.

type = types.listOf (types.submodule { options = {
source = mkOption { type = types.string; default = ""; example = "example.com"; description = "Source of the packets"; };
protocol = mkOption { type = types.enum ["tcp" "udp" "icmp"]; default = ""; example = "tcp"; description = "Protocol used."; };
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.

) 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}"}

@Mic92
Copy link
Member

Mic92 commented Feb 9, 2017

oh, I reviewed with @edolstra in parallel.

networking.firewall.extraAllowed = mkOption {
type = types.listOf (types.submodule { options = {
source = mkOption { type = types.string; default = ""; example = "example.com"; description = "Source of the packets"; };
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"]).

useIPv6 = mkOption { type = types.bool; default = true; description = "Use the IPv6 address of this host."; };
};});
default = [];
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.

concatMapStrings (protocol:
(if addressFamily == "IPv4" then "iptables -A nixos-fw "
else "ip6tables -A nixos-fw ")
+ (if allowed.sourceAddress != null then (" --source " + allowed.sourceAddress) else "")
Copy link
Member

Choose a reason for hiding this comment

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

Using lib.optionalString to conditionally interpolate is best practice in nixpkgs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

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"]);
Copy link
Member

@Mic92 Mic92 Feb 9, 2017

Choose a reason for hiding this comment

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

All instances of types.enum I found, use were lowercase. So I think it would be consistent to keep that

type = types.listOf (types.enum ["ipv4" "ipv6"]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

description = "Destination of the packets. Same comment as for the `sourceAddress` parameter.";
};
protocols = mkOption {
type = types.nullOr (types.listOf (types.enum ["tcp" "udp" "icmp"]));
Copy link
Member

@Mic92 Mic92 Feb 9, 2017

Choose a reason for hiding this comment

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

From the manpage of iptables regarding protocols flag:

The protocol of the rule or of the packet to check. The specified protocol can be one of tcp, udp, udplite, icmp, esp, ah, sctp or the special keyword "all", or it can be a numeric value, representing one of these protocols or a different one. A protocol name from /etc/protocols is also allowed. A "!" argument before the protocol inverts the test. The number zero is equivalent to all. "all" will match with all protocols and is taken as default when this option is omitted.

I think we cannot use a enum here without being too restrictive.

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 agree that adding udplite, icmp, esp, ah, sctp seems like a good idea. According to the same manpage, "all" is implied when this option is omitted, and this is already implemented (besides, "all" doesn't really have the same "type" as other protocols, this might be confusing, what should happen if I say ["tcp" "all"]?).

I'm not sure about including things from /etc/protocols, how is that file generated on nixos? I guess root is able to change manually, right?
Then how does this not breaks referential transparency?

Copy link
Member

@Mic92 Mic92 Feb 12, 2017

Choose a reason for hiding this comment

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

@P-E-Meunier currently /etc/protocols is a file part of iana_etc. User might extend or replace it.

Copy link
Member

@primeos primeos left a comment

Choose a reason for hiding this comment

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

Hey, I'm so sorry about all the (probably annoying) coding-style requests... - I'm probably a bit too picky about that but I'd really like it if we could be consistent with the rest of the module 😄

type = types.nullOr types.string;
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.";
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 please wrap all long lines to about 72 characters? Thanks 😄

Also this module is using two spaces between sentences (GNU/Emacs/19th Century (American) typist/typewriter convention) would be great if we could keep that consistent 😄

(I personally wouldn't mind if we would get rid of this, since I assume only a few people (mostly Emacs/Spacemacs users) are used to this, but keeping it would be simpler and less likely to start a rebellion :D)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. OTOH, some emacs users (including myself) are also used to mode and formatting tools that apply style guidelines automatically…

protocols = mkOption {
type = types.nullOr (types.listOf (types.enum ["tcp" "udp" "icmp"]));
default = null;
example = ["tcp"];
Copy link
Member

Choose a reason for hiding this comment

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

example = [ "tcp" ]; would be consistent with the rest of the module (this applies to e.g. [ "tcp" "udp" "icmp" ] as well).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done, thanks.

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 = [];
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.

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) 😉

sport = mkOption {
type = types.nullOr types.string;
default = null;
example = null;
Copy link
Member

Choose a reason for hiding this comment

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

Imho using a different example than the value of the default would be better.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

concatMapStrings (addressFamily:
concatMapStrings (protocol:
(if addressFamily == "IPv4" then "iptables -A nixos-fw "
else "ip6tables -A nixos-fw ")
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 please add the -w option to iptables and ip6tables (just to be safe 😄)?

Copy link
Member

Choose a reason for hiding this comment

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

Oh, and could we move that part to the end of the "static" stuff (imho right above ${cfg.extraCommands} would be the best place)?

That way reading the output of e.g. iptables -nvL would be easier (the default rules first since they normally don't change and then the custom stuff).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is indeed a good point. I'm changing it.

@@ -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?

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

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!

@Mic92
Copy link
Member

Mic92 commented Feb 12, 2017

I would still not make protocols an enum but make it dynamic because the list of possible values is open ended and too large. For instance I use gre and ospf, which is not in this list.

@danbst
Copy link
Contributor

danbst commented Feb 20, 2017

@Mic92 your use case might be covered by @ericsagnes work on extensible enums (for example #20271)

sha256 = "1dklswbf3acfqid4vv7g2xpqc4gswjgh4ih9xjx3a0m3a69cw9lb";
version = "2017-02-13";
rev = "1e63b42f967d6bb3338526c3570bf81049153702";
sha256 = "17w79fic6ndb724z7zywm1cisha362bjvs8sbvv740nrvsya41gy";
Copy link
Member

Choose a reason for hiding this comment

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

Can you remove these unrelated commits again? A different branch for this work would be ideal.

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'm not sure how to do that, I realised these commits were there after pushing :(

Copy link
Member

@Mic92 Mic92 Feb 26, 2017

Choose a reason for hiding this comment

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

You can checkout a new branch:

$ git checkout firewall

and then use git-rebase:

git rebase -i 6c43d68ff0498dacb75f99ba487fbb7160c68fd5

This will open an editor. Each line is a commit and you can remove unrelated commits by deleting lines. You can also squash commits into the previous by replacing pick with squash at the begin of the line.

Copy link
Member

@primeos primeos Feb 26, 2017

Choose a reason for hiding this comment

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

You can "delete" your last commit by executing git reset --hard 72c75e795ac86333d9aadcd8ba934bfc06cfe08e 😉

I guess you caused that merge commit by running git pull in that case you can run git pull --rebase (while this PR is open) to avoid that the next time (or use git fetch instead but that's a bit more complicated). And for your next PR I would suggest that you create a new branch (i.e. don't use your master branch) - this doesn't really matter but normally it will make things easier for you.

Edit (forgot to mention): After that reset command you have to use the --force option to push (e.g. git push --force).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! After trying these hints out, they look like they would require me to dive into git command-line wizardry.
I'll definitely come back to this when I have more time, or start a new pull request altogether, now that I know what I wanted to do in the first place.

@primeos
Copy link
Member

primeos commented Feb 26, 2017

Please don't merge this - it conflict's with #12940 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

@alexanderkjeldaas
Copy link
Contributor

@primeos I think specifying both full iptables rules as well as supporting CIDR-style fw rules should be supported.

I don't think the two PRs are in conflict as they are at two different abstraction levels.

However, regarding this PR, I think the abstraction presented is slightly too complex. It should be to be able to represent a firewall for incoming/outgoing connections to services mostly.

I think the rules used on AWS is a reasonable abstraction, see https://aws.amazon.com/blogs/aws/new-descriptions-for-security-group-rules/

Thus:

  • Separate inbound and outbound rules
  • Inbound rules are what people will usually care about, so that simplifies documentation
  • For inbound rules, don't allow specifying sport, it's very rarely needed. Or as a minimum, use port to designate the local service port.
  • Likewise for outbound rules, we only care about the local service port in this abstraction. Thus no dport and sport, only port is used.
  • Don't specify ipv4 or ipv6. This should be derived from the syntax for the host/cidr.

Anything more complex should use the mechanisms in issue #12940

@primeos
Copy link
Member

primeos commented Jul 28, 2020

@alexanderkjeldaas uh, sorry about my participation here. I don't remember this PR (only vaguely that I used to try to improve the firewalling situation on NixOS) but scrolling through the rest of this PR it looks like I did a bad job at reviewing it (requesting a lot of nitpick changes first and then suddenly raising concerns about conflicts with another PR - I hope that I at least had an explanation for that at the time... :o). Sorry if this was frustrating for anyone.

Personally I had high hopes for improving the firewalling situation but eventually gave up due to too many edge cases, backends (iptables vs. nftables), design decisions, etc. (e.g. atomic firewall updates would be nice but unfortunately that would break a lot of tools that interfere with the firewall like Docker, LXD, libvirt, ...). Therefore I'm also not sure if trying to implement a complex firewall module is a good idea and feasible to maintain.

Anyway, feel free to just ignore what I said here ;) IIRC I haven't really looked back at our firewalling situation since then and I'm not familiar with our current situation (implementation/PRs/plans/issues/etc.).

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

Successfully merging this pull request may close these issues.

10 participants