WireGuard: Improve config error handling; Prevent panic in case of errors during server initialization (#4566)

IlyaGulya · RPRX · commit 17207fc5e4ab · 2025-03-30T22:55:21.000Z
#4566 (comment)
diff --git a/infra/conf/wireguard.go b/infra/conf/wireguard.go
@@ -67,7 +67,7 @@ func (c *WireGuardConfig) Build() (proto.Message, error) {
 	var err error
 	config.SecretKey, err = ParseWireGuardKey(c.SecretKey)
 	if err != nil {
-		return nil, err
+		return nil, errors.New("invalid WireGuard secret key: %w", err)
 	}
 
 	if c.Address == nil {
@@ -126,6 +126,10 @@ func (c *WireGuardConfig) Build() (proto.Message, error) {
 func ParseWireGuardKey(str string) (string, error) {
 	var err error
 
+	if str == "" {
+		return "", errors.New("key must not be empty")
+	}
+
 	if len(str)%2 == 0 {
 		_, err = hex.DecodeString(str)
 		if err == nil {
diff --git a/infra/conf/xray.go b/infra/conf/xray.go
@@ -241,14 +241,14 @@ func (c *InboundDetourConfig) Build() (*core.InboundHandlerConfig, error) {
 	}
 	rawConfig, err := inboundConfigLoader.LoadWithID(settings, c.Protocol)
 	if err != nil {
-		return nil, errors.New("failed to load inbound detour config.").Base(err)
+		return nil, errors.New("failed to load inbound detour config for protocol ", c.Protocol).Base(err)
 	}
 	if dokodemoConfig, ok := rawConfig.(*DokodemoConfig); ok {
 		receiverSettings.ReceiveOriginalDestination = dokodemoConfig.Redirect
 	}
 	ts, err := rawConfig.(Buildable).Build()
 	if err != nil {
-		return nil, err
+		return nil, errors.New("failed to build inbound handler for protocol ", c.Protocol).Base(err)
 	}
 
 	return &core.InboundHandlerConfig{
@@ -303,15 +303,15 @@ func (c *OutboundDetourConfig) Build() (*core.OutboundHandlerConfig, error) {
 	if c.StreamSetting != nil {
 		ss, err := c.StreamSetting.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build stream settings for outbound detour").Base(err)
 		}
 		senderSettings.StreamSettings = ss
 	}
 
 	if c.ProxySettings != nil {
 		ps, err := c.ProxySettings.Build()
 		if err != nil {
-			return nil, errors.New("invalid outbound detour proxy settings.").Base(err)
+			return nil, errors.New("invalid outbound detour proxy settings").Base(err)
 		}
 		if ps.TransportLayerProxy {
 			if senderSettings.StreamSettings != nil {
@@ -331,7 +331,7 @@ func (c *OutboundDetourConfig) Build() (*core.OutboundHandlerConfig, error) {
 	if c.MuxSettings != nil {
 		ms, err := c.MuxSettings.Build()
 		if err != nil {
-			return nil, errors.New("failed to build Mux config.").Base(err)
+			return nil, errors.New("failed to build Mux config").Base(err)
 		}
 		senderSettings.MultiplexSettings = ms
 	}
@@ -342,11 +342,11 @@ func (c *OutboundDetourConfig) Build() (*core.OutboundHandlerConfig, error) {
 	}
 	rawConfig, err := outboundConfigLoader.LoadWithID(settings, c.Protocol)
 	if err != nil {
-		return nil, errors.New("failed to parse to outbound detour config.").Base(err)
+		return nil, errors.New("failed to load outbound detour config for protocol ", c.Protocol).Base(err)
 	}
 	ts, err := rawConfig.(Buildable).Build()
 	if err != nil {
-		return nil, err
+		return nil, errors.New("failed to build outbound handler for protocol ", c.Protocol).Base(err)
 	}
 
 	return &core.OutboundHandlerConfig{
@@ -490,7 +490,7 @@ func (c *Config) Override(o *Config, fn string) {
 // Build implements Buildable.
 func (c *Config) Build() (*core.Config, error) {
 	if err := PostProcessConfigureFile(c); err != nil {
-		return nil, err
+		return nil, errors.New("failed to post-process configuration file").Base(err)
 	}
 
 	config := &core.Config{
@@ -504,21 +504,21 @@ func (c *Config) Build() (*core.Config, error) {
 	if c.API != nil {
 		apiConf, err := c.API.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build API configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(apiConf))
 	}
 	if c.Metrics != nil {
 		metricsConf, err := c.Metrics.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build metrics configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(metricsConf))
 	}
 	if c.Stats != nil {
 		statsConf, err := c.Stats.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build stats configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(statsConf))
 	}
@@ -536,55 +536,55 @@ func (c *Config) Build() (*core.Config, error) {
 	if c.RouterConfig != nil {
 		routerConfig, err := c.RouterConfig.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build routing configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(routerConfig))
 	}
 
 	if c.DNSConfig != nil {
 		dnsApp, err := c.DNSConfig.Build()
 		if err != nil {
-			return nil, errors.New("failed to parse DNS config").Base(err)
+			return nil, errors.New("failed to build DNS configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(dnsApp))
 	}
 
 	if c.Policy != nil {
 		pc, err := c.Policy.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build policy configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(pc))
 	}
 
 	if c.Reverse != nil {
 		r, err := c.Reverse.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build reverse configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(r))
 	}
 
 	if c.FakeDNS != nil {
 		r, err := c.FakeDNS.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build fake DNS configuration").Base(err)
 		}
 		config.App = append([]*serial.TypedMessage{serial.ToTypedMessage(r)}, config.App...)
 	}
 
 	if c.Observatory != nil {
 		r, err := c.Observatory.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build observatory configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(r))
 	}
 
 	if c.BurstObservatory != nil {
 		r, err := c.BurstObservatory.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build burst observatory configuration").Base(err)
 		}
 		config.App = append(config.App, serial.ToTypedMessage(r))
 	}
@@ -602,7 +602,7 @@ func (c *Config) Build() (*core.Config, error) {
 	for _, rawInboundConfig := range inbounds {
 		ic, err := rawInboundConfig.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build inbound config with tag ", rawInboundConfig.Tag).Base(err)
 		}
 		config.Inbound = append(config.Inbound, ic)
 	}
@@ -616,7 +616,7 @@ func (c *Config) Build() (*core.Config, error) {
 	for _, rawOutboundConfig := range outbounds {
 		oc, err := rawOutboundConfig.Build()
 		if err != nil {
-			return nil, err
+			return nil, errors.New("failed to build outbound config with tag ", rawOutboundConfig.Tag).Base(err)
 		}
 		config.Outbound = append(config.Outbound, oc)
 	}
diff --git a/proxy/wireguard/gvisortun/tun.go b/proxy/wireguard/gvisortun/tun.go
@@ -10,6 +10,7 @@ import (
 	"fmt"
 	"net/netip"
 	"os"
+	"sync"
 	"syscall"
 
 	"golang.zx2c4.com/wireguard/tun"
@@ -33,6 +34,7 @@ type netTun struct {
 	incomingPacket chan *buffer.View
 	mtu            int
 	hasV4, hasV6   bool
+	closeOnce      sync.Once
 }
 
 type Net netTun
@@ -174,18 +176,15 @@ func (tun *netTun) Flush() error {
 
 // Close implements tun.Device
 func (tun *netTun) Close() error {
-	tun.stack.RemoveNIC(1)
+	tun.closeOnce.Do(func() {
+		tun.stack.RemoveNIC(1)
 
-	if tun.events != nil {
 		close(tun.events)
-	}
 
-	tun.ep.Close()
+		tun.ep.Close()
 
-	if tun.incomingPacket != nil {
 		close(tun.incomingPacket)
-	}
-
+	})
 	return nil
 }
 
diff --git a/proxy/wireguard/server_test.go b/proxy/wireguard/server_test.go
@@ -0,0 +1,52 @@
+package wireguard_test
+
+import (
+	"context"
+	"github.com/stretchr/testify/assert"
+	"runtime/debug"
+	"testing"
+
+	"github.com/xtls/xray-core/core"
+	"github.com/xtls/xray-core/proxy/wireguard"
+)
+
+// TestWireGuardServerInitializationError verifies that an error during TUN initialization
+// (triggered by an empty SecretKey) in the WireGuard server does not cause a panic and returns an error instead.
+func TestWireGuardServerInitializationError(t *testing.T) {
+	// Create a minimal core instance with default features
+	config := &core.Config{}
+	instance, err := core.New(config)
+	if err != nil {
+		t.Fatalf("Failed to create core instance: %v", err)
+	}
+	// Set the Xray instance in the context
+	ctx := context.WithValue(context.Background(), core.XrayKey(1), instance)
+
+	// Define the server configuration with an empty SecretKey to trigger error
+	conf := &wireguard.DeviceConfig{
+		IsClient:  false,
+		Endpoint:  []string{"10.0.0.1/32"},
+		Mtu:       1420,
+		SecretKey: "", // Empty SecretKey to trigger error
+		Peers: []*wireguard.PeerConfig{
+			{
+				PublicKey:  "some_public_key",
+				AllowedIps: []string{"10.0.0.2/32"},
+			},
+		},
+	}
+
+	// Use defer to catch any panic and fail the test explicitly
+	defer func() {
+		if r := recover(); r != nil {
+			t.Errorf("TUN initialization panicked: %v", r)
+			debug.PrintStack()
+		}
+	}()
+
+	// Attempt to initialize the WireGuard server
+	_, err = wireguard.NewServer(ctx, conf)
+
+	// Check that an error is returned
+	assert.ErrorContains(t, err, "failed to set private_key: hex string does not fit the slice")
+}

Original file line number	Diff line number	Diff line change
`@@ -241,14 +241,14 @@ func (c InboundDetourConfig) Build() (core.InboundHandlerConfig, error) {`
`241`	`241`	`}`
`242`	`242`	`rawConfig, err := inboundConfigLoader.LoadWithID(settings, c.Protocol)`
`243`	`243`	`if err != nil {`
`244`		`- return nil, errors.New("failed to load inbound detour config.").Base(err)`
	`244`	`+ return nil, errors.New("failed to load inbound detour config for protocol ", c.Protocol).Base(err)`
`245`	`245`	`}`
`246`	`246`	`if dokodemoConfig, ok := rawConfig.(*DokodemoConfig); ok {`
`247`	`247`	`receiverSettings.ReceiveOriginalDestination = dokodemoConfig.Redirect`
`248`	`248`	`}`
`249`	`249`	`ts, err := rawConfig.(Buildable).Build()`
`250`	`250`	`if err != nil {`
`251`		`- return nil, err`
	`251`	`+ return nil, errors.New("failed to build inbound handler for protocol ", c.Protocol).Base(err)`
`252`	`252`	`}`
`253`	`253`
`254`	`254`	`return &core.InboundHandlerConfig{`
`@@ -303,15 +303,15 @@ func (c OutboundDetourConfig) Build() (core.OutboundHandlerConfig, error) {`
`303`	`303`	`if c.StreamSetting != nil {`
`304`	`304`	`ss, err := c.StreamSetting.Build()`
`305`	`305`	`if err != nil {`
`306`		`- return nil, err`
	`306`	`+ return nil, errors.New("failed to build stream settings for outbound detour").Base(err)`
`307`	`307`	`}`
`308`	`308`	`senderSettings.StreamSettings = ss`
`309`	`309`	`}`
`310`	`310`
`311`	`311`	`if c.ProxySettings != nil {`
`312`	`312`	`ps, err := c.ProxySettings.Build()`
`313`	`313`	`if err != nil {`
`314`		`- return nil, errors.New("invalid outbound detour proxy settings.").Base(err)`
	`314`	`+ return nil, errors.New("invalid outbound detour proxy settings").Base(err)`
`315`	`315`	`}`
`316`	`316`	`if ps.TransportLayerProxy {`
`317`	`317`	`if senderSettings.StreamSettings != nil {`
`@@ -331,7 +331,7 @@ func (c OutboundDetourConfig) Build() (core.OutboundHandlerConfig, error) {`
`331`	`331`	`if c.MuxSettings != nil {`
`332`	`332`	`ms, err := c.MuxSettings.Build()`
`333`	`333`	`if err != nil {`
`334`		`- return nil, errors.New("failed to build Mux config.").Base(err)`
	`334`	`+ return nil, errors.New("failed to build Mux config").Base(err)`
`335`	`335`	`}`
`336`	`336`	`senderSettings.MultiplexSettings = ms`
`337`	`337`	`}`
`@@ -342,11 +342,11 @@ func (c OutboundDetourConfig) Build() (core.OutboundHandlerConfig, error) {`
`342`	`342`	`}`
`343`	`343`	`rawConfig, err := outboundConfigLoader.LoadWithID(settings, c.Protocol)`
`344`	`344`	`if err != nil {`
`345`		`- return nil, errors.New("failed to parse to outbound detour config.").Base(err)`
	`345`	`+ return nil, errors.New("failed to load outbound detour config for protocol ", c.Protocol).Base(err)`
`346`	`346`	`}`
`347`	`347`	`ts, err := rawConfig.(Buildable).Build()`
`348`	`348`	`if err != nil {`
`349`		`- return nil, err`
	`349`	`+ return nil, errors.New("failed to build outbound handler for protocol ", c.Protocol).Base(err)`
`350`	`350`	`}`
`351`	`351`
`352`	`352`	`return &core.OutboundHandlerConfig{`
`@@ -490,7 +490,7 @@ func (c Config) Override(o Config, fn string) {`
`490`	`490`	`// Build implements Buildable.`
`491`	`491`	`func (c Config) Build() (core.Config, error) {`
`492`	`492`	`if err := PostProcessConfigureFile(c); err != nil {`
`493`		`- return nil, err`
	`493`	`+ return nil, errors.New("failed to post-process configuration file").Base(err)`
`494`	`494`	`}`
`495`	`495`
`496`	`496`	`config := &core.Config{`
`@@ -504,21 +504,21 @@ func (c Config) Build() (core.Config, error) {`
`504`	`504`	`if c.API != nil {`
`505`	`505`	`apiConf, err := c.API.Build()`
`506`	`506`	`if err != nil {`
`507`		`- return nil, err`
	`507`	`+ return nil, errors.New("failed to build API configuration").Base(err)`
`508`	`508`	`}`
`509`	`509`	`config.App = append(config.App, serial.ToTypedMessage(apiConf))`
`510`	`510`	`}`
`511`	`511`	`if c.Metrics != nil {`
`512`	`512`	`metricsConf, err := c.Metrics.Build()`
`513`	`513`	`if err != nil {`
`514`		`- return nil, err`
	`514`	`+ return nil, errors.New("failed to build metrics configuration").Base(err)`
`515`	`515`	`}`
`516`	`516`	`config.App = append(config.App, serial.ToTypedMessage(metricsConf))`
`517`	`517`	`}`
`518`	`518`	`if c.Stats != nil {`
`519`	`519`	`statsConf, err := c.Stats.Build()`
`520`	`520`	`if err != nil {`
`521`		`- return nil, err`
	`521`	`+ return nil, errors.New("failed to build stats configuration").Base(err)`
`522`	`522`	`}`
`523`	`523`	`config.App = append(config.App, serial.ToTypedMessage(statsConf))`
`524`	`524`	`}`
`@@ -536,55 +536,55 @@ func (c Config) Build() (core.Config, error) {`
`536`	`536`	`if c.RouterConfig != nil {`
`537`	`537`	`routerConfig, err := c.RouterConfig.Build()`
`538`	`538`	`if err != nil {`
`539`		`- return nil, err`
	`539`	`+ return nil, errors.New("failed to build routing configuration").Base(err)`
`540`	`540`	`}`
`541`	`541`	`config.App = append(config.App, serial.ToTypedMessage(routerConfig))`
`542`	`542`	`}`
`543`	`543`
`544`	`544`	`if c.DNSConfig != nil {`
`545`	`545`	`dnsApp, err := c.DNSConfig.Build()`
`546`	`546`	`if err != nil {`
`547`		`- return nil, errors.New("failed to parse DNS config").Base(err)`
	`547`	`+ return nil, errors.New("failed to build DNS configuration").Base(err)`
`548`	`548`	`}`
`549`	`549`	`config.App = append(config.App, serial.ToTypedMessage(dnsApp))`
`550`	`550`	`}`
`551`	`551`
`552`	`552`	`if c.Policy != nil {`
`553`	`553`	`pc, err := c.Policy.Build()`
`554`	`554`	`if err != nil {`
`555`		`- return nil, err`
	`555`	`+ return nil, errors.New("failed to build policy configuration").Base(err)`
`556`	`556`	`}`
`557`	`557`	`config.App = append(config.App, serial.ToTypedMessage(pc))`
`558`	`558`	`}`
`559`	`559`
`560`	`560`	`if c.Reverse != nil {`
`561`	`561`	`r, err := c.Reverse.Build()`
`562`	`562`	`if err != nil {`
`563`		`- return nil, err`
	`563`	`+ return nil, errors.New("failed to build reverse configuration").Base(err)`
`564`	`564`	`}`
`565`	`565`	`config.App = append(config.App, serial.ToTypedMessage(r))`
`566`	`566`	`}`
`567`	`567`
`568`	`568`	`if c.FakeDNS != nil {`
`569`	`569`	`r, err := c.FakeDNS.Build()`
`570`	`570`	`if err != nil {`
`571`		`- return nil, err`
	`571`	`+ return nil, errors.New("failed to build fake DNS configuration").Base(err)`
`572`	`572`	`}`
`573`	`573`	`config.App = append([]*serial.TypedMessage{serial.ToTypedMessage(r)}, config.App...)`
`574`	`574`	`}`
`575`	`575`
`576`	`576`	`if c.Observatory != nil {`
`577`	`577`	`r, err := c.Observatory.Build()`
`578`	`578`	`if err != nil {`
`579`		`- return nil, err`
	`579`	`+ return nil, errors.New("failed to build observatory configuration").Base(err)`
`580`	`580`	`}`
`581`	`581`	`config.App = append(config.App, serial.ToTypedMessage(r))`
`582`	`582`	`}`
`583`	`583`
`584`	`584`	`if c.BurstObservatory != nil {`
`585`	`585`	`r, err := c.BurstObservatory.Build()`
`586`	`586`	`if err != nil {`
`587`		`- return nil, err`
	`587`	`+ return nil, errors.New("failed to build burst observatory configuration").Base(err)`
`588`	`588`	`}`
`589`	`589`	`config.App = append(config.App, serial.ToTypedMessage(r))`
`590`	`590`	`}`
`@@ -602,7 +602,7 @@ func (c Config) Build() (core.Config, error) {`
`602`	`602`	`for _, rawInboundConfig := range inbounds {`
`603`	`603`	`ic, err := rawInboundConfig.Build()`
`604`	`604`	`if err != nil {`
`605`		`- return nil, err`
	`605`	`+ return nil, errors.New("failed to build inbound config with tag ", rawInboundConfig.Tag).Base(err)`
`606`	`606`	`}`
`607`	`607`	`config.Inbound = append(config.Inbound, ic)`
`608`	`608`	`}`
`@@ -616,7 +616,7 @@ func (c Config) Build() (core.Config, error) {`
`616`	`616`	`for _, rawOutboundConfig := range outbounds {`
`617`	`617`	`oc, err := rawOutboundConfig.Build()`
`618`	`618`	`if err != nil {`
`619`		`- return nil, err`
	`619`	`+ return nil, errors.New("failed to build outbound config with tag ", rawOutboundConfig.Tag).Base(err)`
`620`	`620`	`}`
`621`	`621`	`config.Outbound = append(config.Outbound, oc)`
`622`	`622`	`}`