open-policy-agent / opa

Open Policy Agent (OPA) is an open source, general-purpose policy engine.
https://www.openpolicyagent.org
Apache License 2.0
9.37k stars 1.3k forks source link

nil pointer dereference in opa-envoy-plugin #5595

Closed jakubdyszkiewicz closed 1 year ago

jakubdyszkiewicz commented 1 year ago

Hey 👋

Short description

We use OPA in Kong Mesh by embedding it in our sidecar. We noticed that whenever we add multiple policies to the runtime store and send a request, OPA agent panics. I built a minimal repo https://github.com/jakubdyszkiewicz/opa-envoy-panic-repro to reproduce it

If we somehow misuse the API, I'd love to know what we should improve.

Here is the log.

{"addrs":[":9999"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server.","time":"2023-01-25T16:06:30+01:00"}
{"addr":"127.0.0.1:8888","dry-run":false,"enable-reflection":false,"level":"info","msg":"Starting gRPC server.","path":"envoy/authz/allow","query":"","time":"2023-01-25T16:06:30+01:00"}
panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x2 addr=0x20 pc=0x1056ddfd4]

goroutine 9 [running]:
github.com/open-policy-agent/opa/topdown.evalVirtual.eval({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x3, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2266 +0x84
github.com/open-policy-agent/opa/topdown.evalTree.next({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x3, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2101 +0x260
github.com/open-policy-agent/opa/topdown.evalTree.eval({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x3, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2054 +0xd0
github.com/open-policy-agent/opa/topdown.evalTree.next({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x2, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2107 +0x2b0
github.com/open-policy-agent/opa/topdown.evalTree.eval({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x2, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2054 +0xd0
github.com/open-policy-agent/opa/topdown.evalTree.next({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x1, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2107 +0x2b0
github.com/open-policy-agent/opa/topdown.evalTree.eval({0x14000146c00, {0x140003fc920, 0x4, 0x4}, {0x140003fcba0, 0x4, 0x4}, 0x1, 0x14000352c60, 0x14000348378, ...}, ...)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:2054 +0xd0
github.com/open-policy-agent/opa/topdown.(*eval).biunifyRef(0x14000146c00, 0x105e39ca0?, 0x14000348378, 0x14000352c60, 0x14000352c60, 0x1052ec97c?)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:1041 +0x338
github.com/open-policy-agent/opa/topdown.(*eval).biunifyValues(0x14000146c00, 0x14000348360, 0x14000348378, 0x14000352c60, 0x14000352c60, 0x14000352d80)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:957 +0xc4
github.com/open-policy-agent/opa/topdown.(*eval).biunify(0x14000146c00, 0x1056ce0cc?, 0x28?, 0x105d6aa00?, 0x140000d4d01?, 0x104e45bbc?)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:853 +0x5ec
github.com/open-policy-agent/opa/topdown.(*eval).unify(0x140000d4d48?, 0x104e45bbc?, 0x140000d4d58?, 0x104e45bbc?)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:842 +0x28
github.com/open-policy-agent/opa/topdown.(*eval).evalStep(0x14000146c00, 0x14000318ee0)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:360 +0x61c
github.com/open-policy-agent/opa/topdown.(*eval).evalExpr(0x14000146c00, 0x14000318ed0)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:341 +0xe4
github.com/open-policy-agent/opa/topdown.(*eval).eval(0x140000102a0?, 0x10595778d?)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:304 +0x1c
github.com/open-policy-agent/opa/topdown.(*eval).Run(0x14000146c00, 0x14000318ec0)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/eval.go:105 +0xb0
github.com/open-policy-agent/opa/topdown.(*Query).Iter(0x140000d5150, {0x105e37cb8?, 0x14000596750}, 0x140003fcb60)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/topdown/query.go:524 +0x9a8
github.com/open-policy-agent/opa/rego.(*Rego).eval(0x1400002db00, {0x105e37cb8?, 0x14000596750}, 0x140000bcd80)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/rego/rego.go:2007 +0x5fc
github.com/open-policy-agent/opa/rego.PreparedEvalQuery.Eval({{0x1400002db00?, 0x140003188d0?}}, {0x105e37cb8, 0x14000596750}, {0x140000d5540?, 0x140003fc760?, 0x1400057d780?})
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa@v0.48.0/rego/rego.go:411 +0x104
github.com/open-policy-agent/opa-envoy-plugin/envoyauth.Eval({0x105e37cb8, 0x14000596750}, {0x105e3e300, 0x14000578fd0}, {0x105e394a0?, 0x140001cfb80}, 0x140006ef0a0, {0x0, 0x0, 0x0})
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa-envoy-plugin@v0.48.0-envoy/envoyauth/evaluation.go:78 +0x628
github.com/open-policy-agent/opa-envoy-plugin/internal.(*envoyExtAuthzGrpcServer).check(0x14000578fd0, {0x105e37cb8?, 0x14000596750}, {0x105d70fc0, 0x14000596780})
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa-envoy-plugin@v0.48.0-envoy/internal/internal.go:351 +0x54c
github.com/open-policy-agent/opa-envoy-plugin/internal.(*envoyExtAuthzGrpcServer).Check(0x105d70fc0?, {0x105e37cb8?, 0x14000596750?}, 0x140001d0c60?)
    /Users/jakub/go/pkg/mod/github.com/open-policy-agent/opa-envoy-plugin@v0.48.0-envoy/internal/internal.go:291 +0x30
github.com/envoyproxy/go-control-plane/envoy/service/auth/v3._Authorization_Check_Handler({0x105de1d40?, 0x14000578fd0}, {0x105e37cb8, 0x14000596750}, 0x140006ee0e0, 0x0)
    /Users/jakub/go/pkg/mod/github.com/envoyproxy/go-control-plane@v0.10.2-0.20220325020618-49ff273808a1/envoy/service/auth/v3/external_auth.pb.go:692 +0x170
google.golang.org/grpc.(*Server).processUnaryRPC(0x1400016c1e0, {0x105e3dbe0, 0x14000182820}, 0x140001165a0, 0x14000172ed0, 0x106759e10, 0x0)
    /Users/jakub/go/pkg/mod/google.golang.org/grpc@v1.51.0/server.go:1340 +0xb7c
google.golang.org/grpc.(*Server).handleStream(0x1400016c1e0, {0x105e3dbe0, 0x14000182820}, 0x140001165a0, 0x0)
    /Users/jakub/go/pkg/mod/google.golang.org/grpc@v1.51.0/server.go:1713 +0x82c
google.golang.org/grpc.(*Server).serveStreams.func1.2()
    /Users/jakub/go/pkg/mod/google.golang.org/grpc@v1.51.0/server.go:965 +0x84
created by google.golang.org/grpc.(*Server).serveStreams.func1
    /Users/jakub/go/pkg/mod/google.golang.org/grpc@v1.51.0/server.go:963 +0x290
exit status 2

I tried it with both 0.48.0 and 0.47.4, same result.

Steps To Reproduce

I built a minimal repo https://github.com/jakubdyszkiewicz/opa-envoy-panic-repro

Expected behavior

No panic.

Thanks for all the help!

ashutosh-narkar commented 1 year ago

@jakubdyszkiewicz thanks for reporting this. Are you using bundles to serve the policies or some other way? I tried with your policy to repro this.

policy.rego

package envoy.authz

import input.attributes.request.http as http_request

default allow = false

allow {
  http_request.method == "GET"
}

config.yaml

plugins:
    envoy_ext_authz_grpc:
        addr: :9191
        dry-run: false
        enable-reflection: true
decision_logs:
    console: true

Then run opa-envoy:

$ opa_envoy run --server --config-file=config.yaml policy.rego
{"addrs":[":8181"],"diagnostic-addrs":[],"level":"info","msg":"Initializing server.","time":"2023-01-25T10:55:12-08:00"}
{"level":"info","msg":"Starting decision logger.","plugin":"decision_logs","time":"2023-01-25T10:55:12-08:00"}
{"addr":":9191","dry-run":false,"enable-reflection":true,"level":"info","msg":"Starting gRPC server.","path":"test/hello","query":"","time":"2023-01-25T10:55:12-08:00"}
^C{"level":"info","msg":"Shutting down...","time":"2023-01-25T10:55:14-08:00"}
{"level":"info","msg":"Server shutdown.","time":"2023-01-25T10:55:14-08:00"}
{"level":"info","msg":"Stopping decision logger.","plugin":"decision_logs","time":"2023-01-25T10:55:14-08:00"}
jakubdyszkiewicz commented 1 year ago

The bug is only visible with multiple rego policies. If you change const policies = 2 to = 1 in my repro repo and it works. Additionally, just starting OPA with two policies does not expose the problem, the bug is only visible when you send requests.

We do not use bundles, we exchange rego policies between the control plane and data plane via our own protocol (xDS-like). Then we push them to the store like in the example repo using rt.Store.UpsertPolicy.

ashutosh-narkar commented 1 year ago

Can you help me understand why you add the same policy to the store twice under a different id? If you compile the modules, it would generate an error. For ex.

const policy = `
package envoy.authz

import input.attributes.request.http as http_request

default allow = false

allow {
  http_request.method == "GET"
}`
_, err = ast.CompileModules(map[string]string{
        "example1.rego": policy,
        "example2.rego": policy,
    })

You'll see an error like rego_type_error: multiple default rules data.envoy.authz.allow found. Now the panic you reported is something that needs more investigation. I would have expected OPA to maybe give an error and not crash. If you need more info about integrating with OPA's API, you can find that here.

jakubdyszkiewicz commented 1 year ago

The same policy was just a simplification. The real use case would be something like this 1) Mesh operator defines a global policy that denies all requests with specific URL/header (i.e. jndi to protect from Log4Shell) 2) Service owner defines a policy for their own service that allows only GET requests.

After reading more, I see this was a missing part for me https://www.openpolicyagent.org/docs/latest/faq/#collaboration-using-import

I was under the impression that policies would be executed separately. My mistake!

I see that to implement such a use case we need to have a base policy from the mesh operator

package base

default allow = true

deny {
  input.parsed_path == ["jndi"]
}

a policy from the service owner

package service

import input.attributes.request.http as http_request

default allow = false

allow {
  http_request.method == "GET"
}

a policy to stitch this together

package kmesh

import data.service
import data.base

allow {
  service.allow
  base.allow
  not service.deny
  not base.deny
}

and configure OPA Envoy plugin plugins.envoy_ext_authz_grpc.path=kmesh/allow

This works 🎉

I think the issue still might be valid. It would be great if the agent returned a better error.

ashutosh-narkar commented 1 year ago

@jakubdyszkiewicz thanks for confirming. We will into the error message separately.