627 encryptionbuilder aus wls common entfernen und fix config key für crypto.key #635
Conversation
|
@coderabbitai full review |
WalkthroughThe pull request involves a restructuring of the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (9)
wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java (5)
29-29: Consider using a more realistic encryption prefixThe current prefix "encryptionPrefix" is generic. Consider using a more realistic prefix that matches your production environment (e.g., "{AES}") to ensure tests reflect real-world scenarios.
31-38: Add constructor validation testsThe test suite is missing validation for the CryptoService constructor. Consider adding tests to verify:
- Proper initialization with valid Cipher instances
- Handling of null Cipher instances
- Verification of Cipher transformation/mode
45-58: Add edge cases to IsEncrypted testsThe current test coverage for isEncrypted() should be expanded to include:
- Null input handling
- Empty string input
- Case sensitivity verification (if applicable)
Example test cases to add:
@Test void should_handleNullValue() { Assertions.assertThat(unitUnderTest.isEncrypted(null)).isFalse(); } @Test void should_handleEmptyString() { Assertions.assertThat(unitUnderTest.isEncrypted("")).isFalse(); }
63-74: Add cipher initialization verificationThe encryption tests should verify that the cipher is properly initialized for encryption mode. Consider adding verification:
@Test void should_initializeCipherInEncryptMode() throws Exception { val valueToEncrypt = "test"; unitUnderTest.encrypt(valueToEncrypt); Mockito.verify(cipher).init(Cipher.ENCRYPT_MODE, any(Key.class)); }
115-165: Add tests for malformed inputThe decrypt tests should include validation for:
- Malformed Base64 input
- Invalid padding
- Incomplete encrypted data
Example test:
@Test void should_handleMalformedBase64Input() { val malformedInput = ENCRYPTION_PREFIX + "not-base64!@#"; Assertions.assertThatThrownBy(() -> unitUnderTest.decrypt(malformedInput)) .isInstanceOf(TechnischeWlsException.class) .hasMessageContaining("malformed input"); }wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/security/EncryptionBuilder.java (2)
Line range hint
41-43: Security: Remove sensitive data from error logs.The error logging statements include the actual values being encrypted/decrypted. This could expose sensitive information in log files.
Apply this diff to remove sensitive data from logs:
- log.error("Unable to decrypt the given value <" + value + "> as of an " + e.getClass().getSimpleName() + ". Using direct object reference!", e); + log.error("Unable to decrypt value due to {}. Using direct object reference!", e.getClass().getSimpleName(), e);- log.error("Unable to encrypt the given value <" + value + "> as of an " + e.getClass().getSimpleName() + ". Using direct object reference!", e); + log.error("Unable to encrypt value due to {}. Using direct object reference!", e.getClass().getSimpleName(), e);Also applies to: 49-50
Line range hint
33-54: Improve robustness of encryption/decryption methods.The methods should:
- Use explicit character encoding
- Validate Base64 input
- Consider adding input size limits
Apply this diff to improve the implementation:
public String decryptValue(String value) { if (value != null && !value.isEmpty()) { try { - val decode = Base64.getUrlDecoder().decode(value.getBytes()); + val decode = Base64.getUrlDecoder().decode(value.getBytes(StandardCharsets.UTF_8)); val finalized = decryptionCipher.doFinal(decode); - return new String(finalized); + return new String(finalized, StandardCharsets.UTF_8); } catch (IllegalBlockSizeException | BadPaddingException e) { log.error("Unable to decrypt value due to {}. Using direct object reference!", e.getClass().getSimpleName(), e); throw TechnischeWlsException.withCode(technischeExceptionKonstante).inService(formatter.getId()) .buildWithMessage("Problem bei Referenzierung/Dereferenzierung von Objekt-Referenzen"); + } catch (IllegalArgumentException e) { + log.error("Invalid Base64 input", e); + throw TechnischeWlsException.withCode(technischeExceptionKonstante).inService(formatter.getId()) + .buildWithMessage("Ungültiges Format der verschlüsselten Daten"); } } return value; } public String encryptValue(String value) { if (value != null && !value.isEmpty()) { try { - val finalized = encryptionCipher.doFinal(value.getBytes()); + // Add size validation to prevent excessive memory usage + if (value.length() > 1000) { + throw new IllegalArgumentException("Input too large"); + } + val finalized = encryptionCipher.doFinal(value.getBytes(StandardCharsets.UTF_8)); value = Base64.getUrlEncoder().encodeToString(finalized); } catch (IllegalBlockSizeException | BadPaddingException e) { log.error("Unable to encrypt value due to {}. Using direct object reference!", e.getClass().getSimpleName(), e); throw TechnischeWlsException.withCode(technischeExceptionKonstante).inService(formatter.getId()) .buildWithMessage("Problem bei Referenzierung/Dereferenzierung von Objekt-Referenzen"); + } catch (IllegalArgumentException e) { + log.error("Input validation failed", e); + throw TechnischeWlsException.withCode(technischeExceptionKonstante).inService(formatter.getId()) + .buildWithMessage("Eingabedaten zu groß"); } } return value; }Don't forget to add the import:
import java.nio.charset.StandardCharsets;wls-auth-service/src/main/resources/application-local.yml (1)
12-12: Add a newline at the end of the fileThe file does not end with a newline character, which is recommended for POSIX compliance and to prevent potential issues with some tools.
Apply this diff to add the newline:
- seed: change_me + seed: change_me🧰 Tools
🪛 yamllint (1.35.1)
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/AESEncryptionConfiguration.java (1)
Line range hint
18-24: Security: Update cipher initialization with secure modeThe current implementation uses ECB mode by default, which is not recommended for secure operations.
Update the cipher initialization:
- val secret = new SecretKeySpec(key.getBytes(), 0, 16, AES); - val encryptCipher = Cipher.getInstance(AES); + val secret = new SecretKeySpec(deriveKey(), AES); + val encryptCipher = Cipher.getInstance(CIPHER_TRANSFORMATION); + val iv = new byte[12]; // GCM requires 12 bytes IV + SecureRandom.getInstanceStrong().nextBytes(iv); + encryptCipher.init(Cipher.ENCRYPT_MODE, secret, new GCMParameterSpec(128, iv));Similar changes should be applied to the decryption cipher.
Also applies to: 26-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/AESEncryptionConfiguration.java(2 hunks)wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/EncryptionConfiguration.java(0 hunks)wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/exception/ExceptionConstants.java(1 hunks)wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/security/EncryptionBuilder.java(1 hunks)wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java(1 hunks)wls-auth-service/src/main/resources/application-local.yml(1 hunks)wls-auth-service/src/main/resources/application-test.yml(1 hunks)wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/domain/UserRepositoryImplIntegrationTest.java(1 hunks)wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/rest/WahllokalBenutzerControllerIntegrationTest.java(1 hunks)wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceIntegrationTest.java(1 hunks)wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java(1 hunks)wls-common/security/src/test/java/de/muenchen/oss/wahllokalsystem/wls/common/security/EncryptionBuilderIntegrationTest.java(0 hunks)wls-common/security/src/test/java/de/muenchen/oss/wahllokalsystem/wls/common/security/EncryptionBuilderTest.java(0 hunks)
💤 Files with no reviewable changes (3)
- wls-common/security/src/test/java/de/muenchen/oss/wahllokalsystem/wls/common/security/EncryptionBuilderIntegrationTest.java
- wls-common/security/src/test/java/de/muenchen/oss/wahllokalsystem/wls/common/security/EncryptionBuilderTest.java
- wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/configuration/EncryptionConfiguration.java
🧰 Additional context used
🪛 yamllint (1.35.1)
wls-auth-service/src/main/resources/application-local.yml
[error] 12-12: no new line character at the end of file
(new-line-at-end-of-file)
🪛 Gitleaks (8.21.2)
wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceIntegrationTest.java
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (9)
wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/rest/WahllokalBenutzerControllerIntegrationTest.java (3)
Line range hint 92-93: LGTM: Encryption handling maintained correctly
The test correctly uses CryptoService for username decryption and verification, maintaining the encryption functionality while adapting to the architectural changes (removal of EncryptionBuilder).
Also applies to: 94-95
Line range hint 63-175: LGTM: Comprehensive test coverage maintained
The test suite maintains thorough coverage of all operations while properly handling encryption:
- Create/Export: Verifies both persistence and proper encryption/decryption
- Delete: Ensures proper cleanup with wahltagID scoping
- Export: Validates correct retrieval and filtering
36-36: Verify crypto key configuration for tests
The removal of service.config.crypto.key property from the test configuration suggests that this value is now being provided through a different mechanism (likely application-test.yml). Let's verify this to ensure test stability.
wls-auth-service/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java (1)
1-166: Overall test implementation looks solid
The test suite provides good coverage of the CryptoService functionality and aligns well with the PR objectives of moving encryption handling to the Crypto-Service. The structure is clean, and the tests are well-organized.
wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/security/EncryptionBuilder.java (3)
4-4: Package relocation aligns with PR objectives.
The move from wls.common.security to authservice.security successfully fulfills the goal of removing the EncryptionBuilder from WLS-Common.
Line range hint 26-31: Well-structured dependency injection.
Good practices observed:
- Constructor injection
- Proper use of @qualifier to distinguish between Cipher instances
- Final fields ensuring thread-safety
Line range hint 16-17: Verify complete migration to CryptoService.
Since this class is being moved from WLS-Common to Auth-Service as part of integrating with CryptoService, let's verify the integration is complete.
✅ Verification successful
Let me gather more information about the implementation details to verify the complete migration.
Let me try a different approach to get the implementation details.
Migration to CryptoService is complete and properly implemented
The CryptoService has successfully incorporated all functionality from EncryptionBuilder with the following improvements:
- Maintains the same core encryption/decryption logic using the same dependencies
- Adds prefix handling functionality with
encryptedPrefixandisEncrypted()method - Uses consistent error handling with dedicated exception constants
- Implements better logging with @slf4j annotation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that CryptoService implements all functionality from EncryptionBuilder
# Check for any remaining references to EncryptionBuilder in the codebase
echo "Checking for remaining EncryptionBuilder references..."
rg "EncryptionBuilder" --type java
# Check CryptoService implementation
echo "Checking CryptoService implementation..."
rg "class CryptoService" --type java -A 10
Length of output: 5290
Script:
#!/bin/bash
# Get full implementation of both classes to compare functionality
echo "EncryptionBuilder implementation:"
ast-grep --pattern 'class EncryptionBuilder {
$$$
}'
echo -e "\nCryptoService implementation:"
ast-grep --pattern 'class CryptoService {
$$$
}'
# Check for any usages of EncryptionBuilder in the codebase
echo -e "\nChecking EncryptionBuilder usages:"
rg -B 2 -A 2 "new EncryptionBuilder" --type java
Length of output: 397
Script:
#!/bin/bash
# Get the full content of both files
echo "EncryptionBuilder implementation:"
cat wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/security/EncryptionBuilder.java
echo -e "\nCryptoService implementation:"
cat wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java
# Search for any autowired/injected usage of EncryptionBuilder
echo -e "\nChecking for EncryptionBuilder dependencies:"
rg "@Autowired.*EncryptionBuilder|@Inject.*EncryptionBuilder" --type java
Length of output: 6919
wls-auth-service/src/main/resources/application-test.yml (1)
20-21: LGTM
The addition of the crypto.key configuration in the test environment is appropriate and aligns with the application's requirements.
wls-auth-service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/exception/ExceptionConstants.java (1)
10-10: LGTM
The new exception code CRYPTO_EXCEPTION_CODE is appropriately defined and enhances error handling consistency.
...ice/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java
Show resolved
Hide resolved
...ice/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java
Show resolved
Hide resolved
...va/de/muenchen/oss/wahllokalsystem/authservice/configuration/AESEncryptionConfiguration.java
Show resolved
Hide resolved
...t/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceIntegrationTest.java
Show resolved
Hide resolved
...t/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceIntegrationTest.java
Show resolved
Hide resolved
...va/de/muenchen/oss/wahllokalsystem/authservice/domain/UserRepositoryImplIntegrationTest.java
Show resolved
Hide resolved
...service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java
Outdated
Show resolved
Hide resolved
...service/src/main/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoService.java
Outdated
Show resolved
Hide resolved
...ice/src/test/java/de/muenchen/oss/wahllokalsystem/authservice/service/CryptoServiceTest.java
Show resolved
Hide resolved
Co-authored-by: Viviane Johns <[email protected]>
MrSebastian
deleted the
627-encryptionbuilder-aus-wls-common-entfernen
branch
Beschreibung:
Definition of Done (DoD):
Backend
Referenzen1:
Verwandt mit Issue #
Closes #627
Summary by CodeRabbit
New Features
CryptoServiceto validate encryption and decryption processes.CryptoServicefocusing on encryption and decryption functionalities.Bug Fixes
Bug Fixes
EncryptionBuilder.Chores
Footnotes
Nicht zutreffende Referenzen vor dem Speichern entfernen ↩