fix(inbounds): reject missing TLS certificates before saving (#6429)

An inbound could be saved with security "tls" and a certificate row carrying neither a file path nor inline content. Nothing rejected it, so the row reached xray-core, whose readFileOrString fails with "both file and bytes are empty" and takes the whole config build down with it — every other inbound included. Validate the credentials on both sides of the wire. validateInboundTLSCertificates follows xray's file-over-inline precedence, requires a private key for every non-verify certificate and insists on at least one server certificate, so a verify-only CA list no longer passes as a server config. The inbound form's Zod schema enforces the same rules per field and serializes only the editor mode the operator actually used, and a failed save jumps to the Security tab naming the certificate row that broke. On update the guard is scoped to a real TLS edit. A row already stored incomplete is grandfathered: it stays editable, and only a save that breaks a previously valid block is refused. A sub-node stores whatever the master pushes, and Remote.UpdateInbound falls back to AddInbound when the node does not yet hold the tag, so a grandfathered row could otherwise never be deployed or re-seeded — the rejection is swallowed to a logger.Debug line and the node stays on a stale config while the panel shows the client as cut off. The controller now marks a node-sync request (mTLS or a node-sync token) on a per-request copy of InboundService, and the guard steps aside for it on both add and update: the row was judged where the operator acted, and a node that refuses it only falls out of sync. Operator and admin-token saves are held to the guard as before. The security union is parameterised on its tlsSettings branch instead of copied, and tlsCertUsesFiles is the one file-vs-inline inference shared by the form schema and the adapter, so the mode the editor opens in and the pair of fields the save serializes cannot drift apart.
2026-09-17 07:37:15 +00:00 · 2026-09-08 17:47:12 +02:00
parent 9f76a66dcf
commit 47d2303334
28 changed files with 697 additions and 23 deletions
@@ -34,6 +34,9 @@ import (
 type InboundService struct {
 	clientService   ClientService
 	fallbackService FallbackService
+	// FromNodeSync marks a master push: the row was validated where the operator
+	// acted, and a node that refuses it only falls out of sync.
+	FromNodeSync bool
 }

 func normalizeTrafficResetDay(day int) int {
@@ -610,6 +613,64 @@ func canonicalizeStreamNetworkKey(streamSettings string) string {
 	return string(out)
 }

+// validateInboundTLSCertificates rejects incomplete TLS credentials before a save
+// can restart Xray. File paths belong to the node, so only presence is checked.
+func validateInboundTLSCertificates(streamSettings string) error {
+	if strings.TrimSpace(streamSettings) == "" {
+		return nil
+	}
+	var stream struct {
+		Security    string          `json:"security"`
+		TLSSettings json.RawMessage `json:"tlsSettings"`
+	}
+	if err := json.Unmarshal([]byte(streamSettings), &stream); err != nil {
+		return common.NewError("Invalid inbound stream settings: ", err)
+	}
+	if !strings.EqualFold(stream.Security, "tls") {
+		return nil
+	}
+	var settings struct {
+		Certificates []struct {
+			CertificateFile string   `json:"certificateFile"`
+			KeyFile         string   `json:"keyFile"`
+			Certificate     []string `json:"certificate"`
+			Key             []string `json:"key"`
+			Usage           string   `json:"usage"`
+		} `json:"certificates"`
+	}
+	if len(stream.TLSSettings) > 0 {
+		if err := json.Unmarshal(stream.TLSSettings, &settings); err != nil {
+			return common.NewError("Invalid inbound TLS settings: ", err)
+		}
+	}
+	hasServerCertificate := false
+	for i, cert := range settings.Certificates {
+		// Match Xray's file-over-inline precedence for each credential.
+		certificate := cert.CertificateFile
+		if certificate == "" {
+			certificate = strings.Join(cert.Certificate, "\n")
+		}
+		if strings.TrimSpace(certificate) == "" {
+			return common.NewErrorf("TLS certificate %d is missing. Configure a certificate file path or certificate content before saving the inbound.", i+1)
+		}
+		if strings.EqualFold(cert.Usage, "verify") {
+			continue
+		}
+		key := cert.KeyFile
+		if key == "" {
+			key = strings.Join(cert.Key, "\n")
+		}
+		if strings.TrimSpace(key) == "" {
+			return common.NewErrorf("TLS certificate %d is missing its private key. Configure a private key file path or private key content before saving the inbound.", i+1)
+		}
+		hasServerCertificate = true
+	}
+	if !hasServerCertificate {
+		return common.NewError("TLS requires a server certificate and private key. Configure an encipherment or issue certificate before saving the inbound.")
+	}
+	return nil
+}
+
 // finalMaskRealityTcpMasks returns the stream's finalmask.tcp masks when the
 // stream uses REALITY security, or nil otherwise. A non-empty result means
 // this stream carries the finalmask+REALITY combination that panics
@@ -968,6 +1029,11 @@ func (s *InboundService) AddInbound(inbound *model.Inbound) (*model.Inbound, boo
 	inbound.TrafficResetDay = normalizeTrafficResetDay(inbound.TrafficResetDay)
 	// Normalize streamSettings based on protocol
 	s.normalizeStreamSettings(inbound)
+	if !s.FromNodeSync {
+		if err := validateInboundTLSCertificates(inbound.StreamSettings); err != nil {
+			return inbound, false, err
+		}
+	}
 	if err := validateFinalMaskRealityCombo(inbound.StreamSettings); err != nil {
 		return inbound, false, err
 	}
@@ -1520,6 +1586,15 @@ func (s *InboundService) UpdateInbound(inbound *model.Inbound) (*model.Inbound,
 	if err != nil {
 		return inbound, false, err
 	}
+	// Grandfather a row that was already stored incomplete so it stays editable;
+	// only a save that breaks a previously valid TLS block is refused.
+	if !s.FromNodeSync {
+		if err := validateInboundTLSCertificates(inbound.StreamSettings); err != nil {
+			if validateInboundTLSCertificates(oldInbound.StreamSettings) == nil {
+				return inbound, false, err
+			}
+		}
+	}
 	// Restore the stored NodeID before the port-conflict check so a node inbound
 	// stays scoped to its own node (the payload's nodeId is unreliable, often absent).
 	inbound.NodeID = oldInbound.NodeID
@@ -35,7 +35,7 @@ func durableTestInbound(nodeID *int, tag string, port int) *model.Inbound {
 		Enable:         true,
 		Port:           port,
 		Protocol:       model.VLESS,
-		StreamSettings: `{"network":"tcp","security":"tls"}`,
+		StreamSettings: `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`,
 		Settings:       `{"clients":[],"decryption":"none"}`,
 	}
 }
@@ -0,0 +1,185 @@
+package service
+
+import (
+	"reflect"
+	"strings"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/database"
+	"github.com/mhsanaei/3x-ui/v3/internal/database/model"
+)
+
+func TestValidateInboundTLSCertificates(t *testing.T) {
+	tests := []struct {
+		name           string
+		streamSettings string
+		wantErr        bool
+	}{
+		{"empty stream", "", false},
+		{"whitespace stream", " \t\n", false},
+		{"none ignores stale TLS settings", `{"security":"none","tlsSettings":{"certificates":[{}]}}`, false},
+		{"reality needs no TLS certificate", `{"security":"reality","realitySettings":{}}`, false},
+		{"missing TLS settings", `{"security":"tls"}`, true},
+		{"uppercase TLS security", `{"security":"TLS","tlsSettings":{}}`, true},
+		{"mixed-case TLS security", `{"security":"Tls","tlsSettings":{}}`, true},
+		{"null TLS settings", `{"security":"tls","tlsSettings":null}`, true},
+		{"missing certificates", `{"security":"tls","tlsSettings":{}}`, true},
+		{"null certificates", `{"security":"tls","tlsSettings":{"certificates":null}}`, true},
+		{"empty certificates", `{"security":"tls","tlsSettings":{"certificates":[]}}`, true},
+		{"null certificate row", `{"security":"tls","tlsSettings":{"certificates":[null]}}`, true},
+		{"empty default file fields", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"","keyFile":""}]}}`, true},
+		{"empty default inline fields", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":[],"key":[]}]}}`, true},
+		{"whitespace file fields", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":" \t","keyFile":" \n"}]}}`, true},
+		{"whitespace inline certificate", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":[" ","\t"],"key":["private key"]}]}}`, true},
+		{"whitespace inline key", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":["certificate"],"key":[" ","\n"]}]}}`, true},
+		{"missing private key", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem"}]}}`, true},
+		{"missing certificate", `{"security":"tls","tlsSettings":{"certificates":[{"keyFile":"/node/key.pem"}]}}`, true},
+		{"verify only", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"verify","certificateFile":"/node/ca.pem"}]}}`, true},
+		{"verify with private key still needs server certificate", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"verify","certificateFile":"/node/ca.pem","keyFile":"/node/key.pem"}]}}`, true},
+		{"issue needs private key", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"issue","certificateFile":"/node/ca.pem"}]}}`, true},
+		{"file credentials with default usage", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, false},
+		{"inline credentials", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":["certificate"],"key":["private key"]}]}}`, false},
+		{"certificate file and inline key", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","key":["private key"]}]}}`, false},
+		{"inline certificate and key file", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":["certificate"],"keyFile":"/node/key.pem"}]}}`, false},
+		{"encipherment usage", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"encipherment","certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, false},
+		{"issue usage", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"issue","certificateFile":"/node/ca.pem","keyFile":"/node/ca-key.pem"}]}}`, false},
+		{"unknown usage defaults to encipherment like Xray", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"custom","certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, false},
+		{"verify and server certificates", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"verify","certificateFile":"/node/ca.pem"},{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, false},
+		{"verify usage is case insensitive", `{"security":"tls","tlsSettings":{"certificates":[{"usage":"VERIFY","certificateFile":"/node/ca.pem"},{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, false},
+		{"empty extra certificate row", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"},{}]}}`, true},
+		{"empty extra verify certificate", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"},{"usage":"verify"}]}}`, true},
+		{"whitespace certificate file overrides inline content", `{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":" ","certificate":["certificate"],"key":["private key"]}]}}`, true},
+		{"whitespace key file overrides inline content", `{"security":"tls","tlsSettings":{"certificates":[{"certificate":["certificate"],"keyFile":" ","key":["private key"]}]}}`, true},
+		{"malformed stream", `{"security":"tls"`, true},
+		{"malformed TLS settings", `{"security":"tls","tlsSettings":"invalid"}`, true},
+		{"malformed certificate list", `{"security":"tls","tlsSettings":{"certificates":{}}}`, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			err := validateInboundTLSCertificates(tt.streamSettings)
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("validateInboundTLSCertificates() error = %v, wantErr %v", err, tt.wantErr)
+			}
+		})
+	}
+}
+
+func TestValidateInboundTLSCertificatesIdentifiesIncompleteRow(t *testing.T) {
+	err := validateInboundTLSCertificates(`{"security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"},{"certificateFile":"/node/other.pem"}]}}`)
+	if err == nil || !strings.Contains(err.Error(), "TLS certificate 2") || !strings.Contains(err.Error(), "private key") {
+		t.Fatalf("expected actionable error for the second certificate's private key, got %v", err)
+	}
+}
+
+func TestAddInboundRejectsMissingTLSCertificates(t *testing.T) {
+	setupConflictDB(t)
+	mgr := useTestRuntimeManager(t)
+	fake := &fakeNodeRuntime{}
+	mgr.SetLocalRuntimeOverride(fake)
+
+	inbound := &model.Inbound{
+		Tag:            "tls-missing-44310",
+		Enable:         true,
+		Listen:         "0.0.0.0",
+		Port:           44310,
+		Protocol:       model.VLESS,
+		StreamSettings: `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"","keyFile":""}]}}`,
+		Settings:       `{"clients":[]}`,
+	}
+	_, needRestart, err := (&InboundService{}).AddInbound(inbound)
+	if err == nil || !strings.Contains(err.Error(), "TLS") {
+		t.Fatalf("AddInbound: expected TLS validation error, got %v", err)
+	}
+	if needRestart {
+		t.Fatal("AddInbound: rejected TLS configuration requested a restart")
+	}
+	var count int64
+	if err := database.GetDB().Model(&model.Inbound{}).Count(&count).Error; err != nil {
+		t.Fatalf("count inbounds: %v", err)
+	}
+	if count != 0 {
+		t.Fatalf("AddInbound: rejected TLS configuration created %d rows", count)
+	}
+	if fake.addInbound.Load() != 0 || fake.updateInbound.Load() != 0 || fake.delInbound.Load() != 0 {
+		t.Fatal("AddInbound: rejected TLS configuration reached the runtime")
+	}
+}
+
+func TestUpdateInboundRejectsMissingTLSCertificates(t *testing.T) {
+	setupConflictDB(t)
+	mgr := useTestRuntimeManager(t)
+	fake := &fakeNodeRuntime{}
+	mgr.SetLocalRuntimeOverride(fake)
+
+	seedInboundConflict(t, "tls-existing-44311", "0.0.0.0", 44311, model.VLESS,
+		`{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`, `{"clients":[]}`)
+	var existing model.Inbound
+	if err := database.GetDB().Where("tag = ?", "tls-existing-44311").First(&existing).Error; err != nil {
+		t.Fatalf("load existing inbound: %v", err)
+	}
+	update := existing
+	update.Remark = "must not be saved"
+	update.Port = 44312
+	update.StreamSettings = `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem"}]}}`
+	_, needRestart, err := (&InboundService{}).UpdateInbound(&update)
+	if err == nil || !strings.Contains(err.Error(), "TLS") {
+		t.Fatalf("UpdateInbound: expected TLS validation error, got %v", err)
+	}
+	if needRestart {
+		t.Fatal("UpdateInbound: rejected TLS configuration requested a restart")
+	}
+	var reloaded model.Inbound
+	if err := database.GetDB().First(&reloaded, existing.Id).Error; err != nil {
+		t.Fatalf("reload existing inbound: %v", err)
+	}
+	if !reflect.DeepEqual(reloaded, existing) {
+		t.Fatal("UpdateInbound: rejected TLS configuration changed the stored inbound")
+	}
+	if fake.addInbound.Load() != 0 || fake.updateInbound.Load() != 0 || fake.delInbound.Load() != 0 {
+		t.Fatal("UpdateInbound: rejected TLS configuration reached the runtime")
+	}
+}
+
+// The panel used to seed a TLS inbound with an all-empty certificate, so rows in
+// that shape predate the guard and must stay editable — see UpdateInbound.
+func TestUpdateInboundAllowsUntouchedLegacyTLSCertificates(t *testing.T) {
+	const legacyStream = `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"","keyFile":"","certificate":[],"key":[]}]}}`
+
+	tests := []struct {
+		name           string
+		streamSettings string
+	}{
+		{"remark-only edit resends the stored block", legacyStream},
+		{"node push re-encodes the same block", `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"key":[],"certificate":[],"keyFile":"","certificateFile":""}]}}`},
+		{"a partial fix to the stored credentials is tolerated", `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem"}]}}`},
+		{"completing the credentials is accepted", `{"network":"tcp","security":"tls","tlsSettings":{"certificates":[{"certificateFile":"/node/cert.pem","keyFile":"/node/key.pem"}]}}`},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			setupConflictDB(t)
+			mgr := useTestRuntimeManager(t)
+			mgr.SetLocalRuntimeOverride(&fakeNodeRuntime{})
+
+			seedInboundConflict(t, "tls-legacy-44321", "0.0.0.0", 44321, model.VLESS, legacyStream, `{"clients":[]}`)
+			var existing model.Inbound
+			if err := database.GetDB().Where("tag = ?", "tls-legacy-44321").First(&existing).Error; err != nil {
+				t.Fatalf("load legacy inbound: %v", err)
+			}
+
+			update := existing
+			update.Remark = "renamed"
+			update.StreamSettings = tt.streamSettings
+			if _, _, err := (&InboundService{}).UpdateInbound(&update); err != nil {
+				t.Fatalf("UpdateInbound: %v", err)
+			}
+			var reloaded model.Inbound
+			if err := database.GetDB().First(&reloaded, existing.Id).Error; err != nil {
+				t.Fatalf("reload inbound: %v", err)
+			}
+			if reloaded.Remark != "renamed" {
+				t.Fatalf("UpdateInbound: remark = %q, want %q", reloaded.Remark, "renamed")
+			}
+		})
+	}
+}