Using if keyword in rule declaration makes the AST rule name empty

[jalseth]

Short description

Using if keyword in rule declaration makes the AST rule name is empty. This breaks conftest which skips evaluation of rules that do not follow the deny/warn/exception naming pattern. Using the if keyword is recommended by the Regal linter https://docs.styra.com/regal/rules/idiomatic/use-if.

• OPA version: OPA: 0.60.0

Steps To Reproduce

Run conftest test -p examples/awssam/policy examples/awssam/lambda.yaml and confirm it raises violations as expected. Then, apply the following git patch:

diff --git a/examples/awssam/policy/policy.rego b/examples/awssam/policy/policy.rego
index 0f0b0e9..8050085 100644
--- a/examples/awssam/policy/policy.rego
+++ b/examples/awssam/policy/policy.rego
@@ -1,5 +1,7 @@
 package main
 
+import future.keywords.if
+
 denylist := ["*"]
 
 sensitive_denylist := [
@@ -26,25 +28,25 @@ check_runtime(runtime, denylist) {
 	contains(runtime, denylist[_])
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Runtime = "python2.7"
 	msg = "python2.7 runtime not allowed"
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Runtime = runtime
 	check_runtime(runtime, runtime_denylist)
 	msg = sprintf("%s runtime not allowed", [runtime])
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Action = a
 	check_resources(a, denylist)
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Effect = "Allow"
 	msg = "excessive Action permissions not allowed"
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Action = a
 	is_string(a)
 	endswith(a, "*")
@@ -52,14 +54,14 @@ deny[msg] {
 	msg = "excessive Action permissions not allowed"
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Resource = a
 	check_resources(a, denylist)
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Effect = "Allow"
 	msg = "excessive Resource permissions not allowed"
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Resource = a
 	is_string(a)
 	endswith(a, "*")
@@ -67,7 +69,7 @@ deny[msg] {
 	msg = "excessive Resource permissions not allowed"
 }
 
-deny[msg] {
+deny[msg] if {
 	input.Resources.LambdaFunction.Properties.Environment.Variables = a
 	check_sensitive(a, sensitive_denylist)
 	input.Resources.LambdaFunction.Properties.Policies[_].Statement[_].Effect = "Allow"

Now run ./conftest test -p examples/awssam/policy examples/awssam/lambda.yaml again and you will see the tests now pass. With some printf debugging I see the following rule names from this policy file. You'll notice the deny are all gone when using the if keyword and an empty string is returned instead.

Before:

$ ./conftest test -p examples/awssam/policy examples/awssam/lambda.yaml
denylist
sensitive_denylist
runtime_denylist
check_resources
check_sensitive
check_runtime
deny
deny
deny
deny
deny
deny
deny
FAIL - examples/awssam/lambda.yaml - main - Sensitive data not allowed in environment variables
FAIL - examples/awssam/lambda.yaml - main - excessive Action permissions not allowed
FAIL - examples/awssam/lambda.yaml - main - excessive Resource permissions not allowed
FAIL - examples/awssam/lambda.yaml - main - python2.7 runtime not allowed

7 tests, 3 passed, 0 warnings, 4 failures, 0 exceptions

After:

./conftest test -p examples/awssam/policy examples/awssam/lambda.yaml
denylist
sensitive_denylist
runtime_denylist
check_resources
check_sensitive
check_runtime








0 tests, 0 passed, 0 warnings, 0 failures, 0 exceptions

Expected behavior

The *ast.Rule.Head.Name.String() method returns the rule name regardless of usage of the if keyword.

Additional context

https://openpolicyagent.slack.com/archives/CBR63TK2A/p1704221665685919

[anderseknert]

So, there's a few things going on here, but it largely boils down to this.

With the introduction of reference heads, or "nested rules", like...

foo.bar.baz {
    # some conditions
}

...the contains keyword is required when if is used for a partial, set generating rule, like the ones Conftest expects.

Not using contains will have the rule treated as a map-generating rule where the assignment is true unless otherwise stated, i.e:

deny[x] if {
    # some conditions
}

will be interpreted as:

deny[x] := true if {
    # some conditions
}

Fixing this ambiguity is one of the main goals of OPA 1.0, and the relatively new docs on that topic describe this in more detail.

That the name disappears when using if is related, but not for the if itself but because the rule gets interpreted differently.

Nested rules like the one shown above does not emit a name attribute in the AST, simply because it's not clear what the "name" would refer to in a nested rule, e.g:

cluster.deployments[deployment].pods[pod] {
    # some conditions
}

Would it be cluster? Or cluster.deployments, or cluster.deployment.pods? Or perhaps something that included the vars in the rule ref too?

It's still possible to build something akin to a name yourself by looking at the ref for the rule in the AST, but we have no agreed upon convention for how one should choose to do so for nested ref rules.

Finally — it is true that Regal recommends the use of if, but unless manually disabled it would also recommend the use of contains for each of these rules. Had the recommendations from Regal been followed consistently here, this would not have been an issue.

Not sure if there's anything actionable here which isn't already described in other issues, and the docs around OPA 1.0, if, contains, and so on. But I'll leave that for others to decide.

Either way, this is really useful as input to the work being done in this space, so thank you for raising the issue!

[jalseth]

@anderseknert Thanks for the explanation. Feel free to close this, or leave it open if you prefer.

[anderseknert]

Thanks James! I think I'll close this as there are other issues and even open PRs addressing this, but this is great for future reference. Good that you found a solution to work with this in Conftest in the meantime 👍