Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

✨ policy exceptions v2 #943

Merged
merged 2 commits into from
Nov 17, 2023
Merged

✨ policy exceptions v2 #943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
65aca37
policy exceptions v2
imilchev Nov 9, 2023
8af8a25
add tests
imilchev Nov 15, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2,275 changes: 1,152 additions & 1,123 deletions policy/cnspec_policy.pb.go
View file
Open in desktop

Large diffs are not rendered by default.

5 changes: 5 additions & 0 deletions policy/cnspec_policy.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,11 @@ message PolicyGroup {
string title = 24;
PolicyGroupDocs docs = 25;
repeated cnquery.explorer.Author authors = 26;
repeated cnquery.explorer.Author reviewers = 27;
// only applies to GroupType IGNORED/DISABLED
// Only if status is REJECTED, we ignore the group
// Which means that unapproved groups get handled as if they were approved
ReviewStatus review_status = 28;

int64 created = 32;
int64 modified = 33;
Expand Down
15 changes: 7 additions & 8 deletions policy/resolver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -725,13 +725,13 @@ func (s *LocalServices) policyToJobs(ctx context.Context, policyMrn string, owne
}
for i := range group.Checks {
check := group.Checks[i]
if check.Action == explorer.Action_DEACTIVATE {
if check.Action == explorer.Action_DEACTIVATE || group.Type == GroupType_DISABLE {
cache.removedQueries[check.Mrn] = struct{}{}
}
}
for i := range group.Queries {
query := group.Queries[i]
if query.Action == explorer.Action_DEACTIVATE {
if query.Action == explorer.Action_DEACTIVATE || group.Type == GroupType_DISABLE {
cache.removedQueries[query.Mrn] = struct{}{}
}
}
Expand Down Expand Up @@ -891,7 +891,7 @@ func (s *LocalServices) policyGroupToJobs(ctx context.Context, group *PolicyGrou
continue
}

if check.Action == explorer.Action_IGNORE {
if check.Action == explorer.Action_IGNORE || group.Type == GroupType_IGNORED {
stillValid := CheckValidUntil(group.EndDate, check.Mrn)
if !stillValid {
// the exception is no longer valid => score the check
Expand All @@ -904,20 +904,20 @@ func (s *LocalServices) policyGroupToJobs(ctx context.Context, group *PolicyGrou
if impact == nil {
impact = &explorer.Impact{}
}
if check.Action == explorer.Action_IGNORE {
if check.Action == explorer.Action_IGNORE || group.Type == GroupType_IGNORED {
impact.Scoring = explorer.ScoringSystem_IGNORE_SCORE
impact.Action = explorer.Action_IGNORE
}

cache.global.propsCache.Add(check.Props...)

if check.Action == explorer.Action_UNSPECIFIED || check.Action == explorer.Action_ACTIVATE {
if (check.Action == explorer.Action_UNSPECIFIED || check.Action == explorer.Action_ACTIVATE) && group.Type != GroupType_IGNORED {
czunker marked this conversation as resolved.
Show resolved Hide resolved
cache.addCheckJob(ctx, check, impact, ownerJob)
continue
}

// TODO: can we simplify this to simply IGNORE?
if check.Action == explorer.Action_MODIFY || check.Action == explorer.Action_IGNORE {
if check.Action == explorer.Action_MODIFY || check.Action == explorer.Action_IGNORE || group.Type == GroupType_IGNORED {
cache.modifyCheckJob(check, impact)
}
}
Expand Down Expand Up @@ -977,13 +977,12 @@ func (cache *policyResolverCache) addCheckJob(ctx context.Context, check *explor
cache.global.reportingJobsByUUID[uuid] = queryJob
cache.global.reportingJobsByMsum[check.Checksum] = append(cache.global.reportingJobsByMsum[check.Checksum], queryJob)
cache.childJobsByMrn[check.Mrn] = append(cache.childJobsByMrn[check.Mrn], queryJob)
ownerJob.ChildJobs[queryJob.Uuid] = impact
}

// local aspects for the resolved policy
queryJob.Notify = append(queryJob.Notify, ownerJob.Uuid)

ownerJob.ChildJobs[queryJob.Uuid] = impact
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why we need to change the impact for a job that already exists... I think this shouldn't be the case. If we really want to change it, then we should be calling modifyCheckJob instead of addCheckJob but I am not sure

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch! There is a problem that we never fully explored that you are really exposing here:
What if a check is deactivate, and later re-activate?
ie: it is first added (action=unspecified/activate)
then it is deactivated (action=deactivate)
and then another top-level policy comes and wants to actually set it on AND/OR make a modification. We have assumed - so far - that the action for activate and the action for modify are almost interchangeable. However, now that the impact is getting modified, it can lead to some nasty side-effects. This happens because if we ignore a check, the impact is set to impact.Scoring = explorer.ScoringSystem_IGNORE_SCORE. So any call later on that modifies this overwrites the impact's scoring system.

Let's solve this step by step.
And the first step is where I think you are spot on: let's remove this line and keep it only for explicit modifications.

Next, I think we should (1) remove the really outdated Action field from impact (it should only be used for v7 compatibility, otherwise all actions are bound to the mquery) and (2) review the action field, because it is overloaded right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added #949 and #950 for the follow ups. Also added automated tests for the cases used to validate whether exceptions work as expected


if len(check.Variants) != 0 {
err := cache.addCheckJobVariants(ctx, check, queryJob)
if err != nil {
Expand Down
226 changes: 226 additions & 0 deletions policy/scan/local_scanner_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,14 @@ import (

"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/stretchr/testify/suite"
"go.mondoo.com/cnquery/v9/explorer"
"go.mondoo.com/cnquery/v9/llx"
"go.mondoo.com/cnquery/v9/providers"
"go.mondoo.com/cnquery/v9/providers-sdk/v1/inventory"
"go.mondoo.com/cnquery/v9/providers-sdk/v1/testutils"
"go.mondoo.com/cnquery/v9/providers-sdk/v1/upstream"
"go.mondoo.com/cnspec/v9/policy"
)

func TestFilterPreprocess(t *testing.T) {
Expand Down Expand Up @@ -109,3 +114,224 @@ func TestDefaultConfig(t *testing.T) {
require.Equal(t, providers.NullRecording{}, scanner.recording)
})
}

type LocalScannerSuite struct {
suite.Suite
ctx context.Context
schema llx.Schema
job *Job
}

func (s *LocalScannerSuite) SetupSuite() {
s.ctx = context.Background()
runtime := testutils.Local()
s.schema = runtime.Schema()
}

func (s *LocalScannerSuite) BeforeTest(suiteName, testName string) {
s.job = &Job{
Inventory: &inventory.Inventory{
Spec: &inventory.InventorySpec{
Assets: []*inventory.Asset{
{
Connections: []*inventory.Config{
{
Type: "k8s",
Options: map[string]string{
"path": "./testdata/2pods.yaml",
},
},
},
},
},
},
},
}
}

func (s *LocalScannerSuite) TestRunIncognito_ExceptionGroups() {
bundle, err := policy.BundleFromPaths("./testdata/exception-groups.mql.yaml")
s.Require().NoError(err)

_, err = bundle.CompileExt(context.Background(), policy.BundleCompileConf{
Schema: s.schema,
RemoveFailing: true,
})
s.Require().NoError(err)

s.job.Bundle = bundle
s.job.PolicyFilters = []string{"asset-policy"}
bundleMap := bundle.ToMap()

ctx := context.Background()
scanner := NewLocalScanner()
res, err := scanner.RunIncognito(ctx, s.job)
s.Require().NoError(err)
s.Require().NotNil(res)

full := res.GetFull()
s.Require().NotNil(full)

s.Equal(1, len(full.Reports))

for k, r := range full.Reports {
// Verify the score is 100
s.Equal(uint32(100), r.GetScore().Value)

p := full.ResolvedPolicies[k]

// Get the code id for all the executed queries
executedQueries := []string{}
for qCodeId := range p.ExecutionJob.Queries {
executedQueries = append(executedQueries, qCodeId)
}

expectedQueries := []string{
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId,
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/sshd-score-01"].CodeId,
}
s.ElementsMatch(expectedQueries, executedQueries)

queryIdToReportingJob := map[string]*policy.ReportingJob{}
for _, rj := range p.CollectorJob.ReportingJobs {
_, ok := queryIdToReportingJob[rj.QrId]
s.Require().False(ok)
queryIdToReportingJob[rj.QrId] = rj
}

// Make sure the ignored query is ignored
queryRj := queryIdToReportingJob[bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId]
s.Require().NotNil(queryRj)

parent := queryRj.Notify[0]
parentJob := p.CollectorJob.ReportingJobs[parent]
s.Require().NotNil(parentJob)
s.Equal(explorer.ScoringSystem_IGNORE_SCORE, parentJob.ChildJobs[queryRj.Uuid].Scoring)
}
}

func (s *LocalScannerSuite) TestRunIncognito_QueryExceptions() {
bundle, err := policy.BundleFromPaths("./testdata/exceptions.mql.yaml")
s.Require().NoError(err)

_, err = bundle.CompileExt(context.Background(), policy.BundleCompileConf{
Schema: s.schema,
RemoveFailing: true,
})
s.Require().NoError(err)

s.job.Bundle = bundle
s.job.PolicyFilters = []string{"asset-policy"}
bundleMap := bundle.ToMap()

ctx := context.Background()
scanner := NewLocalScanner()
res, err := scanner.RunIncognito(ctx, s.job)
s.Require().NoError(err)
s.Require().NotNil(res)

full := res.GetFull()
s.Require().NotNil(full)

s.Equal(1, len(full.Reports))

for k, r := range full.Reports {
// Verify the score is 100
s.Equal(uint32(100), r.GetScore().Value)

p := full.ResolvedPolicies[k]

// Get the code id for all the executed queries
executedQueries := []string{}
for qCodeId := range p.ExecutionJob.Queries {
executedQueries = append(executedQueries, qCodeId)
}

expectedQueries := []string{
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId,
czunker marked this conversation as resolved.
Show resolved Hide resolved
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/sshd-score-01"].CodeId,
}
s.ElementsMatch(expectedQueries, executedQueries)

queryIdToReportingJob := map[string]*policy.ReportingJob{}
for _, rj := range p.CollectorJob.ReportingJobs {
_, ok := queryIdToReportingJob[rj.QrId]
s.Require().False(ok)
queryIdToReportingJob[rj.QrId] = rj
}

// Make sure the ignored query is ignored
queryRj := queryIdToReportingJob[bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId]
s.Require().NotNil(queryRj)

parent := queryRj.Notify[0]
parentJob := p.CollectorJob.ReportingJobs[parent]
s.Require().NotNil(parentJob)
s.Equal(explorer.ScoringSystem_IGNORE_SCORE, parentJob.ChildJobs[queryRj.Uuid].Scoring)
}
}

func (s *LocalScannerSuite) TestRunIncognito_QueryExceptions_MultipleGroups() {
bundle, err := policy.BundleFromPaths("./testdata/exceptions-multiple-groups.mql.yaml")
s.Require().NoError(err)

_, err = bundle.CompileExt(context.Background(), policy.BundleCompileConf{
Schema: s.schema,
RemoveFailing: true,
})
s.Require().NoError(err)

s.job.Bundle = bundle
s.job.PolicyFilters = []string{"asset-policy"}
bundleMap := bundle.ToMap()

ctx := context.Background()
scanner := NewLocalScanner()
res, err := scanner.RunIncognito(ctx, s.job)
s.Require().NoError(err)
s.Require().NotNil(res)

full := res.GetFull()
s.Require().NotNil(full)

s.Equal(1, len(full.Reports))

for k, r := range full.Reports {
// Verify the score is 100
s.Equal(uint32(100), r.GetScore().Value)

p := full.ResolvedPolicies[k]

// Get the code id for all the executed queries
executedQueries := []string{}
for qCodeId := range p.ExecutionJob.Queries {
executedQueries = append(executedQueries, qCodeId)
}

expectedQueries := []string{
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId,
bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/sshd-score-01"].CodeId,
}
s.ElementsMatch(expectedQueries, executedQueries)

queryIdToReportingJob := map[string]*policy.ReportingJob{}
for _, rj := range p.CollectorJob.ReportingJobs {
_, ok := queryIdToReportingJob[rj.QrId]
s.Require().False(ok)
queryIdToReportingJob[rj.QrId] = rj
}

// Make sure the ignored query is ignored
queryRj := queryIdToReportingJob[bundleMap.Queries["//local.cnspec.io/run/local-execution/queries/ignored-query"].CodeId]
s.Require().NotNil(queryRj)

parent := queryRj.Notify[0]
parentJob := p.CollectorJob.ReportingJobs[parent]
s.Require().NotNil(parentJob)
s.Equal(explorer.ScoringSystem_IGNORE_SCORE, parentJob.ChildJobs[queryRj.Uuid].Scoring)
}
}

func TestLocalScannerSuite(t *testing.T) {
suite.Run(t, new(LocalScannerSuite))
}
2 changes: 1 addition & 1 deletion policy/scan/scan.pb.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

36 changes: 36 additions & 0 deletions policy/scan/testdata/exception-groups.mql.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Copyright (c) Mondoo, Inc.
# SPDX-License-Identifier: BUSL-1.1

# Read more about the policy structure at https://mondoo.com/docs
policies:
- uid: sshd-server-policy
name: SSH Server Policy
version: 1.0.0
groups:
- filters: true == true
checks:
- uid: sshd-score-01
- uid: ignored-query
- uid: deactivate-query
- uid : asset-policy
groups:
- checks:
- uid: ignored-query
type: 4
policies:
- uid: sshd-server-policy
- checks:
- uid: deactivate-query
type: 5
policies:
- uid: sshd-server-policy
queries:
- uid: sshd-score-01
title: Ensure SSH MaxAuthTries is set to 4 or less
mql: true == true
- uid: ignored-query
title: Ignored query
mql: asset.name == "test"
- uid: deactivate-query
title: Deactivate query
mql: asset.name == "test2"
36 changes: 36 additions & 0 deletions policy/scan/testdata/exceptions-multiple-groups.mql.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,36 @@
# Copyright (c) Mondoo, Inc.
# SPDX-License-Identifier: BUSL-1.1

# Read more about the policy structure at https://mondoo.com/docs
policies:
- uid: sshd-server-policy
name: SSH Server Policy
version: 1.0.0
groups:
- filters: true == true
checks:
- uid: sshd-score-01
- uid: ignored-query
- uid: deactivate-query
- uid : asset-policy
groups:
- checks:
- uid: ignored-query
action: 4
policies:
- uid: sshd-server-policy
- checks:
- uid: deactivate-query
action: 2
policies:
- uid: sshd-server-policy
queries:
- uid: sshd-score-01
title: Ensure SSH MaxAuthTries is set to 4 or less
mql: true == true
- uid: ignored-query
title: Ignored query
mql: asset.name == "test"
- uid: deactivate-query
title: Deactivate query
mql: asset.name == "test2"
Loading