feat(threat-intel): ask Brolga for reputation on extracted indicators - #48
Merged
Conversation
Adds Brolga as a ReputationProvider alongside VirusTotal, AbuseIPDB, and GreyNoise. Brolga is the operator's own intelligence store rather than a third party's, so it is asked about every indicator kind Tawny extracts whenever it is configured, not just IPs. Follows the KelpieAlertSink template: typed HttpClient, bearer token from IOptions, absolute-URL validation, per-call timeout from the existing ReputationOptions. The disposition mapping is the part worth reviewing, because a wrong answer here changes whether an alert fires: - unknown -> Unknown, never Clean. Brolga's "unknown" means it has not heard of the indicator. Reading that as clean would suppress an alert that nothing has actually cleared. - Anything unrecognised also falls back to Unknown, so a later Brolga adding a disposition cannot silently start suppressing alerts. - allow_listed gets its own verdict rather than being folded into Clean. Clean is a finding about the indicator; allow-listed is a decision about how it is treated regardless of the finding, and collapsing them would let a feed's opinion override an operator's decision. ReputationVerdict.AllowListed = 5; existing values keep their numbers because cached rows are keyed by them. - A failed lookup is Error, not Unknown. "Brolga did not answer" and "Brolga has not heard of this" are different facts and only one says anything about the indicator. The pack's evidence, entities, and gaps are carried into the cached detail. A verdict an analyst cannot trace to a source is one they cannot act on with confidence. A configured base URL with no token means Brolga is not offered at all: Brolga refuses to serve a reachable address without one, so asking would only produce a 401 and a cached Error. The test handler records every request rather than the last one. GreyNoise needs no API key and always runs for an IPv4 indicator, so a handler keeping only the most recent request would have made these assertions depend on provider order. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds Brolga as a configurable reputation provider, sends authenticated context requests for supported indicators, maps dispositions to Tawny verdicts, preserves response details, and validates enrichment behavior through HTTP-backed tests. ChangesBrolga reputation integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant LookupAsync
participant ReputationEnricher
participant BrolgaAPI
participant TawnyDbContext
LookupAsync->>ReputationEnricher: request indicator reputation
ReputationEnricher->>BrolgaAPI: POST /api/v1/context with bearer token and subject
BrolgaAPI-->>ReputationEnricher: return disposition and context details
ReputationEnricher->>TawnyDbContext: cache verdict and enriched detail
TawnyDbContext-->>LookupAsync: return reputation result
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
backend/src/Tawny.Infrastructure/ThreatIntel/ReputationEnricher.cs (1)
356-374: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid double-parsing and leaking
JsonDocumentinstances for evidence/entities/gaps.
GetRawText()is called one/g/n(already-parsedJsonElements fromdoc), then the resulting strings are re-parsed viaJsonDocument.Parse(...)— three moreJsonDocumentinstances per Brolga response that are never disposed.JsonDocumentrents its backing arrays fromArrayPool<T>and must be disposed to return them; skipping that increases GC pressure on every reputation lookup that hits Brolga. Simplify by cloning the elements directly from the already-opendocbefore it's disposed.♻️ Proposed fix
- // Carried through so an analyst can see where the verdict came from. A verdict with - // nothing to cite is one nobody can act on with confidence. - var evidence = root.TryGetProperty("evidence", out var e) ? e.GetRawText() : "[]"; - var gaps = root.TryGetProperty("gaps", out var g) ? g.GetRawText() : "[]"; - var entities = root.TryGetProperty("entities", out var n) ? n.GetRawText() : "[]"; + // Carried through so an analyst can see where the verdict came from. A verdict with + // nothing to cite is one nobody can act on with confidence. Clone directly out of `doc` + // (rather than round-tripping through GetRawText + a second JsonDocument.Parse) so no + // extra, undisposed JsonDocument instances are left renting from the ArrayPool. + static JsonElement CloneOrEmptyArray(JsonElement root, string property) => + root.TryGetProperty(property, out var value) + ? value.Clone() + : JsonDocument.Parse("[]").RootElement; + + var evidence = CloneOrEmptyArray(root, "evidence"); + var gaps = CloneOrEmptyArray(root, "gaps"); + var entities = CloneOrEmptyArray(root, "entities"); return new ReputationLookup( ReputationProvider.Brolga, verdict, null, new { disposition, observable_id = root.TryGetProperty("observable_id", out var o) ? o.GetString() : null, schema_version = root.TryGetProperty("schema_version", out var s) ? s.GetString() : null, - entities = JsonDocument.Parse(entities).RootElement.Clone(), - evidence = JsonDocument.Parse(evidence).RootElement.Clone(), - gaps = JsonDocument.Parse(gaps).RootElement.Clone(), + entities, + evidence, + gaps, });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@backend/src/Tawny.Infrastructure/ThreatIntel/ReputationEnricher.cs` around lines 356 - 374, In the Brolga response mapping, replace the GetRawText and JsonDocument.Parse flow for evidence, gaps, and entities with direct Clone calls on the corresponding JsonElements from root. Preserve the existing empty-array defaults when those properties are absent, and keep the cloned elements in the returned ReputationLookup metadata without creating additional undisposed JsonDocument instances.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@backend/src/Tawny.Infrastructure/ThreatIntel/ReputationEnricher.cs`:
- Around line 356-374: In the Brolga response mapping, replace the GetRawText
and JsonDocument.Parse flow for evidence, gaps, and entities with direct Clone
calls on the corresponding JsonElements from root. Preserve the existing
empty-array defaults when those properties are absent, and keep the cloned
elements in the returned ReputationLookup metadata without creating additional
undisposed JsonDocument instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 74899021-6925-4dfc-b361-fe28973e6b6d
📒 Files selected for processing (5)
backend/src/Tawny.Api/appsettings.jsonbackend/src/Tawny.Domain/Enums.csbackend/src/Tawny.Infrastructure/ThreatIntel/ReputationEnricher.csbackend/tests/Tawny.Api.Tests/BrolgaReputationTests.csdocker/docker-compose.yml
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Adds Brolga as a
ReputationProvideralongside VirusTotal, AbuseIPDB, and GreyNoise, so a detection can be checked against the operator's own intelligence before it becomes a case.Brolga is asked about every indicator kind Tawny extracts — not just IPs — because it is the operator's own store rather than a third party's rate-limited API.
Follows the
KelpieAlertSinktemplate: typedHttpClient, bearer token fromIOptions, absolute-URL validation, per-call timeout from the existingReputationOptions.The disposition mapping is the part worth reviewing
A wrong answer here changes whether an alert fires.
maliciousMalicioussuspiciousSuspiciousbenignCleanallow_listedAllowListed(new)unknownUnknown— neverCleanUnknownErrorallow_listedgets its own verdict rather than being folded intoClean. Clean is a finding about the indicator; allow-listed is a decision about how it is treated regardless of the finding. Collapsing them would let a feed's opinion override an operator's decision.ReputationVerdict.AllowListed = 5— existing values keep their numbers, becauseReputationCacheEntryrows are keyed by them and renumbering would silently reinterpret cached verdicts.Evidence is carried through
The pack's
evidence,entities, andgapsgo into the cached detail. A verdict an analyst cannot trace back to a source is one they cannot act on with confidence.Misconfiguration is not asked
A configured base URL with no token means Brolga is not offered at all. Brolga refuses to serve a reachable address without one, so asking would only produce a 401 and a cached
Error.A test-harness note
RecordingHandlerrecords every request, not the last one. GreyNoise needs no API key and always runs for an IPv4 indicator, so a handler keeping only the most recent request would have made these assertions depend on the order providers happen to run in. The first version of the test did exactly that and failed for that reason.Verification
dotnet test Tawny.sln— 103 passed, 0 failed. 11 of those are new.Config
Tawny:Reputation:BrolgaBaseUrl/BrolgaApiToken, withTAWNY_BROLGA_BASE_URL/TAWNY_BROLGA_API_TOKENin compose. Origin only — the/api/v1prefix is added when the request is built.Summary by CodeRabbit
New Features
Bug Fixes