Skip to content

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

Open
alghanim wants to merge 1 commit into
bodaay:masterfrom
alghanim:fix/ldap-starttls-server-name
Open

alghanim wants to merge 1 commit into
bodaay:masterfrom
alghanim:fix/ldap-starttls-server-name

Conversation

@alghanim

Copy link
Copy Markdown
Collaborator

An ldap:// directory link can't be used with certificate verification. A verified StartTLS upgrade always fails:

ldap StartTLS upgrade failed; refusing cleartext bind (use ldaps:// or set allow_insecure):
LDAP Result Code 200 "Network Error": TLS handshake failed
(tls: either ServerName or InsecureSkipVerify must be specified in the tls.Config)

So the documented default (ldap:// = StartTLS) never works. Operators are pushed to allow_insecure (cleartext) or skip_tls_verify (unverified), and the admin console offers neither switch.

Separately, the console's Connect button (auto-discover) and the Kerberos setup paths bind without StartTLS. They send the password in cleartext on ldap:// even though the saved config requires StartTLS.

1. StartTLS had no ServerName

LDAPConnect passed &tls.Config{InsecureSkipVerify: cfg.SkipTLSVerify} to conn.StartTLS.

  • go-ldap's StartTLS wraps the open socket with tls.Client, which does not derive ServerName from the address.
  • ldaps:// worked only because go-ldap dials it with tls.DialWithDialer, which does derive it.

Change: ldapTLSConfig(url, skipVerify) sets ServerName from the URL host and is used for both StartTLS and ldaps://. It also adds an unexported ldapRootCAs (nil = system pool, which honours SSL_CERT_FILE) so tests can supply their own CA.

2. Admin paths bypassed the TLS policy

These four call sites dialled with ldaplib.DialURL and bound directly:

Call site Password sent in cleartext on ldap://
handleAutoDiscoverLDAP (console Connect) service account
handleSetupKerberos service account
autoSetupKerberos (import) service account
handleCleanupKerberos the AD admin credentials it is given

Auto-discover then saved a config with allow_insecure unset. The console said "Connected", and every login failed with the StartTLS error above.

Change: all four now connect through auth.LDAPConnect, so they follow exactly the policy logins use. Auto-discover uses the same TLS mode the saved config will have, so Connect fails fast with the real reason instead of reporting success.

Verification

Unit tests. The new tests use internal/ldaptest, a small in-process LDAP server. It answers StartTLS (or refuses it) and records every operation sent before TLS.

Test What it checks On master Here
TestLDAPConnectStartTLSVerifiesCertificate StartTLS with a trusted cert succeeds, and only the StartTLS request is sent in cleartext fails with the ServerName error passes
TestLDAPConnectStartTLSRejectsWrongHost a cert issued for another host is refused (fails closed) — passes
TestLDAPConnectStartTLSSkipVerify skip_tls_verify still works — passes
TestLDAPConnectLDAPSVerifiesCertificate ldaps:// unchanged — passes
TestLDAPTLSConfigServerName host, IPv4 and IPv6 URLs — passes
TestAutoDiscoverDoesNotBindInCleartext Connect never sends a search or bind before TLS fails: sends a SearchRequest (0x63) in cleartext passes
TestSetupKerberosDoesNotBindInCleartext Kerberos setup never binds before TLS fails: sends a BindRequest (0x60) with the password in cleartext passes

go test ./... is green, go vet and gofmt are clean, and go test -race is clean on the new tests.

End to end: a Samba AD DC whose LDAP certificate comes from an internal CA, with SimpleAuth trusting that CA via SSL_CERT_FILE. The console's Connect was called with a bare host name.

Step master this PR
Connect dc1.green.org "connected", password bound in cleartext connected over StartTLS, saved ldap://dc1.green.org:389
POST /api/admin/ldap/test StartTLS upgrade failed … ServerName {"status":"ok"} (certificate verified)
AD user login fails access token issued

Behaviour change

On an ldap:// directory that cannot do StartTLS (no LDAP certificate), console Connect and Kerberos setup now fail with the StartTLS error. Before, they "succeeded" by binding in cleartext, but every login on that config failed anyway, because the saved config never had allow_insecure. Cleartext directories still need allow_insecure: true, exactly as for logins.

🤖 Generated with Claude Code

StartTLS wrapped the connection with a tls.Config that had no ServerName.
tls.Client does not derive it from the address (tls.Dial does, which is why
ldaps:// worked), so with certificate verification on every upgrade failed
with "either ServerName or InsecureSkipVerify must be specified" and an
ldap:// directory could only be used with allow_insecure or skip_tls_verify.
LDAPConnect now sets ServerName from the URL host for both StartTLS and
ldaps://.

The admin console's Connect (auto-discover) and the Kerberos setup, cleanup
and import paths dialled with ldaplib.DialURL and bound directly. On ldap://
that sent the service-account (or AD admin) password in cleartext, ignoring
the StartTLS policy, and auto-discover reported success for a config that then
refused every login. They now connect through auth.LDAPConnect.

Tests use a small in-process LDAP server (internal/ldaptest) that answers
StartTLS and records every operation sent before TLS.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant