imbajin commented on code in PR #2918:
URL:
https://github.com/apache/incubator-hugegraph/pull/2918#discussion_r2629503615
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/auth/StandardAuthManagerV2.java:
##########
@@ -947,6 +947,7 @@ public HugeUser matchUser(String name, String password) {
}
if (StringEncoding.checkPassword(password, user.password())) {
+ // TODO: rehash password if bcrypt work factor is lower than
expected
Review Comment:
**‼️ Critical: Duplicate code - missing upgrade logic here too**
This is identical to StandardAuthManager.java but also needs the automatic
password rehashing implementation. See the comment on StandardAuthManager.java
for the suggested fix.
Apply the same solution here to ensure both auth managers handle password
upgrades consistently.
##########
hugegraph-server/hugegraph-test/src/main/java/org/apache/hugegraph/unit/util/StringEncodingTest.java:
##########
@@ -180,4 +181,23 @@ public void testReadAsciiString() {
buf = Bytes.fromHex("80");
Assert.assertEquals("", StringEncoding.readAsciiString(buf, 0));
}
+
+ @Test
+ public void testCheckPasswordSupportsOldAndNewCost() {
+ String testPassword = "test123!@#";
+
+ // Test old work factor (4)
+ String oldPassword = BCrypt.hashpw(testPassword, BCrypt.gensalt(4));
+ Assert.assertTrue(StringEncoding.checkPassword(testPassword,
oldPassword));
+ Assert.assertFalse(StringEncoding.checkPassword("wrong", oldPassword));
+
+ // Test new work factor (10)
+ String newPassword = BCrypt.hashpw(testPassword, BCrypt.gensalt(10));
+ Assert.assertTrue(StringEncoding.checkPassword(testPassword,
newPassword));
+ Assert.assertFalse(StringEncoding.checkPassword("wrong", newPassword));
+
+ // Test that hashPassword uses the new cost factor
+ String hashedPassword = StringEncoding.hashPassword(testPassword);
+ Assert.assertTrue(StringEncoding.checkPassword(testPassword,
hashedPassword));
Review Comment:
**⚠️ Important: Missing test coverage for actual security requirement**
The test validates backward compatibility but doesn't verify the security
improvement. Missing test cases:
1. **Performance test**: Verify work factor 10 takes significantly longer
than work factor 4 (should be ~64x slower, around 100-200ms)
2. **Brute force resistance**: Document the computational cost difference
3. **Hash format validation**: Verify new hashes contain `$2a$10$` prefix
**Suggested addition:**
```suggestion
@Test
public void testHashPasswordUsesSecureWorkFactor() {
String testPassword = "test123!@#";
String hashedPassword = StringEncoding.hashPassword(testPassword);
// Verify work factor 10 is used (hash format: a$...)
Assert.assertTrue("Hash should use work factor 10",
hashedPassword.startsWith("a$"));
// Verify computational cost (work factor 10 should take 50-300ms)
long startTime = System.currentTimeMillis();
StringEncoding.checkPassword(testPassword, hashedPassword);
long elapsed = System.currentTimeMillis() - startTime;
Assert.assertTrue("Work factor 10 should take at least 50ms",
elapsed >= 50);
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
