imbajin commented on code in PR #2918:
URL:
https://github.com/apache/incubator-hugegraph/pull/2918#discussion_r2618200070
##########
hugegraph-server/hugegraph-core/pom.xml:
##########
@@ -198,7 +198,7 @@
<version>${commons-compress.version}</version>
</dependency>
<dependency>
- <groupId>org.lz4</groupId>
+ <groupId>at.yawk.lz4</groupId>
<artifactId>lz4-java</artifactId>
Review Comment:
‼️ **Critical: Incorrect GroupId for lz4-java**
The dependency change switches from `org.lz4:lz4-java` to
`at.yawk.lz4:lz4-java`, but this appears to be incorrect. The official lz4-java
artifact is maintained under the `org.lz4` groupId.
The `at.yawk.lz4` groupId points to an unofficial fork that may not be
maintained and could introduce compatibility or security issues.
**Recommendation:** Keep the original `org.lz4` groupId and only update the
version to 1.9.0:
```suggestion
<dependency>
<groupId>org.lz4</groupId>
<artifactId>lz4-java</artifactId>
<version>${lz4.version}</version>
</dependency>
```
##########
hugegraph-struct/pom.xml:
##########
@@ -109,9 +109,9 @@
<version>8.1.0</version>
</dependency>
<dependency>
- <groupId>org.lz4</groupId>
+ <groupId>at.yawk.lz4</groupId>
<artifactId>lz4-java</artifactId>
Review Comment:
‼️ **Critical: Incorrect GroupId for lz4-java (hugegraph-struct)**
Same issue as in hugegraph-core - using an unofficial fork `at.yawk.lz4`
instead of the official `org.lz4` groupId.
```suggestion
<dependency>
<groupId>org.lz4</groupId>
<artifactId>lz4-java</artifactId>
<version>1.9.0</version>
</dependency>
```
##########
hugegraph-server/hugegraph-core/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -140,7 +140,7 @@ public static String decompress(byte[] value, float
bufferRatio) {
}
public static String hashPassword(String password) {
- return BCrypt.hashpw(password, BCrypt.gensalt(4));
+ return BCrypt.hashpw(password, BCrypt.gensalt(12));
Review Comment:
⚠️ **Important: BCrypt Work Factor Change Needs Migration Strategy**
Increasing BCrypt rounds from 4 to 12 is a good security improvement, but
this will break authentication for all existing users whose passwords were
hashed with the old work factor (4 rounds).
**Impact:** Existing users won't be able to log in after this change.
**Recommendation:** Implement a gradual migration strategy:
1. Keep verifying old hashes (4 rounds) in `checkPassword()`
2. Re-hash passwords with 12 rounds on successful login
3. Consider adding a configuration option for the work factor
Example approach:
```java
public static String hashPassword(String password) {
// New hashes use 12 rounds
return BCrypt.hashpw(password, BCrypt.gensalt(12));
}
public static boolean checkPassword(String candidatePassword, String
dbPassword) {
boolean valid = BCrypt.checkpw(candidatePassword, dbPassword);
// TODO: Consider re-hashing with new work factor on successful login
return valid;
}
```
##########
hugegraph-struct/src/main/java/org/apache/hugegraph/util/StringEncoding.java:
##########
@@ -165,7 +165,7 @@ public static String decompress(byte[] value, float
bufferRatio) {
}
public static String hashPassword(String password) {
- return BCrypt.hashpw(password, BCrypt.gensalt(4));
+ return BCrypt.hashpw(password, BCrypt.gensalt(12));
Review Comment:
⚠️ **Important: BCrypt Work Factor Change (hugegraph-struct)**
Same BCrypt work factor change as in hugegraph-core. This will affect
password compatibility between modules.
Ensure both modules handle password migration consistently. See the comment
on hugegraph-core/StringEncoding.java for migration strategy details.
--
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]
