feat: add ldap support#232
Conversation
WalkthroughLDAP authentication support was added as an optional authentication source. This includes new configuration fields, CLI flags, and environment variables for LDAP settings, as well as integration into authentication logic. User search and verification now distinguish between local and LDAP users. Supporting types, methods, and tests were updated accordingly. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Server
participant AuthService
participant LDAPService
User->>Server: Submit login (username, password)
Server->>AuthService: SearchUser(username)
alt User is local
AuthService->>AuthService: Find local user
AuthService->>Server: Return UserSearch(type="local")
Server->>AuthService: VerifyUser(UserSearch, password)
AuthService->>AuthService: Check local password
AuthService->>Server: Success/Failure
else User is LDAP
AuthService->>LDAPService: Search(username)
LDAPService->>AuthService: Return userDN
AuthService->>Server: Return UserSearch(type="ldap")
Server->>AuthService: VerifyUser(UserSearch, password)
AuthService->>LDAPService: Bind(userDN, password)
LDAPService->>AuthService: Success/Failure
AuthService->>Server: Success/Failure
end
Server->>User: Login result
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #232 +/- ##
==========================================
- Coverage 12.84% 12.01% -0.83%
==========================================
Files 19 20 +1
Lines 2344 2513 +169
==========================================
+ Hits 301 302 +1
- Misses 2031 2199 +168
Partials 12 12 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
internal/types/types.go (1)
15-19: Improve documentation and consider type safetyThe
UserSearchstruct is well-designed for distinguishing between user types. However, consider these improvements:-// UserSearch is the response of the get user +// UserSearch represents the result of a user search operation, containing +// user identification and the authentication source type type UserSearch struct { Username string Type string // "local", "ldap" or empty }Consider defining constants for the Type field values to improve type safety:
const ( UserTypeLocal = "local" UserTypeLDAP = "ldap" UserTypeEmpty = "" )internal/ldap/ldap.go (1)
65-72: Remove redundant return statement.The
return nilon line 71 is redundant since line 69 already returns any error.Apply this diff to simplify the method:
func (l *LDAP) Bind(userDN string, password string) error { // Bind to the LDAP server with the user's DN and password - err := l.Conn.Bind(userDN, password) - if err != nil { - return err - } - return nil + return l.Conn.Bind(userDN, password) }cmd/root.go (1)
248-253: Consider a more flexible default for LDAP search filter.The default search filter
(uid=%s)assumes the username field isuid. While this is common, different LDAP configurations might usecn,sAMAccountName, or other attributes.Consider documenting this in the flag description or using a more generic example:
- rootCmd.Flags().String("ldap-search-filter", "(uid=%s)", "LDAP search filter for user lookup.") + rootCmd.Flags().String("ldap-search-filter", "(uid=%s)", "LDAP search filter for user lookup (e.g., (uid=%s), (cn=%s), (sAMAccountName=%s)).")internal/auth/auth.go (2)
99-101: Consider normalizing LDAP usernames instead of returning DNReturning the full DN as username (e.g.,
uid=john,ou=users,dc=example,dc=com) might cause issues in other parts of the system that expect simpler usernames. Consider extracting just the uid/cn from the DN.
74-106: Add test coverage for SearchUser methodThis critical authentication method lacks test coverage. Consider adding unit tests for:
- Local user found scenario
- LDAP user found scenario
- User not found in either source
- LDAP search error handling
Would you like me to generate unit tests for this method?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
cmd/root.go(4 hunks)frontend/src/pages/login-page.tsx(1 hunks)go.mod(2 hunks)internal/auth/auth.go(5 hunks)internal/auth/auth_test.go(4 hunks)internal/handlers/handlers.go(4 hunks)internal/hooks/hooks.go(2 hunks)internal/ldap/ldap.go(1 hunks)internal/server/server_test.go(1 hunks)internal/types/config.go(2 hunks)internal/types/types.go(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (4)
internal/server/server_test.go (1)
internal/auth/auth.go (1)
NewAuth(29-49)
internal/hooks/hooks.go (4)
internal/auth/auth.go (1)
Auth(20-27)internal/types/types.go (1)
UserContext(42-52)internal/utils/utils.go (1)
Capitalize(335-340)internal/types/config.go (1)
Config(4-45)
internal/ldap/ldap.go (1)
internal/types/config.go (2)
Config(4-45)LdapConfig(133-140)
internal/handlers/handlers.go (4)
internal/auth/auth.go (1)
Auth(20-27)internal/types/types.go (1)
SessionCookie(32-39)internal/utils/utils.go (1)
Capitalize(335-340)internal/types/config.go (1)
Config(4-45)
🪛 GitHub Check: codecov/patch
internal/hooks/hooks.go
[warning] 38-39: internal/hooks/hooks.go#L38-L39
Added lines #L38 - L39 were not covered by tests
[warning] 41-41: internal/hooks/hooks.go#L41
Added line #L41 was not covered by tests
[warning] 49-54: internal/hooks/hooks.go#L49-L54
Added lines #L49 - L54 were not covered by tests
[warning] 57-59: internal/hooks/hooks.go#L57-L59
Added lines #L57 - L59 were not covered by tests
[warning] 66-66: internal/hooks/hooks.go#L66
Added line #L66 was not covered by tests
[warning] 70-79: internal/hooks/hooks.go#L70-L79
Added lines #L70 - L79 were not covered by tests
cmd/root.go
[warning] 143-163: cmd/root.go#L143-L163
Added lines #L143 - L163 were not covered by tests
[warning] 166-168: cmd/root.go#L166-L168
Added lines #L166 - L168 were not covered by tests
[warning] 171-171: cmd/root.go#L171
Added line #L171 was not covered by tests
[warning] 248-253: cmd/root.go#L248-L253
Added lines #L248 - L253 were not covered by tests
[warning] 289-294: cmd/root.go#L289-L294
Added lines #L289 - L294 were not covered by tests
internal/handlers/handlers.go
[warning] 366-368: internal/handlers/handlers.go#L366-L368
Added lines #L366 - L368 were not covered by tests
[warning] 371-371: internal/handlers/handlers.go#L371
Added line #L371 was not covered by tests
[warning] 385-385: internal/handlers/handlers.go#L385
Added line #L385 was not covered by tests
[warning] 401-417: internal/handlers/handlers.go#L401-L417
Added lines #L401 - L417 were not covered by tests
[warning] 419-424: internal/handlers/handlers.go#L419-L424
Added lines #L419 - L424 were not covered by tests
[warning] 426-428: internal/handlers/handlers.go#L426-L428
Added lines #L426 - L428 were not covered by tests
[warning] 480-480: internal/handlers/handlers.go#L480
Added line #L480 was not covered by tests
internal/auth/auth.go
[warning] 74-74: internal/auth/auth.go#L74
Added line #L74 was not covered by tests
[warning] 76-86: internal/auth/auth.go#L76-L86
Added lines #L76 - L86 were not covered by tests
[warning] 89-96: internal/auth/auth.go#L89-L96
Added lines #L89 - L96 were not covered by tests
[warning] 99-102: internal/auth/auth.go#L99-L102
Added lines #L99 - L102 were not covered by tests
[warning] 105-105: internal/auth/auth.go#L105
Added line #L105 was not covered by tests
[warning] 108-127: internal/auth/auth.go#L108-L127
Added lines #L108 - L127 were not covered by tests
[warning] 130-134: internal/auth/auth.go#L130-L134
Added lines #L130 - L134 were not covered by tests
[warning] 139-140: internal/auth/auth.go#L139-L140
Added lines #L139 - L140 were not covered by tests
[warning] 143-146: internal/auth/auth.go#L143-L146
Added lines #L143 - L146 were not covered by tests
[warning] 149-149: internal/auth/auth.go#L149
Added line #L149 was not covered by tests
[warning] 154-155: internal/auth/auth.go#L154-L155
Added lines #L154 - L155 were not covered by tests
[warning] 355-355: internal/auth/auth.go#L355
Added line #L355 was not covered by tests
🪛 ast-grep (0.38.1)
internal/ldap/ldap.go
[warning] 18-20: MinVersionis missing from this TLS configuration. By default, TLS 1.2 is currently used as the minimum when acting as a client, and TLS 1.0 when acting as a server. General purpose web applications should default to TLS 1.3 with all other protocols disabled. Only where it is known that a web server must support legacy clients with unsupported an insecure browsers (such as Internet Explorer 10), it may be necessary to enable TLS 1.0 to provide support. AddMinVersion: tls.VersionTLS13' to the TLS configuration to bump the minimum version to TLS 1.3.
Context: tls.Config{
InsecureSkipVerify: config.Insecure,
}
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
https://owasp.org/Top10/A02_2021-Cryptographic_Failures
(missing-ssl-minversion-go)
🔇 Additional comments (19)
frontend/src/pages/login-page.tsx (1)
165-167: LGTM: Improved error message stylingThe change from
<h3>to<p>with added width constraint improves the semantic correctness and visual presentation of the error message when no authentication providers are configured. This aligns well with the LDAP integration changes.go.mod (2)
25-25: LGTM: ASN.1 BER dependencyThe
go-asn1-ber/asn1-berdependency is correctly added as an indirect dependency, which is required for LDAP protocol handling.
65-65: LGTM: LDAP client libraryThe
github.com/go-ldap/ldap/v3dependency is the standard Go LDAP client library and is appropriate for this integration.internal/server/server_test.go (1)
87-87: LGTM: Updated auth constructor callThe addition of the third parameter
nilcorrectly aligns with the updatedNewAuthsignature that now accepts an LDAP service parameter. Usingnilis appropriate for tests that don't require LDAP functionality.internal/auth/auth_test.go (4)
21-21: LGTM: Updated auth constructor for rate limiting testsThe addition of the third parameter
nilcorrectly updates theNewAuthcall to match the new signature. This is appropriate for rate limiting tests that don't require LDAP functionality.
65-65: LGTM: Consistent constructor updateThe
NewAuthcall is correctly updated with the third parameter for the timeout testing scenario.
90-90: LGTM: Constructor update for disabled rate limiting testThe constructor call is properly updated to include the LDAP parameter for the disabled rate limiting test case.
106-106: LGTM: Final constructor updateThe last
NewAuthcall is correctly updated to maintain consistency with the new signature for concurrent login attempts testing.internal/types/config.go (1)
39-44: LGTM! Clean LDAP configuration structure.The LDAP configuration fields follow the existing patterns with consistent naming and mapstructure tags. The separate
LdapConfigstruct provides good encapsulation for LDAP-specific parameters.Also applies to: 132-140
internal/hooks/hooks.go (1)
38-79: Excellent refactoring for LDAP integration.The authentication logic has been cleanly refactored to support both local and LDAP users using the new
SearchUser/VerifyUserpattern. The handling of user types is correct - LDAP users have TOTP disabled (since it's a local-only feature) while local users retain their TOTP configuration. The improved error logging is also a nice touch.Also applies to: 107-126
internal/ldap/ldap.go (1)
39-63: LDAP search implementation looks robust.The search method correctly handles the LDAP search flow with proper error checking for multiple/no entries found. Using a configurable search filter with
fmt.Sprintfis a good approach for flexibility.internal/handlers/handlers.go (2)
365-385: Well-implemented LDAP integration in authentication handlers.The login handler correctly adopts the new
SearchUser/VerifyUserpattern and appropriately handles TOTP only for local users. The logic flow ensures LDAP users bypass TOTP completely, which is the correct behavior since TOTP secrets are stored locally.Also applies to: 401-429
480-480: Correct use of GetLocalUser for TOTP validation.Using
GetLocalUserdirectly in the TOTP handler is appropriate since TOTP is only applicable to local users, not LDAP users.cmd/root.go (2)
143-168: Excellent LDAP service integration.The conditional LDAP service creation with proper error handling and the validation ensuring at least one authentication source is configured demonstrate good defensive programming. The dependency injection pattern is consistently applied.
Also applies to: 171-171
289-294: Environment variable bindings follow consistent pattern.The LDAP environment variable bindings maintain consistency with the existing configuration approach.
internal/auth/auth.go (4)
355-355: LGTM!The logic correctly checks if either local users or LDAP authentication is configured.
29-29: All NewAuth callsites updated
Verified that every invocation ofNewAuthnow includes the newldap *ldap.LDAPparameter in both production and test code. No further updates are required.Affected files:
- cmd/root.go
- internal/server/server_test.go
- internal/auth/auth_test.go
143-156: No nil-return issue with GetLocalUserGetLocalUser’s signature returns a concrete types.User (struct), not a pointer, so it never returned nil. Every call site you have:
- internal/auth/auth.go:
if auth.GetLocalUser(username).Username != "" { … }- internal/handlers/handlers.go:
if localUser.TotpSecret != "" { … }- internal/hooks/hooks.go:
assignsuser := hooks.Auth.GetLocalUser(...)without any nil checkall correctly treat “not found” as empty fields rather than a nil pointer. There’s no caller comparing the returned User to nil, and no breaking change here.
Likely an incorrect or invalid review comment.
158-161: bcrypt CompareHashAndPassword is safe with an empty hashCalling
bcrypt.CompareHashAndPassword([]byte(""), …)returns a non-nil error (e.g.ErrHashTooShort), soCheckPasswordsimply returns false—it will not panic. No additional guard is required unless you’d like an early-return for clarity whenuser.Passwordis empty.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
internal/auth/auth.go (1)
74-106: LDAP injection vulnerability persists in SearchUser methodThe
SearchUsermethod correctly implements the dual search strategy (local first, then LDAP), but the LDAP injection vulnerability from previous reviews remains unaddressed. On line 92, the username is passed directly toauth.LDAP.Search(username)without proper escaping.This requires fixing the underlying
Searchmethod in the LDAP implementation to properly escape the username parameter usingldap.EscapeFilter()before embedding it in the search filter. The vulnerability allows attackers to manipulate LDAP queries through crafted usernames.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/auth/auth.go(5 hunks)internal/ldap/ldap.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/ldap/ldap.go
🧰 Additional context used
🧠 Learnings (1)
internal/auth/auth.go (1)
Learnt from: steveiliop56
PR: steveiliop56/tinyauth#122
File: internal/handlers/handlers.go:75-81
Timestamp: 2025-04-30T15:03:09.620Z
Learning: Values from identity providers and user inputs should be sanitized before being set as HTTP headers in TinyAuth to prevent CRLF injection attacks, even if they come from seemingly trusted sources like OAuth providers.
🪛 GitHub Check: codecov/patch
internal/auth/auth.go
[warning] 74-74: internal/auth/auth.go#L74
Added line #L74 was not covered by tests
[warning] 76-86: internal/auth/auth.go#L76-L86
Added lines #L76 - L86 were not covered by tests
[warning] 89-96: internal/auth/auth.go#L89-L96
Added lines #L89 - L96 were not covered by tests
[warning] 99-102: internal/auth/auth.go#L99-L102
Added lines #L99 - L102 were not covered by tests
[warning] 105-105: internal/auth/auth.go#L105
Added line #L105 was not covered by tests
[warning] 108-127: internal/auth/auth.go#L108-L127
Added lines #L108 - L127 were not covered by tests
[warning] 130-135: internal/auth/auth.go#L130-L135
Added lines #L130 - L135 were not covered by tests
[warning] 137-140: internal/auth/auth.go#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-144: internal/auth/auth.go#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 148-149: internal/auth/auth.go#L148-L149
Added lines #L148 - L149 were not covered by tests
[warning] 152-155: internal/auth/auth.go#L152-L155
Added lines #L152 - L155 were not covered by tests
[warning] 158-158: internal/auth/auth.go#L158
Added line #L158 was not covered by tests
[warning] 163-164: internal/auth/auth.go#L163-L164
Added lines #L163 - L164 were not covered by tests
[warning] 364-364: internal/auth/auth.go#L364
Added line #L364 was not covered by tests
🔇 Additional comments (4)
internal/auth/auth.go (4)
10-10: LDAP integration structure looks goodThe LDAP field addition to the Auth struct and corresponding constructor changes are well-implemented and follow good practices.
Also applies to: 26-26, 29-29, 47-47
108-150: VerifyUser method implementation addresses previous security concernsThe method correctly:
- Handles both local and LDAP authentication flows
- Includes proper error handling for the rebind operation (lines 130-135) as requested in previous reviews
- Has a default case in the switch statement (lines 142-144) as requested in previous reviews
- Provides comprehensive logging for debugging
152-165: GetLocalUser method improvements look goodThe changes to return
types.User{}instead of nil, along with the added debug and warning logging, improve the method's robustness and debuggability.
364-364: UserAuthConfigured method correctly includes LDAP checkThe update to include LDAP configuration in the authentication check is appropriate and maintains the method's purpose.
| func (auth *Auth) SearchUser(username string) types.UserSearch { | ||
| // Loop through users and return the user if the username matches | ||
| log.Debug().Str("username", username).Msg("Searching for user") | ||
|
|
||
| if auth.GetLocalUser(username).Username != "" { | ||
| log.Debug().Str("username", username).Msg("Found local user") | ||
|
|
||
| // If user found, return a user with the username and type "local" | ||
| return types.UserSearch{ | ||
| Username: username, | ||
| Type: "local", | ||
| } | ||
| } | ||
|
|
||
| // If no user found, check LDAP | ||
| if auth.LDAP != nil { | ||
| log.Debug().Str("username", username).Msg("Checking LDAP for user") | ||
|
|
||
| userDN, err := auth.LDAP.Search(username) | ||
| if err != nil { | ||
| log.Warn().Err(err).Str("username", username).Msg("Failed to find user in LDAP") | ||
| return types.UserSearch{} | ||
| } | ||
|
|
||
| // If user found in LDAP, return a user with the DN as username | ||
| return types.UserSearch{ | ||
| Username: userDN, | ||
| Type: "ldap", | ||
| } | ||
| } | ||
|
|
||
| return types.UserSearch{} | ||
| } | ||
|
|
||
| func (auth *Auth) VerifyUser(search types.UserSearch, password string) bool { | ||
| // Authenticate the user based on the type | ||
| switch search.Type { | ||
| case "local": | ||
| // Get local user | ||
| user := auth.GetLocalUser(search.Username) | ||
|
|
||
| // Check if password is correct | ||
| return auth.CheckPassword(user, password) | ||
| case "ldap": | ||
| // If LDAP is configured, bind to the LDAP server with the user DN and password | ||
| if auth.LDAP != nil { | ||
| log.Debug().Str("username", search.Username).Msg("Binding to LDAP for user authentication") | ||
|
|
||
| // Bind to the LDAP server | ||
| err := auth.LDAP.Bind(search.Username, password) | ||
| if err != nil { | ||
| log.Warn().Err(err).Str("username", search.Username).Msg("Failed to bind to LDAP") | ||
| return false | ||
| } | ||
|
|
||
| // If bind is successful, rebind with the LDAP bind user | ||
| err = auth.LDAP.Bind(auth.LDAP.Config.BindDN, auth.LDAP.Config.BindPassword) | ||
| if err != nil { | ||
| log.Error().Err(err).Msg("Failed to rebind with service account after user authentication") | ||
| // Consider closing the connection or creating a new one | ||
| return false | ||
| } | ||
|
|
||
| log.Debug().Str("username", search.Username).Msg("LDAP authentication successful") | ||
|
|
||
| // Return true if the bind was successful | ||
| return true | ||
| } | ||
| default: | ||
| log.Warn().Str("type", search.Type).Msg("Unknown user type for authentication") | ||
| return false | ||
| } | ||
|
|
||
| // If no user found or authentication failed, return false | ||
| log.Warn().Str("username", search.Username).Msg("User authentication failed") | ||
| return false | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding comprehensive unit tests for LDAP functionality
The static analysis shows extensive test coverage gaps for the new LDAP functionality. Given the security-critical nature of authentication code and the complexity of the dual authentication flow, comprehensive unit tests are essential.
Would you like me to help generate unit tests that cover:
- SearchUser method with both local and LDAP scenarios
- VerifyUser method for both authentication types
- Error handling paths and edge cases
- LDAP connection failures and timeouts
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 74-74: internal/auth/auth.go#L74
Added line #L74 was not covered by tests
[warning] 76-86: internal/auth/auth.go#L76-L86
Added lines #L76 - L86 were not covered by tests
[warning] 89-96: internal/auth/auth.go#L89-L96
Added lines #L89 - L96 were not covered by tests
[warning] 99-102: internal/auth/auth.go#L99-L102
Added lines #L99 - L102 were not covered by tests
[warning] 105-105: internal/auth/auth.go#L105
Added line #L105 was not covered by tests
[warning] 108-127: internal/auth/auth.go#L108-L127
Added lines #L108 - L127 were not covered by tests
[warning] 130-135: internal/auth/auth.go#L130-L135
Added lines #L130 - L135 were not covered by tests
[warning] 137-140: internal/auth/auth.go#L137-L140
Added lines #L137 - L140 were not covered by tests
[warning] 142-144: internal/auth/auth.go#L142-L144
Added lines #L142 - L144 were not covered by tests
[warning] 148-149: internal/auth/auth.go#L148-L149
Added lines #L148 - L149 were not covered by tests
🤖 Prompt for AI Agents
In internal/auth/auth.go from lines 74 to 150, the LDAP functionality in
SearchUser and VerifyUser lacks comprehensive unit tests. Add unit tests
covering SearchUser for both local user found and LDAP user found scenarios,
VerifyUser for local and LDAP authentication success and failure cases,
including error handling paths such as LDAP search errors, bind failures, and
rebind failures. Also include tests for edge cases like nil LDAP configuration
and unknown user types to ensure full coverage of the dual authentication flow.
1 participant
Summary by CodeRabbit
New Features
Improvements
Chores