OCPBUGS-87205: Strip X-SSL-Client-* headers for plain HTTP#787
Conversation
openshift-ci-robot
added
jira/severity-important
|
@bentito: This pull request references Jira Issue OCPBUGS-86718, 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. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository: openshift/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe HAProxy config template now conditionally strips ChangesmTLS Header Spoofing Prevention
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 15✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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 `@images/router/haproxy/conf/haproxy-config.template`:
- Around line 276-285: The template is conflating ROUTER_MUTUAL_TLS_AUTH_FILTER
(used as a DN regex in frontends fe_sni and fe_no_sni) with a boolean
header-strip switch; create a new boolean env var
ROUTER_MUTUAL_TLS_STRIP_HEADERS (or similar) and use it in the header-stripping
block (replace isTrue (env "ROUTER_MUTUAL_TLS_AUTH_FILTER" "true") with isTrue
(env "ROUTER_MUTUAL_TLS_STRIP_HEADERS" "true")), leave
ROUTER_MUTUAL_TLS_AUTH_FILTER exclusively as the DN regex in frontends fe_sni
and fe_no_sni, and add backward-compatibility logic: if
ROUTER_MUTUAL_TLS_STRIP_HEADERS is unset, treat stripping as enabled only when
ROUTER_MUTUAL_TLS_AUTH_FILTER exactly equals "true" (case-insensitive) so
existing boolean deployments keep behavior but regex values won’t mistakenly
enable/disable stripping; apply the same rename/compatibility change in the
other two header-strip blocks mentioned.
🪄 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: b4ee8516-a792-44bc-8a83-55453bc0488d
📒 Files selected for processing (1)
images/router/haproxy/conf/haproxy-config.template
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
images/router/haproxy/conf/haproxy-config.template (1)
278-285:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winCritical: mTLS identity headers are not fully stripped on plain HTTP, enabling spoofing
With
ROUTER_MUTUAL_TLS_HEADER_FILTER=true, the plain-HTTP strip blocks inimages/router/haproxy/conf/haproxy-config.template(frontend public lines 278-285, plus the same pattern in the fe_sni and fe_no_sni blocks) delete onlyX-SSL-Client-{DN,DER,NotAfter,NotBefore,SHA1,Subject}. They do not deleteX-SSL,X-SSL-Client-{Verify,Serial,Version,CN}, orX-SSL-Issuer, even though these are populated from TLS state infe_sni/fe_no_sni. Since plain HTTP never sets them, attacker-supplied values can reach the backend and may bypass mTLS auth/identity checks if the backend trusts these headers.🔒 Proposed fix: strip all mTLS identity headers
{{- if isTrue (env "ROUTER_MUTUAL_TLS_HEADER_FILTER" "true") }} + http-request del-header X-SSL + http-request del-header X-SSL-Client-CN http-request del-header X-SSL-Client-DN http-request del-header X-SSL-Client-DER + http-request del-header X-SSL-Client-Serial http-request del-header X-SSL-Client-NotAfter http-request del-header X-SSL-Client-NotBefore http-request del-header X-SSL-Client-SHA1 http-request del-header X-SSL-Client-Subject + http-request del-header X-SSL-Client-Verify + http-request del-header X-SSL-Client-Version + http-request del-header X-SSL-Issuer {{- end }}Apply the same deletion additions to the corresponding strip blocks in
fe_sni(lines ~406-413) andfe_no_sni(lines ~532-539).🤖 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 `@images/router/haproxy/conf/haproxy-config.template` around lines 278 - 285, The plain-HTTP header-strip blocks gated by ROUTER_MUTUAL_TLS_HEADER_FILTER currently only remove X-SSL-Client-{DN,DER,NotAfter,NotBefore,SHA1,Subject}, leaving other mTLS-derived headers that can be spoofed; update the strip sections in the frontend public block and the fe_sni and fe_no_sni blocks to also remove X-SSL, X-SSL-Issuer and all X-SSL-Client-{Verify,Serial,Version,CN} headers so no mTLS identity headers reach backends over plain HTTP when the ROUTER_MUTUAL_TLS_HEADER_FILTER is enabled.
🤖 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.
Outside diff comments:
In `@images/router/haproxy/conf/haproxy-config.template`:
- Around line 278-285: The plain-HTTP header-strip blocks gated by
ROUTER_MUTUAL_TLS_HEADER_FILTER currently only remove
X-SSL-Client-{DN,DER,NotAfter,NotBefore,SHA1,Subject}, leaving other
mTLS-derived headers that can be spoofed; update the strip sections in the
frontend public block and the fe_sni and fe_no_sni blocks to also remove X-SSL,
X-SSL-Issuer and all X-SSL-Client-{Verify,Serial,Version,CN} headers so no mTLS
identity headers reach backends over plain HTTP when the
ROUTER_MUTUAL_TLS_HEADER_FILTER is enabled.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository: openshift/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d99a5d53-4fe2-4fbf-90d0-002924f87918
📒 Files selected for processing (1)
images/router/haproxy/conf/haproxy-config.template
|
@Miciah in #787 (review) coderabbit seems to be correct in having found more mTLS header with the same problem, like X-SSL, X-SSL-Client-Verify, X-SSL-Client-Serial, X-SSL-Client-Version, X-SSL-Client-CN, and X-SSL-Issuer what do you think, fix these in this PR too? |
Yes, we should delete them all. |
|
|
||
| # Strip off X-SSL-Client-* headers for plain HTTP if not explicitly disabled. | ||
| # This prevents unauthenticated spoofing of mutual TLS client identities. | ||
| {{- if isTrue (env "ROUTER_MUTUAL_TLS_AUTH_FILTER" "true") }} |
There was a problem hiding this comment.
Are you planning to add an API option and logic in cluster-ingress-operator to set this environment variable, or are you adding the environment variable just in case we later need to add an API field or unsupported override?
There was a problem hiding this comment.
Idea was in case a user has got a need for this, there will be a way. Is that sufficient for now?
There was a problem hiding this comment.
I think the flag will also need to come with a documentation that says "if you enable this, be sure you add a network policy on your router to trust HTTP traffic just from your loadbalancer"
There was a problem hiding this comment.
@rikatz would you want to create a DOCS tracker story please
There was a problem hiding this comment.
|
/retest |
bentito
changed the title
|
/jira refresh |
1 similar comment
|
/jira refresh |
openshift-ci-robot
added
jira/valid-bug
|
@MrSanketkumar: This pull request references Jira Issue OCPBUGS-87205, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
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. |
This prevents unauthenticated spoofing of mutual TLS client identities by deleting X-SSL-Client headers in the fe_http frontend before they can reach the backend. It introduces a new environment variable ROUTER_MUTUAL_TLS_AUTH_FILTER which defaults to true. Setting this to false allows external load balancers to inject these headers.
This avoids a collision with the existing ROUTER_MUTUAL_TLS_AUTH_FILTER regex.
Expands the list of mTLS headers stripped from plain HTTP requests to include all headers set natively by the router (X-SSL, X-SSL-Client-Verify, X-SSL-Client-Serial, X-SSL-Client-Version, X-SSL-Client-CN, X-SSL-Issuer). This fully mitigates the risk of spoofing any of these headers.
Co-authored-by: Miciah Dashiel Butler Masters <mmasters@redhat.com>
|
|
||
| # Strip off X-SSL-Client-* headers for plain HTTP if not explicitly disabled. | ||
| # This prevents unauthenticated spoofing of mutual TLS client identities. | ||
| {{- if isTrue (env "ROUTER_MUTUAL_TLS_HEADER_FILTER" "true") }} |
There was a problem hiding this comment.
The indentation of these Go template stanzas is inconsistent with surrounding Go template stanzas, but the template already has similar inconsistencies elsewhere; we can fix them all up in a follow-up.
| http-request del-header X-SSL-Client-NotBefore | ||
| http-request del-header X-SSL-Client-SHA1 | ||
| http-request del-header X-SSL-Client-Serial | ||
| http-request del-header X-SSL-Client-Subject |
There was a problem hiding this comment.
Where is X-SSL-Client-Subject added? Why does it need to be deleted?
There was a problem hiding this comment.
While we don't set them on router, I think it is a valid deletion. This header can also be used for authentication, in case some day we decide to add it to router passing to backend by a customer request we can run on a problem of forgetting to remove it
There was a problem hiding this comment.
It seems arbitrary. Why not also add http-request del-header X-SSL-Client-Chain-DER or http-request del-header X-SSL-Client-Used or other arbitrary things that an application could theoretically use or HAProxy could theoretically set?
| # Strip off Proxy headers to prevent HTTpoxy (https://httpoxy.org/) | ||
| http-request del-header Proxy | ||
|
|
||
| # Strip off X-SSL-Client-* headers for plain HTTP if not explicitly disabled. |
There was a problem hiding this comment.
With the latest changes, X-SSL-Client-* should be changed to X-SSL* here and below.
| # Strip off X-SSL-Client-* headers for plain HTTP if not explicitly disabled. | |
| # Strip off X-SSL* headers for plain HTTP if not explicitly disabled. |
| # Strip off X-SSL-Client-* headers for plain HTTP if not explicitly disabled. | ||
| # This prevents unauthenticated spoofing of mutual TLS client identities. |
There was a problem hiding this comment.
The comment in fe_no_sni should match the one in fe_sni, not the one in public.
|
@bentito: This pull request references Jira Issue OCPBUGS-87205, which is valid. 3 validation(s) were run on this bug
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. |
|
@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. |
|
Thanks! /approve By the way, do you want to squash the commits? |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Miciah The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
5 participants
This prevents unauthenticated spoofing of mutual TLS client identities by deleting X-SSL-Client headers in the fe_http frontend before they can reach the backend.
It introduces a new environment variable
ROUTER_MUTUAL_TLS_AUTH_FILTERwhich defaults to
true. Setting this tofalseallows external load balancersto inject these headers.
Joint effort of the author and gemini-pro-3.1-preview
Summary by CodeRabbit