KEP 2200: Block service ExternalIPs via admission

[thockin]

This KEP proposes to add a surgical admission controller (optional) to
block use of the Service.spec.externalIPs misfeature.

See CVE-2020-8554: Man in the middle using LoadBalancer or ExternalIPs (kubernetes/kubernetes#97110)
for details.

[maplain]

/cc @maplain

[k8s-ci-robot]

@maplain: GitHub didn't allow me to request PR reviews from the following users: maplain.

Note that only kubernetes members and repo collaborators can review this PR, and authors cannot review their own PRs.

In response to this:

/cc @maplain

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

[deads2k]

I would like to see the plugin created with the ability for a cluster-admin (or someone with sufficient privilege) to set this value. You can see an example of how this has been done by other distros here: https://github.com/openshift/kubernetes/blob/9b48f001e5488bbfb1199fe1401dd70bda8ca5b0/openshift-kube-apiserver/admission/network/externalipranger/externalip_admission.go#L159-L165 .

I also think that ranges allowed by that plugin reflect a balance that we found useful over these years, but are slightly less important.

FWIW, that plugin has been in use since 1.2, if we're looking for an implementation that has walked the usability/security balance for a while.

[thockin]

I'm not clear what you are asking for?

I don't think allowing ranges is that useful at the cluster granularity. Nothing stops 2 users from using the same IPs, even if they are in-range.

What are you actually asking for?

[andrewsykim]

Somewhat related to this, there were past discussions in SIG Cloud Provider on having the cloud provider module stand up an admission webhook server as part of cloud-controller-manager and cloud providers would just have to implement a ValidateService() and MutateService() interface method. The main use-cases were to validate port protocols (since some don't support UDP or mixed protocols) and defaulting spec.allocateLoadBalancerNodePorts, but this seems like another useful reason to have it.

This would just be one of many custom webhooks for policy control but seems like an easier/faster way to enable this policy control for users since many clusters already deploy cloud-controller-manager (or will in the future). ExternalIP validation on cloud providerse could be more granular than the proposed admission control here. Would that be a valuable framework to have and do you think we should tackle it in parallel to this? cc @cheftako @cici37 @maplain

[thockin]

I don't think this particular knob - externalIPs is really cloud-provider centric. I don't know how I feel about your proposal overall (would have to think it thru) but I think it is orthogonal to this.

In this case I really just want a big hammer.

[andrewsykim]

I agree it's orthogonal. It wouldn't replace this, just an added alternative to it.

[lavalamp]

Why not change validation instead of making the admission plugin?

[thockin]

That would be a breaking change. Admission is opt-in. We could add a totally different flag to control it, but this seems more appropriate as a plugin amongst the other plugins.

[thockin]

Updated with feedback so far.

[thockin]

Some smallish updates - hope to get this approved in the next weeks.

[thockin]

@lavalamp @johnbelamaric Regarding alpha for a new admission control: I could add a gate that controls this admission control - is that the normal lifecycle? The controller itself is trivial - what is the norm here?

[tallclair]

That seems sort of weird, since it's already opt-in and doesn't have any API associated with it. What is the risk you're trying to mitigate with a staged alpha -> beta -> ga rollout?

[thockin]

I'm just trying to not subvert the existing processes. If we're all comfortable with it, I'd happily skip this. Will add a commit to remove this and if we like, we keep.

[thockin]

I have taken the liberty of marking it implementable because kubernetes/kubernetes#97395 exists

@tallclair PTAL

[tallclair]

No fundamental issues with this, just a bunch of questions.

[tallclair]

An argument against this model for triggering enablement in the past is that it requires k8s providers to surface the configuration option (or enable the protection) for customers. I don't have a strong stance on this in this case, but it's something to call out / consider.

[thockin]

Yeah, with my provider hat on, I am keenly aware of it. In general I would say to avoid this pattern. That said, there are a few factors to consider:

1. This is a pretty egregious misfeature.
2. We do not have an API for dynamically enabling admission controllers. If we want that, I'm game to help design it, but that seems like a different can of worms for providers. Perhaps we want a class of control that isn't as broad as "all the built in admission controls" but is more built-in that "run a webhook". The problem with that right now is that we have a single example (that I know of). If we had 3 or 4 we might want to tackle that. I posit that we can tackle that additively.
3. I'd argue that sooner is better than later. Ideally we;d have had this before we dropped a CVE, but it didn't seem feasible before (not sure why).

I'll add some of this to the text.

[tallclair]

There are others:

• PodSecurityPolicy (deprecated, but likely to be replaced with another controller
• AlwaysPullImages - not sure who is surfacing this today, but there's a lot of requests for it
• ImagePolicy (depends on additonal config)

I agree with the goal of getting this out there, and the admission controller could be a first step towards removing the feature.

[tallclair]

That seems sort of weird, since it's already opt-in and doesn't have any API associated with it. What is the risk you're trying to mitigate with a staged alpha -> beta -> ga rollout?

[tallclair]

Maybe a subset of this bullet, but what about requiring a status update, similar to how load balancer services work? In other words, the externalIP field would be treated as a request, but only the status field would actually be respected by kube-proxy. Then, either a privileged user or controller would need to "approve" the request by setting the status IP?

Probably not worth the trouble for a feature that should just be derpecated.

[thockin]

You expressed my main point - not worth the effort to try to solve as built-in. At least not without real use-cases to justify it. It would still be a breaking change and much more complicated. Very low RoI.

[thockin]

ping to @tallclair

[tallclair]

/lgtm

[annajung]

Hi, forgot to mention, all KEP now requires a production readiness review to be included.

Please don't forget to follow the instruction to include PRR correctly. Specifically, you are required to create a prod-readiness/<sig>/<KEP number>.yaml file, with the PRR approver's GitHub handle for the specific stage.

It doesn't have to be part of this PR if you're planning on submitting another one. thank you!

[johnbelamaric]

PRR looks mostly good, some minor points. Also will need to create a PRR approval request yaml as described in here: https://github.com/kubernetes/community/blob/master/sig-architecture/production-readiness.md#submitting-a-kep-for-production-readiness-approval

Example PR: https://github.com/kubernetes/enhancements/pull/2179/files

That's how we handle PRR approval now so that we can leverage the OWNERS files in the approval process.

[thockin]

I think all fixed.

[johnbelamaric]

/approve for prod readiness

there are a couple tweaks needed in kep.yaml since it's going straight to stable now.

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: johnbelamaric, thockin

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:

• keps/prod-readiness/OWNERS [johnbelamaric]
• keps/sig-network/OWNERS [johnbelamaric,thockin]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[johnbelamaric]

/lgtm