Disable externalIP by default #7810

smarterclayton · 2016-03-05T05:05:09Z

Allow IP to be set if the admin sets a CIDR. No controls yet on who can
set out of the range. Could be on a limitranger.

Fixes #7808

@derekwaynecarr @deads2k @knobunc this closes the potential security hole for online

smarterclayton · 2016-03-05T05:05:25Z

[test]

smarterclayton · 2016-03-05T05:16:52Z

This should go upstream for 1.3

deads2k · 2016-03-07T12:49:32Z

We built a way to configure admission plugins, why wouldn't we choose to use it for this?

deads2k · 2016-03-07T12:53:21Z

pkg/cmd/server/api/types.go

@@ -325,6 +325,9 @@ type MasterNetworkConfig struct {
 	ClusterNetworkCIDR string
 	HostSubnetLength   uint
 	ServiceNetworkCIDR string
+	// ExternalIPNetworkCIDR controls what values are acceptable for the service external IP field. If empty, no externalIP


That's a strange way to deal with empty string and it makes it impossible to default to existing behavior, doesn't it?

If you moved this to the admission config section, then no config means "allow all" (existing behavior) and config with empty string can mean "restrict all". That seems to be more intuitive.

ExternalIP is a fundamental behavior of the system - by default today it is
insecure. Config per admission control is modeled correctly for admission
controllers that are not a core part of the platform, but is not the right
model for usability or discoverability when it's a core platform feature.
The place admins will look for this control is in the MasterNetworkConfig
because it is comparable to ServiceCIDR and ClusterCIDR, defining the
acceptable range. We will eventually move this into a flag on apiserver
and do the same validation, when we do, the config should come from our
core object, not an admission controller.

ExternalIP is a security risk, and we cannot ship with it enabled by
default, therefore we must disable by default, and existing users will be
broken. We cannot allow through inaction for users to have a vulnerability
here. Release notes will be updated
openshift/openshift-docs#1700

On Mon, Mar 7, 2016 at 6:53 AM, David Eads notifications@github.com wrote:

In pkg/cmd/server/api/types.go
#7810 (comment):

@@ -325,6 +325,9 @@ type MasterNetworkConfig struct {
ClusterNetworkCIDR string
HostSubnetLength uint
ServiceNetworkCIDR string

// ExternalIPNetworkCIDR controls what values are acceptable for the service external IP field. If empty, no externalIP

That's a strange way to deal with empty string and it makes it impossible
to default to existing behavior, doesn't it?

If you moved this to the admission config section, then no config means
"allow all" (existing behavior) and config with empty string can mean
"restrict all". That seems to be more intuitive.

—
Reply to this email directly or view it on GitHub
https://github.com/openshift/origin/pull/7810/files#r55198749.

ExternalIP is a security risk, and we cannot ship with it enabled by
default, therefore we must disable by default, and existing users will be
broken. We cannot allow through inaction for users to have a vulnerability
here. Release notes will be updated
openshift/openshift-docs#1700

@liggitt @sdodson We'll probably get questions about this.

@sosiouxme Can you add a diagnostic check that looks at this setting, the openshift level, and the current services to find ones that violate this? We can't do it client-side because we don't know the CIDR client-side.

@smarterclayton Be careful with this implementation. As written, I think you'll make any controller that updates status end up having to deal with rejections. Also, this makes wiring external services hard for end users because they don't know ahead of time what the CIDR is. It also means we can't show them problems in oc status.

knobunc · 2016-03-07T14:34:15Z

I really like the purpose of the patch, but @deads2k raises some good points.

eparis · 2016-03-07T17:27:19Z

ExternalIPCIDR as a concept makes 0 sense. These are IP addresses assigned to hosts. The idea that some 'range' is valid is non-nonsensical...

While I agree that we should prevent normal users from using externalIPs by default, this is exactly the same problem as hostNetwork. And while this is a different object, I think it should be solved the same way....

eparis · 2016-03-07T17:49:55Z

Let me elaborate on the 'makes 0 sense'. These are IP addresses that exist on the REAL network and are assigned to the REAL hardware (or virtual hardware). Unlike the other concepts which are sets of addresses that WE define, apply, and manage virtually.

A list of ExternalIPCIDRs might make some sense. (Or a list/CIDR per node might make sense). Such a solution would reduce the impact of ExternalIP services getting in the way of command and control traffic.

But has little likehood to benefit HostNetwork implications. The same IPs that you would want/expect HostNetwork pods to be able to use are (except maybe in an amazing contrived enviornment) the same IPs that ExternalIP services are going to want to use. I don't see how we can ever make that 'better'...

None of this changes the fact that we should disable ExternalIP by default for everyone...

smarterclayton · 2016-03-08T01:49:04Z

@deads2k updated with validation and slightly broadened tests

smarterclayton · 2016-03-08T03:41:43Z

This is a new one

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/12419/console

@mrunalp, related to our cgroup thing or something new?

mrunalp · 2016-03-08T04:20:39Z

@smarterclayton This one may not be directly related even though it looks like a timing issue. It could even be a kernel bug :/ I will look into it further.

mrunalp · 2016-03-08T04:22:34Z

Related upstream issue moby/moby#17653

mrunalp · 2016-03-08T04:29:04Z

Looking some more, this is happening on systemd enabled systems. I will follow up with systemd devs to help figure out why this could be happening.

deads2k · 2016-03-08T13:57:39Z

@smarterclayton I'd like to see a response to #7810 (comment). The restriction of pointing a service to something external seems reasonable on the face of it, but why not a list as he suggests? Reading his comment actually made me wonder if we want a blacklist as well.

deads2k · 2016-03-08T14:01:23Z

pkg/service/admission/externalip_admission.go

+
+// Admit determines if the service should be admitted based on the configured network CIDR.
+func (r *externalIPRanger) Admit(a kadmission.Attributes) error {
+	if a.GetResource() != kapi.Resource("services") {


I don't think you want to match on subresources. We should really update the Handler to do what we want.

smarterclayton · 2016-03-08T16:50:40Z

I'm fine with a list, and maybe blacklist, the original goal was to keep
the implementation small enough that we don't delay 3.2 unnecessarily. I
don't want to leave out the possibility someone is using externalIP for
routing to hosts (or asks us to do so because they are doing it on Kube).

On Tue, Mar 8, 2016 at 7:57 AM, David Eads notifications@github.com wrote:

@smarterclayton https://github.com/smarterclayton I'd like to see a
response to #7810 (comment)
#7810 (comment).
The restriction of pointing a service to something external seems
reasonable on the face of it, but why not a list as he suggests? Reading
his comment actually made me wonder if we want a blacklist as well.

—
Reply to this email directly or view it on GitHub
#7810 (comment).

smarterclayton · 2016-03-15T19:52:08Z

Made it a slice of strings, some of which can have !

deads2k · 2016-03-16T12:55:18Z

pkg/cmd/server/api/types.go

@@ -347,6 +347,11 @@ type MasterNetworkConfig struct {
 	ClusterNetworkCIDR string
 	HostSubnetLength   uint
 	ServiceNetworkCIDR string
+	// ExternalIPNetworkCIDRs controls what values are acceptable for the service external IP field. If empty, no externalIP
+	// may be set. It may contain a comma-delimited list of CIDRs which are checked for access. If a CIDR is prefixed with !,


"comma-delimited"? Bad comment? Looks like a slice.

deads2k · 2016-03-16T13:00:52Z

Description has some stale information. lgtm otherwise.

smarterclayton · 2016-03-16T14:20:24Z

Updated, [merge]

Allow IP to be set if the admin sets a CIDR. No controls yet on who can set out of the range. Could be on a limitranger.

openshift-bot · 2016-03-16T15:05:34Z

Evaluated for origin test up to 290ade0

openshift-bot · 2016-03-16T16:36:29Z

continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2195/)

openshift-bot · 2016-03-16T18:45:49Z

continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2195/) (Image: devenv-rhel7_3750)

openshift-bot · 2016-03-16T18:45:50Z

Evaluated for origin merge up to 290ade0

smarterclayton · 2016-03-16T18:53:33Z

[merge]

On Wed, Mar 16, 2016 at 12:36 PM, OpenShift Bot notifications@github.com
wrote:

continuous-integration/openshift-jenkins/test SUCCESS (
https://ci.openshift.redhat.com/jenkins/job/test_pr_origin/2195/)

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#7810 (comment)

Merged by openshift-bot

smarterclayton force-pushed the secure_external_ip branch from 17d2acd to c7e2803 Compare March 5, 2016 05:16

smarterclayton added this to the 1.2.0 milestone Mar 5, 2016

smarterclayton added area/security priority/P1 component/networking labels Mar 5, 2016

smarterclayton assigned knobunc Mar 7, 2016

deads2k reviewed Mar 7, 2016
View reviewed changes

smarterclayton force-pushed the secure_external_ip branch from c7e2803 to 1424e2c Compare March 8, 2016 01:46

deads2k reviewed Mar 8, 2016
View reviewed changes

smarterclayton force-pushed the secure_external_ip branch 2 times, most recently from 5451f23 to 8ef1624 Compare March 15, 2016 16:16

deads2k reviewed Mar 16, 2016
View reviewed changes

smarterclayton force-pushed the secure_external_ip branch from 8ef1624 to 1f89870 Compare March 16, 2016 14:20

Disable externalIP by default

290ade0

Allow IP to be set if the admin sets a CIDR. No controls yet on who can set out of the range. Could be on a limitranger.

smarterclayton force-pushed the secure_external_ip branch from 1f89870 to 290ade0 Compare March 16, 2016 15:04

openshift-bot pushed a commit that referenced this pull request Mar 17, 2016

Merge pull request #7810 from smarterclayton/secure_external_ip

cd3e8a6

Merged by openshift-bot

openshift-bot merged commit cd3e8a6 into openshift:master Mar 17, 2016

chadswen mentioned this pull request Dec 7, 2020

Figure out what to do about external IPs kubernetes/kubernetes#97110

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disable externalIP by default #7810

Disable externalIP by default #7810

smarterclayton commented Mar 5, 2016

smarterclayton commented Mar 5, 2016

smarterclayton commented Mar 5, 2016

deads2k commented Mar 7, 2016

deads2k Mar 7, 2016

smarterclayton Mar 7, 2016

deads2k Mar 8, 2016

knobunc commented Mar 7, 2016

eparis commented Mar 7, 2016

eparis commented Mar 7, 2016

smarterclayton commented Mar 8, 2016

smarterclayton commented Mar 8, 2016

mrunalp commented Mar 8, 2016

mrunalp commented Mar 8, 2016

mrunalp commented Mar 8, 2016

deads2k commented Mar 8, 2016

deads2k Mar 8, 2016

smarterclayton commented Mar 8, 2016

smarterclayton commented Mar 15, 2016

deads2k Mar 16, 2016

deads2k commented Mar 16, 2016

smarterclayton commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

smarterclayton commented Mar 16, 2016

Disable externalIP by default #7810

Disable externalIP by default #7810

Conversation

smarterclayton commented Mar 5, 2016

smarterclayton commented Mar 5, 2016

smarterclayton commented Mar 5, 2016

deads2k commented Mar 7, 2016

deads2k Mar 7, 2016

Choose a reason for hiding this comment

smarterclayton Mar 7, 2016

Choose a reason for hiding this comment

deads2k Mar 8, 2016

Choose a reason for hiding this comment

knobunc commented Mar 7, 2016

eparis commented Mar 7, 2016

eparis commented Mar 7, 2016

smarterclayton commented Mar 8, 2016

smarterclayton commented Mar 8, 2016

https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/12419/console

mrunalp commented Mar 8, 2016

mrunalp commented Mar 8, 2016

mrunalp commented Mar 8, 2016

deads2k commented Mar 8, 2016

deads2k Mar 8, 2016

Choose a reason for hiding this comment

smarterclayton commented Mar 8, 2016

smarterclayton commented Mar 15, 2016

deads2k Mar 16, 2016

Choose a reason for hiding this comment

deads2k commented Mar 16, 2016

smarterclayton commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

openshift-bot commented Mar 16, 2016

smarterclayton commented Mar 16, 2016