Fix: Add mutual TLS support for dedicated rate limiter Redis connection #7301

buger · 2025-08-12T14:42:46Z

User description

Summary

This PR fixes a critical security issue where the dedicated Redis connection for the rate limiter does not properly implement mutual TLS (mTLS) authentication, even when configured.

Problem Statement

The Tyk Gateway has two distinct code paths for establishing Redis connections:

Main Connection: Handled by storage.ConnectionHandler - correctly implements mTLS
Rate Limiter Connection: Dedicated connection via rate.NewStorage when rate_limiter.storage.type is set to redis

The rate limiter's dedicated Redis connection code in internal/rate/storage.go had a critical flaw. When UseSSL was enabled, it only set the InsecureSkipVerify field and completely ignored the client certificate configuration (CertFile, KeyFile, CAFile).

Security Impact

This creates a security vulnerability where:

Even if administrators correctly configure mTLS for all Redis connections
The rate limiter's connection bypasses these security controls
The connection either fails (if Redis requires client certs) or connects insecurely

Solution

Enhanced the NewStorage function in internal/rate/storage.go with proper mTLS support:

Key Changes:

Added createTLSConfig helper function that properly constructs TLS configuration
Full mTLS support:
- Loads CA certificates for server validation
- Loads client certificate and key for mutual authentication
- Supports TLS version configuration (TLSMinVersion, TLSMaxVersion)
Proper error handling with appropriate logging for certificate loading failures
Maintains backward compatibility with existing configurations

Testing

✅ Comprehensive test coverage added:

Unit tests for all TLS configuration scenarios
Tests for basic SSL, mTLS, CA certificates, TLS versions
Error case handling (invalid paths, missing files)
All existing rate limiter tests continue to pass

Test Results:

=== RUN   TestCreateTLSConfig
    --- PASS: TestCreateTLSConfig/No_SSL_configured
    --- PASS: TestCreateTLSConfig/SSL_with_InsecureSkipVerify
    --- PASS: TestCreateTLSConfig/SSL_with_TLS_versions
    --- PASS: TestCreateTLSConfig/SSL_with_invalid_TLS_version
    --- PASS: TestCreateTLSConfig/SSL_with_CA_certificate
    --- PASS: TestCreateTLSConfig/SSL_with_client_certificate_and_key
    --- PASS: TestCreateTLSConfig/SSL_with_full_mTLS_configuration
--- PASS: TestCreateTLSConfig

Configuration Example

With this fix, the rate limiter now properly respects the following configuration:

rate_limiter:
  storage:
    type: redis
    use_ssl: true
    ca_file: /path/to/ca.pem
    cert_file: /path/to/client-cert.pem
    key_file: /path/to/client-key.pem
    tls_min_version: "1.2"
    tls_max_version: "1.3"

Backward Compatibility

✅ This change is fully backward compatible:

Existing configurations without mTLS continue to work
Configurations with InsecureSkipVerify are preserved
No breaking changes to the API or configuration structure

Checklist

Bug fix (non-breaking change which fixes an issue)
My code follows the code style of this project
I have added tests to cover my changes
All new and existing tests pass

🤖 Generated with Claude Code

PR Type

Bug fix, Tests

Description

Add proper mTLS to rate limiter Redis
Introduce TLS version configuration support
Improve TLS error handling and logging
Add comprehensive unit tests for TLS paths

Diagram Walkthrough

flowchart LR
  NewStorage["rate.NewStorage"] -- "cfg.UseSSL" --> createTLSConfig["Create TLS config with mTLS"]
  createTLSConfig --> CA["Load CA (RootCAs)"]
  createTLSConfig --> ClientCert["Load client cert/key"]
  createTLSConfig --> TLSVer["Set TLS min/max versions"]
  createTLSConfig --> ReturnTLS["Return *tls.Config"]
  Tests["storage_tls_test.go"] -- "unit tests" --> createTLSConfig

File Walkthrough

Relevant files

Bug fix

storage.go `Add mTLS-capable TLS builder and integrate` internal/rate/storage.go Use `createTLSConfig` when `UseSSL` is true Implement `createTLSConfig` for mTLS and TLS versions Add `getTLSVersion` helper with warnings Integrate logging for certificate load failures	+68/-3

Tests

