Update identify_push.py

[scacaca]

add checking peer_id in indentify

[seetadev]

@scacaca : Great, thank you so much for sharing this PR. CCing @acul71, who originally worked on adding identify push module in py-libp2p with us.

[acul71]

Thanks @scacaca for this PR. can you add a newsfragment doc ?
take a look at newsfragments directory

[acul71]

PR Review: #959 - Update identify_push.py

This review covers the changes made to the identify_push.py file and provides recommendations for future improvements, particularly regarding configurable TTL values.

Code Review Summary

✅ Positive Changes

1. Security Enhancement: The PR adds crucial peer ID validation to prevent forged peer records
2. Logging Improvements: Better error logging and warning messages for debugging
3. Code Quality: Improved error handling and exception management

🔍 Areas for Improvement

1. Line Length Issues: Fixed linting errors related to line length (88 character limit)
2. Code Documentation: Could benefit from more detailed docstrings
3. Test Coverage: Missing tests for the new validation logic

📋 Detailed Analysis

Security Fix Implementation

# Cross-check peer-id consistency
if str(record.peer_id) != str(peer_id):
    logger.warning(
        "SignedPeerRecord peer-id mismatch: record=%s, sender=%s. "
        "Ignoring.",
        record.peer_id, peer_id,
    )
    return

Assessment: ✅ Excellent security improvement

• Prevents peer ID spoofing attacks
• Clear logging for debugging
• Proper error handling

Error Handling

try:
    # ... existing code ...
except Exception as e:
    logger.error(
        "Error updating the certified addr book for peer %s: %s", peer_id, e
    )

Assessment: ✅ Good error handling

• Comprehensive exception catching
• Informative error messages
• Graceful failure handling

Recommendations for Future PRs

🚨 Critical Issue: Hardcoded TTL Values

There's a fundamental architectural issue with hardcoded TTL values throughout the codebase. For detailed discussion and implementation recommendations, see:

GitHub Discussion #967: Configurable TTL Values

Action Items for Developer

Immediate (PR #959)

1. ✅ Security fix implemented - Peer ID validation added
2. ✅ Linting errors fixed - Line length issues resolved
3. 🔄 Add tests for the new validation logic
4. 🔄 Update documentation for the new security feature
5. 🔄 Add newsfragment - Create a newsfragment file for release notes

Newsfragment Requirements

Based on the newsfragments README, you need to create a newsfragment file:

File name: 959.bugfix.rst (since this is a security bugfix)
Location: newsfragments/959.bugfix.rst
Content example:

Added peer ID validation in identify_push protocol to prevent forged peer records.

This security enhancement ensures that the peer ID in signed peer records matches 
the sender's peer ID, preventing peer ID spoofing attacks.

Important: Ensure the file ends with a newline character (\n) to pass GitHub tox linting checks.

Future PRs (Recommended)

1. Address TTL Configuration Issue
   • See GitHub Discussion #967 for detailed implementation plan
   • Replace hardcoded TTL values with configurable system
   • Ensure Trio compatibility

Conclusion

PR #959 successfully addresses a critical security issue by adding peer ID validation to the identify_push protocol. The implementation is solid and follows good practices.

However, the broader issue of hardcoded TTL values throughout the codebase needs to be addressed in future PRs. See GitHub Discussion #967 for detailed recommendations on implementing a configurable TTL system.

References

• GitHub Discussion #967: Configurable TTL Values
• Go libp2p TTL Implementation
• Trio Documentation
• libp2p TLS Specification

[acul71]

@seetadev @sumanjeet0012 the failing test is part of:
#954

=================================== FAILURES ===================================
____________________ test_reissue_when_listen_addrs_change _____________________
[gw0] linux -- Python 3.11.13 /home/runner/work/py-libp2p/py-libp2p/.tox/py3.11-core/bin/python
  + Exception Group Traceback (most recent call last):
  |   File "/home/runner/work/py-libp2p/py-libp2p/.tox/py3.11-core/lib/python3.11/site-packages/trio/_core/_run.py", line 1125, in __aexit__
  |     raise combined_error_from_nursery
  | ExceptionGroup: Exceptions from Trio nursery (1 sub-exception)
  +-+---------------- 1 ----------------
    | Traceback (most recent call last):
    |   File "/home/runner/work/py-libp2p/py-libp2p/.tox/py3.11-core/lib/python3.11/site-packages/pytest_trio/plugin.py", line 195, in _fixture_manager
    |     yield nursery_fixture
    |   File "/home/runner/work/py-libp2p/py-libp2p/.tox/py3.11-core/lib/python3.11/site-packages/pytest_trio/plugin.py", line 250, in run
    |     await self._func(**resolved_kwargs)
    |   File "/home/runner/work/py-libp2p/py-libp2p/tests/core/kad_dht/test_kad_dht.py", line 432, in test_reissue_when_listen_addrs_change
    |     assert isinstance(env0, Envelope)
    | AssertionError: assert False
    |  +  where False = isinstance(None, Envelope)
    +------------------------------------
=============================== warnings summary ===============================
tests/core/kad_dht/test_kad_dht.py:125
tests/core/kad_dht/test_kad_dht.py:125
  /home/runner/work/py-libp2p/py-libp2p/tests/core/kad_dht/test_kad_dht.py:125: PytestUnknownMarkWarning: Unknown pytest.mark.flaky - is this a typo?  You can register custom marks to avoid this warning - for details, see https://docs.pytest.org/en/stable/how-to/mark.html
    @pytest.mark.flaky(reruns=3, reruns_delay=1)

tests/core/stream_muxer/test_muxer_multistream.py::test_new_conn_passes_timeout_to_multistream_client
  /home/runner/work/py-libp2p/py-libp2p/tests/core/stream_muxer/test_muxer_multistream.py:68: RuntimeWarning: coroutine 'AsyncMockMixin._execute_mock_call' was never awaited
    muxer.multistream_client.select_one_of(
  Enable tracemalloc to get traceback where the object was allocated.
  See https://docs.pytest.org/en/stable/how-to/capture-warnings.html#resource-warnings for more info.

-- Docs: https://docs.pytest.org/en/stable/how-to/capture-warnings.html
============================= slowest 50 durations =============================```

[acul71]

Hi @scacaca what's the status of this PR? you need help ?