feat: verify expected `PeerId` as part of security handshake #4864

denis2glez · 2023-11-14T22:04:45Z

Description

During a handshake, certificates should be verified in order to allow the connection attempt to be aborted during the security handshake (by security protocols such as TLS or Noise). This requires being able to verify the peer ID after the handshake. Otherwise, we will have abnormal behaviors, such as aborting after completing the handshake and leaving the peer not knowing what went wrong.

Fixes: #2946.
Related: #4307.
Related: #4726.
Related: #882.

Tasks

Introduce the InboundSecurityUpgrade/OutboundSecurityUpgrade traits that should be implemented by transports such as Noise or TLS.
Create the Builder::authenticate2 function to accept an upgrade that implements this traits.
Implement a state machine inupgrade::secure that calls the InboundSecurityUpgrade/OutboundSecurityUpgrade traits instead of InboundConnectionUpgrade / OutboundConnectionUpgrade.
Implement the InboundSecurityUpgrade/OutboundSecurityUpgrade traits for Noise and TLS transports, repectively.
Complete the missing parts.
Include unit tests.
Make appropriate changes to the docs.

Notes & open questions

… handshake.

thomaseizinger

Great start!

We'll need to implement the new trait for libp2p-tls and libp2p-noise.

core/src/upgrade.rs

core/src/upgrade/secure.rs

core/src/transport/upgrade.rs

denis2glez · 2023-11-16T07:36:46Z

Thanks for the suggestions!

Introduce `Builder::authenticate2` as an alternative code path given the supplied upgrade implements `SecurityUpgrade` instead of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade`. Inline `secure_inbound` and `secure_outound` functions into `secure`. Fix `PeerId` in the output of `SecurityUpgrade::upgrade_security`. Remove the unnecessary call to `panic`. Co-authored-by: Thomas Eizinger

core/src/transport/upgrade.rs

core/src/upgrade.rs

Use an owned dynamically typed `Future` that is `Send` (i.e. `BoxFuture`).

Introduce `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` instead.

Plus fix doc comments.

Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the TLS transport verifying the expected peer ID on outgoing upgrades.

Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the Noise transport verifying the expected peer ID on outgoing upgrades.

thomaseizinger

That looks great!

I've left some more comments. Can you also please use authenticate2 within the SwarmBuilder in the libp2p crate?

core/src/upgrade.rs

transports/noise/src/lib.rs

transports/tls/src/upgrade.rs

The optional `PeerId` parameter in a security upgrade is never known at this point for an inbound connection.

thomaseizinger

Good progress! I've left a few more comments :)

core/src/transport/upgrade.rs

core/src/upgrade.rs

core/src/upgrade/secure.rs

transports/noise/src/lib.rs

core/src/upgrade.rs

Delegate the implementations of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade` traits to `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` for Noise and TLS.

The `PeerId` coming from the address is the remote `PeerId` and the one from the handshake is the actual `PeerId`.

Introduce the `with_remote_peer_id` constructor in `Config`.

transports/tls/src/upgrade.rs

thomaseizinger · 2023-11-27T05:42:54Z

Can you also please use authenticate2 within the SwarmBuilder in the libp2p crate?

In addition to the above comments, this is also still an open item. So far, we are not using those APIs anywhere, meaning we don't actually get to benefit from the new handshake. You'll have to find the usage sites of the current authenticate function within the SwarmBuilder and use authenticate2 instead. Your IDE should be of great assistance here or alternatively, you can mark the old as deprecated and run the compiler once.

Here is one of the usages sites:

rust-libp2p/libp2p/src/builder/phase/tcp.rs

Lines 84 to 86 in d851d1b

    
           .authenticate( 
        
               security_upgrade.into_security_upgrade(&self.keypair)?, 
        
           )

The `PeerId` coming from the address is the expected `PeerId` and the one from the handshake is the actual `PeerId`.

denis2glez · 2023-11-27T10:00:23Z

Can you also please use authenticate2 within the SwarmBuilder in the libp2p crate?

In addition to the above comments, this is also still an open item. So far, we are not using those APIs anywhere, meaning we don't actually get to benefit from the new handshake. You'll have to find the usage sites of the current authenticate function within the SwarmBuilder and use authenticate2 instead. Your IDE should be of great assistance here or alternatively, you can mark the old as deprecated and run the compiler once.

Here is one of the usages sites:

rust-libp2p/libp2p/src/builder/phase/tcp.rs

Lines 84 to 86 in d851d1b

.authenticate(

security_upgrade.into_security_upgrade(&self.keypair)?,

)

Thanks, I was already working on this.

Explicitly call `make_server_config` and `make_client_config` as part of the upgrade.

thomaseizinger · 2023-11-28T00:27:21Z

Thanks, I was already working on this.

Awesome! Let me know when you want another review! :)

feat: MVP verification of the expected PeerId as part of a protocol…

6748e17

… handshake.

denis2glez changed the title ~~feat: Verification of the expected PeerId as part of a protocol handshake.~~ feat: Verification of expected PeerId during protocol handshake. Nov 15, 2023

denis2glez mentioned this pull request Nov 15, 2023

Consider verifying expected PeerId as part of auth upgrades #2946

Open

chore: fix a typo in the doc comments

dbedc1d

thomaseizinger reviewed Nov 15, 2023

View reviewed changes

denis2glez added 2 commits November 16, 2023 13:18

docs: improve comments related to Builder::authenticate2

af48270

thomaseizinger reviewed Nov 17, 2023

View reviewed changes

core/src/transport/upgrade.rs Show resolved Hide resolved

thomaseizinger reviewed Nov 17, 2023

View reviewed changes

core/src/upgrade.rs Outdated Show resolved Hide resolved

denis2glez added 3 commits November 17, 2023 12:10

fix: add Send constraint to Builder::authenticate2 parameters

31c7e61

Use an owned dynamically typed `Future` that is `Send` (i.e. `BoxFuture`).

refactor: split SecurityUpgrade into two alternative traits

5c5b728

Introduce `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` instead.

docs: mark items related to Builder::authenticate2 as #[doc(hidden)]

b686801

Plus fix doc comments.

denis2glez force-pushed the verify-peer-id branch from a6905ba to b686801 Compare November 17, 2023 14:57

denis2glez added 2 commits November 17, 2023 19:12

feat: impl InboundSecurityUpgrade/OutboundSecurityUpgrade for TLS

c0fad3a

Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the TLS transport verifying the expected peer ID on outgoing upgrades.

feat: impl InboundSecurityUpgrade/OutboundSecurityUpgrade for Noise

399666b

Implement the `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` traits for the Noise transport verifying the expected peer ID on outgoing upgrades.

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

core/src/upgrade.rs Outdated Show resolved Hide resolved

transports/noise/src/lib.rs Outdated Show resolved Hide resolved

transports/tls/src/upgrade.rs Outdated Show resolved Hide resolved

denis2glez mentioned this pull request Nov 19, 2023

Generalize the handshake implementation. denis2glez/p2p-handshake#12

Closed

denis2glez added 2 commits November 19, 2023 22:57

chore: minor doc comments fixes

71e0ac8

refactor: remove the PeerId parameter in InboundSecurityUpgrade

5500663

The optional `PeerId` parameter in a security upgrade is never known at this point for an inbound connection.

thomaseizinger reviewed Nov 19, 2023

View reviewed changes

thomaseizinger changed the title ~~feat: Verification of expected PeerId during protocol handshake.~~ feat: verify expected PeerId as part of security handshake Nov 19, 2023

denis2glez added 5 commits November 22, 2023 17:28

docs: fix several issues in the comments

a726083

refactor: delegate upgrade_inbound and upgrade_outbound methods

514b220

Delegate the implementations of `InboundConnectionUpgrade`/`OutboundConnectionUpgrade` traits to `InboundSecurityUpgrade`/`OutboundSecurityUpgrade` for Noise and TLS.

fix: references to actual and remote PeerId respectively

66df049

The `PeerId` coming from the address is the remote `PeerId` and the one from the handshake is the actual `PeerId`.

chore: keep grouping of module and use declarations

d8440e7

fix: make client and server config ad-hoc in secure_outbound method

c386182

Introduce the `with_remote_peer_id` constructor in `Config`.

denis2glez force-pushed the verify-peer-id branch from c2bf72d to c386182 Compare November 24, 2023 16:35

thomaseizinger reviewed Nov 27, 2023

View reviewed changes

transports/tls/src/upgrade.rs Outdated Show resolved Hide resolved

fix: use the wording of actual and expected PeerId

3eb5f1f

The `PeerId` coming from the address is the expected `PeerId` and the one from the handshake is the actual `PeerId`.

refactor: store the identity::Keypair within Config

2ede11c

Explicitly call `make_server_config` and `make_client_config` as part of the upgrade.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: verify expected `PeerId` as part of security handshake #4864

feat: verify expected `PeerId` as part of security handshake #4864

denis2glez commented Nov 14, 2023 •

edited by thomaseizinger

Loading

thomaseizinger left a comment

denis2glez commented Nov 16, 2023

thomaseizinger left a comment

thomaseizinger left a comment

thomaseizinger commented Nov 27, 2023 •

edited

Loading

denis2glez commented Nov 27, 2023

thomaseizinger commented Nov 28, 2023

feat: verify expected PeerId as part of security handshake #4864

Are you sure you want to change the base?

feat: verify expected PeerId as part of security handshake #4864

Conversation

denis2glez commented Nov 14, 2023 • edited by thomaseizinger Loading

Description

Tasks

Notes & open questions

thomaseizinger left a comment

Choose a reason for hiding this comment

denis2glez commented Nov 16, 2023

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger left a comment

Choose a reason for hiding this comment

thomaseizinger commented Nov 27, 2023 • edited Loading

denis2glez commented Nov 27, 2023

thomaseizinger commented Nov 28, 2023

feat: verify expected `PeerId` as part of security handshake #4864

feat: verify expected `PeerId` as part of security handshake #4864

denis2glez commented Nov 14, 2023 •

edited by thomaseizinger

Loading

thomaseizinger commented Nov 27, 2023 •

edited

Loading