Add support for importing FAA rules#13
Merged
russellhancox merged 4 commits intoMar 27, 2026
Merged
Conversation
📝 WalkthroughWalkthroughAdds import support for Santa File Access Authorization (FAA) policies (.plist and .mobileconfig): new internal FAA parser, CLI flags to select FAA vs static rules, conversion to Workshop Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI as CLI (cmd/main.go)
participant Parser as FAA Parser (internal/faarules)
participant API as Workshop API (gRPC)
User->>CLI: Run tool with input file and flags
CLI->>CLI: Validate flags (mutual exclusion)
alt .plist input
CLI->>Parser: ParseRulesFromFile(filePath)
else .mobileconfig input
CLI->>Parser: ParseRulesFromMobileConfig(filePath)
end
Parser->>Parser: Decode plist/mobileconfig and extract FileAccessPolicy/WatchItems
Parser->>Parser: For each WatchItem:
Parser->>Parser: • Split Paths into literals vs prefixes
Parser->>Parser: • Build process filters (SigningID, TeamID, CDHash, CertSha256, BinaryPath)
Parser->>Parser: • Apply PlatformBinary → prefix "platform:"
Parser->>Parser: • Apply defaults and invert AuditOnly → BlockViolations
Parser-->>CLI: Return []*apipb.FileAccessRule
CLI->>CLI: Tag rules as "global"
loop per FAA rule
CLI->>API: CreateFileAccessRule(rule)
API-->>CLI: Success/Error
end
CLI-->>User: Report created rules and any errors
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
32-57:⚠️ Potential issue | 🟡 MinorRefresh the pasted
--helpoutput.This block still advertises only TOML/CSV inputs and omits
--faa-only/--static-rules-only, so the README no longer matches the actual CLI after this PR. Please regenerate it from current./santa-rule-importer --help.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 32 - 57, The README's pasted output of ./santa-rule-importer --help is outdated and omits new CLI flags (--faa-only and --static-rules-only) and updated accepted input types; regenerate and paste the current help text by running ./santa-rule-importer --help and replacing the block under the example usage in README.md so it exactly matches the live output (ensure the tool name santa-rule-importer and flags like --faa-only, --static-rules-only, --use-custom-msg-as-comment, --zentral-config-id, --zentral-target-identifier, --zentral-target-type, --zentral-url, and any updated input type descriptions appear verbatim).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Around line 118-120: The code discards the error from
faarules.ParseRulesFromMobileConfig; change the call to capture the error (e.g.,
faaRules, parseErr := faarules.ParseRulesFromMobileConfig(filename)) and
propagate or handle it instead of ignoring it: when parseErr != nil return or
exit non‑zero with a clear error (or append to ruleSrcErr) so that failures
parsing the .mobileconfig are not silently ignored; update the logic that checks
!*staticRulesOnly and ruleSrcErr to incorporate parseErr.
In `@internal/faarules/faarules.go`:
- Around line 100-109: The current implementation in faarules.go only inspects
config.PayloadContent[0] and returns nil if the first payload lacks a
FileAccessPolicy, which skips FAA rules in multi-payload profiles; update the
logic to iterate over all entries in config.PayloadContent, find the first
non-nil FileAccessPolicy, assign it to policy and call convertWatchItems(policy)
(returning its result), and only return nil,nil if no payload contains a
FileAccessPolicy; keep references to convertWatchItems, config.PayloadContent,
and the policy variable to locate where to change the code.
- Around line 149-156: The loop over item.Processes currently logs a warning
then returns nil, nil when PlatformBinary is true but SigningID is empty, which
silently drops the watch item; change that to return a non-nil error (e.g.,
fmt.Errorf("watch item %q: PlatformBinary set but SigningID is empty", name))
from the enclosing function instead of returning nil, nil so the caller fails
fast; update the function that builds signingIDs (references: item.Processes,
proc.PlatformBinary, proc.SigningID, signingIDs, name) to propagate this error,
and remove the unused log import if it becomes unused.
---
Outside diff comments:
In `@README.md`:
- Around line 32-57: The README's pasted output of ./santa-rule-importer --help
is outdated and omits new CLI flags (--faa-only and --static-rules-only) and
updated accepted input types; regenerate and paste the current help text by
running ./santa-rule-importer --help and replacing the block under the example
usage in README.md so it exactly matches the live output (ensure the tool name
santa-rule-importer and flags like --faa-only, --static-rules-only,
--use-custom-msg-as-comment, --zentral-config-id, --zentral-target-identifier,
--zentral-target-type, --zentral-url, and any updated input type descriptions
appear verbatim).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 635127ca-9666-4bf5-9b81-08b0d0e1bfe6
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
README.mdcmd/main.gogo.modinternal/faarules/faarules.gointernal/faarules/faarules_test.gointernal/faarules/testdata/faa_policy.plistinternal/faarules/testdata/santa_with_faa.mobileconfig
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
internal/faarules/faarules.go (2)
100-109:⚠️ Potential issue | 🟠 MajorOnly the first payload is inspected; FAA rules in subsequent payloads will be silently ignored.
Multi-payload mobileconfig profiles may have the Santa configuration at an index other than 0. The current logic only checks
PayloadContent[0], which could cause FAA rules to be missed.Suggested fix: iterate all payloads
if len(config.PayloadContent) == 0 { return nil, nil } - policy := config.PayloadContent[0].FileAccessPolicy - if policy == nil { - return nil, nil + for _, payload := range config.PayloadContent { + if payload.FileAccessPolicy != nil { + return convertWatchItems(payload.FileAccessPolicy) + } } - return convertWatchItems(policy) + return nil, nil🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/faarules/faarules.go` around lines 100 - 109, The code only inspects PayloadContent[0] and ignores FAA rules in later payloads; change the logic to iterate over all entries in config.PayloadContent, check each entry's FileAccessPolicy, call convertWatchItems(policy) for each non-nil policy, and aggregate the results (skip nil returns) into a single combined return value; ensure you continue on nil policies and return the merged slice/result instead of only the first payload's output (referencing config.PayloadContent, FileAccessPolicy, and convertWatchItems).
149-156:⚠️ Potential issue | 🟠 MajorSilent drop of watch item on malformed
PlatformBinaryinput.When
PlatformBinaryistruebutSigningIDis empty, the function logs a warning and returnsnil, nil, silently omitting the rule. For policy data, a hard error is safer to avoid reporting a successful import with missing coverage.Suggested fix: return an error instead of silently skipping
for _, proc := range item.Processes { if proc.PlatformBinary != nil && *proc.PlatformBinary { if proc.SigningID != "" { signingIDs = append(signingIDs, "platform:"+proc.SigningID) - } else { - log.Printf("Warning: skipping watch item %q: PlatformBinary is true but no SigningID is set", name) - return nil, nil + } else { + return nil, fmt.Errorf("PlatformBinary is true but no SigningID is set") } continue }If this change is applied, the
logimport can be removed if it becomes unused.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/faarules/faarules.go` around lines 149 - 156, The loop in faarules.go silently drops a watch item when a Process has PlatformBinary == true but empty SigningID (it currently calls log.Printf and returns nil, nil); change this to return a proper error instead of returning nil, nil so the caller sees the failure (e.g., construct and return an error mentioning the watch item name and the missing SigningID when encountering proc.PlatformBinary && proc.SigningID == ""); update the function's error path accordingly and remove the log import if it becomes unused.
🧹 Nitpick comments (1)
internal/faarules/faarules.go (1)
117-126: Map iteration yields non-deterministic rule ordering.Iterating over
policy.WatchItems(a map) produces rules in an unpredictable order. If consistent ordering is desired for reproducibility or testing, consider sorting the keys first.Optional fix: sort keys for deterministic output
+ import "sort" + func convertWatchItems(policy *FAAPolicy) ([]*apipb.FileAccessRule, error) { if len(policy.WatchItems) == 0 { return []*apipb.FileAccessRule{}, nil } + // Sort keys for deterministic output order + names := make([]string, 0, len(policy.WatchItems)) + for name := range policy.WatchItems { + names = append(names, name) + } + sort.Strings(names) + var rules []*apipb.FileAccessRule - for name, item := range policy.WatchItems { + for _, name := range names { + item := policy.WatchItems[name] rule, err := convertWatchItem(name, &item, policy)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/faarules/faarules.go` around lines 117 - 126, The loop over policy.WatchItems produces non-deterministic rules because map iteration is unordered; to fix, collect and sort the keys of policy.WatchItems before iterating, then call convertWatchItem(name, &item, policy) in that deterministic key order and append to the rules slice (variable rules) as before so tests and outputs are reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@internal/faarules/faarules.go`:
- Around line 224-237: getRuleType currently maps strings to
apipb.FileAccessRuleType and silently returns FILE_ACCESS_RULE_TYPE_UNSPECIFIED
for unknown values; change it to either emit a warning log or return an error so
typos are surfaced. Update getRuleType to return (apipb.FileAccessRuleType,
error) (or keep the same return but add a logger call) and update all callers of
getRuleType to handle the error/inspect the log; reference the getRuleType
function and any call sites that consume its result so unrecognized inputs
produce a descriptive error or warning mentioning the original string.
---
Duplicate comments:
In `@internal/faarules/faarules.go`:
- Around line 100-109: The code only inspects PayloadContent[0] and ignores FAA
rules in later payloads; change the logic to iterate over all entries in
config.PayloadContent, check each entry's FileAccessPolicy, call
convertWatchItems(policy) for each non-nil policy, and aggregate the results
(skip nil returns) into a single combined return value; ensure you continue on
nil policies and return the merged slice/result instead of only the first
payload's output (referencing config.PayloadContent, FileAccessPolicy, and
convertWatchItems).
- Around line 149-156: The loop in faarules.go silently drops a watch item when
a Process has PlatformBinary == true but empty SigningID (it currently calls
log.Printf and returns nil, nil); change this to return a proper error instead
of returning nil, nil so the caller sees the failure (e.g., construct and return
an error mentioning the watch item name and the missing SigningID when
encountering proc.PlatformBinary && proc.SigningID == ""); update the function's
error path accordingly and remove the log import if it becomes unused.
---
Nitpick comments:
In `@internal/faarules/faarules.go`:
- Around line 117-126: The loop over policy.WatchItems produces
non-deterministic rules because map iteration is unordered; to fix, collect and
sort the keys of policy.WatchItems before iterating, then call
convertWatchItem(name, &item, policy) in that deterministic key order and append
to the rules slice (variable rules) as before so tests and outputs are
reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: eec93281-949f-42f4-9a6d-840c40a3ea03
📒 Files selected for processing (1)
internal/faarules/faarules.go
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/main.go`:
- Around line 52-53: The flag validation is too lax: the boolean flags faaOnly
and staticRulesOnly must be validated against the selected input file type
before any import runs; add a guard in main (before entering the
mobileconfig/CSV import branches) that examines the input filename/format and
rejects incompatible combinations (e.g., --static-rules-only with a
.mobileconfig must error if the chosen import path would still process FAA
rules, and --faa-only with non-mobileconfig/CSV must error similarly). Locate
the argument parsing where faaOnly and staticRulesOnly are defined and enforce
the check right after parsing and before calling the mobileconfig import branch
(the code section that handles the ".mobileconfig" branch and the CSV/static
import branches), returning a non-zero exit and a clear error message when a
flag/file combination cannot honor the request.
- Around line 181-188: The loop that calls client.CreateFileAccessRule currently
uses context.Background(), which can hang; change it to create a per-RPC context
with a timeout (e.g., using context.WithTimeout) and pass that to
client.CreateFileAccessRule, ensuring you call the returned cancel() (defer or
immediate) to release resources; update the code around faaReq, faaRules and the
call site client.CreateFileAccessRule to use the new timed context so each FAA
write is bounded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a0c5ccf7-9b25-4530-ba22-b31155da186c
📒 Files selected for processing (3)
README.mdcmd/main.gointernal/faarules/faarules.go
✅ Files skipped from review due to trivial changes (1)
- README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- internal/faarules/faarules.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
internal/faarules/faarules.go (1)
157-165: Add comment clarifying PlatformBinary-only identification.The
continuestatement on line 164 is intentional: whenPlatformBinaryis true, the process is identified solely by its signing ID (prefixed with "platform:"), and other fields (CDHash,CertificateSha256,BinaryPath) are ignored. This is confirmed by the test expectation at line 92 offaarules_test.go.Add an inline comment before or after the
continuestatement to explain this behavior, since it silently ignores other identifier fields on the sameProcessEntry:// Platform binaries are identified only by signing ID continue🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/faarules/faarules.go` around lines 157 - 165, Add an inline comment explaining that when a ProcessEntry has PlatformBinary == true the process is identified solely by SigningID (prefixed with "platform:") and other identifier fields (CDHash, CertificateSha256, BinaryPath) are intentionally ignored; place this comment immediately before or after the existing continue in the loop that iterates item.Processes so it's clear why the code returns early in that branch (referencing PlatformBinary, SigningID, CDHash, CertificateSha256, BinaryPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@internal/faarules/faarules.go`:
- Around line 157-165: Add an inline comment explaining that when a ProcessEntry
has PlatformBinary == true the process is identified solely by SigningID
(prefixed with "platform:") and other identifier fields (CDHash,
CertificateSha256, BinaryPath) are intentionally ignored; place this comment
immediately before or after the existing continue in the loop that iterates
item.Processes so it's clear why the code returns early in that branch
(referencing PlatformBinary, SigningID, CDHash, CertificateSha256, BinaryPath).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a948079f-e182-4af2-b405-e69920b12f11
📒 Files selected for processing (1)
internal/faarules/faarules.go
tburgin
approved these changes
Mar 27, 2026
russellhancox
deleted the
rah/snt-200-add-faa-policy-import-to-santa-rule-importer
branch
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
2 participants
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 support for importing file access authorization rules, either from a separate policy plist or embedded in a Santa configuration mobileconfig. To continue allowing only StaticRules or only FAA rule imports, 2 new flags are added.