Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Added TLS support for Redis Datasource #37106

Open
wants to merge 2 commits into
base: release
Choose a base branch
from
Open

feat: Added TLS support for Redis Datasource #37106

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
82d1955
Added TLS support for Redis Datasource
AnnaHariprasad5123 Oct 25, 2024
2abfee6
fix: some changes
AnnaHariprasad5123 Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions ...psmith-interfaces/src/main/java/com/appsmith/external/models/DatasourceConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,9 @@ public class DatasourceConfiguration implements AppsmithDomain {
@JsonView({Views.Public.class, FromRequest.class})
AuthenticationDTO authentication;

@JsonView({Views.Public.class, FromRequest.class})
TlsConfiguration tlsConfiguration;

@JsonView({Views.Public.class, FromRequest.class})
SSHConnection sshProxy;

Expand Down
36 changes: 36 additions & 0 deletions ...rver/appsmith-interfaces/src/main/java/com/appsmith/external/models/TlsConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
package com.appsmith.external.models;

import com.appsmith.external.views.FromRequest;
import com.appsmith.external.views.Views;
import com.fasterxml.jackson.annotation.JsonView;
import lombok.AllArgsConstructor;
import lombok.Getter;
import lombok.NoArgsConstructor;
import lombok.Setter;
import lombok.ToString;

@Getter
@Setter
@ToString
@NoArgsConstructor
@AllArgsConstructor
public class TlsConfiguration {

@JsonView({Views.Public.class, FromRequest.class})
Boolean tlsEnabled;

@JsonView({Views.Public.class, FromRequest.class})
Boolean verifyTlsCertificate;
NilanshBansal marked this conversation as resolved.
Show resolved Hide resolved

@JsonView({Views.Public.class, FromRequest.class})
UploadedFile caCertificateFile;

@JsonView({Views.Public.class, FromRequest.class})
Boolean requiresClientAuth;

@JsonView({Views.Public.class, FromRequest.class})
UploadedFile clientCertificateFile;

@JsonView({Views.Public.class, FromRequest.class})
UploadedFile clientKeyFile;
}
42 changes: 39 additions & 3 deletions app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/plugins/RedisPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
import com.appsmith.external.models.DatasourceTestResult;
import com.appsmith.external.models.Endpoint;
import com.appsmith.external.models.RequestParamDTO;
import com.appsmith.external.models.TlsConfiguration;
import com.appsmith.external.plugins.BasePlugin;
import com.appsmith.external.plugins.PluginExecutor;
import com.external.plugins.exceptions.RedisErrorMessages;
Expand Down Expand Up @@ -45,6 +46,7 @@
import java.util.stream.Collectors;

import static com.appsmith.external.constants.ActionConstants.ACTION_CONFIGURATION_BODY;
import static com.external.utils.RedisTLSManager.createJedisPoolWithTLS;
import static org.apache.commons.lang3.StringUtils.isBlank;

@Slf4j
Expand Down Expand Up @@ -261,9 +263,16 @@ public Mono<JedisPool> datasourceCreate(DatasourceConfiguration datasourceConfig
int timeout =
(int) Duration.ofSeconds(CONNECTION_TIMEOUT).toMillis();
URI uri = RedisURIUtils.getURI(datasourceConfiguration);
JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout);
log.debug(Thread.currentThread().getName() + ": Created Jedis pool.");
return jedisPool;
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);
}
Comment on lines +266 to +275
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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);
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @AnnaHariprasad5123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

})
.subscribeOn(scheduler);
}
Expand Down Expand Up @@ -301,6 +310,33 @@ public Set<String> validateDatasource(DatasourceConfiguration datasourceConfigur
invalids.add(RedisErrorMessages.DS_MISSING_PASSWORD_ERROR_MSG);
}

TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Add null check for TlsConfiguration in validation to prevent NullPointerException

In validateDatasource, accessing getTlsConfiguration() without checking if it's null can cause a NullPointerException. Please ensure that tlsConfiguration is not null before accessing its methods.

TlsConfiguration tlsConfiguration = datasourceConfiguration.getTlsConfiguration();
+ if (tlsConfiguration != null && tlsConfiguration.getTlsEnabled()) {
    // Existing TLS validation logic
}

