SASL 2 #3113

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.

Copilot · 2025-12-18T11:11:41Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/TestSaslMechanism.java

+     * A test SASL Server factory that creates our test mechanism
+     */
+    public static class TestSaslServerFactory implements SaslServerFactory {
+        private static TestSaslServer saslServer;


The static field saslServer in TestSaslServerFactory is accessed without synchronization. If tests run concurrently, this could lead to race conditions where one test's saslServer overwrites another's. Consider using ThreadLocal or synchronizing access to this field.

Copilot · 2025-12-18T11:11:42Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java

+//
+//    @AfterAll
+//    public static void tearDownClass() {
+//        CacheFactory.shutdown();
+//    }


The commented-out @afterall method should either be uncommented if needed or removed to maintain cleaner code. Leaving it commented suggests uncertainty about whether cleanup is needed.

Suggested change

//

// @AfterAll

// public static void tearDownClass() {

// CacheFactory.shutdown();

// }

Copilot · 2025-12-18T11:11:42Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java

+        long start = System.currentTimeMillis();
+        System.out.println("Starting setUp");
+
+        // Setup XMPPServer mock
+        XMPPServer.setInstance(xmppServer);
+        when(xmppServer.getServerInfo()).thenReturn(serverInfo);
+        when(xmppServer.createJID(anyString(), anyString())).thenReturn(new JID("foo@bar"));
+        when(xmppServer.createJID(anyString(), isNull())).thenReturn(new JID("foo@bar"));
+        when(xmppServer.createJID(anyString(), anyString(), anyBoolean())).thenReturn(new JID("foo@bar"));
+
+        // Setup ServerInfo mock
+        when(serverInfo.getXMPPDomain()).thenReturn("example.com");
+        when(serverInfo.getHostname()).thenReturn("openfire.example.com");
+
+        features = DocumentHelper.createElement("features");
+
+        // Create our test SASL server
+        testSaslServer = TestSaslMechanism.registerTestMechanism(clientSession);
+
+        // Enable our test mechanism
+        SASLAuthentication.addSupportedMechanism("TEST-MECHANISM");
+
+
+        sessionDataMap = new HashMap<>();
+        // Mock both get and set to use the real map
+        when(clientSession.getSessionData(anyString())).thenAnswer(inv ->
+            sessionDataMap.get(inv.getArgument(0)));
+
+        doAnswer(inv -> {
+            sessionDataMap.put(inv.getArgument(0), inv.getArgument(1));
+            return null;
+        }).when(clientSession).setSessionData(anyString(), any());
+
+
+        // Instead of setting property, directly set the provider through reflection
+        try {
+            Field providerField = LockOutManager.class.getDeclaredField("provider");
+            providerField.setAccessible(true);
+
+            // Create anonymous implementation
+            LockOutProvider mockProvider = new LockOutProvider() {
+                @Override
+                public LockOutFlag getDisabledStatus(String username) {
+                    return null;
+                }
+                @Override
+                public void setDisabledStatus(LockOutFlag flag) {}
+                @Override
+                public void unsetDisabledStatus(String username) {}
+                @Override
+                public boolean isReadOnly() { return false; }
+                @Override
+                public boolean isDelayedStartSupported() { return false; }
+                @Override
+                public boolean isTimeoutSupported() { return false; }
+                @Override
+                public boolean shouldNotBeCached() { return true; }
+            };
+
+            providerField.set(null, mockProvider);
+        } catch (Exception e) {
+            fail("Could not set mock provider: " + e.getMessage());
+        }
+
+        long end = System.currentTimeMillis();
+        System.out.println("Finished setUp in " + (end-start) + "ms");


The debug print statements (lines 78-79, 142-143, 213-234) should be removed or replaced with proper logging. These appear to be leftover debugging code that doesn't add value to the tests.

Copilot · 2025-12-18T11:11:43Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/TestSaslMechanism.java

+        if (Security.getProvider("Test Provider") == null) {
+            TestSaslServer testSaslServer = new TestSaslServer(clientSession);
+
+            // Set the server instance before registering the provider
+            TestSaslServerFactory.setSaslServer(testSaslServer);
+
+            // Register the provider if not already registered
+            Security.addProvider(new Provider("Test Provider", "1.0", "Test Provider") {{
+                put("SaslServerFactory.TEST-MECHANISM", TestSaslServerFactory.class.getName());
+            }});
+        }
+
+        return TestSaslServerFactory.saslServer;


The method registerTestMechanism always returns the static TestSaslServerFactory.saslServer even when a provider already exists. If called multiple times with different sessions, the first session's TestSaslServer will be returned, not the newly created one. This can cause tests to fail or behave unexpectedly because the returned server instance won't match the clientSession parameter passed in.

Suggested change

if (Security.getProvider("Test Provider") == null) {

TestSaslServer testSaslServer = new TestSaslServer(clientSession);

// Set the server instance before registering the provider

TestSaslServerFactory.setSaslServer(testSaslServer);

// Register the provider if not already registered

Security.addProvider(new Provider("Test Provider", "1.0", "Test Provider") {{

put("SaslServerFactory.TEST-MECHANISM", TestSaslServerFactory.class.getName());

}});

}

return TestSaslServerFactory.saslServer;

// Always create a new TestSaslServer for the provided client session

TestSaslServer testSaslServer = new TestSaslServer(clientSession);

// Ensure the factory returns the server instance associated with this session

TestSaslServerFactory.setSaslServer(testSaslServer);

// Register the provider if not already registered

if (Security.getProvider("Test Provider") == null) {

Security.addProvider(new Provider("Test Provider", "1.0", "Test Provider") {{

put("SaslServerFactory.TEST-MECHANISM", TestSaslServerFactory.class.getName());

}});

}

// Return the server instance tied to the given client session

return testSaslServer;

Copilot · 2025-12-18T11:11:43Z

xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java

+
+            if (elementType == ElementType.AUTHENTICATE) {
+                if (!usingSASL2) {
+                    throw new IllegalStateException("Unexpected data received while negotiating SASL2 authentication. Name of the offending root element: " + doc.getName() + " Namespace: " + doc.getNamespaceURI());


The error message for ElementType.AUTHENTICATE incorrectly states "negotiating SASL2 authentication" when it should say "negotiating SASL authentication" (not SASL2), since this branch is triggered when receiving an AUTHENTICATE element while NOT using SASL2.

Suggested change

throw new IllegalStateException("Unexpected data received while negotiating SASL2 authentication. Name of the offending root element: " + doc.getName() + " Namespace: " + doc.getNamespaceURI());

throw new IllegalStateException("Unexpected data received while negotiating SASL authentication. Name of the offending root element: " + doc.getName() + " Namespace: " + doc.getNamespaceURI());

Copilot · 2025-12-18T11:11:45Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java

+    @BeforeAll
+    public static void setUpClass() throws Exception {
+        CacheFactory.initialize();
+        // Set this or I can't set anythign else.


Spelling error in comment: "anythign" should be "anything".

Suggested change

// Set this or I can't set anythign else.

// Set this or I can't set anything else.

Copilot · 2025-12-18T11:11:45Z

xmppserver/src/test/java/org/jivesoftware/openfire/sasl/SASLAuthenticationTest.java

+        long t0 = System.currentTimeMillis();
+        System.out.println("Test starting: " + System.currentTimeMillis());
+
+        Set<String> implemented = SASLAuthentication.getImplementedMechanisms();
+        System.out.println("After getImplementedMechanisms: " + (System.currentTimeMillis() - t0));
+        Set<String> mechanisms = SASLAuthentication.getSupportedMechanisms();
+        System.out.println("After getSupportedMechanisms: " + (System.currentTimeMillis() - t0));
+        assertNotNull(mechanisms);
+        System.out.println("After assertNotNull: " + (System.currentTimeMillis() - t0));
+
+        // Add multiple mechanisms and verify they're all present
+        SASLAuthentication.addSupportedMechanism("PLAIN");
+        System.out.println("After add PLAIN: " + (System.currentTimeMillis() - t0));
+        SASLAuthentication.addSupportedMechanism("DIGEST-MD5");
+        System.out.println("After add DIGEST-MD5: " + (System.currentTimeMillis() - t0));
+
+        mechanisms = SASLAuthentication.getSupportedMechanisms();
+        System.out.println("After second getSupportedMechanisms: " + (System.currentTimeMillis() - t0));
+
+        assertTrue(mechanisms.contains("PLAIN"));
+        assertTrue(mechanisms.contains("DIGEST-MD5"));
+        System.out.println("Test ending: " + (System.currentTimeMillis() - t0));


The debug print statements (lines 213-234) should be removed or replaced with proper logging. These appear to be leftover debugging code that doesn't add value to the test.

Suggested change

long t0 = System.currentTimeMillis();

System.out.println("Test starting: " + System.currentTimeMillis());

Set<String> implemented = SASLAuthentication.getImplementedMechanisms();

System.out.println("After getImplementedMechanisms: " + (System.currentTimeMillis() - t0));

Set<String> mechanisms = SASLAuthentication.getSupportedMechanisms();

System.out.println("After getSupportedMechanisms: " + (System.currentTimeMillis() - t0));

assertNotNull(mechanisms);

System.out.println("After assertNotNull: " + (System.currentTimeMillis() - t0));

// Add multiple mechanisms and verify they're all present

SASLAuthentication.addSupportedMechanism("PLAIN");

System.out.println("After add PLAIN: " + (System.currentTimeMillis() - t0));

SASLAuthentication.addSupportedMechanism("DIGEST-MD5");

System.out.println("After add DIGEST-MD5: " + (System.currentTimeMillis() - t0));

mechanisms = SASLAuthentication.getSupportedMechanisms();

System.out.println("After second getSupportedMechanisms: " + (System.currentTimeMillis() - t0));

assertTrue(mechanisms.contains("PLAIN"));

assertTrue(mechanisms.contains("DIGEST-MD5"));

System.out.println("Test ending: " + (System.currentTimeMillis() - t0));

SASLAuthentication.getImplementedMechanisms();

Set<String> mechanisms = SASLAuthentication.getSupportedMechanisms();

assertNotNull(mechanisms);

// Add multiple mechanisms and verify they're all present

SASLAuthentication.addSupportedMechanism("PLAIN");

SASLAuthentication.addSupportedMechanism("DIGEST-MD5");

mechanisms = SASLAuthentication.getSupportedMechanisms();

assertTrue(mechanisms.contains("PLAIN"));

assertTrue(mechanisms.contains("DIGEST-MD5"));

Copilot · 2025-12-18T11:11:46Z

xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java

+                startedSASL = true;
+                usingSASL2 = true;
+                saslStatus = SASLAuthentication.handle(session, doc, usingSASL2);
        } else if (startedSASL && "response".equals(tag) || "abort".equals(tag)) {


The condition on line 212 has incorrect operator precedence. It will evaluate as (startedSASL && "response".equals(tag)) || "abort".equals(tag), meaning "abort" will be processed even when startedSASL is false. This should be startedSASL && ("response".equals(tag) || "abort".equals(tag)).

Suggested change

} else if (startedSASL && "response".equals(tag) || "abort".equals(tag)) {

} else if (startedSASL && ("response".equals(tag) || "abort".equals(tag))) {

Copilot · 2025-12-18T11:11:46Z

xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java

+                // User is trying to authenticate using SASL2.
+                startedSASL = true;
+                usingSASL2 = true;
+                saslStatus = SASLAuthentication.handle(session, doc, usingSASL2);


Inconsistent indentation: the "authenticate" block uses extra indentation (lines 208-211) compared to the surrounding code blocks. This should use the same indentation level as the "auth" block above it.

Suggested change

// User is trying to authenticate using SASL2.

startedSASL = true;

usingSASL2 = true;

saslStatus = SASLAuthentication.handle(session, doc, usingSASL2);

// User is trying to authenticate using SASL2.

startedSASL = true;

usingSASL2 = true;

saslStatus = SASLAuthentication.handle(session, doc, usingSASL2);

Copilot · 2025-12-18T11:11:46Z

xmppserver/src/main/java/org/jivesoftware/openfire/net/StanzaHandler.java

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


Variable session may be null at this access as suggested by this null guard.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Uh oh!

SASL 2 #3113

SASL 2 #3113

dwd commented Dec 18, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Copilot AI Dec 18, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

	throw new IllegalStateException("Unexpected data received while negotiating SASL2 authentication. Name of the offending root element: " + doc.getName() + " Namespace: " + doc.getNamespaceURI());
	throw new IllegalStateException("Unexpected data received while negotiating SASL authentication. Name of the offending root element: " + doc.getName() + " Namespace: " + doc.getNamespaceURI());

	// Set this or I can't set anythign else.
	// Set this or I can't set anything else.

	} else if (startedSASL && "response".equals(tag) \|\| "abort".equals(tag)) {
	} else if (startedSASL && ("response".equals(tag) \|\| "abort".equals(tag))) {

Uh oh!

SASL 2 #3113

Are you sure you want to change the base?

SASL 2 #3113

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!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Dec 18, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant