Skip to content

identify: Improve identify message decoding #379

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 10 commits into from
Apr 29, 2025
Merged

identify: Improve identify message decoding #379

merged 10 commits into from
Apr 29, 2025

Conversation

lexnv
Copy link
Collaborator

@lexnv lexnv commented Apr 24, 2025

This PR improves the decoding of the identify messages by checking the following:

  • if provided, the public key must decode properly and point towards the same peer ID
    • on failure the identify message is not propagated to the higher levels
  • empty addresses or addresses corresponding to different peers are removed from the message
  • added extra debug logs to the identify component

Testing Done

cc @paritytech/networking

lexnv added 4 commits April 23, 2025 19:49
@lexnv
identify: Verify provided public key
51ab40b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
identify: Check the provided address ends with the peer ID
c8752c2 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
identify: Ensure the observed address coresponds to us
3d4c886 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv
identify: Add more debug logs
0199557 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv added the enhancement New feature or request label Apr 24, 2025
@lexnv lexnv self-assigned this Apr 24, 2025
dmitry-markin
dmitry-markin approved these changes Apr 25, 2025
View reviewed changes
src/protocol/libp2p/identify.rs
Comment on lines +345 to +357
// The check ensures the provided public key is the same one as the
// one exchanged during the connection establishment.
if let Some(public_key) = &info.public_key {
let public_key = PublicKey::from_protobuf_encoding(&public_key).map_err(|err| {
tracing::debug!(target: LOG_TARGET, ?peer, ?err, "peer identified provided undecodable public key");
err
})?;

if public_key.to_peer_id() != peer {
tracing::debug!(target: LOG_TARGET, ?peer, "peer identified provided invalid public key");
return Err(Error::InvalidData);
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think we need to waste CPU cycles on checking this, as obviously the remote possesses the public key if it is communicating to us over an encrypted noise channel. And it's not our responsibility to check for bugs in the remote implementation of the public key serialization to the Identify message.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had a look at the CPU info reported by substrate_tasks_polling_duration_sum and the dashboards look more or less the same, I expect this will be negligible

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't see a point in checking this if we don't use this info.

lexnv and others added 5 commits April 28, 2025 13:01
@lexnv
identify: Update expect message
f4b0c6b 
Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv @dmitry-markin
Update src/protocol/libp2p/identify.rs
1391075 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv @dmitry-markin
Update src/protocol/libp2p/identify.rs
27a6f3d 
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
@lexnv
Merge branch 'master' into lexnv/identify-checks
291e1e7
@lexnv
Merge branch 'master' into lexnv/identify-checks
b33cb09
lexnv added a commit that referenced this pull request Apr 28, 2025
@lexnv
identify: Improve identify message decoding #379
460bd95 
Squashed commit of the following:

commit b33cb09
Merge: 291e1e7 6ad2161
Author: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date:   Mon Apr 28 17:55:51 2025 +0300

    Merge branch 'master' into lexnv/identify-checks

commit 291e1e7
Merge: 27a6f3d 48d540d
Author: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date:   Mon Apr 28 13:02:58 2025 +0300

    Merge branch 'master' into lexnv/identify-checks

commit 27a6f3d
Author: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date:   Mon Apr 28 13:02:52 2025 +0300

    Update src/protocol/libp2p/identify.rs

    Co-authored-by: Dmitry Markin <dmitry@markin.tech>

commit 1391075
Author: Alexandru Vasile <60601340+lexnv@users.noreply.github.com>
Date:   Mon Apr 28 13:02:34 2025 +0300

    Update src/protocol/libp2p/identify.rs

    Co-authored-by: Dmitry Markin <dmitry@markin.tech>

commit f4b0c6b
Author: Alexandru Vasile <alexandru.vasile@parity.io>
Date:   Mon Apr 28 13:01:01 2025 +0300

    identify: Update expect message

    Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

commit 0199557
Author: Alexandru Vasile <alexandru.vasile@parity.io>
Date:   Thu Apr 24 17:05:37 2025 +0300

    identify: Add more debug logs

    Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

commit 3d4c886
Author: Alexandru Vasile <alexandru.vasile@parity.io>
Date:   Wed Apr 23 20:00:56 2025 +0300

    identify: Ensure the observed address coresponds to us

    Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

commit c8752c2
Author: Alexandru Vasile <alexandru.vasile@parity.io>
Date:   Wed Apr 23 19:53:05 2025 +0300

    identify: Check the provided address ends with the peer ID

    Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

commit 51ab40b
Author: Alexandru Vasile <alexandru.vasile@parity.io>
Date:   Wed Apr 23 19:48:42 2025 +0300

    identify: Verify provided public key

    Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
@lexnv lexnv merged commit 9337fb6 into master Apr 29, 2025
8 checks passed
@lexnv lexnv deleted the lexnv/identify-checks branch April 29, 2025 09:30
This was referenced Apr 29, 2025
Merged
Merged
lexnv added a commit that referenced this pull request May 1, 2025
@lexnv
chore: Release litep2p v0.9.4 (#382)
6d61e2b 
## [0.9.4] - 2025-04-29

This release brings several improvements and fixes to litep2p, advancing
its stability and readiness for production use.

### Performance Improvements

This release addresses an issue where notification protocols failed to
exit on handle drop, lowering CPU usage in scenarios like
minimal-relay-chains from 7% to 0.1%.

### Robustness Improvements

- Kademlia:
- Optimized address store by sorting addresses based on dialing score,
bounding memory consumption and improving efficiency.
- Limited `FIND_NODE` responses to the replication factor, reducing data
stored in the routing table.
- Address store improvements enhance robustness against routing table
alterations.

- Identify Codec:
- Enhanced message decoding to manage malformed or unexpected messages
gracefully.

- Bitswap:
- Introduced a write timeout for sending frames, preventing protocol
hangs or delays.

### Testing and Reliability

- Fuzzing Harness: Added a fuzzing harness by SRLabs to uncover and
resolve potential issues, improving code robustness. Thanks to @R9295
for the contribution!

- Testing Enhancements: Improved notification state machine testing.
Thanks to Dominique (@Imod7) for the contribution!

### Dependency Management

- Updated all dependencies for stable feature flags (default and
"websocket") to their latest versions.

- Reorganized dependencies under specific feature flags, shrinking the
default feature set and avoiding exposure of outdated dependencies from
experimental features.

### Fixed

- notifications: Exit protocols on handle drop to save up CPU of
`minimal-relay-chains`
([#376](#376))
- identify: Improve identify message decoding
([#379](#379))
- crypto/noise: Set timeout limits for the noise handshake
([#373](#373))
- kad: Improve robustness of addresses from the routing table
([#369](#369))
- kad: Bound kademlia messages to the replication factor
([#371](#371))
- codec: Decode smaller payloads for identity to None
([#362](#362))

### Added

- bitswap: Add write timeout for sending frames
([#361](#361))
- notif/tests: check test state
([#360](#360))
- SRLabs: Introduce simple fuzzing harness
([#367](#367))
- SRLabs: Introduce Fuzzing Harness
([#365](#365))

### Changed

- features: Move quic related dependencies under feature flag
([#359](#359))
- tests/substrate: Remove outdated substrate specific conformace testing
([#370](#370))
- ci: Update stable dependencies
([#375](#375))
- build(deps): bump hex-literal from 0.4.1 to 1.0.0
([#381](#381))
- build(deps): bump tokio from 1.44.1 to 1.44.2 in /fuzz/structure-aware
([#378](#378))
- build(deps): bump Swatinem/rust-cache from 2.7.7 to 2.7.8
([#363](#363))
- build(deps): bump tokio from 1.43.0 to 1.43.1
([#368](#368))
- build(deps): bump openssl from 0.10.70 to 0.10.72
([#366](#366))

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
github-merge-queue bot pushed a commit to paritytech/polkadot-sdk that referenced this pull request May 1, 2025
@lexnv @dmitry-markin
networking: Update litep2p network backend to v0.9.4 (#8373)
9869517 
# Litep2p Release

This release brings several improvements and fixes to litep2p, advancing
its stability and readiness for production use.

### Performance Improvements

This release addresses an issue where notification protocols failed to
exit on handle drop, lowering CPU usage in scenarios like
minimal-relay-chains from 7% to 0.1%.

### Robustness Improvements

- Kademlia:
- Optimized address store by sorting addresses based on dialing score,
bounding memory consumption and improving efficiency.
- Limited `FIND_NODE` responses to the replication factor, reducing data
stored in the routing table.
- Address store improvements enhance robustness against routing table
alterations.

- Identify Codec:
- Enhanced message decoding to manage malformed or unexpected messages
gracefully.

- Bitswap:
- Introduced a write timeout for sending frames, preventing protocol
hangs or delays.

### Testing and Reliability

- Fuzzing Harness: Added a fuzzing harness by SRLabs to uncover and
resolve potential issues, improving code robustness. Thanks to @R9295
for the contribution!

- Testing Enhancements: Improved notification state machine testing.
Thanks to Dominique (@Imod7) for the contribution!

### Dependency Management

- Updated all dependencies for stable feature flags (default and
"websocket") to their latest versions.

- Reorganized dependencies under specific feature flags, shrinking the
default feature set and avoiding exposure of outdated dependencies from
experimental features.

### Fixed

- notifications: Exit protocols on handle drop to save up CPU of
`minimal-relay-chains`
([#376](paritytech/litep2p#376))
- identify: Improve identify message decoding
([#379](paritytech/litep2p#379))
- crypto/noise: Set timeout limits for the noise handshake
([#373](paritytech/litep2p#373))
- kad: Improve robustness of addresses from the routing table
([#369](paritytech/litep2p#369))
- kad: Bound kademlia messages to the replication factor
([#371](paritytech/litep2p#371))
- codec: Decode smaller payloads for identity to None
([#362](paritytech/litep2p#362))

### Added

- bitswap: Add write timeout for sending frames
([#361](paritytech/litep2p#361))
- notif/tests: check test state
([#360](paritytech/litep2p#360))
- SRLabs: Introduce simple fuzzing harness
([#367](paritytech/litep2p#367))
- SRLabs: Introduce Fuzzing Harness
([#365](paritytech/litep2p#365))

### Changed

- features: Move quic related dependencies under feature flag
([#359](paritytech/litep2p#359))
- tests/substrate: Remove outdated substrate specific conformace testing
([#370](paritytech/litep2p#370))
- ci: Update stable dependencies
([#375](paritytech/litep2p#375))
- build(deps): bump hex-literal from 0.4.1 to 1.0.0
([#381](paritytech/litep2p#381))
- build(deps): bump tokio from 1.44.1 to 1.44.2 in /fuzz/structure-aware
([#378](paritytech/litep2p#378))
- build(deps): bump Swatinem/rust-cache from 2.7.7 to 2.7.8
([#363](paritytech/litep2p#363))
- build(deps): bump tokio from 1.43.0 to 1.43.1
([#368](paritytech/litep2p#368))
- build(deps): bump openssl from 0.10.70 to 0.10.72
([#366](paritytech/litep2p#366))

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
castillax pushed a commit to paritytech/polkadot-sdk that referenced this pull request May 12, 2025
@lexnv @dmitry-markin @castillax
networking: Update litep2p network backend to v0.9.4 (#8373)
4270c62 
# Litep2p Release

This release brings several improvements and fixes to litep2p, advancing
its stability and readiness for production use.

### Performance Improvements

This release addresses an issue where notification protocols failed to
exit on handle drop, lowering CPU usage in scenarios like
minimal-relay-chains from 7% to 0.1%.

### Robustness Improvements

- Kademlia:
- Optimized address store by sorting addresses based on dialing score,
bounding memory consumption and improving efficiency.
- Limited `FIND_NODE` responses to the replication factor, reducing data
stored in the routing table.
- Address store improvements enhance robustness against routing table
alterations.

- Identify Codec:
- Enhanced message decoding to manage malformed or unexpected messages
gracefully.

- Bitswap:
- Introduced a write timeout for sending frames, preventing protocol
hangs or delays.

### Testing and Reliability

- Fuzzing Harness: Added a fuzzing harness by SRLabs to uncover and
resolve potential issues, improving code robustness. Thanks to @R9295
for the contribution!

- Testing Enhancements: Improved notification state machine testing.
Thanks to Dominique (@Imod7) for the contribution!

### Dependency Management

- Updated all dependencies for stable feature flags (default and
"websocket") to their latest versions.

- Reorganized dependencies under specific feature flags, shrinking the
default feature set and avoiding exposure of outdated dependencies from
experimental features.

### Fixed

- notifications: Exit protocols on handle drop to save up CPU of
`minimal-relay-chains`
([#376](paritytech/litep2p#376))
- identify: Improve identify message decoding
([#379](paritytech/litep2p#379))
- crypto/noise: Set timeout limits for the noise handshake
([#373](paritytech/litep2p#373))
- kad: Improve robustness of addresses from the routing table
([#369](paritytech/litep2p#369))
- kad: Bound kademlia messages to the replication factor
([#371](paritytech/litep2p#371))
- codec: Decode smaller payloads for identity to None
([#362](paritytech/litep2p#362))

### Added

- bitswap: Add write timeout for sending frames
([#361](paritytech/litep2p#361))
- notif/tests: check test state
([#360](paritytech/litep2p#360))
- SRLabs: Introduce simple fuzzing harness
([#367](paritytech/litep2p#367))
- SRLabs: Introduce Fuzzing Harness
([#365](paritytech/litep2p#365))

### Changed

- features: Move quic related dependencies under feature flag
([#359](paritytech/litep2p#359))
- tests/substrate: Remove outdated substrate specific conformace testing
([#370](paritytech/litep2p#370))
- ci: Update stable dependencies
([#375](paritytech/litep2p#375))
- build(deps): bump hex-literal from 0.4.1 to 1.0.0
([#381](paritytech/litep2p#381))
- build(deps): bump tokio from 1.44.1 to 1.44.2 in /fuzz/structure-aware
([#378](paritytech/litep2p#378))
- build(deps): bump Swatinem/rust-cache from 2.7.7 to 2.7.8
([#363](paritytech/litep2p#363))
- build(deps): bump tokio from 1.43.0 to 1.43.1
([#368](paritytech/litep2p#368))
- build(deps): bump openssl from 0.10.70 to 0.10.72
([#366](paritytech/litep2p#366))

---------

Signed-off-by: Alexandru Vasile <alexandru.vasile@parity.io>
Co-authored-by: Dmitry Markin <dmitry@markin.tech>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dmitry-markin dmitry-markin dmitry-markin approved these changes

Assignees

@lexnv lexnv

Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@lexnv @dmitry-markin