OF-2694 & OF-2879: Add Channel binding support & implement XEP-0440#3282
Open
guusdk wants to merge 12 commits into
Open
OF-2694 & OF-2879: Add Channel binding support & implement XEP-0440#3282guusdk wants to merge 12 commits into
guusdk wants to merge 12 commits into
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Adds a pluggable channel-binding mechanism to Openfire, including a built-in tls-server-end-point implementation and initial wiring into Netty-based connections.
Changes:
- Introduces
ChannelBindingProvider+ChannelBindingProviderManagerto register and resolve channel binding data by RFC-defined prefix. - Implements
tls-server-end-pointchannel binding (RFC 5929) based on the server certificate hash, including RSASSA-PSS parameter parsing. - Exposes channel binding retrieval via
Connection#getChannelBindingData(...)and implements it inNettyConnection, with unit tests for both manager and provider.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| xmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingProvider.java | New SPI for channel binding extraction (per type/prefix). |
| xmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingProviderManager.java | New singleton manager for registering and querying providers. |
| xmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingType.java | New enum for well-known channel binding prefixes. |
| xmppserver/src/main/java/org/jivesoftware/util/channelbinding/TlsServerEndPointChannelBindingProvider.java | Implements RFC 5929 tls-server-end-point computation. |
| xmppserver/src/main/java/org/jivesoftware/openfire/Connection.java | Adds default getChannelBindingData(cbPrefix) API. |
| xmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.java | Implements channel binding retrieval using the Netty SslHandler SSLEngine. |
| xmppserver/src/test/java/org/jivesoftware/util/channelbinding/ChannelBindingProviderManagerTest.java | Tests provider registration/ordering and lookup behavior. |
| xmppserver/src/test/java/org/jivesoftware/util/channelbinding/TlsServerEndPointChannelBindingProviderTest.java | Tests hashing behavior and algorithm resolution/normalization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
95566f9 to
18c31e9
Compare
Member
Author
|
The first round of copilot's reviews were all pretty minor: unused import, input validation and white space changes, things like that. I've applied the changes to the original commit, and force-pushed those. |
guusdk
changed the title
Member
Author
|
I have now added support for exposing channel binding capable mechanisms through Openfire's SASL mechanism, and added one such mechanism: The new mechanism is advertised by default (unless configured otherwise, through preexisting options in the admin console). Additionally, I've added support for XEP-0440: SASL Channel-Binding Type Capability as tracked by OF-2879. I have tested the native support in Openfire of the 
I'm unmarking this PR as draft. It is ready for review. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
2 times, most recently
from
a8d2268 to
79cce45
Compare
Member
Author
|
I have added a companion plugin to this work, here: https://github.com/igniterealtime/openfire-tlsexportercb-plugin This plugin uses the framework that is introduced in this PR. It adds the |
guusdk
changed the title
guusdk
changed the title
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
64891a7 to
ce2d1a5
Compare
Member
Author
|
This PR briefly had a commit that improves the reesilience of the SCARM-SHA-1 SASL implementation. I have now separated out those changes into distinct PRs: |
Fishbowler
approved these changes
Apr 26, 2026
Member
Fishbowler
left a comment
Fishbowler
left a comment
There was a problem hiding this comment.
This is ace! The comments are enough to put all the pieces together, even for a muggle like me.
A few comments, but all typographical in nature.
I've read every line, it all makes sense, but I've not run any tests on this yet.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
ce2d1a5 to
f790903
Compare
Member
Author
|
I have addressed all suggestions from @Fishbowler (which were mostly spelling/text changes). I have also rebased this on Main now that #3288 was merged there. |
Add a channel binding abstraction consisting of: - ChannelBindingProvider interface for extracting channel binding data from an SSLEngine - ChannelBindingProviderManager to register, manage, and resolve providers per channel binding type - ChannelBindingType enum defining RFC-aligned binding identifiers - Comprehensive unit tests covering provider registration, ordering, resolution, and failure handling This introduces a best-effort mechanism to obtain channel binding data (as defined in RFC 5705, RFC 5929, and RFC 9266) without introducing hard dependencies on a specific TLS implementation or JDK version. The manager maintains an ordered list of providers per channel binding type and iterates through them until one successfully produces a value. Failures are isolated per provider, ensuring graceful degradation. Why this is needed Channel binding support is highly dependent on the capabilities of the underlying TLS stack and JDK version. Some mechanisms (notably newer exporter-based bindings) are difficult or effectively impossible to implement on older Java versions (e.g., Java 17), even when using reflection. At the same time, increasing the minimum required Java version for Openfire is undesirable, as it would negatively impact existing deployments. This change introduces a "manager/provider" pattern that: - Aligns with existing extensibility mechanisms used elsewhere in the codebase - Enables runtime discovery and prioritization of multiple implementations - Allows third parties to contribute additional providers via plugins Crucially, this design decouples channel binding support from the core runtime: - Core Openfire can remain compatible with Java 17 - Optional plugins can provide advanced implementations that depend on newer Java versions (e.g., Java 25+) These plugins can register their providers with the manager at runtime, seamlessly extending functionality without impacting baseline compatibility
…tion Introduce TlsServerEndPointChannelBindingProvider, an implementation of ChannelBindingProvider for the tls-server-end-point channel binding type as defined in RFC 5929. Implements logic to select the appropriate hash algorithm based on certificate signature algorithm, with special handling for RSASSA-PSS and weak hashes.
Expose per-connection channel binding data retrieval for SASL.
…ault This registers the corresponding provider without any ceremony. Future modifications could introduce configurability, but as support for this channel binding is mandatory per XEP, hard-coding seems acceptable.
Introduces SCRAM-SHA-1-PLUS SASL mechanism, implementing RFC 5802 channel binding for TLS. Advertise SCRAM-SHA-1-PLUS by default, but only when channel binding is available and session is encrypted.
With Channel Binding now available (as this commit follows commits that implements these under OF-2694), Openfire should announce what type of channel binding types it supports. The mechanism for this is define in XEP-0440. This commit implements this mechanism. Additionally, the channel binding type that is used for a session (if any) is now shown on its 'session details' page on the admin console.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
f790903 to
b292378
Compare
Member
Author
|
I've rebased this to HEAD of Main, after merging changes for OF-3257 and OF-3258. |
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
b292378 to
f2b5a7f
Compare
Member
Author
|
@coderabbitai please review |
|
Tip For best results, initiate chat on the files or code changes.
[review] |
…rage This adds more validation of the exchanged data in the SASL handshake, and adds more unit test coverage. Small refactorings have been applied to make ScramSha1SaslServer easier to unit-test. It now no longer unconditionally uses static calls to other classes. Such classes can be provided via a `@VisibleForTesting` constructor.
…when channel binding is supported Refactored SASL mechanism advertisement logic to ensure that -PLUS mechanisms (e.g., SCRAM-SHA-1-PLUS) are only offered when the current connection supports channel binding. The check now relies on Connection#getSupportedChannelBindingTypes(), which must return a non-empty set for -PLUS mechanisms to be advertised. Updated all relevant connection implementations (only NettyConnection, and the default method in the Connection interface) to ensure correct reporting of supported channel binding types. This change prevents authentication failures caused by advertising -PLUS mechanisms on connections that cannot provide channel binding data (e.g., WebSocket connections).
This addresses various typos and spelling issues. None of these are functionally changing anything.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
f2b5a7f to
adb1146
Compare
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis pull request implements comprehensive support for SASL channel binding as defined in XEP-0440, enabling TLS channel binding for the SCRAM-SHA-1-PLUS mechanism. The changes introduce a pluggable provider architecture for channel binding extraction (with RFC 5929 TLS server end-point implementation), extend the SCRAM authentication protocol with full 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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: 4
🤖 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 `@i18n/src/main/resources/openfire_i18n_nl.properties`:
- Line 1736: Replace the English label value for the key
session.details.channel-binding-type in the Dutch resource bundle with a proper
Dutch translation (keep the key unchanged); update the line
`session.details.channel-binding-type=...` to a Dutch string such as
"Kanaalbindingstype" (ensure file encoding remains UTF-8 and no trailing
whitespace or extra characters are added).
In
`@xmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingProviderManager.java`:
- Around line 220-235: The method getSASLChannelBindingTypeCapabilityElement
currently uses the manager-wide getSupportedChannelBindingTypes() which can
advertise channel-binding types not actually available for the current
connection; change the API so callers supply the connection/session-specific
supported set (e.g. add a parameter like Set<String> connectionSupportedTypes or
a method overload) and use that set instead of getSupportedChannelBindingTypes()
inside getSASLChannelBindingTypeCapabilityElement (keep the mech check on
saslMechanisms and the same Element construction logic, but iterate over the
supplied connectionSupportedTypes). Update call sites to compute the runtime
TLS/connection-supported channel-binding types and pass them into
getSASLChannelBindingTypeCapabilityElement.
- Around line 123-141: The removeProvider method currently reads the list from
the providers map and then conditionally removes the map entry based on
list.isEmpty(), which is not atomic and can drop a provider added concurrently;
change the logic in removeProvider to perform an atomic update on the providers
map (e.g., use providers.compute or computeIfPresent) so the removal of the
provider and the decision to remove the map entry happen inside a single atomic
map operation, capture whether the provider was removed (e.g., via a local
boolean/AtomicBoolean set inside the compute lambda) and return that result;
update references to the local variable list and the prefix within the new
atomic operation in the removeProvider method.
In
`@xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.java`:
- Around line 234-237: The test currently builds c= by concatenating two Base64
strings (gs2HeaderBase64 and channelBindingBase64); instead, compute c= by
concatenating the raw bytes of gs2Header and channelBindingData (e.g., combine
gs2Header.getBytes(StandardCharsets.UTF_8) with channelBindingData), then
Base64-encode that combined byte array and use that result in
clientFinalMessageBare; update the code in ScramSha1SaslServerTest where
gs2HeaderBase64, channelBindingBase64 and clientFinalMessageBare are used so c=
is Base64(rawGs2HeaderBytes + channelBindingData) rather than Base64(gs2Header)
+ Base64(channelBindingData).
🪄 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 UI
Review profile: CHILL
Plan: Pro Plus
Run ID: b9dc910a-5dda-4ea1-af83-4b2e56ba4683
📒 Files selected for processing (29)
documentation/openfire.doapdocumentation/protocol-support.htmli18n/src/main/resources/openfire_i18n.propertiesi18n/src/main/resources/openfire_i18n_nl.propertiesxmppserver/src/main/java/org/jivesoftware/openfire/Connection.javaxmppserver/src/main/java/org/jivesoftware/openfire/container/AdminConsolePlugin.javaxmppserver/src/main/java/org/jivesoftware/openfire/group/AbstractGroupProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SocketReadingMode.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.javaxmppserver/src/main/java/org/jivesoftware/openfire/nio/NettyConnection.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/SaslProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/SaslServerFactoryImpl.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalClientSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/LocalIncomingServerSession.javaxmppserver/src/main/java/org/jivesoftware/openfire/websocket/WebSocketClientStanzaHandler.javaxmppserver/src/main/java/org/jivesoftware/util/CertificateManager.javaxmppserver/src/main/java/org/jivesoftware/util/cache/DefaultLocalCacheStrategy.javaxmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingProvider.javaxmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingProviderManager.javaxmppserver/src/main/java/org/jivesoftware/util/channelbinding/ChannelBindingType.javaxmppserver/src/main/java/org/jivesoftware/util/channelbinding/TlsServerEndPointChannelBindingProvider.javaxmppserver/src/main/webapp/session-details.jspxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerFakeKeyTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerSaltTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/util/channelbinding/ChannelBindingProviderManagerTest.javaxmppserver/src/test/java/org/jivesoftware/util/channelbinding/TlsServerEndPointChannelBindingProviderTest.java
…r thread safe This mimics the concurrency control of the implementation that adds these providers.
…est vector Instead of concatenating two base64 strings, concat the byte-parts of the channel-binding data, and base64 encode the result. This better conforms to https://datatracker.ietf.org/doc/html/rfc5802#section-7
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 `@build/ci/conversations/flows/cb.yaml`:
- Around line 39-43: The PATTERN used for log checking is too specific to
"SCRAM-SHA-1-PLUS"; update the PATTERN (the env variable named PATTERN used by
the runScript that calls checkForLogs.js) so the regex matches any SASL
mechanism that ends with "-PLUS" instead of the literal "SCRAM-SHA-1-PLUS" —
i.e., change the part after "Authenticating with SASL/" to a generic class that
allows one-or-more alphanumeric/dash characters followed by "-PLUS" so the check
accepts any valid -PLUS mechanism.
🪄 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 UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dea4a133-6d6c-4461-8d8e-fff62ed7e71c
📒 Files selected for processing (2)
.github/workflows/continuous-integration-workflow.ymlbuild/ci/conversations/flows/cb.yaml
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/continuous-integration-workflow.yml
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
7ab20c3 to
3509621
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
build/ci/conversations/flows/cb.yaml (1)
42-42: ⚡ Quick winTighten the
-PLUSregex to avoid false-positive log matches.On Line 42,
.*-PLUScan match arbitrary text, not just a mechanism token. Prefer a token-shaped pattern ([A-Z0-9-]+-PLUS) so the assertion stays precise.Suggested patch
- PATTERN: 'jane@example\.org.*Authenticating with SASL\/.*-PLUS' + PATTERN: 'jane@example\.org.*Authenticating with SASL\/[A-Z0-9-]+-PLUS\b'🤖 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 `@build/ci/conversations/flows/cb.yaml` at line 42, The PATTERN value currently uses a greedy fragment ".*-PLUS" which can match arbitrary text and cause false positives; update the regex in the PATTERN string (the PATTERN entry that currently contains 'jane@example\.org.*Authenticating with SASL\/.*-PLUS') to constrain the mechanism token portion to a token-shaped class such as '[A-Z0-9-]+-PLUS' (or '\b[A-Z0-9-]+-PLUS\b' if word boundaries are desired), replacing the trailing ".*-PLUS" with the tighter pattern so the assertion only matches valid mechanism tokens.
🤖 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.
Nitpick comments:
In `@build/ci/conversations/flows/cb.yaml`:
- Line 42: The PATTERN value currently uses a greedy fragment ".*-PLUS" which
can match arbitrary text and cause false positives; update the regex in the
PATTERN string (the PATTERN entry that currently contains
'jane@example\.org.*Authenticating with SASL\/.*-PLUS') to constrain the
mechanism token portion to a token-shaped class such as '[A-Z0-9-]+-PLUS' (or
'\b[A-Z0-9-]+-PLUS\b' if word boundaries are desired), replacing the trailing
".*-PLUS" with the tighter pattern so the assertion only matches valid mechanism
tokens.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 94c067e1-bce6-404d-95d0-5b9889320f84
📒 Files selected for processing (2)
.github/workflows/continuous-integration-workflow.ymlbuild/ci/conversations/flows/cb.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/continuous-integration-workflow.yml
This new test launches Conversations against a standard Openfire server (the 'demoboot' configuration) and attempts to authenticate. It is asserted that: - the authentication attempt was successful - a SASL mechanism ending with `-PLUS` was used When this test passes, it's verified that Conversations successfully used channel binding.
guusdk
force-pushed
the
OF-2694_Channel-Binding
branch
from
3509621 to
c4a8340
Compare
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.
Please refer to individual commit messages, but this adds an implementation for Channel Binding support, by:
tls-server-end-pointThis still lacks actual SASL support, and probably a couple of other things. I'm creating this PR as I'm looking for feedback (after churning way to much time on this).
After much experimentation, I've given up on
tls-exporteras a native thing in Openfire. The Java-native route requires Java 25 (where Openfire can go as low as 17), and BouncyCastle is giving me more nightmares.With this manager/provider mechanism, you can create a plugin that implements
tls-exporter. As plugins can define a minimum Java version of their own, that can be leveraged to gettls-exporterin deployments that run on Java 25. I've got a simple plugin in the works (not included here). I have tried implementingtls-exporterin Openfire directly, by using Java reflection to dynamically detect support for the API that was added in Java 25 (in an attempt to be able to include code that would be compatible yet not functional with Java 17), but I ran into problems with that approach. Notably, the added security layers with Java modules, made it all more messy than I am comfortable with.The
tls-server-end-pointimplementation was trickier than I thought, but at least we can do that in Openfire directly. I've refactored my earlier implementation to make use of that same provider mechanism, for consistency. That's included in this PR.