Skip to content
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.

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

Unexpected behaviour when a circuit relay server is also relayed #1690

Closed
marcus-pousette opened this issue Apr 15, 2023 · 2 comments · Fixed by #1732
Closed

Unexpected behaviour when a circuit relay server is also relayed #1690

marcus-pousette opened this issue Apr 15, 2023 · 2 comments · Fixed by #1732
Labels
kind/bug A bug in existing code (including security flaws) need/analysis Needs further analysis before proceeding

Comments

@marcus-pousette
Copy link
Contributor

marcus-pousette commented Apr 15, 2023

  • Version:
    0.44.0

  • Subsystem:
    Circuit Relay

Severity:

Medium

Description:

I have three nodes running locally (A,B,C) each as a circuit relay server and with circuit transport where discoverRelays: 1

First A is dials and connects B.

Both A and B will obtain an relayed address in .getMultiaddrs() (expected)

Then, when B directly dials C address

Unexpectedly (?), I am receiving "onConnect" events on node A from C after this when observering changes

this.topology = createTopology({
	onConnect: ...
});

The incoming connection event on A is as follows.

remotePeer: 12D3KooWDv7TfQ5Ct8jfQ98Vcs3mNSJYXABnKJvHPe4M1ZiGmqoC   (Node C)
remoteAddr: /ip4/127.0.0.1/tcp/61655

Then, additionally, C recieves an incoming connection from A

remotePeer: 12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr  (Node  A)
remoteAddr: /ip4/127.0.0.1/tcp/61623/ws/p2p/12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr

A

"/ip4/127.0.0.1/tcp/61623/ws/p2p/12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr"
"/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz/p2p-circuit/p2p/12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr"```

B

"/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz"
"/ip4/127.0.0.1/tcp/61623/ws/p2p/12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr/p2p-circuit/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz"

C

"/ip4/127.0.0.1/tcp/61625/ws/p2p/12D3KooWDv7TfQ5Ct8jfQ98Vcs3mNSJYXABnKJvHPe4M1ZiGmqoC"
"/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz/p2p-circuit/p2p/12D3KooWDv7TfQ5Ct8jfQ98Vcs3mNSJYXABnKJvHPe4M1ZiGmqoC"

I am not expecting this, and wonder what part of the configuration is wrong?
It seems like if you are dialing a node, are you automatically also dial all relayed connections that starts with the same address?
In this case, if C dials B (/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz)

It seems like both

/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz

and

/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz/p2p-circuit/p2p/12D3KooWR9JDmVK72ZZNEARjArap8SnFyV2v5Exq1jzANSKdDyrr

would receive connection events?

@marcus-pousette marcus-pousette added the need/triage Needs initial labeling and prioritization label Apr 15, 2023
@marcus-pousette
Copy link
Contributor Author

marcus-pousette commented Apr 15, 2023

Doing some digging, I start to understand a little more about this:

On reservation, the server takes all its addresses and forwards them to the node that wants to reserve.

for (const relayAddr of this.addressManager.getAddresses()) {

According to the specification, the addrs field contains all the public relay addrs. For my case, since the server node is also a client reserved in another server, it will the "addrs" field would look something like

"/ip4/127.0.0.1/tcp/61625/ws/p2p/12D3KooWDv7TfQ5Ct8jfQ98Vcs3mNSJYXABnKJvHPe4M1ZiGmqoC"
"/ip4/127.0.0.1/tcp/61622/ws/p2p/12D3KooWHw7m2xHFQjJJH3ozx59Z6AWT7yWyMHgfj1x3fELa8Baz/p2p-circuit/p2p/12D3KooWDv7TfQ5Ct8jfQ98Vcs3mNSJYXABnKJvHPe4M1ZiGmqoC"

Then the "client" node takes all these addresses and starts to listen on them

await this.transportManager.listen(

Which leads to an connection to be opened to every address that the server sent.

const relayConn = await this.connectionManager.openConnection(ma)

This means, that if B and C connects, C will reserve a slot in B, and obtain all addresses of B in the process. One of these addresses is the relayed address through A, hence C will also connect to A.

Putting a breakpoint at

await this.transportManager.listen(

I see the addresses that are beeing sent to this.transportManager.listen

Is

'/ip4/127.0.0.1/tcp/60025/ws/p2p/12D3KooWQrMumZzLpF6fgmt8vdaCf7dCqKVXuFsr5jrjH5dzRpbX/p2p-circuit'
'/ip4/127.0.0.1/tcp/60024/ws/p2p/12D3KooWBj1WJNmLDwEG7Y6cL3xua367ZsXc5zCuiw3BsRcve6j8/p2p-circuit/p2p/12D3KooWQrMumZzLpF6fgmt8vdaCf7dCqKVXuFsr5jrjH5dzRpbX/p2p-circuit'

The second one looks not quite right (?). I assume here that the issue is that we should not return relayed addresses for the RESERVE response from the relay server.

A fix, perhaps server side to filter out relayed addresses?

- for (const relayAddr of this.addressManager.getAddresses()) {
+ for (const relayAddr of this.addressManager.getAddresses().filter(addr => !addrs.protoCodes().includes(290)) {

for (const relayAddr of this.addressManager.getAddresses()) {

but according to the specification "all" addresses should be returned.

Another solution is to do the filtering on the client side.

@marcus-pousette marcus-pousette changed the title How to prevent dialing of relayed connections? Unexpected behaviour when a circuit relay server is also relayed Apr 15, 2023
@achingbrain achingbrain added kind/bug A bug in existing code (including security flaws) need/analysis Needs further analysis before proceeding and removed need/triage Needs initial labeling and prioritization labels May 2, 2023
@achingbrain
Copy link
Member

This needs some analysis - the spec disallows dialling relays through relays and there are tests for this but perhaps a case has been missed.

achingbrain added a commit that referenced this issue May 5, 2023
…nts (#1732)

Fixes a bug whereby receiving a circuit relay address would cause js-libp2p to try to listen on the non-relayed parts of the address.  For example if a relay listening on a WebTransport multiaddr was dialled, js-libp2p would then try to listen on the same address due to the relay reservation store calling `transportManager.listen` with the full address.  This would then fail and the relaying would not succeed.

The fix is to call `transportManager.listen` with a multiaddr that just has the peer id and the `p2p-circuit` segments so only the relay transport tries to use the address.

For safety if listening on an address or writing to the peer store fails while creating a relay reservation, remove the reservation from the reservation store to allow us to try again.

Also updates logging to make what is going on a bit clearer.

Closes #1690

---------

Co-authored-by: Chad Nehemiah <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug A bug in existing code (including security flaws) need/analysis Needs further analysis before proceeding
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants