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

feat: provide a way to define advertised addresses #1797

Closed
filoozom opened this issue Jun 13, 2023 · 13 comments · Fixed by #2122 or #2141
Closed

feat: provide a way to define advertised addresses #1797

filoozom opened this issue Jun 13, 2023 · 13 comments · Fixed by #2122 or #2141
Assignees
Labels
enhancement New feature or request

Comments

@filoozom
Copy link

filoozom commented Jun 13, 2023

Problem

I'm running nwaku nodes behind an SSL gateway that manages all the encryption and certificates. Basically, the nwaku node listens to WebSocket requests without any encryption, and I add the right multi address via --ext-multiaddr.

However, there's no way to disable the default /ip4/.../tcp/8000/ws/p2p/... address, meaning that js-waku will try to connect to it for example.

I end up with the following advertised addresses:

- /ip4/.../tcp/60001/p2p/peerId
- /dns4/.../tcp/443/wss/p2p/peerId (added with `--ext-multiaddr`)
- /ip4/.../tcp/8000/ws/p2p/peerId (invalid, port not exposed, should be removed)

Suggested solution

Add a --advertised-multiaddr flag that makes it possible to override all advertised addresses. In my example, I'd need to remove --ext-multiaddr and to add the following flags:

--ext-multiaddr:/dns4/fqdn/tcp/443/wss
--ext-multiaddr:/ip4/ip/tcp/60001

I could also then presumably remove --nat:extip:ip.

Alternatives considered

An alternative that at least doesn't really break this is to also advertise the WebSocket address and forward that on the reverse proxy, so that /ip4/.../tcp/8000/ws/p2p/... actually works. But that's not what I want.

An alternative solution to implement could be not to add /ip4/.../tcp/8000/ws/p2p/... by default when enabling WebSocket without making it much more flexible.

Additional context

