LDAP: Add mTLS / client certificate authentication support#509
LDAP: Add mTLS / client certificate authentication support#509steveiliop56 merged 6 commits intotinyauthapp:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds LDAP client-certificate (mTLS) support and config fields; LDAP service loads a client cert at Init, switches TLS/bind behavior when a cert is present, and auth rebinds now call a new BindService method. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App as App Bootstrap
participant LDAP as LdapService
participant TLS as TLS Loader
participant Conn as LDAP Connection
participant Auth as Auth Service
App->>LDAP: Init(config with AuthCert/AuthKey)
LDAP->>TLS: load cert from AuthCert/AuthKey
alt cert loaded
TLS-->>LDAP: cert (*tls.Certificate)
LDAP->>Conn: connect() using mTLS (client cert)
Conn-->>LDAP: connected
LDAP->>LDAP: BindService(false) -> ExternalBind with cert
else no cert
LDAP->>Conn: connect() using standard TLS (InsecureSkipVerify, TLS1.2+)
Conn-->>LDAP: connected
LDAP->>LDAP: BindService(false) -> Bind with BindDN/BindPassword
end
App->>Auth: Authenticate user
Auth->>LDAP: user bind then LdapService.BindService(true) for rebind
LDAP->>Conn: perform bind (ExternalBind or Bind)
Conn-->>LDAP: bind result
LDAP-->>Auth: result
Auth-->>App: authentication complete
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
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. Comment |
plaes
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmd/root.go (1)
68-69: Consider improving flag descriptions for clarity.The descriptions are slightly inconsistent. Consider aligning them for better user experience:
- {"ldap-auth-cert", "", "LDAP client certificate for authentication"}, - {"ldap-auth-key", "", "LDAP client certificate authentication key"}, + {"ldap-auth-cert", "", "Path to LDAP client certificate for mTLS authentication."}, + {"ldap-auth-key", "", "Path to LDAP client private key for mTLS authentication."},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
cmd/root.go(1 hunks)internal/bootstrap/app_bootstrap.go(1 hunks)internal/config/config.go(1 hunks)internal/service/auth_service.go(1 hunks)internal/service/ldap_service.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(17-48)
🔇 Additional comments (5)
internal/config/config.go (1)
37-38: LGTM!The new configuration fields for mTLS authentication follow the existing naming conventions and placement within the LDAP configuration block.
internal/service/ldap_service.go (2)
85-95: mTLS path ignores theInsecureconfiguration flag.When a client certificate is provided,
InsecureSkipVerifyis not set in the TLS config (line 86-89), whereas the non-certificate path respectsldap.config.Insecure(line 92). If this is intentional (enforcing server cert validation when using mTLS), consider documenting this behavior. Otherwise, apply the flag consistently.
138-150: LGTM - BindService abstraction is well-designed.The method correctly handles both mTLS (
ExternalBind) and DN/password binding scenarios. Therebindparameter appropriately controls mutex acquisition to prevent deadlock when called fromconnect()which already holds the lock.internal/bootstrap/app_bootstrap.go (1)
113-114: LGTM!The new certificate configuration fields are correctly wired through to the LDAP service, following the established pattern for other LDAP configuration options.
internal/service/auth_service.go (1)
106-110: LGTM!The change correctly delegates rebinding to
BindService(true), which abstracts the binding mechanism (mTLS viaExternalBindvs. DN/password viaBind). This maintains the existing error handling flow while supporting both authentication methods.
steveiliop56
left a comment
steveiliop56
left a comment
There was a problem hiding this comment.
Looks good @plaes, just left you two comments and we can merge.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/service/ldap_service.go (1)
182-207: Race condition:Close()called without mutex protection.The
reconnect()method has a concurrency issue. At line 192,ldap.conn.Close()is called without holding the mutex, while all other accesses toldap.conn(inSearch,Bind,heartbeat) are protected by the mutex.Race scenario:
heartbeat()detects failure and returns error (releases lock)- Heartbeat goroutine calls
reconnect()(no lock held)operationcloses connection at line 192 (no lock held)- Meanwhile, another goroutine calls
Search(), acquires lock, and attempts to use the closed connection- This leads to undefined behavior or crashes
The fix should ensure
ldap.conn.Close()is protected by the mutex, or ensure no other operations can access the connection during reconnection.🔎 Potential fix approach
One approach is to acquire the lock before closing:
func (ldap *LdapService) reconnect() error { log.Info().Msg("Reconnecting to LDAP server") exp := backoff.NewExponentialBackOff() exp.InitialInterval = 500 * time.Millisecond exp.RandomizationFactor = 0.1 exp.Multiplier = 1.5 exp.Reset() operation := func() (*ldapgo.Conn, error) { + ldap.mutex.Lock() ldap.conn.Close() + ldap.mutex.Unlock() conn, err := ldap.connect() if err != nil { return nil, err } return conn, nil } _, err := backoff.Retry(context.TODO(), operation, backoff.WithBackOff(exp), backoff.WithMaxTries(3)) if err != nil { return err } return nil }Note: This approach holds the lock briefly during Close(), then releases it before connect() (which acquires its own lock). This prevents other goroutines from accessing the closed connection between Close() and connect().
♻️ Duplicate comments (1)
internal/service/ldap_service.go (1)
144-147: Remove unnecessaryelsefor cleaner early return.As previously noted by @steveiliop56, the
elsestatement can be removed for a cleaner early-return pattern.🔎 Proposed refactor
if ldap.cert != nil { return ldap.conn.ExternalBind() } - } else { - return ldap.conn.Bind(ldap.config.BindDN, ldap.config.BindPassword) - } + return ldap.conn.Bind(ldap.config.BindDN, ldap.config.BindPassword)
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/ldap_service.go
🔇 Additional comments (2)
internal/service/ldap_service.go (2)
22-23: LGTM! Clean structure for mTLS support.The new
AuthCertandAuthKeyfields, the privateconfigfield, and thecertstorage field are well-designed for certificate-based authentication. The encapsulation improvements are appropriate.Also applies to: 27-27, 30-30, 35-35
84-94: Verify intentional omission ofInsecureSkipVerifyin mTLS path.When using client certificates (lines 85-88), the
InsecureSkipVerifyoption is not set, so it defaults tofalse(secure verification). In contrast, the non-mTLS path (lines 90-92) respectsldap.config.Insecure. This means users cannot disable server certificate verification when using mTLS.While this is likely intentional for security (mTLS scenarios typically require mutual verification), it's a behavioral difference from the non-mTLS path. Confirm this is the desired behavior, especially for development or testing scenarios where self-signed certificates might be used.
Now that we have ldap.BindService(), we don't need to access any members of LdapService.config externally.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/service/ldap_service.go (2)
40-48: Good implementation! Past review concerns addressed.The certificate loading logic properly returns errors instead of calling
log.Fatal(), allowing the caller to handle initialization failures gracefully. The typo fix and control flow improvements from previous reviews have been applied.💡 Optional: Validate partial configuration
Consider adding validation to detect when only one of AuthCert/AuthKey is set, which likely indicates a configuration error:
+ if (ldap.config.AuthCert != "") != (ldap.config.AuthKey != "") { + return fmt.Errorf("both AuthCert and AuthKey must be specified together for mTLS authentication") + } + // Check whether authentication with client certificate is possible if ldap.config.AuthCert != "" && ldap.config.AuthKey != "" {This would provide clearer feedback to users who misconfigure the certificate authentication.
71-106: Well-designed TLS configuration handling.The connect method properly handles both certificate-based and password-based authentication:
- Conditional TLS configuration based on certificate presence is correct
- Early return pattern makes the code more readable
- The TODO comment clearly documents the STARTTLS limitation mentioned in the PR description
- Proper locking and binding flow with
BindService(false)One consideration: when using client certificates (mTLS), the
Insecureconfig flag is not applied to server certificate validation (it defaults to secure). While this is generally the right behavior for mTLS deployments, consider documenting this behavior if users might expect to combine client certificates with self-signed or invalid server certificates.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
internal/bootstrap/service_bootstrap.gointernal/config/config.gointernal/service/auth_service.gointernal/service/ldap_service.go
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/service/auth_service.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(17-32)
🔇 Additional comments (5)
internal/config/config.go (1)
69-70: LGTM! Clean configuration additions.The new fields for mTLS authentication are properly declared with appropriate yaml tags and descriptions. The naming is clear and consistent with the existing LDAP configuration structure.
internal/bootstrap/service_bootstrap.go (1)
40-41: LGTM! Configuration properly wired.The new certificate configuration fields are correctly passed to the LDAP service, following the same pattern as the existing LDAP configuration fields.
internal/service/ldap_service.go (3)
22-23: LGTM! Well-structured additions.The new fields for mTLS support are cleanly integrated:
- AuthCert and AuthKey added to configuration
- Private
certfield to store the loaded certificate- Config field renamed to private
configfollowing Go conventionsAlso applies to: 27-27, 30-30, 35-35
108-135: LGTM! Field references correctly updated.The Search method properly uses the private
configfield, consistent with the struct changes.
137-148: Excellent implementation of BindService!The conditional locking design elegantly handles both initial binding (where the caller already holds the lock) and rebinding scenarios (where the method must acquire the lock). This prevents deadlock while maintaining thread safety.
The early return pattern (addressing a previous review comment) makes the logic clear: use ExternalBind for certificate-based authentication, otherwise fall back to standard binding with DN/password.
|
PR has been rebase against main and tested it against Google LDAP service. Though there seem to be some regressions due to 03ed183:
|
|
@plaes no worries, I will look into them. Thank you for the pull request. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
internal/service/ldap_service.go (1)
148-159: Clarify the locking rationale in the comment.The conditional locking based on the
rebindparameter is correct but subtle:
- When
rebind=false(initial bind fromconnect()), the caller already holds the mutex, so locking here would cause a deadlock.- When
rebind=true(subsequent rebind from auth flow), the caller doesn't hold the mutex, so this method must acquire it.Consider improving the comment to make this rationale more explicit, which will help future maintainers understand why the lock is conditional.
🔎 Suggested comment improvement
- // Locks must not be used for initial binding attempt + // When rebind=false, the caller (connect) already holds the mutex. + // When rebind=true, this is called externally and must acquire the lock. if rebind { ldap.mutex.Lock() defer ldap.mutex.Unlock() }Note: The
elsestatement on lines 155-158 could also be removed as suggested in past comments, though this is a style preference:if ldap.cert != nil { return ldap.conn.ExternalBind() } return ldap.conn.Bind(ldap.config.BindDN, ldap.config.BindPassword)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/ldap_service.go
🧰 Additional context used
🧬 Code graph analysis (1)
internal/service/ldap_service.go (1)
internal/config/config.go (1)
Config(17-32)
🔇 Additional comments (6)
internal/service/ldap_service.go (6)
22-23: LGTM!The new configuration fields for client certificate authentication are well-integrated and align with the PR objectives.
27-27: Good practice to make the config field private.This reduces the public API surface and properly encapsulates the service's configuration.
30-30: LGTM!The certificate field is appropriately typed as a pointer to support optional mTLS authentication.
40-59: LGTM!The certificate loading logic correctly returns errors instead of calling
log.Fatal(), allowing graceful error handling by the caller. The typo in the error message has also been fixed. The TODO comment about optional CA certificates is a reasonable placeholder for future enhancement.
110-116: LGTM!The refactored binding logic correctly uses
BindService(false)to perform the initial bind after establishing the connection. Thefalseparameter indicates this is not a rebind operation, which is appropriate sinceconnect()already holds the mutex lock.
95-99: ExternalBind is the correct SASL EXTERNAL mechanism for client certificate authentication with go-ldap/ldap/v3, and the implementation follows the expected pattern: configure TLS with the client certificate → DialURL with TLS config → ExternalBind() call.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/service/ldap_service.go (2)
148-153: Consider clarifying the locking comment.The conditional locking logic is correct (prevents double-locking when called from
connect()which already holds the mutex at line 83), but the comment could be clearer about why locking is conditional.🔎 Suggested comment clarification
func (ldap *LdapService) BindService(rebind bool) error { - // Locks must not be used for initial binding attempt + // Lock is already held by connect() during initial bind (rebind=false). + // Only acquire lock when rebinding from external callers (rebind=true). if rebind { ldap.mutex.Lock() defer ldap.mutex.Unlock() }
49-58: Optional: CA certificate configuration for custom/private CAs.The TODO correctly identifies a future enhancement for specifying custom CA certificates when connecting to LDAP servers with non-public CAs. This would eliminate the need for system-level CA trust configuration.
Do you want me to help implement this enhancement or open an issue to track it?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/service/ldap_service.go
🔇 Additional comments (2)
internal/service/ldap_service.go (2)
40-47: LGTM! Certificate loading is properly implemented.The certificate loading logic correctly uses
tls.LoadX509KeyPair, returns errors appropriately (addressing the previouslog.Fatalissue), and logs success. The conditional check ensures both cert and key are provided before attempting to load.
95-105: LGTM! TLS configuration correctly enforces security with mTLS.The conditional TLS setup appropriately handles both certificate-based and standard authentication. When a client certificate is present, server certificate verification is always enforced (no
InsecureSkipVerify), which is the correct security posture for mTLS as discussed in previous reviews.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #509 +/- ##
==========================================
- Coverage 20.26% 20.12% -0.14%
==========================================
Files 37 37
Lines 2147 2196 +49
==========================================
+ Hits 435 442 +7
- Misses 1682 1724 +42
Partials 30 30 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
It's now possible to connect to LDAP servers using client certificates. It was tested against Google's LDAP service (though only available for Google Workspace users):
When any of the
ldap-auth-*variables is used, it will attempt to load certificate-key pair and if something fails, it exits immedately ignoringldap-bind-dnandldap-bind-passwordvalues.Also added note about missing STARTTLS support.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.