feat: add AmneziaWG outbound#2
Merged
Merged
Conversation
Add an amneziawg outbound that reuses sing-box's WireGuard transport stack but drives the amnezia-vpn/amneziawg-go engine, so the obfuscation parameters (Jc/Jmin/Jmax, S1-S4, H1-H4, I1-I5) can be configured. Plain WireGuard is left untouched. - transport/amneziawg: copy of transport/wireguard with the device/conn/tun imports pointed at amneziawg-go (3-arg NewDevice, no listener/reserved-bind extensions), plus device-level AmneziaWG IPC params appended to the UAPI config. The device-stack files are kept byte-identical to the wireguard sibling for maintainability. - protocol/amneziawg: outbound registering option.AmneziaWGOutboundOptions under C.TypeAmneziaWG. - option.AmneziaWGOutboundOptions: WireGuard fields + AWG obfuscation knobs. - constant: TypeAmneziaWG. amnezia-vpn/amneziawg-go is consumed as a pristine upstream dependency (no fork), so it can be updated independently.
Owner
Author
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds AmneziaWG as a new outbound protocol in sing-box. The change introduces ChangesAmneziaWG Outbound Protocol
Sequence Diagram(s)sequenceDiagram
participant Router as sing-box Router
rect rgba(100, 149, 237, 0.5)
note over Router,Endpoint: Startup
Router->>Outbound: Start StartStage
Outbound->>Endpoint: Start resolve
Endpoint->>Device: Start
Endpoint->>Device: IpcSet AmneziaWG config
Device-->>Endpoint: ready
end
rect rgba(144, 238, 144, 0.5)
note over Router,ClientBind: Data plane – outgoing packet
Router->>Outbound: DialContext ctx tcp destination
Outbound->>DNSRouter: Lookup if FQDN
DNSRouter-->>Outbound: resolved IP
Outbound->>Endpoint: DialContext resolved
Endpoint->>Device: DialContext
Device->>ClientBind: Send buf – patch bytes[1:4]
ClientBind->>Peer: UDP datagram
end
rect rgba(255, 165, 0, 0.5)
note over Peer,ClientBind: Data plane – incoming packet
Peer->>ClientBind: UDP datagram
ClientBind->>ClientBind: receive clear bytes[1:4]
ClientBind->>Device: deliver packet
Device-->>Router: net.Conn read
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| e.options.Logger.Error(fmt.Sprintf(strings.ToLower(format), args...)) | ||
| }, | ||
| } | ||
| wgDevice := device.NewDevice(e.tunDevice, bind, logger) |
There was a problem hiding this comment.
Workers option carried through but silently discarded
EndpointOptions.Workers (and the corresponding AmneziaWGOutboundOptions.Workers JSON field) is forwarded all the way to Endpoint.Start, but amneziawg-go's device.NewDevice only accepts 3 arguments (tun, bind, logger) — no workers parameter — so the value is never applied. A user who configures workers: 4 in their config will silently get single-threaded behaviour with no warning. Either document the field as no-op for this outbound or remove it from the options struct.
Comment on lines
+80
to
+83
| InitPacketMagicHeader string `json:"h1,omitempty"` | ||
| ResponsePacketMagicHeader string `json:"h2,omitempty"` | ||
| UnderloadPacketMagicHeader string `json:"h3,omitempty"` | ||
| TransportPacketMagicHeader string `json:"h4,omitempty"` |
There was a problem hiding this comment.
string while AmneziaWG protocol defines them as uint32
H1–H4 (InitPacketMagicHeader / ResponsePacketMagicHeader / etc.) are random 32-bit integers in the AmneziaWG specification. Every existing AmneziaWG client (including the amneziawg-go reference config and NekoBox itself) represents them as integers in config. Typing them string means users must supply their values as decimal strings (e.g. "h1": "1234567890") which is inconsistent with peer clients and is likely to cause confusion or integration friction with config converters that export numeric values.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (1)
transport/amneziawg/client_bind.go (1)
202-204: 💤 Low valuePotential data race on
reservedForEndpointmap.
SetReservedForEndpointwrites to the map without synchronization, whileSend(line 175) reads from it concurrently. Although the current call pattern (setup before device start) likely prevents this race in practice, adding synchronization would make the code defensively correct.Proposed fix
func (c *ClientBind) SetReservedForEndpoint(destination netip.AddrPort, reserved [3]byte) { + c.connAccess.Lock() + defer c.connAccess.Unlock() c.reservedForEndpoint[destination] = reserved }And in
Send, wrap the map read:+ c.connAccess.Lock() reserved, loaded := c.reservedForEndpoint[destination] + c.connAccess.Unlock()Alternatively, use
sync.Mapif the map is read-heavy.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@transport/amneziawg/client_bind.go` around lines 202 - 204, The `reservedForEndpoint` map is accessed concurrently without synchronization: `SetReservedForEndpoint` writes to it while `Send` reads from it, creating a potential data race. Add a mutex field to the ClientBind struct and use it to protect all accesses to `reservedForEndpoint`. Lock the mutex in `SetReservedForEndpoint` when writing to the map, and lock it in the `Send` method (around line 175) when reading from the map to ensure thread-safe concurrent access.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@constant/proxy.go`:
- Line 18: The ProxyDisplayName() function is missing a case handler for the
newly added TypeAmneziaWG constant, causing it to return "Unknown" instead of a
proper display name. Add a case statement inside the ProxyDisplayName() function
(which contains the switch statement handling different proxy types) that checks
for TypeAmneziaWG and returns the appropriate display name "AmneziaWG". The case
should be placed alongside the other existing cases in the switch statement to
maintain consistency with how other proxy types are handled.
In `@go.mod`:
- Line 8: The amneziawg-go dependency in go.mod is pinned to version v1.0.4
which does not exist in the repository. Update the version specification for the
github.com/amnezia-vpn/amneziawg-go dependency to an actual existing version,
such as v0.2.19 which is the latest available version, to resolve the module
resolution failure.
In `@protocol/amneziawg/outbound.go`:
- Around line 87-93: The type assertion `outboundDialer.(dialer.ResolveDialer)`
in the ResolvePeer function is unsafe and will panic if outboundDialer does not
implement the dialer.ResolveDialer interface. Replace the direct type assertion
with the comma-ok idiom by assigning the result to two variables (value and ok
boolean), check if the assertion succeeded, and return an appropriate error if
it fails before attempting to call QueryOptions() on the asserted type.
- Around line 78-82: The CreateDialer callback uses common.Must1 which will
panic at runtime if dialer.NewDefault returns an error, crashing the
application. To fix this, change the callback signature from returning only
N.Dialer to returning (N.Dialer, error) in both the EndpointOptions and
DeviceOptions structs. Then replace the common.Must1 call with proper error
handling where the callback returns the dialer and any error from
dialer.NewDefault directly, and update the call site in device_system.go that
invokes this callback to properly handle the returned error instead of allowing
a panic.
In `@transport/amneziawg/device_stack.go`:
- Around line 268-277: The WritePackets method in wireEndpoint has two issues:
first, packetBuffer.IncRef() is called unconditionally before checking if the
endpoint is closed, causing reference leaks when the done channel is selected;
second, the function returns 0 packets written when done is selected, losing the
count of packets that were actually sent before shutdown. Fix this by: moving
the IncRef() call to only execute when the packet is successfully sent to the
outbound channel (inside the outbound case of the select), tracking the number
of packets successfully written in a counter variable, and returning that
counter instead of 0 when the done channel is selected.
- Around line 205-213: The Close method in the stackDevice type is not
idempotent and will panic if called multiple times because closing an
already-closed channel causes a panic in Go. To fix this, add a guard mechanism
(such as using sync.Once or a boolean flag) to ensure that the channel close
operations on done and events only execute once, and wrap the close logic
accordingly. This will allow Close to be safely called multiple times without
triggering panics on repeated close attempts.
In `@transport/amneziawg/device_system_stack.go`:
- Around line 54-64: In the Write method of the systemStackDevice type, the
BatchWrite call is incorrectly passing the original `bufs` parameter instead of
the filtered `w.writeBufs` slice. The filtering logic collects packets that
failed to write via writeStack into w.writeBufs, but then passes all original
packets to BatchWrite, which defeats the filtering logic. Change the BatchWrite
call to pass w.writeBufs instead of bufs to ensure only the packets that
couldn't be written to the stack are sent to the batch device.
- Around line 93-117: The writeStack function does not handle cases where
header.IPVersion(packet) returns a version other than IPv4Version or
IPv6Version, leaving networkProtocol uninitialized with value 0 and destination
as a zero-value before calling w.endpoint.dispatcher.DeliverNetworkPacket. Add
validation after the switch statement to check if networkProtocol is still 0
(meaning an invalid or unrecognized IP version was encountered), and return
false early to prevent delivering packets with invalid protocol numbers to the
dispatcher.
In `@transport/amneziawg/device_system.go`:
- Around line 151-154: The Close() method in the systemDevice struct is not
nil-safe and not idempotent. Add a nil check before calling w.device.Close() to
handle cases where Start() failed before initialization. Additionally, protect
the closing of w.events channel from being closed multiple times by using
sync.Once or a guard flag to ensure Close() can be safely called multiple times
without panicking. Store the sync.Once as a field in systemDevice and use it to
ensure the close operation runs only once.
In `@transport/amneziawg/endpoint_options.go`:
- Line 29: The Workers field in EndpointOptions is publicly exposed but is never
applied during endpoint configuration generation in the endpoint setup path
(around endpoint.go lines 36-121). Either integrate the Workers value into the
IPC/device configuration generation (such as adding a workers parameter to the
config string) so that the user-provided Workers value is actually applied at
runtime, or remove the Workers field from EndpointOptions entirely if it is not
meant to be a configurable option, to avoid exposing unused configuration that
silently gets ignored.
In `@transport/amneziawg/endpoint.go`:
- Around line 179-182: The error handling in the wgDevice.IpcSet call includes
the ipcConf parameter in the E.Cause error message, which contains sensitive
private key material. Remove the ipcConf argument from the E.Cause call and keep
only the descriptive error message "setup wireguard" to prevent exposing
cryptographic material in error logs or reports.
- Around line 76-81: In the reserved value validation block for peers, the error
message on line 78 incorrectly references len(peer.reserved) which always
returns 3 (the fixed size of the [3]uint8 array), making the error message
ineffective for debugging. Change the error message to reference
len(rawPeer.Reserved) instead, which represents the actual length of the input
data being validated against the required 3 bytes.
- Around line 205-213: The Close method in Endpoint leaks the TUN device
resource because e.tunDevice is never closed, and the shutdown order is
incorrect. Add a check to close e.tunDevice if it is not nil before closing
e.device, and reorder the operations so that e.pauseCallback is unregistered
from e.pause first (before closing the device) to prevent callbacks from firing
during shutdown. This ensures proper resource cleanup and safe shutdown
sequencing.
- Around line 215-222: The onPauseUpdated method can cause a nil pointer panic
because e.device may not be initialized if Start was never called or returned
early. Add a nil check at the beginning of the onPauseUpdated method to guard
against calling Down() or Up() on a nil device pointer. If e.device is nil, the
method should return early without performing any operations.
- Around line 161-188: The code starts the tunDevice with e.tunDevice.Start()
and creates a wgDevice with device.NewDevice(), but if wgDevice.IpcSet() fails
and returns an error, these resources are not cleaned up before returning. Add
proper cleanup logic (such as calling appropriate Close or Stop methods on both
wgDevice and tunDevice) in the error handling path after wgDevice.IpcSet()
fails, or use a defer statement earlier in the function to ensure cleanup
happens regardless of the error path taken.
---
Nitpick comments:
In `@transport/amneziawg/client_bind.go`:
- Around line 202-204: The `reservedForEndpoint` map is accessed concurrently
without synchronization: `SetReservedForEndpoint` writes to it while `Send`
reads from it, creating a potential data race. Add a mutex field to the
ClientBind struct and use it to protect all accesses to `reservedForEndpoint`.
Lock the mutex in `SetReservedForEndpoint` when writing to the map, and lock it
in the `Send` method (around line 175) when reading from the map to ensure
thread-safe concurrent access.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 4fafbad4-7c64-443f-ad26-ff25f0274637
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (13)
constant/proxy.gogo.modoption/wireguard.goprotocol/amneziawg/outbound.gotransport/amneziawg/client_bind.gotransport/amneziawg/device.gotransport/amneziawg/device_stack.gotransport/amneziawg/device_stack_gonet.gotransport/amneziawg/device_stack_stub.gotransport/amneziawg/device_system.gotransport/amneziawg/device_system_stack.gotransport/amneziawg/endpoint.gotransport/amneziawg/endpoint_options.go
| toolchain go1.24.9 | ||
|
|
||
| require ( | ||
| github.com/amnezia-vpn/amneziawg-go v1.0.4 |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify amneziawg-go v1.0.4 exists and check for security advisories
# Check if the version exists on GitHub
echo "=== Checking release tags ==="
gh api repos/amnezia-vpn/amneziawg-go/tags --jq '.[].name' | head -10
# Check for security advisories
echo "=== Checking security advisories ==="
gh api graphql -f query='
{
securityVulnerabilities(first: 5, ecosystem: GO, package: "github.com/amnezia-vpn/amneziawg-go") {
nodes {
advisory {
summary
severity
publishedAt
}
vulnerableVersionRange
firstPatchedVersion {
identifier
}
}
}
}'Repository: hawkff/sing-box
Length of output: 319
🏁 Script executed:
#!/bin/bash
# Check all tags for v1.0.4
echo "=== Searching for v1.0.4 tag ==="
gh api repos/amnezia-vpn/amneziawg-go/tags --paginate --jq '.[].name' | grep -i "1.0.4" || echo "v1.0.4 not found"
# Also check releases
echo "=== Checking releases ==="
gh api repos/amnezia-vpn/amneziawg-go/releases --jq '.[].tag_name' | grep -i "1.0.4" || echo "v1.0.4 not found in releases"
# Check the go.mod file in the actual repository
echo "=== Checking go.mod content ==="
cat go.mod | grep -A2 -B2 "amneziawg-go"Repository: hawkff/sing-box
Length of output: 332
Nonexistent dependency version: v1.0.4 does not exist in amneziawg-go repository.
The specified version v1.0.4 does not exist in the amnezia-vpn/amneziawg-go repository. The latest available version is v0.2.19. This will cause Go module resolution to fail during builds. Either correct the version number or ensure the intended version is released before merging.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@go.mod` at line 8, The amneziawg-go dependency in go.mod is pinned to version
v1.0.4 which does not exist in the repository. Update the version specification
for the github.com/amnezia-vpn/amneziawg-go dependency to an actual existing
version, such as v0.2.19 which is the latest available version, to resolve the
module resolution failure.
Comment on lines
+78
to
+82
| CreateDialer: func(interfaceName string) N.Dialer { | ||
| return common.Must1(dialer.NewDefault(ctx, option.DialerOptions{ | ||
| BindInterface: interfaceName, | ||
| })) | ||
| }, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check CreateDialer signature and usage across the codebase
rg -n "CreateDialer" --type go -C3Repository: hawkff/sing-box
Length of output: 5242
Panic risk in CreateDialer callback at runtime.
common.Must1 will panic if dialer.NewDefault returns an error. Since this callback is invoked at runtime (in device_system.go), a panic would crash the application rather than returning a graceful error.
The callback signature func(interfaceName string) N.Dialer provides no mechanism to propagate errors. Changing this requires updating the signature in both EndpointOptions and DeviceOptions structs, and handling errors at all call sites (including transport/amneziawg/device_system.go:39).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@protocol/amneziawg/outbound.go` around lines 78 - 82, The CreateDialer
callback uses common.Must1 which will panic at runtime if dialer.NewDefault
returns an error, crashing the application. To fix this, change the callback
signature from returning only N.Dialer to returning (N.Dialer, error) in both
the EndpointOptions and DeviceOptions structs. Then replace the common.Must1
call with proper error handling where the callback returns the dialer and any
error from dialer.NewDefault directly, and update the call site in
device_system.go that invokes this callback to properly handle the returned
error instead of allowing a panic.
Comment on lines
+87
to
+93
| ResolvePeer: func(domain string) (netip.Addr, error) { | ||
| endpointAddresses, lookupErr := outbound.dnsRouter.Lookup(ctx, domain, outboundDialer.(dialer.ResolveDialer).QueryOptions()) | ||
| if lookupErr != nil { | ||
| return netip.Addr{}, lookupErr | ||
| } | ||
| return endpointAddresses[0], nil | ||
| }, |
There was a problem hiding this comment.
Unsafe type assertion may panic.
The type assertion outboundDialer.(dialer.ResolveDialer) will panic if outboundDialer does not implement dialer.ResolveDialer. Use the comma-ok idiom for safety.
🐛 Proposed fix
ResolvePeer: func(domain string) (netip.Addr, error) {
- endpointAddresses, lookupErr := outbound.dnsRouter.Lookup(ctx, domain, outboundDialer.(dialer.ResolveDialer).QueryOptions())
+ resolveDialer, ok := outboundDialer.(dialer.ResolveDialer)
+ if !ok {
+ return netip.Addr{}, E.New("dialer does not support DNS resolution")
+ }
+ endpointAddresses, lookupErr := outbound.dnsRouter.Lookup(ctx, domain, resolveDialer.QueryOptions())
if lookupErr != nil {
return netip.Addr{}, lookupErr
}
return endpointAddresses[0], nil
},🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@protocol/amneziawg/outbound.go` around lines 87 - 93, The type assertion
`outboundDialer.(dialer.ResolveDialer)` in the ResolvePeer function is unsafe
and will panic if outboundDialer does not implement the dialer.ResolveDialer
interface. Replace the direct type assertion with the comma-ok idiom by
assigning the result to two variables (value and ok boolean), check if the
assertion succeeded, and return an appropriate error if it fails before
attempting to call QueryOptions() on the asserted type.
Comment on lines
+205
to
+213
| func (w *stackDevice) Close() error { | ||
| close(w.done) | ||
| close(w.events) | ||
| w.stack.Close() | ||
| for _, endpoint := range w.stack.CleanupEndpoints() { | ||
| endpoint.Abort() | ||
| } | ||
| w.stack.Wait() | ||
| return nil |
There was a problem hiding this comment.
Close() should be idempotent to avoid teardown panics.
Directly closing done/events without a guard can panic on repeated close paths.
Suggested fix
import (
"context"
"net"
"os"
+ "sync"
@@
type stackDevice struct {
stack *stack.Stack
mtu uint32
events chan wgTun.Event
outbound chan *stack.PacketBuffer
done chan struct{}
+ closeOnce sync.Once
dispatcher stack.NetworkDispatcher
addr4 tcpip.Address
addr6 tcpip.Address
}
@@
func (w *stackDevice) Close() error {
- close(w.done)
- close(w.events)
- w.stack.Close()
- for _, endpoint := range w.stack.CleanupEndpoints() {
- endpoint.Abort()
- }
- w.stack.Wait()
+ w.closeOnce.Do(func() {
+ close(w.done)
+ close(w.events)
+ w.stack.Close()
+ for _, endpoint := range w.stack.CleanupEndpoints() {
+ endpoint.Abort()
+ }
+ w.stack.Wait()
+ })
return nil
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (w *stackDevice) Close() error { | |
| close(w.done) | |
| close(w.events) | |
| w.stack.Close() | |
| for _, endpoint := range w.stack.CleanupEndpoints() { | |
| endpoint.Abort() | |
| } | |
| w.stack.Wait() | |
| return nil | |
| func (w *stackDevice) Close() error { | |
| w.closeOnce.Do(func() { | |
| close(w.done) | |
| close(w.events) | |
| w.stack.Close() | |
| for _, endpoint := range w.stack.CleanupEndpoints() { | |
| endpoint.Abort() | |
| } | |
| w.stack.Wait() | |
| }) | |
| return nil | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/device_stack.go` around lines 205 - 213, The Close method
in the stackDevice type is not idempotent and will panic if called multiple
times because closing an already-closed channel causes a panic in Go. To fix
this, add a guard mechanism (such as using sync.Once or a boolean flag) to
ensure that the channel close operations on done and events only execute once,
and wrap the close logic accordingly. This will allow Close to be safely called
multiple times without triggering panics on repeated close attempts.
Comment on lines
+76
to
+81
| if len(rawPeer.Reserved) > 0 { | ||
| if len(rawPeer.Reserved) != 3 { | ||
| return nil, E.New("invalid reserved value for peer ", peerIndex, ", required 3 bytes, got ", len(peer.reserved)) | ||
| } | ||
| copy(peer.reserved[:], rawPeer.Reserved[:]) | ||
| } |
There was a problem hiding this comment.
Bug: Error message references wrong variable.
Line 78 uses len(peer.reserved) which always evaluates to 3 (the size of the target [3]uint8 array), making this validation ineffective. It should reference len(rawPeer.Reserved).
🐛 Proposed fix
if len(rawPeer.Reserved) > 0 {
if len(rawPeer.Reserved) != 3 {
- return nil, E.New("invalid reserved value for peer ", peerIndex, ", required 3 bytes, got ", len(peer.reserved))
+ return nil, E.New("invalid reserved value for peer ", peerIndex, ", required 3 bytes, got ", len(rawPeer.Reserved))
}
copy(peer.reserved[:], rawPeer.Reserved[:])
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if len(rawPeer.Reserved) > 0 { | |
| if len(rawPeer.Reserved) != 3 { | |
| return nil, E.New("invalid reserved value for peer ", peerIndex, ", required 3 bytes, got ", len(peer.reserved)) | |
| } | |
| copy(peer.reserved[:], rawPeer.Reserved[:]) | |
| } | |
| if len(rawPeer.Reserved) > 0 { | |
| if len(rawPeer.Reserved) != 3 { | |
| return nil, E.New("invalid reserved value for peer ", peerIndex, ", required 3 bytes, got ", len(rawPeer.Reserved)) | |
| } | |
| copy(peer.reserved[:], rawPeer.Reserved[:]) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/endpoint.go` around lines 76 - 81, In the reserved value
validation block for peers, the error message on line 78 incorrectly references
len(peer.reserved) which always returns 3 (the fixed size of the [3]uint8
array), making the error message ineffective for debugging. Change the error
message to reference len(rawPeer.Reserved) instead, which represents the actual
length of the input data being validated against the required 3 bytes.
Comment on lines
+161
to
+188
| err := e.tunDevice.Start() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| logger := &device.Logger{ | ||
| Verbosef: func(format string, args ...interface{}) { | ||
| e.options.Logger.Debug(fmt.Sprintf(strings.ToLower(format), args...)) | ||
| }, | ||
| Errorf: func(format string, args ...interface{}) { | ||
| e.options.Logger.Error(fmt.Sprintf(strings.ToLower(format), args...)) | ||
| }, | ||
| } | ||
| wgDevice := device.NewDevice(e.tunDevice, bind, logger) | ||
| e.tunDevice.SetDevice(wgDevice) | ||
| ipcConf := e.ipcConf | ||
| for _, peer := range e.peers { | ||
| ipcConf += peer.GenerateIpcLines() | ||
| } | ||
| err = wgDevice.IpcSet(ipcConf) | ||
| if err != nil { | ||
| return E.Cause(err, "setup wireguard: \n", ipcConf) | ||
| } | ||
| e.device = wgDevice | ||
| e.pause = service.FromContext[pause.Manager](e.options.Context) | ||
| if e.pause != nil { | ||
| e.pauseCallback = e.pause.RegisterCallback(e.onPauseUpdated) | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Resource leak on failure after tunDevice.Start().
If wgDevice.IpcSet (line 179) fails, the started tunDevice and newly created wgDevice are not cleaned up, leaving resources in an inconsistent state.
🐛 Proposed fix with cleanup on failure
err := e.tunDevice.Start()
if err != nil {
return err
}
logger := &device.Logger{
Verbosef: func(format string, args ...interface{}) {
e.options.Logger.Debug(fmt.Sprintf(strings.ToLower(format), args...))
},
Errorf: func(format string, args ...interface{}) {
e.options.Logger.Error(fmt.Sprintf(strings.ToLower(format), args...))
},
}
wgDevice := device.NewDevice(e.tunDevice, bind, logger)
e.tunDevice.SetDevice(wgDevice)
ipcConf := e.ipcConf
for _, peer := range e.peers {
ipcConf += peer.GenerateIpcLines()
}
err = wgDevice.IpcSet(ipcConf)
if err != nil {
+ wgDevice.Close()
- return E.Cause(err, "setup wireguard: \n", ipcConf)
+ return E.Cause(err, "setup wireguard")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| err := e.tunDevice.Start() | |
| if err != nil { | |
| return err | |
| } | |
| logger := &device.Logger{ | |
| Verbosef: func(format string, args ...interface{}) { | |
| e.options.Logger.Debug(fmt.Sprintf(strings.ToLower(format), args...)) | |
| }, | |
| Errorf: func(format string, args ...interface{}) { | |
| e.options.Logger.Error(fmt.Sprintf(strings.ToLower(format), args...)) | |
| }, | |
| } | |
| wgDevice := device.NewDevice(e.tunDevice, bind, logger) | |
| e.tunDevice.SetDevice(wgDevice) | |
| ipcConf := e.ipcConf | |
| for _, peer := range e.peers { | |
| ipcConf += peer.GenerateIpcLines() | |
| } | |
| err = wgDevice.IpcSet(ipcConf) | |
| if err != nil { | |
| return E.Cause(err, "setup wireguard: \n", ipcConf) | |
| } | |
| e.device = wgDevice | |
| e.pause = service.FromContext[pause.Manager](e.options.Context) | |
| if e.pause != nil { | |
| e.pauseCallback = e.pause.RegisterCallback(e.onPauseUpdated) | |
| } | |
| return nil | |
| err := e.tunDevice.Start() | |
| if err != nil { | |
| return err | |
| } | |
| logger := &device.Logger{ | |
| Verbosef: func(format string, args ...interface{}) { | |
| e.options.Logger.Debug(fmt.Sprintf(strings.ToLower(format), args...)) | |
| }, | |
| Errorf: func(format string, args ...interface{}) { | |
| e.options.Logger.Error(fmt.Sprintf(strings.ToLower(format), args...)) | |
| }, | |
| } | |
| wgDevice := device.NewDevice(e.tunDevice, bind, logger) | |
| e.tunDevice.SetDevice(wgDevice) | |
| ipcConf := e.ipcConf | |
| for _, peer := range e.peers { | |
| ipcConf += peer.GenerateIpcLines() | |
| } | |
| err = wgDevice.IpcSet(ipcConf) | |
| if err != nil { | |
| wgDevice.Close() | |
| return E.Cause(err, "setup wireguard") | |
| } | |
| e.device = wgDevice | |
| e.pause = service.FromContext[pause.Manager](e.options.Context) | |
| if e.pause != nil { | |
| e.pauseCallback = e.pause.RegisterCallback(e.onPauseUpdated) | |
| } | |
| return nil |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/endpoint.go` around lines 161 - 188, The code starts the
tunDevice with e.tunDevice.Start() and creates a wgDevice with
device.NewDevice(), but if wgDevice.IpcSet() fails and returns an error, these
resources are not cleaned up before returning. Add proper cleanup logic (such as
calling appropriate Close or Stop methods on both wgDevice and tunDevice) in the
error handling path after wgDevice.IpcSet() fails, or use a defer statement
earlier in the function to ensure cleanup happens regardless of the error path
taken.
Comment on lines
+179
to
+182
| err = wgDevice.IpcSet(ipcConf) | ||
| if err != nil { | ||
| return E.Cause(err, "setup wireguard: \n", ipcConf) | ||
| } |
There was a problem hiding this comment.
Avoid logging the full IPC config in error messages.
The ipcConf string contains the private key in hex format. Including it in error messages risks exposing sensitive cryptographic material through logs or error reports.
🔒 Proposed fix
err = wgDevice.IpcSet(ipcConf)
if err != nil {
- return E.Cause(err, "setup wireguard: \n", ipcConf)
+ return E.Cause(err, "setup wireguard")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| err = wgDevice.IpcSet(ipcConf) | |
| if err != nil { | |
| return E.Cause(err, "setup wireguard: \n", ipcConf) | |
| } | |
| err = wgDevice.IpcSet(ipcConf) | |
| if err != nil { | |
| return E.Cause(err, "setup wireguard") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/endpoint.go` around lines 179 - 182, The error handling
in the wgDevice.IpcSet call includes the ipcConf parameter in the E.Cause error
message, which contains sensitive private key material. Remove the ipcConf
argument from the E.Cause call and keep only the descriptive error message
"setup wireguard" to prevent exposing cryptographic material in error logs or
reports.
Comment on lines
+205
to
+213
| func (e *Endpoint) Close() error { | ||
| if e.device != nil { | ||
| e.device.Close() | ||
| } | ||
| if e.pauseCallback != nil { | ||
| e.pause.UnregisterCallback(e.pauseCallback) | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Close does not release tunDevice.
e.tunDevice is created in NewEndpoint but never closed. This leaks the underlying TUN device resources. Also, reversing the close order (pause callback before device) ensures callbacks don't fire during shutdown.
🐛 Proposed fix
func (e *Endpoint) Close() error {
+ if e.pauseCallback != nil {
+ e.pause.UnregisterCallback(e.pauseCallback)
+ }
if e.device != nil {
e.device.Close()
}
- if e.pauseCallback != nil {
- e.pause.UnregisterCallback(e.pauseCallback)
- }
- return nil
+ return e.tunDevice.Close()
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/endpoint.go` around lines 205 - 213, The Close method in
Endpoint leaks the TUN device resource because e.tunDevice is never closed, and
the shutdown order is incorrect. Add a check to close e.tunDevice if it is not
nil before closing e.device, and reorder the operations so that e.pauseCallback
is unregistered from e.pause first (before closing the device) to prevent
callbacks from firing during shutdown. This ensures proper resource cleanup and
safe shutdown sequencing.
Comment on lines
+215
to
+222
| func (e *Endpoint) onPauseUpdated(event int) { | ||
| switch event { | ||
| case pause.EventDevicePaused, pause.EventNetworkPause: | ||
| e.device.Down() | ||
| case pause.EventDeviceWake, pause.EventNetworkWake: | ||
| e.device.Up() | ||
| } | ||
| } |
There was a problem hiding this comment.
Nil pointer dereference if device not started.
If Start was never called or returned early (e.g., when resolve=false and FQDN peers exist), e.device remains nil. Subsequent pause events will cause a panic.
🐛 Proposed fix
func (e *Endpoint) onPauseUpdated(event int) {
+ if e.device == nil {
+ return
+ }
switch event {
case pause.EventDevicePaused, pause.EventNetworkPause:
e.device.Down()
case pause.EventDeviceWake, pause.EventNetworkWake:
e.device.Up()
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func (e *Endpoint) onPauseUpdated(event int) { | |
| switch event { | |
| case pause.EventDevicePaused, pause.EventNetworkPause: | |
| e.device.Down() | |
| case pause.EventDeviceWake, pause.EventNetworkWake: | |
| e.device.Up() | |
| } | |
| } | |
| func (e *Endpoint) onPauseUpdated(event int) { | |
| if e.device == nil { | |
| return | |
| } | |
| switch event { | |
| case pause.EventDevicePaused, pause.EventNetworkPause: | |
| e.device.Down() | |
| case pause.EventDeviceWake, pause.EventNetworkWake: | |
| e.device.Up() | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@transport/amneziawg/endpoint.go` around lines 215 - 222, The onPauseUpdated
method can cause a nil pointer panic because e.device may not be initialized if
Start was never called or returned early. Add a nil check at the beginning of
the onPauseUpdated method to guard against calling Down() or Up() on a nil
device pointer. If e.device is nil, the method should return early without
performing any operations.
…play name - device_system_stack.go: replace the verbatim copy (which called the sagernet/wireguard-go-only Device.InputPacket, absent in amneziawg-go and thus uncompilable) with a stub that reports system-interface mixed-stack mode as unsupported. The AmneziaWG outbound runs gVisor netstack only. (Greptile P1) - Drop the Workers option/plumbing: amneziawg-go's 3-arg NewDevice has no workers parameter, so the field was silently discarded. (CodeRabbit, Greptile) - constant: add TypeAmneziaWG case to ProxyDisplayName. (CodeRabbit)
Owner
Author
|
@coderabbitai review Pushed ed45ea9 addressing the genuinely-new-code findings:
Intentionally not changed:
|
|
✏️ Learnings added
✅ Action performedReview finished.
|
Owner
Author
Status after ed45ea9Resolved (new code):
Declined — verbatim parity with upstream
These are pre-existing upstream sing-box patterns; fixing them only in the amneziawg clone would create drift from the wireguard sibling they're copied from. They should be fixed upstream in
H1–H4 Build + vet clean with libcore tags; libcore.aar builds; verified end-to-end on Android. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds an amneziawg outbound: an obfuscated WireGuard variant (AmneziaWG) that evades DPI via junk packets (Jc/Jmin/Jmax), randomized packet-size prefixes (S1-S4), magic-header obfuscation (H1-H4), and optional CPS signature packets (I1-I5). With all params zero it behaves like plain WireGuard.
Approach
Consumes
amnezia-vpn/amneziawg-goas a pristine upstream dependency (no fork) and adapts our sing-box transport instead — the divergence fromsagernet/wireguard-gois tiny (oneNewDevicesignature + a couple ofconn.Bindextras), keeping amneziawg-go independently updatable.Changes
constant.TypeAmneziaWG = "amneziawg"option.AmneziaWGOutboundOptions: WireGuard fields + AWG obfuscation knobstransport/amneziawg/: copy oftransport/wireguardwithdevice/conn/tunimports repointed at amneziawg-go (3-argNewDevice); device-stack files kept byte-identical to the wireguard sibling for maintainability. System-interface mixed-stack mode is unsupported (amneziawg-go lacksDevice.InputPacket); the outbound runs gVisor netstack mode.protocol/amneziawg/: the outbound, registering underC.TypeAmneziaWG.Test plan
go build+go vetclean with libcore build tags.Paired with NekoBox app PR hawkff/NekoBoxForAndroid#25.
Greptile Summary
This PR adds an
amneziawgoutbound type — an obfuscated WireGuard variant that evades DPI through junk packets, randomized packet sizes, and magic-header obfuscation. The implementation adapts the upstreamamneziawg-vpn/amneziawg-golibrary by copying the existingtransport/wireguardstack and repointing imports, keeping the diff from the WireGuard sibling minimal.transport/amneziawg/): near-identical copies of the WireGuard transport files withamneziawg-goimports;device_system_stack.gocorrectly returns an error for the unsupported system+gVisor mixed mode, andgenerateAmneziaWGIpcLinesinendpoint.gocorrectly emits AWG-specific IPC fields (jc/jmin/jmax/s1-s4/h1-h4/i1-i5), falling back to plain WireGuard when all are zero.include/amneziawg.goandinclude/amneziawg_stub.goare absent, soamneziawg.RegisterOutboundis never called fromOutboundRegistry()— the outbound type is invisible to the runtime regardless of build tags, making the feature entirely non-functional in the current state.stringinAmneziaWGOutboundOptionswhile the AmneziaWG spec and peer clients define them asuint32(noted in a prior review thread).Confidence Score: 4/5
Not safe to merge in current state — the outbound type is never registered and will be silently unavailable at runtime.
The transport layer and outbound implementation are logically sound and closely mirror the battle-tested WireGuard sibling, but the PR omits the include/amneziawg.go + include/amneziawg_stub.go registration pair that every other optionally-compiled outbound requires. Without these files, amneziawg.RegisterOutbound is never called from OutboundRegistry(), so any config referencing type=amneziawg will fail to load with an unknown-type error.
include/registry.go and the missing include/amneziawg.go / include/amneziawg_stub.go files need attention; the rest of the transport stack is a clean, mechanical adaptation of the WireGuard sibling.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD Config["Config: type=amneziawg"] --> OutboundRegistry OutboundRegistry["OutboundRegistry()"] -->|"registerWireGuardOutbound ✓"| WG["wireguard.RegisterOutbound"] OutboundRegistry -->|"amneziawg.RegisterOutbound ✗ MISSING"| MISS["❌ Not registered\n(no include/amneziawg.go)"] Config2["Config loaded"] --> NewOutbound["amneziawg.NewOutbound"] NewOutbound --> NewEndpoint["amneziawg.NewEndpoint"] NewEndpoint --> NewDevice["transport/amneziawg.NewDevice"] NewDevice -->|"System=false (default)"| StackDevice["newStackDevice\n(gVisor netstack)"] NewDevice -->|"System=true, Handler=nil"| SysDevice["newSystemDevice\n(kernel TUN)"] NewDevice -->|"System=true, Handler≠nil"| ErrDevice["❌ Error: unsupported\n(no InputPacket in amneziawg-go)"] StackDevice --> Start["Endpoint.Start()"] SysDevice --> Start Start --> IpcSet["device.IpcSet(ipcConf)\nwith AWG params: jc/jmin/jmax/s1-s4/h1-h4/i1-i5"] IpcSet --> Running["AmneziaWG tunnel running"]%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%% flowchart TD Config["Config: type=amneziawg"] --> OutboundRegistry OutboundRegistry["OutboundRegistry()"] -->|"registerWireGuardOutbound ✓"| WG["wireguard.RegisterOutbound"] OutboundRegistry -->|"amneziawg.RegisterOutbound ✗ MISSING"| MISS["❌ Not registered\n(no include/amneziawg.go)"] Config2["Config loaded"] --> NewOutbound["amneziawg.NewOutbound"] NewOutbound --> NewEndpoint["amneziawg.NewEndpoint"] NewEndpoint --> NewDevice["transport/amneziawg.NewDevice"] NewDevice -->|"System=false (default)"| StackDevice["newStackDevice\n(gVisor netstack)"] NewDevice -->|"System=true, Handler=nil"| SysDevice["newSystemDevice\n(kernel TUN)"] NewDevice -->|"System=true, Handler≠nil"| ErrDevice["❌ Error: unsupported\n(no InputPacket in amneziawg-go)"] StackDevice --> Start["Endpoint.Start()"] SysDevice --> Start Start --> IpcSet["device.IpcSet(ipcConf)\nwith AWG params: jc/jmin/jmax/s1-s4/h1-h4/i1-i5"] IpcSet --> Running["AmneziaWG tunnel running"]Comments Outside Diff (1)
constant/proxy.go, line 68-70 (link)TypeAmneziaWGis missing from theString()switch. Any log line, UI element, or diagnostic that calls this function for an AmneziaWG outbound will display "Unknown" instead of a recognizable name.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "fix: stub unsupported system-stack mode;..." | Re-trigger Greptile