SASL 2 by dwd · Pull Request #3113 · igniterealtime/Openfire

dwd · 2025-12-18T11:00:05Z

This is a "clean" branch of SASL2 only (no Bind2 etc).

It includes automated testing covering the essentials.

Manual testing (should be) done with Conversations and Gajim.

# Conflicts: # xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java # xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java

Copilot

Pull request overview

This PR implements SASL 2 (Extensible SASL Profile, XEP-0388) support for Openfire, providing a modernized SASL authentication mechanism alongside the existing SASL 1 implementation.

Adds SASL2 namespace support with <authenticate> element handling
Introduces user agent information capture during SASL2 authentication
Includes comprehensive test coverage for both SASL1 and SASL2 flows

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 14 comments.

Show a summary per file

File	Description
xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java	Core implementation adding SASL2 support, including new `addSASLMechanisms` methods, AUTHENTICATE element handling, and user agent extraction
xmppserver/src/main/java/org/jivesoftware/openfire/net/UserAgentInfo.java	New class for parsing and validating user agent information from SASL2 authentication requests
xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java	Updates to handle SASL2 `<authenticate>` elements and manage SASL2 state flags
xmppserver/src/main/java/org/jivesoftware/openfire/net/SocketReadingMode.java	Socket connection handler updates for SASL2 support
xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalClientSession.java	Migrates from deprecated `getSASLMechanisms` to new `addSASLMechanisms` method
xmppserver/src/main/java/org/jivesoftware/openfire/session/LocalIncomingServerSession.java	Migrates from deprecated `getSASLMechanisms` to new `addSASLMechanisms` method
xmppserver/src/main/java/org/jivesoftware/openfire/websocket/WebSocketClientStanzaHandler.java	Updates WebSocket handler to use new SASL mechanisms API
xmppserver/src/main/java/org/jivesoftware/openfire/http/HttpSession.java	Updates HTTP/BOSH handler to use new SASL mechanisms API
xmppserver/src/main/java/org/jivesoftware/openfire/SessionPacketRouter.java	Adds `usingSASL2` parameter to handle method call
xmppserver/src/test/java/org/jivesoftware/openfire/sasl/TestSaslMechanism.java	New test helper providing a controllable SASL mechanism for testing
xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java	Comprehensive test suite covering SASL1, SASL2, authentication flows, user agent capture, and edge cases
xmppserver/src/test/java/org/jivesoftware/openfire/net/UserAgentInfoTest.java	Test suite for user agent information extraction and validation

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

guusdk

Thanks for this! Apologies for taking so long to review it.

I've extensively reviewed the PR, and have left a mix of comments: some relate to code style, others to implementation issues.

guusdk · 2026-04-09T13:24:31Z

    }

-    public static Element getSASLMechanismsElement( ClientSession session )
+    public static void addSASLMechanisms( @Nonnull List<Element> features, @Nonnull LocalSession session )


This may be me, but I find this code hard to reason over, because of its mutating design. In general, a method that returns data (which the caller then adds) is usually easier to reason about, test, and reuse.

The mutating design is a awkward here, as the code has a "getter-like" concern ("what mechanisms apply?"), unlike something like a Builder, where I'd expect to see this design.

The code here performs a side effect on a passed-in container (Element / List<Element>). That works, but it makes call sites harder to reason about and is a bit inconsistent with the get... style used elsewhere in this class.

Would it be cleaner to have a pure method that returns the mechanism elements for a session (for example getSASLMechanisms(LocalSession): List<Element>), and let callers add those to their own XML structure? That would make side effects explicit at the call site, reduce duplication between the two overloads, and generally simplify things.

guusdk · 2026-04-09T13:27:44Z

-     * @param session The current session
-     *
-     * @return The valid SASL mechanisms available for the specified session.
+     * addSASLMechanisms adds suitable mechanisms to either an Element (intended to be the features element)


Please avoid writing javadoc (that by nature is tied to exactly one method) that describes several overloaded methods. Just give each method it's own javadoc. I spent a good 5 minutes analyzing usages of this method, trying to figure out how the list of elements came into play, before noticing that there was a different method that simply accept a List instead of a singular Element as an argument.

