Skip to content

refactor: detection rule sets as a registry (#478) - #517

Open
n1ckl0sk0rtge wants to merge 23 commits into
mainfrom
refactor/detection-rule-set-registry
Open

refactor: detection rule sets as a registry (#478)#517
n1ckl0sk0rtge wants to merge 23 commits into
mainfrom
refactor/detection-rule-set-registry

Conversation

@n1ckl0sk0rtge

Copy link
Copy Markdown
Contributor

Closes #478.

What this does

Every detection-rule class used to cache its own rule list with a Memoize.of(...) supplier and hand it out through a public static rules() method. Because that accessor was public and static — and because several classes had context-parameterized variants that skipped the memo — callers could rebuild whole rule subtrees. That is issue #476. The C# module had no caching at all and rebuilt everything on every call.

This replaces that with a registry in the engine module, package com.ibm.engine.rule:

  • DetectionRuleSet<T> — a rule class extends it and implements only protected List<IDetectionRule<T>> buildRules(). protected is the point: nothing outside com.ibm.engine.rule can call it, and every rule class is final, so there is no way to widen access by subclassing.
  • ContextualDetectionRuleSet<T> — for rule sets whose rules depend on one or more IDetectionContext. The context list is positional; a null means "use the default for that position"; read it with contextAt(list, index).
  • RuleSets — the single door. rulesOf(Class) caches in a ClassValue; rulesOf(Class, IDetectionContext...) caches in a ConcurrentHashMap keyed on class plus contexts.
  • Detection contexts gained value-based equals/hashCode so they can be part of that key, and DetectionContext now copies its property map defensively.
  • RuleAccessorEnforcementTest in each of the four language modules scans the compiled classes and fails the build if any class under com.ibm.plugin.rules.detection exposes a public static method returning a rule list. The compiler cannot police a static method; this test can.

All four language modules are converted: java, python, go and csharp.

Result

Distinct reachable rule objects: 2,563 → 1,015 (a 60% reduction). The plan predicted only a small drop; most of the gain came from two places:

  • Context-parameterized calls such as BcDigests.rules(ctx) used to rebuild a fresh subtree on every call. They now share one.
  • RuleSets.rulesOf(X.class, someNullableContext) always binds the varargs overload, even when the value is null, so it landed on a separate cache entry from a bare rulesOf(X.class). RuleSets now trims trailing nulls and falls through to the default path — exact rather than heuristic, because contextAt already returns null past the end of the list. That one change alone took the count from 1,069 to 1,015.

Evidence the rules themselves did not change

The exported rule JSON was generated from the branch base in a separate worktree before any of this work and compared byte-for-byte after:

Module md5 Nodes
java a19a6ddf64e91bb156a39e4134677da8 515
python d1c5ce3945ec370d15c5118372e0f043 114

Both identical before and after. Go and C# have no export test; their suites are the content check.

mvn clean package: BUILD SUCCESS, 433 tests, 0 failures, 0 errors. The base measured 413; the delta is +21 new engine tests, −2 net in java (two test classes deleted, one added) and +1 in csharp.

Also in this PR

  • docs/LANGUAGE_SUPPORT.md and docs/DETECTION_RULE_STRUCTURE.md still taught the public static rules() pattern that the build now rejects. Both are updated — a contributor following them would otherwise have written a class that neither compiled nor passed the guard test.
  • RuleGraphMemoizationTest asserted the object count was under 60,000 while the real value is 1,015. Tightened to 1,500 so the result is actually defended by CI. This test is also the only automated guard against the other route back to Reduce detection-rule memory footprint (BouncyCastle rule graph OOMs the Scanner Engine) #476: a buildRules() that builds a fresh context per call, which the accessor guard cannot see.
  • Two rule sets were extracted so each class owns exactly one list: BcBlockCipherAndEngines (was BcBlockCipher.all()) and BcCipherParametersBases (was BcCipherParameters.bases(), which was never memoized and was called repeatedly).
  • PycaHashWrapper in python, for the same reason (PycaHash.wrapperRules()).

Reviewing this

The bulk of the diff is the same mechanical conversion across ~180 rule classes. The design-bearing files are:

  • engine/src/main/java/com/ibm/engine/rule/RuleSets.java, DetectionRuleSet.java, ContextualDetectionRuleSet.java
  • engine/src/main/java/com/ibm/engine/model/context/DetectionContext.java and its four siblings
  • java/src/test/java/com/ibm/plugin/rules/RuleAccessorEnforcementTest.java

Note the contextual cache deliberately uses get / build / putIfAbsent rather than computeIfAbsent: rule builds are recursive — a set asks the registry for another set while it is itself being built — and a nested update inside computeIfAbsent throws. There is a comment saying so at the call site.

Known limitation

RuleSets.instantiate reports "could not be instantiated; it needs an accessible no-argument constructor" for every reflective failure. That is accurate when the constructor is missing, but misleading for an abstract subclass or a constructor that throws — both of which do have one. It cannot fire today (every rule class is final and concrete), and the real cause is chained. Left as-is.

@n1ckl0sk0rtge
n1ckl0sk0rtge requested a review from a team as a code owner August 21, 2026 07:33
@n1ckl0sk0rtge n1ckl0sk0rtge self-assigned this Aug 21, 2026
@n1ckl0sk0rtge n1ckl0sk0rtge added the enhancement New feature or request label Aug 21, 2026
@n1ckl0sk0rtge
n1ckl0sk0rtge force-pushed the refactor/detection-rule-set-registry branch from dd205d1 to 8cdd7f8 Compare August 21, 2026 13:50
Replaces the per-class Memoize.of boilerplate with an abstract base plus a
RuleSets registry that owns the cache. Rule classes keep only buildRules();
there is no public path that can rebuild a subtree.

Also caches the context-parameterized path, which needs equals/hashCode on
the detection context classes.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Eleven tasks across five PRs. The java module converts in three passes with
temporary rules() shims so the build stays green at every commit.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
ContextualLeaf.buildRules() returned List.of() unconditionally, which
the JDK interns to one shared empty-list singleton, so isSameAs and
isNotSameAs assertions passed regardless of cache correctness. Return
a fresh, non-empty, mutable list per build instead, restoring the
brief's six assertions verbatim (#478).

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…ntextualTest

isNotSameAs alone would pass even with no cache at all, since every
call then allocates a fresh list. Add a LEAF_BUILDS counter and assert
the build-count delta is exactly 2 alongside the existing isNotSameAs
checks in differentContextsGetDifferentLists and
positionMattersWhenOneOfTwoContextsIsNull, so the tests fail whether
the cache key over-collapses or the cache is missing entirely (#478)

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…ry (#478)

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…#478)

RuleSets.rulesOf(Class, IDetectionContext...) always bound the varargs
overload once any argument was passed, even a single null. That put a
trailing-null call under a distinct CONTEXTUAL cache key instead of the
DEFAULTS entry the no-arg overload uses, so callers forwarding a nullable
context (e.g. BcBlockCipherAndEngines -> BcBlockCipherEngine.rules(ctx))
built and cached the same rules twice under two different keys.

Trim trailing nulls from the varargs before building the cache key: they
carry no information, since contextAt already returns null past the end
of the list. Interior nulls are still preserved and remain positionally
significant.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
)

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
)

returnsRuleList missed a raw List return and a raw IDetectionRule
element (both are genuine second doors around RuleSets). The
discoverClassNames $ filter also skipped every named nested class,
not just synthetic anonymous/lambda classes, so an accessor hidden
in a named nested class was never enumerated.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
LANGUAGE_SUPPORT.md and DETECTION_RULE_STRUCTURE.md still taught the
old public-static rules() pattern that RuleAccessorEnforcementTest now
rejects. Replace it with the current shape: extend DetectionRuleSet
(or ContextualDetectionRuleSet) and implement buildRules(), read rule
lists back only through RuleSets.rulesOf(...), and make the quoted
BcBlockCipher snippet match the real file.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
The measured distinct-rule-object count is 1,015, but the test only
asserted < 60,000 — a threshold so loose it defended nothing. Lower it
to 1500, modest headroom over the measured value and well under the
spec's 2,563 acceptance criterion. Rewrite the javadoc: it described
"memoization" (removed by this refactor) and a target of "the low tens
of thousands" (off by more than an order of magnitude). It now
describes what the test actually guards against: a buildRules() that
builds a fresh context per call, which would get its own RuleSets
cache entry and duplicate the subtree, invisible to
RuleAccessorEnforcementTest.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…#478)

The class javadoc claimed every set is "built at most once", which is
false under a race for both caches: ClassValue's own javadoc says
computeValue may run more than once concurrently with one result
installed, and the contextual path deliberately uses get/build/
putIfAbsent (not computeIfAbsent, since builds recurse) with the same
property. Reword to state what actually holds: lazy build, shared by
reference once installed, with a race resolved by discarding the
loser.

Also soften instantiate()'s exception message. It always claimed the
class "must have a no-argument constructor", but that's wrong for two
of its three failure modes: an abstract subclass throws
InstantiationException and a throwing constructor throws
InvocationTargetException, and both classes do have a no-arg
constructor. RuleSetsTest still asserts the message contains
"no-argument constructor", so that substring is kept.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…ext (#478)

Contexts are now used as RuleSets cache keys, but nothing said that
implementations must have value-based equals/hashCode covering all of
their state. KeyContext and SignatureContext already do this
correctly; write the rule down so a future subclass that adds state
does not forget to override equals and silently collide with a
sibling that has different state, handing that sibling's cached rules
to the wrong caller.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
…ntext (#478)

buildRules() reads contextAt(contexts, 0) and silently ignores any
context past index 0. No caller passes more than one today, so this is
not a live bug, and changing which contexts feed the key would change
cache keys — behaviour is unchanged, this only documents it.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
DetectionContextEqualityTest only asserted the negative case (KeyContexts
and SignatureContexts with different kinds are not equal), never the
positive one. Add a test that two KeyContexts built with the same kind
have both equal equals() and matching hashCode(), matching the style of
the existing sameClassAndSamePropertiesAreEqual test.

Signed-off-by: Nicklas Körtge <Nicklas.Koertge1@ibm.com>
@n1ckl0sk0rtge
n1ckl0sk0rtge force-pushed the refactor/detection-rule-set-registry branch from 8cdd7f8 to e98a6c2 Compare August 21, 2026 13:52
@n1ckl0sk0rtge

Copy link
Copy Markdown
Contributor Author

Performance test results

I ran both levels of docs/PERFORMANCE_TESTING.md on this branch. Heap stays small — no regression.

A. JUnit harness

Run Result Gate
CallStackHeapPerfTest, 200 files retainedWithTree=0 detached=367/367 ratio=1.000, 2.4 s pass
CallStackHeapPerfTest, 3000-file soak retainedWithTree=0 detached=5500/5500 ratio=1.000, 88 s pass
RuleGraphMemoizationTest 1015 distinct rule objects < 1500, pass

AST-detach is fully intact (ratio 1.000, nothing pinning a tree).

B. Keycloak end-to-end

Keycloak main (94 compiled modules), SonarQube 26.1, -Xmx6g, JDK 21.

  • ANALYSIS SUCCESSFUL in 13 min 27 s, no OOM.
  • CBOM generated: CycloneDX 1.6, 108 detected assets → 68 components. Same as the reference run in the doc, so detections really fired.
  • Heap oscillates (4553 → 3425 → 4602 → 3624 MB …). G1 reclaims, no monotonic climb. That is the healthy shape the doc describes.
  • Peak heap_used 5794 MB, peak RSS 6783 MB.

C. Attribution — the point of this PR

com.ibm.** bytes, sampled every 3 minutes after a forced GC:

Sample engine.rule callstack mapper.model plugin total whole-heap floor
early 0.07 MB / 1880 inst 0.10 MB 0 0.39 MB 0.33 GB
t1 0.07 MB / 1880 inst 4.58 MB 0.01 MB 8.52 MB 1.50 GB
t2 0.07 MB / 1880 inst 7.98 MB 0.01 MB 14.60 MB 2.48 GB
t3 0.07 MB / 1880 inst 12.42 MB 0.01 MB 22.51 MB 3.30 GB
t4 0.07 MB / 1880 inst 15.58 MB 0.01 MB 28.18 MB 4.09 GB

The rule graph is flat at 1880 instances / 0.07 MB for the whole scan. The RuleSets registry builds each rule set once and shares it by reference. No duplication creeps back in over a long run.

Total plugin footprint is 28.18 MB, essentially the same as the 28.4 MB recorded for the AST-detach run.

One difference worth flagging

The post-GC floor ended at 4.09 GB here, against 2.90 GB in the doc's 2026-07-06 run. This is not the plugin. The top retained classes at t4 are all byte[] (542 MB), Object[] (348 MB), HashMap$Node (335 MB), InternalPosition (154 MB) and ECJ MethodBinding (145 MB) — SonarQube and ECJ baseline. No com.ibm.* class appears anywhere near the top. Keycloak main has grown since July, so its baseline analysis cost is higher. The plugin's own share is unchanged.

Note: the scan needs the sonar-java-crypto:Inventory rule activated in the quality profile, otherwise nothing is detected and the measurement is meaningless.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor detection-rule classes onto an abstract base so memoization is structurally enforced (follow-up to #476)

1 participant