OCPBUGS-65482: Fix HAProxy redirect: strip port from Host header #696
Conversation
Previously, when a route was configured with insecureEdgeTerminationPolicy: Redirect, HAProxy would preserve the port from the Host header in the redirect Location. This caused clients to attempt HTTPS connections on port 80 (e.g., https://example.com:80) if the original request included the port. This commit updates the HAProxy template to explicitly strip any port number from the Host header using 'regsub(:[0-9]+$,,)' before constructing the redirect URL. This ensures redirects always go to the default HTTPS port (443). A regression test 'Secure Redirect Strips Port' has been added to pkg/router/router_test.go to verify this configuration generation.
bentito
changed the title
openshift-ci-robot
added
jira/severity-moderate
|
@bentito: This pull request references Jira Issue OCPBUGS-65482, 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. |
|
/jira refresh |
|
@bentito: This pull request references Jira Issue OCPBUGS-65482, 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 |
openshift-ci-robot
added
jira/valid-bug
|
@bentito: This pull request references Jira Issue OCPBUGS-65482, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
reproduced it with 4.21.0-0.nightly-2025-11-22-193140 and verified it with 4.21.0-0-2025-12-02-060314-test-ci-ln-sikxch2-latest Reproduced: Verified: |
|
/label qe-approved |
openshift-ci
bot
added
the
qe-approved
|
@bentito: This pull request references Jira Issue OCPBUGS-65482, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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
added
the
verified
|
@ShudiLi: 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. |
Signed-off-by: Brett Tofel <btofel@redhat.com>
openshift-ci-robot
removed
the
verified
|
/assign |
|
As discussed in Slack, we would need an e2e test for this change. Unfortunately it can't be done in this repository as we host them in |
|
@bentito: 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. |
|
/assign @Thealisyed |
|
Tested it again and saw the 302 redirect message |
| # check if we need to redirect/force using https. | ||
| acl secure_redirect base,map_reg_int(/var/lib/haproxy/conf/os_route_http_redirect.map) -m bool | ||
| redirect scheme https if secure_redirect | ||
| http-request redirect location https://%[req.hdr(host),regsub(:[0-9]+$,,)]%[path] code 302 if secure_redirect |
There was a problem hiding this comment.
Did the original redirect scheme preserve query strings? If so, then just a thought that this new change could lead to regression IIUC ?
There was a problem hiding this comment.
Good catch! Yes, redirect scheme does redirect query component of the URL.
4.21 cluster, router pod unmodified:
$ oc -n openshift-ingress rsh router-default-55fb59cffb-cmxsj cat /var/lib/haproxy/conf/haproxy.config | grep 'redirect scheme'
redirect scheme https if secure_redirect
$ curl -H "Host: console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org:80" http://router-default.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org/123?here=there -v 2>&1 | grep '< location:'
< location: https://console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org:80/123?here=there
Router image replaced with %[path] pattern:
$ oc -n openshift-ingress rsh router-default-6cf87fbfd-4xg8h cat /var/lib/haproxy/conf/haproxy.config | grep 'redirect location'
http-request redirect location https://%[req.hdr(host),regsub(:[0-9]+$,,)]%[path] code 302 if secure_redirect
$ curl -H "Host: console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org:80" http://router-default.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org/123?here=there -v 2>&1 | grep '< location:'
< location: https://console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org/123
%[url] pattern looks to be covering the query case:
$ oc -n openshift-ingress rsh router-default-849c97866f-frw8m cat /var/lib/haproxy/conf/haproxy.config | grep 'redirect location'
http-request redirect location https://%[req.hdr(host),regsub(:[0-9]+$,,)]%[url] code 302 if secure_redirect
$ curl -H "Host: console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org:80" http://router-default.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org/123?here=there -v 2>&1 | grep '< location:'
< location: https://console-openshift-console.apps.ci-ln-8qpdpsb-76ef8.aws-4.ci.openshift.org/123?here=there
This should be covered in the e2e test case (openshift/cluster-ingress-operator#1316).
There was a problem hiding this comment.
yep thansk! I'll fix this and add test cases in the e2e to prevent regression in the future.
|
[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 |
5 participants
Previously, when a route was configured with insecureEdgeTerminationPolicy: Redirect, HAProxy would preserve the port from the Host header in the redirect Location. This caused clients to attempt HTTPS connections on port 80 (e.g., https://example.com:80) if the original request included the port.
This commit updates the HAProxy template to explicitly strip any port number from the Host header using 'regsub(:[0-9]+$,,)' before constructing the redirect URL. This ensures redirects always go to the default HTTPS port (443).
A regression test 'Secure Redirect Strips Port' has been added to pkg/router/router_test.go to verify this configuration generation.