mirror of
https://github.com/MHSanaei/3x-ui.git
synced 2026-09-17 15:47:14 +00:00
fix(amneziawg): resolve 7 Medium findings from the automated PR review
Each is independently reproducible; fixed together since one review pass found all of them. - manager.go: the shared "ip rule add fwmark" policy route had no existence check, so it duplicated in "ip rule show" on every interface bounce (which hostRulesFingerprint forces on any client add/remove/ re-IP). Now checked via "ip rule list | grep -q ..." first. (Finding 2) - params.go: ExternalInterface, IPv6ExternalInterface, and subnetIp/ subnetCidr are interpolated unescaped into a shell-executed PostUp/ PostDown line, but only obfuscation and the IPv6 subnet were validated before save. Added ValidateInterfaceName (a strict charset+length pattern) and ValidateSubnetIPv4 (netip.ParsePrefix), wired into normalizeAmneziaWGSettings. (Finding 3) - amneziawg_job.go: IsAwgInstalled() existed but nothing ever called it, so a host without awg/awg-quick (the Docker image, RHEL, Arch, a failed install.sh PPA step) logged a reconcile failure every 10s forever. Now checked once an inbound actually needs it, warning once instead of spamming. (Finding 4) - client_inbound_apply.go: the WireGuard/AmneziaWG credential carry-forward (added so a metadata-only client edit doesn't rotate keys) never covered ForwardedPorts, so a partial edit -- an API call or Telegram-bot toggle that omits the field -- silently wiped a client's port-forwarding spec. Carried forward and written back the same way the key fields already are. (Finding 5) - manager.go: hostRulesFingerprint keyed each peer on its IPv4 address only, and structuralFingerprint omitted IPv6Enabled/IPv6ExternalInterface entirely, so an IPv6-only change could pick the syncconf reload path (which never re-runs PostUp, leaving a stale NDP-proxy entry) or be a complete no-op. Both fingerprints now cover the IPv6 fields. (Finding 6) - port_conflict.go: the AmneziaWG egress bridge (injectAmneziawgEgress) binds 127.0.0.1:63100+id with no collision check anywhere, since it isn't a database row the ordinary port-conflict query can see -- same blind spot the reserved Xray API port already has its own check for. Added the equivalent check for the AmneziaWG bridge port. (Finding 7) - install.sh: install_amneziawg ran unconditionally for every install/ update, building a DKMS kernel module and enabling host-wide IPv4/IPv6 forwarding whether or not the feature is ever used. Gated behind a new should_install_amneziawg (XUI_INSTALL_AMNEZIAWG=true/false, or an interactive y/N prompt defaulting to no). Also replaced the deprecated apt-key adv with a dedicated keyring + signed-by= on the Debian branch, and guarded its sources.list appends against duplication on a retried install. (Finding 8) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
This commit is contained in:
@@ -134,6 +134,8 @@ func (inst Instance) structuralFingerprint() string {
|
||||
strconv.Itoa(o.S1), strconv.Itoa(o.S2), strconv.Itoa(o.S3), strconv.Itoa(o.S4),
|
||||
o.H1, o.H2, o.H3, o.H4, o.I1,
|
||||
inst.ExternalInterface,
|
||||
strconv.FormatBool(inst.IPv6Enabled),
|
||||
inst.IPv6ExternalInterface,
|
||||
}
|
||||
return strings.Join(parts, "|")
|
||||
}
|
||||
@@ -155,8 +157,9 @@ func (inst Instance) peersFingerprint() string {
|
||||
}
|
||||
|
||||
// hostRulesFingerprint identifies per-peer state that only ever takes effect
|
||||
// through PostUp/PostDown shell rules — forwarded ports, and (unconditionally,
|
||||
// for every peer with a usable IPv4 address) the TPROXY rule into this
|
||||
// through PostUp/PostDown shell rules — forwarded ports, the peer's IPv6
|
||||
// address (its NDP-proxy PostUp/PostDown entry), and (unconditionally, for
|
||||
// every peer with a usable IPv4 address) the TPROXY rule into this
|
||||
// instance's own Xray bridge — rather than the WireGuard peer table itself.
|
||||
// It is checked separately from peersFingerprint because `awg syncconf`
|
||||
// never re-runs PostUp/PostDown, so a change here must force a full
|
||||
@@ -165,11 +168,15 @@ func (inst Instance) peersFingerprint() string {
|
||||
// now tied to every peer's mere presence (there's no more per-peer opt-in
|
||||
// flag), every peer is included unconditionally: adding, removing, or
|
||||
// re-addressing a peer now also forces a bounce, the same way a
|
||||
// ForwardedPorts-only change always did.
|
||||
// ForwardedPorts-only change always did. The IPv6 address must be included
|
||||
// here too: without it, a peer that only changes its IPv6 AllowedIPs entry
|
||||
// still matches on FirstIPv4 alone, so ensureActionFor would pick the
|
||||
// syncconf reload path — which never re-runs PostUp — leaving that peer's
|
||||
// NDP-proxy entry pointed at its old, now-wrong address.
|
||||
func (inst Instance) hostRulesFingerprint() string {
|
||||
pairs := make([]string, 0, len(inst.Peers))
|
||||
for _, p := range inst.Peers {
|
||||
pairs = append(pairs, fmt.Sprintf("%s=fwd:%s;ip:%s", p.Email, p.ForwardedPorts, FirstIPv4(p.AllowedIPs)))
|
||||
pairs = append(pairs, fmt.Sprintf("%s=fwd:%s;ip:%s;ip6:%s", p.Email, p.ForwardedPorts, FirstIPv4(p.AllowedIPs), firstIPv6(p.AllowedIPs)))
|
||||
}
|
||||
slices.Sort(pairs)
|
||||
return strings.Join(pairs, "|")
|
||||
@@ -687,8 +694,13 @@ func defaultPostUpDown(inst Instance, ext string) (postUp, postDown string) {
|
||||
// IPv6-forwarding sysctl above — it is added idempotently here and
|
||||
// never torn down in PostDown; a second AmneziaWG instance must find
|
||||
// it already in place, not race to remove what the first still needs.
|
||||
// "ip rule add" is not itself idempotent (a second call inserts a
|
||||
// duplicate rather than deduplicating), and hostRulesFingerprint keys
|
||||
// on every peer's presence/IP, so PostUp re-runs on any client
|
||||
// add/remove/re-IP — without the existence check below, "ip rule
|
||||
// show" would accumulate one duplicate entry per bounce forever.
|
||||
up = append(up,
|
||||
fmt.Sprintf("ip rule add fwmark %#x lookup %d 2>/dev/null || true", EgressFwmark, EgressTable),
|
||||
fmt.Sprintf("ip rule list | grep -q 'fwmark %#x lookup %d' || ip rule add fwmark %#x lookup %d", EgressFwmark, EgressTable, EgressFwmark, EgressTable),
|
||||
fmt.Sprintf("ip route replace local 0.0.0.0/0 dev lo table %d", EgressTable),
|
||||
)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user