There are many reasons why one could want this:

  • Not requiring heavy changes to an existing infrastructure (Nomad, Kubernetes, Docker Swarm, ...)
  • If I trust my infra's SSL capabilities, and see no need to involve some additional SSL "dependencies" that might be less secure or slower
  • If I want my certificates to be managed automatically by my existing infra (Let's Encrypt integrations for example)
  • If I don't want the entire service to restart just to rotate keys
  • It might not be possible to do this without a reverse proxy (DAppNode?)

Being able to precisely confuse the advertised addresses also makes everything more flexible. For example, I can have my nwaku node listen on port 60000 in a Docker container, VM or whatever, and NAT that to any IP:PORT combination without having to listen on that specific port and setting --nat:extip:IP

Acceptance criteria

I can configure exactly which multi addresses are being advertised by my node.

@Ivansete-status
Copy link
Collaborator

We may need to adjust around the next:

announcedAddresses.add(hostAddress) # We always have at least a bind address for the host

@filoozom
Copy link
Author

I guess you can just reset that entire array and set it to the array passed in arguments if there is one? I don't know nim at all, but something like:

if advertisedMultiaddress.isSome():
  announcedAddresses = advertisedMultiaddress

after line 121?

@rymnc
Copy link
Contributor

rymnc commented Jun 13, 2023

Related to #1509, #1475

@Ivansete-status Ivansete-status moved this to Triage in Waku Jun 13, 2023
@SionoiS SionoiS moved this from Triage to To Do in Waku Jun 13, 2023
@Ivansete-status
Copy link
Collaborator

Ivansete-status commented Jun 13, 2023

I guess you can just reset that entire array and set it to the array passed in arguments if there is one? I don't know nim at all, but something like:

if advertisedMultiaddress.isSome():
  announcedAddresses = advertisedMultiaddress

after line 121?

Hey @filoozom !

Yes sth like that but we need to review it carefully so that other users are not impacted.

We have been discussing the different options:

  1. Change the behavior of --ext-multiaddr and make it advertise only the addresses provided by this parameter, in which case we would need to properly inform the users about the change, and make tests from the feature branch.

  2. Add a new parameter, --force-advertised-multiaddr, or sth like that, in order to give the possibility to force the advertised addresses.

Personally, I think the best long-term option is to adapt the --ext-multiaddr and then make it clear to the users that, in case this parameter is used, all the needed addresses should be explicitly given, including the default ones. ( e.g.: /ip4/0.0.0.0/tcp/60001, /ip4/0.0.0.0/tcp/8000/ws .)

@vpavlin , @SionoiS , @rymnc , @alrevuelta - Feel free to correct me if I misunderstood or missed anything.

@fryorcraken
Copy link
Collaborator

@filoozom just to confirm. js-waku/js-libp2p should probably (I haven't checked but I know rust-libp2p does that) try to connect to the various multiaddrs available for a peer. Isn't it happening? What behaviour do you witness?

@filoozom
Copy link
Author

@fryorcraken Yes, js-waku does try to connect to all of them and thus actually works (it's not an issue with js-waku), I just want to remove that advertised address as it's just useless noise. It's not a blocker right now though.

@fryorcraken fryorcraken added the enhancement New feature or request label Oct 4, 2023
@fryorcraken
Copy link
Collaborator

Finally wrapped my around this issue after discussing with @mfw78.

I think it would make sense to correct this so that nwaku does not advertise useless ws address.
Not sure what happens in the ENR as there is limited space.

@gabrielmer gabrielmer self-assigned this Oct 4, 2023
@fryorcraken
Copy link
Collaborator

Note that advertising a ws address only makes sense when doing local development as a browser will only allow connection to a ws if:

  • page is http: which nowadays only happen for local devl
  • page is 127.0.0.1: again, local dev.

While a generalized solution may be useful, one that only enables to discard ws could be good enough.

@gabrielmer
Copy link
Contributor

The code that publishes the ws address is the following:

nwaku/waku/node/config.nim

Lines 125 to 128 in f05528d

if wsExtAddress.isSome():
announcedAddresses.add(wsExtAddress.get())
elif wsHostAddress.isSome():
announcedAddresses.add(wsHostAddress.get())

Notice that if we have a wsExtAddress, then wsHostAddress won't be published.
In addition to that, they are only published if ws or wss are enabled.

The variables wsExtAddress and wsHostAddress are also used for the wss address in case it's enabled, so even if we don't use ws, we would see the same issue of an unwanted address being published with wss.

So wsHostAddress is only published if we enable ws or wss and at the same time, we think that we don't have an external address for it.
In the case provided in this issue, it seems that the code notices that ws or wss are enabled but no external address is provided (when it actually is).

The solution that comes to mind is to correct our assumption that we don't have an external ws/wss address when we actually do.
In other words, check for the addresses passed with the flag --ext-multiaddr, and in case we have a ws or wss address in it, then flag it and stop behaving as if we didn't have one (therefore stop publishing the host's ws/wss address).

@gabrielmer
Copy link
Contributor

Weekly Update

  • achieved: went over the code and found the root cause of the issue and a preliminary solution
  • next: finish discussing the approach to the solution and implement it

@gabrielmer
Copy link
Contributor

Weekly Update

  • achieved: implemented solution and raised PR
  • next: get feedback, implement suggested improvements and close

@github-project-automation github-project-automation bot moved this from In Progress to Done in Waku Oct 17, 2023
@vpavlin vpavlin reopened this Oct 19, 2023
@vpavlin
Copy link
Member

vpavlin commented Oct 19, 2023

The PR #2122 did not actually solve the original issue.

The proposal is then to add --ext-multiaddr-only=true flag which make wakunode2 to only use --ext-multiaddr content in announcedAddresses

We'll come up with the PR and @filoozom can test the PR image to see if it solves the issue:) Then we can release v0.21.1

@gabrielmer
Copy link
Contributor

Weekly Update

  • achieved: merged PR with initial fix. Implemented and raised PR for the --ext-multiaddr-only CLI flag
  • next: get PR reviewed, implement feedback and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
6 participants