OF-3261: Allow reverse proxies to be verified#3302
Open
guusdk wants to merge 2 commits intoigniterealtime:mainfrom
Open
OF-3261: Allow reverse proxies to be verified#3302guusdk wants to merge 2 commits intoigniterealtime:mainfrom
guusdk wants to merge 2 commits intoigniterealtime:mainfrom
Conversation
Before trusting remote-peer provided HTTP headers like `Forwarded` and `X-Forwarded-For`, the source of these headers should be validated. This prevents malicious clients from setting this header themselves. This commit introduces a wrapper for Jetty's ForwarededRequestCustomizer (which replaces the reported IP address of the remote peer with a value from the HTTP headers). When Openfire is now configured with a non-empty set of trusted proxies, such replacement only occurs when the remote peer is one of the trusted proxies. This functionality has been added to the Admin Console and webbinding implementations, using two distinct properties: - `adminConsole.forwarded.trusted.proxies` - `httpbind.forwarded.trusted.proxies` Valid values are IP addresses (IPv4 and IPv6) and ranges. The admin console has been modified to allow for configuration of these values through the pages where related functionality was already provided. A small CSS tweak was introduced: Openfire's setup had an implementation where a question-mark icon can be used to provide a tooltip help text. That has now been moved from 'setup' to 'global', so that it can be used both during setup, but also in the admin console itself (after setup has finished).
Contributor
There was a problem hiding this comment.
Pull request overview
Adds defense-in-depth for Forwarded / X-Forwarded-* processing by only honoring these headers when the immediate TCP peer is a configured trusted reverse proxy, for both the Admin Console and HTTP Bind endpoints.
Changes:
- Introduces
TrustedForwardedRequestCustomizerto conditionally apply Jetty’sForwardedRequestCustomizerbased on a configured set of trusted proxy IPs/ranges. - Adds two new system properties and exposes them in the Admin Console and HTTP Bind configuration UIs.
- Moves tooltip CSS from setup-only styling to global styling for reuse outside of setup.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/main/java/org/jivesoftware/util/netty/TrustedForwardedRequestCustomizer.java | New Jetty customizer wrapper that enforces “trusted proxy only” forwarded-header processing. |
| xmppserver/src/test/java/org/jivesoftware/util/netty/TrustedForwardedRequestCustomizerTest.java | Unit tests for trusted proxy matching and delegation behavior. |
| xmppserver/src/main/java/org/jivesoftware/openfire/container/AdminConsolePlugin.java | Adds admin-console trusted-proxies system property and wraps forwarded customizer when configured. |
| xmppserver/src/main/java/org/jivesoftware/openfire/http/HttpBindManager.java | Adds HTTP bind trusted-proxies system property and wraps forwarded customizer when configured. |
| xmppserver/src/main/webapp/system-admin-console-access.jsp | Admin Console UI for configuring trusted proxies and persisting the new property. |
| xmppserver/src/main/webapp/http-bind.jsp | HTTP Bind UI for configuring trusted proxies and persisting the new property. |
| i18n/src/main/resources/openfire_i18n.properties | Adds i18n strings for new trusted-proxies settings and help text. |
| xmppserver/src/main/webapp/style/global.css | Adds global tooltip CSS for reuse outside of setup pages. |
| xmppserver/src/main/webapp/style/setup.css | Removes tooltip CSS that is now defined globally. |
| xmppserver/src/main/webapp/style/ldap.css | Minor CSS tweak (font-family punctuation) related to tooltip styling consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Changed package name to reflect that the code relates to Jetty (not Netty) - Switch to IpUtils-provided API that ignores zone/scope parts in IPv6 addresses - Use default capacity of new sets - Fixes javadoc
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+75
to
+79
| String trustedProxy = tokenizer.nextToken(); | ||
| if (IpUtils.isValidIpAddressOrRange(trustedProxy)) { | ||
| newTrustedProxies.add(trustedProxy); | ||
| } else { | ||
| errorMap.put("trusted-proxy", "invalid-syntax"); |
| public static final SystemProperty<Set<String>> HTTP_BIND_FORWARDED_TRUSTED_PROXIES = SystemProperty.Builder.ofType(Set.class) | ||
| .setKey("httpbind.forwarded.trusted.proxies") | ||
| .setDynamic(false) // TODO This can easily be made dynamic with <tt>.addListener(HttpBindManager.getInstance()::restartServer)</tt>. Existing implementation was not dynamic. Should it? | ||
| .setDefaultValue(new HashSet<>()) |
| public static final SystemProperty<Set<String>> ADMIN_CONSOLE_FORWARDED_TRUSTED_PROXIES = SystemProperty.Builder.ofType(Set.class) | ||
| .setKey("adminConsole.forwarded.trusted.proxies") | ||
| .setDynamic(false) | ||
| .setDefaultValue(new HashSet<>()) |
Comment on lines
+111
to
+115
| if (address.getAddress() == null) { | ||
| Log.trace("Unable to determine IP address of remote socket (unresolved address): {}", addr); | ||
| return false; | ||
| } | ||
| return IpUtils.isAddressInAnyOf(address.getAddress(), trustedProxies); |
Comment on lines
172
to
+176
| AdminConsolePlugin.ADMIN_CONSOLE_FORWARDED_HOST_NAME.setValue(name.trim()); | ||
| } | ||
|
|
||
| AdminConsolePlugin.ADMIN_CONSOLE_FORWARDED_TRUSTED_PROXIES.setValue(newTrustedProxies); | ||
|
|
Comment on lines
123
to
+127
| HttpBindManager.HTTP_BIND_FORWARDED_HOST_NAME.setValue(name.trim()); | ||
| } | ||
|
|
||
| HttpBindManager.HTTP_BIND_FORWARDED_TRUSTED_PROXIES.setValue(newTrustedProxies); | ||
|
|
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
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.
Before trusting remote-peer provided HTTP headers like
ForwardedandX-Forwarded-For, the source of these headers should be validated. This prevents malicious clients from setting this header themselves.This commit introduces a wrapper for Jetty's ForwarededRequestCustomizer (which replaces the reported IP address of the remote peer with a value from the HTTP headers). When Openfire is now configured with a non-empty set of trusted proxies, such replacement only occurs when the remote peer is one of the trusted proxies.
This functionality has been added to the Admin Console and webbinding implementations, using two distinct properties:
adminConsole.forwarded.trusted.proxieshttpbind.forwarded.trusted.proxiesValid values are IP addresses (IPv4 and IPv6) and ranges.
The admin console has been modified to allow for configuration of these values through the pages where related functionality was already provided.
A small CSS tweak was introduced: Openfire's setup had an implementation where a question-mark icon can be used to provide a tooltip help text. That has now been moved from 'setup' to 'global', so that it can be used both during setup, but also in the admin console itself (after setup has finished).