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

OPA server can return result for wrong query when using ?partial #2247

Closed
patrick-east opened this issue Mar 30, 2020 · 0 comments · Fixed by #2248
Closed

OPA server can return result for wrong query when using ?partial #2247

patrick-east opened this issue Mar 30, 2020 · 0 comments · Fixed by #2248
Assignees
Labels

Comments

@patrick-east
Copy link
Contributor

Expected Behavior

Starting up the server with some test policies:

policy.rego

package example_rbac

default allow = false

allow {
    user_has_role[role_name]
    role_has_permission[role_name]
}

user_has_role[role_name] {
    role_binding = data.bindings[_]
    role_binding.role = role_name
    role_binding.user = input.subject.user
}

role_has_permission[role_name] {
    role = data.roles[_]
    role.name = role_name
    role.operation = input.action.operation
    role.resource = input.action.resource
}

data.json

{
    "roles": [
        {
            "operation": "read",
            "resource": "widgets",
            "name": "widget-reader"
        },
        {
            "operation": "write",
            "resource": "widgets",
            "name": "widget-writer"
        }
    ],
    "bindings": [
        {
            "user": "inspector-alice",
            "role": "widget-reader"
        },
        {
            "user": "maker-bob",
            "role": "widget-writer"
        }
    ]
}
opa run -s ./data.json ./policy.rego

I should then be able to make multiple queries to the server with them affecting the results of each other:

input.json

{
  "input": {
    "subject": {
      "user": "maker-bob"
    },
    "action": {
      "resource": "widgets",
      "operation": "write"
    }
  }
}

First query for allow

{15:50} ✘ /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/allow'
{"result":true}%

Now for user_has_role

{15:50} /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/user_has_role'
{"result":["widget-writer"]}%

Back to allow

{15:50} /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/allow'
{"result":true}%

🎉
Success!

I should then be able to do the same thing with ?partial

Actual Behavior

The response I get back for any cached ?partial query is the result of the last one to have been partially evaluated:
First query for allow

{15:50} /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/allow?partial'
{"result":true}%

Now for user_has_role

{15:51} /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/user_has_role?partial'
{"result":["widget-writer"]}%

Back to allow

{15:51} /tmp ❯ curl -X POST -d "@input.json" 'http://localhost:8181/v1/data/example_rbac/allow?partial'
{"result":["widget-writer"]}%

🤔

Additional Info

It looks like the problem is that we share a compiler for all of the cached partially evaluated rego's and each time we do the partial evaluation we store the freshly generated module on the compiler with id __partialresult__ and the same data.partial.__result__ query. Now when any subsequent evaluations occur for a cached partial query path we end up using that same module, regardless of whether or not it was actually the partial module associated with the query path originally.

@patrick-east patrick-east self-assigned this Mar 30, 2020
patrick-east added a commit to patrick-east/opa that referenced this issue Apr 2, 2020
Previously we let it use the default namespace, which meant that
every cached evaluation would use the same query on the compiler..
which isn't correct. They need to be unique per path.

We'll now use a hash of the path (since it needs to be a valid var).

While doing this the logic for the Rego opts was refactored in
`makeRego` to only define the list a single time.. this should help
reduce the risk of any regressions in the future.

Fixes: open-policy-agent#2247
Signed-off-by: Patrick East <[email protected]>
tsandall pushed a commit that referenced this issue Apr 6, 2020
Previously we let it use the default namespace, which meant that
every cached evaluation would use the same query on the compiler..
which isn't correct. They need to be unique per path.

We'll now use a hash of the path (since it needs to be a valid var).

While doing this the logic for the Rego opts was refactored in
`makeRego` to only define the list a single time.. this should help
reduce the risk of any regressions in the future.

Fixes: #2247
Signed-off-by: Patrick East <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Archived in project
Development

Successfully merging a pull request may close this issue.

1 participant