storage_tls_test.go `Unit tests for TLS config and mTLS paths` internal/rate/storage_tls_test.go Add unit tests for TLS config scenarios Cover CA loading, client certs, TLS versions Include helpers to generate test certs/keys Validate error and edge-case behaviors	+247/-0

## Problem The dedicated Redis connection for the rate limiter (when configured with `rate_limiter.storage.type: redis`) was not properly implementing mutual TLS authentication. When `UseSSL` was enabled, the code only set `InsecureSkipVerify` and ignored the client certificate configuration (`CertFile`, `KeyFile`, `CAFile`). This created a security vulnerability where even if administrators configured mTLS for all Redis connections, the rate limiter's dedicated connection would bypass these security controls. ## Solution Enhanced the `NewStorage` function in `internal/rate/storage.go` to properly handle TLS configuration with full mutual TLS support: - Added `createTLSConfig` helper function that loads CA certificates, client certificates, and keys - Added support for TLS version configuration (`TLSMinVersion`, `TLSMaxVersion`) - Properly handles certificate loading errors with appropriate logging - Maintains backward compatibility with existing configurations ## Testing - Added comprehensive unit tests for all TLS configuration scenarios - Tests cover: basic SSL, mTLS, CA certificates, TLS versions, and error cases - All existing rate limiter tests continue to pass ## Impact This fix ensures that when administrators configure mutual TLS for Redis connections, the rate limiter's dedicated connection will properly authenticate using client certificates, maintaining consistent security across all Redis connections. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

github-actions · 2025-08-12T14:43:42Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 PR contains tests
🔒 Security concerns Potentially weakened security on errors: The helper logs and returns a TLS config even when loading CA or client cert fails, which may downgrade intended mTLS to server-only TLS or to TLS with InsecureSkipVerify if configured. Consider returning an error and failing connection setup when mTLS is configured but cert loading fails.
⚡ Recommended focus areas for review mTLS Config Behavior When CA/cert loading fails, the function logs and returns a partially configured TLS config. Confirm this fallback is intended (e.g., still uses InsecureSkipVerify if set) and won’t silently weaken security when mTLS is expected. if cfg.CAFile != "" { caCert, err := os.ReadFile(cfg.CAFile) if err != nil { logrus.WithError(err).Error("Failed to load CA certificate for rate limiter Redis") return tlsConfig } caCertPool := x509.NewCertPool() if !caCertPool.AppendCertsFromPEM(caCert) { logrus.Error("Failed to parse CA certificate for rate limiter Redis") return tlsConfig } tlsConfig.RootCAs = caCertPool } // Load client certificate and key for mutual TLS if cfg.CertFile != "" && cfg.KeyFile != "" { cert, err := tls.LoadX509KeyPair(cfg.CertFile, cfg.KeyFile) if err != nil { logrus.WithError(err).Error("Failed to load client certificate and key for rate limiter Redis") return tlsConfig } tlsConfig.Certificates = []tls.Certificate{cert} } TLS Version Parsing Unknown TLS versions are ignored with a warning, leaving Min/MaxVersion unset. Verify this aligns with configuration expectations and doesn’t mask misconfiguration; consider failing fast or surfacing errors. // getTLSVersion converts a string TLS version to the corresponding tls constant func getTLSVersion(version string) (uint16, bool) { switch version { case "1.0": return tls.VersionTLS10, true case "1.1": return tls.VersionTLS11, true case "1.2": return tls.VersionTLS12, true case "1.3": return tls.VersionTLS13, true default: logrus.Warnf("Unknown TLS version: %s", version) return 0, false } UseSSL Path TLS config is created only when UseSSL is true; tests create TLS config even when UseSSL is false. Ensure callers never pass a non-nil tls.Config when SSL is disabled and rate limiter behavior matches main Redis path. if cfg.UseSSL { tlsConfig = createTLSConfig(cfg) }

github-actions · 2025-08-12T14:44:13Z

API Changes

no api changes detected

