santad: persist network flow rules in SNTRuleTable#969
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR adds comprehensive support for network flow rules to Santa's rule persistence layer: new public APIs, DB migration (v13) with a network_flow_rules table, deterministic hashing and cache invalidation, transactional write/cleanup integration, bulk retrieval, updated callers/build deps, and extensive tests. ChangesNetwork Flow Rules Feature
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs:
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Adds a third sub-table (network_flow_rules) and extends the combined addExecutionRules:fileAccessRules:... entry point to accept a networkFlowRules: parameter so all three rule types apply in a single transaction (matching how sync delivers them together in RuleDownloadResponse). networkFlowRulesHash joins SNTRuleTableRulesHash and is computed via hashOfHashes alongside the other two. Storage holds the raw NetworkFlowRule.Add proto blob as received over the wire; santad does not deserialize or re-serialize it. Hash is Xxhash128 over the blobs in rule_id ASC order — rule_id is field 1 of the proto so it does not need to be hashed separately. Retrieve and hash loops use indexed-column FMDB access because enterprise blocklists are expected to reach 100k+ rules. XPC delivery to santanetd and the corresponding XPC entry on SNTDaemonControlController will follow in a later PR.
fadb0b7 to
f061a8c
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Source/santad/DataLayer/SNTRuleTableTest.mm (1)
842-954: ⚡ Quick winAdd a mixed-rule rollback atomicity test.
Coverage is strong for network-flow behavior, but it doesn’t explicitly verify all-or-nothing rollback when a combined execution/file-access/network write fails. Add one failing mixed-input case and assert all three table counts remain unchanged.
Proposed test addition
+ - (void)testAddMixedRulesFailureRollsBackAllTables { + SNTRule* validExec = [self _exampleBinaryRule]; + SNTFileAccessRule* validFAA = [self _exampleFileAccessAddRuleWithName:@"mixed"]; + SNTNetworkFlowRule* validNF = [self _exampleNetworkFlowAddRuleWithId:500 blob:@"ok"]; + + SNTRule* invalidExec = [[SNTRule alloc] init]; + invalidExec.identifier = @"7ae80b9ab38af0c63a9a81765f434d9a7cd8f720eb6037ef303de39d779bc258"; + invalidExec.type = SNTRuleTypeCertificate; + // Intentionally missing state -> invalid. + + NSArray<NSError*>* errors; + XCTAssertFalse([self.sut addExecutionRules:@[ validExec, invalidExec ] + fileAccessRules:@[ validFAA ] + networkFlowRules:@[ validNF ] + ruleCleanup:SNTRuleCleanupNone + errors:&errors]); + XCTAssertGreaterThan(errors.count, 0u); + XCTAssertEqual(self.sut.executionRuleCount, 0); + XCTAssertEqual(self.sut.fileAccessRuleCount, 0); + XCTAssertEqual(self.sut.networkFlowRuleCount, 0); +}🤖 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 `@Source/santad/DataLayer/SNTRuleTableTest.mm` around lines 842 - 954, Add a test that verifies atomic rollback when a mixed add (execution + file-access + network) fails: use the existing helpers (_exampleBinaryRule, _exampleFileAccessRuleWithId:, _exampleNetworkFlowAddRuleWithId:) to insert one of each, arrange inputs so the addExecutionRules:fileAccessRules:networkFlowRules:ruleCleanup:errors: call fails (e.g. include an invalid rule to trigger failure), then assert that all three table counts (e.g. self.sut.executionRuleCount, self.sut.fileAccessRuleCount, self.sut.networkFlowRuleCount) are unchanged after the failure; this should mirror existing tests like testAddNetworkFlowRulesPersists and testAddAllEmptyArraysWithoutCleanupFails and ensure full rollback atomicity.
🤖 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 `@Source/santad/DataLayer/SNTRuleTableTest.mm`:
- Around line 842-954: Add a test that verifies atomic rollback when a mixed add
(execution + file-access + network) fails: use the existing helpers
(_exampleBinaryRule, _exampleFileAccessRuleWithId:,
_exampleNetworkFlowAddRuleWithId:) to insert one of each, arrange inputs so the
addExecutionRules:fileAccessRules:networkFlowRules:ruleCleanup:errors: call
fails (e.g. include an invalid rule to trigger failure), then assert that all
three table counts (e.g. self.sut.executionRuleCount,
self.sut.fileAccessRuleCount, self.sut.networkFlowRuleCount) are unchanged after
the failure; this should mirror existing tests like
testAddNetworkFlowRulesPersists and testAddAllEmptyArraysWithoutCleanupFails and
ensure full rollback atomicity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: b42df6bf-a774-4c94-bc3e-4c70d66b06cd
📒 Files selected for processing (5)
Source/santad/BUILDSource/santad/DataLayer/SNTRuleTable.hSource/santad/DataLayer/SNTRuleTable.mmSource/santad/DataLayer/SNTRuleTableTest.mmSource/santad/SNTDaemonControlController.mm
Summary
network_flow_rules(migration v12 → v13) holding rawNetworkFlowRule.Addproto blobs keyed byrule_id.addExecutionRules:fileAccessRules:…to also takenetworkFlowRules:, so all three rule types from a singleRuleDownloadResponseapply in one transaction. The 3-arg legacy wrapper is preserved for the compiler/standalone callers.networkFlowRulesHashtoSNTRuleTableRulesHash, populated viahashOfHashes. Xxhash128 over the blobs inrule_id ASCorder —rule_idis field 1 of the proto so it's not hashed separately.Dependencies
Stacked on #965 (
mlw/network-rules-common, currently in review). This PR's base is that branch — please merge #965 first; GitHub will retarget this PR tomainautomatically. The two are intended to stand independently in review.XPC delivery to
santanetdand the correspondingSNTDaemonControlControllerentry are out of scope here and will follow in a later PR.Test plan
bazel test //Source/santad:SNTRuleTableTest— 16 new tests covering persist / upsert / remove / remove-nonexistent / CleanAll / CleanNonTransitive / CleanupExecutionRules-preserves / empty-array rejection / network-only non-empty guard / hash stability / hex length / blob sensitivity / order independence / empty-table.bazel test //Source/santad:SNTDaemonControlControllerTest //Source/santad:SNTCompilerControllerTest //Source/santad:SNTExecutionControllerTest— existing tests still pass with the 5-arg signature.bazel build //Source/santad:SantadTesting/check_test_suites.shclean.