tests/configs/.rr-redis.yaml (1)

Missing TLS configuration file for Redis tests

The test file storage_plugin_test.go contains a test case TestRedisTLS that explicitly looks for a configuration file at configs/.rr-redis-tls.yaml, but this file is missing from the repository. This is a critical issue as the TLS test will fail without the proper configuration.

• Add the missing tests/configs/.rr-redis-tls.yaml file with appropriate TLS configuration settings for Redis
• The file should include TLS-specific settings like certificates, keys, and TLS-enabled Redis connection parameters

🔗 Analysis chain

Line range hint 1-15: Verify TLS configuration file relationship

This appears to be the non-TLS configuration file. Since this PR introduces TLS testing, we should verify that there's a corresponding TLS configuration file with appropriate settings.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for the existence and contents of the TLS configuration file
# Expected: Find a companion TLS config file with appropriate TLS settings

# Check for the TLS config file
echo "Checking for TLS configuration file..."
fd -t f ".rr-redis-tls.yaml" tests/configs/

# If found, show its contents focusing on TLS-related settings
echo "Checking TLS-specific settings..."
rg -A 5 "tls:" tests/configs/

Length of output: 237

---

Script:

#!/bin/bash
# Let's broaden our search to find any TLS-related configuration files
echo "Searching for TLS-related YAML files..."
fd -e yaml -e yml . tests/configs/

echo "Searching for TLS-related configurations in YAML files..."
rg -l "tls|ssl|cert" tests/configs/

echo "Showing contents of all YAML files in configs directory..."
for f in $(fd -e yaml -e yml . tests/configs/); do
    echo "=== Contents of $f ==="
    cat "$f"
    echo -e "\n"
done

Length of output: 453

---

Script:

#!/bin/bash
# Let's check the PR changes to understand the TLS testing context
echo "Checking PR changes for TLS-related files..."
git diff --name-only

echo "Checking PR changes content..."
git diff

echo "Looking for test files related to TLS..."
rg -l "tls|TLS" tests/

Length of output: 348

---

Script:

#!/bin/bash
# Let's examine the test file that mentions TLS
echo "Checking TLS-related test content..."
cat tests/storage_plugin_test.go

# Also check if there are any Redis-related test files
echo -e "\nChecking Redis-related test files..."
rg -l "redis" tests/

Length of output: 8828

tests/configs/.rr-redis-tls.yaml (1)

1-19: Consider enhancing configuration flexibility and documentation

Recommendations for improvement:

1. Support environment variables for certificate paths to make the configuration more flexible across different test environments
2. Add comments in the YAML file documenting the certificate requirements and expected format
3. Consider adding validation for minimum TLS version and cipher suites for enhanced security

Example enhancement:

 tls:
+  # Client certificate in PEM format
   cert: ${REDIS_CLIENT_CERT:-./certs/localhost+2-client.pem}
+  # Client private key in PEM format
   key: ${REDIS_CLIENT_KEY:-./certs/localhost+2-client-key.pem}
+  # Root CA certificate for verification
   root_ca: ${REDIS_ROOT_CA:-./certs/rootCA.pem}
+  # Minimum TLS version (recommended: TLS 1.2)
+  min_version: "1.2"

.github/workflows/linux.yml (2)

43-52: Consider pinning Redis version and adding verification step

While the installation steps are well-structured, consider:

1. Pinning the Redis version for reproducible builds
2. Adding a verification step to ensure Redis is installed correctly

 sudo apt-get update
-sudo apt-get install redis
+sudo apt-get install redis-server=6:7.2.* # Pin to specific version
+redis-server --version # Verify installation

---

61-61: Remove trailing whitespace

There is a trailing space at the end of line 61.

-mkcert -client localhost 127.0.0.1 ::1 
+mkcert -client localhost 127.0.0.1 ::1

🧰 Tools

🪛 yamllint

[error] 61-61: trailing spaces

(trailing-spaces)

kv/tlsconfig.go (1)

Line range hint 119-128: Consider adding early validation of certificate files