Committable suggestion was skipped due to low confidence.

if (tlsConfiguration != null && tlsConfiguration.getTlsEnabled()) {
// Check for CA certificate if TLS verification is enabled
if (tlsConfiguration.getVerifyTlsCertificate()
&& (tlsConfiguration.getCaCertificateFile() == null
|| StringUtils.isNullOrEmpty(
tlsConfiguration.getCaCertificateFile().getBase64Content()))) {
invalids.add(RedisErrorMessages.CA_CERTIFICATE_MISSING_ERROR_MSG);
}

// Check for client certificate and key if client authentication is required
if (tlsConfiguration.getRequiresClientAuth()) {
if (tlsConfiguration.getClientCertificateFile() == null
|| StringUtils.isNullOrEmpty(
tlsConfiguration.getClientCertificateFile().getBase64Content())) {
invalids.add(
RedisErrorMessages.TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_CERTIFICATE_MISSING_ERROR_MSG);
}

if (tlsConfiguration.getClientKeyFile() == null
|| StringUtils.isNullOrEmpty(
tlsConfiguration.getClientKeyFile().getBase64Content())) {
invalids.add(RedisErrorMessages.TLS_CLIENT_AUTH_ENABLED_BUT_CLIENT_KEY_MISSING_ERROR_MSG);
}
}
}

return invalids;
}

Expand Down
15 changes: 15 additions & 0 deletions ...plugins/redisPlugin/src/main/java/com/external/plugins/exceptions/RedisErrorMessages.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,4 +29,19 @@ public class RedisErrorMessages extends BasePluginErrorMessages {

public static final String DS_MISSING_PASSWORD_ERROR_MSG =
"Could not find password. Please edit the 'Password' field to provide the password.";

/*
************************************************************************************************************************************************
Error messages related to TLS configuration.
************************************************************************************************************************************************
*/

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_KEY_MISSING_ERROR_MSG =
"Client authentication is enabled but the client key is missing. Please upload the client key.";
}
131 changes: 131 additions & 0 deletions ...server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisTLSManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,131 @@
package com.external.utils;

import com.appsmith.external.models.DatasourceConfiguration;
import com.appsmith.external.models.TlsConfiguration;
import lombok.extern.slf4j.Slf4j;
import redis.clients.jedis.JedisPool;
import redis.clients.jedis.JedisPoolConfig;

import javax.net.ssl.KeyManagerFactory;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
import javax.net.ssl.TrustManagerFactory;
import javax.net.ssl.X509TrustManager;
import java.io.ByteArrayInputStream;
import java.net.URI;
import java.nio.charset.StandardCharsets;
import java.security.KeyFactory;
import java.security.KeyStore;
import java.security.PrivateKey;
import java.security.SecureRandom;
import java.security.cert.CertificateFactory;
import java.security.cert.X509Certificate;
import java.security.spec.PKCS8EncodedKeySpec;
import java.util.Base64;

@Slf4j
public class RedisTLSManager {

public static JedisPool createJedisPoolWithTLS(
JedisPoolConfig poolConfig, URI uri, int timeout, DatasourceConfiguration datasourceConfiguration)
throws Exception {
NilanshBansal marked this conversation as resolved.
Show resolved Hide resolved

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");
}
SSLContext sslContext = SSLContext.getInstance("TLS");
KeyManagerFactory keyManagerFactory = null;

// Handle client authentication if required, regardless of certificate verification
if (requiresClientAuth) {

CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509");

// Load client certificate
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);
Comment on lines +52 to +59
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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

PrivateKey privateKey = loadPrivateKey(clientKey);
Comment on lines +52 to +60
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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 data

Committable suggestion was skipped due to low confidence.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @AnnaHariprasad5123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


// KeyStore for client authentication
KeyStore keyStore = KeyStore.getInstance(KeyStore.getDefaultType());
keyStore.load(null, null);
keyStore.setKeyEntry("client-key", privateKey, null, new X509Certificate[] {clientCert});

keyManagerFactory = KeyManagerFactory.getInstance(KeyManagerFactory.getDefaultAlgorithm());
keyManagerFactory.init(keyStore, null);
}

