Bug 1996689: Tighten up RestrictedEndpointsAdmission #899

danwinship · 2021-08-23T13:33:14Z

We prevent users from creating Endpoints that point into the cluster or service networks (or to the cloud metadata server, or to the MCS), but we don't currently prevent the creation of EndpointSlices in the same way.

[EDIT] OK, so actually project admins don't normally have EndpointSlice edit permission anyway. So this part isn't needed to prevent the CVE. However, it does make it so that if the admin chooses to give someone EndpointSlice edit permission, then it will work like the existing Endpoints edit permission; they can make any modification except adding restricted endpoints. The new e2e test checks this.

Also, we were failing to block updates to NotReadyEndpoints in Endpoints, which could potentially be used for a CVE-2021-25740-like attack in the future.

openshift-ci · 2021-08-23T13:33:21Z

@danwinship: This pull request references Bugzilla bug 1996689, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @wangke19

In response to this:

Bug 1996689: Make RestrictedEndpointsAdmission restrict EndpointSlices as well

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.

openshift-ci-robot · 2021-08-23T13:33:21Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

3fd7dc9|Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message

openshift-ci-robot · 2021-08-23T14:28:27Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

3cd80ee|Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message
3fd7dc9|Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message

openshift-ci-robot · 2021-08-23T18:45:13Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

5034fde|UPSTREAM: : Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message
e96648f|UPSTREAM: : Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/testdata/cluster-roles.yaml

openshift-ci-robot · 2021-08-26T14:27:57Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

a390318|UPSTREAM: : Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message
bf58b20|UPSTREAM: : Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message

openshift-ci-robot · 2021-08-26T17:25:21Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

5887943|UPSTREAM: : Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message
60268c4|UPSTREAM: : Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message

openshift-ci-robot · 2021-08-26T19:44:54Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

28bdde9|UPSTREAM: : Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message
bbbcdfe|UPSTREAM: : Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message

danwinship · 2021-08-26T19:46:35Z

/hold cancel
This works:

ordinary users can edit endpoints as long as they don't add illegal IPs to them
ordinary users can't edit endpointslice, as with upstream
EndpointsController has unrestricted endpoints permission and EndpointSliceController has unrestricted endpointslice permission

aojea · 2021-08-26T20:16:31Z

openshift-kube-apiserver/admission/network/restrictedendpoints/endpoint_admission.go

+
+func (r *restrictedEndpointsAdmission) sliceFindRestrictedIP(slice *discovery.EndpointSlice, restricted []*net.IPNet) error {
+	for _, endpoint := range slice.Endpoints {
+		for _, addr := range endpoint.Addresses {


do you want to check the EndpointConditions to see if the endpoint is Ready?

Why? If it has restricted IPs, then it's restricted, regardless of conditions

because the endpoints code only checks the ready ones?

it was curiosity only, the endpoints code only parses for _, addr := range subset.Addresses , it doesn't parse subsets.NotReadyAddresses

hm... that seems more like a bug in the Endpoints version 😯
(it doesn't currently matter since we don't point any services to the NotReadyAddresses, but someone might use them for something in the future.)

yeah, as I've said, it was mostly curiosity,

as commented elsewhere, adding NotReadyAddresses validation is a technically a breaking change. If we want that, then we need a good reason, e.g. a CVE.

(it doesn't currently matter since we don't point any services to the NotReadyAddresses, but someone might use them for something in the future.)

I've talked with Stefan and we shouldn't add it because if someone have created it before, after upgrade the endpoint will become invalid and as you say it seems only cosmetical, sorry for the noise #899 (comment)

commented in the other thread; it's part of CVE-2021-25740; upstream solved that CVE by blocking all Endpoints modifications, we are blocking it with a more surgical blocking. But previously we had missed part of it

…dresses

openshift-ci-robot · 2021-08-27T13:33:26Z

@danwinship: the contents of this pull request could not be automatically validated.

The following commits could not be validated and must be approved by a top-level approver:

5ea3080|UPSTREAM: : Make RestrictedEndpointsAdmission restrict EndpointSlices as well: does not specify an upstream backport in the commit message
7e00427|UPSTREAM: : Revert "Remove Endpoints write access from aggregated edit role": does not specify an upstream backport in the commit message
f346309|UPSTREAM: : Make RestrictedEndpointsAdmission check NotReadyAddresses: does not specify an upstream backport in the commit message

danwinship · 2021-08-27T17:20:52Z

/retest

danwinship · 2021-08-28T17:35:06Z

/retest-required

plugin/pkg/auth/authorizer/rbac/bootstrappolicy/controller_policy.go

openshift-ci · 2021-08-31T12:24:08Z

@danwinship: This pull request references Bugzilla bug 1996689, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @wangke19

In response to this:

Bug 1996689: Tighten up RestrictedEndpointsAdmission

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.

abhat · 2021-08-31T15:35:47Z

/lgtm

abhat · 2021-08-31T17:35:57Z

/test e2e-gcp-upgrade

wallylewis · 2021-09-01T00:44:50Z

/test e2e-gcp-upgrade

aojea · 2021-09-01T07:59:18Z

/retest
/lgtm
It seems all comments has been addressed, the discussion about NotReadyAddresses I agree with Dan, it's a potential security hole with low risk of API break (:crossed_fingers: )

sttts · 2021-09-01T16:13:38Z

/approve

openshift-ci · 2021-09-01T16:16:07Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhat, aojea, danwinship, sttts

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:

~~DOWNSTREAM_OWNERS~~ [sttts]

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

openshift-bot · 2021-09-01T22:26:44Z

/retest-required