support basic gRPC payload decoding #202

pnnferreira · 2020-10-01T09:01:10Z

with the new functionality from envoy to "pack_as_bytes", opa can decode the raw body into the parsed body to make policy decisions.

srenatus

👏 🎉 Glad to see the PR opened! Added a few comments inline 👇

internal/internal.go

internal/internal_test.go

srenatus · 2020-10-01T09:17:51Z

internal/internal_test.go

+	tests := map[string]struct {
+		input               *ext_authz.CheckRequest
+		want                interface{}
+		isBodyTruncated     bool


I don't think we currently support proper detection of a truncated protobuf body, do we? This could be a follow-up, I suppose.

currently i dont think so

test/files/Example.proto

srenatus · 2020-10-01T09:22:47Z

test/files/GenerateProto.md

@@ -0,0 +1,3 @@
+Folder: TestGrpc
+
+protoc --proto_path=TestGrpc --go_out=testGrpc --go_opt=paths=source_relative TestGrpc/Example.proto --descriptor_set_out=TestGrpc/example.pb --include_imports


Can you make this a README or a Makefile? Also I believe the paths aren't correct at the moment. 🤔

srenatus · 2020-10-01T09:24:31Z

internal/internal_test.go

@@ -1062,7 +1062,7 @@ func TestGetResponseHttpStatus(t *testing.T) {
 		t.Fatal("Expected error but got nil")
 	}

-	input["http_status"] = json.Number(301)
+	input["http_status"] = json.Number("1")


I suspect that this can disappear when rebasing this branch. It had been dealt with in #201.

ashutosh-narkar · 2020-10-01T17:25:19Z

Thanks for creating the PR @pnnferreira ! The new Envoy functionality to send raw bytes to the external authz server is great and we sure would want to incorporate that in the plugin. The next release of the envoyproxy/go-control-plane with this new feature included is expected soon and I think it would be better to depend on an official release of the library.

pnnferreira · 2020-10-02T11:20:40Z

@ashutosh-narkar Do you have any idea when be the feature will be added on a release?

srenatus · 2020-10-02T11:31:03Z

Couldn't find the commit @ashutosh-narkar has been referring to (or having in mind), but 0.9.7 was released half a day ago (https://github.com/envoyproxy/go-control-plane/releases/tag/v0.9.7) and since it corresponds to what's in master for that repo, I'd assume the release is there now. @ashutosh-narkar Am I missing something? Can you point us to the code change there you've been thinking about, please? 😃

ashutosh-narkar · 2020-10-02T17:02:08Z

v0.9.7 is the release I was referring to before. @pnnferreira can you please update the PR to use the new release ? Thanks

pnnferreira · 2020-10-08T09:46:15Z

i have updated the control plane version

internal/internal_test.go

pnnferreira · 2020-10-12T10:22:43Z

@ashutosh-narkar any obstacle to move forward with this PR?

ashutosh-narkar

@pnnferreira this seems like a good start. I had some comments about the implementation and some nits. Also we should look into Go protocol buffers APIv2. It may cut down on the dependencies.

go.mod

internal/internal.go

ashutosh-narkar · 2020-10-16T00:58:09Z

internal/internal.go

+func getGRPCBody(in []byte, parsedPath []interface{}, data interface{}, protoDescriptor string) error {
+
+	// the first 5 bytes are part of gRPC framing. We need to remove them to be able to parse
+	//https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md


Can you describe this in more detail. I'm not able to locate the relevant section in https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md

I think we should be able to use this instead: https://pkg.go.dev/github.com/matttproud/golang_protobuf_extensions/pbutil#ReadDelimited

💡 I was wrong! 5 is alright, it's documented here:

The repeated sequence of Length-Prefixed-Message items is delivered in DATA frames

Length-Prefixed-Message → Compressed-Flag Message-Length Message
Compressed-Flag → 0 / 1 # encoded as 1 byte unsigned integer
Message-Length → {length of Message} # encoded as 4 byte unsigned integer (big endian)
Message → *{binary octet}

So 1 byte is Compressed-Flag, the next 4 are the Message-Length.

(The ReadDelimited method from pbutil is for situations when the message is prefixed with a varint size, which isn't the case of gRPC's mapping to HTTP2.)

internal/internal.go

ashutosh-narkar · 2020-10-16T01:10:07Z

internal/internal.go

+	inputType := ""
+	packageName := fd.GetPackage()
+	pathServiceRaw := parsedPath[0].(string)
+	pathService := strings.Replace(pathServiceRaw, packageName+".", "", 1)


It's not clear to me what's happening here. Some comments may help.

//To get the service.
//The package and service are in the parsedpath[0] (Example.Test.GRPC.ProtoServiceIExampleApplication), we need to get the service (ProtoServiceIExampleApplication) to find the methods available

internal/internal.go

ashutosh-narkar · 2020-10-16T01:20:03Z

internal/internal.go

+	jsonBody, err := json.Marshal(message)
+	if err != nil {
+		return err
+	}
+
+	if err := util.Unmarshal([]byte(jsonBody), &data); err != nil {
+		return err
+	}


Can this be avoided ?

srenatus · 2020-10-16T11:24:07Z

internal/internal.go

+
+	message := dynamic.NewMessage(msgDesc)
+
+	if err := proto.Unmarshal(in, message); err != nil {


I believe instead of skipping the first few bytes (above), we might be able to do something like this here:

Suggested change

if err := proto.Unmarshal(in, message); err != nil {

if _, err := pbutil.ReadDelimited(bytes.NewReader(in), message); err != nil

I was wrong here, see #202 (comment)

srenatus · 2020-10-16T11:28:53Z

Also we should look into Go protocol buffers APIv2. It may cut down on the dependencies.

@ashutosh-narkar I agree, and I volunteer dealing with this in a follow-up PR. Let's get this in first. 😃

srenatus · 2020-10-29T10:52:51Z

I think it would be good if could get this merged and go on from there.

Right now, what's missing, I believe, is:

document the limitations: no compression, only a single message; also only protobuf (gRPC could be used with other encodings)
rebase, squash the branch

In follow-up PRs, we can deal with

decompression of request bodies, or logging if a compressed on was sent
dealing with multiple messages, perhaps?
eventually switching to APIv2 of protobuf

srenatus · 2020-11-12T08:42:16Z

We've discussed this PR in slack and @pnnferreira thought it would be a good idea if took over.

So, I'm going to pick this up soon.

@pnnferreira I could re-use this PR if you made me a collaborator on your fork, https://github.com/pnnferreira/opa-envoy-plugin, please 😃

pnnferreira · 2020-11-12T09:30:52Z

@srenatus as we discussed on slack i agree.
I have already made me you a collaborator on your fork. thanks 👍

srenatus · 2020-11-16T13:17:14Z

Alright, I've started working on this. It looks like I cannot switch it back to draft, but I'll let y'all know when to have another look. 😎

srenatus · 2020-11-16T13:22:09Z

internal/internal.go

-		cfg:                    *cfg,
-		server:                 grpc.NewServer(),
-		preparedQueryDoOnce:    new(sync.Once),
-		interQueryBuiltinCache: iCache.NewInterQueryCache(m.InterQueryBuiltinCacheConfig()),


TODO merge/rebase messup by me

ashutosh-narkar · 2020-11-16T19:17:35Z

@srenatus I've moved the PR to draft mode. Feel free to switch it back. Thanks !

srenatus · 2020-11-17T13:45:31Z

I've cleaned up a few things here:

parse the protoset once only (in plugin initialization)
not building the golang pb code, we don't need it
using a combined protoset for multiple services (like envoy does it in grpc_json transcoding)
using protobuf APIv2

Remaining TODOs:

see if e2e tests can be expanded to cover this
play around with the plugin and envoy some more, look into edge cases (streaming, compression)
more detailed documentation in README.md

srenatus · 2020-11-18T13:34:21Z

Now, we're dealing properly with edge cases (empty messages, client compression, truncated payloads).

Next, I'll add an examples/grpc/ folder, with a README on how to generate the protoset, and some example calls.
The stuff from that folder can then help extending the e2e tests we currently have. Also, I've probably broken the existing ones, since we're going to ext auth v3 here.

💭 Thinking about it, going to v3 might make more sense in a separate PR, even if it blocks this one. I'd like to avoid this one becoming a hairy mud ball mega-PR.

tsandall · 2020-11-18T13:50:01Z

Next, I'll add an examples/grpc/ folder, with a README on how to generate the protoset, and some example calls.
The stuff from that folder can then help extending the e2e tests we currently have. Also, I've probably broken the existing ones, since we're going to ext auth v3 here.

Thinking about it, going to v3 might make more sense in a separate PR, even if it blocks this one. I'd like to avoid this one becoming a hairy mud ball mega-PR.

Are there backwards compatibility considerations here? What is the minimum version of Envoy that will work? We don't necessarily have to be too strict here but it would be good to know. If we break compatibility with older versions of Envoy, I'm sure there will be users who run into this.

srenatus · 2020-11-18T13:55:58Z

Are there backwards compatibility considerations here? What is the minimum version of Envoy that will work? We don't necessarily have to be too strict here but it would be good to know. If we break compatibility with older versions of Envoy, I'm sure there will be users who run into this.

From the discussion of open-policy-agent/opa#2798, it looks like the support for v2 will be dropped next month. But I can follow-up on it. We could support both gRPC APIs (v2 and v3) in one service, if there's a considerable need for it.

I'm afraid this is not 100% accurate,

Even after incorporating the V3 API the plugin itself would still be backward compatible so I'm not too worried about breaking anyone

...after switching to v3, the service will just not recognize v2 requests anymore.

All the more reason to detangle this from the gRPC body feature.

internal/internal.go

ashutosh-narkar · 2020-12-02T01:02:58Z

internal/internal.go

+		return false, false, fmt.Errorf("less than 5 bytes")
+	}
+
+	// Can be 0 or 1, 1 indicates thet the payload is compressed.


Nit: thet

ashutosh-narkar · 2020-12-02T01:05:54Z

internal/internal.go

+	// Can be 0 or 1, 1 indicates thet the payload is compressed.
+	// The method could be looked up in the request headers, and the
+	// request decompressed; but for now, let's skip it.
+	if in[0] != 0 {


We are simply checking here if the payload is compressed and for now we won't support that.

ashutosh-narkar · 2020-12-02T01:12:01Z

internal/internal.go

+	if int(size) > len(in)-5 {
+		return false, true, nil // truncated body
+	}
+	in = in[5 : size+5]


Is everything from the 6th byte the actual input ?

The structure is <size><payload><size><payload>... if there are multiple messages (i.e. a streaming gRPC method). So, we only support one message for now, and only read that one (if we've been given enough bytes).

ashutosh-narkar · 2020-12-02T01:13:17Z

internal/internal.go

+	svc, err := findService(parsedPath[0].(string), files)
+	if err != nil {
+		return false, false, nil
+	}


Why don't we return the error ?

If we don't bail on these, the policy code can decide what to do with a payload that OPA didn't know how to parse.

They could still forbid any request that doesn't have a input.parsed_body; but we'd allow them to do other things.

I think it's conceivable, for example, that some services (like health checks) aren't included in the proto descriptor set, and the input.parsed_path is used to allow them. If we returned the error here, that wouldn't be possible, every gRPC request ever made would have to be part of our proto descriptor set.

Ultimately, I'd love some user feedback on this. 😃

internal/internal.go

internal/internal_test.go

srenatus · 2020-12-02T11:06:11Z

.github/workflows/post-merge.yaml

+        working-directory: examples/grpc
+
+      - name: Run test
+        run: make test-setup test


I believe this could be flakey. test-setup run docker-compose up -d, and the tests start calling the service; there's a chance that envoy isn't ready yet.

However, I haven't been able to trigger this so far locally,

$ docker-compose up -d; grpcurl -plaintext -protoset testsrv.pb 127.0.0.1:51051 test.KitchenSink/Ping Creating network "grpc_default" with the default driver Creating grpc_envoy_1 ... done Creating grpc_testsrv_1 ... done Creating grpc_opa-envoy_1 ... done { } $

and on github, we're more than likely to pull down the grpcurl image anyways, given envoy plenty of time. While I'm not happy with this arrangement, let's try to roll with it. We'll deal with it if it becomes a problem.

srenatus · 2020-12-02T13:43:21Z

ℹ️ We're rebuilding the testsrv image although it's very unlikely to ever change. Since it's below one minute, I've decided that it might be good enough for now. Future alternatives: push the image to dockerhub, or use grpcbin instead (but they're lacking some WKT, https://github.com/moul/pb/blob/master/grpcbin/grpcbin.proto#L67)

ashutosh-narkar · 2020-12-03T01:03:32Z

.github/workflows/pull-request.yaml

+      - name: Run test log dump and cleanup
+        run: make test-teardown
+        if: ${{ always() }}
+        working-directory: examples/grpc


It would be great if we could find a way to avoid all the duplication. We could create a make target but probably won't look that great in the UI.

Totally agreed! I think github actions have a feature for that, the one or two steps that are specific to each variant (PR check, post-merge) can probably be if:'ed accordingly.

ashutosh-narkar · 2020-12-03T01:11:26Z

README.md

+The protoset can be generated using `protoc`, e.g. `protoc --descriptor_set_out=protoset.pb --include_imports`.
+
+Note that gRPC message payload decoding is only available [using the v3 API](#envoy-xds-v2-and-v2).
+See [`examples/grpc`](examples/grpc) an example setup using Envoy, a gRPC service, and opa-envoy-plugin examining the


Nit: for an example setup using Envoy ...

ashutosh-narkar · 2020-12-03T01:25:24Z

examples/grpc/docker-compose.yaml

+    command:
+      - run
+      - --server
+#      - --log-level=debug


Nit: remove

ashutosh-narkar · 2020-12-03T01:25:37Z

examples/grpc/docker-compose.yaml

+    volumes:
+      - ./envoy.yaml:/etc/envoy/envoy.yaml
+  opa-envoy:
+    image: openpolicyagent/opa:latest-istio


Nit: latest-envoy

internal/internal.go

srenatus · 2020-12-03T09:19:00Z

ℹ️ This would be the kind of (debug) log we'd get when a gRPC request was passed to opa-envoy-plugin that we couldn't make sense of given the provided descriptor set:

{"decision-id":"00347a74-7f36-4c21-bde0-23a8cdd3524e","err":"proto: not found","level":"debug","msg":"could not find service","time":"2020-12-03T09:17:09Z"}

srenatus · 2020-12-07T09:18:44Z

Looks like this has bitrot again. I'll rebase.

internal/internal_test.go

ashutosh-narkar

LGTM.

ashutosh-narkar · 2020-12-08T19:13:44Z

@srenatus can you please fix the conflict and then we can merge !

With the new functionality from envoy to "pack_as_bytes", opa can decode the raw body into the parsed body to make policy decisions. Co-authored-by: Stephan Renatus <stephan@styra.com> Signed-off-by: Pedro Ferreira <pnlferreira@gmail.com>

- only read one message, balk on compressed payload - move proto parsing into plugin initialization, adapt/add tests - use protobuf APIv2, replace jhump/protoreflect - don't error our on unknown service/method What shall we do if the gRPC service/method isn't in our protoset? We could fail, or we could skip unmarshaling the body (since we cannot). This commit makes the plugin skip unmarshaling the body, since it's then up to the policy code to determine if an unknown request payload is OK or not OK. - cover empty message - skip compressed payloads - deal with truncated gRPC bodies - examples/grpc: add testsrv Imported from https://github.com/fullstorydev/grpcui/tree/47d4d718028c86c/testing/cmd/testsvr The MIT License (MIT) Copyright (c) 2017 FullStory, Inc The only change is the listening address (127.0.0.1 -> 0.0.0.0), and adding a Dockerfile. - examples/grpc: add docker-compose setup with envoy, testsrv, opa-envoy - workflow: add Envoy e2e to post-merge workflow - workflow: combine post-merge and pull-request - add debug logs for gRPC payload processing Also slightly refactors the other callsites of logrus.Debug() -- the loglevel check was redundant. Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus · 2020-12-08T19:21:18Z

@ashutosh-narkar done! :)

srenatus · 2020-12-08T19:32:37Z

@pnnferreira Look at that! Finally 🎊 Thanks for all your help and effort getting this off the ground.

pnnferreira · 2020-12-09T14:11:28Z

Great news 😃 Thanks for all the help and for finishing this 👏

srenatus reviewed Oct 1, 2020

View reviewed changes

pnnferreira force-pushed the master branch from 359b8c0 to 3ba4c5c Compare October 1, 2020 16:40

srenatus reviewed Oct 8, 2020

View reviewed changes

internal/internal_test.go Outdated Show resolved Hide resolved

pnnferreira marked this pull request as ready for review October 8, 2020 12:57

ashutosh-narkar reviewed Oct 16, 2020

View reviewed changes

srenatus reviewed Oct 16, 2020

View reviewed changes

ashutosh-narkar mentioned this pull request Oct 16, 2020

Envoy Plugin: Migrate to xDS ext_auth v3 open-policy-agent/opa#2798

Closed

srenatus mentioned this pull request Oct 28, 2020

update deps #209

Merged

srenatus force-pushed the master branch from b8f3c59 to d7b6e59 Compare November 16, 2020 13:14

srenatus reviewed Nov 16, 2020

View reviewed changes

srenatus force-pushed the master branch from d7b6e59 to 4e39aa1 Compare November 16, 2020 13:59

srenatus mentioned this pull request Nov 16, 2020

Support gRPC body in OPA Istio plugin open-policy-agent/opa#1931

Closed

ashutosh-narkar marked this pull request as draft November 16, 2020 19:16

srenatus force-pushed the master branch from 70f2184 to f62da02 Compare November 17, 2020 12:58

srenatus force-pushed the master branch from 069dc11 to 9b27b24 Compare December 1, 2020 18:59

ashutosh-narkar reviewed Dec 2, 2020

View reviewed changes

srenatus force-pushed the master branch 3 times, most recently from 87fa3ac to 2ae78cd Compare December 2, 2020 10:57

srenatus marked this pull request as ready for review December 2, 2020 11:02

srenatus reviewed Dec 2, 2020

View reviewed changes

srenatus force-pushed the master branch from 7558e22 to 911a78f Compare December 2, 2020 11:09

ashutosh-narkar reviewed Dec 3, 2020

View reviewed changes

srenatus force-pushed the master branch from 1ad8f2b to 3d69a1e Compare December 3, 2020 09:19

srenatus force-pushed the master branch 2 times, most recently from 7d0c3dc to 2dcf056 Compare December 7, 2020 19:46

srenatus changed the title ~~Decode rawbody envoy~~ support gRPC payload decoding Dec 7, 2020

srenatus changed the title ~~support gRPC payload decoding~~ support basic gRPC payload decoding Dec 7, 2020

ashutosh-narkar reviewed Dec 8, 2020

View reviewed changes

internal/internal_test.go Outdated Show resolved Hide resolved

srenatus force-pushed the master branch from 2dcf056 to 7d228bf Compare December 8, 2020 08:23

ashutosh-narkar approved these changes Dec 8, 2020

View reviewed changes

pnnferreira and others added 3 commits December 8, 2020 20:17

feature: parse gRPC request payloads

b3d204d

With the new functionality from envoy to "pack_as_bytes", opa can decode the raw body into the parsed body to make policy decisions. Co-authored-by: Stephan Renatus <stephan@styra.com> Signed-off-by: Pedro Ferreira <pnlferreira@gmail.com>

deps: revendor

28498e2

Signed-off-by: Stephan Renatus <stephan.renatus@gmail.com>

srenatus force-pushed the master branch from 7d228bf to 28498e2 Compare December 8, 2020 19:19

srenatus mentioned this pull request Dec 8, 2020

workflow: use docker-env for building image (to ensure golang version) #223

Closed

ashutosh-narkar merged commit 948fe78 into open-policy-agent:master Dec 8, 2020

		@@ -0,0 +1,3 @@
		Folder: TestGrpc

		protoc --proto_path=TestGrpc --go_out=testGrpc --go_opt=paths=source_relative TestGrpc/Example.proto --descriptor_set_out=TestGrpc/example.pb --include_imports


		message := dynamic.NewMessage(msgDesc)

		if err := proto.Unmarshal(in, message); err != nil {

	if err := proto.Unmarshal(in, message); err != nil {
	if _, err := pbutil.ReadDelimited(bytes.NewReader(in), message); err != nil

support basic gRPC payload decoding #202

support basic gRPC payload decoding #202

Conversation

pnnferreira commented Oct 1, 2020 • edited Loading

srenatus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ashutosh-narkar commented Oct 1, 2020 • edited Loading

pnnferreira commented Oct 2, 2020

srenatus commented Oct 2, 2020

ashutosh-narkar commented Oct 2, 2020

pnnferreira commented Oct 8, 2020

pnnferreira commented Oct 12, 2020

ashutosh-narkar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Oct 16, 2020

srenatus commented Oct 29, 2020 • edited Loading

srenatus commented Nov 12, 2020

pnnferreira commented Nov 12, 2020

srenatus commented Nov 16, 2020

Choose a reason for hiding this comment

ashutosh-narkar commented Nov 16, 2020

srenatus commented Nov 17, 2020 • edited Loading

srenatus commented Nov 18, 2020

tsandall commented Nov 18, 2020

srenatus commented Nov 18, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Dec 2, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srenatus commented Dec 3, 2020

srenatus commented Dec 7, 2020

ashutosh-narkar left a comment

Choose a reason for hiding this comment

ashutosh-narkar commented Dec 8, 2020

srenatus commented Dec 8, 2020

srenatus commented Dec 8, 2020

pnnferreira commented Dec 9, 2020

pnnferreira commented Oct 1, 2020 •

edited

Loading

ashutosh-narkar commented Oct 1, 2020 •

edited

Loading

srenatus commented Oct 29, 2020 •

edited

Loading

srenatus commented Nov 17, 2020 •

edited

Loading