github-actions · 2025-08-12T14:44:20Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Security	Avoid early returns; set ServerName When a custom CA is provided, set `ServerName` if available to ensure proper hostname verification and avoid relying on IP SANs only. Additionally, prefer returning the config only after finishing all settings; remove early returns so other TLS options (e.g., client certs) are still applied even if CA load fails. internal/rate/storage.go [79-92] -// Load CA certificate if provided if cfg.CAFile != "" { caCert, err := os.ReadFile(cfg.CAFile) if err != nil { logrus.WithError(err).Error("Failed to load CA certificate for rate limiter Redis") - return tlsConfig + } else { + caCertPool := x509.NewCertPool() + if !caCertPool.AppendCertsFromPEM(caCert) { + logrus.Error("Failed to parse CA certificate for rate limiter Redis") + } else { + tlsConfig.RootCAs = caCertPool + if cfg.Host != "" && tlsConfig.ServerName == "" { + tlsConfig.ServerName = cfg.Host + } + } } - - caCertPool := x509.NewCertPool() - if !caCertPool.AppendCertsFromPEM(caCert) { - logrus.Error("Failed to parse CA certificate for rate limiter Redis") - return tlsConfig - } - tlsConfig.RootCAs = caCertPool } Suggestion importance[1-10]: 7 __ Why: Removing early returns allows other TLS options to be applied even if CA loading fails, which improves robustness, and setting `ServerName` can enhance hostname verification. However, it depends on the presence of a `Host` field in `cfg`, which isn't shown here, so the impact is moderate.	Medium
General	Remove early return on mTLS load Do not return early on client cert load failure; continue building the TLS config so connections that don't require mTLS can still work. Log the error but avoid mutating flow control based on it. internal/rate/storage.go [95-102] -// Load client certificate and key for mutual TLS if cfg.CertFile != "" && cfg.KeyFile != "" { cert, err := tls.LoadX509KeyPair(cfg.CertFile, cfg.KeyFile) if err != nil { logrus.WithError(err).Error("Failed to load client certificate and key for rate limiter Redis") - return tlsConfig + } else { + tlsConfig.Certificates = []tls.Certificate{cert} } - tlsConfig.Certificates = []tls.Certificate{cert} } Suggestion importance[1-10]: 6 __ Why: Avoiding early return lets non-mTLS connections proceed with other TLS settings, improving resilience. The change aligns with the code context, but the impact is limited to specific misconfiguration scenarios.	Low
Possible issue	Do not set TLS when disabled Avoid creating a TLS config when SSL is disabled to prevent unintended defaults (like `InsecureSkipVerify` being false but other fields zeroed). Only pass a TLS config to Redis when SSL is explicitly enabled. This reduces the risk of silently altering connection behavior in non-SSL setups. internal/rate/storage.go [30-32] if cfg.UseSSL { tlsConfig = createTLSConfig(cfg) +} else { + tlsConfig = nil } Suggestion importance[1-10]: 4 __ Why: The suggestion is mostly a no-op since `tlsConfig` is already nil unless SSL is enabled, and the added else sets it to nil explicitly without changing behavior. It is accurate but offers marginal value and no functional impact.	Low

github-actions · 2025-08-12T14:46:26Z

🚀 Performance Snapshot

Effort	Perf Risk	Hot Paths	Benchmarks	TL;DR
Low	🟢	✅	❔	Adding mTLS support to rate limiter Redis connection has minimal performance impact

## Performance Impact Analysis

The PR adds proper mTLS support to the dedicated rate limiter Redis connection by implementing a createTLSConfig helper function. This change primarily affects connection establishment, not request processing. The TLS configuration is created only once during initialization, and certificate loading happens outside the request path. The actual TLS handshake overhead occurs only during connection establishment and reconnection events, not during normal request processing.

## Critical Areas

The most performance-sensitive area affected is the Redis connection initialization for the rate limiter. The added code loads certificates and configures TLS parameters during startup, which adds a small one-time overhead. Since this happens during initialization and not in the request path, the impact on runtime performance is negligible. The TLS handshake itself adds some latency to connection establishment, but this is standard for secure connections and only happens during connection setup.

## Optimization Recommendations

The implementation appears well-optimized with no significant performance concerns. The certificate loading is done once during initialization rather than per request. One minor suggestion would be to consider adding error metrics for certificate loading failures to monitor any potential issues in production. Additionally, ensuring proper certificate caching and reuse across reconnection attempts would help minimize the overhead during connection reestablishment.

## Summary

