[release-4.22] OCPBUGS-87166: Prevent SSRF via FQDN-typed EndpointSlices - #813
[release-4.22] OCPBUGS-87166: Prevent SSRF via FQDN-typed EndpointSlices#813MrSanketkumar wants to merge 1 commit 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
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87166, 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. |
WalkthroughChangesThe router now always constructs Endpoint validation and address-type filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant EndpointSlice
participant ConvertEndpointSlice
participant ExtendedValidator
participant DownstreamPlugin
EndpointSlice->>ConvertEndpointSlice: provide typed endpoint data
ConvertEndpointSlice->>ExtendedValidator: produce converted endpoints
ExtendedValidator->>ExtendedValidator: filter restricted addresses
ExtendedValidator->>DownstreamPlugin: forward filtered endpoints
Possibly related PRs
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 |
|
[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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
/jira refresh |
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87166, which is invalid:
Comment 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. |
|
/jira refresh |
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87166, which is invalid:
Comment 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. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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`:
- Around line 66-73: Update filterValidAddresses and the other
endpoint-rejection logging paths in this file to stop emitting raw IP or
endpoint values, including the duplicated value in err. Log only a safe
rejection reason or aggregate count while preserving the existing filtering
behavior.
- Around line 87-90: Update the address validation logic around azureMetadata
and awsIPv6IMDS to recognize and reject RFC 6052 IPv4/IPv6 translation ranges,
including 64:ff9b::/96 and 64:ff9b:1::/48, before IPv6 normalization can bypass
checks. Prefer the existing trust-boundary allow-list approach where applicable,
and add regression cases covering translated addresses that resolve to cloud
metadata endpoints.
- Around line 172-177: Update the invalid-route branch in the extended
validation flow to capture and propagate the error returned by
p.plugin.HandleRoute(watch.Deleted, route) instead of discarding it. Preserve
the existing rejection recording and invalid-configuration error behavior while
ensuring downstream deletion failures are returned to the caller.
🪄 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: 1dbeddef-7001-4749-ae23-6b6657f47c4a
📒 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
| func filterValidAddresses(addrs []kapi.EndpointAddress) []kapi.EndpointAddress { | ||
| return slices.DeleteFunc(addrs, func(addr kapi.EndpointAddress) bool { | ||
| err := validateEndpointAddress(addr.IP) | ||
| if err != nil { | ||
| log.Error(err, "Skipping endpoint address with restricted or invalid IP", "address", addr.IP) | ||
| } | ||
| return err != nil | ||
| }) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove raw endpoint values from rejection logs.
Both err and the "address" field contain addr.IP; malformed values can be internal hostnames such as metadata.google.internal. Log only a reason or aggregate count.
Proposed fix
func filterValidAddresses(addrs []kapi.EndpointAddress) []kapi.EndpointAddress {
return slices.DeleteFunc(addrs, func(addr kapi.EndpointAddress) bool {
- err := validateEndpointAddress(addr.IP)
- if err != nil {
- log.Error(err, "Skipping endpoint address with restricted or invalid IP", "address", addr.IP)
+ if validateEndpointAddress(addr.IP) == nil {
+ return false
}
- return err != nil
+ log.V(4).Info("Skipping endpoint address with restricted or invalid IP")
+ return true
})
}As per coding guidelines, “Flag logging that may expose internal hostnames or customer data.”
Also applies to: 76-80, 115-130
🤖 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` around lines 66 - 73, Update
filterValidAddresses and the other endpoint-rejection logging paths in this file
to stop emitting raw IP or endpoint values, including the duplicated value in
err. Log only a safe rejection reason or aggregate count while preserving the
existing filtering behavior.
Source: Coding guidelines
| var ( | ||
| azureMetadata = netip.MustParseAddr("168.63.129.16") | ||
| awsIPv6IMDS = netip.MustParseAddr("fd00:ec2::254") | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Block IPv4/IPv6 translation prefixes that bypass normalization.
64:ff9b::a9fe:a9fe passes these checks as ordinary IPv6, although 64:ff9b::/96 embeds an IPv4 destination; 64:ff9b:1::/48 is also reserved for local translation. On NAT64-enabled nodes, this can translate to 169.254.169.254, restoring IMDS access. Reject these translation prefixes and add regression cases, or enforce a deployment-aware backend CIDR allow-list. (rfc-editor.org)
As per path instructions, “Validate at trust boundaries with allow-lists, not deny-lists.”
Also applies to: 105-113, 115-130
🤖 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` around lines 87 - 90, Update the
address validation logic around azureMetadata and awsIPv6IMDS to recognize and
reject RFC 6052 IPv4/IPv6 translation ranges, including 64:ff9b::/96 and
64:ff9b:1::/48, before IPv6 normalization can bypass checks. Prefer the existing
trust-boundary allow-list approach where applicable, and add regression cases
covering translated addresses that resolve to cloud metadata endpoints.
Source: Path instructions
| if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil { | ||
| log.Error(err, "skipping route due to invalid configuration", "route", routeName) | ||
|
|
||
| p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) | ||
| p.plugin.HandleRoute(watch.Deleted, route) | ||
| return fmt.Errorf("invalid route configuration") |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Propagate failure to remove the invalid route.
Line 176 discards the downstream deletion error, potentially leaving stale invalid configuration installed.
Proposed fix
p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error())
- p.plugin.HandleRoute(watch.Deleted, route)
+ if deleteErr := p.plugin.HandleRoute(watch.Deleted, route); deleteErr != nil {
+ return fmt.Errorf("invalid route configuration; failed to remove route: %w", deleteErr)
+ }
return fmt.Errorf("invalid route configuration")As per path instructions, “Never ignore error returns.”
📝 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.
| if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil { | |
| log.Error(err, "skipping route due to invalid configuration", "route", routeName) | |
| p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) | |
| p.plugin.HandleRoute(watch.Deleted, route) | |
| return fmt.Errorf("invalid route configuration") | |
| if err := routeapihelpers.ExtendedValidateRoute(route).ToAggregate(); err != nil { | |
| log.Error(err, "skipping route due to invalid configuration", "route", routeName) | |
| p.recorder.RecordRouteRejection(route, "ExtendedValidationFailed", err.Error()) | |
| if deleteErr := p.plugin.HandleRoute(watch.Deleted, route); deleteErr != nil { | |
| return fmt.Errorf("invalid route configuration; failed to remove route: %w", deleteErr) | |
| } | |
| return fmt.Errorf("invalid route configuration") |
🤖 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` around lines 172 - 177, Update
the invalid-route branch in the extended validation flow to capture and
propagate the error returned by p.plugin.HandleRoute(watch.Deleted, route)
instead of discarding it. Preserve the existing rejection recording and
invalid-configuration error behavior while ensuring downstream deletion failures
are returned to the caller.
Source: Path instructions
|
/jira refresh |
openshift-ci-robot
added
the
jira/valid-bug
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87166, which is valid. The bug has been moved to the POST state. 7 validation(s) were run on this bug
No GitHub users were found matching the public email listed for the QA contact in Jira (uyendava@redhat.com), skipping review request. 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. |
openshift-ci-robot
removed
the
jira/invalid-bug
|
/retest-required |
|
/test e2e-aws-fips |
1 similar comment
|
/test e2e-aws-fips |
|
@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. |
|
/abel backport-risk-assessed |
openshift-ci-robot
added
jira/invalid-bug
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87166, which is invalid:
Comment 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. |
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-64b87db7bc-kd2kp
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-64b87db7bc-kd2kp
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-64b87db7bc-kd2kp
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-64b87db7bc-kd2kp
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-64b87db7bc-kd2kp (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
|
@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. |
6 participants
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.
Backported from #812
Summary by CodeRabbit