OF-3305: Abort Blowfish PBKDF2 migration when security.xml is not persistable#3385
Open
viv wants to merge 2 commits into
Open
Conversation
…able When conf/security.xml cannot be loaded or written (missing, empty, corrupt or not writable), JiveGlobals falls back to a non-persisting, in-memory-only properties object. The SHA1-to-PBKDF2 migration would generate a PBKDF2 salt, re-encrypt and commit the database properties, and report success, while the salt and the kdf=pbkdf2 flag were silently discarded. After a restart the server defaulted back to SHA1 with no salt and could no longer decrypt the re-encrypted values (including the keystore passwords, which are stored as encrypted database properties), so an HTTPS-only admin console failed to start. The randomly generated salt is unrecoverable, leaving the data readable only from a pre-migration backup. Fail fast before any irreversible change: - Add XMLProperties.isPersistable() and JiveGlobals.isSecurityPropertiesPersistable(). - migrateXMLPropertiesFromSHA1ToPBKDF2() now throws before deriving any key or touching the database when security properties are not persistable. The servlet runs this before database work and reports no database changes were made. - BlowfishMigrationServlet.doPost() adds a pre-flight check that blocks migration with a clear admin message before any changes. getBlowfishSalt() and setBlowfishKdf() are intentionally left unchanged (no global throw): they are also used by legitimate non-persisted scenarios (including the unit-test environment), so the targeted guard prevents the dangerous path without altering unrelated behaviour. Also corrects a misleading log message in loadSecurityProperties() that reported the openfire.xml path when it was actually security.xml that failed to load, which sent investigators to the wrong file during diagnosis. See: OF-3305, ADR-004
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThis PR addresses OF-3305 by preventing unsafe Blowfish-to-PBKDF2 migration when Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
xmppserver/src/test/java/org/jivesoftware/admin/servlet/BlowfishMigrationServletTest.java (2)
586-615: 💤 Low valueConsider consistent test naming convention.
The new test uses
should_X_when_Ynaming while existing tests followtestDoPost_Xconvention. While both styles are valid, consistency within a file improves readability.Optional: align with existing convention
-public void should_blockMigrationAndReportError_when_securityXmlNotPersistable() throws Exception { +public void testDoPost_SecurityXmlNotPersistable_BlocksMigration() throws Exception {🤖 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/test/java/org/jivesoftware/admin/servlet/BlowfishMigrationServletTest.java` around lines 586 - 615, The test method should_blockMigrationAndReportError_when_securityXmlNotPersistable in BlowfishMigrationServletTest uses a different naming style than the rest of the file; rename it to follow the existing testDoPost_* convention (e.g., testDoPost_securityXmlNotPersistable_blocksMigration) and update any references to the method (test runner will pick it up by reflection), keeping the assertions and calls to servlet.doPost(request, response), ClusterManager mocks, and verify(...) expectations unchanged.
623-635: 💤 Low valueConsider consistent test naming convention.
Same naming convention inconsistency as the previous test method.
Optional: align with existing convention
-public void should_throwWithSecurityXmlMessage_when_migratingWhileNotPersistable() { +public void testMigrateXMLPropertiesFromSHA1ToPBKDF2_SecurityXmlNotPersistable_ThrowsIllegalStateException() {🤖 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/test/java/org/jivesoftware/admin/servlet/BlowfishMigrationServletTest.java` around lines 623 - 635, The test method name uses underscores and is inconsistent with the project's test naming convention; rename the method should_throwWithSecurityXmlMessage_when_migratingWhileNotPersistable to match the existing convention used across BlowfishMigrationServletTest (e.g., camelCase like shouldThrowWithSecurityXmlMessageWhenMigratingWhileNotPersistable), update any references in the test class, and keep the method body and assertions (JiveGlobals.setBlowfishKdf and the assertThrows on JiveGlobals.migrateXMLPropertiesFromSHA1ToPBKDF2) 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/test/java/org/jivesoftware/admin/servlet/BlowfishMigrationServletTest.java`:
- Around line 586-615: The test method
should_blockMigrationAndReportError_when_securityXmlNotPersistable in
BlowfishMigrationServletTest uses a different naming style than the rest of the
file; rename it to follow the existing testDoPost_* convention (e.g.,
testDoPost_securityXmlNotPersistable_blocksMigration) and update any references
to the method (test runner will pick it up by reflection), keeping the
assertions and calls to servlet.doPost(request, response), ClusterManager mocks,
and verify(...) expectations unchanged.
- Around line 623-635: The test method name uses underscores and is inconsistent
with the project's test naming convention; rename the method
should_throwWithSecurityXmlMessage_when_migratingWhileNotPersistable to match
the existing convention used across BlowfishMigrationServletTest (e.g.,
camelCase like
shouldThrowWithSecurityXmlMessageWhenMigratingWhileNotPersistable), update any
references in the test class, and keep the method body and assertions
(JiveGlobals.setBlowfishKdf and the assertThrows on
JiveGlobals.migrateXMLPropertiesFromSHA1ToPBKDF2) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: dfa6a439-bd7b-4f54-91d2-ee6ee0b1bfea
📒 Files selected for processing (7)
doc/adr/004-manual-blowfish-pbkdf2-migration.mdi18n/src/main/resources/openfire_i18n.propertiesxmppserver/src/main/java/org/jivesoftware/admin/servlet/BlowfishMigrationServlet.javaxmppserver/src/main/java/org/jivesoftware/util/JiveGlobals.javaxmppserver/src/main/java/org/jivesoftware/util/XMLProperties.javaxmppserver/src/test/java/org/jivesoftware/admin/servlet/BlowfishMigrationServletTest.javaxmppserver/src/test/java/org/jivesoftware/util/XMLPropertiesTest.java
The four tests added for OF-3305 used a should_X_when_Y style, whereas the existing tests in BlowfishMigrationServletTest and XMLPropertiesTest use the testXxx convention. Rename them to match, for consistency within each file. No behavioural change. See: OF-3305
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
1 participant
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.
Problem
The Blowfish SHA1 to PBKDF2 migration (OF-3075) re-encrypts the database properties
with a PBKDF2 key derived from a freshly generated salt, and stores that salt and the
kdf=pbkdf2flag inconf/security.xml.When
conf/security.xmlcannot be loaded or written (it is missing, empty, corrupt,or not writable),
JiveGlobalsfalls back to a non-persisting, in-memory-onlyXMLPropertiesinstance. In that state:getBlowfishSalt()generates a salt and callssetProperty(), which cannot save(
IllegalStateException: No file specified). The error is logged and the booleanresult is ignored.
setBlowfishKdf()likewise fails to persist, and the tool reports success.After a restart,
security.xmlis still unusable, so the server defaults to thesha1KDF with no salt and can no longer decrypt the PBKDF2-encrypted values. Amongthose values are the certificate-store passwords (
*.keypass/*.trustpass), whichOpenfire stores as encrypted database properties. With them unreadable, the keystores
fail to load (
UnrecoverableKeyException: Password verification failed), including theWEBADMINstore used by the admin console, so the admin console cannot start its HTTPSlistener. On a server that reaches the admin console over HTTPS, this locks the
administrator out (in the original report there was no admin listener on either port,
and the server logged "admin console not started due to configuration settings"). The
randomly generated salt is unrecoverable, so the re-encrypted data can be recovered
only from a pre-migration database backup.
See OF-3305, ADR-004, ADR-005.
Follow-up to #3067 (OF-3075), which added this migration. Reported on the forums: https://discourse.igniterealtime.org/t/admin-gui-not-starting-after-migration-to-pbkdf2/96500
Fix
Fail fast, before any irreversible change, if the security configuration cannot be
persisted:
XMLProperties.isPersistable()(true only when backed by a file).JiveGlobals.isSecurityPropertiesPersistable().JiveGlobals.migrateXMLPropertiesFromSHA1ToPBKDF2()throwsIllegalStateExceptionif security properties are not persistable, before deriving any key or touching the
database. The servlet runs this before any database work and reports that no
database changes were made.
BlowfishMigrationServlet.doPost()adds a pre-flight check (after the CSRF, backup,and clustering gates) that blocks migration and shows a clear admin message
(
security.blowfish.migration.error.security-xml-not-writable).JiveGlobals.loadSecurityProperties()toname
security.xmlrather thanopenfire.xml.The check runs at the very start of the migration, before any key derivation or
database write, so an unwritable
security.xmlis reported to the administrator as aclear, actionable error and the database is never modified.
Testing
XMLPropertiesTest:isPersistable()returns false for a non-persisted instance andtrue for a file-backed one.
BlowfishMigrationServletTest: the migration throws with asecurity.xmlmessagewhen not persistable;
doPost()sets the new error key and redirects withoutattempting migration. Existing migration and clustering tests pass.
xmppserversuite green viamvn clean verify: 1911 tests, 0 failures, 0 errors.