From bab39393f187e4da2ce6a4947ad195761fb550cb Mon Sep 17 00:00:00 2001 From: n0ctal <4c866w5fn9@privaterelay.appleid.com> Date: Sun, 16 Aug 2026 00:31:31 +0500 Subject: [PATCH] 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 <293235942+n0ctal@users.noreply.github.com> --- internal/web/service/node_mtls.go | 16 +--- internal/web/service/setting_mtls.go | 40 +++++++++- .../web/service/setting_mtls_bundle_test.go | 74 +++++++++++++++++++ 3 files changed, 116 insertions(+), 14 deletions(-) create mode 100644 internal/web/service/setting_mtls_bundle_test.go diff --git a/internal/web/service/node_mtls.go b/internal/web/service/node_mtls.go index 18bb1f77b..242eedce1 100644 --- a/internal/web/service/node_mtls.go +++ b/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) diff --git a/internal/web/service/setting_mtls.go b/internal/web/service/setting_mtls.go index e6c9abb48..d62812cd6 100644 --- a/internal/web/service/setting_mtls.go +++ b/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 +} diff --git a/internal/web/service/setting_mtls_bundle_test.go b/internal/web/service/setting_mtls_bundle_test.go new file mode 100644 index 000000000..c1dface5f --- /dev/null +++ b/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) + } +}