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.

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

Draft: Fine-grained excludes #584

Open
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

moredhel
Copy link
Contributor

@moredhel moredhel commented Jun 25, 2021

Introduction

This PR is aiming to include an exclusions keyword which would allow more fine-grained control over which policies to deny. The current exception blocks will apply to an entire file, rather than a single entity within that file.

This PR aims to expand the functionality of conftest to allow for excluding a single deny instance of a policy within a file. This should bring assumed the functionality of conftest (exceptions) and the actual functionality in-line (via excludes).

Outstanding


  • Write Tests
    • bats tests
    • go tests (standard_test.go)
  • Documentation
    • How to use
    • Examples for the website
    • Clarity on exceptions & their scope

This is an initial PoC/thought process on how to do fine-grained Exclusions of policies.

There are currently some limitations that come with it:

  • such as having to recreate the msg from both the deny/warn rule & the exclusion rule.
  • The exclusion_<name> section must match with the corresponding deny_<name> that you would like to have match the exceptions against. (More a feature than a limitation)
  • The response object from both the rule & the exclusion is more complicated (but optional, both methods still work)

Examples

I've included a sample in examples/exceptions2 which details some policies, a working exception & a working exclusion.
see for examples:

  • examples/excludes
  • examples/excludes2

You can run it similarly to in the tests:

$ ./conftest test -p examples/excludes/policy/ examples/excludes
FAIL - examples/exceptions2/main.tf - main - Resource Name 'invalid-name' contains dashes
EXCP - examples/exceptions2/main.tf - main - data.main.exception[_][_] == "resource_type"
EXCL - examples/exceptions2/main.tf - main - data.main.exclude_name[_][_] = "Resource Name 'exception-name' contains dashes"

3 tests, 0 passed, 0 warnings, 1 failure, 1 exception, 1 exclusion

The aim of this PR is to open discussion on a potential avenue towards having more fine-grained exclusions within conftest.

Have a play with it & let me know what your thoughts are.

Decisions / points of discussion

  • modelling the exclusions similarly to denies & warns by naming them.
  • Return value of an exclusion must match the error message from the matching deny/warn.
  • Potential performance hit due to evaluating the excludes for each failure (1 run per failure)

Signed-off-by: Hamish Hutchings <[email protected]>
@moredhel moredhel changed the title Fine-grained excludes Draft: Fine-grained excludes Jun 25, 2021
@moredhel moredhel marked this pull request as ready for review June 29, 2021 07:38
Copy link

@mykter mykter left a comment

Choose a reason for hiding this comment

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

Is there any mileage in having exclusions actually modify the rule itself?

say you write something like:

exclude_name[vars] {
  # we can't fix wid-get, its name is everywhere, just ignore it.
  vars.name == "wid-get"
}

and then deny_name is implicitly modified to look like this?

deny_name[msg] {
# unchanged:
	input.resource[_][name]
	contains(name, "-")
	msg := func_name_msg(name)
# magic happens here:
        not exclude_name({"name": name})
}

my thinking is you get access to the internal state of the rule, which means you can add arbitrary conditions

It's fragile, in that any changes to how the original rule works (including renaming variables) could break your exclusion, but then matching on messages is also fragile.

examples/exceptions2/policy/deny.rego Outdated Show resolved Hide resolved
examples/exceptions2/policy/deny.rego Outdated Show resolved Hide resolved
examples/exceptions2/policy/deny.rego Outdated Show resolved Hide resolved
- Add New Example based on ticket (excludes2)
- Refactor to allow for structured content being returned

Signed-off-by: Hamish Hutchings <[email protected]>
- Refactor the terraform example

Signed-off-by: Hamish Hutchings <[email protected]>
@rdsubhas
Copy link

rdsubhas commented Dec 28, 2021

Contextual exclusions will greatly improve quality of policies. Thank you for kickstarting this and for the PoC PR! I would like to offer one additional viewpoint, if I may...

A lot of the cumbersomeness with contextual exclusions – is passing the context. The actual deny or warn rule has the full localized context, and somehow bits & pieces of this have to be passed on to evaluate exclusions. This adds an additional layer of connection/indirection, which is hard to enforce.

What if we never have to pass on the context, and the exclusion can be done inline – right where the context is? i.e. What if exclusion is not a separate construct, but a function. For example:

deny_root[result] {
	input.kind == "Deployment"

	exclude(input.name == "mydep")
	# ^^^^^^^^^^^^ let's use context right here

	c = input.spec.template.spec.containers[_]
	not c.securityContext.runAsNonRoot

	exclude(c.name == "host-agent")
	# ^^^^^^^^^^^^ let's use context right here

	result := sprintf("container %s in deployment %s doesn't set runAsNonRoot", [c.name, input.metadata.name])
	# ^^^^^^^^^^^^ normal sprintf result, no need to pass around context
}

Would be awesome if the exclude could be caught & the context made a note of without breaking the flow – i.e. the exclude function doesn't do anything, but the rule is allowed to continue executing so that the reason is captured, and then at the end of evaluation the rule is marked as excluded with it's reason intact. Or maybe the excludes have to be moved to the bottom to have the reason already present in the context like:

deny_root[result] {
	input.kind == "Deployment"
	c = input.spec.template.spec.containers[_]
	not c.securityContext.runAsNonRoot
	result := sprintf("container %s in deployment %s doesn't set runAsNonRoot", [c.name, input.metadata.name])

	exclude(c.name == "host-agent")
	exclude(input.name == "mydep")
}

@jalseth
Copy link
Member

jalseth commented Jan 29, 2022

Hi @moredhel. Friendly ping, please provide an update on the status of this PR.

@moredhel
Copy link
Contributor Author

Hi @jalseth,

Apologies for the late reply, have been on holiday the last few weeks.

I have the following issue where I am asking for more general feedback on if this is a reasonable way to proceed #591

I'm still wanting this to go ahead, but would like to get some feedback for a member on how valuable this is (for myself I feel the current solution isn't flexible enough, so would appreciate the extra flexibility of what I'm proposing).

Essentially lost of the work is done, outstanding tasks:

  • Documentation
  • Unit tests
  • a once-over to verify everything looks good.

Let me know your thoughts and if you're happy I can push this through to be mergeable.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants