luci icon indicating copy to clipboard operation
luci copied to clipboard

luci-proto-amneziawg: add new package

Open leninalive opened this issue 1 year ago • 15 comments

image

  • [x] This PR is not from my main or master branch :poop:, but a separate branch :white_check_mark:
  • [x] Each commit has a valid :black_nib: Signed-off-by: <[email protected]> row (via git commit --signoff)
  • [x] Each commit and PR title has a valid :memo: <package name>: title first line subject for packages
  • [x] Incremented :up: any PKG_VERSION in the Makefile
  • [x] Tested on: armv8/22.03.5/Safari :white_check_mark:
  • [x] ( Preferred ) Screenshot or mp4 of changes:
  • [x] ( Optional ) Depends on: e.g. openwrt/packages#23633
  • [x] Description: AmneziaWG VPN kernel module makes it resource efficient to connect to Amnezia VPN services. It is the hardened version of WireGuard protocol that allows to change key WG header values and add junk to handshake. This is the LUCI protocol package to manage AmneziaWG settings with GUI.

leninalive avatar Mar 12 '24 20:03 leninalive

Hi @leninisdead - thanks for this PR. I think most of this can be reduced by re-using the existing wireguard stuff. By checking whether wireguard or amnezia is installed, one can display the one extra tab for amnezia settings.

Everything else is basically the same, right?

systemcrash avatar Mar 12 '24 21:03 systemcrash

@systemcrash I don't agree with that. I don't know how exactly amneziawg is working and what configuration parameters it needs . But regardless of that, a few remarks why it es better to but this in a separate packages (luci-app-amneziawg):

  • Simplifies the dependency handling. I do not have to install amneziawg if I only need wireguard.
  • Code components should only do what they are intended for. Keeps it simple.
  • What if one of the two is no longer needed? Then the code must be searched to see what can be removed and what can remain

I am in favor of putting it in a separate luci-proto-amneziawg package.

If there are similarities, then we could move this in a library packages.

feckert avatar Mar 13 '24 07:03 feckert

@systemcrash in addition to @feckert arguments, the amneziawg has a few additional amnezia-specific options (as far as I understand to better mask traffic), it may become too complex to maintain those additional options within a single package.

stangri avatar Mar 13 '24 09:03 stangri

