WIP: Add TLS in the security options #1085
Conversation
libp2p/__init__.py
Outdated
| TLS_PROTOCOL_ID: TLSTransport ( | ||
| key_pair | ||
| ), |
There was a problem hiding this comment.
Added the option for TLS as the 1st priority option for security upgrade,
|
@seetadev: For auto-tls to work, the py-libp2p node needs to be on tls as the autotls-broker was negotiating for tls sec-opt ( as mentioned in this comment ). So presently, while using tls as the security-option, after the completion of security upgrade, the muxer upgrade is failing for some reason possibly during read/write operations, which were happing over a tls-secured stream. The logs are mentioned in this comment |
lla-dane
changed the title
- Changed default key generation back to Ed25519 from RSA - Maintains compatibility with Rust/Go libp2p implementations - TLS transport still works with Ed25519 keys
- Fix line-too-long errors by shortening log messages - Fix import order in muxer_multistream.py (move logger after imports) - Fix type error: handle TProtocol | None from multiselect.negotiate() - Update logging messages for consistency - All tests passing (1744 passed, 12 skipped) - make pr passes: lint, typecheck, and tests all green
|
Hi @seetadev @lla-dane Please take a look at #966 (comment) and comment. This will help me understanding better the TLS and certificates .. |
TLS Muxer Upgrade Fix - Response to @lla-daneIssue SummaryAfter TLS security upgrade (TLS handshake) completion, the multiplexer upgrade was failing/hanging. The code would print Root Causes Identified1. Debug Print Statements
2. TLS Read Hanging Issue (Critical)
3. Server-Side Muxer Negotiation Bug
Fixes AppliedFix 1: TLS Read Returns Data Immediately (Critical Fix)Problem: Solution: Changed to return data immediately when available (stream semantics): # In libp2p/security/tls/io.py - TLSStreamReadWriter.read()
data = self._ssl_socket.read(min(n if n else 65536, 65536))
if data:
buffer.extend(data)
logger.debug("TLS read: %d decrypted bytes", len(data))
# Return immediately when we have data (stream semantics)
break # ← Critical: don't wait to fill bufferWhy this works: TLS is a stream protocol. Small messages (like multistream negotiation) should be processed immediately, not buffered until a large chunk is available. Fix 2: Server-Side Muxer NegotiationProblem: Solution: Use the correct multiselect method based on connection role: # In libp2p/stream_muxer/muxer_multistream.py
if conn.is_initiator:
# Client: select a protocol from the list
protocol = await self.multistream_client.select_one_of(
tuple(self.transports.keys()), communicator, self.negotiate_timeout
)
else:
# Server: negotiate with the client
negotiated_protocol, _ = await self.multiselect.negotiate(
communicator, self.negotiate_timeout
)
if negotiated_protocol is None:
raise MultiselectError(
"Fail to negotiate a stream muxer protocol: no protocol selected"
)
protocol = negotiated_protocolWhy this works: The client (initiator) selects a protocol from a list, while the server (non-initiator) negotiates with the client's request. Using the wrong method causes protocol mismatch. Fix 3: Removed Debug PrintsAll # Before:
print("SECRURITY UPGRADE: COMPLETE")
print("MUX UPGRADE: COMPLETE")
# After:
logger.debug("Swarm: security upgrade completed for peer %s", peer_id)
logger.debug("Swarm: muxer upgrade completed for peer %s", peer_id)VerificationAll issues have been resolved and verified:
Technical DetailsWhy TLS Read Was HangingTLS is a stream protocol, not a message-based protocol like Noise. The original implementation tried to read large chunks (65536 bytes), which works for bulk data transfer but fails for small protocol negotiation messages. Before (hanging): # Tries to read up to 65536 bytes
data = await self.stream_writer.read(65536)
# If only 20 bytes available, waits for more...After (fixed): # Returns immediately when any data is available
data = self._ssl_socket.read(min(n if n else 65536, 65536))
if data:
break # Return immediately, don't waitWhy Server-Side Negotiation Was WrongThe multistream protocol has two roles:
Using the wrong method causes the negotiation to fail because the message flow doesn't match the expected protocol. Files Modified
ConclusionThe muxer upgrade now completes successfully after TLS security upgrade. The root cause was a combination of:
Both issues have been fixed and all tests are passing. |
AI Pull Request Review: PR #1085PR Title: WIP: Add TLS in the security options 1. Summary of ChangesThis PR adds TLS (Transport Layer Security) as a security option in the Key Changes:
Related Context:
Affected Modules:
Breaking Changes:
2. Branch Sync Status and Merge ConflictsBranch Sync Status
Merge Conflict Analysis
3. StrengthsArchitecture and Design
Code Quality
Testing and Reliability
Security
4. Issues FoundCriticalNone - All critical issues have been resolved in this version of the PR. Major1. Missing Issue Reference
2. Missing Newsfragment
Minor1. Documentation Could Be Enhanced
2. Test Coverage for Edge Cases
5. Security ReviewSecurity Strengths
Security Considerations1. Python SSL Limitations
2. Mutual TLS Default Behavior
3. Certificate Generation
Overall Security Assessment
6. Documentation and ExamplesDocumentation Status
Suggestions
7. Newsfragment RequirementCurrent Status
Requirements
Action Required
8. Tests and ValidationTest Execution Summary
Test Coverage by CategoryTLS-Specific Tests
Integration Tests
Warnings
Linting Results
Type Checking Results
Documentation Build Results
Overall Test Assessment
9. Recommendations for ImprovementHigh Priority (Blockers)
Medium Priority
Low Priority
10. Questions for the Author
11. Overall AssessmentQuality RatingExcellent - The PR demonstrates high-quality code with proper architecture, comprehensive testing, and good documentation. All critical bugs have been fixed, and the implementation follows libp2p specifications. Security ImpactLow to Medium - The implementation follows security best practices. Python SSL limitations are well-documented and workarounds are appropriate. No security vulnerabilities identified. Merge ReadinessNeeds fixes - While the code quality is excellent, the PR cannot be approved without:
These are mandatory requirements, not optional. ConfidenceHigh - The implementation is solid, all tests pass, and the code follows best practices. The only blockers are administrative requirements (issue reference and newsfragment). SummaryThis PR successfully adds TLS 1.3 support to py-libp2p with excellent code quality and comprehensive testing. The implementation correctly handles TLS as a stream protocol, includes proper peer ID verification, and addresses Python SSL limitations appropriately. All critical bugs (TLS read hanging, server-side muxer negotiation) have been fixed. The PR is technically ready for merge but requires:
Once these administrative requirements are met, the PR can be approved and merged. Appendix: Key Fixes AppliedFix 1: TLS Read Hanging Issue (Critical)
Fix 2: Server-Side Muxer Negotiation
Fix 3: Debug Print Statements
Fix 4: Default Key Type
Review completed: 2024-12-11 |
3 participants
This PR aims to add TLS-security as one of the security options in the
TransportUpgrader.