-
-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
@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."; }; |
There was a problem hiding this comment.
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"?
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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.
There was a problem hiding this 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."; }; |
There was a problem hiding this comment.
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 ( |
There was a problem hiding this comment.
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"; }; |
There was a problem hiding this comment.
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."; }; |
There was a problem hiding this comment.
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 "") |
There was a problem hiding this comment.
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}"}
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."; }; |
There was a problem hiding this comment.
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; } ]; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 "") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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"]); |
There was a problem hiding this comment.
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"]);
There was a problem hiding this comment.
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"])); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this 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."; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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"]; |
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 = []; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
default = [ ];
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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)...
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ") |
There was a problem hiding this comment.
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 😄)?
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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 { |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!
I would still not make |
@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"; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
).
There was a problem hiding this comment.
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.
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 |
@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:
Anything more complex should use the mechanisms in issue #12940 |
@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.). |
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 whennmap
ping 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
(nix.useSandbox on NixOS,
or option
build-use-sandbox
innix.conf
on non-NixOS)
nix-shell -p nox --run "nox-review wip"
./result/bin/
)