✨ policy exceptions v2 #943

imilchev · 2023-11-09T16:26:14Z

We want to support a new way of defining exceptions for checks in policies. The new way is almost identical to compliance framework exceptions.

Example exception old:

policies:
  - uid: sshd-server-policy
    name: SSH Server Policy
    version: 1.0.0
    groups:
      - filters: asset.family.contains("darwin")
        checks:
          - uid: sshd-score-01
          - uid: ignored-query
          - uid: deactivate-query
  - uid : asset-policy
    groups:
      - checks:
          - uid: ignored-query
            action: 4
          - 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"

New example:

policies:
  - uid: sshd-server-policy
    name: SSH Server Policy
    version: 1.0.0
    groups:
      - filters: asset.family.contains("darwin")
        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"

imilchev · 2023-11-14T17:23:46Z

policy/resolver.go

 	}

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

-	ownerJob.ChildJobs[queryJob.Uuid] = impact


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

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.

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

czunker

Thanks for all the tests @imilchev

Code is looking good to me. There is only one thing I currently do not understand. I'll try to test this manually.

policy/scan/local_scanner_test.go

policy/resolver.go

policy/scan/local_scanner_test.go

czunker · 2023-11-16T05:50:23Z

I caused a panic during testing:

cnspec scan local -f policy/scan/testdata/exceptions.mql.yaml 
→ loaded configuration from /etc/opt/mondoo/mondoo.yml using source default
→ using service account credentials
! Scanning with local bundles will switch into --incognito mode by default. Your results will not be sent upstream.
→ discover related assets for 1 asset(s)
panic: invalid score report

goroutine 94 [running]:
go.mondoo.com/cnspec/v9/policy/executor/internal.(*ReportingJobNodeData).consume(0xc0001a5540, {0xc000014c00, 0xc}, 0xc001467d40)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/internal/nodes.go:442 +0x19a
go.mondoo.com/cnspec/v9/policy/executor/internal.(*GraphExecutor).Execute(0xc001493680)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/internal/graph.go:137 +0x832
go.mondoo.com/cnspec/v9/policy/executor.ExecuteResolvedPolicy({0x17e3e80, 0xc000fde1c0}, {0x17e6de0?, 0xc000eb11a0}, {0xc0004c9700, 0x3a}, 0xc000bcfc40?, {0xc001004408, 0x2, 0x8}, ...)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/graph.go:47 +0x479
go.mondoo.com/cnspec/v9/policy/scan.(*localAssetScanner).runPolicy(0xc001387d48)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:996 +0x509
go.mondoo.com/cnspec/v9/policy/scan.(*localAssetScanner).run(0xc001387d48)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:777 +0x33
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).runMotorizedAsset.func1(0x17e3e80?, 0xc000a3a1c0?)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:628 +0x217
go.mondoo.com/cnspec/v9/internal/datalakes/inmemory.WithDb({0x17e3e80?, 0xc000a3a1c0?}, 0x7fdfc97d7a68?, 0xc000bcfdf8)
	/home/christian/workspace/mondoo/github.com/cnspec/internal/datalakes/inmemory/inmemory.go:52 +0x42
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).runMotorizedAsset(0xc00100e0f0?, 0x15d0478?)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:604 +0x75
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).RunAssetJob(0xc0004c6850, 0xc000fe4c80)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:568 +0x3fb
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).distributeJob.func2()
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:492 +0x105
created by go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).distributeJob in goroutine 1
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:472 +0x1749

I only removed the action field from the ignored-query in above policy.

The panic does not happen with cnspec v9.7.0. So, my guess is this PR introduced/triggered it.

The panic also happens when I only change the action field from 4 to 0.

czunker · 2023-11-16T06:27:14Z

Thanks for all the tests @imilchev

Code is looking good to me. There is only one thing I currently do not understand. I'll try to test this manually.

I tried policy groups with "IGNORED" and "DISABLED", that changes the score as expected. But, from the code I don't understand why this happens.

Signed-off-by: Ivan Milchev <[email protected]>

czunker · 2023-11-17T12:15:20Z

I caused a panic during testing:

cnspec scan local -f policy/scan/testdata/exceptions.mql.yaml 
→ loaded configuration from /etc/opt/mondoo/mondoo.yml using source default
→ using service account credentials
! Scanning with local bundles will switch into --incognito mode by default. Your results will not be sent upstream.
→ discover related assets for 1 asset(s)
panic: invalid score report

