Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. Scanned FilesNone |
…etection - Add APPLICATION value to ResourceType enum - Create ApplicationComplianceCheckImpl for session-level security analysis - Implement 3-tier BruteForceAttack.drl rules (MINOR/MAJOR/CRITICAL) - Update kmodule.xml to include application rules package - Update RulesEngineImpl to insert application session data - Add BruteForceAttackRulesTest with 9 passing unit tests Agent-Logs-Url: https://github.com/Hack23/cia/sessions/0be4aadc-da0e-4b99-9dbc-81169915699e Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
📚 Javadoc GeneratedThe Javadoc documentation has been generated for this PR. Download the |
…lter, remove debug output Agent-Logs-Url: https://github.com/Hack23/cia/sessions/0be4aadc-da0e-4b99-9dbc-81169915699e Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
📚 Javadoc GeneratedThe Javadoc documentation has been generated for this PR. Download the |
There was a problem hiding this comment.
Pull request overview
Implements the previously-empty BruteForceAttack.drl by introducing application-level compliance checks and Drools rules to detect brute-force login attempts, integrating with the existing rules-compliance reporting pipeline.
Changes:
- Added
APPLICATIONas a newResourceTypeand introducedApplicationComplianceCheckImplto represent session-level security signals for rules evaluation. - Implemented 3-tier brute force detection rules (MINOR/MAJOR/CRITICAL) in
BruteForceAttack.drl(MVEL dialect) and enabled rule loading viakmodule.xml. - Extended
RulesEngineImplto insert application-session-derived compliance checks into the KIE session and added unit tests for the new rules.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| service.impl/src/test/java/com/hack23/cia/service/impl/rules/BruteForceAttackRulesTest.java | Adds unit tests for brute force thresholds and rule firing behavior. |
| service.impl/src/main/resources/META-INF/kmodule.xml | Registers the new application rules package so Drools loads the DRL. |
| service.impl/src/main/resources/com/hack23/cia/service/impl/rules/application/BruteForceAttack.drl | Implements the MINOR/MAJOR/CRITICAL brute force detection rules using MVEL. |
| service.impl/src/main/java/com/hack23/cia/service/impl/rules/RulesEngineImpl.java | Inserts application session compliance checks by counting failed auth attempts from session events. |
| service.impl/src/main/java/com/hack23/cia/service/impl/rules/ApplicationComplianceCheckImpl.java | New compliance check wrapper holding sessionId, IP info, and failed-auth count for Drools. |
| model.internal.application.user.impl/src/main/java/com/hack23/cia/model/internal/application/data/rules/impl/ResourceType.java | Adds APPLICATION enum value for rule violations tied to application-level resources. |
| import com.hack23.cia.model.internal.application.data.ministry.impl.ViewRiksdagenMinistry; | ||
| import com.hack23.cia.model.internal.application.data.party.impl.ViewRiksdagenPartySummary; | ||
| import com.hack23.cia.model.internal.application.data.politician.impl.ViewRiksdagenPolitician; | ||
| import com.hack23.cia.model.internal.application.system.impl.ApplicationActionEvent; |
There was a problem hiding this comment.
Unused import ApplicationActionEvent in this class; it isn't referenced anywhere and will fail builds that enforce no-unused-imports. Remove the import (or use the type explicitly if intended).
| import com.hack23.cia.model.internal.application.system.impl.ApplicationActionEvent; |
| final long failedAuthAttempts = session.getEvents().stream() | ||
| .filter(event -> ApplicationOperationType.AUTHENTICATION == event.getApplicationOperation() | ||
| && event.getErrorMessage() != null && !event.getErrorMessage().isEmpty()) | ||
| .count(); |
There was a problem hiding this comment.
Failed authentication attempts are currently inferred from event.getErrorMessage() being non-empty. Elsewhere (e.g., login blocking) failures are tracked via applicationMessage == ServiceResult.FAILURE; using errorMessage risks undercounting failures where no error text is set (e.g., some AUTHENTICATION failures). Consider counting by ServiceResult.FAILURE in applicationMessage (and optionally also checking errorMessage for context).
| private static void insertApplicationSessions(final KieSession ksession, final List<ApplicationSession> list) { | ||
| for (final ApplicationSession session : list) { | ||
| if (session != null && session.getEvents() != null) { | ||
| final long failedAuthAttempts = session.getEvents().stream() | ||
| .filter(event -> ApplicationOperationType.AUTHENTICATION == event.getApplicationOperation() | ||
| && event.getErrorMessage() != null && !event.getErrorMessage().isEmpty()) | ||
| .count(); | ||
|
|
||
| if (failedAuthAttempts > 0) { | ||
| final ApplicationComplianceCheckImpl check = new ApplicationComplianceCheckImpl( | ||
| session.getSessionId(), | ||
| session.getIpInformation(), | ||
| failedAuthAttempts); | ||
| ksession.insert(check); | ||
| } |
There was a problem hiding this comment.
applicationSessionDAO.getAll() + iterating session.getEvents() will typically trigger an extra query per session because events is a @OneToMany with default LAZY loading. This can become a large N+1 query pattern on systems with many sessions. Prefer a DAO/query that pre-aggregates failed AUTHENTICATION counts per session (or fetch-joins events in bulk) and only inserts checks for sessions meeting the rule threshold.
| /** | ||
| * Test salience ordering - highest severity wins. | ||
| * The AbstractComplianceCheckImpl keeps the highest severity violation. | ||
| */ | ||
| @Test | ||
| public void testSalienceOrderingHighestSeverityWins() { | ||
| final ApplicationComplianceCheckImpl check = new ApplicationComplianceCheckImpl( | ||
| "session-6", "192.168.0.100", 25); | ||
|
|
||
| kieSession.insert(check); | ||
| kieSession.fireAllRules(); | ||
|
|
||
| final List<RuleViolation> violations = check.getRuleViolations(); | ||
| assertFalse("Should have violations", violations.isEmpty()); | ||
| assertEquals("Should have exactly 1 violation (highest severity wins)", 1, violations.size()); | ||
|
|
||
| final RuleViolation violation = violations.get(0); | ||
| assertEquals("Highest severity should be CRITICAL for 25 attempts", Status.CRITICAL, violation.getStatus()); | ||
| } |
There was a problem hiding this comment.
testSalienceOrderingHighestSeverityWins doesn't actually validate salience/"highest severity wins" behavior: with the current DRL conditions (5–9, 10–19, 20+), only the CRITICAL rule matches for 25 attempts, so ordering never comes into play. Either adjust the rules/test so multiple rules can fire and the map keeps the highest severity, or rename/update the test and its Javadoc to reflect what it truly asserts.
|
@copilot apply changes based on the comments in this thread |
…e N+1 query, fix test name - Remove ApplicationSessionDAO dependency; use DataViewer.findListByProperty to query failed AUTHENTICATION events with applicationMessage=FAILURE in a single query, eliminating the N+1 lazy-load pattern - Match LoginBlockedAccessImpl pattern: check applicationMessage == ServiceResult.FAILURE.toString() instead of errorMessage - Rename testSalienceOrderingHighestSeverityWins to testOnlyCriticalRuleFires with accurate Javadoc (non-overlapping conditions mean only one rule fires) Agent-Logs-Url: https://github.com/Hack23/cia/sessions/0859b5e4-d8b1-4d2c-8f66-ca8e669926f8 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All four review comments addressed in commit
|
📚 Javadoc GeneratedThe Javadoc documentation has been generated for this PR. Download the |
| final long failedCount = entry.getValue().size(); | ||
| if (failedCount > 0) { | ||
| final ApplicationComplianceCheckImpl check = new ApplicationComplianceCheckImpl( | ||
| entry.getKey(), entry.getKey(), failedCount); |
There was a problem hiding this comment.
ApplicationComplianceCheckImpl is constructed with ipInformation set to the sessionId (entry.getKey()), which will produce misleading names like Session:<id> IP:<id> and incorrect data in persisted RuleViolations. Either populate ipInformation from ApplicationSession.ipInformation (if available) or use a clear placeholder (e.g., "unknown") and/or rename the field to reflect what it actually contains.
| entry.getKey(), entry.getKey(), failedCount); | |
| entry.getKey(), "unknown", failedCount); |
| final Map<String, List<ApplicationActionEvent>> eventsBySession = failedAuthEvents.stream() | ||
| .filter(event -> event.getSessionId() != null) | ||
| .collect(Collectors.groupingBy(ApplicationActionEvent::getSessionId)); | ||
|
|
||
| for (final Map.Entry<String, List<ApplicationActionEvent>> entry : eventsBySession.entrySet()) { | ||
| final long failedCount = entry.getValue().size(); |
There was a problem hiding this comment.
This groups all failed auth events into Map<String, List<ApplicationActionEvent>>, but only the count per session is used. Collecting full lists can waste a lot of memory for large event tables; prefer grouping directly to counts (e.g., groupingBy(sessionId, counting())) and iterate those counts.
| final Map<String, List<ApplicationActionEvent>> eventsBySession = failedAuthEvents.stream() | |
| .filter(event -> event.getSessionId() != null) | |
| .collect(Collectors.groupingBy(ApplicationActionEvent::getSessionId)); | |
| for (final Map.Entry<String, List<ApplicationActionEvent>> entry : eventsBySession.entrySet()) { | |
| final long failedCount = entry.getValue().size(); | |
| final Map<String, Long> failedCountsBySession = failedAuthEvents.stream() | |
| .filter(event -> event.getSessionId() != null) | |
| .collect(Collectors.groupingBy(ApplicationActionEvent::getSessionId, Collectors.counting())); | |
| for (final Map.Entry<String, Long> entry : failedCountsBySession.entrySet()) { | |
| final long failedCount = entry.getValue(); |
| rule "Brute force attack detected - minor threshold, 5 or more failed login attempts in session" | ||
| dialect "mvel" | ||
| salience 10 | ||
| when | ||
| $app : ApplicationComplianceCheckImpl(failedAuthenticationAttempts >= 5 && failedAuthenticationAttempts < 10) | ||
| then | ||
| $app.addViolation( Status.MINOR, "BruteForceAttack","Security", kcontext.getRule().getName(),"login.failed.threshold.minor"); | ||
| end | ||
|
|
||
| rule "Brute force attack detected - major threshold, 10 or more failed login attempts in session" | ||
| dialect "mvel" | ||
| salience 50 | ||
| when | ||
| $app : ApplicationComplianceCheckImpl(failedAuthenticationAttempts >= 10 && failedAuthenticationAttempts < 20) | ||
| then |
There was a problem hiding this comment.
The rule names say "5 or more" / "10 or more" failed attempts, but the conditions are non-overlapping ranges (5–9, 10–19). Since kcontext.getRule().getName() is persisted as the violation description, the current names will be misleading; adjust the rule names to match the actual thresholds (or change the conditions to match the wording).
| kieSession.fireAllRules(); | ||
|
|
There was a problem hiding this comment.
testOnlyCriticalRuleFires asserts a single violation, but doesn’t verify that only one rule fired (multiple rules could fire and still collapse to one violation because AbstractComplianceCheckImpl deduplicates by ruleName). Capture fireAllRules() return value and assert it is 1 to match the test’s intent and prevent regressions to overlapping conditions.
| kieSession.fireAllRules(); | |
| final int firedRules = kieSession.fireAllRules(); | |
| assertEquals("Should have exactly 1 fired rule (only CRITICAL matches)", 1, firedRules); |
|
@copilot apply changes based on the comments in this thread |
… IP/rule names, strengthen test - Use Collectors.counting() instead of collecting full event lists (memory optimization) - Use "unknown" for ipInformation instead of duplicating sessionId - Fix DRL rule names: "5 or more" → "5 to 9", "10 or more" → "10 to 19" to match actual non-overlapping ranges - Assert fireAllRules() returns 1 in testOnlyCriticalRuleFires to verify exactly one rule fired Agent-Logs-Url: https://github.com/Hack23/cia/sessions/730053c9-1251-43ae-9b93-5e9f4868edb7 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All four review comments addressed in commit
|
📚 Javadoc GeneratedThe Javadoc documentation has been generated for this PR. Download the |
| public void setUp() { | ||
| final KieServices kieServices = KieServices.Factory.get(); | ||
| final KieFileSystem kfs = kieServices.newKieFileSystem(); | ||
| kfs.write(ResourceFactory.newClassPathResource( | ||
| "com/hack23/cia/service/impl/rules/application/BruteForceAttack.drl")); | ||
| final KieBuilder kieBuilder = kieServices.newKieBuilder(kfs).buildAll(); | ||
| final Results results = kieBuilder.getResults(); | ||
| assertFalse("DRL compilation should have no errors: " + results.getMessages(), | ||
| results.hasMessages(Message.Level.ERROR)); | ||
| kieContainer = kieServices.newKieContainer(kieServices.getRepository().getDefaultReleaseId()); | ||
| kieSession = kieContainer.newKieSession(); |
There was a problem hiding this comment.
This test builds a KieModule containing only BruteForceAttack.drl, so it won’t catch regressions where the real classpath kmodule.xml doesn’t include the application rules package (or where the default KieSession name differs). Consider also loading kmodule.xml into the KieFileSystem (or using the same classpath KieContainer wiring used in integration tests) so the test validates the production rule-discovery/config path as well as rule behavior.
| @SuppressWarnings("unchecked") | ||
| private void insertFailedAuthenticationSessions(final KieSession ksession) { | ||
| final List<ApplicationActionEvent> failedAuthEvents = dataViewer.findListByProperty( | ||
| ApplicationActionEvent.class, | ||
| new Object[] { ApplicationOperationType.AUTHENTICATION, ServiceResult.FAILURE.toString() }, | ||
| ApplicationActionEvent_.applicationOperation, ApplicationActionEvent_.applicationMessage); | ||
|
|
||
| final Map<String, Long> failedCountsBySession = failedAuthEvents.stream() | ||
| .filter(event -> event.getSessionId() != null) | ||
| .collect(Collectors.groupingBy(ApplicationActionEvent::getSessionId, Collectors.counting())); | ||
|
|
||
| for (final Map.Entry<String, Long> entry : failedCountsBySession.entrySet()) { | ||
| final long failedCount = entry.getValue(); | ||
| if (failedCount > 0) { | ||
| final ApplicationComplianceCheckImpl check = new ApplicationComplianceCheckImpl( | ||
| entry.getKey(), "unknown", failedCount); |
There was a problem hiding this comment.
The @SuppressWarnings("unchecked") on this method appears redundant (DataViewer.findListByProperty is already generic; the unchecked warning is handled at the varargs declaration). Also, after groupingBy/counting(), every entry’s failedCount will be >= 1, so the if (failedCount > 0) branch is unnecessary noise.
| /** The minister. */ | ||
| MINISTER; | ||
| MINISTER, | ||
|
|
||
| /** The application. */ | ||
| APPLICATION; |
There was a problem hiding this comment.
Adding ResourceType.APPLICATION may require updating code/tests that validate ResourceType against a fixed allowlist. For example, service.impl/src/test/java/com/hack23/cia/service/impl/rules/RiskRulesITest currently asserts violations are only POLITICIAN/PARTY/COMMITTEE/MINISTRY; it will fail once APPLICATION violations are produced.
|
@copilot apply changes based on the comments in this thread |
…kmodule.xml in test, update RiskRulesITest - Remove redundant @SuppressWarnings("unchecked") annotation - Remove unnecessary `if (failedCount > 0)` guard (groupingBy/counting always produces >= 1) - Load kmodule.xml into KieFileSystem in BruteForceAttackRulesTest to validate production config - Add ResourceType.APPLICATION to RiskRulesITest allowlist and switch statement Agent-Logs-Url: https://github.com/Hack23/cia/sessions/0bc92915-dfbb-462c-bfff-0fb2c3b1e729 Co-authored-by: pethers <1726836+pethers@users.noreply.github.com>
All three review comments addressed in commit
|
|
📚 Javadoc GeneratedThe Javadoc documentation has been generated for this PR. Download the |
Description
The
BruteForceAttack.drlfile contained only a package declaration with zero rules — a gap in the security controls documented in SECURITY_ARCHITECTURE.md and THREAT_MODEL.md. This implements 3-tier brute force detection rules integrated with the existing Drools compliance check framework.Rules implemented:
Key changes:
ResourceType.java— addedAPPLICATIONenum valueApplicationComplianceCheckImpl.java— new compliance check wrapper tracking sessionId and failed auth countBruteForceAttack.drl— 3 rules usingmveldialect (ECJ compiler incompatible with JDK 26), with rule names accurately reflecting non-overlapping ranges ("5 to 9", "10 to 19", "20 or more")kmodule.xml— addedcom.hack23.cia.service.impl.rules.applicationpackageRulesEngineImpl.java— queries failedAUTHENTICATIONevents whereapplicationMessage == ServiceResult.FAILUREviaDataViewer.findListByProperty()in a single query, groups by session ID usingCollectors.counting()for memory efficiency, and inserts compliance checks into the KIE sessionBruteForceAttackRulesTest.java— 9 unit tests loading bothkmodule.xmlandBruteForceAttack.drlintoKieFileSystemto validate production rule-discovery config alongside rule behavior; covers thresholds, boundaries, rule firing count verification, and property correctnessRiskRulesITest.java— updatedResourceTypeallowlist and switch to includeAPPLICATION, preventing integration test failures when application-level violations are producedType of Change
Primary Changes
Political Analysis
Technical Changes
Impact Analysis
Political Analysis Impact
Technical Impact
DataViewer.findListByProperty()query to fetch only failed AUTHENTICATION events withapplicationMessage=FAILURE, then groups to counts viaCollectors.counting()(no full event lists retained in memory), avoiding both N+1 lazy-load overhead and unnecessary memory allocationServiceResult.FAILUREinapplicationMessageconsistent withLoginBlockedAccessImplTesting
9 unit tests validate: below-threshold no-fire, all 3 severity thresholds, extreme counts, single-rule-fires verification (asserts
fireAllRules()return value equals 1), boundary conditions, and property correctness. Tests loadkmodule.xmlintoKieFileSystemto validate the production rule-discovery/config path.RiskRulesITestupdated to includeAPPLICATIONin theResourceTypeallowlist and switch statement. All 38 existing service.impl tests pass.Documentation
Screenshots
N/A — backend rule engine changes only.
Related Issues
Checklist
Additional Notes
Uses
mveldialect instead ofjavabecause the Drools ECJ (Eclipse Java Compiler) has a known NPE inmoduleNotFoundwhen running under JDK 26. MVEL handles the simpleaddViolation()calls identically. Unit tests load bothkmodule.xmlandBruteForceAttack.drlintoKieFileSystemvia programmaticKieBuilderAPI to validate production rule configuration alongside rule behavior.Failed authentication detection uses
applicationMessage == ServiceResult.FAILURE.toString()(matching theLoginBlockedAccessImplpattern) rather than checkingerrorMessage, which could undercount failures where no error text is set.DRL rule names accurately reflect non-overlapping conditions: "5 to 9", "10 to 19", and "20 or more" — since
kcontext.getRule().getName()is persisted as the violation description, accuracy prevents misleading RuleViolation records.IP information is set to
"unknown"sinceApplicationActionEventdoes not carry IP data; the session ID serves as the primary identifier for compliance check grouping.Committee and ministry rule packages remain absent from
kmodule.xml— that's a pre-existing issue not addressed here.Security Considerations
Implements MITRE ATT&CK T1110 (Brute Force) detection per THREAT_MODEL.md. CodeQL scan: 0 alerts.
Release Notes
Added application-level brute force attack detection via Drools rules engine. Sessions with 5+ failed authentication attempts (identified by
applicationMessage=FAILURE) now generateRuleViolationentries at MINOR/MAJOR/CRITICAL severity levels.