Support better algorithms for password hashing and add encryption with salt - #7111
Support better algorithms for password hashing and add encryption with salt#7111stweil wants to merge 11 commits into
Conversation
- Replace unsalted MD5/SHA password hashing with SSHA format in LdapUser.configure() and LdapServerService.changeUserPassword() for CWE-759 compliance (CWE-759: Choice of Less-Protected Protection Mechanism). LM/NTLM hashes are retained for AD compatibility. Assisted-by: qwen3.6-36b (Alibaba) Signed-off-by: Stefan Weil <sw@weilnetz.de>
Add a comment explaining that the weak LM and NTLM password hashes are required for Active Directory and Samba compatibility and cannot be changed without breaking AD domain joins. Also fix a typo in an existing comment: "Enconding" -> "Encoding" and "und" -> "and". This accompanies commit 7ef9cde0c3 which replaced unsalted MD5/SHA with SSHA format for the userPassword attribute. Assisted-by: minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
Change hardcoded SHA-1 to use configurable algorithm (SHA or MD5)
from PasswordEncryption setting. Both algorithms now use a random
salt and produce {SSHA} or {SMD5} format respectively, per LDAP
standards (RFC 2307). This restores admin-configurable password
hashing while maintaining the security fix from commit 7ef9cde0c3.
Assisted-by: minimax-m2.7 (MiniMax)
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Add getLdapPrefix() method to PasswordEncryption enum so the RFC 2307
prefix ({SSHA}, {SMD5}, {SSHA-256}) is defined alongside the algorithm
name, not duplicated in switch/if-else logic across service classes.
Refactor LdapUser.configure() and LdapServerService.changeUserPassword()
to use passwordEncryption.getLdapPrefix() instead of inline string
mapping. Also mark enum fields as final for immutability.
Assisted-by: minimax-m2.7 (MiniMax)
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Add PasswordEncryptionTest with 7 tests:
- Verify getValue(), getTitle(), getLdapPrefix() for SHA, MD5, SHA-256
- Verify getEncryptionFromValue() with valid values (0, 1, 2)
- Verify getEncryptionFromValue() returns default SHA for null/unknown
- Verify all enum values have unique getValue()
Add LdapUserTest with 4 tests:
- Verify {SSHA} prefix and correct salted hash for SHA algorithm
- Verify {SMD5} prefix and correct salted hash for MD5 algorithm
- Verify {SSHA-256} prefix and correct salted hash for SHA-256 algorithm
- Verify sambaLMPassword and sambaNTPassword are set
Also fix missing SecureRandom import in LdapServerService.
Assisted-by: minimax-m2.7 (MiniMax)
Signed-off-by: Stefan Weil <sw@weilnetz.de>
Assisted-by: OpenCode + minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
Assisted-by: OpenCode + minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
Signed-off-by: Stefan Weil <sw@weilnetz.de>
|
This is currently a draft pull request until it was tested with existing and new installations. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 42 |
| Duplication | 4 |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
|
As far as I know @thomaslow is working on this as part of #6208. (@stweil I think you were in the development fund meeting where this was commisioned) Maybe you could coordinate your efforts with him so this work isn't done twice. |
|
I know, and that's one of the reasons why I marked it as draft and did not invest more efforts. @thomaslow, feel free to re-use this PR or ignore it, whichever fits best for you. |
- Add AdaptivePasswordEncoder using Spring Security bcrypt (strength 16) - Add BCRYPT to PasswordEncryption enum - Update LdapUser and LdapServerService to use bcrypt for password hashing - Add unit tests for AdaptivePasswordEncoder and BCRYPT in LdapUserTest Assisted-by: OpenCode + minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
- Add SCRYPT and PBKDF2 support in AdaptivePasswordEncoder - Add SCRYPT and PBKDF2 enum values to PasswordEncryption - Update LdapUser and LdapServerService to use new algorithms - Fix SCryptPasswordEncoder constructor parameters - Fix scrypt hash prefix validation in tests Assisted-by: OpenCode + minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
| md.update(inPassword.getBytes(StandardCharsets.UTF_8)); | ||
| String encodedDigest = new String(Base64.encodeBase64(md.digest()), StandardCharsets.UTF_8); | ||
| this.attributes.put("userPassword", "{" + passwordEncrytion + "}" + encodedDigest); | ||
| private void setSambaPasswords(String inPassword, MD4Digest digester) throws NoSuchAlgorithmException { |
There was a problem hiding this comment.
If i understand correctly, the long term goal is to remove write access to the LDAP from Kitodo production, so enhancing password security would benefit an writable-LDAP implementation, which we eventually want to replace
There was a problem hiding this comment.
Thank you, this is an important information. So the question remains what "long term" means. Maybe removing write access can be done early to fix the issue?
There was a problem hiding this comment.
#4646 has been assessed in multiple development fund rounds but has not been funded yet. Newer Debian/Ubuntu versions are not only moving to newer Samba versions but keep on deprecating related functionality in newer Kernels. So i do not know if this is already urgent or if Debian 13/Ubuntu 26 still support the current architecture.
- Rename PBKDF2 methods to follow camelCase naming convention - Add Javadoc comments to PBKDF2 methods - Fix scrypt hash prefix in tests Assisted-by: OpenCode + minimax-m2.7 (MiniMax) Signed-off-by: Stefan Weil <sw@weilnetz.de>
|
I noticed that checkstyle complains: |
3 participants
This fixes the weak password hashing which is reported by Codacy.
Test cases for password hashing were missing and are added, too.