Skip to content

Commit

Permalink
Merge pull request enterprise-contract#1129 from lcarva/EC-517
Browse files Browse the repository at this point in the history
Document pitfalls of writing policy rules
  • Loading branch information
lcarva authored Sep 6, 2024
2 parents 7b17eb7 + 286854a commit 203b21e
Showing 1 changed file with 85 additions and 0 deletions.
85 changes: 85 additions & 0 deletions antora/docs/modules/ROOT/pages/authoring.adoc
Original file line number Diff line number Diff line change
Expand Up @@ -58,3 +58,88 @@ further reference on annotations.
The https://enterprisecontract.dev/docs/ec-cli/main/index.html[ec-cli] is reponsible for gathering
the information to be validated which is made available to policies via the `input` object. Its
structure is defined https://enterprisecontract.dev/docs/ec-cli/main/policy_input.html[here].

== Pitfalls

Today, EC takes the https://www.conftest.dev/[conftest] approach for asserting violations and
warnings. The approach follows the path of proving a negative. The policy rules search for issues.
If there are no issues, the policy rule passes. This has pitfalls, e.g. a policy rule could
accidentally pass if not written carefully.

The main motivation for the current state is that this allows policy rules to provide precise error
messages. This is an important requirement of EC; a simple pass/fail result is just not enough.

To illustrate the pitfalls, consider the following policy rule.

```rego
package main

import rego.v1

deny contains result if {
some att in input.attestations
got := att.statement.predicateType
got != "https://slsa.dev/provenance/v0.2"
result := {"msg": sprintf(
"Unexpected predicate type %s in statement %s",
[got, att.statement._type],
)}
}
```

The policy rule above iterates over the list of attestations in the input, extracts the
predicateType for each, and verifies the value is not unexpected. Even this simple policy rule has a
few pitfalls.

First, if there are no attestations, the policy rule passes. Consider adding an explicit check to
ensure `input.attestations` is not empty.

Second, if an attestation does not have the statement or the statement.predicateType attribute the
rule passes for that attestation. This is problematic because a missing value is clearly not equal
to the expected value, `"https://slsa.dev/provenance/v0.2"`. Use helper functions with default
values to access such attributes.

Third, if the statement for an attestation does not set the `_type` attribute, the policy rule
passes. Notice how this particular attribute is used only for error reporting. Use helper functions
with default values to access such attributes.

Fourth, typos when accessing nested attributes, e.g. `att.statement.predicateTypoooo`, cause the
policy rule to pass. Unlike typos in function names or variable names, these are not caught by the
linter nor the compiler. Use helper functions with default values to access such attributes and
ensure code is tested with real-world data.

Each of those pitfalls can be prevented. However, these require a conscious decision by the policy
rule author. This can be even more challenging for authors new to the rego programming language.

The pitfalls above showcase an interesting property of rego. A statement within the rule that
produces no value (or a false value) causes the rule to not produce a value as well. In our case,
since the rule is asserting a violation, no value means no violation.

Here is a safer version of the example above:

```rego
package main

import rego.v1

deny contains result if {
some att in attestations
got := predicate_type(att)
got != "https://slsa.dev/provenance/v0.2"
result := {"msg": sprintf(
"Unexpected predicate type %s in statement %s",
[got, statement_type(att)],
)}
}

deny contains result if {
count(attestations) == 0
result := {"msg": "No attestation found"}
}

attestations := object.get(input, "attestations", [])

statement_type(att) := object.get(att, ["statement", "_type"], "N/A")

predicate_type(att) := object.get(att, ["statement", "predicateType"], "N/A")
```

0 comments on commit 203b21e

Please sign in to comment.