Skip to content

Conversation

@Fatumayattani
Copy link

What was wrong?

Issue #700 follow-up. After @guha-rahul’s TLS (PR #831) was merged, there was still no simple runnable example demonstrating how to start a TLS-enabled host in py-libp2p.
Developers needed a small working reference to verify that the new transport layer functions as expected.

How was it fixed?

Added a short example under examples/doc-examples/example_tls.py that demonstrates how to create and start a TLS-secured host using TLSTransport.
The example runs locally, prints the peer ID, and lists the secure multiaddrs for verification.

To-Do

Output

TLS host starts successfully with secure addresses and confirms that TLS handshake is active.
A sample terminal output is attached below for reference.

pyexample

Cute Animal Picture

@seetadev
Copy link
Contributor

seetadev commented Oct 8, 2025

@yashksaini-coder : Thank you so much to you and @Fatumayattani for your efforts. Appreciate it.

It took me sometime to reach this PR as there were close to 12+ PRs in the review queue. Did re-run the CI/CD pipeline. Waiting for the test results.

Wish if you could collaborate with @Fatumayattani and arrive at a good conclusion on this PR soon.

We need a proper screencast demonstrating the example using TLS along with proper documentation to enable interop testing using TLS. CCing @acul71, who could help you on the transport interop testing front using TLS example.

@acul71
Copy link
Contributor

acul71 commented Oct 8, 2025

CCing @acul71, who could help you on the transport interop testing front using TLS example.

ping me when is merged, I'll try to integrate in interop-tests

@Fatumayattani
Copy link
Author

Thank you @seetadev and @acul71 for the feedback and clear guidance 🙏
Really appreciate everyone here for the support.

Here’s the screencast showing the TLS example running successfully

libv.mp4

@yashksaini-coder , please feel free to add the documentation part to make it easier to use for interop testing 🙏

@yashksaini-coder
Copy link
Contributor

Thank you @seetadev and @acul71 for the feedback and clear guidance 🙏 Really appreciate everyone here for the support.

Here’s the screencast showing the TLS example running successfully

libv.mp4
@yashksaini-coder , please feel free to add the documentation part to make it easier to use for interop testing 🙏

Got it, I will add the documentation along with discussion post to map the implementation done

@acul71
Copy link
Contributor

acul71 commented Oct 9, 2025

Here’s the screencast showing the TLS example running successfully

Hi @Fatumayattani thanks for the video. Is that showing a python server ? Is there a client also ?

@Fatumayattani
Copy link
Author

Here’s the screencast showing the TLS example running successfully

Hi @Fatumayattani thanks for the video. Is that showing a python server ? Is there a client also ?

Hi @acul71, yes that’s the server side. I’ll add a simple client example too to make interop testing smoother 🙌🏽

@seetadev
Copy link
Contributor

@yashksaini-coder : Thank you so much for considering our feedback points. Appreciate the efforts.

Re-ran the CI/CD pipeline. Hopefully, all the tests should pass.

CCing @acul71, who could use this example for testing transport interop tests.

@seetadev
Copy link
Contributor

@yashksaini-coder : Wish if you could resolve the CI/CD issues.

…le scripts; improve certificate verification docstring formatting
@yashksaini-coder
Copy link
Contributor

@yashksaini-coder : Wish if you could resolve the CI/CD issues.

All issues resolved, failing ci-checks. Requesting an update and further feedback.
@seetadev @acul71 @lla-dane

@acul71
Copy link
Contributor

acul71 commented Nov 24, 2025

@yashksaini-coder @Fatumayattani
Thanks, for this PR. please give feedback to this review

AI Pull Request Review: #978

