Feat/ldap tls verify

[hungz]

fix issue #749

Summary by CodeRabbit

• New Features

  • Added configurable CA certificate bundles for trusted LDAP/LDAPS server verification.
  • Added certificate and hostname verification controls for legacy and provider-based LDAP configurations.
  • CA bundles automatically enable verification and extend system trust.

• Bug Fixes

  • Provider-based LDAP connections now verify server certificates by default.
  • Added clear errors for missing or invalid CA certificate files.

• Documentation

  • Updated LDAP TLS configuration guidance and examples.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 0ad92750-8914-446a-a9ef-eb928795fec9

📥 Commits

Reviewing files that changed from the base of the PR and between a8910c7 and 0d20828.

📒 Files selected for processing (2)

• api/login.go
• docs

🚧 Files skipped from review as they are similar to previous changes (2)

• docs
• api/login.go

---

📝 Walkthrough

Walkthrough

LDAP TLS configuration now supports explicit CA bundles and verification controls. Legacy and provider settings are updated. LDAPS login uses centralized TLS configuration with validation errors and test coverage.

Changes

LDAP TLS verification

Layer / File(s)	Summary
TLS configuration contract and provider wiring util/config.go, util/LdapProvider.go, util/LdapProvider_test.go, config.schema.yaml, docs	Legacy LDAP TLS fields are replaced with verification and CA-bundle settings. Provider mapping and ShouldVerifyTLS() treat configured CA bundles as enabling verification. Tests, schema definitions, and the documentation pointer are updated.
LDAPS TLS configuration and dialing api/login.go, api/login_ldap_tls_test.go	LDAPS connections use centralized TLS configuration with system trust or an appended PEM CA bundle. Missing files and invalid PEM data return explicit errors before dialing. Tests cover defaults, opt-out behavior, CA overrides, and file errors.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LDAPLogin
  participant ldapTLSConfig
  participant LdapProvider
  participant CertificatePool
  participant LDAPServer
  LDAPLogin->>ldapTLSConfig: Request TLS configuration
  ldapTLSConfig->>LdapProvider: Evaluate ShouldVerifyTLS()
  ldapTLSConfig->>CertificatePool: Load system roots and configured CA bundle
  LDAPLogin->>LDAPServer: Dial LDAPS with tls.Config

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly identifies the main change: adding LDAP TLS verification support.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

api/login.go (1)

58-62: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

Consider setting an explicit MinVersion on the LDAPS tls.Config.

Static analysis flags the missing MinVersion. Go's client default is already TLS 1.2, so this isn't an active vulnerability, but making it explicit avoids relying on implicit defaults. Note: forcing tls.VersionTLS13 (as the tool literally suggests) could break enterprise LDAP/AD servers that only support TLS 1.2, so tls.VersionTLS12 is the safer explicit floor.

🔒 Proposed fix

 	cfg := &tls.Config{
 		// `#nosec` G402 -- opt-out is deliberate and documented above.
 		InsecureSkipVerify: !provider.ShouldVerifyTLS(),
+		MinVersion:         tls.VersionTLS12,
 	}

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@api/login.go` around lines 58 - 62, Update ldapTLSConfig to set
tls.Config.MinVersion explicitly to tls.VersionTLS12, preserving the existing
InsecureSkipVerify behavior and compatibility with LDAP servers supporting TLS
1.2.

Source: Linters/SAST tools

util/LdapProvider.go (1)

21-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Doc comment references a non-existent TLSVerify field.

LdapProvider has no TLSVerify field — only TLSSkipVerify. ShouldVerifyTLS() (Line 47) actually overrides TLSSkipVerify when TLSCACertFile is set. The schema doc for this same field (config.schema.yaml, provider tls_ca_cert_file) correctly says "forces verification on"; this comment should match.

✏️ Proposed wording fix

 	// TLSCACertFile is a PEM bundle used to verify the LDAP server's
 	// certificate, in addition to the system trust store. Set this when the
-	// server uses a self-signed or internal-CA cert. Setting it implies
-	// TLSVerify.
+	// server uses a self-signed or internal-CA cert. Setting it forces
+	// certificate verification on (overrides TLSSkipVerify).
 	TLSCACertFile string `json:"tls_ca_cert_file"`

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@util/LdapProvider.go` around lines 21 - 25, Update the TLSCACertFile doc
comment in LdapProvider to reference the existing TLSSkipVerify behavior
accurately: state that setting TLSCACertFile forces TLS certificate verification
on, matching ShouldVerifyTLS() and the schema wording, and remove the
non-existent TLSVerify reference.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@api/login.go`:
- Around line 58-62: Update ldapTLSConfig to set tls.Config.MinVersion
explicitly to tls.VersionTLS12, preserving the existing InsecureSkipVerify
behavior and compatibility with LDAP servers supporting TLS 1.2.

In `@util/LdapProvider.go`:
- Around line 21-25: Update the TLSCACertFile doc comment in LdapProvider to
reference the existing TLSSkipVerify behavior accurately: state that setting
TLSCACertFile forces TLS certificate verification on, matching ShouldVerifyTLS()
and the schema wording, and remove the non-existent TLSVerify reference.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: eaf269e5-4aa3-4f33-b72f-3a7c495833d8

📥 Commits

Reviewing files that changed from the base of the PR and between 0bd3589 and a8910c7.

📒 Files selected for processing (7)

• api/login.go
• api/login_ldap_tls_test.go
• config.schema.yaml
• docs
• util/LdapProvider.go
• util/LdapProvider_test.go
• util/config.go