goroutine 94 [running]:
go.mondoo.com/cnspec/v9/policy/executor/internal.(*ReportingJobNodeData).consume(0xc0001a5540, {0xc000014c00, 0xc}, 0xc001467d40)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/internal/nodes.go:442 +0x19a
go.mondoo.com/cnspec/v9/policy/executor/internal.(*GraphExecutor).Execute(0xc001493680)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/internal/graph.go:137 +0x832
go.mondoo.com/cnspec/v9/policy/executor.ExecuteResolvedPolicy({0x17e3e80, 0xc000fde1c0}, {0x17e6de0?, 0xc000eb11a0}, {0xc0004c9700, 0x3a}, 0xc000bcfc40?, {0xc001004408, 0x2, 0x8}, ...)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/executor/graph.go:47 +0x479
go.mondoo.com/cnspec/v9/policy/scan.(*localAssetScanner).runPolicy(0xc001387d48)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:996 +0x509
go.mondoo.com/cnspec/v9/policy/scan.(*localAssetScanner).run(0xc001387d48)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:777 +0x33
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).runMotorizedAsset.func1(0x17e3e80?, 0xc000a3a1c0?)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:628 +0x217
go.mondoo.com/cnspec/v9/internal/datalakes/inmemory.WithDb({0x17e3e80?, 0xc000a3a1c0?}, 0x7fdfc97d7a68?, 0xc000bcfdf8)
	/home/christian/workspace/mondoo/github.com/cnspec/internal/datalakes/inmemory/inmemory.go:52 +0x42
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).runMotorizedAsset(0xc00100e0f0?, 0x15d0478?)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:604 +0x75
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).RunAssetJob(0xc0004c6850, 0xc000fe4c80)
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:568 +0x3fb
go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).distributeJob.func2()
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:492 +0x105
created by go.mondoo.com/cnspec/v9/policy/scan.(*LocalScanner).distributeJob in goroutine 1
	/home/christian/workspace/mondoo/github.com/cnspec/policy/scan/local_scanner.go:472 +0x1749

I only removed the action field from the ignored-query in above policy.

The panic does not happen with cnspec v9.7.0. So, my guess is this PR introduced/triggered it.

The panic also happens when I only change the action field from 4 to 0.

Tried to reproduce this today, but it is gone.

czunker

Thanks @imilchev

imilchev force-pushed the ivan/policy-exceptions branch 4 times, most recently from d5523ad to 34b5086 Compare November 14, 2023 17:21

imilchev commented Nov 14, 2023

View reviewed changes

imilchev force-pushed the ivan/policy-exceptions branch from 34b5086 to 7d45747 Compare November 14, 2023 17:48

This was referenced Nov 15, 2023

Remove impact action field #949

Open

Investigate the usage of query action field #950

Open

imilchev marked this pull request as ready for review November 15, 2023 11:35

czunker reviewed Nov 16, 2023

View reviewed changes

policy/scan/local_scanner_test.go Outdated Show resolved Hide resolved

policy/resolver.go Show resolved Hide resolved

policy/scan/local_scanner_test.go Show resolved Hide resolved

czunker mentioned this pull request Nov 17, 2023

🧹 Prevent nil panic when recording is not set #953

Merged

imilchev added 2 commits November 17, 2023 12:14

policy exceptions v2

65aca37

Signed-off-by: Ivan Milchev <[email protected]>

add tests

8af8a25

Signed-off-by: Ivan Milchev <[email protected]>

imilchev force-pushed the ivan/policy-exceptions branch from 0f77c97 to 8af8a25 Compare November 17, 2023 11:16

czunker approved these changes Nov 17, 2023

View reviewed changes

imilchev merged commit 9f4a0a7 into main Nov 17, 2023
9 checks passed

imilchev deleted the ivan/policy-exceptions branch November 17, 2023 12:22

github-actions bot locked and limited conversation to collaborators Nov 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ policy exceptions v2 #943

✨ policy exceptions v2 #943

imilchev commented Nov 9, 2023 •

edited

Loading

imilchev Nov 14, 2023

arlimus Nov 15, 2023

imilchev Nov 15, 2023

czunker left a comment

czunker commented Nov 16, 2023

czunker commented Nov 16, 2023

czunker commented Nov 17, 2023

czunker left a comment

✨ policy exceptions v2 #943

✨ policy exceptions v2 #943

Conversation

imilchev commented Nov 9, 2023 • edited Loading

imilchev Nov 14, 2023

Choose a reason for hiding this comment

arlimus Nov 15, 2023

Choose a reason for hiding this comment

imilchev Nov 15, 2023

Choose a reason for hiding this comment

czunker left a comment

Choose a reason for hiding this comment

czunker commented Nov 16, 2023

czunker commented Nov 16, 2023

czunker commented Nov 17, 2023

czunker left a comment

Choose a reason for hiding this comment

imilchev commented Nov 9, 2023 •

edited

Loading