OF-3258: Guard against user enumeration in ScramSha1SaslServer#3288
Merged
guusdk merged 9 commits intoigniterealtime:mainfrom Apr 27, 2026
Merged
OF-3258: Guard against user enumeration in ScramSha1SaslServer#3288guusdk merged 9 commits intoigniterealtime:mainfrom
guusdk merged 9 commits intoigniterealtime:mainfrom
Conversation
This replaces the use of randomly generated salts for unknown users with a deterministic but cryptographically unpredictable value derived from the username and a server-side secret. Prior to this change, a non-deterministic salt was used, which can be used (by retrieving it more than once) to determine if a user exists.
Member
Author
|
There is some overlap between the implementations in these two PRs:
Notably, the introduction of the same system property. Both PRs should introduce an exact duplicate. This, however, could trigger a merge conflict. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses a user-enumeration risk in SCRAM-SHA-1 SASL authentication by replacing non-deterministic “fake” salts (for unknown users) with deterministic salts derived from a username and a server-side secret.
Changes:
- Introduces a server-side secret system property and derives deterministic fake salts for unknown users.
- Refactors salt retrieval to
getOrCreateSalt, including handling for missing salts. - Adds unit tests and i18n strings for the new system property.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 8 comments.
| File | Description |
|---|---|
xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.java |
Adds server secret property, deterministic fake salt generation, and refactors salt retrieval logic. |
xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerSaltTest.java |
Adds tests for deterministic fake salts and behavior when server secret changes. |
i18n/src/main/resources/openfire_i18n.properties |
Adds description text for the new system property. |
i18n/src/main/resources/openfire_i18n_nl.properties |
Adds Dutch translation for the new system property description. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…ISTING_USERS The documentation for SERVER_SECRET_NONEXISTING_USERS incorrectly stated that the value is used for salt derivation only. In practice, this secret is used more broadly to derive deterministic, fake SCRAM credentials for non-existing users, including stored keys and server keys (and where applicable salt values). Update the Javadoc and i18n labels to accurately reflect this behavior.Additionally, document the effect of changing (rotating) this value.
…secret constant Having an empty value for the server secret value is unlikely to happen, but should be replaced. This is an easy hardening with no downside.
When an AuthProvider cannot return a password, it should throw one of the exceptions defined in the contract, rather than return a null password. This change guards against misbehaving providers that return null, by interpreting null in the same way as a non-existent user and/or missing support for password retrieval.
The usage of fake salts is to avoid attackers to determine if a username exists. For this to work, the salts need to be indistinguishable from real ones. Using the same size for both helps.
…EXISTENT_USERS The previous method for one-time initialization of the SERVER_SECRET_NONEXISTENT_USERS property (which depended on a static initializer block) proved to be fragile. In this commit, initialization happens in a getter that should be used instead of directly accessing the property.
…setting the property after each test.
Member
Author
Fishbowler
approved these changes
Apr 26, 2026
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.
This replaces the use of randomly generated salts for unknown users with a deterministic but cryptographically unpredictable value derived from the username and a server-side secret.
Prior to this change, a non-deterministic salt was used, which can be used (by retrieving it more than once) to determine if a user exists.