aws: fix ICMP ACL #1550
aws: fix ICMP ACL #1550
Conversation
openshift-ci-robot
added
the
size/M
|
/retest |
1 similar comment
|
/retest |
do you have a BZ for broken PMTUD? |
|
This was identified by QE as a bug in https://bugzilla.redhat.com/show_bug.cgi?id=1689857 - they couldn't ping between nodes. That's when I realized that all ICMP is blocked, which is definitely considered a poor practice. |
|
/retest |
|
Funny story - it turns out there were already SG rules that allowed all ICMP, they were just broken. Fixed (and made it more explicit) |
data/data/aws/vpc/sg-master.tf
Outdated
| @@ -31,9 +31,9 @@ resource "aws_security_group_rule" "master_ingress_icmp" { | |||
| security_group_id = "${aws_security_group.master.id}" | |||
|
|
|||
| protocol = "icmp" | |||
| cidr_blocks = ["${data.aws_vpc.cluster_vpc.cidr_block}"] | |||
There was a problem hiding this comment.
can we keep the cidr restricted to vpc ?
There was a problem hiding this comment.
I was matching what was in sg-worker.txt. What's important is that we can receive ICMP from the nlbs. I'm not sure what the source IP will be for that.
(it also looks like the nlb creates an additional rule allowing itself - i need to investigate that)
| to_port = 0 | ||
| cidr_blocks = ["0.0.0.0/0"] | ||
| from_port = -1 | ||
| to_port = -1 |
There was a problem hiding this comment.
The docs for these have>
... or ICMP type number if protocol is "icmp"...
From the IANA registry, it looks like we'd want to allow 8 (Echo) and 0 (Echo Reply). Do we want to block or allow the other types? Personally, I'm in favor of necking this down to the absolute minimum and then opening back up individual types as we have requests for them (e.g. "we want ping support, and that requires types 0 and 8").
|
@wking there is no security benefit to blocking certain types of ICMP, so I don't really see the point in whitelisting. |
Really? Even if existing types are safe (I don't know), what if someone registers a new, risky one? Or a kernel bug turns the handling of some theoretically-safe type into a information leak or whatever. It's a lot harder to audit security without a whitelist. But I'm not a net-security expert, maybe we are comfortable with all ICMP? I'd just like more motivation than a one-line assertion ;). |
There was a typo in the rule that allowed icmp - it accidentally blocked all icmp. Also, fix the ip block to match for both master and workers.
openshift-ci-robot
added
size/S
|
Updated to just fix the existing ICMP security group rule. |
|
/lgtm ... since this is clearly the intention of the previous code. I'm not against opening ICMP more broadly, but I want to make sure we're have some motivation / risk-assessment we can point folks at if we do open it up. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: squeed, wking The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci-robot
added
the
approved
|
@wking I hear what you're saying. I'm not super concerned about this, mostly because none of the nodes have public IPs. That being said, it is possible to write a safe whitelist of ICMP codes. My authoritative source for this is http://shouldiblockicmp.com/ , which publishes a reasonable list of codes to allow (keep in mind, that's for public-facing nodes). We also need to allow "destination unreachable" messages, since those are generated by kube-proxy. |
And in the UPI CloudFormation templates too. We've allowed ICMP ingress for OpenStack since 6f76298 (OpenStack prototype, 2017-02-16, coreos/tectonic-installer#1), which did not motivate the ICMP ingress. Allowing ICMP ingress for AWS dates back to b620c16 (modules/aws: tighten security groups, 2017-04-19, coreos/tectonic-installer#264). The master rule was restricted to the VPC in e7bd29a (modules/aws/vpc - Better security for master nodes, 2017-10-16, coreos/tectonic-installer#2147). And the worker rules was restricted to the VPC in e131a74 (aws: fix ICMP ACL, 2019-04-08, openshift#1550), before which a typo had blocked all ICMP ingress. There are reasons to allow in-cluster ICMP, including Path MTU Discovery (PMTUD) [1,2,3]. Folks also use ping to troubleshoot connectivity [4]. Restricting this to in-cluster security groups will avoid exposing ICMP ports to siblings living in shared VPCs, as we move towards allowing the installer to launch clusters in a pre-existing VPC. It might also block ICMP ingress from our load balancers, where we probably want PMTUD and possibly other ICMP calls. I'm not sure if there's a convenient way to allow access from the load-balancers while excluding it from sibling clusters that share the same VPC, but this commit is my attempt to get that. [1]: http://shouldiblockicmp.com/ [2]: https://tools.ietf.org/html/rfc1191 [3]: https://tools.ietf.org/html/rfc8201 [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1689857#c2
And in the UPI CloudFormation templates too. We've allowed ICMP ingress for OpenStack since 6f76298 (OpenStack prototype, 2017-02-16, coreos/tectonic-installer#1), which did not motivate the ICMP ingress. Allowing ICMP ingress for AWS dates back to b620c16 (modules/aws: tighten security groups, 2017-04-19, coreos/tectonic-installer#264). The master rule was restricted to the VPC in e7bd29a (modules/aws/vpc - Better security for master nodes, 2017-10-16, coreos/tectonic-installer#2147). And the worker rules was restricted to the VPC in e131a74 (aws: fix ICMP ACL, 2019-04-08, openshift#1550), before which a typo had blocked all ICMP ingress. There are reasons to allow in-cluster ICMP, including Path MTU Discovery (PMTUD) [1,2,3]. Folks also use ping to troubleshoot connectivity [4]. Restricting this to in-cluster security groups will avoid exposing ICMP ports to siblings living in shared VPCs, as we move towards allowing the installer to launch clusters in a pre-existing VPC. It might also block ICMP ingress from our load balancers, where we probably want PMTUD and possibly other ICMP calls. I'm not sure if there's a convenient way to allow access from the load-balancers while excluding it from sibling clusters that share the same VPC, but this commit is my attempt to get that. [1]: http://shouldiblockicmp.com/ [2]: https://tools.ietf.org/html/rfc1191 [3]: https://tools.ietf.org/html/rfc8201 [4]: https://bugzilla.redhat.com/show_bug.cgi?id=1689857#c2
There was a typo in the rule that allowed icmp - it accidentally blocked all icmp.
Also, fix the ip block to match for both master and workers.
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1689857