Skip to content

Commit

Permalink
Don't change the impact of ignored checks to 0
Browse files Browse the repository at this point in the history 
This will make it impossible to tell what the actual score was.
In this case, we will set the impact to 0 only when we merge it
into a policy
  • Loading branch information
@jaym
jaym committed Nov 15, 2024
1 parent 64a59b6 commit d746901
Show file tree
Hide file tree
Showing 2 changed files with 151 additions and 52 deletions.
89 changes: 39 additions & 50 deletions policy/resolved_policy_builder.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -555,7 +555,6 @@ func (b *resolvedPolicyBuilder) collectQueryTypes(policyMrn string, acc map[stri

func (b *resolvedPolicyBuilder) gatherGlobalInfoFromFramework(framework *Framework) {
actions := b.actionOverrides
impacts := b.impactOverrides

if framework == nil {
return
Expand All @@ -579,16 +578,6 @@ func (b *resolvedPolicyBuilder) gatherGlobalInfoFromFramework(framework *Framewo
if action != explorer.Action_UNSPECIFIED && action != explorer.Action_MODIFY {
actions[c.Mrn] = action
}

if action == explorer.Action_IGNORE {
// Since we traversed the children first, we overwrite the impact with ignore
// if the action is ignore. We do not merge in this case
impacts[c.Mrn] = &explorer.Impact{
Scoring: explorer.ScoringSystem_IGNORE_SCORE,
}
} else {
delete(impacts, c.Mrn)
}
}
}
}
Expand Down Expand Up @@ -646,22 +635,14 @@ func (b *resolvedPolicyBuilder) gatherGlobalInfoFromPolicy(policy *Policy) {

for _, c := range g.Checks {
impact := c.Impact
action := normalizeAction(g.Type, c.Action, impact)
if action == explorer.Action_IGNORE {
// Since we traversed the children first, we overwrite the impact with ignore
// if the action is ignore. We do not merge in this case
impact = &explorer.Impact{
Scoring: explorer.ScoringSystem_IGNORE_SCORE,
}
} else {
if qBundle, ok := b.bundleMap.Queries[c.Mrn]; ok {
// Check the impact defined on the query
impact = getWorstImpact(impact, qBundle.Impact)
}

impact = getWorstImpact(impact, impacts[c.Mrn])
if qBundle, ok := b.bundleMap.Queries[c.Mrn]; ok {
// Check the impact defined on the query
impact = getWorstImpact(impact, qBundle.Impact)
}

impact = getWorstImpact(impact, impacts[c.Mrn])

action := normalizeAction(g.Type, c.Action, impact)
if action != explorer.Action_UNSPECIFIED && action != explorer.Action_MODIFY {
actions[c.Mrn] = action
}
Expand Down Expand Up @@ -781,7 +762,7 @@ func (b *resolvedPolicyBuilder) addPolicy(policy *Policy) bool {
// because it will get included in a policy that wants it run.
// This will prevent the check from being connected to the policy that
// overrides its action
if isOverride(c.Action) {
if isOverride(c.Action, g.Type) {
b.propsCache.Add(c.Props...)
continue
}
Expand All @@ -793,7 +774,14 @@ func (b *resolvedPolicyBuilder) addPolicy(policy *Policy) bool {
}

if _, ok := b.addQuery(c); ok {
b.addEdge(c.Mrn, policy.Mrn, nil)
action := b.actionOverrides[c.Mrn]
var impact *explorer.Impact
if action == explorer.Action_IGNORE {
impact = &explorer.Impact{
Scoring: explorer.ScoringSystem_IGNORE_SCORE,
}
}
b.addEdge(c.Mrn, policy.Mrn, impact)
}
}

Expand All @@ -802,7 +790,7 @@ func (b *resolvedPolicyBuilder) addPolicy(policy *Policy) bool {
// because it will get included in a policy that wants it run.
// This will prevent the query from being connected to the policy that
// overrides its action
if isOverride(q.Action) {
if isOverride(q.Action, g.Type) {
b.propsCache.Add(q.Props...)
continue
}
Expand All @@ -823,7 +811,7 @@ func (b *resolvedPolicyBuilder) addPolicy(policy *Policy) bool {

hasMatchingRiskFactor := false
for _, r := range policy.RiskFactors {
if len(r.Checks) == 0 || isOverride(r.Action) {
if len(r.Checks) == 0 || isOverride(r.Action, GroupType_UNCATEGORIZED) {
continue
}

Expand Down Expand Up @@ -931,6 +919,8 @@ func (b *resolvedPolicyBuilder) addRiskFactor(riskFactor *RiskFactor) (bool, err
// TODO: we should just score the risk factor normally, I don't know why we ignore the score
b.addEdge(c.CodeId, riskFactor.Mrn, &explorer.Impact{Scoring: explorer.ScoringSystem_IGNORE_SCORE})

selectedCodeIds = append(selectedCodeIds, c.CodeId)

// TODO: we cannot use addQuery here because of the way cnspec tries to filter out
// sending scores for queries that are risk factors. This code, which is in collector.go
// needs to be refactored in such a way that it is natively integrated into the graph
Expand Down Expand Up @@ -979,8 +969,7 @@ func (b *resolvedPolicyBuilder) addFramework(framework *Framework) bool {
for _, fmap := range framework.FrameworkMaps {
for _, control := range fmap.Controls {
if b.addControl(control) {
impact := b.impactOverrides[control.Mrn]
b.addEdge(control.Mrn, fmap.FrameworkOwner.Mrn, impact)
b.addEdge(control.Mrn, fmap.FrameworkOwner.Mrn, b.actionToImpact(control.Mrn))
}
}
}
Expand Down Expand Up @@ -1016,11 +1005,7 @@ func (b *resolvedPolicyBuilder) addControl(control *ControlMap) bool {
}
qNode, ok := n.(*rpBuilderGenericQueryNode)
if ok {
impact := b.impactOverrides[q.Mrn]
if impact.GetScoring() != explorer.ScoringSystem_IGNORE_SCORE {
impact = nil
}
b.addEdge(qNode.selectedCodeId, control.Mrn, impact)
b.addEdge(qNode.selectedCodeId, control.Mrn, b.actionToImpact(q.Mrn))
hasChild = true
}
}
Expand All @@ -1034,36 +1019,27 @@ func (b *resolvedPolicyBuilder) addControl(control *ControlMap) bool {
}
qNode, ok := n.(*rpBuilderGenericQueryNode)
if ok {
impact := b.impactOverrides[q.Mrn]
if impact.GetScoring() != explorer.ScoringSystem_IGNORE_SCORE {
impact = nil
}
b.addEdge(qNode.selectedCodeId, control.Mrn, impact)
b.addEdge(qNode.selectedCodeId, control.Mrn, nil)
hasChild = true
}
}
}

for _, p := range control.Policies {
if _, ok := b.nodes[p.Mrn]; ok {
impact := b.impactOverrides[p.Mrn]
if impact.GetScoring() != explorer.ScoringSystem_IGNORE_SCORE {
impact = nil
}
// Add the edge from the control to the policy
b.addEdge(p.Mrn, control.Mrn, impact)
b.addEdge(p.Mrn, control.Mrn, b.actionToImpact(p.Mrn))
hasChild = true
}
}

for _, c := range control.Controls {
impact := b.impactOverrides[c.Mrn]
// We will just assume that the control is in the graph
// If its not, it will get filtered out later when we build
// the resolved policy
// Doing this so we don't need to topologically sort the dependency
// tree for the controls
b.addEdge(c.Mrn, control.Mrn, impact)
b.addEdge(c.Mrn, control.Mrn, b.actionToImpact(c.Mrn))
hasChild = true
}

Expand All @@ -1075,6 +1051,16 @@ func (b *resolvedPolicyBuilder) addControl(control *ControlMap) bool {
return true
}

func (b *resolvedPolicyBuilder) actionToImpact(mrn string) *explorer.Impact {
action := b.actionOverrides[mrn]
if action == explorer.Action_IGNORE {
return &explorer.Impact{
Scoring: explorer.ScoringSystem_IGNORE_SCORE,
}
}
return nil
}

func addReportingJob(qrId string, qrIdIsMrn bool, uuid string, typ ReportingJob_Type, rp *ResolvedPolicy) *ReportingJob {
if _, ok := rp.CollectorJob.ReportingJobs[uuid]; !ok {
rp.CollectorJob.ReportingJobs[uuid] = &ReportingJob{
Expand Down Expand Up @@ -1158,6 +1144,9 @@ func normalizeAction(groupType GroupType, action explorer.Action, impact *explor
}
}

func isOverride(action explorer.Action) bool {
return action != explorer.Action_UNSPECIFIED
func isOverride(action explorer.Action, groupType GroupType) bool {
return action != explorer.Action_UNSPECIFIED ||
groupType == GroupType_DISABLE ||
groupType == GroupType_OUT_OF_SCOPE ||
groupType == GroupType_IGNORED
}
114 changes: 112 additions & 2 deletions policy/resolver_v2_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1667,8 +1667,8 @@ policies:
rpTester.CodeIdReportingJobForMrn(queryMrn("query-1")).Notifies(queryMrn("query-1"))
rpTester.CodeIdReportingJobForMrn(queryMrn("query-2")).Notifies(queryMrn("query-2"))

rpTester.CodeIdReportingJobForMrn(queryMrn("sshd-service-running")).Notifies(queryMrn("sshd-service-running"))
rpTester.ReportingJobByMrn(queryMrn("sshd-service-running")).Notifies(riskFactorMrn("sshd-service")).WithImpact(&explorer.Impact{Scoring: explorer.ScoringSystem_IGNORE_SCORE})
// rpTester.CodeIdReportingJobForMrn(queryMrn("sshd-service-running")).Notifies(queryMrn("sshd-service-running"))
rpTester.CodeIdReportingJobForMrn(queryMrn("sshd-service-running")).Notifies(riskFactorMrn("sshd-service")).WithImpact(&explorer.Impact{Scoring: explorer.ScoringSystem_IGNORE_SCORE})
rpTester.ReportingJobByMrn(riskFactorMrn("sshd-service")).WithType(policy.ReportingJob_RISK_FACTOR).Notifies(policyMrn("risk-factors-security"))

rpTester.ReportingJobByMrn(queryMrn("query-1")).Notifies(policyMrn("testpolicy1"))
Expand Down Expand Up @@ -1898,3 +1898,113 @@ framework_maps:
rpTester.doTest(t, rp)
})
}

func TestResolveV2_PolicyExceptionIgnored(t *testing.T) {
ctx := contextResolverV2()
b := parseBundle(t, `
owner_mrn: //test.sth
policies:
- uid: policy1
groups:
- type: chapter
filters: "true"
checks:
- uid: check1
mql: asset.name == props.name
props:
- uid: name
mql: return "definitely not the asset name"
queries:
- uid: query1
mql: asset{*}
- owner_mrn: //test.sth
mrn: //test.sth
groups:
- policies:
- uid: policy1
- type: ignored
uid: "exceptions-1"
checks:
- uid: check1
`)

srv := initResolver(t, []*testAsset{
{asset: "asset1", policies: []string{policyMrn("policy1")}},
}, []*policy.Bundle{b})

t.Run("resolve with correct filters", func(t *testing.T) {
rp, err := srv.Resolve(ctx, &policy.ResolveReq{
PolicyMrn: "//test.sth",
AssetFilters: []*explorer.Mquery{{Mql: "true"}},
})
require.NoError(t, err)
require.NotNil(t, rp)

rpTester := newResolvedPolicyTester(b, srv.NewCompilerConfig())
rpTester.ExecutesQuery(queryMrn("query1"))
rpTester.
ExecutesQuery(queryMrn("check1")).
WithProps(map[string]string{"name": `return "definitely not the asset name"`})
rpTester.CodeIdReportingJobForMrn(queryMrn("check1")).Notifies(queryMrn("check1"))
rpTester.CodeIdReportingJobForMrn(queryMrn("query1")).Notifies(queryMrn("query1"))
rpTester.ReportingJobByMrn(queryMrn("check1")).Notifies(policyMrn("policy1")).WithImpact(&explorer.Impact{Scoring: explorer.ScoringSystem_IGNORE_SCORE})
rpTester.ReportingJobByMrn(queryMrn("query1")).Notifies(policyMrn("policy1"))
rpTester.ReportingJobByMrn(policyMrn("policy1")).Notifies("root")

rpTester.doTest(t, rp)
})
}

func TestResolveV2_PolicyExceptionDisabled(t *testing.T) {
ctx := contextResolverV2()
b := parseBundle(t, `
owner_mrn: //test.sth
policies:
- uid: policy1
groups:
- type: chapter
filters: "true"
checks:
- uid: check1
mql: asset.name == props.name
props:
- uid: name
mql: return "definitely not the asset name"
queries:
- uid: query1
mql: asset{*}
- owner_mrn: //test.sth
mrn: //test.sth
groups:
- policies:
- uid: policy1
- type: disable
uid: "exceptions-1"
checks:
- uid: query1
`)

srv := initResolver(t, []*testAsset{
{asset: "asset1", policies: []string{policyMrn("policy1")}},
}, []*policy.Bundle{b})

t.Run("resolve with correct filters", func(t *testing.T) {
rp, err := srv.Resolve(ctx, &policy.ResolveReq{
PolicyMrn: "//test.sth",
AssetFilters: []*explorer.Mquery{{Mql: "true"}},
})
require.NoError(t, err)
require.NotNil(t, rp)

rpTester := newResolvedPolicyTester(b, srv.NewCompilerConfig())
rpTester.DoesNotExecutesQuery(queryMrn("query1"))
rpTester.
ExecutesQuery(queryMrn("check1")).
WithProps(map[string]string{"name": `return "definitely not the asset name"`})
rpTester.CodeIdReportingJobForMrn(queryMrn("check1")).Notifies(queryMrn("check1"))
rpTester.ReportingJobByMrn(queryMrn("check1")).Notifies(policyMrn("policy1"))
rpTester.ReportingJobByMrn(policyMrn("policy1")).Notifies("root")

rpTester.doTest(t, rp)
})
}

0 comments on commit d746901

Please sign in to comment.