The PR adds proper mTLS support to the rate limiter Redis connection with minimal performance impact
Certificate loading and TLS configuration happen during initialization, not in the request path
TLS handshake overhead only occurs during connection establishment, not during normal request processing
The security benefits of proper mTLS implementation far outweigh the minimal performance overhead
No suggestions to provide – change LGTM.

Tip: Mention me again using /performance <request>.
Powered by Probe AI
Performance Impact Reviewer Prompt

github-actions · 2025-08-12T14:46:35Z

📦 Impact Review Snapshot

Effort	Downstream Updates	Compatibility	Docs	TL;DR
Low	✅	🟢	📖	mTLS support for rate limiter Redis is a self-contained security fix with no breaking changes

## Impact Assessment

This PR adds proper mutual TLS (mTLS) support for the dedicated rate limiter Redis connection, fixing a security vulnerability. The change is self-contained within the Gateway codebase and doesn't modify any API schemas, protocols, or configuration structures. It simply ensures that existing TLS configuration options are properly applied to the rate limiter Redis connection, which previously only set InsecureSkipVerify and ignored client certificate configuration.

## Required Updates

No code changes are required in downstream repositories:

tyk-operator: No changes needed as this PR doesn't modify API definition schemas or add new configuration fields
tyk-charts: No schema changes required, though documentation updates may be beneficial
portal: No changes needed as it doesn't interact with the rate limiter Redis connection
tyk-sink: No direct impact on MDCB functionality

Documentation updates should mention that mTLS is now properly supported for the rate limiter Redis connection when rate_limiter.storage.type is set to redis.

## Compatibility Concerns

The change is fully backward compatible:

Existing configurations without mTLS continue to work as before
Configurations with InsecureSkipVerify are preserved
No breaking changes to the API or configuration structure
The fix simply ensures that when mTLS is configured, it's properly applied to the rate limiter Redis connection

The PR includes comprehensive test coverage for all TLS configuration scenarios, ensuring reliability.

## Summary & Recommendations

This is a critical security fix that properly implements mTLS for the dedicated rate limiter Redis connection
No downstream repository code changes are required, as this is a self-contained fix
Consider updating documentation in tyk-charts to highlight that mTLS is now properly supported for rate limiter Redis connections
The PR includes proper error handling and logging for certificate loading failures

No suggestions to provide – change LGTM.

Tip: Mention me again using /dependency <request>.
Powered by Probe AI
Tyk Gateway Dependency Impact Reviewer

…ad failures Based on PR feedback, improved the TLS configuration to be more resilient: - Removed early returns when CA certificate or client certificate loading fails - Other TLS settings (versions, InsecureSkipVerify, etc.) are still applied even if one component fails - This allows connections that don't strictly require mTLS to still work with partial configuration - Added test case to verify resilient behavior when CA loading fails but client certs still load This change makes the rate limiter Redis connection more robust in edge cases where certificate files might be temporarily unavailable or misconfigured. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <[email protected]>

github-actions · 2025-08-12T14:49:07Z

🚦 Connectivity Review Snapshot

Effort	Tests	Security	Perf	TL;DR
Low	✅	🔒 critical fix	🟢	Adds missing mTLS support to rate limiter Redis connection, fixing a security vulnerability

## Connectivity Assessment

Redis Connections: PR correctly addresses a critical security issue where the dedicated rate limiter Redis connection was not properly implementing mTLS, even when configured. The fix properly loads CA certificates, client certificates, and respects TLS version settings.
RPC Connections: No impact on RPC connections; changes are isolated to the rate limiter's Redis connectivity.
Synchronization Mechanisms: No changes to synchronization mechanisms; the PR maintains the existing connection behavior while adding proper security.

## Test Coverage Validation

Redis Tests: Excellent test coverage with new storage_tls_test.go file that tests all TLS configuration scenarios including basic SSL, mTLS, CA certificates, TLS versions, and error cases.
RPC Tests: Not applicable as the PR doesn't modify RPC connectivity.
Failure Scenario Tests: Good coverage of error handling for certificate loading failures, invalid paths, and missing files. The implementation logs errors but continues with partial TLS configuration rather than failing completely.

## Security & Performance Impact

Authentication Changes: Critical security improvement by properly implementing mTLS for rate limiter Redis connections, ensuring client authentication works when configured.
Performance Considerations: Minimal performance impact; certificate loading happens once during connection setup.
Error Handling: The implementation logs errors when certificate loading fails but continues with a partial TLS configuration. This is a reasonable approach for backward compatibility but could silently downgrade security if certificates are misconfigured.