PR Title: Add TLS example under examples/doc-examples (follow-up to PR #831)
Author: Fatumayattani
Review Date: 2025-11-24
Review Version: 0


1. Summary of Changes

This PR adds TLS example implementations to demonstrate how to use TLS-secured connections in py-libp2p. The PR is a follow-up to PR #831 (which added TLS transport support) and addresses the need for simple, runnable examples that developers can use to verify TLS functionality.

Issues Addressed

  • Issue Add tls #700: "Add tls" - This PR provides the missing TLS examples that were requested after TLS transport was implemented in PR Add tls #831.

Files Changed

  1. New Files:

    • examples/doc-examples/example_tls.py - TLS server example (155 lines)
    • examples/doc-examples/example_tls_client.py - TLS client example with echo and chat modes (360 lines)
  2. Modified Files:

    • libp2p/security/tls/certificate.py - Added strict_verify parameter to verify_certificate_chain() function with development mode support
    • libp2p/security/tls/io.py - Added extensive debug print statements throughout TLS handshake, read, and write operations
    • libp2p/security/secure_session.py - Added debug print statements to read/write methods and improved error handling

Architecture Impact

  • Examples Module: Adds two new example files demonstrating TLS usage patterns
  • Security Module: Modifies certificate verification to support a "development mode" that relaxes strict verification
  • TLS IO Module: Adds verbose debugging output (via print statements) throughout the TLS connection lifecycle

Breaking Changes

None - This is an additive change that only adds examples and optional development-mode features.


2. Branch Sync Status and Merge Conflicts

Branch Sync Status

  • Status:Ahead of origin/main
  • Details: Branch is 0 commits behind and 26 commits ahead of origin/main (0 26)
  • Interpretation: The PR branch contains 26 commits that are not yet in origin/main. This is expected for a feature branch and indicates the branch needs to be merged into main.

Merge Conflict Analysis

  • Conflicts Detected:No conflicts - PR can be merged cleanly
  • Details: Test merge completed successfully with no conflicting files detected.

3. Strengths

  1. Comprehensive Examples: The PR provides both server and client examples, with the client supporting both echo and chat modes, giving developers multiple usage patterns to learn from.

  2. Good Documentation: Both example files include clear docstrings, usage instructions, and helpful comments explaining the TLS setup process.

  3. Address Paradigm Usage: The server example (example_tls.py) correctly uses the new address paradigm functions (get_available_interfaces and get_optimal_binding_address), demonstrating best practices.

  4. Error Handling: The client example includes retry logic and proper error handling for connection failures.

  5. User-Friendly Output: Both examples provide clear, timestamped output that helps users understand what's happening during connection establishment and message exchange.

  6. Type Hints: The code includes proper type hints (e.g., bytes | None return types), improving code clarity.


4. Issues Found

Critical

None identified.

Major

4.1 Missing Newsfragment

  • File: newsfragments/
  • Issue: No newsfragment file exists for issue Add tls #700 or PR Add TLS example under examples/doc-examples (follow-up to PR #831) #978
  • Impact: PR cannot be approved without a newsfragment (mandatory requirement)
  • Suggestion: Create newsfragments/700.feature.rst (or 700.misc.rst if this is considered a documentation/example addition) with content describing the addition of TLS examples for users. The file must:
    • Follow the format <ISSUE_NUMBER>.<TYPE>.rst
    • Contain a user-facing description (not developer-focused)
    • End with a newline character
    • Example content: "Added TLS server and client examples demonstrating how to create and connect to TLS-secured libp2p hosts."

4.2 Test Failure: Address Paradigm Not Used in Client Example

  • File: examples/doc-examples/example_tls_client.py
  • Line(s): Entire file
  • Issue: The client example does not use the address paradigm functions (get_available_interfaces or get_optimal_binding_address), causing test test_doc_examples_use_paradigm to fail
  • Test Output:
    FAILED tests/examples/test_examples_bind_address.py::TestExamplesAddressParadigm::test_doc_examples_use_paradigm
    AssertionError: Documentation example examples/doc-examples/example_tls_client.py should use get_available_interfaces
    
  • Suggestion: While the client doesn't need to listen on addresses (it uses listen_addrs=[]), the test requires all doc examples to use the address paradigm functions. Consider adding a comment or minimal usage of these functions, or update the test to allow exceptions for client-only examples. However, since the client doesn't bind addresses, this may require discussion with maintainers about whether the test requirement is appropriate for client examples.

4.3 Linting Errors: Line Too Long

  • File: libp2p/security/tls/certificate.py
  • Line(s): 275, 281
  • Issue: Two lines exceed the 88-character limit (both are 94 characters)
    • Line 275: strict_verify: If True, enforce strict verification; if False, log errors but continue
    • Line 281: ValueError: If verification fails and strict_verify=True, such as expired certificate,
  • Suggestion: Break these lines to comply with the project's line length limit. For example:
    strict_verify: If True, enforce strict verification; if False,
        log errors but continue

4.4 Debug Print Statements in Production Code

  • File: libp2p/security/tls/io.py, libp2p/security/secure_session.py
  • Line(s): Multiple locations throughout both files
  • Issue: Extensive use of print() statements for debugging in production code. These statements:
    • Will always execute and cannot be disabled
    • Clutter output for users who don't need debug information
    • Are not the standard Python logging approach
    • Examples include: print(f"[TLS] Starting handshake..."), print(f"[SecureSession] Reading message...")
  • Suggestion: Replace print() statements with proper logging using Python's logging module. This allows:
    • Users to control log levels via configuration
    • Debug information to be disabled in production
    • Proper log formatting and output destinations
    • Example: logger.debug("[TLS] Starting handshake...") instead of print(...)

4.5 Development Mode in Certificate Verification

  • File: libp2p/security/tls/certificate.py
  • Line(s): 267-466 (entire verify_certificate_chain function)
  • Issue: The addition of strict_verify=False parameter creates a "development mode" that:
    • Bypasses certificate validation errors (expired certificates, missing extensions, invalid signatures)
    • Generates placeholder keys when verification fails
    • Uses print() statements for warnings instead of logging
    • Could mask security issues if accidentally used in production
  • Suggestion:
    • Consider if this development mode should be in the main codebase or only in examples
    • If kept, add clear warnings and documentation about security implications
    • Use proper logging instead of print statements
    • Consider making this a separate function or clearly marking it as "unsafe for production"
    • Ensure the default behavior (strict_verify=False) is intentional and documented

Minor

4.6 Unused Import Removed by Linter

  • File: examples/doc-examples/example_tls.py, examples/doc-examples/example_tls_client.py
  • Issue: The linter automatically removed unused imports (multiaddr from server, get_available_interfaces/get_optimal_binding_address from client)
  • Note: This was automatically fixed by pre-commit hooks, but indicates the original code had unused imports that should have been caught earlier.

4.7 Variable Name Shadowing

  • File: examples/doc-examples/example_tls_client.py
  • Line(s): 90, 96, 259, 264, 392, 405, 443, 444, 477
  • Issue: The variable name message is used in multiple contexts (function parameter, local variable, formatted string), which can be confusing
  • Suggestion: Use more descriptive variable names like client_message, formatted_message, error_message, etc. to avoid shadowing and improve clarity.

4.8 Inconsistent Error Handling

  • File: examples/doc-examples/example_tls_client.py
  • Line(s): 326-327
  • Issue: Some error paths return None while others raise exceptions, making error handling inconsistent
  • Suggestion: Consider a consistent error handling strategy (either always return None/raise exceptions, or use a Result type pattern)

5. Security Review

Security Concerns

5.1 Development Mode Security Risk

  • Risk: The strict_verify=False mode in verify_certificate_chain() bypasses critical security checks
  • Impact: HIGH - If used in production, this could allow:
    • Expired certificates to be accepted
    • Certificates with invalid signatures to be trusted
    • Missing libp2p extensions to be ignored
    • Placeholder keys to be generated instead of proper peer verification
  • Mitigation:
    • Ensure strict_verify=False is clearly documented as "development only"
    • Consider requiring an explicit environment variable or configuration flag to enable this mode
    • Add runtime warnings when development mode is active
    • Consider making strict_verify=True the default in production code
    • Document security implications clearly in function docstring

5.2 Print Statements May Leak Sensitive Information

  • Risk: Debug print statements in TLS code may output sensitive connection details
  • Impact: LOW - Print statements could expose:
    • TLS handshake details
    • Connection timing information
    • Certificate information (though certificates are public)
  • Mitigation: Replace with proper logging that can be controlled and filtered, and ensure no sensitive data (like private keys) is ever logged

5.3 Certificate Verification Bypass Logic

  • Risk: The development mode generates placeholder Ed25519 keys when certificate verification fails
  • Impact: MEDIUM - This could lead to:
    • Incorrect peer identity verification
    • Security vulnerabilities if accidentally enabled in production
  • Mitigation:
    • Add clear warnings and documentation
    • Consider requiring explicit opt-in for development mode
    • Ensure production code paths never use development mode

Positive Security Aspects

  1. TLS 1.3 Enforcement: The code checks for TLS 1.3 version (though the check is relaxed in some cases)
  2. Proper Certificate Chain Handling: The certificate verification logic is comprehensive when strict_verify=True
  3. Secure Defaults in Examples: The examples use proper TLS transport configuration

6. Documentation and Examples

Strengths

  1. Comprehensive Example Files: Both server and client examples are well-documented with:

    • Clear docstrings explaining purpose and usage
    • Command-line argument parsing with help text
    • Usage examples in docstrings
    • Helpful output messages for users
  2. Multiple Usage Patterns: The client example demonstrates both echo and chat modes, giving users flexibility

  3. Clear Instructions: Examples include instructions on how to run and connect to the server

Issues

  1. Missing README Update: The examples/doc-examples/ directory may need a README update to document the new TLS examples

  2. No Integration with Existing Documentation: The examples aren't referenced in the main documentation (though this may be intentional for doc-examples)

  3. Development Mode Undocumented: The strict_verify parameter and its security implications are not documented in user-facing documentation


7. Newsfragment Requirement

⚠️ CRITICAL BLOCKER

Missing Newsfragment

  • Severity: CRITICAL / BLOCKER
  • Issue: No newsfragment file exists for issue Add tls #700 or PR Add TLS example under examples/doc-examples (follow-up to PR #831) #978
  • Impact: PR cannot be approved without a newsfragment (this is a mandatory requirement, not optional)
  • Required Action:
    1. Create a newsfragment file: newsfragments/700.feature.rst (or 700.misc.rst if examples are considered miscellaneous)
    2. The file must:
      • Follow the format <ISSUE_NUMBER>.<TYPE>.rst
      • Contain a user-facing description (focus on what users get, not implementation details)
      • End with a newline character (\n)
    3. Suggested content:
      Added TLS server and client examples demonstrating how to create and connect to TLS-secured libp2p hosts. The examples include both echo and interactive chat modes for the client.
      
    4. Type Selection: Use .feature.rst if this is considered a new feature, or .misc.rst if examples are considered miscellaneous improvements

Issue Reference Verification

  • PR References Issue Add tls #700: ✅ Yes, the PR description mentions "Issue Add tls #700 follow-up"
  • Issue Exists: ✅ Yes, issue Add tls #700 exists (though it's closed and has minimal description)
  • Newsfragment Required: ✅ Yes, a newsfragment using issue Add tls #700 is required

8. Tests and Validation

Test Results Summary

  • Total Tests: 1604
  • Passed: 1599
  • Failed: 1
  • Skipped: 4
  • Warnings: 1
  • Exit Code: 1 (failure)

Test Failures

8.1 test_doc_examples_use_paradigm Failure

  • Test: tests/examples/test_examples_bind_address.py::TestExamplesAddressParadigm::test_doc_examples_use_paradigm
  • Error:
    AssertionError: Documentation example examples/doc-examples/example_tls_client.py should use get_available_interfaces
    
  • Root Cause: The test requires all documentation examples to use the address paradigm functions (get_available_interfaces or get_optimal_binding_address), but the client example doesn't use them because it doesn't bind to addresses (it uses listen_addrs=[]).
  • Impact: This is a test requirement issue. The client example is correct in not using address binding functions, but the test expects all examples to use them.
  • Recommendation:
    • Option 1: Update the test to allow exceptions for client-only examples that don't bind addresses
    • Option 2: Add a minimal usage or comment referencing the address paradigm in the client example
    • Option 3: Discuss with maintainers whether this test requirement should apply to client examples

Linting Results

  • Status:FAILED
  • Errors: 2 line length violations in libp2p/security/tls/certificate.py (lines 275, 281)
  • Auto-fixes: Pre-commit hooks fixed 5 other issues (unused imports, formatting)
  • Exit Code: 1

Type Checking Results

  • Status:PASSED
  • mypy: Passed
  • pyrefly: Passed
  • Exit Code: 0

Documentation Build Results

  • Status:PASSED
  • Build: Successful
  • Doctests: 4 tests, all passed
  • Exit Code: 0

9. Recommendations for Improvement

High Priority

  1. Add Newsfragment: Create newsfragments/700.feature.rst (or 700.misc.rst) with user-facing description of the TLS examples addition.

  2. Fix Linting Errors: Break long lines in libp2p/security/tls/certificate.py lines 275 and 281 to comply with 88-character limit.

  3. Replace Print Statements with Logging:

    • Replace all print() statements in libp2p/security/tls/io.py and libp2p/security/secure_session.py with proper logging module calls
    • Use appropriate log levels (DEBUG for detailed TLS handshake info, INFO for important events)
    • Allow users to control logging via standard Python logging configuration
  4. Address Test Failure:

    • Resolve the test_doc_examples_use_paradigm failure by either:
      • Updating the test to allow client-only examples
      • Adding minimal address paradigm usage/comments to the client example
      • Discussing with maintainers about test requirements

Medium Priority

  1. Document Development Mode:

    • Add clear documentation about the strict_verify parameter and its security implications
    • Consider adding runtime warnings when development mode is active
    • Document when/why development mode should be used
  2. Improve Variable Naming:

    • Rename variables in example_tls_client.py to avoid shadowing (e.g., messageclient_message, formatted_message, etc.)
  3. Security Hardening:

    • Consider making strict_verify=True the default in production code
    • Add explicit opt-in mechanism for development mode
    • Add security warnings in docstrings

Low Priority

  1. Code Cleanup:

    • Remove any remaining unused imports (already handled by linter)
    • Consider consolidating duplicate error handling patterns
  2. Documentation Updates:

    • Consider adding TLS examples to main documentation
    • Update examples README if one exists

10. Questions for the Author

  1. Development Mode Intent: The strict_verify=False mode in certificate verification - is this intended for production use, or only for examples/development? Should this be documented more clearly?

  2. Print Statements: The extensive print() statements in TLS code - are these temporary for debugging, or intended to stay? Should they be converted to proper logging?

  3. Test Requirement: The test_doc_examples_use_paradigm test failure - should client examples that don't bind addresses be exempt from this test, or should the client example be updated to use the address paradigm functions in some way?

  4. Security Defaults: Should strict_verify=True be the default instead of False to ensure secure defaults in production?

  5. Example Scope: Are there plans to add more TLS examples (e.g., mutual TLS, certificate pinning, etc.) or is the current scope sufficient?


11. Overall Assessment

Quality Rating: Needs Work

The PR adds valuable TLS examples that fill an important gap, but several issues need to be addressed before it can be merged:

  • Missing newsfragment (BLOCKER)
  • Linting errors (must fix)
  • Test failure (must resolve)
  • Debug print statements in production code (should fix)
  • Security concerns with development mode (should address)

Security Impact: Medium

The addition of a "development mode" in certificate verification that bypasses security checks is concerning if it could be accidentally used in production. However, the examples themselves are secure and demonstrate proper TLS usage.

Merge Readiness: Needs Fixes

The PR cannot be merged until:

  1. ✅ Newsfragment is added (BLOCKER)
  2. ✅ Linting errors are fixed
  3. ✅ Test failure is resolved
  4. ⚠️ Print statements should be converted to logging (recommended)
  5. ⚠️ Development mode security implications should be documented (recommended)

Confidence: High

The review is comprehensive and all issues are clearly identified with specific file locations and suggestions for resolution.


Review Checklist

  • Checked out PR branch
  • Checked branch sync status and merge conflicts
  • Read PR body and comments
  • Identified related issues (Add tls #700)
  • Read related issue (Add tls #700)
  • Activated virtual environment
  • Created PR review directory
  • Ran make lint with full output capture
  • Ran make typecheck with full output capture
  • Ran make test with full output capture
  • Ran make linux-docs with full output capture
  • Reviewed all output files
  • Understood full context and motivation

Review completed by: AI Assistant
Next steps: Author should address blockers (newsfragment, linting, test failure) before re-review

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.

4 participants