fix(ldap): verify StartTLS certificates; keep admin binds encrypted

internal/auth/ldap.go

@@ -2,8 +2,10 @@ package auth
 
 import (
 	"crypto/tls"
+	"crypto/x509"
 	"errors"
 	"fmt"
+	"net/url"
 	"strconv"
 	"strings"
 	"time"
@@ -52,15 +54,30 @@ type LDAPResult struct {
 	Disabled bool
 }
 
+// ldapRootCAs is the trust pool for directory certificates; nil means the system
+// pool (which honours SSL_CERT_FILE). Tests point it at their own CA.
+var ldapRootCAs *x509.CertPool
+
+// ldapTLSConfig returns the TLS settings for a connection to the server in rawURL.
+// ServerName must be set explicitly: StartTLS wraps an already-open socket with
+// tls.Client, which (unlike tls.Dial for ldaps://) does not derive it from the
+// address, and without it every verified handshake fails with "either ServerName
+// or InsecureSkipVerify must be specified in the tls.Config".
+func ldapTLSConfig(rawURL string, skipVerify bool) *tls.Config {
+	cfg := &tls.Config{InsecureSkipVerify: skipVerify, RootCAs: ldapRootCAs}
+	if u, err := url.Parse(rawURL); err == nil {
+		cfg.ServerName = u.Hostname()
+	}
+	return cfg
+}
+
 func LDAPConnect(cfg *LDAPConfig) (*ldap.Conn, error) {
 	var conn *ldap.Conn
 	var err error
 
 	if cfg.UseTLS {
 		// ldaps:// — implicit TLS from the first byte.
-		conn, err = ldap.DialURL(cfg.URL, ldap.DialWithTLSConfig(&tls.Config{
-			InsecureSkipVerify: cfg.SkipTLSVerify,
-		}))
+		conn, err = ldap.DialURL(cfg.URL, ldap.DialWithTLSConfig(ldapTLSConfig(cfg.URL, cfg.SkipTLSVerify)))
 		if err != nil {
 			return nil, fmt.Errorf("ldap connect: %w", err)
 		}
@@ -78,7 +95,7 @@ func LDAPConnect(cfg *LDAPConfig) (*ldap.Conn, error) {
 	if cfg.AllowInsecure {
 		return conn, nil
 	}
-	if err := conn.StartTLS(&tls.Config{InsecureSkipVerify: cfg.SkipTLSVerify}); err != nil {
+	if err := conn.StartTLS(ldapTLSConfig(cfg.URL, cfg.SkipTLSVerify)); err != nil {
 		conn.Close()
 		return nil, fmt.Errorf("ldap StartTLS upgrade failed; refusing cleartext bind (use ldaps:// or set allow_insecure): %w", err)
 	}

internal/auth/ldap_tls_test.go

@@ -0,0 +1,96 @@
+package auth
+
+import (
+	"net"
+	"strings"
+	"testing"
+
+	"simpleauth/internal/ldaptest"
+)
+
+func TestLDAPTLSConfigServerName(t *testing.T) {
+	cases := []struct{ url, want string }{
+		{"ldap://dc01.corp.local:389", "dc01.corp.local"},
+		{"ldap://dc01.corp.local", "dc01.corp.local"},
+		{"ldaps://dc01.corp.local:636", "dc01.corp.local"},
+		{"ldap://10.0.0.5:389", "10.0.0.5"},
+		{"ldap://[2001:db8::1]:389", "2001:db8::1"},
+	}
+	for _, c := range cases {
+		if got := ldapTLSConfig(c.url, false).ServerName; got != c.want {
+			t.Errorf("ldapTLSConfig(%q).ServerName = %q, want %q", c.url, got, c.want)
+		}
+	}
+	if !ldapTLSConfig("ldap://dc01", true).InsecureSkipVerify {
+		t.Error("skip_tls_verify not carried into the TLS config")
+	}
+}
+
+// trustCA makes LDAPConnect verify directory certificates against ca for this test.
+func trustCA(t *testing.T, ca *ldaptest.CA) {
+	t.Helper()
+	ldapRootCAs = ca.Pool
+	t.Cleanup(func() { ldapRootCAs = nil })
+}
+
+// Regression: StartTLS used a tls.Config without ServerName, so with certificate
+// verification on (the default) every upgrade failed with "either ServerName or
+// InsecureSkipVerify must be specified" and ldap:// could never be used securely.
+func TestLDAPConnectStartTLSVerifiesCertificate(t *testing.T) {
+	ca := ldaptest.NewCA(t)
+	trustCA(t, ca)
+	cert := ca.Issue(t, nil, []net.IP{net.ParseIP("127.0.0.1")})
+	srv := ldaptest.StartTLSServer(t, &cert)
+
+	conn, err := LDAPConnect(&LDAPConfig{URL: "ldap://" + srv.Addr})
+	if err != nil {
+		t.Fatalf("StartTLS with a trusted certificate failed: %v", err)
+	}
+	defer conn.Close()
+	state, ok := conn.TLSConnectionState()
+	if !ok || !state.HandshakeComplete {
+		t.Fatal("connection was not upgraded to TLS")
+	}
+	if ops := srv.CleartextOps(); len(ops) != 1 || ops[0] != ldaptest.TagExtendedRequest {
+		t.Errorf("operations sent in cleartext = %x, want only the StartTLS request", ops)
+	}
+}
+
+func TestLDAPConnectStartTLSRejectsWrongHost(t *testing.T) {
+	ca := ldaptest.NewCA(t)
+	trustCA(t, ca)
+	cert := ca.Issue(t, []string{"dc01.other.test"}, nil)
+	srv := ldaptest.StartTLSServer(t, &cert)
+
+	conn, err := LDAPConnect(&LDAPConfig{URL: "ldap://" + srv.Addr})
+	if err == nil {
+		conn.Close()
+		t.Fatal("StartTLS accepted a certificate issued for another host")
+	}
+	if !strings.Contains(err.Error(), "refusing cleartext bind") || !strings.Contains(err.Error(), "certificate") {
+		t.Errorf("unexpected error: %v", err)
+	}
+}
+
+func TestLDAPConnectStartTLSSkipVerify(t *testing.T) {
+	cert := ldaptest.NewCA(t).Issue(t, []string{"dc01.other.test"}, nil)
+	srv := ldaptest.StartTLSServer(t, &cert)
+
+	conn, err := LDAPConnect(&LDAPConfig{URL: "ldap://" + srv.Addr, SkipTLSVerify: true})
+	if err != nil {
+		t.Fatalf("StartTLS with skip_tls_verify failed: %v", err)
+	}
+	conn.Close()
+}
+
+func TestLDAPConnectLDAPSVerifiesCertificate(t *testing.T) {
+	ca := ldaptest.NewCA(t)
+	trustCA(t, ca)
+	srv := ldaptest.StartLDAPSServer(t, ca.Issue(t, nil, []net.IP{net.ParseIP("127.0.0.1")}))
+
+	conn, err := LDAPConnect(&LDAPConfig{URL: "ldaps://" + srv.Addr, UseTLS: true})
+	if err != nil {
+		t.Fatalf("ldaps:// with a trusted certificate failed: %v", err)
+	}
+	conn.Close()
+}

internal/handler/admin_kerberos.go

@@ -11,6 +11,8 @@ import (
 
 	ldaplib "github.com/go-ldap/ldap/v3"
 	"github.com/jcmturner/gokrb5/v8/keytab"
+
+	"simpleauth/internal/auth"
 )
 
 // KerberosConfig is stored in BoltDB config bucket as JSON under key "kerberos:config".
@@ -103,7 +105,7 @@ func (h *Handler) handleSetupKerberos(w http.ResponseWriter, r *http.Request) {
 	spn := "HTTP/" + req.ServiceHostname
 
 	// Connect to LDAP
-	conn, err := ldaplib.DialURL(p.URL)
+	conn, err := auth.LDAPConnect(ldapConfigFromStore(p))
 	if err != nil {
 		jsonError(w, fmt.Sprintf("failed to connect to LDAP: %v", err), http.StatusBadGateway)
 		return
@@ -218,7 +220,7 @@ func (h *Handler) handleCleanupKerberos(w http.ResponseWriter, r *http.Request)
 
 	// Try to remove SPN from AD
 	if req.Username != "" && req.Password != "" {
-		conn, err := ldaplib.DialURL(p.URL)
+		conn, err := auth.LDAPConnect(ldapConfigFromStore(p))
 		if err == nil {
 			defer conn.Close()
 			if err := conn.Bind(req.Username, req.Password); err == nil {
@@ -333,7 +335,7 @@ func (h *Handler) autoSetupKerberos(serviceHostname string, r *http.Request) (ma
 
 	spn := "HTTP/" + serviceHostname
 
-	conn, err := ldaplib.DialURL(p.URL)
+	conn, err := auth.LDAPConnect(ldapConfigFromStore(p))
 	if err != nil {
 		return nil, fmt.Errorf("failed to connect to LDAP: %v", err)
 	}

internal/handler/admin_ldap.go

@@ -263,8 +263,10 @@ func (h *Handler) handleAutoDiscoverLDAP(w http.ResponseWriter, r *http.Request)
 		}
 	}
 
-	// Step 1: Connect
-	conn, err := ldaplib.DialURL(serverURL)
+	// Step 1: Connect with the same TLS policy the saved config will have
+	// (ldaps://, or ldap:// upgraded with StartTLS), so the service-account
+	// password is never sent in cleartext during discovery.
+	conn, err := auth.LDAPConnect(&auth.LDAPConfig{URL: serverURL, UseTLS: strings.HasPrefix(serverURL, "ldaps://")})
 	if err != nil {
 		jsonError(w, fmt.Sprintf("failed to connect to %s: %v", serverURL, err), http.StatusBadGateway)
 		return

internal/handler/ldap_tls_policy_test.go

@@ -0,0 +1,56 @@
+package handler
+
+import (
+	"net/http"
+	"strings"
+	"testing"
+
+	"simpleauth/internal/ldaptest"
+)
+
+// assertNoCleartextOps fails when anything but the StartTLS request reached the
+// directory before TLS — a search or, worse, a bind carrying a password.
+func assertNoCleartextOps(t *testing.T, srv *ldaptest.Server) {
+	t.Helper()
+	for _, op := range srv.CleartextOps() {
+		if op != ldaptest.TagExtendedRequest {
+			t.Errorf("LDAP operation 0x%x was sent in cleartext", op)
+		}
+	}
+}
+
+// The console's "Connect" (auto-discover) used to dial ldap:// and bind without
+// StartTLS, sending the service-account password in cleartext and reporting
+// success for a config that then failed every login.
+func TestAutoDiscoverDoesNotBindInCleartext(t *testing.T) {
+	h, _ := testSetup(t)
+	srv := ldaptest.StartTLSServer(t, nil) // directory that refuses StartTLS
+
+	w := doJSON(h, "POST", "/api/admin/ldap/auto-discover", map[string]string{
+		"server": "ldap://" + srv.Addr, "username": "svc@corp.local", "password": "secret",
+	}, adminHeaders())
+
+	if w.Code != http.StatusBadGateway || !strings.Contains(w.Body.String(), "StartTLS") {
+		t.Errorf("auto-discover = %d %s, want 502 with the StartTLS error", w.Code, w.Body.String())
+	}
+	assertNoCleartextOps(t, srv)
+}
+
+func TestSetupKerberosDoesNotBindInCleartext(t *testing.T) {
+	h, _ := testSetup(t)
+	srv := ldaptest.StartTLSServer(t, nil)
+
+	w := doJSON(h, "PUT", "/api/admin/ldap", map[string]interface{}{
+		"url": "ldap://" + srv.Addr, "base_dn": "DC=corp,DC=local",
+		"bind_dn": "CN=svc,DC=corp,DC=local", "bind_password": "secret",
+	}, adminHeaders())
+	if w.Code != http.StatusOK {
+		t.Fatalf("save ldap config: %d %s", w.Code, w.Body.String())
+	}
+
+	w = doJSON(h, "POST", "/api/admin/ldap/setup-kerberos", map[string]string{"service_hostname": "auth.corp.local"}, adminHeaders())
+	if w.Code != http.StatusBadGateway || !strings.Contains(w.Body.String(), "StartTLS") {
+		t.Errorf("setup-kerberos = %d %s, want 502 with the StartTLS error", w.Code, w.Body.String())
+	}
+	assertNoCleartextOps(t, srv)
+}