Consider verifying expected PeerId as part of auth upgrades

[thomaseizinger]

That works, but it’s not a very nice behavior. Certificates should be verified during the handshake, as this allows you to abort the connection attempt during TLS handshake.
Verifying the peer ID after the handshake leads to the successful establishment of a connection on the server side, just to have that connection killed 1 RTT later. Even worse, while aborting during the handshake lets the peer know why the handshake failed (as this is communicated by a TLS alert), aborting after handshake completion leaves the peer guessing what went wrong.

Originally posted by @marten-seemann in #2945 (comment)

[mxinden]

If I am not mistaken, this would play nicely with your idea of splitting up the UpgradeXXXbound traits, right @thomaseizinger?

[thomaseizinger]

If I am not mistaken, this would play nicely with your idea of splitting up the UpgradeXXXbound traits, right @thomaseizinger?

It should yes. It would allow us to encode a specific error set where PeerIdMismatch is one variant.

[tthebst]

Is there anything blocking this currently? I would like to help with this.

[thomaseizinger]

Is there anything blocking this currently? I would like to help with this.

Thank you! Nothing is per se blocking this but implementing it requires some bigger changes to our abstractions. At the moment upgrading a connection or stream is generic via the InboundUpgrade and OutboundUpgrade traits. This means that we cannot return a specific error from the "auth upgrade" because the Swarm itself doesn't see a sequence of upgrades where one is the authentication bit but just one big upgrade that either succeeds or fails.

One idea on how to solve this is that we introduce dedicated upgrade traits for:

• unauthenticated -> authenticated
• single connection -> multiplexed connection

Then, the first one can define concrete failure modes such as a peerid mismatch and we can abort the upgrade early.

If you want to have a stab at this, I'd suggest to open a draft PR as soon as possible. Likely, we will then have to split that one up into multiple PRs once we settle on a design but first we need some explorative design work.

What complicates the issue further is that the InboundUpgrade and OutboundUpgrade traits are also used for streams as part of the ConnectionHandler although we are working on moving away from that already: #2863

A final, albeit only marginally, related issue is that I'd actually like to split out this "upgrade" machinery into a dedicated crate, see #3271.

[thomaseizinger]

So I think this is actually much easier than I initially thought.

• We can introduce a new trait: SecurityUpgrade that we implement for noise and tls
• In the Builder::authenticate function, we then force the user to pass an upgrade that implements this trait
• Lastly, we will need to write a separate state machine in replacement of upgrade::apply that calls the SecurityUpgrade trait instead of InboundUpgrade / OutboundUpgrade. I think it is fine to write that using async/await and box it into a trait object

The SecurityUpgrade trait can then have a dedicated function signature that takes in an Option<PeerId> for outbound upgrades. This can be used by security transports such as TLS to directly validate the expected PeerId.

Lastly, we populate this PeerId by checking the address of the ConnectedPoint::Dialer variant for a /p2p postfix and extract the PeerId out of it.

Whilst sounding a bit complicated now, there aren't actually that many moving piece and the change is quite localized.

@tthebst Let me know if you are still up for it with this revised idea :)

[denis2glez]

@thomaseizinger I started addressing this issue in #4864 following your suggestions above, let me know any feedback.