openwrt / luci

LuCI - OpenWrt Configuration Interface
Apache License 2.0
6.27k stars 2.51k forks source link

luci-proto-wireguard - Add DNS Option #2512

Closed Strykar closed 4 years ago

Strykar commented 5 years ago

@zx2c4 @jow- Adding option dns '8.8.8.8' to /etc/config/network appears to work, but perhaps the correct place to specify this is in Wireguard's interface options in Luci?

feckert commented 5 years ago

@Strykar For whom should the dns information be useful? Interface section or peer section? And to which section did you added the option dns '8.8.8.8' in the network config?

ghost commented 5 years ago

@feckert It should be exposed in config interface 'wgX' - if it is any help/guidance to go by export wg config from its Android app.

It is useful to prevent DNS queries leaking to the ISP, respectively outside the tunnel.

zx2c4 commented 5 years ago

Seems to be a departure from how openwrt actually works.

Strykar commented 5 years ago

@feckert The peer section. @zx2c4 Where else do you suggest this be added to luci?

zx2c4 commented 5 years ago

@Strykar DNS does NOT belong in the peer section.

This kind of thing most likely doesn't belong anywhere near the wireguard module.

:-1:

feckert commented 5 years ago

@Strykar Since you mentioned the Android app. I think the information is only use full for that purpose? May be we could add this information to the QR-Code See https://github.com/openwrt/luci/pull/2749

ghost commented 5 years ago

Me mentioned Android. And I reckon that is the point that @zx2c4 is trying to make - Android device not being a router and thus provisioning a DNS setting in the WG context makes sense.

But OpenWRT catering for router devices the DNS decision should be made in that context and not the context of WG.

Strykar commented 5 years ago

I meant luci-wireguard should probably have a method to provide a DNS address for a (mobile) peer to use.

WhatsApp Image 2019-06-28 at 6 13 35 PM

@feckert Ah, I think I see what @zx2c4 means now. Perhaps this does belong in a QR section instead of the wg@interface config? But that would mean luci-wireguard can only provide peers with a DNS config if the peer is configured via QR code, which seems restrictive?

zx2c4 commented 5 years ago

Wait what? Is somebody adding qrcodes to openwrt?

feckert commented 5 years ago

@zx2c4 it already added but in #2749 we want to make compatible to android ?

Strykar commented 5 years ago

@zx2c4 Yes, luci-app-fwknopd uses qrencode.

feckert commented 5 years ago

Is this still an issue? I think it is more a feature request. In https://github.com/openwrt/luci/pull/2749 @dibdot has added QR-Code generation logic so we can scan this with the Android-App. There is still work todo, but maybe we could add the DNS information to the QR-Code.

Strykar commented 4 years ago

@feckert This is still unresolved, and I feel @n8v8R and I have made the case for it, unsure how we could make the issue clearer. So while @zx2c4 states:

This kind of thing most likely doesn't belong anywhere near the wireguard module.

He does not provide an alternate/solution from OpenWrt's perspective, which could be:

  1. A server for road-warriors
  2. A client (NATing entire LANs) to a remote server
  3. Other combinations of roles

The fact remains that to prevent DNS leaks from mobile clients, the DNS option needs to be set. By leaving this to Qrencode, we would be implying that DNS leak-proof configurations for mobile clients are available only via QR configured setups, which seems wrong.

jow- commented 4 years ago

Closing this since the wireguard proto handler does not appear to implement DNS settings. Even if it did, it would merely append another server to the system wide resolv list. You can as well add the IP in the WAN, WAN6 or LAN interface to achieve the same effect.

Strykar commented 4 years ago

@jow- Wouldn't it be cleaner to have the DNS change via the wireguard-proto when the interface is brought up and down if all traffic is routed through it? I'm trying to understand the resistance to adding this.

jow- commented 4 years ago

No it wouldn't be cleaner, it would work completely different to any other proto handler and it would do things that are entirely out of scope of the per proto DNS option.

All an option dns in a proto handler is doing, is adding the given IP addresses to the global resolver list. It does not do any kind of priorization, leak prevention, rerouting, split horizon or whatever.

I agree that an option to replace the previous DNS addresses with another set of addresses on ifup might be useful, but this is not the place to discuss that. It first needs to be implemented in OpenWrt base, then in the wireguard proto handler and finally we can expose it in the gui.