Packet: improve bootstrap iptables rules #202

invidian · 2020-03-20T07:52:43Z

This PR further locks down bootstrap iptables rules. The disadvantage though, which we need to consider is, that if user updates some of those CIDRs and rely on them, then Calico rules are flushed for whatever reason, the original rules will be applied (from node creation time), unless the node is replaced during the process. We need to make sure, that this is the behavior we want.

Alternatively, changing CIDRs would require node upgrade rollout we plan to implement.

Refs #137 #8

rata

@invidian regarding the management CIDR, I'm not sure what to do. Both options (using it or leave port 22 open to the world) seem wrong, and the only reason we need to do that is because of bootkube :-/.

I'd move propose to do something safe: move forward with the other iptables changes (i.e. not the ones using management_cidr) and I'd leave the management cidr changes for another PR.

What do you think?

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/cl/controller.yaml.tmpl

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/controllers.tf

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/calico-host-protection.yaml.tmpl

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/workers/workers.tf

rata · 2020-05-12T15:25:11Z

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/workers/cl/worker.yaml.tmpl

-          -A INPUT -p tcp --dport 10256 -j ACCEPT
+          %{ for management_cidr in management_cidrs ~}
+          -A INPUT -s ${management_cidr} -p tcp --dport 22 -j ACCEPT
+          %{~ endfor }


I don't think ssh is needed for workers before they are running calico (out of band console can be used if needed). What do you think?

It seems reasonable, though opinionated, so it should be documented. If calico fails to start for some reason, it makes it more difficult to debug. What do you think @kinvolk/team-cloud?

Hm, maybe port 22 should also be allowed from node_private_cidr?

IMO we want to be able to SSH into workers at all times, and especially during failures.

I don't think we want to allow SSH from node_private_cidr. This increases the nodes' attack surface (very theoretically) but mainly I don't find it useful. My guess is you're suggesting this to allow using a bastion host (right?), in which case I would include the bastion's /32 address in management_cidrs because it's a management CIDR.

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/cl/controller.yaml.tmpl

johananl

I've added some inline comments.

Regarding management CIDRs:

Like @rata, I'm ambivalent regarding restricting "fallback rules" to the management CIDRs. Here is why:

These rules are "fallback rules" whose purpose is to allow the k8s control plane to function during bootstrap and failure scenarios without being completely exposed to the outside world. The policy which would be enforced most of the time is the one enforced by Calico.
Management CIDRs change frequently, and this is why I think changing them should entail a k8s API operation only. Putting management CIDRs in Ignition means one of the following:

We have to replace all the nodes to update management CIDRs.
We create a mismatch between the GNP-based policy and the fallback policy assuming CIDRs change over time.

While I do see value in not exposing SSH to the entire internet, I don't see a serious security threat there since brute-forcing private SSH keys is impractical. The main threat in having SSH without filtering sources AFAICT is a new zero day CVE. Given that the fallback policy should only apply during very limited time windows, I lean towards not filtering by management CIDRs.

One last thought: maybe we can achieve a state where the iptables rules injected by Calico stay in place when Calico dies: rather than falling back to the bootstrap policy, we "freeze" the Calico policy until Calico is converged again. Once Calico is converged, it's possible that the policy be updated by Calico if GNP objects were modified in the meantime. Have we checked that Calico-injected rules indeed disappear during failure scenarios?

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/calico-host-protection.yaml.tmpl

johananl · 2020-05-20T11:25:31Z

assets/lokomotive-kubernetes/packet/flatcar-linux/kubernetes/workers/cl/worker.yaml.tmpl

-          -A INPUT -p tcp --dport 10256 -j ACCEPT
+          %{ for management_cidr in management_cidrs ~}
+          -A INPUT -s ${management_cidr} -p tcp --dport 22 -j ACCEPT
+          %{~ endfor }


IMO we want to be able to SSH into workers at all times, and especially during failures.

I don't think we want to allow SSH from node_private_cidr. This increases the nodes' attack surface (very theoretically) but mainly I don't find it useful. My guess is you're suggesting this to allow using a bastion host (right?), in which case I would include the bastion's /32 address in management_cidrs because it's a management CIDR.

