search

openwrt / luci

LuCI - OpenWrt Configuration Interface
Apache License 2.0
6.34k stars 2.53k forks source link

luci-app-wireguard: Missing option to configure peer address #6050

Open manuelsteiner opened 2 years ago

manuelsteiner commented 2 years ago

The Wireguard peer configuration section is missing the address field. I am not aware of configurations omitting the address parameter. It would be handy to set that in the peer configuration so the exported configuration as well as QR code will automatically include the parameter.

At least on Android, the Wireguard app will complain if no address is set and one has to be entered manually after scanning the QR code from the OpenWrt peer config generation page.

In case this is desired behaviour (i.e. to export configs to peers without an address and manually set it on the respective devices), please consider this issue void.

Steps to reproduce:

  1. Go to: Network → Interfaces → Wireguard → Edit → Peers tab → Edit a peer

Actual behavior:

  1. The option to set the address parameter is not present
  2. As a result, the generated peer config does not contain any address parameter

Expected behavior:

  1. An option to set the address parameter for a peer in the peer configuration settings
  2. The address parameter is included in the generated peer configs (text and QR code)

Additional Information:

DISTRIB_ID='OpenWrt'
DISTRIB_RELEASE='22.03.1'
DISTRIB_REVISION='r19777-2853b6d652'
DISTRIB_TARGET='rockchip/armv8'
DISTRIB_ARCH='aarch64_generic'
DISTRIB_DESCRIPTION='OpenWrt 22.03.1 r19777-2853b6d652'
DISTRIB_TAINTS=''
castillofrancodamian commented 2 years ago

I think the option is Allowed IPs in the peer settings.

manuelsteiner commented 2 years ago

Hm but that generates the parameter AllowedIps for the Peer section and not Address for the Interface section. So that setting would indicate which packet sources are accepted from the connected endpoint. Which is how it is supposed to be used and apparently mapped correctly by the Luci interface.

castillofrancodamian commented 2 years ago

You have to enter an IP in the Allowed IPs section. For example 192.168.34.2/32.

manuelsteiner commented 2 years ago

Well but what would I do if I want to use Allowed IPs for its intended purpose? Say I want to configure the peer with Address 10.0.0.2/32 in the Interface section and want to set AllowedIps 192.168.0.0/24 so the device can receive packets from that specific range?

In that case the generated config and QR Code will contain the AllowedIps parameter, as to its intended usage. Still, there is no address parameter for the Interface section and I need to configure that manually on the target device, for which the configuration is used. Otherwise (at least on Android), it won't even try to establish a connection.

castillofrancodamian commented 2 years ago

Take a screenshot.

manuelsteiner commented 2 years ago

Ok,

here are two scenarios. The screenshot QR codes are obfuscated because the endpoint contains a public IP and scanning the QR code would set that as endpoint.

So in summary, there is no way to enter an IP address in the peer config section in Luci in order for it to show up as Address parameter in the Interface section of the exported configuration.

I hope this makes my issue clear now?

castillofrancodamian commented 2 years ago

To configure an IP address on the interface, you must enter the corresponding IP in IP Addresses. That is in case the device in question is a "client".

jow- commented 2 years ago

The reason why support for that was not implemented is that it does not appear to be an official option according to man 8 wg. We could add it - but what are the requirements? Is it a single address or can it be multiple (IPv4 + IPv6)? Must the addresses lie within the subnets of the router side wireguard interface addresses? Shall we enforce uniqueness among all peers or allow duplicates? Shall we automatically allocate a new IP when adding a peer and prefill the field or leave it empty? What if the user changes the subnets (addresses) of the wireguard interface, shall we clear out all peer addresses or keep them?

manuelsteiner commented 2 years ago

Ah, thank you for clarifying. That at least makes sense. So i guess the current path to success (at least on Android) is to scan the QR code and add the IP Address parameter in the app itself.

Zburatorul commented 1 year ago

So i guess the current path to success (at least on Android) is to scan the QR code and add the IP Address parameter in the app itself.

imo, that is poor user experience because it requires whoever is doing the scanning to magically come up with an IP that's not already in use. That is why this is best handled by Luci. It could at least check whatever [Interface] Address is manually assigned to the given peer against the other peers.

jow- commented 1 year ago

imo, that is poor user experience because it requires whoever is doing the scanning to magically come up with an IP that's not already in use. That is why this is best handled by Luci.

Nobody disagrees, but then LuCI has to "magically come up with an IP that's not already in use" and this has to be better defined before we can implement it. See considerations three comments above.

damadmai commented 1 year ago

Is it a single address or can it be multiple (IPv4 + IPv6)?

It can be none, one of that or both at the same time and even multiple ones. I would suggest the same input field as used in AllowedIPs.

Must the addresses lie within the subnets of the router side wireguard interface addresses?

The router side wireguard interface does not need to have an interface adress at all, I would not enforce anything other than it should be a valid IPv4 or IPv6 adresse.

Shall we enforce uniqueness among all peers or allow duplicates?

In the beginning it should be sufficent to omit any check for that in order to keep it simple. For example with static DHCP/DNS entries there is no check and dnsmasq does not start if you input duplicates, but I would also keep it simple by omitting the check because I could imagine a scenario where somebody would do

Shall we automatically allocate a new IP when adding a peer and prefill the field or leave it empty?

No, just keep it optional like the private key field.

What if the user changes the subnets (addresses) of the wireguard interface, shall we clear out all peer addresses or keep them?

Just keep it simple, as said before the wireguard interface does not need to have an IP and even if it has one it does not need to be in the same subnet.

damadmai commented 1 year ago

In summary I think it would be very helpful to add an optional field like "Private Key" name "Addresses" with the same input type as "Allowed IPs" field and just add the input from there to the generated config.

damadmai commented 1 year ago

The Address Field is not part of wg but of wg-quick see also man 8 wg-quick

damadmai commented 1 year ago

It may be desirable to also make the possibility for setting DNS and MTU

nevumx commented 1 year ago

Hi folks, one PR for for the Addresses field is here, and there is also one for the DNS setting here. I didn't do MTU yet, but if someone really wants it, I can add it. First time contributing so let me know if anything is amiss. The PRs will conflict with each other, but I can fix that after as well.

nevumx commented 1 year ago

@jow- and @castillofrancodamian, thoughts on the above PRs?

nevumx commented 1 year ago

Any updates here, folks? @jow- / @castillofrancodamian ?

castillofrancodamian commented 1 year ago

@nevumx I'm not a developer 😢

nevumx commented 1 year ago

@castillofrancodamian Oh sorry, do you know who I should tag, then? 😅

castillofrancodamian commented 1 year ago

@nevumx The other person you tagged is a developer.

nevumx commented 1 year ago

@jow- I updated my PRs above(https://github.com/openwrt/luci/pull/6352 and https://github.com/openwrt/luci/pull/6351) to address feedback and merge conflicts, let me know your thoughts.