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

luci-proto-amneziawg: add new package #6986

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

leninalive
Copy link

@leninalive leninalive commented Mar 12, 2024

image

  • This PR is not from my main or master branch 💩, but a separate branch ✅
  • Each commit has a valid ✒️ Signed-off-by: <[email protected]> row (via git commit --signoff)
  • Each commit and PR title has a valid 📝 <package name>: title first line subject for packages
  • Incremented 🆙 any PKG_VERSION in the Makefile
  • Tested on: armv8/22.03.5/Safari ✅
  • ( Preferred ) Screenshot or mp4 of changes:
  • ( Optional ) Depends on: e.g. amneziawg-tools: add new package packages#23633
  • 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.

@systemcrash systemcrash marked this pull request as draft March 12, 2024 20:57
@systemcrash systemcrash added the depends on PR in other repo PR depends on PR in sister repo e.g. openwrt/packags label Mar 12, 2024
@systemcrash
Copy link
Contributor

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?

@feckert
Copy link
Member

feckert commented Mar 13, 2024

@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.

@stangri
Copy link
Member

stangri commented Mar 13, 2024

@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 stangri marked this pull request as ready for review March 15, 2024 02:33
@egc112
Copy link
Contributor

egc112 commented Mar 19, 2024

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

Copy link
Member

@feckert feckert left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

LUCI_DEPENDS:=+luci-base +amneziawg-tools +ucode
LUCI_PKGARCH:=all

PKG_PROVIDES:=luci-proto-amneziawg
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This line is not needed because it is already in the directory luci-proto-amneziawg

include $(TOPDIR)/rules.mk

PKG_MAINTAINER:=Amnezia Admin <[email protected]>
PKG_VERSION:=0.0.1-1
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is not needed because the source is maintained in this repository

@@ -0,0 +1,107 @@
// Copyright 2022 Jo-Philipp Wich <[email protected]>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add your name.

generateKeyPair: {
call: function() {
const priv = command('awg genkey 2>/dev/null');
const pub = command(`echo ${shellquote(priv)} | awg pubkey 2>/dev/null`);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the line above you use ' and there you use `.
Please unify this.

});