guusdk · 2026-04-09T13:33:29Z

    {
        // Add (proprietary) Providers of SASL implementation to the Java security context.
-        Security.addProvider( new org.jivesoftware.openfire.sasl.SaslProvider() );
+        if (Security.getProvider( "JiveSoftware" ) == null) {


Guarding to avoid duplicate registration seems like the right behavior. I'm wondering if this needs to happen in different places, too.

I'm interested in finding out how you ran into this being an issue.

Is it worth pulling this change out into a separate ticket and PR? That both allows us to keep the SASL 2 PR focused, but also give this provider-registration hardening a dedicated review path.

guusdk · 2026-04-09T13:38:23Z

    private static final Pattern BASE64_ENCODED = Pattern.compile("^(=|([A-Za-z0-9+/]{4})*([A-Za-z0-9+/]{4}|[A-Za-z0-9+/]{3}=|[A-Za-z0-9+/]{2}==))$");

    private static final String SASL_NAMESPACE = "urn:ietf:params:xml:ns:xmpp-sasl";
+    private static final String SASL2_NAMESPACE = "urn:xmpp:sasl:2";


I don't see why we should keep these namespace declarations private. If anything, the SASLAuthentication class should be the authoritative source for such declarations, right?

guusdk · 2026-04-09T13:41:19Z

+     * @see <a href="https://xmpp.org/extensions/xep-0388.html">XEP-0388: Extensible SASL Profile</a>
+     */
+    public static final SystemProperty<Boolean> ENABLE_SASL2 = SystemProperty.Builder.ofType(Boolean.class)
+        .setKey("xmpp.auth.sasl2")


This needs an entry in openfire_i18n.properties, something like:

system_property.xmpp.auth.sasl2=Enables support for SASL2 authentication (XEP-0388) and advertisement of SASL2 stream features.

guusdk · 2026-04-10T12:49:29Z

+                if (authenticateClient(doc, true)) {
                    // SASL authentication was successful so open a new stream and offer
                    // resource binding and session establishment (to client sessions only)
                    saslSuccessful();


In this explicit SASL2 branch, saslSuccessful() is not compatible with SASL2. Notably, it sends a <stream:stream which is appropriate for SASL(1) only (SASL2 does not restart the stream upon success).

Also, the code comment is wrong here. It describes the SASL(1) functionality, not the SASL2 functionality, which will cause future-us to scratch behind the ears.

guusdk · 2026-04-10T12:54:49Z

+            }
+            if (saslStatus == SASLAuthentication.Status.authenticated && usingSASL2) {
+                Element features = generateFeatures();
+                session.deliverRawText(features.asXML());


Sending the (post SASL) features here happens after receiving "response" or "abort" from the client. Shouldn't it also happen after receiving "success"?

guusdk · 2026-04-10T13:22:02Z

+        }
+    }
+
+    public static Element getSASLMechanismsElement( ClientSession session, boolean usingSASL2 )


SASL2 mechanisms can only be advertised if the session is encrypted. I'm not sure if that's best enforced in this method, or in callers of this method, but if SASL2 is used, the corresponding mechanisms can be returned only if the session has already established TLS / encryption.

For SASL(1) RFC 6120, section 5.2 leaves some wiggle-room:

An initiating entity SHOULD use TLS to secure its stream with the receiving entity before proceeding with SASL authentication.

For SASL2, XEP-0388, section 3 is more strict:

SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation

guusdk · 2026-04-10T13:23:11Z

    }

-    public static Element getSASLMechanismsElement( LocalIncomingServerSession session )
+    public static Element getSASLMechanismsElement( LocalIncomingServerSession session, boolean usingSASL2 )


(same code review as the one I left for the method that processes client sessions): SASL2 mechanisms can only be advertised if the session is encrypted. I'm not sure if that's best enforced in this method, or in callers of this method, but if SASL2 is used, the corresponding mechanisms can be returned only if the session has already established TLS / encryption.

For SASL(1) RFC 6120, section 5.2 leaves some wiggle-room:

An initiating entity SHOULD use TLS to secure its stream with the receiving entity before proceeding with SASL authentication.

For SASL2, XEP-0388, section 3 is more strict:

SASL2 MUST only be used by Clients or offered by Servers after TLS negotiation

guusdk · 2026-04-10T14:55:41Z

+                        decoded = new byte[0];
                    }
-                    else if (encoded.equals("="))
+                    else if ( decoded.length == 0 )


I think something is wrong here. In the decodeData method, the emptyNull argument is used, in part, to determine if the resulting value (decoded) is null or an empty byte array. In the lines just above this, a null value is unconditionally replaced by an empty byte array, which happens before the return value of decodeData was otherwise evaluated. That suggests that either the implementation in decodeData that conditionally returns null is wrong (null values are immediately replaced anyways) or that this code is not correct.

Another reason for me to believe that something is wrong: for the SASL2 flow, the data argument to decodeData() will be null if there's no initial response (and emptyNull will be false), because of this in line 490-493:

if (elementType == ElementType.AUTHENTICATE) { data = doc.element("initial-response"); }

This will cause decodeData() to return an empty byte array (line 375):

if (doc == null) return new byte[0]; if (doc == null) return new byte[0];

By the time that we get to the code where I'm adding this review, that will cause the SASL_LAST_RESPONSE_WAS_PROVIDED_BUT_EMPTY to be set on the session - while in fact the SASL response did not provide an initial response.

Dave Cridland and others added 19 commits November 14, 2025 14:15

First crack at SASL2 (XEP-0388)

611fbd2

Bit more SASL2 WIP

86e4a24

SASL2 WIP - compiling/passing existing tests

8edbaf5

Add basic tests

92407f2

Features tests

c75543e

Auth tests (failing)

2eecb2d

Auth tests (passing)

d6bbce3

Add tests for additional data, initial responses, and multistep

b216ada

Add back null vs empty processing

fef20b6

Typoish in comment

00902b3

Disable SASL2 by default, support it on S2S

2168470

Base UserAgentInfo class

bbdf000

UserAgentInfo tests

81a0f06

Wire in UserAgentInfo extraction and storage

db78de0

Tests for wiring in UserAgentInfo extraction and storage

41a45bd

Move the tests

b68fa0a

Linty winty

213a90f

Fix feature name

778b6ac

SASL2 working, Bind2 ignored.

c3498b0

# Conflicts: # xmppserver/src/main/java/org/jivesoftware/openfire/net/Bind2Request.java # xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java

dwd requested review from Copilot and guusdk December 18, 2025 11:00

Copilot started reviewing on behalf of dwd December 18, 2025 11:00 View session

Copilot AI reviewed Dec 18, 2025

View reviewed changes

dwd marked this pull request as ready for review April 3, 2026 12:00

dwd added 2 commits April 3, 2026 13:01

Merge remote-tracking branch 'ignite/main' into sasl-2-clean

06d7220

Address copilot review points

336d78c

dwd requested a review from Fishbowler April 3, 2026 13:28

guusdk requested changes Apr 10, 2026

View reviewed changes

Uh oh!

Conversation

dwd commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

guusdk left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants