PrepareForEval error when using partial evaluation: "rego_unsafe_var_error: expression is unsafe" #4766

ghost · 2022-06-09T23:03:46Z

Short description

With OPA go library versions v0.39.0 and v0.41.0, when we use the every keyword we're seeing an unexpected error from PrepareForEval, but only when we use WithPartialEval:

1 error occurred: policy.rego:8: rego_unsafe_var_error: expression is unsafe

As far as we knew this error never came up when we were evaluating the rego.Rego object directly. It started happening when we moved over to using PrepareForEval.

For reproduction steps, policies, and example go code that reproduces the problem, see below.

Note that it seems to have something to do with the structure of modules/packages that we use--if I just put everything in the same file I can't seem to reproduce the problem.

Code

main.go

package main

import (
	"context"
	_ "embed"
	"fmt"

	"github.com/open-policy-agent/opa/rego"
)

//go:embed iam.rego
var iamModule string

//go:embed internal.rego
var internalModule string

//go:embed policy.rego
var policyModule string

func main() {
	ctx := context.Background()
	modules := []func(r *rego.Rego){
		rego.Module("iam.rego", iamModule),
		rego.Module("internal.rego", internalModule),
		rego.Module("policy.rego", policyModule),
		rego.Query("data.iam.internal.check_access"),
	}
	input := rego.EvalInput(map[string]interface{}{})
	r := rego.New(modules...)

	q, err := r.PrepareForEval(ctx)
	if err != nil {
		fmt.Printf("PrepareForEval without partial: %s\n", err.Error())
		return
	}
	_, err = q.Eval(ctx, input)
	if err != nil {
		fmt.Printf("Eval without partial: %s\n", err.Error())
		return
	}

	q, err = r.PrepareForEval(ctx, rego.WithPartialEval())
	if err != nil {
		fmt.Printf("PrepareForEval with partial: %s\n", err.Error())
		return
	}
	_, err = q.Eval(ctx, input)
	if err != nil {
		fmt.Printf("Eval with partial: %s\n", err.Error())
		return
	}
}

go.mod

module bug

go 1.18

require github.com/open-policy-agent/opa v0.41.0

require (
	github.com/OneOfOne/xxhash v1.2.8 // indirect
	github.com/agnivade/levenshtein v1.0.1 // indirect
	github.com/ghodss/yaml v1.0.0 // indirect
	github.com/gobwas/glob v0.2.3 // indirect
	github.com/pkg/errors v0.9.1 // indirect
	github.com/rcrowley/go-metrics v0.0.0-20200313005456-10cdbea86bc0 // indirect
	github.com/vektah/gqlparser/v2 v2.4.4 // indirect
	github.com/xeipuuv/gojsonpointer v0.0.0-20190905194746-02993c407bfb // indirect
	github.com/xeipuuv/gojsonreference v0.0.0-20180127040603-bd5ef7bd5415 // indirect
	github.com/yashtewari/glob-intersection v0.1.0 // indirect
	gopkg.in/yaml.v2 v2.4.0 // indirect
)

Policies

iam.rego

package iam

value_missing(role, resource, key) {
	value := object.get(role, key, "")
	value == ""
}

internal.rego

package iam.internal

check_access[identifier] {
	data.resourceID.policyID.allow(input.subject.roles[_], input.resource.resources[resource_idx])
	identifier := input.resource.resources[resource_idx].identifier
}

policy.rego

package resourceID.policyID

import future.keywords.every

foo(outer_role, resource, key) {
	every role in outer_role {
		# PrepareForEval error: policy.rego:8: rego_unsafe_var_error: expression is unsafe
		not data.iam.value_missing(role, resource, key)
	}
}

allow(role, resource) {
	foo(role, resource, "bar")
}

Steps To Reproduce

Create main.go, go.mod, and all .rego files as named and with the above contents
Run main.go: go run .
Observe error from PrepareForEval() but only under WithPartialEval():

$ go run .
PrepareForEval with partial: 1 error occurred: policy.rego:8: rego_unsafe_var_error: expression is unsafe

Expected behavior

We would expect that PrepareForEval() completes without error using WithPartialEval(), i.e. the above script runs without producing any output.

Additional context

We've successfully worked around this issue by avoiding the use of the every keyword and instead using the "not-some-not" pattern mentioned in the docs, which results in Rego policies that do what we need them to do but are harder to read.

The text was updated successfully, but these errors were encountered:

srenatus · 2022-06-10T08:40:39Z

Thanks for the detailed report!

Having a look, here's what the compiler does to your modules when running PrepareForEval with partial eval:

package partial.iam.internal # PE support module

check_access[__local7__1] {
    __local49__ = input.subject.roles[_]
    every __local0__, __local1__ in __local49__ {
        not data.iam.value_missing(__local1__, __local16__, "bar")
    }
    __local7__1 = input.resource.resources[resource_idx1].identifier
}

package iam.internal # internal.rego

check_access[__local7__] {
    __local25__ = input.subject.roles[_]
    __local26__ = input.resource.resources[resource_idx]
    data.resourceID.policyID.allow(__local25__, __local26__)
    __local7__ = input.resource.resources[resource_idx].identifier
}

package resourceID.policyID # policy.rego

foo(__local42__, __local43__, __local44__) = true {
    __local23__ = __local42__
    __local24__ = __local23__
    __local50__ = __local24__
    __local51__ = __local50__
    __local52__ = __local51__
    every __local45__, __local46__ in __local52__ {
        not data.iam.value_missing(__local46__, __local43__, __local44__)
    }
}
allow(__local47__, __local48__) = true {
    data.resourceID.policyID.foo(__local47__, __local48__, "bar")
}

package iam # iam.rego

value_missing(__local13__, __local14__, __local27__) = true {
    object.get(__local13__, __local27__, "", __local22__)
    __local3__ = __local22__
    __local3__ = ""
}

package partial # PE result

__result__ = _ {
    data.partial.iam.internal.check_access = _
}

🔍 Looks like we're losing our future.keywords.every imports along the way.

Interestingly, the same is not true for running PE upfront via opa eval -p:

opa eval -fpretty -p -d policy.rego -d internal.rego -d iam.rego data.iam.internal.check_access
+-----------+------------------------------------------------------------------------------------------------------------------------+
| Query 1   | data.partial.iam.internal.check_access                                                                                 |
+-----------+------------------------------------------------------------------------------------------------------------------------+
| Support 1 | package partial.iam.internal                                                                                           |
|           |                                                                                                                        |
|           | import future.keywords.every                                                                                           |
|           |                                                                                                                        |
|           | check_access[__local4__1] {                                                                                            |
|           |   every __local8__, __local9__ in input.subject.roles[_] { not data.iam.value_missing(__local9__, __local6__, "bar") } |
|           |   __local4__1 = input.resource.resources[resource_idx1].identifier                                                     |
|           | }                                                                                                                      |
+-----------+------------------------------------------------------------------------------------------------------------------------+

Just the first steps. We'll need to look further into this.

srenatus · 2022-06-10T09:10:04Z

🤔 I think the "missing imports" are a red herring. The modules have already been parsed, so the import doesn't need to be there...

Anyways, commenting out the first eval, to avoid potential crossed wires, running only

	q, err := r.PrepareForEval(ctx, rego.WithPartialEval())
	if err != nil {
		fmt.Printf("PrepareForEval with partial: %s\n", err.Error())
		return
	}
	_, err = q.Eval(ctx, input)
	if err != nil {
		fmt.Printf("Eval with partial: %s\n", err.Error())
		return
	}

in main.go, we'll get this error:

PrepareForEval with partial: 1 error occurred: policy.rego:8: rego_unsafe_var_error: var resource is unsafe

foo(outer_role, resource, key) {
	every role in outer_role {
		not data.iam.value_missing(role, resource, key) # HERE
	}
}

OHH wait I believe I've found it:

package partial.iam.internal # PE support module

check_access[__local7__1] {
    __local49__ = input.subject.roles[_]
    every __local0__, __local1__ in __local49__ {
        not data.iam.value_missing(__local1__, __local16__, "bar")
    }
    __local7__1 = input.resource.resources[resource_idx1].identifier
}

I'm not sure about the location and all that, but __local16__ is definitely unsafe there.

So the problem has to do with allow and foo getting inlined, without having properly rewritten the body of the every expression.

And looking at the support module in my previous comment more closely, it exhibits the same problem:

check_access[__local4__1] {
    every __local8__, __local9__ in input.subject.roles[_] {
        not data.iam.value_missing(__local9__, __local6__, "bar") # <<< __local6__ is unsafe
    }
    __local4__1 = input.resource.resources[resource_idx1].identifier
}

srenatus · 2022-06-10T09:28:15Z

ℹ️ minimal example

package test

import future.keywords.every

p {
	f(input)
}

f(x) {
	every y in [1] {
		a := x
		1 == y
	}
}

$ opa eval -p -fsource -d e.rego data.test.p          
# Query 1
every __local1__, __local2__ in [1] { __local3__ = __local0__; 1 = __local2__}
__local0__2 = input

☝️ __local3__ = __local0__ is unsafe.

ghost · 2022-06-10T17:38:29Z

Thanks for looking into this !

I'm not sure if it makes a difference but one thing to note is the policies here aren't exactly what we're using. In actual usage we're consuming all arguments in the fn analogous to iam.value_missing given here. I can share the exact policies privately if necessary

srenatus · 2022-06-13T08:33:44Z

@jguenther-va With the branch of that PR ☝️ your main.go runs through without errors. If you could take a look, and perhaps try it with your real-world policies, that would be great.

ghost · 2022-06-14T17:41:18Z

@srenatus it does fix the error in the main.go above but unfortunately it doesn't fix all instances of "unsafe expression" we're seeing from our actual policies. There's 2 places we had been using every and the other one must be different in some way 🤔

I will see if I can reproduce the same situation in main.go again here, thank you

ghost · 2022-06-14T20:00:19Z

@srenatus this seems to reproduce it again (with these changes to iam.rego and policy.rego, and using your opa fork branch from #4775, but otherwise the same as in the original description).

It's not exactly how our policies are actually defined/pseudocode, so it probably doesn't make much sense to read but:

PrepareForEval with partial: 1 error occurred: iam.rego:17: rego_unsafe_var_error: expression is unsafe

iam.rego

package iam

import future.keywords.every

baz(resource, key) = r {
	r := key
}

bazs(resource, keys) = r {
	r := [key | key := baz(resource, keys[_])]
}

bar(role, resource, role_attr_key, resource_attr_keys) {
	role_attr_keys := bazs(resource, role_attr_key)
	role_attrs := object.get(role, role_attr_keys, [""])
	resource_attrs := object.get(resource.attributes, resource_attr_keys, [""])
	every resource_attr in resource_attrs {
		resource_attr == role_attrs[_]
	}
}

policy.rego

package resourceID.policyID

allow(role, resource) {
	data.iam.bar(role, resource, "foo", "bar")
}

srenatus · 2022-06-15T08:30:13Z

@jguenther-va thanks for being persistent. I'll have another look with that second case 🔍

srenatus · 2022-06-15T09:36:59Z

So this one seems unrelated to the previous one. When reordering this rule body for safety,

check_access[__local16__1] {
    __local6__4 = [ __local5__4 |
        __local24__4 = __local4__4[_]
        data.iam.baz(input.resource.resources[resource_idx1], __local24__4, __local19__4)
        __local5__4 = __local19__4
        __local4__4 = "foo"
    ]
    object.get(input.subject.roles[_], __local6__4, [""], __local21__3)
    object.get(input.resource.resources[resource_idx1].attributes, "bar", [""], __local54__)
    every __local28__, __local29__ in __local54__ {
        __local29__ = __local21__3[_]
    }
    __local16__1 = input.resource.resources[resource_idx1].identifier
}

it fails, complaining that the every expression wasn't safe because of __local21__3. Now, that local is safe -- it's set by the first object.get call.

However, this bit looks puzzling:

    __local6__4 = [ __local5__4 |
        __local24__4 = __local4__4[_]
        data.iam.baz(input.resource.resources[resource_idx1], __local24__4, __local19__4)
        __local5__4 = __local19__4
        __local4__4 = "foo"
    ]

I don't see how this would ever be satisfiable: __local4__4 = "foo" is makes __local4__4 a string, but those can't be indexed, so __local24__4 = __local4__4[_] wouldn't work out at all. However that seems like an artifact of the test call.

When using data.iam.bar(role, resource, ["foo"], "bar") in policy.rego, we get this rule body,

	__local6__4 = [__local5__4 |
		__local24__4 = __local4__4[_]
		data.iam.baz(input.resource.resources[resource_idx1], __local24__4, __local19__4)
		__local5__4 = __local19__4
		__local4__4 = ["foo"]
	]

which is OK. But the error persists.

srenatus · 2022-06-15T10:24:50Z

📝
when this reordered in reorderBodyForClosures,

object.get(input.subject.roles[_], __local6__4, [""], __local21__3)
object.get(input.resource.resources[resource_idx1].attributes, "bar", [""], __local54__)
__local16__1 = input.resource.resources[resource_idx1].identifier
__local6__4 = [__local5__4 |
  __local24__4 = __local4__4[_]
  data.iam.baz(input.resource.resources[resource_idx1], __local24__4, __local19__4)
  __local5__4 = __local19__4
  __local4__4 = ["foo"]
]

then outputVarsForBody(reordered, ...) gives us[__local16__1 __local54__ __local6__4 resource_idx1]. I think that's missing __local21__3.

It's missing that because when the output vars of the call are checked, we get nothing: it'll recognize that __local6__4 is not safe and give up on that call. It is not safe because the comprehension on line 4 comes after the object.get call of line 1. It's not properly reordered in reordered. 🤔

srenatus · 2022-06-15T12:49:32Z

So no patch yet, but I'm closing in on the problem. This is a very productive issue, thanks for that 👍 😄

ghost · 2022-06-15T18:27:08Z

Please let me know if it would help to see the actual policies we're using (can share privately). I made sure the error is the exact same after trimming it down and anonymizing it, but I'm not sure if that could have changed something unintentionally--there are several rules in actual usage that aren't in the policies above.

Thanks again !

srenatus · 2022-06-15T18:42:01Z

Thanks a bunch. Even if it was a wrongly-trimmed policy, it's been putting the spotlight on a real bug. Let's keep this dance going, when I get a fix pushed, you can run it against your real policies... If it still doesn't work out, I'll happily have a look at your policies. 🔍

Our previous approach, ordering for closures first, and taking the growing set of output variables of the reordered body into account in a second step, did not work out for some examples: object.get(input.subject.roles[_], comp, [""], output) comp = [ 1 | true ] every y in [2] { y in output } Here, the closure of `every` would have not checkout out because we've never registered the output variable `output` -- since the first call to object.get is unsafe without `comp`, too. Now, the two stages have been merged. It's got surprisingly little fallout, one test case had to be adjusted, which I believe to be a minor case, too. Fixes the second part of open-policy-agent#4766. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus · 2022-06-21T12:01:26Z

Hello there! I've just opened a second PR, #4801, to address the second bug we've cornered here.

I've pushed both commits to an extra branch for experimenting, and I might be missing something -- it's been a while -- but go run main.go now passes without trouble for me. Please try this branch.

Our previous approach, ordering for closures first, and taking the growing set of output variables of the reordered body into account in a second step, did not work out for some examples: object.get(input.subject.roles[_], comp, [""], output) comp = [ 1 | true ] every y in [2] { y in output } Here, the closure of `every` would have not checkout out because we've never registered the output variable `output` -- since the first call to object.get is unsafe without `comp`, too. Now, the two stages have been merged. It's got surprisingly little fallout, one test case had to be adjusted, which I believe to be a minor case, too. Fixes the second part of open-policy-agent#4766. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

ghost · 2022-06-22T17:33:39Z

@srenatus on the sr/issue-4766 branch (commit 3c400b8) I'm now seeing a different error:

PrepareForEval with partial: 1 error occurred: rego_type_error: undefined ref: __local19__4[_]
	__local19__4[_]
	^^^^^^^^^^^^
	have: string

not sure what the difference is here that you're not seeing that error, just double checked and the only change after the original PR description was the 2 policy files mentioned in this comment

edit: if I try the branch in that second PR this is the error I get (may just be because it doesn't have the fix from the first PR though?)

PrepareForEval with partial: 2 errors occurred:
iam.rego:18: rego_unsafe_var_error: var _ is unsafe
iam.rego:18: rego_unsafe_var_error: var role_attrs is unsafe

srenatus · 2022-06-22T17:52:56Z

PrepareForEval with partial: 1 error occurred: rego_type_error: undefined ref: __local19__4[_]
	__local19__4[_]
	^^^^^^^^^^^^
	have: string

This is consistent with not having [ ] around the "foo" argument, see the last parts of #4766 (comment)

ghost · 2022-06-22T18:28:36Z

@srenatus whoops my bad, just checked and the fix from sr/issue-4766 does indeed fix our actual usage of every where we originally saw this problem

many thanks !

Our previous approach, ordering for closures first, and taking the growing set of output variables of the reordered body into account in a second step, did not work out for some examples: object.get(input.subject.roles[_], comp, [""], output) comp = [ 1 | true ] every y in [2] { y in output } Here, the closure of `every` would have not checkout out because we've never registered the output variable `output` -- since the first call to object.get is unsafe without `comp`, too. Now, the two stages have been merged. It's got surprisingly little fallout, one test case had to be adjusted, which I believe to be a minor case, too. Fixes the second part of #4766. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus · 2022-06-29T06:41:52Z

✔️ all merged

ghost added the bug label Jun 9, 2022

srenatus mentioned this issue Jun 13, 2022

topdown/eval: fix 'every' term plugging on save #4775

Merged

srenatus mentioned this issue Jun 21, 2022

ast/compile: reorder body for safety differently #4801

Merged

srenatus closed this as completed Jun 29, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

PrepareForEval error when using partial evaluation: "rego_unsafe_var_error: expression is unsafe" #4766

PrepareForEval error when using partial evaluation: "rego_unsafe_var_error: expression is unsafe" #4766

ghost commented Jun 9, 2022

srenatus commented Jun 10, 2022

srenatus commented Jun 10, 2022

srenatus commented Jun 10, 2022

ghost commented Jun 10, 2022

srenatus commented Jun 13, 2022

ghost commented Jun 14, 2022

ghost commented Jun 14, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

ghost commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 21, 2022

ghost commented Jun 22, 2022 •

edited by ghost

Loading

srenatus commented Jun 22, 2022

ghost commented Jun 22, 2022

srenatus commented Jun 29, 2022

PrepareForEval error when using partial evaluation: "rego_unsafe_var_error: expression is unsafe" #4766

PrepareForEval error when using partial evaluation: "rego_unsafe_var_error: expression is unsafe" #4766

Comments

ghost commented Jun 9, 2022

Short description

Code

Policies

Steps To Reproduce

Expected behavior

Additional context

srenatus commented Jun 10, 2022

srenatus commented Jun 10, 2022

srenatus commented Jun 10, 2022

ghost commented Jun 10, 2022

srenatus commented Jun 13, 2022

ghost commented Jun 14, 2022

ghost commented Jun 14, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 15, 2022

ghost commented Jun 15, 2022

srenatus commented Jun 15, 2022

srenatus commented Jun 21, 2022

ghost commented Jun 22, 2022 • edited by ghost Loading

srenatus commented Jun 22, 2022

ghost commented Jun 22, 2022

srenatus commented Jun 29, 2022

ghost commented Jun 22, 2022 •

edited by ghost

Loading