fix(nodes): validate every certificate in the node mTLS trust bundle (#6188)

* fix(nodes): validate every certificate in the node mTLS trust bundle

AppendCertsFromPEM reports success once a single certificate parses, so a
trust bundle whose later entries are damaged or truncated was accepted with
those entries silently absent from the pool. Parse and validate every PEM
block instead, and reject the bundle if any of them is malformed.

* fix(mtls): reject malformed certificate bundle layout

---------

Co-authored-by: n0ctal <[email protected]>

internal/web/service/node_mtls.go

@@ -2,8 +2,6 @@ package service
 
 import (
 	"crypto/tls"
-	"crypto/x509"
-	"encoding/pem"
 	"strings"
 
 	"github.com/mhsanaei/3x-ui/v3/internal/util/common"
@@ -42,19 +40,13 @@ func (s *NodeService) ReloadMasterMtlsClient() error {
 	return nil
 }
 
-// SetNodeMtlsTrustCA stores the CA certificate this panel trusts for incoming
-// node-API client certificates. An empty value clears it (mTLS off). A
-// non-empty value must be a PEM certificate (fail closed). Takes effect on the
-// next panel restart, when the listener's ClientCAs is rebuilt.
+// SetNodeMtlsTrustCA stores the CA certificate bundle trusted for incoming
+// node-API clients. An empty value clears it; changes apply after restart.
 func (s *NodeService) SetNodeMtlsTrustCA(caPem string) error {
 	caPem = strings.TrimSpace(caPem)
 	if caPem != "" {
-		block, _ := pem.Decode([]byte(caPem))
-		if block == nil || block.Type != "CERTIFICATE" {
-			return common.NewError("trust CA must be a PEM-encoded certificate")
-		}
-		if _, err := x509.ParseCertificate(block.Bytes); err != nil {
-			return common.NewError("invalid trust CA certificate: " + err.Error())
+		if _, err := parseCertificateBundlePEM([]byte(caPem)); err != nil {
+			return common.NewError("invalid trust CA certificate bundle: ", err)
 		}
 	}
 	return (&SettingService{}).setString(settingNodeMtlsClientCA, caPem)

internal/web/service/setting_mtls.go

@@ -1,11 +1,14 @@
 package service
 
 import (
+	"bytes"
 	"crypto/sha256"
 	"crypto/tls"
 	"crypto/x509"
 	"encoding/hex"
 	"encoding/pem"
+	"errors"
+	"fmt"
 	"strings"
 	"sync"
 
@@ -189,9 +192,42 @@ func (s *SettingService) NodeMtlsClientCAPool() (*x509.CertPool, error) {
 	if caPem == "" {
 		return nil, nil
 	}
+	certs, err := parseCertificateBundlePEM([]byte(caPem))
+	if err != nil {
+		return nil, fmt.Errorf("nodeMtlsClientCAPem is not a valid certificate bundle: %w", err)
+	}
 	pool := x509.NewCertPool()
-	if !pool.AppendCertsFromPEM([]byte(caPem)) {
-		return nil, common.NewError("nodeMtlsClientCAPem is not a valid certificate")
+	for _, cert := range certs {
+		pool.AddCert(cert)
 	}
 	return pool, nil
 }
+
+// parseCertificateBundlePEM avoids AppendCertsFromPEM because that helper can
+// silently accept a bundle after parsing only its first certificate.
+func parseCertificateBundlePEM(bundle []byte) ([]*x509.Certificate, error) {
+	rest := bytes.TrimSpace(bundle)
+	if len(rest) == 0 {
+		return nil, errors.New("certificate bundle is empty")
+	}
+	certs := make([]*x509.Certificate, 0, 1)
+	for len(rest) > 0 {
+		if !bytes.HasPrefix(rest, []byte("-----BEGIN CERTIFICATE-----")) {
+			return nil, errors.New("certificate bundle contains malformed or non-PEM data")
+		}
+		block, next := pem.Decode(rest)
+		if block == nil {
+			return nil, errors.New("certificate bundle contains malformed or non-PEM data")
+		}
+		if block.Type != "CERTIFICATE" {
+			return nil, errors.New("certificate bundle contains a non-certificate PEM block")
+		}
+		cert, err := x509.ParseCertificate(block.Bytes)
+		if err != nil {
+			return nil, errors.New("certificate bundle contains an invalid certificate")
+		}
+		certs = append(certs, cert)
+		rest = bytes.TrimSpace(next)
+	}
+	return certs, nil
+}

internal/web/service/setting_mtls_bundle_test.go

@@ -0,0 +1,74 @@
+package service
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/mhsanaei/3x-ui/v3/internal/util/crypto"
+)
+
+func mustNodeCAPEM(t *testing.T, name string) string {
+	t.Helper()
+	ca, err := crypto.GenerateNodeCA(name)
+	if err != nil {
+		t.Fatalf("GenerateNodeCA(%q): %v", name, err)
+	}
+	return string(ca.CertPEM)
+}
+
+func TestParseCertificateBundlePEM(t *testing.T) {
+	first := mustNodeCAPEM(t, "bundle test CA one")
+	second := mustNodeCAPEM(t, "bundle test CA two")
+
+	corrupt := strings.Replace(second, "-----BEGIN CERTIFICATE-----\n", "-----BEGIN CERTIFICATE-----\nAA", 1)
+
+	tests := []struct {
+		name      string
+		bundle    string
+		wantCerts int
+		wantErr   string
+	}{
+		{name: "single certificate", bundle: first, wantCerts: 1},
+		{name: "two certificates", bundle: first + second, wantCerts: 2},
+		{name: "empty", bundle: "", wantErr: "certificate bundle is empty"},
+		{name: "whitespace only", bundle: "\n\t  \n", wantErr: "certificate bundle is empty"},
+		{name: "leading non-PEM data", bundle: "junk\n" + first, wantErr: "certificate bundle contains malformed or non-PEM data"},
+		{name: "interstitial non-PEM data", bundle: first + "junk\n" + second, wantErr: "certificate bundle contains malformed or non-PEM data"},
+		{name: "second certificate corrupt", bundle: first + corrupt, wantErr: "certificate bundle contains malformed or non-PEM data"},
+		{name: "trailing non-PEM data", bundle: first + "not a certificate\n", wantErr: "certificate bundle contains malformed or non-PEM data"},
+		{name: "non-certificate block", bundle: first + "-----BEGIN PRIVATE KEY-----\nAAAA\n-----END PRIVATE KEY-----\n", wantErr: "certificate bundle contains malformed or non-PEM data"},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			certs, err := parseCertificateBundlePEM([]byte(tt.bundle))
+			if tt.wantErr != "" {
+				if err == nil || err.Error() != tt.wantErr {
+					t.Fatalf("parseCertificateBundlePEM() error = %v, want %q", err, tt.wantErr)
+				}
+				return
+			}
+			if err != nil {
+				t.Fatalf("parseCertificateBundlePEM(): %v", err)
+			}
+			if len(certs) != tt.wantCerts {
+				t.Fatalf("parseCertificateBundlePEM() = %d certs, want %d", len(certs), tt.wantCerts)
+			}
+		})
+	}
+}
+
+func TestNodeMtlsClientCAPoolRejectsPartiallyValidBundle(t *testing.T) {
+	s := setupSettingMtlsDB(t)
+
+	valid := mustNodeCAPEM(t, "pool test CA")
+	if err := s.setString("nodeMtlsClientCAPem", valid+"-----BEGIN CERTIFICATE-----\nnot base64\n-----END CERTIFICATE-----\n"); err != nil {
+		t.Fatalf("setString: %v", err)
+	}
+
+	pool, err := s.NodeMtlsClientCAPool()
+	want := "nodeMtlsClientCAPem is not a valid certificate bundle: certificate bundle contains malformed or non-PEM data"
+	if err == nil || err.Error() != want {
+		t.Fatalf("NodeMtlsClientCAPool() = %v, error = %v, want %q", pool, err, want)
+	}
+}