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: flag to force advertisement of wss #1500

Closed
dao opened this issue Jan 19, 2023 · 9 comments
Closed

feat: flag to force advertisement of wss #1500

dao opened this issue Jan 19, 2023 · 9 comments

Comments

@dao
Copy link

dao commented Jan 19, 2023

Problem

We use nginx to handle ssl and certs and proxy wss to nwaku ws. This works and is fine until nwaku advertises itself with ws address rather than wss. Makes sense as it does not know it is being proxied to wss. But it breaks things after a while.

Suggested solution

Add flag to force nwaku to advertise itself with wss rather than ws maddr.

Alternatives considered

Could completely reconfigure docker stack to renew certbot certs and put them where nwaku can reach them but I'd rather not.

Additional context

Nginx is very very good at reverse proxy and there are many reasons to include it in a stack. Allowing something else to handle ssl and letting nwaku know it is being handled is a good idea.

Acceptance criteria

Flag set to true makes nwaku advertise itself with wss maddr rather than ws

If websockets aren't enabled at all it should error on start "cannot force wss if websockets aren't enabled" or similar

If native secure websockets are enabled, error on start "secure websockets are already enabled. Do not use this flag."

The flag does not effect the dns/ip address portion being advertised; just switch ws for wss.

@fryorcraken
Copy link
Collaborator

I don't agree that it should be just about forcing wss instead of ws. I think it should be about setting the network part of the full multiaddr using to advertised as it would also contains the fqdn that needs to be match the SSL cert used by nginx.

ie, something like

--public-multiaddr=/dns4/this.is.an.example/tcp/443/wss

nwaku can then feel the p2p part with the peer id.

It should be possible to repeat the argument so that it could also have:

---public-multiaddr=/ip4/1.2.3.4/tcp/8888

To solve a case where nwaku is ran in local lan that does not support uPNP / there is a custom router setup.

Also, this is relevant for the usage of nwaku in DappNode as DappNode handles SSL+DyDNS for the user. Cc @mfw78

@mfw78
Copy link

mfw78 commented Jan 19, 2023

Also, this is relevant for the usage of nwaku in DappNode as DappNode handles SSL+DyDNS for the user. Cc @mfw78

This is certainly relevant for the dappnode setup. FYI, this is related to issue/duplicate of #1475.

@jm-clius
Copy link
Contributor

Thanks for this issue and, yes, as @mfw78 has pointed out, there is already a duplicate issue for this. :)

@dao
Copy link
Author

dao commented Jan 19, 2023

To solve a case where nwaku is ran in local lan that does not support uPNP / there is a custom router setup.

I don't think I understand this part - I can already specify domain name, and in doing so I believe I'm telling nwaku to advertise that, and hence do not want to have to specify it again in --public-multiaddr. IP address in this case seems irrelevant, no? If I don't specify domain, IP is advertised. If I do specify domain, domain is used.

I will close when I better understand the advantage of --public-multiaddr vs eg --force-wss

@fryorcraken
Copy link
Collaborator

I can already specify domain name

Ah right. I'll leave it to the dev to decide how to handle this the best. I was suggested to have one argument to handle them all but this may not make sense.

@jm-clius
Copy link
Contributor

I think the design may not be a feature purely for wss, but a general way to add advertised multiaddrs to the discoverable ENR. For example, I can imagine circuit-relay addresses be included in a similar way. Since there is no standard mapping of any type of multiaddr to ENR, this will allow users some flexibility to add custom multiaddrs.

@jm-clius
Copy link
Contributor

Note, parallel discussion on which types of addresses should/should not be added to multiaddrs field: #1491 (comment)

@jm-clius jm-clius moved this to Todo in Waku Jan 23, 2023
@jm-clius jm-clius added this to the Release 0.15.0 milestone Jan 23, 2023
@rymnc
Copy link
Contributor

rymnc commented Feb 7, 2023

This issue is addressed in #1512

@rymnc
Copy link
Contributor

rymnc commented Feb 7, 2023

@jm-clius can we close this issue as #1512 has been merged?

@jm-clius jm-clius closed this as completed Feb 7, 2023
@github-project-automation github-project-automation bot moved this from Todo to Done in Waku Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

No branches or pull requests

5 participants