rather than switching to a redirect, it seems less disruptive to require the selected endpoint address to actually refer to a real pod in the same namespace with a matching IP

At that point, require the selected Address to have a targetRef, look up the pod, make sure it exists and has a matching IP. Something like:

// Pick a random address.
addr := ss.Addresses[rand.Intn(len(ss.Addresses))]

if noExternalAddresses {
	ns := genericapirequest.NamespaceFrom(ctx)
	if addr.TargetRef == nil {
		return nil, nil, errors.NewServiceUnavailable("...")
	}
	if addr.TargetRef.Kind != "Pod" {
		return nil, nil, errors.NewServiceUnavailable("...")
	}
	if addr.TargetRef.Namespace == "" || addr.TargetRef.Namespace != ns {
		return nil, nil, errors.NewServiceUnavailable("...")
	}
	pod, err := rs.pods.GetPod(ctx, addr.TargetRef.Name)
	if err != nil {
		return nil, nil, errors.NewServiceUnavailable("...")
	}
	if pod.Status.PodIP != addr.IP {
		return nil, nil, errors.NewServiceUnavailable("...")
	}
}

ip := addr.IP
port := int(ss.Ports[i].Port)
return &url.URL{
	Scheme: svcScheme,
	Host:   net.JoinHostPort(ip, strconv.Itoa(port)),
}, rs.proxyTransport, nil

@liggitt thanks for the feedback, I'll address the readiness and port/scheme comments.

Regarding redirect vs. validation, my concern is that as it is currently constructed, the proxy is enabling the end user to cloak their requests with the APIServer's IP address. That makes me fairly uncomfortable from an audit/network ACL perspective.

The only valid way to access the /proxy endpoint is via web sockets/spdy right shouldn't every web sockets/spdy client be able to handle a redirect?

Also, in a world where masters are network partitioned from workers, except for master -> kubelet communication, requiring masters to be able to directly communicate with Service/Pod IPs opens another potentially vulnerable network path.

The only valid way to access the /proxy endpoint is via web sockets/spdy right shouldn't every web sockets/spdy client be able to handle a redirect?

No, any http request is valid, and many http libraries don't follow redirects automatically.

the proxy is enabling the end user to cloak their requests with the APIServer's IP address. That makes me fairly uncomfortable from an audit/network ACL perspective.

if that is the issue, then this PR doesn't really help... it's just redirecting to the pods/proxy API endpoint, which still goes through the API server and cloaks the request with the APIServer's IP address, right?

The difference is that the pod proxy path tunnels things to the kubelet rather than communicating with the IP address directly. My concern isn't around losing the client IP address (that is going to happen either way as you suggest) but my concern is that the request is coming from the APIServer's IP address which is much more likely to have special permissions in network configuration rules.

Also, since the path to the kubelet is an HTTP call, there's a much better audit trail than making a random IP request from the APIServer's network namespace.

Basically, as currently written this code enables an arbitrary pod to appear to be traffic from the API server, that doesn't make me super comfortable.

also wrt to clients following redirects, sure, that's true of course, but if your client isn't handling redirects, you're probably already in a world of hurt (e.g. OAuth relies on redirects), so it doesn't seem that bad to me to say "if you want proxy, you need to handle redirects" Proxy isn't core functionality imho, so adding some limitations doesn't seem that bad.

The difference is that the pod proxy path tunnels things to the kubelet rather than communicating with the IP address directly.

Actually, the service proxy subresource communicates with the IP the same way the pod proxy subresource does (see the transport it is given to use, which is the same transport the pod proxy uses). Requiring the resolved IP to be a pod IP seems sufficient to me, and gets you identical protection, without changing behavior to redirect.

I'll take a look at the transport, if it is indeed tunnelling to the kubelet, then I agree that validation seems reasonable to me.

Hrm, as far as I read they both just use the default networking. I don't really like either of those... I swear the pod proxy tunneled to kubelet sigh I agree it's no worse that what would happen with a redirect to the Pod, so I'll just do the Pod IP validation...

But I think we should also change the default so that we don't drive from the APIServer's IP address...

@liggitt re-factored as you suggested to just filter based on Pod TargetRef.

Please take another look.

/retest

/retest

Name is fine with me, are there others who need to approve?

Not sure. Is the intent to deprecate and default off this "feature" eventually (I'd be in favor of that), leaving the feature gate as an escape hatch for people who still wanted this behavior during the deprecation period? If so, I wonder if we'd want the feature gate to sound more perjorative.

Just noticed the release-note isn't accurate... it's not defaulted off yet.

Yes, my plan is to eventually remove the "feature" since it is a bug imho, but I want to leave an escape hatch for at least one release for people whom it breaks.

I believe that the release note is correct.

The feature is "allow external ips" the default for features is false afaik, and thus the note:

By default disable access to external IP addresses from the apiserver service proxy.

Appears to be correct?

Yes, my plan is to eventually remove the "feature" since it is a bug imho

I'd agree

The feature is "allow external ips" the default for features is false afaik, and thus the note:

By default disable access to external IP addresses from the apiserver service proxy.

Appears to be correct?

Ah, you're right, though I think you need to add it to the defaultKubernetesFeatureGates so it can actually be set. Not sure what the designation is... sort of need a "DEPRECATED" state.

@liggitt added to defaultKubernetesFeatureGates default false. Added a new section for 'backwards compatability' features.

ptal.

Thanks

thanks

/lgtm

/retest

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, liggitt

Associated issue: #58761

The full list of commands accepted by this bot can be found here.

/test all [submit-queue is verifying that this PR is safe to merge]

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

I don't agree this is a bug and in fact it's a feature I depend on.
After you deprecate ServiceProxyAllowExternalIPs in 1.11, then what is the alternative replacement for this feature?

1). Does this feature block an External Service(or IP), that I am trying to access such as my database that is outside the kubernetes cluster.

2). Also, if we are trying to protect the access of services, present in another namespace. Cannot the same result be achieved by NetworkPolicy objects or even RBAC/ABAC for that matter, instead of introducing the above feature in this PR ?