OF-3327: Parameterize existing SCRAM implementation#3421
Conversation
📝 WalkthroughWalkthroughThis change generalizes SCRAM handling to use explicit HMAC and digest algorithm names instead of hardcoded SHA-1 values. ChangesPossibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java (1)
348-357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect parameterization; consider extracting shared key-derivation helper.
Switching to explicit
HMAC_ALGORITHM_NAME/DIGEST_ALGORITHM_NAMEconstants here is correct and matches the newScramUtilsoverloads. However, this salted-password → clientKey → storedKey derivation is duplicated almost verbatim insetPassword(Lines 394-397), plussetPasswordadditionally derivesserverKey. Since the PR's stated goal is to support additional SCRAM mechanisms (e.g.SCRAM-SHA-256), this duplication will need to be repeated per-mechanism unless consolidated now.Consider extracting a small private helper (or a
ScramUtilsconvenience method) that takes(salt, password, iterations, hmacAlgorithm, digestAlgorithm)and returns the derivedstoredKey/serverKey, then have bothcheckPasswordandsetPasswordcall it.🤖 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/auth/DefaultAuthProvider.java` around lines 348 - 357, The SCRAM key-derivation logic in DefaultAuthProvider is duplicated between the password check path and setPassword, and it will have to be repeated for each new mechanism unless centralized. Extract the shared salted-password → clientKey → storedKey derivation into a small private helper (or ScramUtils convenience method) that accepts the salt, password, iterations, HMAC algorithm, and digest algorithm, and have both checkPassword and setPassword reuse it; also keep serverKey derivation alongside that helper for setPassword.xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoom.java (1)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider decoupling occupant-ID hashing from the SASL/SCRAM implementation.
generateOccupantIdnow importsScramSha1SaslServerpurely to reuseHMAC_ALGORITHM_NAMEfor an internal, authentication-unrelated hash (XEP-0421 occupant IDs). This ties a MUC-layer feature to a SASL-mechanism-specific constant, creating a somewhat awkwardmuc→sasldependency for a value that has no conceptual relation to SCRAM authentication. Functionally this is equivalent to the prior implicit SHA-1 default, so there's no behavior change, but a locally-defined constant (e.g. inMUCRoomor a shared crypto-utility) would better express intent and avoid an unrelated cross-module coupling.Also applies to: 4207-4209
🤖 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/muc/MUCRoom.java` at line 32, The generateOccupantId logic in MUCRoom is coupled to ScramSha1SaslServer only to reuse its HMAC algorithm constant, which creates an unnecessary MUC-to-SASL dependency. Replace that import with a locally defined or shared crypto utility constant in MUCRoom, and update the occupant-ID hashing code to reference that symbol directly so the intent stays internal and unrelated to SCRAM authentication.
🤖 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
`@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java`:
- Around line 348-357: The SCRAM key-derivation logic in DefaultAuthProvider is
duplicated between the password check path and setPassword, and it will have to
be repeated for each new mechanism unless centralized. Extract the shared
salted-password → clientKey → storedKey derivation into a small private helper
(or ScramUtils convenience method) that accepts the salt, password, iterations,
HMAC algorithm, and digest algorithm, and have both checkPassword and
setPassword reuse it; also keep serverKey derivation alongside that helper for
setPassword.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoom.java`:
- Line 32: The generateOccupantId logic in MUCRoom is coupled to
ScramSha1SaslServer only to reuse its HMAC algorithm constant, which creates an
unnecessary MUC-to-SASL dependency. Replace that import with a locally defined
or shared crypto utility constant in MUCRoom, and update the occupant-ID hashing
code to reference that symbol directly so the intent stays internal and
unrelated to SCRAM authentication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 57c1b45a-3bb8-4719-a64c-1715ea9a667f
📒 Files selected for processing (13)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/HybridAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.javaxmppserver/src/main/java/org/jivesoftware/openfire/ldap/LdapUserProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoom.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/SaslServerFactoryImpl.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/ScramUtilsTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1TestFixtures.java
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/auth/ScramUtils.java (1)
82-90: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winReject non-positive SCRAM iteration counts.
Line 90 skips the loop for
iters <= 0and returnsU1, silently treating an invalid SCRAM iteration count as one iteration.Proposed fix
public static byte[] createSaltedPassword(byte[] salt, String password, int iters, String hmacAlgorithm) throws SaslException { + if (iters < 1) { + throw new SaslException("SCRAM iteration count must be positive."); + } final Mac mac = createHmac(password.getBytes(StandardCharsets.UTF_8), hmacAlgorithm);🤖 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/auth/ScramUtils.java` around lines 82 - 90, Reject non-positive SCRAM iteration counts in createSaltedPassword before any hashing work starts, since the current loop in ScramUtils can silently treat iters <= 0 as a single iteration. Add an explicit validation at the start of createSaltedPassword that throws a SaslException for invalid iteration counts, and keep the existing SCRAM computation unchanged for valid values.
🧹 Nitpick comments (2)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.java (1)
147-179: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid coupling
ScramUtilsback to the SASL binding layer.These deprecated utility wrappers now depend on
ScramSha1SaslServerconstants, while the PR stack makes the SASL layer depend onScramUtils. Keep the SHA-1 legacy constants local to this utility to avoid an auth↔sasl package cycle.Proposed direction
+private static final String SCRAM_SHA_1_HMAC_ALGORITHM_NAME = "HmacSHA1"; + ... - return createSaltedPassword(salt, password, iters, ScramSha1SaslServer.HMAC_ALGORITHM_NAME); + return createSaltedPassword(salt, password, iters, SCRAM_SHA_1_HMAC_ALGORITHM_NAME); ... - return computeHmac(key, string, ScramSha1SaslServer.HMAC_ALGORITHM_NAME); + return computeHmac(key, string, SCRAM_SHA_1_HMAC_ALGORITHM_NAME); ... - return createHmac(keyBytes, ScramSha1SaslServer.HMAC_ALGORITHM_NAME); + return createHmac(keyBytes, SCRAM_SHA_1_HMAC_ALGORITHM_NAME);🤖 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/auth/ScramUtils.java` around lines 147 - 179, The deprecated legacy wrappers in ScramUtils are reaching back into ScramSha1SaslServer for SHA-1 constants, creating an unwanted auth↔sasl dependency cycle. Update createSaltedPassword, computeHmac, and createSha1Hmac to use a locally defined SHA-1 HMAC algorithm constant inside ScramUtils (or an equivalent local legacy constant) instead of referencing ScramSha1SaslServer, so the utility remains self-contained.xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java (1)
352-352: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winCompare SCRAM keys in constant time.
Avoid
String.equalsfor authentication key material. Decode the stored key and compare bytes withMessageDigest.isEqual; the existingIllegalArgumentExceptioncatch already covers malformed Base64.Suggested change
- return DatatypeConverter.printBase64Binary(keys.storedKey).equals(credential.storedKey); + return MessageDigest.isEqual(keys.storedKey, DatatypeConverter.parseBase64Binary(credential.storedKey));Also re-add:
+import java.security.MessageDigest;🤖 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/auth/DefaultAuthProvider.java` at line 352, The SCRAM key comparison in DefaultAuthProvider is using String.equals on Base64 text, which is not constant time. Update the logic around the credential/storedKey comparison to decode the stored key and compare the raw byte arrays with MessageDigest.isEqual, using the existing IllegalArgumentException handling for malformed Base64. Keep the change localized to the authentication path that handles keys.storedKey and credential.storedKey so the comparison remains secure and consistent.
🤖 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/auth/DefaultAuthProvider.java`:
- Around line 389-397: The SCRAM key derivation failure in DefaultAuthProvider’s
credential update flow is being logged but then allowed to continue with null
keys. Update the SCRAM persistence path around ScramUtils.deriveScramKeys and
the surrounding try/catch in the password update logic so that a SaslException
or NoSuchAlgorithmException aborts the update instead of persisting unusable
SCRAM data, especially in scramOnly mode. Use the existing DefaultAuthProvider
flow to return a failure or rethrow after logging, and make sure the same fix is
applied to the corresponding duplicated persistence block referenced by the same
SCRAM update code.
---
Outside diff comments:
In `@xmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.java`:
- Around line 82-90: Reject non-positive SCRAM iteration counts in
createSaltedPassword before any hashing work starts, since the current loop in
ScramUtils can silently treat iters <= 0 as a single iteration. Add an explicit
validation at the start of createSaltedPassword that throws a SaslException for
invalid iteration counts, and keep the existing SCRAM computation unchanged for
valid values.
---
Nitpick comments:
In
`@xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java`:
- Line 352: The SCRAM key comparison in DefaultAuthProvider is using
String.equals on Base64 text, which is not constant time. Update the logic
around the credential/storedKey comparison to decode the stored key and compare
the raw byte arrays with MessageDigest.isEqual, using the existing
IllegalArgumentException handling for malformed Base64. Keep the change
localized to the authentication path that handles keys.storedKey and
credential.storedKey so the comparison remains secure and consistent.
In `@xmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.java`:
- Around line 147-179: The deprecated legacy wrappers in ScramUtils are reaching
back into ScramSha1SaslServer for SHA-1 constants, creating an unwanted
auth↔sasl dependency cycle. Update createSaltedPassword, computeHmac, and
createSha1Hmac to use a locally defined SHA-1 HMAC algorithm constant inside
ScramUtils (or an equivalent local legacy constant) instead of referencing
ScramSha1SaslServer, so the utility remains self-contained.
🪄 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: 1332a137-9cef-4672-a345-27fd3edb051d
📒 Files selected for processing (3)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/ScramUtilsTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- xmppserver/src/test/java/org/jivesoftware/openfire/auth/ScramUtilsTest.java
| ScramUtils.ScramKeys keys = null; | ||
| final int iterations = ScramSha1SaslServer.ITERATION_COUNT.getValue(); | ||
| byte[] saltedPassword = null, clientKey = null, storedKey = null, serverKey = null; | ||
| try { | ||
| saltedPassword = ScramUtils.createSaltedPassword(saltShaker, password, iterations); | ||
| clientKey = ScramUtils.computeHmac(saltedPassword, "Client Key"); | ||
| storedKey = MessageDigest.getInstance("SHA-1").digest(clientKey); | ||
| serverKey = ScramUtils.computeHmac(saltedPassword, "Server Key"); | ||
| keys = ScramUtils.deriveScramKeys( | ||
| saltShaker, password, iterations, | ||
| ScramSha1SaslServer.HMAC_ALGORITHM_NAME, | ||
| ScramSha1SaslServer.DIGEST_ALGORITHM_NAME); | ||
| } catch (SaslException | NoSuchAlgorithmException e) { | ||
| Log.warn("Unable to persist values for SCRAM authentication."); | ||
| Log.warn("Unable to derive SCRAM credential while trying to persist values for SCRAM authentication", e); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Fail the password update if SCRAM key derivation fails.
Right now a derivation error is logged, then null keys are persisted. In scramOnly mode, that can remove plaintext/encrypted credentials while storing an unusable SCRAM row.
Suggested change
- ScramUtils.ScramKeys keys = null;
+ final ScramUtils.ScramKeys keys;
final int iterations = ScramSha1SaslServer.ITERATION_COUNT.getValue();
try {
keys = ScramUtils.deriveScramKeys(
saltShaker, password, iterations,
ScramSha1SaslServer.HMAC_ALGORITHM_NAME,
ScramSha1SaslServer.DIGEST_ALGORITHM_NAME);
} catch (SaslException | NoSuchAlgorithmException e) {
Log.warn("Unable to derive SCRAM credential while trying to persist values for SCRAM authentication", e);
+ throw new IllegalStateException("Unable to derive SCRAM credential.", e);
}
...
- keys == null ? null : DatatypeConverter.printBase64Binary(keys.storedKey),
- keys == null ? null : DatatypeConverter.printBase64Binary(keys.serverKey)
+ DatatypeConverter.printBase64Binary(keys.storedKey),
+ DatatypeConverter.printBase64Binary(keys.serverKey)Also applies to: 454-455
🤖 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/auth/DefaultAuthProvider.java`
around lines 389 - 397, The SCRAM key derivation failure in
DefaultAuthProvider’s credential update flow is being logged but then allowed to
continue with null keys. Update the SCRAM persistence path around
ScramUtils.deriveScramKeys and the surrounding try/catch in the password update
logic so that a SaslException or NoSuchAlgorithmException aborts the update
instead of persisting unusable SCRAM data, especially in scramOnly mode. Use the
existing DefaultAuthProvider flow to return a failure or rethrow after logging,
and make sure the same fix is applied to the corresponding duplicated
persistence block referenced by the same SCRAM update code.
This prepares the codebase to have additional SCRAM-based mechanisms (such as `SCRAM-SHA-256`). It does so by moving generic bits of the implementation that is currently specific to the implementation of `SCRAM-SHA-` into a base class that was introduced earlier for this purpose. The implementation in `ScramUtils` now no longer is specific the `SCRAM-SHA-1`: overloaded methods (that take an algorithm reference) have been introduced, the old methods that are implicitly specific to `SCRAM-SHA-1` have been deprecated. This all allows the `ScramSha1SaslServer` implementation to shrink significantly, to only this `SHA-1` bindings. Future implementations of algorithms like `SHA-256` should follow suit and be equally small/short.
Extract the common SCRAM key derivation logic from DefaultAuthProvider into ScramUtils, eliminating duplicated code between password verification and password storage.
95bb8b2 to
a105c22
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java (1)
676-677: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a shared
-PLUSconstant to avoid literal-concat duplication.
ScramSha1SaslServer.MECHANISM_NAME+"-PLUS"is constructed twice (here and at Line 777). Both remain valid compile-time constants for the switch case, so no functional issue — just a minor DRY opportunity if aMECHANISM_NAME_PLUSconstant were added toScramSha1SaslServer.♻️ Optional refactor
- case ScramSha1SaslServer.MECHANISM_NAME: // intended fall-through - case ScramSha1SaslServer.MECHANISM_NAME+"-PLUS": + case ScramSha1SaslServer.MECHANISM_NAME: // intended fall-through + case ScramSha1SaslServer.MECHANISM_NAME_PLUS:Also applies to: 777-777
🤖 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 676 - 677, The switch in SASLAuthentication repeats the SCRAM-SHA1 “-PLUS” mechanism string literal concatenation in multiple case labels. Add a shared MECHANISM_NAME_PLUS constant to ScramSha1SaslServer and update the switch cases in SASLAuthentication to use that constant instead of constructing the string inline, keeping the existing fall-through behavior unchanged.
🤖 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
`@xmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.java`:
- Around line 676-677: The switch in SASLAuthentication repeats the SCRAM-SHA1
“-PLUS” mechanism string literal concatenation in multiple case labels. Add a
shared MECHANISM_NAME_PLUS constant to ScramSha1SaslServer and update the switch
cases in SASLAuthentication to use that constant instead of constructing the
string inline, keeping the existing fall-through behavior unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9a54381e-ad44-4179-b913-b41168768d1f
📒 Files selected for processing (13)
xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/HybridAuthProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.javaxmppserver/src/main/java/org/jivesoftware/openfire/ldap/LdapUserProvider.javaxmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoom.javaxmppserver/src/main/java/org/jivesoftware/openfire/net/SASLAuthentication.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/SaslServerFactoryImpl.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSaslServer.javaxmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/auth/ScramUtilsTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.javaxmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1TestFixtures.java
🚧 Files skipped from review as they are similar to previous changes (10)
- xmppserver/src/main/java/org/jivesoftware/openfire/ldap/LdapUserProvider.java
- xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1TestFixtures.java
- xmppserver/src/main/java/org/jivesoftware/openfire/sasl/SaslServerFactoryImpl.java
- xmppserver/src/test/java/org/jivesoftware/openfire/auth/DefaultAuthProviderScramStorageTest.java
- xmppserver/src/main/java/org/jivesoftware/openfire/muc/MUCRoom.java
- xmppserver/src/test/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServerTest.java
- xmppserver/src/test/java/org/jivesoftware/openfire/auth/ScramUtilsTest.java
- xmppserver/src/main/java/org/jivesoftware/openfire/auth/DefaultAuthProvider.java
- xmppserver/src/main/java/org/jivesoftware/openfire/sasl/ScramSha1SaslServer.java
- xmppserver/src/main/java/org/jivesoftware/openfire/auth/ScramUtils.java
This prepares the codebase to have additional SCRAM-based mechanisms (such as
SCRAM-SHA-256). It does so by moving generic bits of the implementation that is currently specific to the implementation ofSCRAM-SHA-into a base class that was introduced earlier for this purpose.The implementation in
ScramUtilsnow no longer is specific theSCRAM-SHA-1: overloaded methods (that take an algorithm reference) have been introduced, the old methods that are implicitly specific toSCRAM-SHA-1have been deprecated.This all allows the
ScramSha1SaslServerimplementation to shrink significantly, to only thisSHA-1bindings. Future implementations of algorithms likeSHA-256should follow suit and be equally small/short.