Migrate Ldap manager passwords to up-to-date encryption strategy - #7248
Draft
thomaslow wants to merge 3 commits into
Draft
Migrate Ldap manager passwords to up-to-date encryption strategy#7248thomaslow wants to merge 3 commits into
thomaslow wants to merge 3 commits into
Conversation
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 42 |
| Duplication | 2 |
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.
| */ | ||
| @Test | ||
| public void checkIsEncrypted() throws Exception { | ||
| String cipher = LegacyAesUtil.encrypt(INPUT, SECRET); |
| @Test | ||
| public void checkIsEncrypted() throws Exception { | ||
| String cipher = LegacyAesUtil.encrypt(INPUT, SECRET); | ||
| assertTrue(LegacyAesUtil.isEncrypted(cipher)); |
| assertTrue(LegacyAesUtil.isEncrypted(cipher)); | ||
|
|
||
| String potentialCipher = "Lorem Ipsum"; | ||
| assertFalse(LegacyAesUtil.isEncrypted(potentialCipher)); |
|
|
||
| String potentialCipher = "Lorem Ipsum"; | ||
| assertFalse(LegacyAesUtil.isEncrypted(potentialCipher)); | ||
| assertFalse(LegacyAesUtil.isEncrypted(Base64.encode(potentialCipher))); |
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
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.
This PR addresses two CodeQL security warnings about outdated encryption algorithms:
Related Issues
How it works
The class
AESUtilis needed to symmetrically encrypt Ldap manager passwords. These passwords are used to write credentials of users to an Ldap server (if configured). The secret key for this encryption can be configured inkitodo_config.propertiesviasecurity.secret.ldapManagerPassword.Currently,
AESUtiluses the encryption methodAES/CBC, which is generally not bad, but a bit outdated. This PR migrates passwords toAES/GCM, which is considered the current standard.This PR follows a similar strategy as #7188 and the Spring Security "DelegatingPasswordEncoder". It prefixes encrypted passwords with a unique identifier, e.g.,
{aes-gcm}, which is used to identify the encryption strategy for this password. Passwords that have previously stored without such a prefix are treated as "legacy" and are decrypted with the legacy decryption strategy. In the future, passwords can be easily migrated to new and better algorithms by adding another strategy implementation.In order to migrate passwords to the newest encryption algorithm, the button
LDAP Manager Passwörter migrierenon page "System -> Migrieren" can be used.Since Kitodo.Production needs to be capable of decrypting legacy passwords for some time, the CodeQL security warning message will remain active. The legacy class
AESUtil(nowLegacyAesUtil) has been marked as@Deprecated.UI Changes
The password input for ldap manager passwords was previously set up with
redisplay="true", meaning, the actual clear-text password was already embedded in the HTML, but there was no "eye" button for users to display the clear-text password. This PR adds the "eye" button and fixes a simple layout issue (the password input did not stretch to 100% width).I also added a
maxlengthrestriction of 128 characters to the password input such that you cannot enter passwords that will be encrypted to something that does not fit themanagerPassworddatabase column that only supports 255 characters (defined asVARCHAR(255)). A password with 128 characters is encoded with roughly ~250 characters. In case the encoding still doesn't fit, a general "Ldap Server cannot be saved" error is shown to the user (as implemented in the current main branch).Migration / Release Notes
LDAP Manager Passwörter migrierenon pageSystem -> Migrieren