The current implementation defers all certificate validation to the callback function. Consider validating the certificate files' existence and permissions when the configuration is first loaded.

Example implementation:

func getClientCertificate(cfg *TLSConfig) func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
	if cfg == nil || cfg.Cert == "" || cfg.Key == "" {
		return nil
	}
+	// Validate certificate files early
+	if _, err := os.Stat(cfg.Cert); err != nil {
+		return nil
+	}
+	if _, err := os.Stat(cfg.Key); err != nil {
+		return nil
+	}
+	// Validate file permissions (on Unix systems)
+	if runtime.GOOS != "windows" {
+		certInfo, _ := os.Stat(cfg.Cert)
+		keyInfo, _ := os.Stat(cfg.Key)
+		if mode := certInfo.Mode().Perm(); mode&0077 != 0 {
+			return nil
+		}
+		if mode := keyInfo.Mode().Perm(); mode&0077 != 0 {
+			return nil
+		}
+	}
	return func(_ *tls.CertificateRequestInfo) (*tls.Certificate, error) {
		cert, err := tls.LoadX509KeyPair(cfg.Cert, cfg.Key)
		if err != nil {
			return nil, err
		}
		return &cert, nil
	}
}

This change:

• Validates file existence early
• Checks file permissions on Unix systems
• Provides better error handling and user experience

tests/storage_plugin_test.go (1)

217-250: Consider extracting common test setup code.

The error handling and shutdown mechanism is duplicated across test functions (TestRedis, TestRedisGlobalSection, and TestRedisTLS). Consider extracting this into a helper function to improve maintainability.

Here's a suggested refactor:

+func setupTestContainer(t *testing.T, cont *endure.Container) (chan struct{}, *sync.WaitGroup) {
+    sig := make(chan os.Signal, 1)
+    signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
+
+    wg := &sync.WaitGroup{}
+    wg.Add(1)
+
+    stopCh := make(chan struct{}, 1)
+
+    ch, err := cont.Serve()
+    assert.NoError(t, err)
+
+    go func() {
+        defer wg.Done()
+        for {
+            select {
+            case e := <-ch:
+                assert.Fail(t, "error", e.Error.Error())
+                err = cont.Stop()
+                if err != nil {
+                    assert.FailNow(t, "error", err.Error())
+                }
+            case <-sig:
+                err = cont.Stop()
+                if err != nil {
+                    assert.FailNow(t, "error", err.Error())
+                }
+                return
+            case <-stopCh:
+                err = cont.Stop()
+                if err != nil {
+                    assert.FailNow(t, "error", err.Error())
+                }
+                return
+            }
+        }
+    }()
+
+    return stopCh, wg
+}

Then use it in the test functions:

-    sig := make(chan os.Signal, 1)
-    signal.Notify(sig, os.Interrupt, syscall.SIGINT, syscall.SIGTERM)
-    
-    wg := &sync.WaitGroup{}
-    wg.Add(1)
-    
-    stopCh := make(chan struct{}, 1)
-    
-    go func() {
-        defer wg.Done()
-        for {
-            select {
-            case e := <-ch:
-                assert.Fail(t, "error", e.Error.Error())
-                err = cont.Stop()
-                if err != nil {
-                    assert.FailNow(t, "error", err.Error())
-                }
-            case <-sig:
-                err = cont.Stop()
-                if err != nil {
-                    assert.FailNow(t, "error", err.Error())
-                }
-                return
-            case <-stopCh:
-                err = cont.Stop()
-                if err != nil {
-                    assert.FailNow(t, "error", err.Error())
-                }
-                return
-            }
-        }
-    }()
+    stopCh, wg := setupTestContainer(t, cont)

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Check if there are any logging statements in the test files that might expose sensitive TLS information
rg -g '*.go' -i '(log|print|fmt).*\.(key|cert|password|secret)' ./tests/

#!/bin/bash
# Let's check the Redis TLS test configuration and related test files for any sensitive data logging
rg -g '*.{go,yaml}' -i 'tls' ./tests/ -A 5 -B 5

