feat: Added TLS support for Redis Datasource #37106
Conversation
WalkthroughThe changes in this pull request introduce a new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Outside diff range and nitpick comments (8)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java (2)
12-17: Add class-level documentation.Please add Javadoc to describe the purpose of this class, its role in TLS configuration, and usage guidelines.
+/** + * Configuration class for TLS/SSL connections. + * Contains settings for TLS enablement, certificate verification, + * and client authentication including necessary certificate files. + */ @Getter
25-35: Consider additional security measures for certificate handling.For security-sensitive fields:
- Implement validation to ensure certificate files meet security requirements
- Consider adding mechanisms to securely clean up certificate data when no longer needed
- Add field-level documentation specifying expected file formats and requirements
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
37-39: Consider adding validation annotations.Since this is security-related configuration, consider adding validation annotations (e.g.,
@Valid) to ensure proper validation of the TLS configuration object.@JsonView({Views.Public.class, FromRequest.class}) + @Valid TlsConfiguration tlsConfiguration;app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java (1)
39-40: Error messages are clear and consistentThe error messages are well-structured and provide clear guidance to users. They align well with the PR's objective of implementing TLS support for Redis.
Consider adding period consistency. Some existing messages end with periods while others don't. For consistency, suggest removing the periods from the new messages:
- public static final String CA_CERTIFICATE_MISSING_ERROR_MSG = - "CA certificate is missing. Please upload the CA certificate."; + public static final String CA_CERTIFICATE_MISSING_ERROR_MSG = + "CA certificate is missing. Please upload the CA certificate"; - public static final String TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_CERTIFICATE_MISSING_ERROR_MSG = - "Client authentication is enabled but the client certificate is missing. Please upload the client certificate."; + public static final String TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_CERTIFICATE_MISSING_ERROR_MSG = + "Client authentication is enabled but the client certificate is missing. Please upload the client certificate"; - public static final String TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_KEY_MISSING_ERROR_MSG = - "Client authentication is enabled but the client key is missing. Please upload the client key."; + public static final String TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_KEY_MISSING_ERROR_MSG = + "Client authentication is enabled but the client key is missing. Please upload the client key";Also applies to: 42-43, 45-46
app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (1)
86-96: Consider adding validation for CA certificate format.While the encryption is properly configured, adding a validation message for accepted certificate formats (e.g., .pem, .crt) would improve user experience.
{ "label": "Upload CA Certificate", "configProperty": "datasourceConfiguration.tlsConfiguration.caCertificateFile", "controlType": "FILE_PICKER", "encrypted": true, + "validationMessage": "Please upload a valid certificate file (.pem, .crt)", + "allowedFileTypes": [".pem", ".crt"], "hidden": { "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", "comparison": "NOT_EQUALS", "value": true } }app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (1)
44-85: Ensure resources are properly managed to prevent leaksObjects like
CertificateFactoryandKeyStoreshould be properly managed. Although they don't require explicit closure, consider using try-with-resources or ensuring that any streams or resources they use are properly closed.app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (2)
268-268: Use logging placeholders instead of string concatenationReplace string concatenation in log statements with placeholders for better performance and readability.
- log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); + log.debug("{}: Created Jedis pool.", Thread.currentThread().getName());
271-272: Add logging for TLS-enabled Jedis pool creationConsider adding a debug log statement when creating a Jedis pool with TLS for consistency and easier debugging.
} else { + log.debug("{}: Created Jedis pool with TLS.", Thread.currentThread().getName()); return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (8)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1 hunks)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (4 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (8 hunks)
🧰 Additional context used
🪛 Gitleaks
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
117-118: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (6)
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java (1)
1-11: LGTM! Package structure and imports are well-organized.app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java (1)
37-39: LGTM! Clean integration of TLS configuration.The addition of
tlsConfigurationfield follows the established patterns in the class and properly integrates with the serialization views.app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java (1)
33-37: LGTM! Well-organized section headerThe new section for TLS configuration error messages follows the existing pattern and maintains good code organization.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (2)
16-17: LGTM! Correct SSL scheme constant.The
REDIS_SSL_SCHEMEconstant correctly uses the standard "rediss://" scheme for SSL/TLS connections.
20-25: Verify TLS validation implementation.Ensure that TLS configuration validation is implemented in the appropriate validation methods.
✅ Verification successful
TLS validation is properly implemented
The TLS validation is comprehensively implemented in
RedisPlugin.javawith proper checks for:
- TLS certificate verification
- CA certificate presence when TLS verification is enabled
- Client certificate and key validation when client authentication is required
- Appropriate error messages in
RedisErrorMessages.javaThe implementation in
RedisURIUtils.javacorrectly uses this validated configuration.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for TLS validation implementation rg -l "validateTlsConfiguration|validateTLS" --type java # Search for TLS-related validation messages rg "TLS|SSL" app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/constants/RedisErrorMessages.javaLength of output: 315
Script:
#!/bin/bash # Search for TLS/SSL related validation or configuration in Redis plugin rg -A 5 "TLS|SSL|tls|ssl" app/server/appsmith-plugins/redisPlugin/src/main/java/ # Search for validation methods in Redis plugin ast-grep --pattern 'class $_ { $$$ validate($$$) { $$$ } $$$ }'Length of output: 26549
app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (1)
61-85: LGTM! Basic TLS configuration is well structured.The implementation follows security best practices with secure-by-default values and proper conditional visibility rules.
app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java
Show resolved
Hide resolved
| if (datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | ||
| builder.append(REDIS_SSL_SCHEME); | ||
| } else { | ||
| builder.append(REDIS_SCHEME); | ||
| } |
There was a problem hiding this comment.
Add null check for TLS configuration.
The direct access to getTlsConfiguration() could cause a NullPointerException if the TLS configuration is not set.
Consider adding a null check:
- if (datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) {
+ if (datasourceConfiguration.getTlsConfiguration() != null &&
+ datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) {
builder.append(REDIS_SSL_SCHEME);
} else {
builder.append(REDIS_SCHEME);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | |
| builder.append(REDIS_SSL_SCHEME); | |
| } else { | |
| builder.append(REDIS_SCHEME); | |
| } | |
| if (datasourceConfiguration.getTlsConfiguration() != null && | |
| datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | |
| builder.append(REDIS_SSL_SCHEME); | |
| } else { | |
| builder.append(REDIS_SCHEME); | |
| } |
| { | ||
| "label": "Requires TLS Client Authentication", | ||
| "configProperty": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | ||
| "controlType": "CHECKBOX", | ||
| "initialValue": false, | ||
| "hidden": { | ||
| "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", | ||
| "comparison": "NOT_EQUALS", | ||
| "value": true | ||
| } | ||
| }, | ||
| { | ||
| "label": "Upload Client Certificate", | ||
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientCertificateFile", | ||
| "controlType": "FILE_PICKER", | ||
| "encrypted": true, | ||
| "hidden": { | ||
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | ||
| "comparison": "EQUALS", | ||
| "value": false | ||
| } | ||
| }, | ||
| { | ||
| "label": "Upload Client Key", | ||
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientKeyFile", | ||
| "controlType": "FILE_PICKER", | ||
| "encrypted": true, | ||
| "hidden": { | ||
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | ||
| "comparison": "EQUALS", | ||
| "value": false | ||
| } | ||
| } |
There was a problem hiding this comment.
Enhance visibility conditions for client authentication fields.
The current visibility rules for client certificate and key should also check if TLS is enabled to prevent edge cases.
Apply this change to both client certificate and key configurations:
"hidden": {
- "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth",
- "comparison": "EQUALS",
- "value": false
+ "conditions": [
+ {
+ "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
+ "comparison": "EQUALS",
+ "value": true
+ },
+ {
+ "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth",
+ "comparison": "EQUALS",
+ "value": true
+ }
+ ],
+ "operator": "AND"
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| { | |
| "label": "Requires TLS Client Authentication", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "controlType": "CHECKBOX", | |
| "initialValue": false, | |
| "hidden": { | |
| "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", | |
| "comparison": "NOT_EQUALS", | |
| "value": true | |
| } | |
| }, | |
| { | |
| "label": "Upload Client Certificate", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientCertificateFile", | |
| "controlType": "FILE_PICKER", | |
| "encrypted": true, | |
| "hidden": { | |
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "comparison": "EQUALS", | |
| "value": false | |
| } | |
| }, | |
| { | |
| "label": "Upload Client Key", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientKeyFile", | |
| "controlType": "FILE_PICKER", | |
| "encrypted": true, | |
| "hidden": { | |
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "comparison": "EQUALS", | |
| "value": false | |
| } | |
| } | |
| { | |
| "label": "Requires TLS Client Authentication", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "controlType": "CHECKBOX", | |
| "initialValue": false, | |
| "hidden": { | |
| "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", | |
| "comparison": "NOT_EQUALS", | |
| "value": true | |
| } | |
| }, | |
| { | |
| "label": "Upload Client Certificate", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientCertificateFile", | |
| "controlType": "FILE_PICKER", | |
| "encrypted": true, | |
| "hidden": { | |
| "conditions": [ | |
| { | |
| "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", | |
| "comparison": "EQUALS", | |
| "value": true | |
| }, | |
| { | |
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "comparison": "EQUALS", | |
| "value": true | |
| } | |
| ], | |
| "operator": "AND" | |
| } | |
| }, | |
| { | |
| "label": "Upload Client Key", | |
| "configProperty": "datasourceConfiguration.tlsConfiguration.clientKeyFile", | |
| "controlType": "FILE_PICKER", | |
| "encrypted": true, | |
| "hidden": { | |
| "conditions": [ | |
| { | |
| "path": "datasourceConfiguration.tlsConfiguration.tlsEnabled", | |
| "comparison": "EQUALS", | |
| "value": true | |
| }, | |
| { | |
| "path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth", | |
| "comparison": "EQUALS", | |
| "value": true | |
| } | |
| ], | |
| "operator": "AND" | |
| } | |
| } |
There was a problem hiding this comment.
It is not working.
There was a problem hiding this comment.
@AnnaHariprasad5123, sorry to hear that it's not working.
Please update the hidden property for both the Upload Client Certificate and Upload Client Key fields as follows:
"hidden": {
"conditions": [
{
"path": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
"comparison": "NOT_EQUALS",
"value": true
},
{
"path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth",
"comparison": "NOT_EQUALS",
"value": true
}
],
"operator": "OR"
}This will ensure these fields are visible only when TLS is enabled and client authentication is required.
Let me know if this resolves the issue.
| trustManagers = new TrustManager[] { | ||
| new X509TrustManager() { | ||
| @Override | ||
| public X509Certificate[] getAcceptedIssuers() { | ||
| return new X509Certificate[0]; | ||
| } | ||
|
|
||
| @Override | ||
| public void checkClientTrusted(X509Certificate[] certs, String authType) {} | ||
|
|
||
| @Override | ||
| public void checkServerTrusted(X509Certificate[] certs, String authType) {} | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Be cautious with accepting all certificates when certificate verification is disabled
When verifyTlsCertificate is false, the code accepts all server certificates without validation. This can expose the application to man-in-the-middle attacks. Consider informing users about the security implications and ensure this behavior is explicitly documented.
There was a problem hiding this comment.
It is user preference to verify the CA certificate or not.
There was a problem hiding this comment.
@AnnaHariprasad5123, understood. Thank you for clarifying.
✏️ Learnings added
Learnt from: AnnaHariprasad5123
PR: appsmithorg/appsmith#37106
File: app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java:92-106
Timestamp: 2024-11-06T06:27:39.819Z
Learning: In `RedisTLSManager.java`, it's acceptable to accept all server certificates without validation when `verifyTlsCertificate` is false, as it is the user's preference to verify the CA certificate or not.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration(); | ||
| boolean verifyTlsCertificate = tlsConfiguration.getVerifyTlsCertificate(); | ||
| boolean requiresClientAuth = tlsConfiguration.getRequiresClientAuth(); |
There was a problem hiding this comment.
Add null checks for TLS configuration to prevent NullPointerException
Currently, the code assumes that tlsConfiguration and its methods will not be null. To prevent potential NullPointerException, please add null checks for tlsConfiguration, getVerifyTlsCertificate(), and getRequiresClientAuth().
Apply this diff to add null checks:
TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration();
+if (tlsConfiguration == null) {
+ throw new IllegalArgumentException("TLS configuration is missing");
+}
Boolean verifyTlsCertificate = tlsConfiguration.getVerifyTlsCertificate();
Boolean requiresClientAuth = tlsConfiguration.getRequiresClientAuth();
+if (verifyTlsCertificate == null || requiresClientAuth == null) {
+ throw new IllegalArgumentException("TLS configuration flags cannot be null");
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration(); | |
| boolean verifyTlsCertificate = tlsConfiguration.getVerifyTlsCertificate(); | |
| boolean requiresClientAuth = tlsConfiguration.getRequiresClientAuth(); | |
| TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration(); | |
| if (tlsConfiguration == null) { | |
| throw new IllegalArgumentException("TLS configuration is missing"); | |
| } | |
| Boolean verifyTlsCertificate = tlsConfiguration.getVerifyTlsCertificate(); | |
| Boolean requiresClientAuth = tlsConfiguration.getRequiresClientAuth(); | |
| if (verifyTlsCertificate == null || requiresClientAuth == null) { | |
| throw new IllegalArgumentException("TLS configuration flags cannot be null"); | |
| } |
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | ||
| tlsConfiguration.setTlsEnabled(false); | ||
| invalidDatasourceConfiguration.setTlsConfiguration(tlsConfiguration); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor TLS configuration initialization to avoid duplication
Repeated TLS configuration detected. Using a helper method can reduce redundancy.
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | ||
| tlsConfiguration.setTlsEnabled(false); | ||
| invalidDatasourceConfiguration.setTlsConfiguration(tlsConfiguration); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor TLS configuration initialization to avoid duplication
Consider refactoring the TLS configuration setup into a shared method to improve code clarity.
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | ||
| tlsConfiguration.setTlsEnabled(false); | ||
| invalidDatasourceConfiguration.setTlsConfiguration(tlsConfiguration); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor TLS configuration initialization to avoid duplication
The TLS configuration is set up repeatedly. A helper method can enhance maintainability.
| TlsConfiguration tlsConfiguration = new TlsConfiguration(); | ||
| tlsConfiguration.setTlsEnabled(false); | ||
| datasourceConfiguration.setTlsConfiguration(tlsConfiguration); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor TLS configuration initialization to avoid duplication
Creating a helper method for TLS configuration can eliminate repetitive code.
| tlsConfiguration.setVerifyTlsCertificate(true); | ||
| tlsConfiguration.setRequiresClientAuth(false); | ||
| datasourceConfiguration.setTlsConfiguration(tlsConfiguration); | ||
| datasourceConfiguration.getTlsConfiguration().setVerifyTlsCertificate(true); |
There was a problem hiding this comment.
Remove redundant setVerifyTlsCertificate(true) call
The verifyTlsCertificate property is already set at line 188. The call at line 191 is redundant and can be removed.
Apply this diff to remove the redundant line:
- datasourceConfiguration.getTlsConfiguration().setVerifyTlsCertificate(true);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (3)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (1)
115-116: Enhance debug logging with connection detailsThe current debug log could be more helpful by including connection details (excluding sensitive information).
-log.debug(Thread.currentThread().getName() + ": Created Jedis pool with TLS."); +log.debug("{}: Created Jedis pool with TLS for URI: {}, clientAuth: {}, certVerification: {}", + Thread.currentThread().getName(), uri.getHost(), requiresClientAuth, verifyTlsCertificate);app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2)
60-64: Consider renaming the helper methodThe method creates a default disabled TLS configuration. Consider renaming to
createDefaultDisabledTlsConfigurationto better reflect its purpose.
174-229: LGTM with a suggestion for improvementThe test methods thoroughly cover TLS validation scenarios. However, consider extracting the common TLS configuration setup into helper methods to reduce duplication.
Example helper method:
private TlsConfiguration createTlsConfiguration(boolean verifyTls, boolean requiresClientAuth) { TlsConfiguration tlsConfig = new TlsConfiguration(); tlsConfig.setTlsEnabled(true); tlsConfig.setVerifyTlsCertificate(verifyTls); tlsConfig.setRequiresClientAuth(requiresClientAuth); return tlsConfig; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (4)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (4 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (1 hunks)
- app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (9 hunks)
🧰 Additional context used
🪛 Gitleaks
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
122-123: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (5)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (1)
16-17: LGTM! Well-defined constant for Redis SSL scheme.The constant follows Java naming conventions and uses the correct scheme for Redis SSL connections.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (2)
13-13: LGTM: Required imports added for TLS supportThe necessary imports for TLS functionality have been correctly added.
Also applies to: 49-49
313-338: LGTM: Comprehensive TLS validation implementationThe validation logic thoroughly checks all required TLS components with proper error handling:
- CA certificate validation when TLS verification is enabled
- Client certificate and key validation when client authentication is required
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2)
11-11: LGTM!The TlsConfiguration import is correctly added.
74-74: LGTM!The TLS configuration is consistently integrated across all existing test methods.
Also applies to: 91-91, 104-104, 118-118, 135-135, 156-156
|
|
||
| if (datasourceConfiguration.getTlsConfiguration() != null | ||
| && datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) { | ||
| builder.append(REDIS_SSL_SCHEME); | ||
| } else { | ||
| builder.append(REDIS_SCHEME); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding TLS port validation.
The implementation correctly handles TLS configuration with proper null checks. However, when TLS is enabled, it might be worth validating that the port is appropriate for TLS connections (typically 6379 for non-TLS and 6380 for TLS).
if (datasourceConfiguration.getTlsConfiguration() != null
&& datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) {
builder.append(REDIS_SSL_SCHEME);
+ // Validate TLS port
+ Endpoint endpoint = datasourceConfiguration.getEndpoints().get(0);
+ if (endpoint.getPort() != null && endpoint.getPort() == DEFAULT_PORT) {
+ log.warn("Using default non-TLS port {} with TLS enabled", DEFAULT_PORT);
+ }
} else {
builder.append(REDIS_SCHEME);
}Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Show resolved
Hide resolved
| sslContext.init( | ||
| requiresClientAuth ? keyManagerFactory.getKeyManagers() : null, trustManagers, new SecureRandom()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using a strong SecureRandom implementation
Instead of using the default SecureRandom constructor, specify a strong algorithm explicitly.
-sslContext.init(
- requiresClientAuth ? keyManagerFactory.getKeyManagers() : null, trustManagers, new SecureRandom());
+SecureRandom secureRandom = SecureRandom.getInstance("SHA1PRNG");
+sslContext.init(
+ requiresClientAuth ? keyManagerFactory.getKeyManagers() : null, trustManagers, secureRandom);Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| String clientCertContent = | ||
| new String(tlsConfiguration.getClientCertificateFile().getDecodedContent(), StandardCharsets.UTF_8); | ||
| X509Certificate clientCert = (X509Certificate) | ||
| certificateFactory.generateCertificate(new ByteArrayInputStream(clientCertContent.getBytes())); | ||
|
|
||
| // Load client private key | ||
| String clientKey = | ||
| new String(tlsConfiguration.getClientKeyFile().getDecodedContent(), StandardCharsets.UTF_8); | ||
| PrivateKey privateKey = loadPrivateKey(clientKey); |
There was a problem hiding this comment.
Enhance security by clearing sensitive data
The client certificate and private key are stored as strings in memory. Consider using char arrays or ByteBuffers that can be explicitly cleared after use.
-String clientCertContent =
- new String(tlsConfiguration.getClientCertificateFile().getDecodedContent(), StandardCharsets.UTF_8);
+byte[] clientCertBytes = tlsConfiguration.getClientCertificateFile().getDecodedContent();
X509Certificate clientCert = (X509Certificate)
- certificateFactory.generateCertificate(new ByteArrayInputStream(clientCertContent.getBytes()));
+ certificateFactory.generateCertificate(new ByteArrayInputStream(clientCertBytes));
+java.util.Arrays.fill(clientCertBytes, (byte) 0); // Clear sensitive dataCommittable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| private static PrivateKey loadPrivateKey(String clientKey) throws Exception { | ||
| clientKey = clientKey | ||
| .replace("-----BEGIN PRIVATE KEY-----", "") | ||
| .replace("-----END PRIVATE KEY-----", "") | ||
| .replaceAll("\\s", ""); | ||
|
|
||
| byte[] keyBytes = Base64.getDecoder().decode(clientKey); | ||
| PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes); | ||
| KeyFactory kf = KeyFactory.getInstance("RSA"); | ||
| return kf.generatePrivate(spec); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve private key handling and support multiple algorithms
The current implementation has several areas for improvement:
- Only supports RSA keys
- Uses string operations for key processing
- Doesn't clear sensitive data from memory
private static PrivateKey loadPrivateKey(String clientKey) throws Exception {
- clientKey = clientKey
- .replace("-----BEGIN PRIVATE KEY-----", "")
- .replace("-----END PRIVATE KEY-----", "")
- .replaceAll("\\s", "");
+ // Extract key type from header
+ String keyType = "RSA"; // Default to RSA
+ if (clientKey.contains("BEGIN EC PRIVATE KEY")) {
+ keyType = "EC";
+ }
+
+ // Process key data
+ byte[] keyBytes = null;
+ try {
+ String cleanKey = clientKey.replaceAll("-----(?:BEGIN|END)[^-]+-----", "")
+ .replaceAll("\\s", "");
+ keyBytes = Base64.getDecoder().decode(cleanKey);
+ PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes);
+ KeyFactory kf = KeyFactory.getInstance(keyType);
+ return kf.generatePrivate(spec);
+ } finally {
+ if (keyBytes != null) {
+ java.util.Arrays.fill(keyBytes, (byte) 0); // Clear sensitive data
+ }
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private static PrivateKey loadPrivateKey(String clientKey) throws Exception { | |
| clientKey = clientKey | |
| .replace("-----BEGIN PRIVATE KEY-----", "") | |
| .replace("-----END PRIVATE KEY-----", "") | |
| .replaceAll("\\s", ""); | |
| byte[] keyBytes = Base64.getDecoder().decode(clientKey); | |
| PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes); | |
| KeyFactory kf = KeyFactory.getInstance("RSA"); | |
| return kf.generatePrivate(spec); | |
| } | |
| private static PrivateKey loadPrivateKey(String clientKey) throws Exception { | |
| // Extract key type from header | |
| String keyType = "RSA"; // Default to RSA | |
| if (clientKey.contains("BEGIN EC PRIVATE KEY")) { | |
| keyType = "EC"; | |
| } | |
| // Process key data | |
| byte[] keyBytes = null; | |
| try { | |
| String cleanKey = clientKey.replaceAll("-----(?:BEGIN|END)[^-]+-----", "") | |
| .replaceAll("\\s", ""); | |
| keyBytes = Base64.getDecoder().decode(cleanKey); | |
| PKCS8EncodedKeySpec spec = new PKCS8EncodedKeySpec(keyBytes); | |
| KeyFactory kf = KeyFactory.getInstance(keyType); | |
| return kf.generatePrivate(spec); | |
| } finally { | |
| if (keyBytes != null) { | |
| java.util.Arrays.fill(keyBytes, (byte) 0); // Clear sensitive data | |
| } | |
| } | |
| } |
🧰 Tools
🪛 Gitleaks
122-123: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
There was a problem hiding this comment.
cc: @AnnaHariprasad5123 please check this suggestion
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| if (datasourceConfiguration.getTlsConfiguration() != null | ||
| && !datasourceConfiguration | ||
| .getTlsConfiguration() | ||
| .getTlsEnabled()) { | ||
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | ||
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | ||
| return jedisPool; | ||
| } else { | ||
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | ||
| } |
There was a problem hiding this comment.
Improve TLS configuration check logic
The current condition creates a non-TLS pool only when TLS is explicitly disabled. This should be reversed to create a TLS pool only when TLS is explicitly enabled.
Apply this change:
-if (datasourceConfiguration.getTlsConfiguration() != null
- && !datasourceConfiguration
- .getTlsConfiguration()
- .getTlsEnabled()) {
+if (datasourceConfiguration.getTlsConfiguration() == null
+ || !datasourceConfiguration
+ .getTlsConfiguration()
+ .getTlsEnabled()) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (datasourceConfiguration.getTlsConfiguration() != null | |
| && !datasourceConfiguration | |
| .getTlsConfiguration() | |
| .getTlsEnabled()) { | |
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | |
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | |
| return jedisPool; | |
| } else { | |
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | |
| } | |
| if (datasourceConfiguration.getTlsConfiguration() == null | |
| || !datasourceConfiguration | |
| .getTlsConfiguration() | |
| .getTlsEnabled()) { | |
| JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout); | |
| log.debug(Thread.currentThread().getName() + ": Created Jedis pool."); | |
| return jedisPool; | |
| } else { | |
| return createJedisPoolWithTLS(poolConfig, uri, timeout, datasourceConfiguration); | |
| } |
There was a problem hiding this comment.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
|
Need to prioritise review for this PR. |
|
Hi Appsmith Team, Could someone please review this PR when you have a chance? Your feedback would be greatly appreciated. Thank you! |
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11698192683. |
|
@AnnaHariprasad5123 All tests have passed on the shadow PRs. |
|
Deploy-Preview-URL: https://ce-37106.dp.appsmith.com |
|
Hi @NilanshBansal. I've implemented most of the changes suggested by Code Rabbit and marked them with a thumbs-up. For some changes, I’ve added comments, and I’ll add the rest shortly. Meanwhile, could you please check the functionality of Redis TLS |
|
@carinanfonseca can you please check this DP from the design perspective? |
|
Hi @AnnaHariprasad5123, thanks for your contribution. Can you check and ensure the spacing on the datasources page matches the attached screenshot? When I checked it after sign-up and on the regular datasources form, the spacing seemed off, with extra margins. Thank you! |
| String clientCertContent = | ||
| new String(tlsConfiguration.getClientCertificateFile().getDecodedContent(), StandardCharsets.UTF_8); | ||
| X509Certificate clientCert = (X509Certificate) | ||
| certificateFactory.generateCertificate(new ByteArrayInputStream(clientCertContent.getBytes())); | ||
|
|
||
| // Load client private key | ||
| String clientKey = | ||
| new String(tlsConfiguration.getClientKeyFile().getDecodedContent(), StandardCharsets.UTF_8); |
There was a problem hiding this comment.
@AnnaHariprasad5123 can you please add error handling here for exceptions that can come up from generateCertificate() etc. Adding a try-catch block should be good for handling exceptions for CertificateException and IOException type
| new String(tlsConfiguration.getCaCertificateFile().getDecodedContent(), StandardCharsets.UTF_8); | ||
|
|
||
| CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509"); | ||
| X509Certificate caCert = (X509Certificate) |
There was a problem hiding this comment.
Similarly please add a try-catch exception handling here
Fixes #26723
What's in this PR :
For more clarity : Video about Redis connection with TLS enabled
Screenshots :
Before : Unable to connect Redis via TLS
After : Able to connect Redis via TLS with or without Client Authentication redis setup
Added Test cases :
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests