feat: implement TLS support #860
Conversation
|
Hi, this is WorkflowBot.
|
emiliyank
force-pushed
the
feat/855-implement-tls-support
branch
from
f317bbc to
1280cb1
Compare
Signed-off-by: emiliyank <e.kadiyski@gmail.com>
emiliyank
force-pushed
the
feat/855-implement-tls-support
branch
from
6d638b6 to
5ededd8
Compare
|
@Adityarya11 would it interest you to help review this? |
|
Request review @manishdait @nadineloepfe if able |
|
@0xivanov would you be able to help review this PR? |
| client = Client(network) | ||
|
|
||
| # Enable TLS for consensus nodes. Mirror nodes already require TLS. | ||
| client.set_transport_security(False) |
There was a problem hiding this comment.
Aren't we disabling it here?
There was a problem hiding this comment.
Yes, because locally I cannot run my solo to work with TLS on consensus nodes.
There was a problem hiding this comment.
but it would work on Tetsnet?
Signed-off-by: emiliyank <e.kadiyski@gmail.com>
emiliyank
force-pushed
the
feat/855-implement-tls-support
branch
from
85e94fd to
0edb33b
Compare
There was a problem hiding this comment.
Pull request overview
This PR implements comprehensive TLS support for the Hedera SDK Python, addressing issue #855. The implementation provides fine-grained control over transport security and certificate verification through new API methods.
Key Changes:
- Added
set_transport_security()andset_verify_certificates()methods for TLS configuration - Implemented custom certificate validation using SHA-384 hash verification via
_HederaTrustManager - TLS enabled by default for hosted networks (mainnet, testnet, previewnet), disabled for local networks (solo, localhost)
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 32 comments.
Show a summary per file
| File | Description |
|---|---|
| src/hiero_sdk_python/node.py | Implements core TLS functionality including _HederaTrustManager for certificate validation, certificate fetching, and channel creation with SSL credentials |
| src/hiero_sdk_python/managed_node_address.py | Adds port management for secure/insecure transitions (50211↔50212, 5600↔443) with _to_secure() and _to_insecure() methods |
| src/hiero_sdk_python/client/network.py | Implements network-level TLS configuration with default settings per network type and mirror REST URL construction |
| src/hiero_sdk_python/client/client.py | Exposes TLS control methods at the client level for convenient API access with method chaining support |
| tests/unit/test_node_tls.py | Comprehensive unit tests for node-level TLS operations including channel creation and certificate validation |
| tests/unit/test_network_tls.py | Unit tests for network and client TLS configuration, including default behavior verification |
| tests/unit/test_managed_node_address.py | Tests for port conversion logic between secure and insecure modes |
| tests/unit/test_hedera_trust_manager.py | Tests for certificate hash validation and trust manager initialization |
| tests/integration/tls_integration_test.py | Integration tests verifying TLS functionality across different network types with real queries |
| examples/tls_query_balance.py | Example demonstrating TLS configuration (currently disables TLS despite name) |
| docs/sdk_developers/setup.md | Documentation update noting TLS defaults and VERIFY_CERTS environment variable |
| CHANGELOG.md | Documents the new TLS feature addition |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| context = ssl.create_default_context() | ||
| context.check_hostname = False | ||
| context.verify_mode = ssl.CERT_NONE |
There was a problem hiding this comment.
Potential security concern: Setting verify_mode = ssl.CERT_NONE and check_hostname = False bypasses standard certificate validation. While the code implements custom hash-based verification, this approach could be vulnerable if the hash comparison fails silently or is bypassed. Consider adding a comment explaining why this is safe in this context (custom validation via SHA-384 hash comparison) and ensure the hash validation cannot be skipped.
|
|
||
| def _fetch_server_certificate_hash(self) -> str: | ||
| """ | ||
| Perform a TLS handshake and compute the SHA-384 hash of the server certificate PEM. | ||
| (Kept for backwards compatibility) | ||
| """ | ||
| pem_cert = self._fetch_server_certificate_pem() | ||
| return hashlib.sha384(pem_cert).hexdigest() |
There was a problem hiding this comment.
The method _fetch_server_certificate_hash (lines 256-262) is defined for "backwards compatibility" but there's no evidence it was present in the codebase before (this is a new feature). This method is not used anywhere and should be removed unless there's a specific backwards compatibility requirement.
| def _fetch_server_certificate_hash(self) -> str: | |
| """ | |
| Perform a TLS handshake and compute the SHA-384 hash of the server certificate PEM. | |
| (Kept for backwards compatibility) | |
| """ | |
| pem_cert = self._fetch_server_certificate_pem() | |
| return hashlib.sha384(pem_cert).hexdigest() | |
| # Disable TLS for consensus nodes. Mirror nodes already require TLS. | ||
| client.set_transport_security(False) |
There was a problem hiding this comment.
The example demonstrates TLS functionality but disables TLS on line 58 with client.set_transport_security(False). This contradicts the example's purpose stated in the docstring: "Demonstrates how to connect to the Hedera network with TLS enabled." The example should either enable TLS or update the docstring to clarify that it demonstrates TLS configuration flexibility.
| # Disable TLS for consensus nodes. Mirror nodes already require TLS. | |
| client.set_transport_security(False) | |
| # Mirror nodes already require TLS. |
| node._address_book._cert_hash = wrong_hash | ||
|
|
||
| with patch.object(node, '_fetch_server_certificate_pem', return_value=pem_cert): | ||
| with pytest.raises(ValueError, match="Failed to confirm"): |
There was a problem hiding this comment.
The error message matching pattern "Failed to confirm" is incomplete. The actual error message from _HederaTrustManager.check_server_trusted() is "Failed to confirm the server's certificate from a known address book." Using a partial match could lead to false positives. Consider using the full message or a more specific pattern like "Failed to confirm the server's certificate".
| with pytest.raises(ValueError, match="Failed to confirm"): | |
| with pytest.raises(ValueError, match="Failed to confirm the server's certificate"): |
| assert node._address._is_transport_security() is False, f"Node {node._account_id} should use plaintext port" | ||
| assert node._address._get_port() == 50211, f"Node {node._account_id} should use port 50211 for plaintext" | ||
| finally: | ||
| client.close() |
| assert node._address._is_transport_security() is True, f"Node {node._account_id} should use TLS port after enabling" | ||
| assert node._address._get_port() == 50212, f"Node {node._account_id} should use port 50212 for TLS" | ||
| finally: | ||
| client.close() |
| assert node._address._is_transport_security() is False, f"Node {node._account_id} should use plaintext port after disabling" | ||
| assert node._address._get_port() == 50211, f"Node {node._account_id} should use port 50211 for plaintext" | ||
| finally: | ||
| client.close() |
| assert node._verify_certificates is False, f"Node {node._account_id} should have verification disabled" | ||
| assert node._address._is_transport_security() is True, f"Node {node._account_id} should still use TLS" | ||
| finally: | ||
| client.close() |
| assert rest_url.startswith('https://'), f"REST URL {rest_url} should use HTTPS" | ||
| assert rest_url.endswith('/api/v1'), f"REST URL {rest_url} should end with /api/v1" | ||
| finally: | ||
| client.close() |
|
oh wow, copilot has been going to town :D However, as previously mentioned, I fully agree with: -> If it needs to be disabled there, let's update the docstring. @emiliyank |
| """ | ||
| PORT_NODE_PLAIN = 50211 | ||
| PORT_NODE_TLS = 50212 | ||
| PORT_MIRROR_TLS = 443 |
There was a problem hiding this comment.
I believe src/hiero_sdk_python/managed_node_address.py handles only consensus node connections. If this is true, we should remove the mirror node logic from here and move it to a separate class that handles the mirror node tls
exploreriii
left a comment
exploreriii
left a comment
There was a problem hiding this comment.
Hi @emiliyank
Just wanted to let you know I am making my way through this but you've added a lot of new things, many of which I need to learn, and this will take me some time
Thus request continued help if available @nadineloepfe @0xivanov @rbarker-dev @manishdait - thank you for your help so far
If there is an issue you'd like to work on meanwhile, please feel free while we review this!
4 participants
Description:
TLS support added to Hedera SDK with configurable transport security and certificate verification via new set_transport_security() and set_verify_certificates() methods;
Add support for ...
Fixes #855
Checklist