## Summary & Recommendations

The PR correctly fixes a critical security vulnerability where mTLS was not properly implemented for rate limiter Redis connections.
Consider adding a configuration option to fail hard when certificate loading fails, rather than silently continuing with reduced security.
The error handling approach prioritizes availability over security, which is reasonable but should be documented.

Tip: Mention me again using /connectivity <request>.
Powered by Probe AI
Connectivity Issues Reviewer Prompt for Tyk Gateway

github-actions · 2025-08-12T14:51:43Z

Performance Impact Reviewer Analysis: PR #7301 - Add mTLS Support for Rate Limiter Redis Connection

This PR addresses a critical security issue where the dedicated Redis connection for the rate limiter wasn't properly implementing mutual TLS (mTLS) authentication, even when configured. The fix adds proper mTLS support to the rate limiter's Redis connection.

Security Impact

The PR fixes a significant security vulnerability where:

Even if administrators correctly configured mTLS for all Redis connections
The rate limiter's connection bypassed these security controls
The connection either failed (if Redis required client certs) or connected insecurely

The implementation now properly loads CA certificates, client certificates, and respects TLS version configurations.

Performance Impact

The performance impact of this change is minimal:

The TLS configuration is created only once during initialization, not in the request path
Certificate loading happens during startup, not during request processing
The actual TLS handshake overhead occurs only during connection establishment and reconnection events
The implementation follows the same pattern as the main Redis connection handler

Code Quality

The implementation is well-structured:

The createTLSConfig helper function follows good practices for TLS configuration
Error handling is appropriate with informative log messages
The code maintains backward compatibility with existing configurations
The PR includes comprehensive test coverage for all TLS configuration scenarios

Recommendations

The implementation is solid, but there are a few minor suggestions:

Consider setting ServerName when a CA certificate is provided to ensure proper hostname verification
The early returns in error handling could be restructured to ensure all TLS options are still applied even if one part fails
Consider adding metrics for certificate loading failures to help monitor issues in production

Overall, this is a critical security fix with minimal performance impact that should be approved.

Tip: Mention me again using /performance <request>.
Powered by Probe AI
Performance Impact Reviewer Prompt

github-actions · 2025-08-12T14:52:05Z

📦 Impact Review Snapshot

Effort	Downstream Updates	Compatibility	Docs	TL;DR
Low	✅	🟢	📖	Critical security fix that properly implements mTLS for rate limiter Redis connections with no breaking changes

## Impact Assessment

This PR addresses a critical security vulnerability by properly implementing mutual TLS (mTLS) for the dedicated rate limiter Redis connection. Previously, when UseSSL was enabled, the code only set the InsecureSkipVerify field and completely ignored client certificate configuration (CertFile, KeyFile, CAFile). The fix adds proper TLS configuration including CA certificate loading, client certificate/key loading, and TLS version configuration, bringing it in line with the main Redis connection's security capabilities.

## Required Updates

No code changes are required in downstream repositories as this is a self-contained security fix within the Tyk Gateway codebase. The fix doesn't modify any API schemas, protocols, or configuration structures - it simply ensures that existing TLS configuration options are properly applied to the rate limiter Redis connection.

Documentation updates should mention that mTLS is now properly supported for the rate limiter Redis connection when rate_limiter.storage.type is set to redis.

## Compatibility Concerns

The change is fully backward compatible:

Existing configurations without mTLS continue to work as before
Configurations with InsecureSkipVerify are preserved
No breaking changes to the API or configuration structure
The implementation includes graceful error handling for certificate loading failures

The PR includes comprehensive test coverage for all TLS configuration scenarios, including error cases, ensuring reliability across different configuration patterns.

## Summary & Recommendations

This PR fixes a critical security vulnerability by properly implementing mTLS for the dedicated rate limiter Redis connection
The implementation is robust with proper error handling and comprehensive test coverage
The fix is self-contained within the Gateway codebase with no downstream impact
Consider updating documentation to highlight that mTLS is now properly supported for rate limiter Redis connections

No suggestions to provide – change LGTM.