# Also check for any logging statements in test files
rg -g '*.go' -i '(log|print|fmt).*' ./tests/ -A 2

      - name: Cleanup sensitive files
        if: always()
        run: |
          cd tests
          rm -rf certs/
          rm -f localhost+2*.pem
          rm -f rootCA.pem

#!/bin/bash
# Search for TLS-related test cases and configurations
echo "Searching for TLS test cases..."
rg -l "TLS|tls" --type go --type yaml

# Search for certificate/key path configurations
echo "Searching for certificate configurations..."
rg -l "cert.*=|key.*=" --type yaml

# Check for any potential hardcoded certificate paths
echo "Checking for hardcoded certificate paths..."
rg -l "\.crt|\.key|\.pem" --type go --type yaml

#!/bin/bash
# Check the TLS configuration in the test yaml file
echo "Content of TLS config file:"
cat tests/configs/.rr-redis-tls.yaml

# Check the test implementation
echo -e "\nTLS-related test implementation:"
rg -A 10 "TLS|tls" tests/storage_plugin_test.go

# Check how TLS config is used in the driver
echo -e "\nTLS usage in driver:"
rg -A 5 "TLS|tls" kv/driver.go

func testRPCMethodsRedis(t *testing.T) {
    conn, err := net.Dial("tcp", "127.0.0.1:6001")
    assert.NoError(t, err)
+
+    // For TLS tests, verify TLS connection
+    if tlsConn, ok := conn.(*tls.Conn); ok {
+        assert.True(t, tlsConn.ConnectionState().HandshakeComplete)
+        assert.NotNil(t, tlsConn.ConnectionState().PeerCertificates)
+    }
+
    client := rpc.NewClientWithCodec(goridgeRpc.NewClientCodec(conn))

.github/workflows/linux.yml (2)

43-52: Add error handling to installation steps

The installation steps look good but could benefit from error handling to ensure the workflow fails fast if any step fails.

 - name: Install Redis
   run: |
+    set -eo pipefail
     sudo apt update
     sudo apt-get install -y lsb-release curl gpg libnss3-tools
     curl -fsSL https://packages.redis.io/gpg | sudo gpg --dearmor -o /usr/share/keyrings/redis-archive-keyring.gpg
     sudo chmod 644 /usr/share/keyrings/redis-archive-keyring.gpg
     echo "deb [signed-by=/usr/share/keyrings/redis-archive-keyring.gpg] https://packages.redis.io/deb $(lsb_release -cs) main" | sudo tee /etc/apt/sources.list.d/redis.list
     sudo apt-get update
     sudo apt-get install redis

---

43-66: Consider refactoring into reusable components

The Redis installation and TLS setup could be refactored into reusable composite actions or separate scripts for better maintainability and reuse across workflows.

Consider:

1. Creating a composite action for Redis installation
2. Moving TLS setup to a separate script
3. Creating a reusable workflow for Redis-related CI tasks

This would make the workflow more maintainable and allow reuse in other repositories that need Redis with TLS support.

🧰 Tools

🪛 actionlint

54-54: shellcheck reported issue in this script: SC2046:warning:10:7: Quote this to prevent word splitting

(shellcheck)

🪛 yamllint

[error] 61-61: trailing spaces

(trailing-spaces)

Configuration used: CodeRabbit UI
Review profile: CHILL

.github/workflows/linux.yml (1)

43-51: Consider pinning Redis version for consistent test environments

While the installation process is secure, consider pinning the Redis version to ensure consistent test environments across different runs.

 sudo apt update
 sudo apt-get install -y lsb-release curl gpg libnss3-tools
 curl -fsSL https://packages.redis.io/gpg | sudo gpg --dearmor -o /usr/share/keyrings/redis-archive-keyring.gpg
 sudo chmod 644 /usr/share/keyrings/redis-archive-keyring.gpg
 echo "deb [signed-by=/usr/share/keyrings/redis-archive-keyring.gpg] https://packages.redis.io/deb $(lsb_release -cs) main" | sudo tee /etc/apt/sources.list.d/redis.list
 sudo apt-get update
-sudo apt-get install redis
+sudo apt-get install redis=6:7.2.*  # Pin to specific version

Configuration used: CodeRabbit UI
Review profile: CHILL

#!/bin/bash
# Description: Verify Redis is running with correct TLS configuration
echo "Checking Redis process and ports..."
ps aux | grep redis-server
netstat -tlpn | grep redis

echo "Verifying Redis responds on both ports..."
redis-cli ping
redis-cli -p 16380 --tls --cert certs/localhost+2-client.pem \
  --key certs/localhost+2-client-key.pem \
  --cacert certs/rootCA.pem ping

#!/bin/bash
# Let's check the workflow file to understand the Redis setup context
cat .github/workflows/linux.yml

.github/workflows/linux.yml (1)

43-52: Add error handling to Redis installation steps

While the installation steps are secure, they would benefit from error handling to ensure each step completes successfully.

 - name: Install Redis
   run: |
+    set -e  # Exit on any error
     sudo apt update
     sudo apt-get install -y lsb-release curl gpg libnss3-tools
     curl -fsSL https://packages.redis.io/gpg | sudo gpg --dearmor -o /usr/share/keyrings/redis-archive-keyring.gpg
+    [ $? -eq 0 ] || { echo "Failed to download and import Redis GPG key"; exit 1; }
     sudo chmod 644 /usr/share/keyrings/redis-archive-keyring.gpg
     echo "deb [signed-by=/usr/share/keyrings/redis-archive-keyring.gpg] https://packages.redis.io/deb $(lsb_release -cs) main" | sudo tee /etc/apt/sources.list.d/redis.list
     sudo apt-get update
     sudo apt-get install redis
+    redis-server --version || { echo "Redis installation failed"; exit 1; }

Configuration used: CodeRabbit UI
Review profile: CHILL

tests/certs/get-test-certs.sh (3)

1-10: Add security warnings to the documentation

While the documentation clearly describes the generated files, it should include explicit warnings about:

• These certificates being for testing purposes only
• Not using these certificates in production
• The potential security implications of committing test certificates to the repository

 # Generate some test certificates which are used by the regression test suite:
+# WARNING: These certificates are for testing purposes only and should never be used
+# in production environments. They are self-signed and have weak security parameters.
+# WARNING: Committing certificates to source control may pose security risks.
 #
 #   tests/tls/ca.{crt,key}          Self signed CA certificate.

---

45-58: Enhance OpenSSL configuration security

The OpenSSL configuration could be more secure with additional restrictions and extended key usage.

 [ server_cert ]
 keyUsage = digitalSignature, keyEncipherment
 nsCertType = server
+extendedKeyUsage = serverAuth
+basicConstraints = CA:FALSE
 subjectAltName = @alt_names
 
 [ client_cert ]
 keyUsage = digitalSignature, keyEncipherment
 nsCertType = client
+extendedKeyUsage = clientAuth
+basicConstraints = CA:FALSE
 
 [ alt_names ]
 IP.1 = 127.0.0.1
 DNS.1 = localhost
+# Add additional test domains if needed
+# DNS.2 = redis.test.local

---

1-64: Add script permissions check and cleanup functionality

Consider adding these improvements for better CI/CD integration:

1. Check script execution permissions
2. Add cleanup functionality
3. Add usage instructions

Add this to the beginning of the script:

 #!/bin/bash
+
+# Ensure script is executable
+if [ ! -x "$0" ]; then
+    echo "Error: Script must be executable"
+    exit 1
+fi
+
+# Add usage function
+usage() {
+    echo "Usage: $0 [--cleanup]"
+    echo "  --cleanup  Remove all generated certificates"
+    exit 1
+}
+
+# Add cleanup function
+cleanup() {
+    echo "Cleaning up generated certificates..."
+    rm -rf tests/tls
+}
+
+# Handle command line arguments
+while [ "$1" != "" ]; do
+    case $1 in
+        --cleanup )  cleanup
+                    exit
+                    ;;
+        * )         usage
+                    exit 1
+    esac
+    shift
+done

Configuration used: CodeRabbit UI
Review profile: CHILL