I agree with @stangri , (see also this discussion: https://github.com/openwrt/packages/pull/23633) better make it a separate package so that it can be maintained separately

egc112 avatar Mar 19 '24 17:03 egc112

These were just coding style remarks. I have not tested this yet.

I think you'll find that most of these remarks apply equally to the original wireguard files, since this is basically a copy paste of that. In such a case I think where they're identical, they can be left to be as is. Especially the i18n strings so as not to create new translation work. (I haven't double checked yet but I think this is the case)

systemcrash avatar Mar 20 '24 10:03 systemcrash

I'm not really keen of adding that much duplicated code. The existing wg implementation alone had myriads of bugs and took years to mature. Copy-pasting that now into a 90% identical package does not seem ideal. I fully expect this to diverge and bit rot over time...

Consider factoring out the existing wireguard proto logic (such as qr code generation, key handling etc.) into a tools class which you can use from both proto handlers and work towards sharing the rpcd plugin procedures and related ACL definitions into a common package.

Since this "amneziawg" protocol appears to be a superset of plain wireguard, you can likely simply depend on luci-proto-wireguard from it and utilize resources from that package without having to introduce a 3rd common shared code package for both protos.

jow- avatar Mar 20 '24 12:03 jow-

@feckert @jow- thank you for the review.

Since this "amneziawg" protocol appears to be a superset of plain wireguard, you can likely simply depend on luci-proto-wireguard from it and utilize resources from that package without having to introduce a 3rd common shared code package for both protos.

@jow- this is not a superset, but a fork of wireguard and is designed to work independently from wireguard (wireguard proper does not need to be installed for amneziawg to work). If this package depends on (resources from) luci-proto-wireguard, would it also not bring the wireguard/other dependencies then? Can you please elaborate on how resources from luci-proto-wireguard can be reused without unnecessary package dependencies?

Could you please also have a look at https://github.com/openwrt/packages/pull/23632 -- what needs to be added to the Makefile to make sure the wireguard sources are downloaded prior to build? The PKG_BUILD_DEPENDS:=package/linux/kernel doesn't seem to help.

stangri avatar Mar 20 '24 18:03 stangri

@jow- this is not a superset, but a fork of wireguard and is designed to work independently from wireguard (wireguard proper does not need to be installed for amneziawg to work). If this package depends on (resources from) luci-proto-wireguard, would it also not bring the wireguard/other dependencies then? Can you please elaborate on how resources from luci-proto-wireguard can be reused without unnecessary package dependencies?

I reviewed the source, and while it's a fork, it does seem to be a superset. It wraps wireguard up in an extra something something, where basically none of the original wireguard source (that determines the proto behavior, anyway) is modified, I think.

But you raise valid points. We are approaching dep hell.

Is there some Makefile-ese way of saying ignore deps if being called from a specific package Makefile?

systemcrash avatar Mar 20 '24 19:03 systemcrash

I wouldn’t mind hearing from Jason @zx2c4. If the layer that amnezia adds really doesn't modify the wg proto, and formal verification is thus unaffected, why not just try adding the new bits to wg itself? I have no notion of whether that has happened. Sure it'd be a long road but @zx2c4 could elaborate as to why or why not adding it might be a good idea.

There's the possibility that you can replace wg with amnesia and existing configs break. But you can replace amnesia with wg and the amnesia config continues to work....?

Do we also get stuck at that hurdle that config changes are not yet handled by netif/procd? Can someone please do something about that...

systemcrash avatar Mar 20 '24 19:03 systemcrash

In the meantime, you can provide your own "repo" where you serve up all of the packages you make. Be sure to include instructions on your website to add a new repo.

Or in the meantime you could use @stangri's repo. https://source.openwrt.melmac.net/

systemcrash avatar Mar 27 '24 01:03 systemcrash

Since this "amneziawg" protocol appears to be a superset of plain wireguard, you can likely simply depend on luci-proto-wireguard from it and utilize resources from that package without having to introduce a 3rd common shared code package for both protos.

@jow- If this package were to depend on (resources from) luci-proto-wireguard, would it also not bring the wireguard/other dependencies then? Can you please elaborate on how resources from luci-proto-wireguard can be reused without unnecessary package dependencies?

stangri avatar Apr 05 '24 20:04 stangri

Shares libraries. Which wg doesn't have or is not.

systemcrash avatar Apr 07 '24 10:04 systemcrash

Can you please elaborate on how resources from luci-proto-wireguard can be reused without unnecessary package dependencies?

By moving shared code into a luci-lib-wireguard or similar, then make both luci-proto-* packages depend on that.

jow- avatar Apr 07 '24 11:04 jow-

By moving shared code into a luci-lib-wireguard or similar, then make both luci-proto-* packages depend on that.

I'd still support this being merged as is and hoping that a js guru (@stokito or @systemcrash) want to take a shot at making a shared library later.

@jow- could you please also have a look at https://github.com/openwrt/packages/pull/23632 -- what needs to be added to the Makefile to make sure the wireguard sources are downloaded prior to build? The PKG_BUILD_DEPENDS:=package/linux/kernel doesn't seem to help.

PS. There's a decent amount of interest in the amneziawg packages for OpenWrt, exhibited by the fact that there are forks of the original project set up just to use github workflows to build binaries for them.

stangri avatar Apr 07 '24 22:04 stangri

I see this as a complex problem on different levels.

The PR is difficult to review because the code was copied from luci-proto-wireguard but it's not clear what exactly was changed. @leninalive could you please split it into two commits: the first if just a folder creation with the wireguard code as is and the second commit is a diff of what was changed. In your code you have a different code formatting like using double quote " instead of single '. This makes diff too big.

Another big source of difference is translation keys where the only change is that the WireGuard was changed to Amnezia. I think this can be partially solved by just preserving keys as is and leaving the WireGuard in keys. That's should be fine for users. You may add a note above that the AmneziaWG is a fork for the WireGuard and these term are interchangeable here.

As an alternative we may change keys in the WireGuard itself to be more generic e.g. replace the WireGuard keep alive interval to VPN server keep alive interval.

Ideally would be if the Amnezia store own options just in the same config file as the WG but just in own section. Similarly the Amnezia might be some transport plugin for the WG but not a fork.

But today we have what we have. And users needs for a working solution today, even if it's not ideal. I don't believe it's possible to make some shared library because basically a fork can change any aspect that can't be now shared. For example the WG may deprecate some option but the Amnezia won't follow. Also shared libraries are mainly needed if it's expected to use both packages simultaneously which is not really expected here. So having just a copy paste should be fine and not so difficult to maintain. I believe that core options won't be changed in near time and even is some version is stalled that won't be a problem for most users.

The more general problem is that such advanced VPNs are by their nature continuously forked, created and improving every day. This makes this difficult to maintain but also core developers may simply not use them and can't really test. Also each tool may be targeted for a specific region. This is slippery slope to maintain highly volatile packages in the mainstream source code and packages feed. This is simply too big burden for core developers.

So I think it should be placed in a separate packages feed. The Amnezia can create own feed on their website: it's really just ipk file and Packages.gz. This is also not ideal for several reasons: build for all platforms, trust for binaries signed by someone who don't really trustworthy, complicated for users to add an alternative feed. All the problems can be at least partially solved, for example build with github actions so we can trust to the resulted binary, alternative feeds can be pre-configured in a stock firmware etc. This is more complicated way but it should be more productive.

For example today it's exists the ImmortalWrt which is intended mainly for China users. And this is a successful community and some of their packages were backported to the OpenWrt e.g. luci-app-cloudflared.

Maybe you can also offer the Amnezia to them. Anyway, it would be good to have some alt feed with some apps that definitely won't be added to main stream like the luci-theme-kucat that was rejected as inappropriate to be included into official repo.

stokito avatar Apr 08 '24 10:04 stokito