SAK-52107 Microsot integration secret is not encrypted in database #14233
Conversation
WalkthroughAdds AES‑GCM encryption for storing Microsoft secrets, decrypts secrets when reading credentials, validates the decrypted secret before creating AdminAuthProvider/GraphServiceClient, encrypts secrets on save (including batch saves), and reads the encryption key from sakai.properties. Changes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java(2 hunks)microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (e.g., logger.info("Value is: {}", value)) instead of string concatenation in log statements
Write log messages and code comments in English; never translate log messages
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types.
When proposing or writing Java code, spell out full types in local variable declarations, for-loops, and try-with-resources.
If Java code uses var, replace it with an explicit type.
Review gate: Treat any PR or suggestion containing Java var as non-compliant; require explicit types before merge.
Files:
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.javamicrosoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
🪛 ast-grep (0.39.9)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[warning] 170-170: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGO)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 198-198: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGO)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 170-170: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGO)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 198-198: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGO)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🪛 Gitleaks (8.29.0)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[high] 51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Outdated
Show resolved
Hide resolved
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
jesusmmp
left a comment
jesusmmp
left a comment
There was a problem hiding this comment.
Tested on our test instance. It works fine.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (2)
180-202: Encryption routine is correct; minor perf/refactor nits onlyThe AES‑GCM implementation (IV generation, tag length, IV+ciphertext concatenation, Base64 encoding) is correct and matches the constants defined above. If this path becomes hot, you might consider reusing a single
SecureRandominstance instead of constructing a new one per call, but that’s a micro‑optimization and not required for this PR.
227-229: Let me verify if there are any other call sites or dynamic usage patterns I might have missed:Let me check for test files and any other potential call sites:
Ensure
saveConfigItemscannot write the secret in plaintextThe review comment suggests hardening the
saveConfigItemsmethod to route secret keys through encryption. However, verification of the codebase shows that this concern, while theoretically valid from a defensive programming perspective, does not represent a current bug:
- The only call site (ConfigController.java line 82) passes items exclusively from
getDefaultSynchronizationConfigItems(), which contains only synchronization-related boolean flags, never secretsMicrosoftCredentials.KEY_SECRETis stored through a separate, properly-encrypted code path viasaveOrUpdateCredentials()→saveOrUpdateSecretConfigItem()(which explicitly encrypts the value)- The two concerns (synchronization items and credentials) are architecturally separated
The proposed fix is a valid defensive hardening measure to prevent accidental misuse, as per security best practices for API design. However, since no current code path actually mixes secrets with synchronization items through
saveConfigItems, applying this fix is optional rather than mandatory.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
🪛 ast-grep (0.40.0)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[warning] 185-185: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 213-213: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 185-185: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 213-213: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (2)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (2)
45-52: AES‑GCM parameters and config property look goodUsing
AES/GCM/NoPaddingwith a 12‑byte IV and 128‑bit tag is a solid choice, and exposingLTI_ENCRYPTION_KEYas a constant for reuse fromsakai.propertieskeeps the configuration centralized. No issues here.
54-75: Credential decryption + legacy plaintext fallback are reasonableThe updated
getCredentials()correctly attempts to decrypt the stored secret and falls back to the original value when decryption returnsnull, which preserves legacy plaintext values on upgrade. ThegetKey()length check enforces a 32‑character key, matching AES‑256 requirements. Behavior here looks correct.
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties (1)
5734-5749: Align documentation with actual key semanticsThe property name and 32‑character requirement match the implementation, but two small cleanups would avoid confusion:
- Clarify that the 32 characters should be ASCII (so the UTF‑8 byte length is exactly 32); non‑ASCII characters can exceed 32 bytes and break encryption.
- The current code logs and falls back to the stored value when decryption fails or the key is missing, rather than making secrets “appear blank or removed” in the UI. Consider rephrasing that sentence to reflect the actual behavior (e.g., “existing secrets will no longer be usable until re‑entered with the correct key”).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
config/configuration/bundles/src/bundle/org/sakaiproject/config/bundle/default.sakai.properties(1 hunks)microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java(2 hunks)microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftCommonServiceImpl.java
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
🪛 ast-grep (0.40.0)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[warning] 201-201: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 229-229: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 201-201: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 229-229: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: sakai-deploy
- GitHub Check: maven-build
- GitHub Check: maven-build
🔇 Additional comments (2)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (2)
18-40: Crypto imports and types look appropriateUse of
StandardCharsets,SecureRandom,Base64, andjavax.cryptotypes is appropriate for AES‑GCM and keeps dependencies minimal. No issues here.
45-52: Good: centralizing the encryption key nameDefining
MC_ENCRYPTION_KEY = "microsoft.encryption.key"here and using it consistently in the implementation and properties file is a solid choice for keeping configuration aligned.
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (1)
75-83: Strengthen key validation to check byte length, not character length.The current check validates string length (
key.length() != 32) but doesn't prevent multi-byte UTF-8 characters that would produce a byte array of incorrect length, causing crypto failures. Since the error message specifies "32 ASCII characters," enforce that the UTF-8 encoding yields exactly 32 bytes.Apply this diff:
private static byte[] getKey() { String key = ServerConfigurationService.getString(MC_ENCRYPTION_KEY, null); - if (key == null || key.length() != 32) { + byte[] keyBytes = (key == null) ? null : key.getBytes(StandardCharsets.UTF_8); + if (keyBytes == null || keyBytes.length != 32) { throw new RuntimeException( - "Property '" + MC_ENCRYPTION_KEY + "' must be defined in sakai.properties and contain 32 ASCII characters for AES‑256" + "Property '" + MC_ENCRYPTION_KEY + "' must be defined in sakai.properties and be exactly 32 bytes when UTF-8 encoded (use ASCII characters)" ); } - return key.getBytes(StandardCharsets.UTF_8); + return keyBytes; }Based on past review comments, this issue was previously flagged but remains unaddressed.
🧹 Nitpick comments (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (1)
173-194: Refine secret detection to avoid unintended encryption and add null guards.The check
item.getKey().toUpperCase().contains("SECRET")is overly broad and may inadvertently encrypt configuration items whose keys happen to contain "SECRET" but aren't actual secrets. Additionally, it lacks null safety foritemoritem.getKey().Consider a more precise approach:
public void saveOrUpdateConfigItem(MicrosoftConfigItem item) { - if (item.getKey() != null && item.getKey().toUpperCase().contains("SECRET")) { + if (item != null && MicrosoftCredentials.KEY_SECRET.equals(item.getKey())) { String encrypted = encrypt(item.getValue()); if (encrypted == null) { log.error("The Microsoft secret could not be encrypted."); return; } item = MicrosoftConfigItem.builder() .key(item.getKey()) .value(encrypted) .build(); } + if (item == null || item.getKey() == null) { + log.warn("Attempted to save null config item or item with null key"); + return; + } + if (microsoftConfigRepository.exists(item.getKey())) { microsoftConfigRepository.merge(item); } else { microsoftConfigRepository.save(item); } }If future extensibility for multiple secret keys is desired, use an explicit set:
private static final Set<String> SECRET_KEYS = Set.of(MicrosoftCredentials.KEY_SECRET /* , add others here */); if (item != null && item.getKey() != null && SECRET_KEYS.contains(item.getKey())) { // encryption logic }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
🪛 ast-grep (0.40.0)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[warning] 201-201: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 229-229: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 201-201: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 229-229: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
...on/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (2)
75-83: Consider validating byte length for non-ASCII safety.The current check validates string length (32 characters), and the error message specifies "32 ASCII characters." If an administrator accidentally uses non-ASCII characters, UTF-8 encoding could yield a different byte count, causing cryptographic failures later.
Consider this refinement:
private static byte[] getKey() { String key = ServerConfigurationService.getString(MC_ENCRYPTION_KEY, null); - if (key == null || key.length() != 32) { + byte[] keyBytes = (key != null) ? key.getBytes(StandardCharsets.UTF_8) : null; + if (keyBytes == null || keyBytes.length != 32) { throw new RuntimeException( - "Property '" + MC_ENCRYPTION_KEY + "' must be defined in sakai.properties and contain 32 ASCII characters for AES‑256" + "Property '" + MC_ENCRYPTION_KEY + "' must be defined in sakai.properties and be exactly 32 bytes when UTF-8 encoded for AES-256" ); } - return key.getBytes(StandardCharsets.UTF_8); + return keyBytes; }
91-91: Fix inconsistent indentation.Line 91 has inconsistent leading whitespace compared to the surrounding lines (previously flagged by reviewer jesusmmp).
Apply consistent indentation:
saveOrUpdateConfigItem(MicrosoftConfigItem.builder().key(MicrosoftCredentials.KEY_SECRET).value(credentials.getSecret()).build()); - saveOrUpdateConfigItem(MicrosoftConfigItem.builder().key(MicrosoftCredentials.KEY_SCOPE).value(credentials.getScope()).build()); + saveOrUpdateConfigItem(MicrosoftConfigItem.builder().key(MicrosoftCredentials.KEY_SCOPE).value(credentials.getScope()).build());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.java
📄 CodeRabbit inference engine (.cursor/rules/logging-rule.mdc)
**/*.java: Use SLF4J parameterized logging (logger.info("Value is: {}", value)) instead of string concatenation (logger.info("Value is: " + value))
Log messages and code comments should be in English. Log messages should never be translated.
**/*.java: Java: Never use local variable type inference (var). Always declare explicit types. Yes:Map<String, Integer> counts = new HashMap<>();No:var counts = new HashMap<String, Integer>();
When proposing Java code, spell out full types in local variable declarations,forloops, and try-with-resources
When editing Java, prefer clarity over brevity; avoid introducing language features that aren't widely used in the repo
Treat any PR or suggestion containing Javavaras non-compliant. Recommend replacing with explicit types before merge
**/*.java: Use Java 17 for trunk development (Java 11 was used for Sakai 22 and Sakai 23)
Do not use local variable type inference (var) in Java code. Always declare explicit types (e.g.,List<String> names = new ArrayList<>();notvar names = new ArrayList<String>();). Enforced by Checkstyle rule duringmvn validate
Files:
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
🪛 ast-grep (0.40.0)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java
[warning] 204-204: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 232-232: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 204-204: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 232-232: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Cipher.getInstance(ALGORITHM)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: maven-build
- GitHub Check: maven-build
- GitHub Check: sakai-deploy
🔇 Additional comments (6)
microsoft-integration/impl/src/main/java/org/sakaiproject/microsoft/impl/MicrosoftConfigurationServiceImpl.java (6)
48-51: LGTM! Encryption parameters are correctly configured.The use of AES-GCM with a 12-byte IV and 128-bit authentication tag follows NIST recommendations for authenticated encryption.
Note: The static analysis warnings about ECB mode and 3DES are false positives—the code explicitly uses
"AES/GCM/NoPadding"at line 48.
54-73: LGTM! Backward-compatible decryption with legacy plaintext fallback.The method correctly attempts decryption and falls back to the raw secret value if decryption fails, preventing breakage on upgrade for existing installations with plaintext secrets. The warning log at line 68 appropriately alerts admins to the condition.
173-194: LGTM! Encryption is correctly applied to all secrets before persistence.The implementation encrypts any config item whose key contains "SECRET" and fails safely by returning early (line 180) if encryption fails, preventing plaintext secrets from being persisted.
The pattern-matching approach (line 174) differs from the past review's suggestion to use a separate
saveOrUpdateSecretConfigItemmethod, but it achieves the same security goal: all code paths that save secrets (includingsaveCredentialsandsaveConfigItems) now automatically encrypt them.
196-221: LGTM! Encryption implementation is cryptographically sound.The method correctly:
- Generates a fresh random IV for each encryption (line 202-203)
- Uses AES-GCM authenticated encryption
- Prepends the IV to the ciphertext for later decryption
- Base64-encodes the result for database storage
- Handles null input gracefully (lines 197-199)
223-244: LGTM! Decryption correctly reverses the encryption process.The method properly:
- Validates input (line 224)
- Base64-decodes the stored value
- Separates the IV from the encrypted payload
- Decrypts using the same AES-GCM parameters
- Returns
nullon any error (enabling the fallback logic ingetCredentials())The
public staticvisibility is appropriate for external usage (e.g.,MicrosoftCommonServiceImplas shown in the provided snippets).
246-248: LGTM! Batch saves correctly route through encryption logic.By delegating to
saveOrUpdateConfigItem, this method ensures that any items with keys containing "SECRET" are encrypted before persistence (per the check at line 174), addressing the past review concern about batch operations bypassing the encryption layer.
2 participants
https://sakaiproject.atlassian.net/browse/SAK-52107
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.