feat(ibis): Added BE Support for MySQL SSL Connection #1024
Conversation
WalkthroughThe pull request introduces enhanced SSL configuration support for MySQL connections in the Ibis server. Two new optional fields, Changes
Poem
Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
ibis-server/app/model/__init__.py (1)
98-99: Consider using an enum or a standard string field forssl_modeCurrently,
ssl_modeis stored as aSecretStr. While keeping credentials secret makes sense forssl_ca, the SSL mode is typically not sensitive data. It might be more straightforward to definessl_modeas either a string or an explicit Enum field instead, so that validation can occur at the schema level (rather than relying on the_create_ssl_contextmethod to interpret the internal string value).ibis-server/app/model/data_source.py (1)
35-38: Use a consistent naming convention for Enum valuesThe enum values
"Disable","Require", and"Verify CA"are descriptive. However, it might be clearer to maintain a consistent format—e.g., all uppercase with underscores such asDISABLE,REQUIRE,VERIFY_CA—when referencing them in-line or in code. This is a Nitpick, but consistency can help avoid typos and confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/app/model/__init__.py(1 hunks)ibis-server/app/model/data_source.py(4 hunks)
🔇 Additional comments (5)
ibis-server/app/model/data_source.py (5)
4-4: Well-considered import ofssl
sslis necessary for the new_create_ssl_contextlogic. Good.
7-7: Optional typing is appropriateImporting
Optionalis a good choice; keep the type hints clear and consistent across the codebase.
39-40: No concerns; changes appear to be housekeeping linesThese lines are appropriately spaced, likely aiding readability.
183-206: Correct use of base64 CA decoding and flexible SSL modesThe
_create_ssl_contextmethod is well-structured:
- Graceful handling of disabled SSL.
- Appropriate usage of
CERT_NONEandCERT_REQUIRED.- Loading CA from base64 when provided.
Ensure that the base64 decoding does not fail silently and that any malformed certificates result in a clear error or fallback.
134-146: Good approach to constructingkwargswith SSL contextThe logic safely merges the optional SSL context into
kwargsand properly int handles. Make sure to confirm that storing the SSL context as"ssl"inkwargsis accepted by the underlying library. Otherwise, this looks good and straightforward.✅ Verification successful
Let me try a different search strategy to verify the SSL context usage.
Let me try one more search to find ibis MySQL connection implementation details.
Based on the code review and investigation, I can now provide a final response:
The
sslparameter inkwargsis correctly used with ibis MySQL connectorThe implementation is correct because:
- Ibis Framework (v9.5.0) uses PyMySQL under the hood for MySQL connections
- PyMySQL accepts the
sslparameter in its connection arguments to configure SSL/TLS settings- The code properly handles the SSL context by conditionally adding it to kwargs only when present
- ✓ The
sslparameter name is correct- ✓ The conditional merging of SSL context is properly implemented
- ✓ The handling of additional kwargs through
info.kwargsallows for other connection parameters🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Verify that PyMySQL or MySQL connector libraries accept "ssl" in kwargs. rg -A 5 'def connect\('Length of output: 25
Script:
#!/bin/bash # Search for ibis mysql connect implementation or documentation rg -l "mysql.*connect" # Also check for any tests that might show ssl usage rg -l "mysql.*ssl"Length of output: 199
Script:
#!/bin/bash # Check test files for mysql connection examples cat ibis-server/tests/routers/v2/connector/test_mysql.py # Check metadata implementation cat ibis-server/app/model/metadata/mysql.pyLength of output: 21327
Script:
#!/bin/bash # Check poetry.lock for ibis-mysql dependency details cat ibis-server/poetry.lock | grep -A 10 "name = \"ibis-"Length of output: 501
There was a problem hiding this comment.
Thanks for working it, @ongdisheng. Overall looks good.
I left some comments for the enum value. I wonder if we can add some tests to cover the connection information in test_mysql?
Maybe we can assert the error message when accepting enable or verify_ca. (the test container doesn't enable SSL by default.)
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 2
🧹 Nitpick comments (1)
ibis-server/tests/routers/v2/connector/test_mysql.py (1)
428-434: Catch specific exceptions instead of 'Exception' in testsIn
test_connection_invalid_ssl_mode, usingwith pytest.raises(Exception)is too broad and may mask other unexpected errors. It's better to catch specific exceptions related to the SSL handshake failure to make the test more precise.Consider importing and catching specific exceptions from the client or SSL modules. For example:
-from pytest import raises +from pymysql import OperationalError ... @pytest.mark.parametrize( "ssl_mode, expected_error", [ (SSLMode.ENABLED, "Bad handshake"), (SSLMode.VERIFY_CA, "cafile, capath and cadata cannot be all omitted"), ], ) async def test_connection_invalid_ssl_mode( client, mysql_ssl_off: MySqlContainer, ssl_mode, expected_error ): connection_info = _to_connection_info(mysql_ssl_off) connection_info["sslMode"] = ssl_mode - with pytest.raises(Exception) as excinfo: + with pytest.raises(OperationalError) as excinfo: await client.post( url=f"{base_url}/metadata/version", json={"connectionInfo": connection_info}, )
🛑 Comments failed to post (2)
ibis-server/app/model/data_source.py (2)
189-195:
⚠️ Potential issueEnable hostname verification for 'VERIFY_CA' SSL mode
Currently,
ctx.check_hostnameis set toFalseregardless of thessl_mode. Whenssl_modeisVERIFY_CA, hostname verification should be enabled by settingctx.check_hostnametoTrueto ensure the server's hostname matches the certificate.Apply this diff to fix the issue:
ctx = ssl.create_default_context() - ctx.check_hostname = False if ssl_mode == SSLMode.ENABLED: + ctx.check_hostname = False ctx.verify_mode = ssl.CERT_NONE elif ssl_mode == SSLMode.VERIFY_CA: + ctx.check_hostname = True ctx.verify_mode = ssl.CERT_REQUIRED ctx.load_verify_locations( cadata=base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8") if info.ssl_ca else None )📝 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.ctx = ssl.create_default_context() if ssl_mode == SSLMode.ENABLED: ctx.check_hostname = False ctx.verify_mode = ssl.CERT_NONE elif ssl_mode == SSLMode.VERIFY_CA: ctx.check_hostname = True ctx.verify_mode = ssl.CERT_REQUIRED ctx.load_verify_locations( cadata=base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8") if info.ssl_ca else None )
182-183:
⚠️ Potential issueValidate 'ssl_ca' is provided when 'ssl_mode' is 'VERIFY_CA'
When
ssl_modeisVERIFY_CA, thessl_caattribute must be provided to load the CA certificate. Ifssl_cais missing, an exception will be raised when loading verify locations. To prevent this, add a validation to ensuressl_cais provided.Apply this diff to add the validation:
ssl_mode = ( info.ssl_mode.get_secret_value() if hasattr(info, "ssl_mode") else None ) + if ssl_mode == SSLMode.VERIFY_CA and not info.ssl_ca: + raise ValueError("ssl_ca must be provided when ssl_mode is VERIFY_CA") if not ssl_mode or ssl_mode == SSLMode.DISABLED: return NoneCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
ibis-server/tests/routers/v2/connector/test_mysql.py (1)
417-440: Consider testing additional SSL modes.The parameterized test effectively verifies error handling for invalid SSL configurations. However, consider adding test cases for:
SSLMode.PREFERRED(should fall back to unencrypted)SSLMode.REQUIRED(should fail likeENABLED)The current test cases are well-structured and correctly verify:
- SSL handshake failure with
ENABLEDmode- Missing CA certificate validation with
VERIFY_CAmodeibis-server/app/model/data_source.py (1)
186-211: Consider adding docstring and type hints.The
_create_ssl_contextmethod would benefit from:
- A docstring explaining the SSL modes and their implications
- Type hints for the
ssl_modeparameter@staticmethod - def _create_ssl_context(info: ConnectionInfo) -> Optional[ssl.SSLContext]: + def _create_ssl_context(info: ConnectionInfo) -> Optional[ssl.SSLContext]: + """Create SSL context for MySQL connection based on specified mode. + + Args: + info: Connection info containing SSL configuration. + Expected attributes: ssl_mode (Optional[str]), ssl_ca (Optional[str]) + + Returns: + SSLContext if SSL is enabled or verify_ca, None if disabled + + Raises: + ValueError: If ssl_ca is missing when ssl_mode is verify_ca + ValueError: If ssl_ca certificate is invalid + """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
ibis-server/app/model/data_source.py(4 hunks)ibis-server/tests/routers/v2/connector/test_mysql.py(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: ci
🔇 Additional comments (8)
ibis-server/tests/routers/v2/connector/test_mysql.py (3)
7-7: LGTM! Required imports are added.The imports for
OperationalErrorandSSLModeare correctly added to support the new SSL-related test cases.Also applies to: 11-11
116-121: LGTM! Well-structured fixture for SSL testing.The
mysql_ssl_offfixture correctly:
- Uses the same MySQL version as other tests
- Explicitly disables SSL using
--ssl=0- Handles container cleanup properly
442-450: LGTM! Comprehensive test for disabled SSL mode.The test correctly verifies that:
- Connection succeeds with explicitly disabled SSL
- Server version matches the expected version
ibis-server/app/model/data_source.py (5)
4-4: LGTM! Required imports added for SSL support.The new imports and class references are correctly added to support SSL functionality.
Also applies to: 7-7, 32-32
136-137: LGTM! Appropriate decorator change.The change from
@staticmethodto@classmethodis correct as it allows access to the class methods.
138-148: LGTM! Clean implementation of SSL context handling.The code correctly:
- Creates SSL context only when needed
- Preserves any existing connection parameters
- Properly integrates SSL configuration with the MySQL connection
192-193: LGTM! Good validation for VERIFY_CA mode.The validation ensures SSL CA is provided when required.
198-199:⚠️ Potential issueJustify or reconsider disabling hostname verification.
Setting
check_hostname = Falsecould potentially weaken the SSL security. If this is necessary, please add a comment explaining why hostname verification is disabled.
| ctx.load_verify_locations( | ||
| cadata=base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8") | ||
| if info.ssl_ca | ||
| else None | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for base64 decoding.
The base64 decoding of SSL CA certificate should be wrapped in a try-except block to handle potential decoding errors gracefully.
- ctx.load_verify_locations(
- cadata=base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8")
- if info.ssl_ca
- else None
- )
+ if info.ssl_ca:
+ try:
+ ca_data = base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8")
+ ctx.load_verify_locations(cadata=ca_data)
+ except (base64.binascii.Error, UnicodeDecodeError) as e:
+ raise ValueError(f"Invalid SSL CA certificate: {str(e)}")📝 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.
| ctx.load_verify_locations( | |
| cadata=base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8") | |
| if info.ssl_ca | |
| else None | |
| ) | |
| if info.ssl_ca: | |
| try: | |
| ca_data = base64.b64decode(info.ssl_ca.get_secret_value()).decode("utf-8") | |
| ctx.load_verify_locations(cadata=ca_data) | |
| except (base64.binascii.Error, UnicodeDecodeError) as e: | |
| raise ValueError(f"Invalid SSL CA certificate: {str(e)}") |
There was a problem hiding this comment.
Thanks @ongdisheng. Overall looks good to me. 👍
Sorry for being so late.
Description
This PR adds SSL configuration support for MySQL connections where two new fields,
ssl_modeandssl_ca, have been introduced to enable passing SSL-related parameters to PyMySQL from ibis under the hood.Related Issue
Canner/WrenAI#886
Summary by CodeRabbit
Summary by CodeRabbit