feat(identifiers): file hashes on the file entity, for risk acceptance by hash - #704
Conversation
…e by hash Risk acceptance compares an exception only against the alert's identifier block, so a hash-scoped exception had nothing to match on: FileEntity carried the file name and its directory and no hash. Flatten() is the whole contract - both postgres-connector's SQL predicate and event-ingester's exception cache key range over it and hold no list of known fields - so adding the three hashes here makes hash scoping reach the query and the cache with no logic change downstream. Values are lower case. The comparison is string equality or LIKE, and neither folds case, so the producer and the exception writer both normalise. Also writes out MalwareSignature's bson tags. They only spell out what the driver already did, and the point is FirstSeen: with no tag the driver keyed it by the lower-cased Go name, so it is stored as "firstseen" while the same field is served as "first_seen". Renaming the stored key would orphan the value in every document already written, so both names are now pinned by a test. The doc says why, and records that an unset FirstSeen is served as year 1 rather than omitted, because omitempty cannot suppress a struct. Signed-off-by: Alon Liwsky <alon@armosec.io>
Both were found by an adversarial review that ran the code instead of reading the comments, and both were wrong in the same direction: the change described itself as smaller than it was. The bson tags did NOT "spell out what the driver already does". They carried ",omitempty", and time.Time satisfies the driver's Zeroer interface, so an unset FirstSeen - and every other empty field - would have started being dropped from stored documents, splitting the corpus into two shapes. The omitempty is gone, so the claim is now true, and a test pins the SPARSE document, which the original fully-populated fixture could never have caught. The casing contract had no code behind it in this repository and it contradicts cdr-azure-event-shape.md, which says to match case-insensitively and normalize nowhere. NormalizeHashScopeValues now lives here, so the two write paths call one function instead of keeping their own copies, and the doc says why a hex digest is not an Azure subscription id: no original casing to preserve, no meaning in its case, and no case-insensitive operator on the Postgres comparison at all. Also: the doc said the producer is filled in event-ingester-service as a plain fact, when that is a decision this epic makes and a change of owner from what the plan says. It is now labelled as such. Hash cases added to the exception-filter table - that function is a third generic consumer in this repo and it had none. The identifier keys are a stored contract, so a literal assertion pins them; the JSON- derived one cannot fail if a key changes on both sides at once. Signed-off-by: Alon Liwsky <alon@armosec.io>
AlonLiwsky
added
the
ai-assisted
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 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. Comment |
There was a problem hiding this comment.
Pull request overview
Adds file-hash identifiers (md5/sha1/sha256) to the runtime alert identifier contract so risk acceptance rules can match specific known-good files by hash, and introduces shared normalization to prevent case-mismatch drift across services. Also hardens Mongo storage compatibility for malware signatures by pinning BSON field names (notably firstseen) with tests and documentation.
Changes:
- Extend
common.FileEntity+Identifiers.Flatten()to emitfile.md5,file.sha1,file.sha256for exception matching. - Add
armotypes.NormalizeHashScopeValuesand tests to ensure hash-scoped exception values are case-folded consistently and stay in sync withFlatten(). - Add explicit BSON tags for
MalwareSignatureplus tests/docs to prevent silent key renames andomitempty-driven storage shape changes.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| docs/features/malware-file-hash-identifiers.md | Documents the rationale, contracts, and cross-repo surfaces for hash-based identifiers and BSON key stability. |
| armotypes/malware.go | Adds explicit BSON tags to preserve Mongo key names (especially firstseen) and documents the no-omitempty constraint. |
| armotypes/malware_test.go | Pins BSON vs JSON key behavior and guards against omitempty changing stored document shape. |
| armotypes/exceptionpolicy.go | Introduces hash-entity set and in-place normalization for hash-scoped advanced-scope values. |
| armotypes/exceptionpolicy_test.go | Adds coverage for hash scope filter paths and normalization behavior, including drift prevention vs Flatten(). |
| armotypes/common/runtime_common.go | Adds file hash fields to FileEntity and emits them from Identifiers.Flatten(). |
| armotypes/common/runtime_common_test.go | Extends flatten tests and pins stable identifier key literals for file entities (including hashes). |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Coverage Report for CI Build 33317130358Warning No base build found for commit Coverage: 48.413%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
Coverage Report for CI Build 33375796026Warning No base build found for commit Coverage: 48.443%Details
Uncovered ChangesNo uncovered changes found. Coverage RegressionsRequires a base build to compare against. How to fix this → Coverage Stats
💛 - Coveralls |
…promising Review feedback from Copilot, both fair. HashScopeEntities was an exported map. An exported map is mutable by any consumer, and this one decides whether a value is folded before it is compared - so a stray write anywhere would change matching behaviour globally and silently. It is now unexported behind IsHashScopeEntity. The FileEntity comment said the hashes ARE lower case, which reads as a guarantee this struct does not give. It now says callers are expected to store them lower case and points at who actually folds them. Also renames a test case that said "hashes only" while its fixture carried a name. Signed-off-by: Alon Liwsky <alon@armosec.io>
|
Thanks — all three taken, in Mutable exported map. Fair, and it is the one with teeth: that map decides whether a value is folded before it is compared, so a stray write anywhere in the estate would change matching globally and silently. It is now unexported behind The comment promised a guarantee the type does not give. Reworded — callers are expected to store lower case, and the comment now points at who actually folds and why it has to happen on write. The type genuinely cannot enforce it: Test case named "hashes only" with a name in the fixture. Renamed to what it actually covers — a name and a hash with no directory, which is what a bare file path produces. |
|
Thanks — all three taken, in Mutable exported map. Fair, and it is the one with teeth: that map decides whether a value is folded before it is compared, so a stray write anywhere in the estate would change matching globally and silently. It is now unexported behind The comment promised a guarantee the type does not give. Reworded — callers are expected to store lower case, and the comment now points at who actually folds and why it has to happen on write. The type genuinely cannot enforce it: Test case named "hashes only" with a name in the fixture. Renamed to what it actually covers — a name and a hash with no directory, which is what a bare file path produces. |
…ay files The third adversarial review found the same class of defect as the first two: a comment claiming more than the code does. This one is mine twice over, because I had already fixed exactly this in config-service and did not carry the fix here. I fixed an instance, not the class. The guard said "Derive the list from Flatten() so the two cannot drift". It derived from a HAND-WRITTEN fixture. I reproduced the reviewer's mutation: add a SHA512 field to FileEntity and a Flatten case for it - the drift the comment describes - and every test in the repository still passed, because the fixture never set the new field so Flatten never emitted it so the loop never checked it. The fixture is now filled by reflection, and every FileEntity field must be a declared decision: a hash the fold covers, or a value deliberately left alone because it is case-significant on Linux. Both mutations now fail loudly - a new field with no fold rule, and a new field with no Flatten case at all. Also from the review: - The FileEntity comment sent the reader to NormalizeHashScopeValues for "who does the folding". That function folds the exception side and never touches FileEntity. It now names both writers, one per side of the comparison. - Recorded the residual risk two reviewers have now raised: nothing validates the entity name on the way in, so an unrecognised one is stored and silently matches nothing. Closing that means rejecting unknown entities at the API boundary, which also affects the CDR entities and needs its own decision. - docs frontmatter was missing the two files a whole section of the doc is about. - A gofmt sweep had pulled three unrelated files into the diff. Reverted. - A fixture used the real dev signature row id. Synthetic now. Signed-off-by: Alon Liwsky <alon@armosec.io>
Why
Risk acceptance compares an exception against one thing only: the
identifiersblock on the alert. Today that block carries a file name and a directory, and no hash — so an analyst can create an "accept this SHA256" rule and it can never match.Accepting by hash clears one known-good file by identity. A path exception does not: copy or move the file and it stops applying, and it covers every other file that later lands on that path. This is FR-17.
What changed
common.FileEntitygainsMD5,SHA1,SHA256;Identifiers.Flatten()emitsfile.md5,file.sha1,file.sha256.armotypes.NormalizeHashScopeValues— one definition of "which entities are hashes and fold their case", socadashboardbeandevent-ingester-servicecannot drift apart.MalwareSignaturegets explicit bson tags. See the second note below; this one has a trap in it.Why adding a field here is enough — and where it is not
Flatten()is the whole contract for the two generic consumers:postgres-connectorbuilds the SQL predicate by ranging over it, andevent-ingester-servicekeys its exception cache the same way. Neither holds a list of known fields, so a new key starts affecting the query and starts separating cache entries at the same moment.Four surfaces are not generic and are hand-written. All four are changed in this epic's other PRs. None of them was ever changed for
network.sourceIP, which is why that field is unusable in the form, unsearchable and unindexed seven weeks after it shipped:armo-ng-appcadashboardbeconfig-serviceevent-ingester-serviceThere is a third generic consumer in this repo —
GetRuntimeIncidentsRequestFilterFromExceptionPolicy, which drives the "how many incidents would this affect" preview and the retroactive resolve on a new exception. It needed no change, and now has test coverage for hashes, which it did not have.Two things worth a reviewer's attention
1. The producer moves repo, deliberately. The execution plan assigns "fill alert Identifiers" to the sensor stream in
private-node-agent. The hashes are filled server-side instead, inevent-ingester-service. A sensor fix reaches a customer only when they update the node agent, which we do not control; the backend already holds the hash on every agent version. It also repairs path scoping for agents older than private-node-agent 546, which send no identifiers at all. The doc records this as a decision rather than stating it as fact.2. The bson tags carry no
,omitempty, and that omission is load-bearing.MalwareSignaturehad no bson tags, so the driver keyed every field by its lower-cased Go name — which meansFirstSeenis stored asfirstseenwhile JSON servesfirst_seen. Renaming the stored key would orphan the value in every existing document, so both names are now written out and pinned by a test. Adding,omitemptywould have silently started dropping empty fields, becausetime.Timesatisfies the driver'sZeroerinterface. A test pins the sparse document; a fully populated fixture cannot see that.Merge order
This is the base library. Merge and tag it first, then the three Go consumers re-pin from the pseudo-version to the tag:
config-service,cadashboardbe,event-ingester-servicearmo-ng-appTest plan
go test ./...— 13 packages, no failures.Flatten()emission fails; adding,omitemptyback fails; deleting an index fails its guard.Review
An adversarial clean-context review ran before this opened and found the
omitemptybehaviour change and a casing claim with no code behind it. Both are fixed in the second commit.AI-skills: armosec-shared-rules:multi-repo-feature-workflow,armosec-shared-rules:fe-design-system