// Handle server certificate verification
TrustManager[] trustManagers;
if (verifyTlsCertificate) {

// CA certificate verification
String caCertContent =
new String(tlsConfiguration.getCaCertificateFile().getDecodedContent(), StandardCharsets.UTF_8);

CertificateFactory certificateFactory = CertificateFactory.getInstance("X.509");
X509Certificate caCert = (X509Certificate)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly please add a try-catch exception handling here

certificateFactory.generateCertificate(new ByteArrayInputStream(caCertContent.getBytes()));

KeyStore trustStore = KeyStore.getInstance(KeyStore.getDefaultType());
trustStore.load(null, null);
trustStore.setCertificateEntry("ca-cert", caCert);

TrustManagerFactory trustManagerFactory =
TrustManagerFactory.getInstance(TrustManagerFactory.getDefaultAlgorithm());
trustManagerFactory.init(trustStore);
trustManagers = trustManagerFactory.getTrustManagers();
} else {
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) {}
}
};
}
Comment on lines +92 to +106
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is user preference to verify the CA certificate or not.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.


// Initialize SSL context with appropriate managers
sslContext.init(
requiresClientAuth ? keyManagerFactory.getKeyManagers() : null, trustManagers, new SecureRandom());
Comment on lines +109 to +110
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @AnnaHariprasad5123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!


SSLSocketFactory sslSocketFactory = sslContext.getSocketFactory();

// Create and return JedisPool with TLS
JedisPool jedisPool = new JedisPool(poolConfig, uri, timeout, sslSocketFactory, null, null);
log.debug(Thread.currentThread().getName() + ": Created Jedis pool with TLS.");
return jedisPool;
}

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);
}
Comment on lines +120 to +130
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Improve private key handling and support multiple algorithms

The current implementation has several areas for improvement:

  1. Only supports RSA keys
  2. Uses string operations for key processing
  3. 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.

Suggested change
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)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @AnnaHariprasad5123 please check this suggestion

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

}
10 changes: 9 additions & 1 deletion app/server/appsmith-plugins/redisPlugin/src/main/java/com/external/utils/RedisURIUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,17 @@ public class RedisURIUtils {
public static final Long DEFAULT_PORT = 6379L;
private static final String REDIS_SCHEME = "redis://";

private static final String REDIS_SSL_SCHEME = "rediss://";

public static URI getURI(DatasourceConfiguration datasourceConfiguration) throws URISyntaxException {
StringBuilder builder = new StringBuilder();
builder.append(REDIS_SCHEME);

if (datasourceConfiguration.getTlsConfiguration() != null
&& datasourceConfiguration.getTlsConfiguration().getTlsEnabled()) {
builder.append(REDIS_SSL_SCHEME);
} else {
builder.append(REDIS_SCHEME);
}
Comment on lines +20 to +26
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc: @AnnaHariprasad5123

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 uriAuth = getUriAuth(datasourceConfiguration);
builder.append(uriAuth);
Expand Down
72 changes: 72 additions & 0 deletions app/server/appsmith-plugins/redisPlugin/src/main/resources/form.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,78 @@
]
}
]
},
{
"sectionName": "TLS Configuration",
"id": 3,
"children": [
{
"sectionName": null,
"children": [
{
"label": "Enable TLS",
"configProperty": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
"controlType": "CHECKBOX",
"initialValue": false
},
{
"label": "Verify TLS Certificate",
"configProperty": "datasourceConfiguration.tlsConfiguration.verifyTlsCertificate",
"controlType": "CHECKBOX",
"initialValue": false,
"hidden": {
"path": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
"comparison": "NOT_EQUALS",
"value": true
}
},
{
"label": "Upload CA Certificate",
"configProperty": "datasourceConfiguration.tlsConfiguration.caCertificateFile",
"controlType": "FILE_PICKER",
"encrypted": true,
"hidden": {
"path": "datasourceConfiguration.tlsConfiguration.tlsEnabled",
"comparison": "NOT_EQUALS",
"value": true
}
},
{
"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
}
}
Comment on lines +97 to +129
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 25, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
{
"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"
}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not working.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

]
}
]
}
]
}
Loading
Loading