Tip: Mention me again using /dependency <request>.
Powered by Probe AI
Tyk Gateway Dependency Impact Reviewer

github-actions · 2025-08-12T14:52:29Z

🛡️ Security Snapshot

Effort	Risk Level	Tests	Compliance	TL;DR
Low	🟢	✅	✔️	Fixes critical mTLS bypass in rate limiter Redis connection

## Security Impact Analysis

This PR addresses a critical security vulnerability where the dedicated Redis connection for the rate limiter did not properly implement mutual TLS (mTLS) authentication even when configured. The fix properly implements mTLS support by adding certificate loading, TLS version configuration, and proper error handling. The implementation is similar to the main Redis connection handler but differs in error handling approach.

## Identified Vulnerabilities

High Severity (Fixed):

The current implementation only sets InsecureSkipVerify and ignores client certificate configuration, allowing potential unauthorized access to Redis when mTLS is expected.

Medium Severity (Remaining):

The error handling in the new implementation continues with a partially configured TLS config when certificate loading fails, potentially silently downgrading security from mTLS to server-only TLS or to TLS with InsecureSkipVerify.
This differs from the main Redis connection handler which returns errors on certificate loading failures, preventing connections with incomplete security configurations.

## Security Recommendations

Consider aligning error handling with the main Redis connection handler by returning errors when certificate loading fails rather than continuing with a partially configured TLS config.
Add a configuration option to fail hard when mTLS is required but cannot be properly configured, rather than continuing with a less secure connection.
Consider adding metrics or alerts when certificate loading fails to ensure operators are aware of potential security downgrades.
Add documentation to clearly explain the behavior when certificate loading fails, so administrators understand the security implications.

## OWASP Compliance

This change improves compliance with several OWASP security principles:

A2:2021 - Cryptographic Failures: Properly implements TLS and mTLS for Redis connections
A3:2021 - Injection: Helps prevent unauthorized access to Redis by enforcing proper authentication
A7:2021 - Identification and Authentication Failures: Ensures proper client authentication through certificates

## Summary

This PR fixes a critical security vulnerability by properly implementing mTLS for the dedicated rate limiter Redis connection.
The implementation is comprehensive, supporting CA certificates, client certificates, and TLS version configuration.
The error handling could be improved to prevent silent security downgrades when certificate loading fails.
The test coverage is excellent, covering various scenarios including error cases.

Tip: Mention me again using /security <request>.
Powered by Probe AI
Security Impact Reviewer Prompt

github-actions · 2025-08-12T14:52:30Z

🛡️ Security Snapshot

Effort	Risk Level	Tests	Compliance	TL;DR
Low	🟢	✅	✔️	Critical security fix that properly implements mTLS for rate limiter Redis connections

## Security Impact Analysis

This PR fixes a critical security vulnerability where the dedicated Redis connection for the rate limiter did not properly implement mutual TLS (mTLS) authentication. The implementation now correctly loads and applies CA certificates, client certificates, and TLS version configurations. The fix ensures that security controls configured by administrators are consistently applied across all Redis connections, eliminating a potential security bypass.

## Identified Vulnerabilities

High Severity:

Fixed: TLS Bypass in Rate Limiter Redis Connection - Prior to this fix, even when mTLS was configured, the rate limiter's Redis connection would ignore client certificate settings, potentially allowing unauthorized connections or exposing traffic to MITM attacks.

Medium Severity:

Partially Addressed: Error Handling - The implementation logs errors but continues with a partially configured TLS setup when certificate loading fails. This could lead to unexpected security downgrade if certificates are misconfigured.

## Security Recommendations

Consider failing the connection setup when certificate loading fails instead of continuing with a partially configured TLS setup, especially for critical security components like mTLS certificates.
Add a warning log when falling back to a less secure configuration, making it more visible to operators that security expectations may not be met.
Consider setting the ServerName in the TLS config when a host is available to ensure proper hostname verification.

## OWASP Compliance

This PR directly addresses OWASP Top 10 concerns:

A2:2021 - Cryptographic Failures: Fixes improper TLS implementation that could lead to insecure communications
A7:2021 - Identification and Authentication Failures: Properly implements mutual TLS for client authentication
A5:2021 - Security Misconfiguration: Ensures security settings are consistently applied across all components

## Summary

