feat: Added TLS support for Redis Datasource #37106
Conversation
AnnaHariprasad5123
requested review from
nidhi-nair and
sondermanish
as code owners
WalkthroughThe changes in this pull request introduce a new Changes
Assessment against linked issues
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
|
AnnaHariprasad5123
changed the title
AnnaHariprasad5123
changed the title
AnnaHariprasad5123
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
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
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java
Outdated
Show resolved
Hide resolved
NilanshBansal
requested review from
a team
and removed request for
nidhi-nair and
sondermanish
|
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? |
NilanshBansal
added
Query & JS Pod
|
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! |
|
Hi @NilanshBansal, Could you check now. |
|
/build-deploy-preview skip-tests=true recreate=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11891941699. |
|
Deploy-Preview-URL: https://ce-37106.dp.appsmith.com |
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java
Outdated
Show resolved
Hide resolved
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2)
52-59: Secure the mock certificates and keysThe mock certificates and keys should be shortened or obfuscated further in test code to prevent any potential security risks, even though they are mock values.
Apply this diff to improve security:
- private static final String MOCK_CA_CERTIFICATE = - "-----BEGIN CERTIFICATE-----\n" + "MIIC...mock-ca-cert...\n" + "-----END CERTIFICATE-----"; + private static final String MOCK_CA_CERTIFICATE = + "-----BEGIN CERTIFICATE-----\n" + "MOCK_CERT" + "\n-----END CERTIFICATE-----";🧰 Tools
🪛 Gitleaks
59-59: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
98-120: Consider extracting certificate creation logicThe certificate and key creation logic in
createDatasourceConfigurationWithTLSEnabledcould be moved to a separate helper method for better reusability.Consider creating a helper method like this:
+ private TlsConfiguration createTlsConfigurationWithCertificates() { + String base64CaCert = Base64.getEncoder().encodeToString(MOCK_CA_CERTIFICATE.getBytes()); + String base64ClientCert = Base64.getEncoder().encodeToString(MOCK_CLIENT_CERTIFICATE.getBytes()); + String base64PrivateKey = Base64.getEncoder().encodeToString(MOCK_PRIVATE_KEY.getBytes()); + + TlsConfiguration tlsConfiguration = new TlsConfiguration(); + tlsConfiguration.setTlsEnabled(true); + tlsConfiguration.setVerifyTlsCertificate(true); + tlsConfiguration.setCaCertificateFile( + createUploadedFile("ca-cert.crt", base64CaCert, "application/x-x509-ca-cert")); + tlsConfiguration.setRequiresClientAuth(true); + tlsConfiguration.setClientCertificateFile( + createUploadedFile("client-cert.crt", base64ClientCert, "application/x-x509-ca-cert")); + tlsConfiguration.setClientKeyFile( + createUploadedFile("client-key.key", base64PrivateKey, "application/x-pem-file")); + return tlsConfiguration; + }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
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(11 hunks)
🧰 Additional context used
📓 Learnings (1)
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
Learnt from: NilanshBansal
PR: appsmithorg/appsmith#37106
File: app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java:277-287
Timestamp: 2024-11-18T10:40:04.648Z
Learning: When testing with mock certificates in `RedisPluginTest.java`, additional assertions for verifying the connection's security, TLS handshake, and certificate usage are not required.
🪛 Gitleaks
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
59-59: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (6)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (2)
6-6: LGTM! Good additions for TLS support.
The @slf4j annotation and REDIS_SSL_SCHEME constant are well-placed additions that follow existing patterns.
Also applies to: 13-13, 18-18
23-33: LGTM! Well-implemented TLS configuration handling.
The implementation includes proper null checks and maintains a clean builder pattern.
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (4)
69-78: LGTM! Well-structured helper method
The createUploadedFile method is well-implemented with proper null checks and base64 encoding.
220-275: LGTM! Comprehensive TLS validation test cases
The test cases cover all essential TLS validation scenarios:
- Missing CA certificate
- Missing client certificate
- Missing client key
- Valid certificates validation
277-287: LGTM! Test case matches requirements
The test case itShouldTestDatasourceWithTlsEnabledAndValidCertificates is implemented as requested, with appropriate assertions for the mock certificate scenario.
80-84:
Consider removing unused parameter
The addTLSConfiguration method doesn't use its DatasourceConfiguration parameter.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (1)
30-31: Enhance the warning message and consider using StringBuilder consistently.
- The warning message could be more specific about security implications.
- String concatenation in getUriHostAndPort could use StringBuilder for consistency.
Consider these improvements:
- log.warn("Using default non-TLS port {} with TLS enabled", DEFAULT_PORT); + log.warn("Security warning: Using default non-TLS port {} with TLS enabled. Consider using a dedicated TLS port.", DEFAULT_PORT); // In getUriHostAndPort method: - builder.append(host + ":" + port); + builder.append(host).append(":").append(port);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java(1 hunks)
🔇 Additional comments (2)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java (2)
6-6: LGTM! Clean addition of logging and SSL scheme constant.
The @slf4j annotation and REDIS_SSL_SCHEME constant are well-defined and follow existing patterns.
Also applies to: 13-13, 18-18
23-36: LGTM! Comprehensive null checks and TLS validation.
The implementation properly handles all edge cases with appropriate null checks and endpoint validation.
|
|
|
/build-deploy-preview skip-tests=true |
|
Deploying Your Preview: https://github.com/appsmithorg/appsmith/actions/runs/11912651879. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (2)
30-41: Consider declaring specific exceptions instead of generic ExceptionThe method signature could be more explicit about the exceptions it throws, helping callers handle specific error cases better.
public static JedisPool createJedisPoolWithTLS( JedisPoolConfig poolConfig, URI uri, int timeout, DatasourceConfiguration datasourceConfiguration) - throws Exception { + throws CertificateException, IOException, GeneralSecurityException {
42-97: Consider adding debug logging for TLS configuration stepsWhile error logging is present, adding debug logs for successful configuration steps would help with troubleshooting.
if (requiresClientAuth) { + log.debug("Configuring client authentication"); CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509"); + log.debug("Successfully created certificate factory");app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2)
52-59: Consider using shorter mock certificatesThe mock certificates and keys are quite verbose. Consider using shorter placeholder values that still maintain the certificate structure but are more concise for testing purposes.
- private static final String MOCK_CA_CERTIFICATE = - "-----BEGIN CERTIFICATE-----\n" + "MIIC...mock-ca-cert...\n" + "-----END CERTIFICATE-----"; + private static final String MOCK_CA_CERTIFICATE = + "-----BEGIN CERTIFICATE-----\n" + "TEST_CERT" + "\n-----END CERTIFICATE-----";🧰 Tools
🪛 Gitleaks
59-59: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
98-117: Refactor TLS configuration setup for better readabilityThe TLS configuration setup can be made more concise by extracting the certificate encoding logic into a helper method.
+ private String encodeToBase64(String content) { + return Base64.getEncoder().encodeToString(content.getBytes()); + } + private DatasourceConfiguration createDatasourceConfigurationWithTLSEnabled() { // ... endpoint setup ... - String base64CaCert = Base64.getEncoder().encodeToString(MOCK_CA_CERTIFICATE.getBytes()); - String base64ClientCert = Base64.getEncoder().encodeToString(MOCK_CLIENT_CERTIFICATE.getBytes()); - String base64PrivateKey = Base64.getEncoder().encodeToString(MOCK_PRIVATE_KEY.getBytes()); + String base64CaCert = encodeToBase64(MOCK_CA_CERTIFICATE); + String base64ClientCert = encodeToBase64(MOCK_CLIENT_CERTIFICATE); + String base64PrivateKey = encodeToBase64(MOCK_PRIVATE_KEY);
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (6)
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/resources/form.json(1 hunks)app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java(11 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java
- app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java
🧰 Additional context used
📓 Learnings (2)
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (1)
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-12T08:11:25.417Z
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.
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (1)
Learnt from: NilanshBansal
PR: appsmithorg/appsmith#37106
File: app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java:277-287
Timestamp: 2024-11-18T10:40:04.648Z
Learning: When testing with mock certificates in `RedisPluginTest.java`, additional assertions for verifying the connection's security, TLS handshake, and certificate usage are not required.
🪛 Gitleaks
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java
59-59: Identified a Private Key, which may compromise cryptographic security and sensitive data encryption.
(private-key)
🔇 Additional comments (9)
app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json (2)
61-85: LGTM! Well-structured TLS configuration section.
The new TLS configuration section is well-organized and follows the existing form structure pattern. The visibility rule for the client authentication checkbox is correctly implemented.
86-107:
Update visibility conditions for certificate and key fields.
The visibility rules for client certificate and key fields should check both TLS enabled status and client authentication requirement.
As mentioned in the previous review, update the hidden property for both fields to:
"hidden": {
"conditions": [
{
"path": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
"comparison": "NOT_EQUALS",
"value": true
},
{
"path": "datasourceConfiguration.tlsConfiguration.requiresClientAuth",
"comparison": "NOT_EQUALS",
"value": true
}
],
"operator": "OR"
}app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java (3)
1-28: LGTM! Well-organized imports and class structure.
The class is properly organized with all necessary imports for SSL, security, and Redis functionality.
112-138: LGTM! Secure implementation of private key loading
The implementation properly handles:
- Multiple key types (RSA and EC)
- Cleanup of sensitive data
- Comprehensive error handling
98-110: Consider validating pool configuration parameters
Add validation for poolConfig and timeout parameters to ensure they meet Jedis requirements.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java (2)
13-13: LGTM!
The TLS configuration import is correctly placed and necessary for the implementation.
266-275: LGTM! TLS pool creation logic is well-structured
The implementation correctly handles both TLS and non-TLS scenarios, with proper delegation to the TLS utility for secure connections.
app/server/appsmith-plugins/redisPlugin/src/test/java/com/external/plugins/RedisPluginTest.java (2)
80-84: Remove unused parameter from method signature
Based on the past review comments, the datasourceConfiguration parameter is not being used in this method.
- private TlsConfiguration addTLSConfiguration() {
+ private TlsConfiguration createDefaultTlsConfiguration() {
TlsConfiguration tlsConfiguration = new TlsConfiguration();
tlsConfiguration.setTlsEnabled(false);
return tlsConfiguration;
}217-266: LGTM! Comprehensive test coverage for TLS validation
The test cases cover all essential scenarios:
- Missing client certificate
- Missing client key
- Valid certificates
- TLS-enabled connection test
Based on the learnings from @NilanshBansal, the basic assertions are sufficient when using mock certificates.
app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java
Show resolved
Hide resolved
|
Deploy-Preview-URL: https://ce-37106.dp.appsmith.com |
|
@AnnaHariprasad5123 I am getting this error on using my TLS configuration with Redis. The same configuration was working earlier when I uploaded the CA certificate file. |
|
Hi @NilanshBansal, Find the videos below |
|
This PR has not seen activitiy for a while. It will be closed in 7 days unless further activity is detected. |
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