-
Notifications
You must be signed in to change notification settings - Fork 1.3k
feat(machine-identities): LDAP Auth Lockout #4417
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🎉 Snyk checks have passed. No issues have been found so far.✅ security/snyk check is complete. No issues have been found. (View Details) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This PR implements a comprehensive LDAP authentication lockout feature for machine identities to prevent brute force attacks. The changes span both frontend and backend components to add configurable account lockout mechanisms after failed authentication attempts.
Backend Changes:
- Added four new lockout configuration fields to both
IdentityLdapAuth
andIdentityUniversalAuth
schemas:lockoutEnabled
(default true),lockoutThreshold
(default 3 attempts),lockoutDuration
(default 300 seconds), andlockoutCounterReset
(default 30 seconds) - Implemented lockout logic in authentication flows that tracks failed attempts using Redis keystore with TTL-based expiry
- Added PostgreSQL advisory locks to prevent race conditions during concurrent login attempts
- Created new API endpoints for manually clearing lockouts with proper authorization
- Enhanced error responses to use
UnauthorizedError
instead ofNotFoundError
to prevent identity enumeration attacks - Updated audit logging to track lockout configuration changes and clear lockout operations
Frontend Changes:
- Added new "Lockout" tab to both LDAP and Universal Auth configuration forms with validation for threshold (1-30), duration (30s-1day), and counter reset (5s-1hour)
- Implemented duration conversion utilities (
durationToSeconds
,getObjectFromSeconds
) in a centralized datetime helper module - Added
LockoutFields
component to display lockout configuration and provide "Clear All Lockouts" functionality with proper permission checks - Enhanced identity views to show lockout status with red lock icons and pass lockout state through component hierarchy
- Added new audit log event types and constants for tracking lockout operations
Database Changes:
- Created migrations to add lockout columns to both
IdentityUniversalAuth
andIdentityLdapAuth
tables - Updated identity service to fetch active lockout status from Redis using key pattern matching
- Added
activeLockoutAuthMethods
field to identity responses
Integration:
- Added keystore dependency to identity services for encrypted lockout state management
- Enhanced API documentation with lockout parameter descriptions
- Maintained consistency between Universal Auth and LDAP Auth implementations
The feature provides administrators with flexible security controls while maintaining backward compatibility through sensible defaults. The implementation follows established patterns in the codebase and includes comprehensive error handling and user feedback mechanisms.
Confidence score: 4/5
- This PR implements a complex security feature with potential for issues in concurrent scenarios and edge cases
- Score reflects the comprehensive implementation but concerns about race conditions, error handling consistency, and validation complexity
- Pay close attention to authentication flow logic, lockout validation, and database migration files
Context used:
Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)
Objective
Flag database queries that use or
conditions without proper grouping, which can break outer filters and cause unintended data exposure.
What to Flag
Look for query builder patterns where or
methods are called directly on the main query object without being wrapped in a subquery or callback function.
Flag these patterns:
.orWhere()
,.orWhereRaw()
,.orWhereILike()
,.orWhereIn()
, etc. called directly on the main query- Multiple chained
or
conditions without proper grouping - Any
or
condition that could bypass security filters or WHERE clauses applied elsewhere
Examples to FLAG:
// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
.where('deleted_at', null)
.orWhere('name', 'like', '%search%')
.orWhereRaw('email ilike ?', ['%search%']);
What NOT to Flag
Do NOT flag or
conditions that are properly grouped within a callback function or subquery.
Examples that are SAFE:
// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
.where((qb) => {
qb.where('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
.where('deleted_at', null)
.where(function() {
this.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
Detection Pattern
Flag any line containing:
.orWhere*()
methods called directly on a query object- NOT preceded by
.where((
or.where(function
- NOT inside a callback function block
Review Message Template
When flagging, use this message:
⚠️ **Unsafe OR Query Detected**
This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.
**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.
**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
.orWhere('field1', condition)
.orWhere('field2', condition);
// Do this:
query.where('important_filter', value)
.where((qb) => {
qb.where('field1', condition)
.orWhere('field2', condition);
});
Security Impact: High - Could expose unauthorized data
## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))
**Rule -** Add validation for string fields that have database column length limits to prevent insertion failures. For username template fields, add a 255 character limit validation. ([link](https://app.greptile.com/review/custom-context?memory=ab5293aa-4452-426c-a274-f034c06d0066))
<sub>37 files reviewed, 10 comments</sub>
<sub>[Edit Code Review Bot Settings](https://app.greptile.com/review/github) | [Greptile](https://greptile.com?utm_source=greptile_expert&utm_medium=github&utm_campaign=code_reviews&utm_content=infisical_4417)</sub>
...ation/IdentityDetailsByIDPage/components/ViewIdentityAuthModal/IdentityAuthLockoutFields.tsx
Show resolved
Hide resolved
...ementPage/components/OrgIdentityTab/components/IdentitySection/IdentityUniversalAuthForm.tsx
Outdated
Show resolved
Hide resolved
...ssManagementPage/components/OrgIdentityTab/components/IdentitySection/lockout/LockoutTab.tsx
Outdated
Show resolved
Hide resolved
...ssManagementPage/components/OrgIdentityTab/components/IdentitySection/lockout/LockoutTab.tsx
Show resolved
Hide resolved
@greptile review this |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Greptile Summary
This review covers only the changes made since the last review, not the entire PR. The changes implement LDAP authentication lockout functionality for machine identities by adding lockout configuration fields, validation logic, and UI components. The key additions include:
- Database Schema Updates: Migration adds four lockout fields (
lockoutEnabled
,lockoutThreshold
,lockoutDurationSeconds
,lockoutCounterResetSeconds
) to the LDAP auth table with sensible defaults - Backend Service Enhancement: The LDAP auth service now includes distributed locking mechanisms using Redis keystore to track failed authentication attempts and enforce lockouts
- API Extensions: New endpoints for clearing lockouts and updated LDAP auth endpoints to accept lockout configuration parameters
- Frontend Components: New reusable
LockoutTab
andLockoutFields
components extracted from Universal Auth to provide consistent lockout UI across authentication methods - Form Validation: Shared validation logic in
superRefineLockout
function to ensure lockout parameters fall within security bounds (30s-1day for duration, 5s-1hr for counter reset) - Audit Logging: Complete audit trail for LDAP lockout operations including configuration changes and lockout clearance events
The implementation mirrors the existing Universal Auth lockout pattern, providing consistent security controls across authentication methods. The lockout mechanism prevents brute force attacks by temporarily blocking authentication after configurable failed attempt thresholds, with administrative override capabilities.
Confidence score: 4/5
- This PR implements important security features with well-structured code that follows existing patterns
- Score reflects good implementation quality but potential for security-related edge cases in lockout logic
- Pay close attention to the lockout validation logic and Redis key management patterns
Context used:
Rule - # Greptile Code Review Prompt: OR Query Safety Check (knex.js)
Objective
Flag database queries that use or
conditions without proper grouping, which can break outer filters and cause unintended data exposure.
What to Flag
Look for query builder patterns where or
methods are called directly on the main query object without being wrapped in a subquery or callback function.
Flag these patterns:
.orWhere()
,.orWhereRaw()
,.orWhereILike()
,.orWhereIn()
, etc. called directly on the main query- Multiple chained
or
conditions without proper grouping - Any
or
condition that could bypass security filters or WHERE clauses applied elsewhere
Examples to FLAG:
// ❌ DANGEROUS - or conditions break outer filters
query.where('status', 'active')
.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
// ❌ DANGEROUS - mixed with other conditions
query.where('tenantId', userId)
.where('deleted_at', null)
.orWhere('name', 'like', '%search%')
.orWhereRaw('email ilike ?', ['%search%']);
What NOT to Flag
Do NOT flag or
conditions that are properly grouped within a callback function or subquery.
Examples that are SAFE:
// ✅ SAFE - or conditions grouped in callback
query.where('status', 'active')
.where((qb) => {
qb.where('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
// ✅ SAFE - explicit subquery grouping
query.where('tenantId', userId)
.where('deleted_at', null)
.where(function() {
this.orWhere('name', 'like', '%search%')
.orWhere('email', 'like', '%search%');
});
Detection Pattern
Flag any line containing:
.orWhere*()
methods called directly on a query object- NOT preceded by
.where((
or.where(function
- NOT inside a callback function block
Review Message Template
When flagging, use this message:
⚠️ **Unsafe OR Query Detected**
This query uses `or` conditions directly on the main query object, which can bypass outer filters and security constraints.
**Issue:** OR conditions at the query root level can break tenant isolation, permission checks, or other important filters.
**Fix:** Wrap OR conditions in a grouped WHERE clause:
```javascript
// Instead of this:
query.where('important_filter', value)
.orWhere('field1', condition)
.orWhere('field2', condition);
// Do this:
query.where('important_filter', value)
.where((qb) => {
qb.where('field1', condition)
.orWhere('field2', condition);
});
Security Impact: High - Could expose unauthorized data
## Additional Context
This pattern is particularly dangerous in multi-tenant applications, permission systems, or any query with security-critical WHERE clauses. Always ensure OR conditions are logically grouped to maintain the integrity of outer security filters. ([link](https://app.greptile.com/review/custom-context?memory=c4ca0367-148d-42b9-bcbd-958caf88aa07))
<sub>21 files reviewed, 1 comment</sub>
<sub>[Edit Code Review Bot Settings](https://app.greptile.com/review/github) | [Greptile](https://greptile.com?utm_source=greptile_expert&utm_medium=github&utm_campaign=code_reviews&utm_content=infisical_4417)</sub>
Description 📣
Lockout for LDAP
Related to: #4394