This PR fixes a critical security vulnerability by properly implementing mTLS for the rate limiter Redis connection
The implementation includes proper CA certificate validation, client certificate authentication, and TLS version configuration
Comprehensive test coverage ensures the implementation works correctly across various scenarios
The error handling approach could be improved to fail securely rather than continuing with a partially configured TLS setup
Overall, this is a significant security improvement that closes an important gap in the TLS implementation

Tip: Mention me again using /security <request>.
Powered by Probe AI
Security Impact Reviewer Prompt

github-actions · 2025-08-12T14:52:38Z

🚦 Connectivity Review Snapshot

Effort	Tests	Security	Perf	TL;DR
Low	✅	🔒 fixed	🟢	Fixed critical security issue by adding proper mTLS support to rate limiter Redis connection

## Connectivity Assessment

Redis Connections: The PR fixes a critical security issue where the rate limiter's dedicated Redis connection only set InsecureSkipVerify when SSL was enabled, completely ignoring client certificate configuration. The fix properly implements mTLS support with CA certificate validation and client certificate authentication.
RPC Connections: Not affected by this change.
Synchronization Mechanisms: The PR maintains the same synchronization mechanisms but ensures they operate over secure connections when configured.

## Test Coverage Validation

Redis Tests: Excellent test coverage added with comprehensive test cases for all TLS configuration scenarios including CA certificates, client certificates, TLS versions, and error handling.
RPC Tests: Not applicable to this change.
Failure Scenario Tests: Tests include proper handling of invalid paths, missing files, and partial configuration failures, ensuring the system degrades gracefully.

## Security & Performance Impact

Authentication Changes: The PR properly implements mutual TLS authentication for the rate limiter Redis connection, fixing a security vulnerability where client certificates were ignored.
Performance Considerations: Minimal performance impact as certificate loading happens only during initialization, not in the request path.
Error Handling: Improved error handling with appropriate logging for certificate loading failures, allowing the system to continue operating with partial TLS configuration.

## Summary & Recommendations

This PR addresses a critical security issue by ensuring that mTLS configuration is properly applied to the dedicated rate limiter Redis connection.
The implementation follows the same pattern as the main Redis connection handler, ensuring consistency across the codebase.
The comprehensive test suite validates all aspects of the TLS configuration, including error cases.
The fix is backward compatible and maintains the same behavior for existing configurations.
No suggestions to provide – change LGTM.

Tip: Mention me again using /connectivity <request>.
Powered by Probe AI
Connectivity Issues Reviewer Prompt for Tyk Gateway

github-actions bot added the Review effort 2/5 label Aug 12, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix: Add mutual TLS support for dedicated rate limiter Redis connection #7301

Fix: Add mutual TLS support for dedicated rate limiter Redis connection #7301

Uh oh!

buger commented Aug 12, 2025 •

edited by github-actions bot

Loading

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

Uh oh!

Fix: Add mutual TLS support for dedicated rate limiter Redis connection #7301

Are you sure you want to change the base?

Fix: Add mutual TLS support for dedicated rate limiter Redis connection #7301

Uh oh!

Conversation

buger commented Aug 12, 2025 • edited by github-actions bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Summary

Problem Statement

Security Impact

Solution

Key Changes:

Testing

Test Results:

Configuration Example

Backward Compatibility

Checklist

PR Type

Description

Diagram Walkthrough

File Walkthrough

Uh oh!

github-actions bot commented Aug 12, 2025

PR Reviewer Guide 🔍

Uh oh!

github-actions bot commented Aug 12, 2025

Uh oh!

github-actions bot commented Aug 12, 2025

PR Code Suggestions ✨

Uh oh!

github-actions bot commented Aug 12, 2025

🚀 Performance Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

📦 Impact Review Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

🚦 Connectivity Review Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

Performance Impact Reviewer Analysis: PR #7301 - Add mTLS Support for Rate Limiter Redis Connection

Security Impact

Performance Impact

Code Quality

Recommendations

Uh oh!

github-actions bot commented Aug 12, 2025

📦 Impact Review Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

🛡️ Security Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

🛡️ Security Snapshot

Uh oh!

github-actions bot commented Aug 12, 2025

🚦 Connectivity Review Snapshot

Uh oh!

Uh oh!

buger commented Aug 12, 2025 •

edited by github-actions bot

Loading