diff --git a/internal/auth/ldap.go b/internal/auth/ldap.go index 6903eff..383e739 100644 --- a/internal/auth/ldap.go +++ b/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) } diff --git a/internal/auth/ldap_tls_test.go b/internal/auth/ldap_tls_test.go new file mode 100644 index 0000000..9bfe44f --- /dev/null +++ b/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() +} diff --git a/internal/handler/admin_kerberos.go b/internal/handler/admin_kerberos.go index d856668..6a01a83 100644 --- a/internal/handler/admin_kerberos.go +++ b/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) } diff --git a/internal/handler/admin_ldap.go b/internal/handler/admin_ldap.go index 80de2ad..919ef17 100644 --- a/internal/handler/admin_ldap.go +++ b/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 diff --git a/internal/handler/ldap_tls_policy_test.go b/internal/handler/ldap_tls_policy_test.go new file mode 100644 index 0000000..11778d1 --- /dev/null +++ b/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) +} diff --git a/internal/ldaptest/ldaptest.go b/internal/ldaptest/ldaptest.go new file mode 100644 index 0000000..ce2528d --- /dev/null +++ b/internal/ldaptest/ldaptest.go @@ -0,0 +1,222 @@ +// Package ldaptest is a minimal LDAP server for tests of the connection/TLS layer. +// It speaks just enough of the protocol to answer a StartTLS extended request; it +// records the tag of every operation the client sends before TLS is established, so +// tests can assert that no bind or search ever crosses the wire in cleartext. +package ldaptest + +import ( + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/tls" + "crypto/x509" + "crypto/x509/pkix" + "io" + "math/big" + "net" + "sync" + "testing" + "time" +) + +// LDAP protocol-op tags (RFC 4511, [APPLICATION n]). +const ( + TagBindRequest = 0x60 + TagSearchRequest = 0x63 + TagExtendedRequest = 0x77 +) + +// Server is a fake directory listening on 127.0.0.1. +type Server struct { + Addr string + + mu sync.Mutex + cleartext []byte + tlsUpgraded bool +} + +// CleartextOps returns the tags of the operations received before TLS was set up. +func (s *Server) CleartextOps() []byte { + s.mu.Lock() + defer s.mu.Unlock() + return append([]byte(nil), s.cleartext...) +} + +// TLSUpgraded reports whether a StartTLS handshake completed. +func (s *Server) TLSUpgraded() bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.tlsUpgraded +} + +// StartTLSServer starts a plain ldap:// server. With cert set it accepts StartTLS and +// completes the TLS handshake; with cert nil it refuses StartTLS (protocolError). +// Any other first operation closes the connection. +func StartTLSServer(t *testing.T, cert *tls.Certificate) *Server { + t.Helper() + ln, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { ln.Close() }) + s := &Server{Addr: ln.Addr().String()} + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + go s.serve(c, cert) + } + }() + return s +} + +func (s *Server) serve(c net.Conn, cert *tls.Certificate) { + defer c.Close() + c.SetDeadline(time.Now().Add(5 * time.Second)) + id, tag, err := readMessage(c) + if err != nil { + return + } + s.mu.Lock() + s.cleartext = append(s.cleartext, tag) + s.mu.Unlock() + if tag != TagExtendedRequest { + return + } + if cert == nil { + c.Write(extendedResponse(id, 2)) // protocolError: StartTLS not supported + return + } + c.Write(extendedResponse(id, 0)) + tc := tls.Server(c, &tls.Config{Certificates: []tls.Certificate{*cert}}) + if err := tc.Handshake(); err != nil { + return + } + s.mu.Lock() + s.tlsUpgraded = true + s.mu.Unlock() + io.Copy(io.Discard, tc) +} + +// StartLDAPSServer starts an ldaps:// server (TLS from the first byte) that only +// completes the handshake. +func StartLDAPSServer(t *testing.T, cert tls.Certificate) *Server { + t.Helper() + ln, err := tls.Listen("tcp", "127.0.0.1:0", &tls.Config{Certificates: []tls.Certificate{cert}}) + if err != nil { + t.Fatalf("listen: %v", err) + } + t.Cleanup(func() { ln.Close() }) + s := &Server{Addr: ln.Addr().String()} + go func() { + for { + c, err := ln.Accept() + if err != nil { + return + } + go func() { + defer c.Close() + c.SetDeadline(time.Now().Add(5 * time.Second)) + if err := c.(*tls.Conn).Handshake(); err == nil { + s.mu.Lock() + s.tlsUpgraded = true + s.mu.Unlock() + } + io.Copy(io.Discard, c) + }() + } + }() + return s +} + +// readMessage reads one LDAPMessage and returns its message ID bytes and protocol-op tag. +func readMessage(r io.Reader) (id []byte, tag byte, err error) { + hdr := make([]byte, 2) + if _, err = io.ReadFull(r, hdr); err != nil { + return nil, 0, err + } + n := int(hdr[1]) + if n&0x80 != 0 { + lenBytes := make([]byte, n&0x7f) + if _, err = io.ReadFull(r, lenBytes); err != nil { + return nil, 0, err + } + n = 0 + for _, b := range lenBytes { + n = n<<8 | int(b) + } + } + body := make([]byte, n) + if _, err = io.ReadFull(r, body); err != nil { + return nil, 0, err + } + // body: INTEGER messageID (02 len value...), then the protocol op. + idLen := int(body[1]) + return body[2 : 2+idLen], body[2+idLen], nil +} + +// extendedResponse encodes an ExtendedResponse with the given result code. +func extendedResponse(id []byte, resultCode byte) []byte { + body := append([]byte{0x02, byte(len(id))}, id...) + body = append(body, 0x78, 0x07, 0x0a, 0x01, resultCode, 0x04, 0x00, 0x04, 0x00) + return append([]byte{0x30, byte(len(body))}, body...) +} + +// CA is a throwaway certificate authority. +type CA struct { + Pool *x509.CertPool + cert *x509.Certificate + key *ecdsa.PrivateKey +} + +// NewCA creates a self-signed CA. +func NewCA(t *testing.T) *CA { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(1), + Subject: pkix.Name{CommonName: "ldaptest CA"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + IsCA: true, + KeyUsage: x509.KeyUsageCertSign, + BasicConstraintsValid: true, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, tmpl, &key.PublicKey, key) + if err != nil { + t.Fatal(err) + } + cert, _ := x509.ParseCertificate(der) + pool := x509.NewCertPool() + pool.AddCert(cert) + return &CA{Pool: pool, cert: cert, key: key} +} + +// Issue returns a server certificate for the given DNS names and IP addresses. +func (ca *CA) Issue(t *testing.T, dnsNames []string, ips []net.IP) tls.Certificate { + t.Helper() + key, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + if err != nil { + t.Fatal(err) + } + tmpl := &x509.Certificate{ + SerialNumber: big.NewInt(time.Now().UnixNano()), + Subject: pkix.Name{CommonName: "ldaptest server"}, + NotBefore: time.Now().Add(-time.Hour), + NotAfter: time.Now().Add(time.Hour), + KeyUsage: x509.KeyUsageDigitalSignature, + ExtKeyUsage: []x509.ExtKeyUsage{x509.ExtKeyUsageServerAuth}, + DNSNames: dnsNames, + IPAddresses: ips, + } + der, err := x509.CreateCertificate(rand.Reader, tmpl, ca.cert, &key.PublicKey, ca.key) + if err != nil { + t.Fatal(err) + } + return tls.Certificate{Certificate: [][]byte{der}, PrivateKey: key} +}