[release-4.21] OCPBUGS-86729: Prevent SSRF via FQDN-typed EndpointSlices - #818
Open
MrSanketkumar wants to merge 2 commits into
Open
[release-4.21] OCPBUGS-86729: Prevent SSRF via FQDN-typed EndpointSlices#818MrSanketkumar wants to merge 2 commits into
MrSanketkumar wants to merge 2 commits into
Conversation
The OpenShift Router previously did not validate backend destinations resolved from FQDN-typed EndpointSlices. This allowed the usage of invalid EndpointSlices to target hostnames that resolves to restricted IPs (like the cloud metadata service at 169.254.169.254). This commit disables the usage of EndpointSlices of type FQDN, and add validations on Endpoints to check if a restricted IP is being used before adding them to HAProxy endpoints Backend. The implementation and disabling the usage of FQDN-backed endpoints is based on the following: * RFE-2832 to implement support for ExternalName was considered and rejected due to security concerns, so OpenShift today does not support officially the usage of FQDN-based endpoints on router * OCPBUGS-55506 relates to a mistake caused by the customer that caused unavailability and not the need to support FQDN-based names * The behavior of an endpointslice of type FQDN is deprecated on Kubernetes * The behavior of an endpointslice using an address that is not IPv4 or IPv6 (eg.: hostname) is unespecified by Kubernetes and has no usage. * The hostname field on endpointslice is not used on router This way, there is a common understanding that the usage of FQDN based addresses on Router was a mistake, and disabling it is the right fix. Additional validations of the IP address on the generated endpointnt array is added to guarantee that no invalid nor restricted IP is used.
openshift-ci-robot
added
jira/severity-important
Contributor
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-86729, which is invalid:
Comment The bug has been updated to refer to the pull request using the external bug tracker. DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughChangesEndpoint validation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant TemplateRouterOptions.Run
participant ExtendedValidator
participant router.Plugin
participant RouteStatusRecorder
TemplateRouterOptions.Run->>ExtendedValidator: wrap plugin with extendedRouteValidation
ExtendedValidator->>ExtendedValidator: filter non-deleted endpoint addresses
ExtendedValidator->>router.Plugin: forward filtered endpoint event
ExtendedValidator->>RouteStatusRecorder: record invalid route rejection
ExtendedValidator->>router.Plugin: forward rejected route as deleted
Suggested reviewers: Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (13 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
MrSanketkumar
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/router/controller/extended_validator.go`:
- Line 176: The watch.Deleted branch in the route validation flow ignores the
error returned by downstream p.plugin.HandleRoute. Capture that error and log it
using the component’s existing logger, while preserving the current propagation
behavior and route arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 50bfb0a6-1ede-457c-a324-9eac8330eada
📒 Files selected for processing (8)
pkg/cmd/infra/router/template.gopkg/router/controller/endpointsubset/converter.gopkg/router/controller/endpointsubset/converter_test.gopkg/router/controller/extended_validator.gopkg/router/controller/extended_validator_test.gopkg/router/controller/factory/factory_endpointslices_test.gopkg/router/router_test.gopkg/router/template/plugin_test.go
| log.Error(err, "skipping route due to invalid configuration", "route", routeName) | ||
|
|
||
| p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) | ||
| p.plugin.HandleRoute(watch.Deleted, route) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Ignored error from downstream HandleRoute.
The watch.Deleted propagation to the next plugin drops its returned error. If the downstream delete fails while rejecting an invalid route, that failure is silently lost. At minimum log it.
As per path instructions: "Never ignore error returns".
🔧 Suggested change
p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error())
- p.plugin.HandleRoute(watch.Deleted, route)
+ if derr := p.plugin.HandleRoute(watch.Deleted, route); derr != nil {
+ log.Error(derr, "failed to propagate deletion for invalid route", "route", routeName)
+ }
return fmt.Errorf("invalid route configuration")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| p.plugin.HandleRoute(watch.Deleted, route) | |
| if derr := p.plugin.HandleRoute(watch.Deleted, route); derr != nil { | |
| log.Error(derr, "failed to propagate deletion for invalid route", "route", routeName) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/router/controller/extended_validator.go` at line 176, The watch.Deleted
branch in the route validation flow ignores the error returned by downstream
p.plugin.HandleRoute. Capture that error and log it using the component’s
existing logger, while preserving the current propagation behavior and route
arguments.
Source: Path instructions
Contributor
|
@MrSanketkumar: all tests passed! Full PR test history. Your PR dashboard. DetailsInstructions 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-sigs/prow repository. I understand the commands that are listed here. |
Cluster Versionoc get clusterversionOutputSSRF Protection Validation - EndpointSlice Verification ResultsTest Environment SetupCreate test namespace and serviceoc create namespace ssrf-test
oc -n ssrf-test create service clusterip metadata-svc --tcp=80:80
oc -n ssrf-test patch svc metadata-svc --type=json \
-p='[{"op":"remove","path":"/spec/selector"}]'Create routeCLUSTER_DOMAIN=$(oc get ingresses.config cluster -o jsonpath='{.spec.domain}')
oc -n ssrf-test expose svc metadata-svc \
--hostname=ssrf-test.${CLUSTER_DOMAIN}Save URLTEST_URL="http://ssrf-test.${CLUSTER_DOMAIN}/latest/meta-data/"Test 1: FQDN EndpointSlice (Layer 1)EndpointSlice cat <<EOF | oc apply -f -
apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-fqdn
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: FQDN
endpoints:
- addresses:
- "metadata.google.internal"
ports:
- port: 80
EOF
Warning: spec.addressType: FQDN endpoints are deprecated
endpointslice.discovery.k8s.io/test-fqdn created
Verificationcurl -s -o /dev/null -w "%{http_code}" $TEST_URL
503
oc -n openshift-ingress logs deployment/router-default | grep "unsupported address type"pe"
Found 2 pods, using pod/router-default-6549ffc5ff-54zzg
I0717 06:18:42.188302 1 converter.go:18] "msg"="Skipping EndpointSlice with unsupported address type" "addressType"="FQDN" "logger"="endpointsubset" "name"="test-fqdn" "namespace"="ssrf-test"Test 2: IPv6 Hex Metadata IP (Layer 2 Pass 2)EndpointSlicecat <<EOF | oc apply -f -
apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-metadata-hex
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "::a9fe:a9fe"
ports:
- port: 80
EOF
endpointslice.discovery.k8s.io/test-ipv6-metadata-hex createdVerificationcurl -s -o /dev/null -w "%{http_code}" $TEST_URL
503
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "a9fe"fe"
**Found 2 pods, using pod/router-default-6549ffc5ff-54zzg
E0717 06:22:45.976337 1 extended_validator.go:70] "msg"="Skipping endpoint address with restricted or invalid IP" "error"="IP address 169.254.169.254 is a restricted link-local IP" "address"="::a9fe:a9fe" "logger"="controller"**Test 3: IPv6 Hex Loopback (Layer 2 Pass 2)EndpointSlicecat <<EOF | oc apply -f -
apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-loopback-hex
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "::7f00:1"
ports:
- port: 80
EOF
endpointslice.discovery.k8s.io/test-ipv6-loopback-hex createdVerificationcurl -s -o /dev/null -w "%{http_code}" $TEST_URL
503
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "7f00"
Found 2 pods, using pod/router-default-6549ffc5ff-54zzg
E0717 06:25:35.243202 1 extended_validator.go:70] "msg"="Skipping endpoint address with restricted or invalid IP" "error"="IP address 127.0.0.1 is a restricted loopback IP" "address"="::7f00:1" "logger"="controller"Test 4: AWS IPv6 IMDS (Layer 2 Pass 1)EndpointSlicecat <<EOF | oc apply -f -
apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-aws-ipv6-imds
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "fd00:ec2::254"
ports:
- port: 80
EOF
endpointslice.discovery.k8s.io/test-aws-ipv6-imds createdVerificationcurl -s -o /dev/null -w "%{http_code}" $TEST_URL
503
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "fd00"
Found 2 pods, using pod/router-default-6549ffc5ff-54zzg
E0717 06:27:50.255380 1 extended_validator.go:70] "msg"="Skipping endpoint address with restricted or invalid IP" "error"="IP address fd00:ec2::254 is a restricted cloud metadata IP" "address"="fd00:ec2::254" "logger"="controller"Test 5: IPv6 Loopback (Layer 2 Pass 1)EndpointSliceapiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-loopback
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "::1"
ports:
- port: 80Verificationoc apply -f test-ipv6-loopback.yaml
curl -s -o /dev/null -w "%{http_code}" $TEST_URL
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "::1"OutputTest 6: IPv6 Link-Local (Layer 2 Pass 1)EndpointSliceapiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-linklocal
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "fe80::1"
ports:
- port: 80Verificationoc apply -f test-ipv6-linklocal.yaml
curl -s -o /dev/null -w "%{http_code}" $TEST_URL
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "fe80"OutputTest 7: IPv6 Multicast (Layer 2 Pass 1)EndpointSliceapiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-multicast
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "ff02::1"
ports:
- port: 80Verificationoc apply -f test-ipv6-multicast.yaml
curl -s -o /dev/null -w "%{http_code}" $TEST_URL
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "ff02"OutputTest 8: IPv6 Unspecified (Layer 2 Pass 1)EndpointSliceapiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-ipv6-unspecified
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "::"
ports:
- port: 80Verificationoc apply -f test-ipv6-unspecified.yaml
curl -s -o /dev/null -w "%{http_code}" $TEST_URL
oc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "unspecified"OutputTest 9: Valid IPv6 Pass-ThroughEndpointSlicecat <<EOF | oc apply -f -
apiVersion: discovery.k8s.io/v1
kind: EndpointSlice
metadata:
name: test-valid-ipv6
namespace: ssrf-test
labels:
kubernetes.io/service-name: metadata-svc
addressType: IPv6
endpoints:
- addresses:
- "2001:db8::1"
ports:
- port: 80
EOF
endpointslice.discovery.k8s.io/test-valid-ipv6 createdVerificationoc -n openshift-ingress logs deployment/router-default | grep "restricted" | grep "2001"
Found 2 pods, using pod/router-default-6549ffc5ff-54zzg (EXPECTED)
Expected Results:
No log line found regarding this address being restricted.
The router accepts it as a valid backend.
Test 10: Normal Application Regression Testoc -n ssrf-test new-app --image=openshift/hello-openshift --name=hello
oc -n ssrf-test expose svc hello --hostname=hello-test.${CLUSTER_DOMAIN}
# Wait for pod to be ready
oc -n ssrf-test wait --for=condition=ready pod -l app=hello --timeout=60s
--> Found container image 7af3297 (8 years old) from Docker Hub for "openshift/hello-openshift"
* An image stream tag will be created as "hello:latest" that will track this image
--> Creating resources ...
imagestream.image.openshift.io "hello" created
deployment.apps "hello" created
service "hello" created
--> Success
Application is not exposed. You can expose services to the outside world by executing one or more of the commands below:
'oc expose service/hello'
Run 'oc status' to view your app.
route.route.openshift.io/hello exposed
Verificationcurl -s http://hello-test.${CLUSTER_DOMAIN}
Hello OpenShift!
NoteKubernetes/OpenShift prevent the creation of EndpointSlices that reference special-purpose IP address ranges, including loopback (127.0.0.0/8, ::1/128), unspecified (0.0.0.0, ::), multicast (224.0.0.0/4, ff00::/8), and other non-routable address ranges such as link-local addresses (169.254.0.0/16, fe80::/10) because of the built-in validation implemented by Kubernetes/OpenShift. Tests 5–8 are blocked by Kubernetes/OpenShift built-in EndpointSlice validation before reaching the router SSRF validation logic. /verified by ci |
openshift-ci-robot
added
the
verified
Contributor
|
@UdayYendva: This PR has been marked as verified by DetailsIn response to this:
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 openshift-eng/jira-lifecycle-plugin repository. |
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
The OpenShift Router previously did not validate backend destinations resolved from FQDN-typed EndpointSlices. This allowed the usage of invalid EndpointSlices to target hostnames that resolves to restricted IPs (like the cloud metadata service at 169.254.169.254).
This commit disables the usage of EndpointSlices of type FQDN, and add validations on Endpoints to check if a restricted IP is being used before adding them to HAProxy endpoints Backend.
The implementation and disabling the usage of FQDN-backed endpoints is based on the following:
This way, there is a common understanding that the usage of FQDN based addresses on Router was a mistake, and disabling it is the right fix.
Additional validations of the IP address on the generated endpointnt array is added to guarantee that no invalid nor restricted IP is used.
Backport :#813
Summary by CodeRabbit
Security & Reliability
Bug Fixes
Tests