Skip to content

Conversation

@taylorleese
Copy link
Contributor

Summary

This PR fixes a bug that causes SSL connections to fail with the error "Cannot set verify_mode to CERT_NONE when check_hostname is enabled" when using REDIS_SSL_CERT_REQS=none. It also corrects inconsistent environment variable naming in the documentation.

1. SSL Hostname Verification Bug Fix

Problem: When setting REDIS_SSL_CERT_REQS=none, the server would crash with:

ValueError: Cannot set verify_mode to CERT_NONE when check_hostname is enabled

This happens because Python's SSL library requires that when verify_mode=ssl.CERT_NONE, the check_hostname parameter must also be set to False.

Use Case: This is essential for scenarios like AWS SSM port forwarding, where:

  • The connection goes to localhost:6379 (via the tunnel)
  • But the SSL certificate is issued for the actual AWS hostname
  • Hostname verification fails because localhost ≠ aws-hostname.amazonaws.com

Solution: Added REDIS_SSL_CHECK_HOSTNAME configuration that:

  • Automatically defaults to False when REDIS_SSL_CERT_REQS=none
  • Matches the behavior of redis-cli --insecure
  • Can be explicitly overridden if needed

2. Environment Variable Naming Fixes

Fixed inconsistencies in .env.example, README.md, and smithery.yaml where some SSL variables were missing the SSL_ prefix:

  • REDIS_CA_PATHREDIS_SSL_CA_PATH
  • REDIS_CERT_REQSREDIS_SSL_CERT_REQS
  • REDIS_CA_CERTSREDIS_SSL_CA_CERTS

These now match the actual variable names used in src/common/config.py.

Changes

  • ✅ Added REDIS_SSL_CHECK_HOSTNAME configuration option to config.py
  • ✅ Automatically disable hostname checking when cert_reqs="none"
  • ✅ Added ssl_check_hostname support in parse_redis_uri() for URI-based config
  • ✅ Pass ssl_check_hostname to both Redis and RedisCluster connections
  • ✅ Added comprehensive tests for the new functionality
  • ✅ Updated all documentation to reflect correct variable names

Test Plan

  • Verified SSL connection with REDIS_SSL_CERT_REQS=none no longer crashes
  • Tested that redis-cli --tls --insecure behavior is matched
  • Added unit tests for the new ssl_check_hostname configuration
  • Verified default behavior (cert_reqs=required → check_hostname=true)
  • Verified auto-disable behavior (cert_reqs=none → check_hostname=false)
  • Verified explicit override works (cert_reqs=none + CHECK_HOSTNAME=true)

Related

This addresses the issue discovered while debugging AWS SSM port forwarding connections where the certificate hostname doesn't match the forwarded localhost address.

@jit-ci
Copy link

jit-ci bot commented Nov 4, 2025

Hi, I’m Jit, a friendly security platform designed to help developers build secure applications from day zero with an MVS (Minimal viable security) mindset.

In case there are security findings, they will be communicated to you as a comment inside the PR.

Hope you’ll enjoy using Jit.

Questions? Comments? Want to learn more? Get in touch with us.

This commit addresses two issues:

1. **SSL Hostname Verification Bug**: Fixed the error "Cannot set verify_mode
   to CERT_NONE when check_hostname is enabled" by adding support for the
   `ssl_check_hostname` parameter. When `REDIS_SSL_CERT_REQS=none` is set,
   hostname checking is now automatically disabled by default, matching the
   behavior of redis-cli's --insecure flag. This is essential for scenarios
   like AWS SSM port forwarding where the connection goes to localhost but
   the certificate is issued for the actual hostname.

2. **Environment Variable Naming**: Fixed inconsistencies in documentation
   (.env.example, README.md, smithery.yaml) where SSL-related environment
   variables were missing the "SSL_" prefix. Updated:
   - REDIS_CA_PATH → REDIS_SSL_CA_PATH
   - REDIS_CERT_REQS → REDIS_SSL_CERT_REQS
   - REDIS_CA_CERTS → REDIS_SSL_CA_CERTS

Changes:
- Added REDIS_SSL_CHECK_HOSTNAME configuration option
- Automatically sets check_hostname=False when cert_reqs="none"
- Added ssl_check_hostname support in parse_redis_uri()
- Passed ssl_check_hostname to both Redis and RedisCluster connections
- Added comprehensive tests for the new functionality
- Updated documentation to reflect correct variable names
@vchomakov vchomakov force-pushed the fix/ssl-hostname-verification branch from 1351460 to 079ebd6 Compare November 4, 2025 17:44
| `REDIS_SSL_CERTFILE` | Client's certificate file for client authentication | None |
| `REDIS_SSL_CERT_REQS` | Certificate requirements (none, optional, or required) | `"required"` |
| `REDIS_SSL_CA_CERTS` | Path to the trusted CA certificates file | None |
| `REDIS_SSL_CHECK_HOSTNAME` | Verify SSL certificate hostname (auto-disabled when cert_reqs=none)| `True` |
Copy link
Contributor

Choose a reason for hiding this comment

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

when REDIS_SSL_CERT_REQS=none

@codecov-commenter
Copy link

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@19994a0). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main      #83   +/-   ##
=======================================
  Coverage        ?   86.03%           
=======================================
  Files           ?       18           
  Lines           ?      795           
  Branches        ?        0           
=======================================
  Hits            ?      684           
  Misses          ?      111           
  Partials        ?        0           
Flag Coverage Δ
unittests 86.03% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants