OF-3268 & OF-3273: Refactor SASL EXTERNAL S2S success path#3311
Open
guusdk wants to merge 7 commits into
Open
Conversation
…checks Remove redundant inbound S2S post-auth verification from `SASLAuthentication.handle(...)` after `saslServer.isComplete()`. For SASL EXTERNAL, certificate/domain verification is already performed by `ExternalServerSaslServer.evaluateResponse(...)`, while authentication method assignment is already handled in `authenticationSuccessful(...)`. This change simplifies control flow and clarifies responsibility without intended behavior change.
|
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: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCentralizes SASL mechanism eligibility selection, enforces session-specific mechanism availability during handle(), delegates mechanism-specific verification to the SaslServer on completion, maps EXTERNAL to SASL_EXTERNAL via a ServerSession helper, and adds unit tests for eligibility and completion behaviors. ChangesSASL Mechanisms & Completion Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR refactors the SASL S2S success path by removing an inbound server-side post-authentication certificate/domain verification block from SASLAuthentication.handle(...), relying instead on the SASL mechanism implementation (notably EXTERNAL) to perform any required verification.
Changes:
- Removed redundant inbound S2S certificate verification and authentication-method assignment from the
saslServer.isComplete()success path. - Added an explanatory comment clarifying that mechanism-specific verification is expected to happen inside the
SaslServerimplementation.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…thentication Ensure SASL mechanism selection in `SASLAuthentication.handle(...)` is constrained to the mechanisms available for the current session, matching stream feature advertisement behavior. Previously, Openfire validated mechanisms against globally enabled configuration only, which allowed peers to attempt mechanisms that were not advertised for a specific connection/session type. This change adds a session-scoped eligibility check and rejects non-available mechanisms with `invalid-mechanism`. This aligns mechanism acceptance with negotiated capabilities and prevents use of mechanisms outside per-session policy.
Stop assuming inbound s2s SASL authentication always uses EXTERNAL. When SASL succeeds, derive `ServerSession.AuthenticationMethod` from the actual negotiated mechanism name instead: - EXTERNAL -> SASL_EXTERNAL - any other mechanism -> OTHER This makes session state reflect the real authentication method used and removes brittle coupling to current mechanism availability.
…sions Replace Element-based mechanism lookups with direct set operations. Eliminates redundant dom4j manipulation. Adds unit test coverage None of this is expected to introduce functional changes. Pre-existing public method signatures are left intact for backwards compatibility.
guusdk
changed the title
guusdk
changed the title
![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
`@xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java`:
- Around line 831-846: getAvailableMechanismsForServerSession currently adds
"EXTERNAL" based only on TLS/certificate checks and can advertise a mechanism
that getSupportedMechanisms() (and configuration sasl.mechs) may have disabled;
update getAvailableMechanismsForServerSession to check the global supported
mechanism set (call getSupportedMechanisms() or otherwise consult sasl.mechs)
and only add "EXTERNAL" if it is present there as well. Also add a regression
test that disables EXTERNAL in sasl.mechs, ensures stream features do NOT
advertise EXTERNAL for an encrypted server session with a trusted cert, and
verifies the server rejects EXTERNAL when disabled. Ensure references to
getAvailableMechanismsForServerSession and getSupportedMechanisms() are used so
the gate is clear.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: da8150bc-a595-470f-889d-3a87b1f7c73f
📒 Files selected for processing (4)
xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaxmppserver/src/main/java/org/jivesoftware/openfire/session/ServerSession.javaxmppserver/src/test/java/org/jivesoftware/openfire/net/SASLAuthenticationTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/session/ServerSessionTest.java
✅ Files skipped from review due to trivial changes (1)
- xmppserver/src/main/java/org/jivesoftware/openfire/session/ServerSession.java
Only include EXTERNAL in available server-session mechanisms when it is enabled/supported, preventing it from being advertised in stream features when disabled in sasl.mechs.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java (1)
162-189:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPublish SASL mechanism refreshes atomically.
This listener now rebuilds
mechanismsfrom a dispatcher thread, butinitMechanisms()publishes a new mutableHashSetbefore it is fully populated. Readers inhandle()/getSupportedMechanisms()can observe an empty or partial set immediately after asasl.mechschange, which can transiently hide advertised mechanisms or reject valid auth attempts. Please switch this to a copy-on-write snapshot (or otherwise synchronize all reads/writes) before wiring it to async property callbacks.🤖 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 `@xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java` around lines 162 - 189, The PropertyEventListener currently calls initMechanisms() from a dispatcher thread which publishes a mutable HashSet into the shared mechanisms reference while it is being populated; change initMechanisms() to build the new set locally (e.g., a temporary HashSet), fully populate it, then atomically replace the shared reference with an unmodifiable/copy-on-write snapshot so readers in handle() and getSupportedMechanisms() never see a partially-built set; alternatively make mechanisms a volatile/AtomicReference<Set<String>> and swap the completed set, or synchronize all reads/writes to mechanisms, ensuring the listener uses the atomic replace approach before wiring into PropertyEventDispatcher.
🤖 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
`@xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java`:
- Around line 439-444: authenticationSuccessful(...) may short-circuit to
authenticationFailed(...) so the current code erroneously clears SaslServer,
records SaslMechanism and returns Status.authenticated even when authentication
actually failed; update authenticationSuccessful (in SASLAuthentication) to
return a boolean (or throw on failure) and here, after calling
authenticationSuccessful(session, saslServer.getAuthorizationID(),
saslServer.getMechanismName(), challenge), check its returned success flag (or
catch the thrown exception) and only then call
session.removeSessionData("SaslServer"), session.setSessionData("SaslMechanism",
saslServer.getMechanismName()) and return Status.authenticated; if
authenticationSuccessful indicates failure, do not clear/set session state and
instead return the appropriate failure Status (or propagate the exception) so
the caller does not see a successful status for a failed exchange.
---
Outside diff comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java`:
- Around line 162-189: The PropertyEventListener currently calls
initMechanisms() from a dispatcher thread which publishes a mutable HashSet into
the shared mechanisms reference while it is being populated; change
initMechanisms() to build the new set locally (e.g., a temporary HashSet), fully
populate it, then atomically replace the shared reference with an
unmodifiable/copy-on-write snapshot so readers in handle() and
getSupportedMechanisms() never see a partially-built set; alternatively make
mechanisms a volatile/AtomicReference<Set<String>> and swap the completed set,
or synchronize all reads/writes to mechanisms, ensuring the listener uses the
atomic replace approach before wiring into PropertyEventDispatcher.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0c1ae283-42d8-4455-80f8-8f57351ef243
📒 Files selected for processing (1)
xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java
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.
Remove redundant inbound S2S post-auth verification from
SASLAuthentication.handle(...)aftersaslServer.isComplete().For SASL EXTERNAL, certificate/domain verification is already performed by
ExternalServerSaslServer.evaluateResponse(...), while authentication method assignment is already handled inauthenticationSuccessful(...).This change simplifies control flow and clarifies responsibility without intended behavior change.
Additionally, this refactors `SASLAuthentication to no longer assumes that any (successful) authentication for S2S is SASL External. Instead, the method that was used is explicitly checked.
Some additional refactoring was added to clean up the code and introduce unit test coverage.
Summary by CodeRabbit
New Features
Bug Fixes
Tests