Bug 1996689: Tighten up RestrictedEndpointsAdmission #899
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,6 +12,7 @@ import ( | |
| "k8s.io/apiserver/pkg/authorization/authorizer" | ||
| "k8s.io/klog/v2" | ||
| kapi "k8s.io/kubernetes/pkg/apis/core" | ||
| "k8s.io/kubernetes/pkg/apis/discovery" | ||
|
|
||
| "github.com/openshift/library-go/pkg/config/helpers" | ||
| "k8s.io/kubernetes/openshift-kube-apiserver/admission/network/apis/restrictedendpoints" | ||
|
|
@@ -110,37 +111,56 @@ var ( | |
| } | ||
| ) | ||
|
|
||
| func checkRestrictedIP(ipString string, restricted []*net.IPNet) error { | ||
| ip := net.ParseIP(ipString) | ||
| if ip == nil { | ||
| return nil | ||
| } | ||
| for _, net := range restricted { | ||
| if net.Contains(ip) { | ||
| return fmt.Errorf("endpoint address %s is not allowed", ipString) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func checkRestrictedPort(protocol kapi.Protocol, port int32, restricted []kapi.EndpointPort) error { | ||
| for _, rport := range restricted { | ||
| if protocol == rport.Protocol && port == rport.Port { | ||
| return fmt.Errorf("endpoint port %s:%d is not allowed", protocol, port) | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) endpointsFindRestrictedIP(ep *kapi.Endpoints, restricted []*net.IPNet) error { | ||
| for _, subset := range ep.Subsets { | ||
| for _, addr := range subset.Addresses { | ||
| if err := checkRestrictedIP(addr.IP, restricted); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| for _, addr := range subset.NotReadyAddresses { | ||
| if err := checkRestrictedIP(addr.IP, restricted); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) endpointsFindRestrictedPort(ep *kapi.Endpoints, restricted []kapi.EndpointPort) error { | ||
| for _, subset := range ep.Subsets { | ||
| for _, port := range subset.Ports { | ||
| if err := checkRestrictedPort(port.Protocol, port.Port, restricted); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) endpointsCheckAccess(ctx context.Context, attr admission.Attributes) (bool, error) { | ||
| authzAttr := authorizer.AttributesRecord{ | ||
| User: attr.GetUserInfo(), | ||
| Verb: "create", | ||
|
|
@@ -155,11 +175,7 @@ func (r *restrictedEndpointsAdmission) checkAccess(ctx context.Context, attr adm | |
| return authorized == authorizer.DecisionAllow, err | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) endpointsValidate(ctx context.Context, a admission.Attributes) error { | ||
| ep, ok := a.GetObject().(*kapi.Endpoints) | ||
| if !ok { | ||
| return nil | ||
|
|
@@ -169,18 +185,18 @@ func (r *restrictedEndpointsAdmission) Validate(ctx context.Context, a admission | |
| return nil | ||
| } | ||
|
|
||
| restrictedErr := r.endpointsFindRestrictedIP(ep, r.restrictedNetworks) | ||
| if restrictedErr == nil { | ||
| restrictedErr = r.endpointsFindRestrictedIP(ep, defaultRestrictedNetworks) | ||
| } | ||
| if restrictedErr == nil { | ||
| restrictedErr = r.endpointsFindRestrictedPort(ep, defaultRestrictedPorts) | ||
| } | ||
| if restrictedErr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| allow, err := r.endpointsCheckAccess(ctx, a) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
@@ -189,3 +205,87 @@ func (r *restrictedEndpointsAdmission) Validate(ctx context.Context, a admission | |
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) sliceFindRestrictedIP(slice *discovery.EndpointSlice, restricted []*net.IPNet) error { | ||
| for _, endpoint := range slice.Endpoints { | ||
| for _, addr := range endpoint.Addresses { | ||
|
There was a problem hiding this comment. do you want to check the There was a problem hiding this comment. Why? If it has restricted IPs, then it's restricted, regardless of conditions There was a problem hiding this comment. because the endpoints code only checks the ready ones? There was a problem hiding this comment. it was curiosity only, the endpoints code only parses There was a problem hiding this comment. hm... that seems more like a bug in the Endpoints version 😯 There was a problem hiding this comment. yeah, as I've said, it was mostly curiosity, There was a problem hiding this comment. 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. There was a problem hiding this comment.
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) There was a problem hiding this 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 |
||
| if err := checkRestrictedIP(addr, restricted); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) sliceFindRestrictedPort(slice *discovery.EndpointSlice, restricted []kapi.EndpointPort) error { | ||
| for _, port := range slice.Ports { | ||
| if port.Port == nil { | ||
| continue | ||
| } | ||
| sliceProtocol := kapi.ProtocolTCP | ||
| if port.Protocol != nil { | ||
| sliceProtocol = *port.Protocol | ||
| } | ||
| if err := checkRestrictedPort(sliceProtocol, *port.Port, restricted); err != nil { | ||
| return err | ||
| } | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) sliceCheckAccess(ctx context.Context, attr admission.Attributes) (bool, error) { | ||
| authzAttr := authorizer.AttributesRecord{ | ||
| User: attr.GetUserInfo(), | ||
| Verb: "create", | ||
| Namespace: attr.GetNamespace(), | ||
| Resource: "endpointslices", | ||
| Subresource: "restricted", | ||
| APIGroup: discovery.GroupName, | ||
| Name: attr.GetName(), | ||
| ResourceRequest: true, | ||
| } | ||
| authorized, _, err := r.authorizer.Authorize(ctx, authzAttr) | ||
| return authorized == authorizer.DecisionAllow, err | ||
| } | ||
|
|
||
| func (r *restrictedEndpointsAdmission) sliceValidate(ctx context.Context, a admission.Attributes) error { | ||
| slice, ok := a.GetObject().(*discovery.EndpointSlice) | ||
| if !ok { | ||
| return nil | ||
| } | ||
| old, ok := a.GetOldObject().(*discovery.EndpointSlice) | ||
| if ok && reflect.DeepEqual(slice.Endpoints, old.Endpoints) && reflect.DeepEqual(slice.Ports, old.Ports) { | ||
| return nil | ||
| } | ||
|
|
||
| restrictedErr := r.sliceFindRestrictedIP(slice, r.restrictedNetworks) | ||
| if restrictedErr == nil { | ||
| restrictedErr = r.sliceFindRestrictedIP(slice, defaultRestrictedNetworks) | ||
| } | ||
| if restrictedErr == nil { | ||
| restrictedErr = r.sliceFindRestrictedPort(slice, defaultRestrictedPorts) | ||
| } | ||
| if restrictedErr == nil { | ||
| return nil | ||
| } | ||
|
|
||
| allow, err := r.sliceCheckAccess(ctx, a) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| if !allow { | ||
| return admission.NewForbidden(a, restrictedErr) | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| // Validate determines if the endpoints or endpointslice object should be admitted | ||
| func (r *restrictedEndpointsAdmission) Validate(ctx context.Context, a admission.Attributes, _ admission.ObjectInterfaces) error { | ||
| if a.GetResource().GroupResource() == kapi.Resource("endpoints") { | ||
| return r.endpointsValidate(ctx, a) | ||
| } else if a.GetResource().GroupResource() == discovery.Resource("endpointslices") { | ||
| return r.sliceValidate(ctx, a) | ||
| } else { | ||
| return nil | ||
| } | ||
| } | ||
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this makes the admission tighter than it was in <4.9. Why is that correct? IMO it breaks the API.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@danwinship if there is no specific reason to have this change in the same PR, please split it up, so we can get the 4.9 blocker out of the way quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it was a security hole before. We need to block restricted changes to
NotReadyEndpointsfor the same reason that upstream blocked all changes to Endpoints.(AFAIK in all existing OCP releases (including 4.9) it is only a potential security hole, with no actual way to exploit it in the shipped product. But at any point, someone might add some component that makes use of
NotReadyAddresses, at which point it would be possible to subvert that component in CVE-2021-25740-like ways.)There is really no plausible use case for someone manually adding IPs to
NotReadyEndpointsanyway.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary from Slack why this is fine (for ourselves in 6 months):
The kube-controller-manager controllers have extended permission anyway and this admission check is skipped.