var qrIcon =
'<svg viewBox="0 0 29 29" xmlns="http://www.w3.org/2000/svg"><path fill="#fff" d="M0 0h29v29H0z"/><path d="M4 4h1v1H4zM5 4h1v1H5zM6 4h1v1H6zM7 4h1v1H7zM8 4h1v1H8zM9 4h1v1H9zM10 4h1v1h-1zM12 4h1v1h-1zM13 4h1v1h-1zM14 4h1v1h-1zM15 4h1v1h-1zM16 4h1v1h-1zM18 4h1v1h-1zM19 4h1v1h-1zM20 4h1v1h-1zM21 4h1v1h-1zM22 4h1v1h-1zM23 4h1v1h-1zM24 4h1v1h-1zM4 5h1v1H4zM10 5h1v1h-1zM12 5h1v1h-1zM14 5h1v1h-1zM16 5h1v1h-1zM18 5h1v1h-1zM24 5h1v1h-1zM4 6h1v1H4zM6 6h1v1H6zM7 6h1v1H7zM8 6h1v1H8zM10 6h1v1h-1zM12 6h1v1h-1zM18 6h1v1h-1zM20 6h1v1h-1zM21 6h1v1h-1zM22 6h1v1h-1zM24 6h1v1h-1zM4 7h1v1H4zM6 7h1v1H6zM7 7h1v1H7zM8 7h1v1H8zM10 7h1v1h-1zM12 7h1v1h-1zM13 7h1v1h-1zM14 7h1v1h-1zM15 7h1v1h-1zM18 7h1v1h-1zM20 7h1v1h-1zM21 7h1v1h-1zM22 7h1v1h-1zM24 7h1v1h-1zM4 8h1v1H4zM6 8h1v1H6zM7 8h1v1H7zM8 8h1v1H8zM10 8h1v1h-1zM16 8h1v1h-1zM18 8h1v1h-1zM20 8h1v1h-1zM21 8h1v1h-1zM22 8h1v1h-1zM24 8h1v1h-1zM4 9h1v1H4zM10 9h1v1h-1zM12 9h1v1h-1zM13 9h1v1h-1zM15 9h1v1h-1zM18 9h1v1h-1zM24 9h1v1h-1zM4 10h1v1H4zM5 10h1v1H5zM6 10h1v1H6zM7 10h1v1H7zM8 10h1v1H8zM9 10h1v1H9zM10 10h1v1h-1zM12 10h1v1h-1zM14 10h1v1h-1zM16 10h1v1h-1zM18 10h1v1h-1zM19 10h1v1h-1zM20 10h1v1h-1zM21 10h1v1h-1zM22 10h1v1h-1zM23 10h1v1h-1zM24 10h1v1h-1zM13 11h1v1h-1zM14 11h1v1h-1zM15 11h1v1h-1zM16 11h1v1h-1zM4 12h1v1H4zM5 12h1v1H5zM8 12h1v1H8zM9 12h1v1H9zM10 12h1v1h-1zM13 12h1v1h-1zM15 12h1v1h-1zM19 12h1v1h-1zM21 12h1v1h-1zM22 12h1v1h-1zM23 12h1v1h-1zM24 12h1v1h-1zM5 13h1v1H5zM6 13h1v1H6zM8 13h1v1H8zM11 13h1v1h-1zM13 13h1v1h-1zM14 13h1v1h-1zM15 13h1v1h-1zM16 13h1v1h-1zM19 13h1v1h-1zM22 13h1v1h-1zM4 14h1v1H4zM5 14h1v1H5zM9 14h1v1H9zM10 14h1v1h-1zM11 14h1v1h-1zM15 14h1v1h-1zM18 14h1v1h-1zM19 14h1v1h-1zM20 14h1v1h-1zM21 14h1v1h-1zM22 14h1v1h-1zM23 14h1v1h-1zM7 15h1v1H7zM8 15h1v1H8zM9 15h1v1H9zM11 15h1v1h-1zM12 15h1v1h-1zM13 15h1v1h-1zM17 15h1v1h-1zM18 15h1v1h-1zM20 15h1v1h-1zM21 15h1v1h-1zM23 15h1v1h-1zM4 16h1v1H4zM6 16h1v1H6zM10 16h1v1h-1zM11 16h1v1h-1zM13 16h1v1h-1zM14 16h1v1h-1zM16 16h1v1h-1zM17 16h1v1h-1zM18 16h1v1h-1zM22 16h1v1h-1zM23 16h1v1h-1zM24 16h1v1h-1zM12 17h1v1h-1zM16 17h1v1h-1zM17 17h1v1h-1zM18 17h1v1h-1zM4 18h1v1H4zM5 18h1v1H5zM6 18h1v1H6zM7 18h1v1H7zM8 18h1v1H8zM9 18h1v1H9zM10 18h1v1h-1zM14 18h1v1h-1zM16 18h1v1h-1zM17 18h1v1h-1zM21 18h1v1h-1zM22 18h1v1h-1zM23 18h1v1h-1zM4 19h1v1H4zM10 19h1v1h-1zM12 19h1v1h-1zM13 19h1v1h-1zM15 19h1v1h-1zM16 19h1v1h-1zM19 19h1v1h-1zM21 19h1v1h-1zM23 19h1v1h-1zM24 19h1v1h-1zM4 20h1v1H4zM6 20h1v1H6zM7 20h1v1H7zM8 20h1v1H8zM10 20h1v1h-1zM12 20h1v1h-1zM13 20h1v1h-1zM15 20h1v1h-1zM18 20h1v1h-1zM19 20h1v1h-1zM20 20h1v1h-1zM22 20h1v1h-1zM23 20h1v1h-1zM24 20h1v1h-1zM4 21h1v1H4zM6 21h1v1H6zM7 21h1v1H7zM8 21h1v1H8zM10 21h1v1h-1zM13 21h1v1h-1zM15 21h1v1h-1zM16 21h1v1h-1zM19 21h1v1h-1zM21 21h1v1h-1zM23 21h1v1h-1zM24 21h1v1h-1zM4 22h1v1H4zM6 22h1v1H6zM7 22h1v1H7zM8 22h1v1H8zM10 22h1v1h-1zM13 22h1v1h-1zM15 22h1v1h-1zM18 22h1v1h-1zM19 22h1v1h-1zM20 22h1v1h-1zM21 22h1v1h-1zM22 22h1v1h-1zM4 23h1v1H4zM10 23h1v1h-1zM12 23h1v1h-1zM13 23h1v1h-1zM14 23h1v1h-1zM17 23h1v1h-1zM18 23h1v1h-1zM20 23h1v1h-1zM22 23h1v1h-1zM4 24h1v1H4zM5 24h1v1H5zM6 24h1v1H6zM7 24h1v1H7zM8 24h1v1H8zM9 24h1v1H9zM10 24h1v1h-1zM12 24h1v1h-1zM13 24h1v1h-1zM14 24h1v1h-1zM16 24h1v1h-1zM17 24h1v1h-1zM18 24h1v1h-1zM22 24h1v1h-1zM24 24h1v1h-1z"/></svg>';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we move this to a svg file please?

"endpoint",
_("Connection endpoint"),
_(
"The public hostname or IP address of this system the peer should connect to. This usually is a static public IP address, a static hostname or a DDNS domain."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this into separate translation units on every dot.

"allowed_ips",
_("Allowed IPs"),
_(
"IP addresses that are allowed inside the tunnel. The peer will accept tunnelled packets with source IP addresses matching this list and route back packets with matching destination IP."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please split this into separate translation units on every dot.

form.Value,
"endpoint_port",
_("Endpoint Port"),
_("Optional. Port of peer.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And move Optional. to the tail (optional) of the whole line.

form.Flag,
"route_allowed_ips",
_("Route Allowed IPs"),
_("Optional. Create routes for Allowed IPs for this peer.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And move Optional. to the tail (optional) of the whole line.

form.Value,
"description",
_("Description"),
_("Optional. Description of peer.")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And move Optional. to the tail (optional) of the whole line.

@systemcrash
Copy link
Contributor

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)

@jow-
Copy link
Contributor

jow- commented Mar 20, 2024

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.

@stangri
Copy link
Member

stangri commented Mar 20, 2024

@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 openwrt/packages#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.

@systemcrash
Copy link
Contributor

systemcrash commented Mar 20, 2024

@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
Copy link
Contributor

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 systemcrash marked this pull request as draft March 20, 2024 23:23
@systemcrash
Copy link
Contributor

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/

@stangri
Copy link
Member

stangri commented Apr 5, 2024

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?

@systemcrash
Copy link
Contributor

systemcrash commented Apr 7, 2024 via email

@jow-
Copy link
Contributor

jow- commented Apr 7, 2024

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.

@stangri
Copy link
Member

stangri commented Apr 7, 2024

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 openwrt/packages#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.

@stokito
Copy link
Contributor

stokito commented Apr 8, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
depends on PR in other repo PR depends on PR in sister repo e.g. openwrt/packags
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants