feat(analyzer): policy-configurable security scoring (closes #49)#53
Open
initializ-mk wants to merge 1 commit into
Open
feat(analyzer): policy-configurable security scoring (closes #49)#53initializ-mk wants to merge 1 commit into
initializ-mk wants to merge 1 commit into
Conversation
Thread SecurityPolicy through AnalyzeSkillDescriptor / AnalyzeSkillEntry so operators can adjust scoring for custom skills without editing any file under forge-skills/analyzer/. - types.go: extend SecurityPolicy with AcknowledgedBins and AcknowledgedEnv; document TrustedDomains plus the two new fields as scoring overrides distinct from policy-check fields. - scoring.go: wire policy.TrustedDomains into scoreEgress (was always passed nil — the existing wiring gap). Add policy-aware variants for scoreBinaries and scoreEnv so AcknowledgedBins / AcknowledgedEnv lower the points and mark the RiskFactor description with "(acknowledged by policy)" or "(via policy)" so the override is visible in audit output. Builtin classifications still win to preserve audit consistency across projects. - policy_loader.go: LoadPolicyFromFile parses a YAML SecurityPolicy. - cmd/skills.go: --policy <path> flag on `forge skills audit` replaces DefaultPolicy() with the YAML-loaded one for both scoring and policy checks. - Tests: full coverage of overrides (TrustedDomains, AcknowledgedBins, AcknowledgedEnv), confirmation that builtin trust isn't re-attributed to policy, that AcknowledgedBins doesn't promote standard binaries, and a YAML loader round-trip including the end-to-end scoring effect.
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
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.
Summary
Fixes #49 — a custom skill's egress domain, binary, or env var can now be scored as non-risky without editing any file under
forge-skills/analyzer/.This closes the three wiring gaps called out in the issue:
SecurityPolicy.TrustedDomainsexisted but was never threaded intoscoreEgress(always passednil). It now is.SecurityPolicy.MaxRiskScorewas only consulted in policy violation checks. With the policy now flowing through scoring, that check uses the same effective policy.forge skills audithardcodedanalyzer.DefaultPolicy()with no flag to override. A--policy <path>flag now loads a custom YAML.Two new
SecurityPolicyfields support binary and env-var overrides:trusted_domains: [...]trusted domain (via policy): Xacknowledged_bins: [...]high-risk binary (acknowledged by policy): Xacknowledged_env: [...]sensitive variable (acknowledged by policy): XBuiltin classifications still win — listing an already-builtin-trusted domain like
api.github.comundertrusted_domainsis a no-op rather than re-attributing the trust to policy. This keeps audit reports consistent across projects that share the same builtin allowlist.Auditability
Every policy-driven score reduction is recorded in the corresponding
RiskFactor.Description. JSON and text audit output both surface"(via policy)"/"(acknowledged by policy)"next to the affected item, so operators can see which items were downgraded by policy without diffing scores.Usage
# project.policy.yaml trusted_domains: - internal.example.com acknowledged_bins: - python acknowledged_env: - DB_PASSWORD max_risk_score: 90 script_policy: allowA skill that previously scored 35 (script +20, python +15) might now score 23 (script +20, python +3 acknowledged) — and the audit report makes the override traceable.
API change
AnalyzeSkillDescriptorandAnalyzeSkillEntrynow take aSecurityPolicyparameter. Both functions and their internal score helpers were internal toforge-skills/analyzer; the only external call sites areGenerateReport/GenerateReportFromEntries(which already accept a policy and now thread it through). PassingSecurityPolicy{}preserves the historical default scoring exactly.Tests
New tests (analyzer package):
TestAnalyzeSkillDescriptor_PolicyTrustedDomain— domain in policy scored as trusted withvia policyannotation.TestAnalyzeSkillDescriptor_PolicyAcknowledgedBin— high-risk binary in policy drops to +3 with annotation.TestAnalyzeSkillDescriptor_PolicyAcknowledgedEnv— sensitive env var in policy drops to +5 with annotation.TestAnalyzeSkillDescriptor_PolicyPreservesDefault— builtin trust is not re-attributed to policy.TestAnalyzeSkillDescriptor_PolicyDoesNotPromoteStandardBin—AcknowledgedBinscannot escalate a non-high-risk binary.TestLoadPolicyFromFile— YAML round-trip for all new and existing fields.TestLoadPolicyFromFile_PolicyAffectsScoring— end-to-end: YAML →LoadPolicyFromFile→scoreEgress/scoreBinarieshonor it.TestLoadPolicyFromFile_MissingFile,…_InvalidYAML— error paths.Existing analyzer tests updated to pass
SecurityPolicy{}so all prior expected scores are preserved unchanged.Acceptance criteria from #49
forge-skills/analyzer/.forge skills auditaccepts a configurable policy that affects both scoring and policy checks, not only the latter.Out of scope (deferred follow-ups)
Issue #49 listed two further candidate approaches that are intentionally not in this PR:
forge.securityblock inSKILL.mdfrontmatter — per-skill self-attestation (issue option 3).forge.yamlproject-level policy block — implicit policy without--policyflag (issue option 4).Both build on this PR's policy-threading without further analyzer changes; they'd be net-additive parsers feeding the same
SecurityPolicystruct. Worth a separate enhancement if there's demand.Test plan
forge skills auditon the embedded registry without--policyand confirm the report matches the pre-PR output.policy.yamlwithtrusted_domainsfor a project-specific domain and confirmforge skills audit --policy policy.yamllowers the corresponding skill's score and annotates the factor.acknowledged_bins: [python]lowers a skill that requirespythonand surfaces"(acknowledged by policy)"in the text and JSON output.