This commit changes the template method we use in Packet Terraform files where possible, from template_file data source coming from 3rd party Terraform provider to built-in 'templatefile' function, which is available from Terraform 0.12, as it provides the exact same functionality, but do not require downloading 3rd party provider. Also 'template' provider recommends using this function: https://www.terraform.io/docs/providers/template/d/file.html. Part of #196 Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

If there is no management_cidrs specified, the template produces empty line after 'managementCIDRs:' line, which shouldn't be there. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

invidian · 2020-05-25T08:10:01Z

Addressed all review feedback, please have a look again.

johananl

LGTM

rata

LGTM. However, I'd add a comment saying why we use 10.0.0.0/8 as that is a trck not obvious for most readers not very involved with Packet networking internals or any new comer.

Other than the simple comment, LGTM :)

This commit adds extra filtering for cluster-internal ports, so even if we listen on all interfaces, those ports won't be accessible from the internet, but only from Packet private CIDR. Later on, Calico should further tighten those rules. SSH port 22 stays accesible from all addresses on purpose, to allow eventual debugging if provisioning fails. We use 10.0.0.0/8 as this is Packet private network CIDR. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

invidian · 2020-05-25T14:44:42Z

LGTM. However, I'd add a comment saying why we use 10.0.0.0/8 as that is a trck not obvious for most readers not very involved with Packet networking internals or any new comer.

Make sense, done.

rata

LGTM. Thanks again! :)

iaguis

LGTM

invidian force-pushed the invidian/firewall-improvements branch from 9ca8bf1 to 549a850 Compare March 20, 2020 10:27

invidian mentioned this pull request May 12, 2020

Tighten firewall rules during node boot window #8

Closed

rata reviewed May 12, 2020

View reviewed changes

johananl reviewed May 20, 2020

View reviewed changes

invidian added 2 commits May 25, 2020 09:37

packet: fix formatting management CIDRs values for host protection

c1f7ea9

If there is no management_cidrs specified, the template produces empty line after 'managementCIDRs:' line, which shouldn't be there. Signed-off-by: Mateusz Gozdek <mateusz@kinvolk.io>

invidian force-pushed the invidian/firewall-improvements branch from 549a850 to 009b169 Compare May 25, 2020 08:09

invidian requested review from johananl and rata May 25, 2020 08:09

johananl previously approved these changes May 25, 2020

View reviewed changes

rata previously approved these changes May 25, 2020

View reviewed changes

invidian dismissed stale reviews from rata and johananl via 257ed36 May 25, 2020 14:44

invidian force-pushed the invidian/firewall-improvements branch from 009b169 to 257ed36 Compare May 25, 2020 14:44

invidian requested review from johananl and rata May 25, 2020 14:44

rata approved these changes May 25, 2020

View reviewed changes

invidian requested a review from iaguis May 25, 2020 15:37

iaguis approved these changes May 25, 2020

View reviewed changes

invidian merged commit 2962730 into master May 25, 2020

invidian deleted the invidian/firewall-improvements branch May 25, 2020 17:01

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Packet: improve bootstrap iptables rules #202

Packet: improve bootstrap iptables rules #202

invidian commented Mar 20, 2020

rata left a comment

rata May 12, 2020

invidian May 13, 2020

invidian May 13, 2020

johananl May 20, 2020

johananl left a comment •

edited

Loading

johananl May 20, 2020

invidian commented May 25, 2020

johananl left a comment

rata left a comment

invidian commented May 25, 2020

rata left a comment

iaguis left a comment

Packet: improve bootstrap iptables rules #202

Packet: improve bootstrap iptables rules #202

Conversation

invidian commented Mar 20, 2020

rata left a comment

Choose a reason for hiding this comment

rata May 12, 2020

Choose a reason for hiding this comment

invidian May 13, 2020

Choose a reason for hiding this comment

invidian May 13, 2020

Choose a reason for hiding this comment

johananl May 20, 2020

Choose a reason for hiding this comment

johananl left a comment • edited Loading

Choose a reason for hiding this comment

johananl May 20, 2020

Choose a reason for hiding this comment

invidian commented May 25, 2020

johananl left a comment

Choose a reason for hiding this comment

rata left a comment

Choose a reason for hiding this comment

invidian commented May 25, 2020

rata left a comment

Choose a reason for hiding this comment

iaguis left a comment

Choose a reason for hiding this comment

johananl left a comment •

edited

Loading