A41: xDS RBAC Support#237
Conversation
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks like a great start!
| `:scheme`, `:authority`, and`:path` matchers and they should match the values | ||
| received on-the-wire independent of whether they are stored in Metadata or in | ||
| separate APIs. `:scheme` is not universally available in gRPC APIs, so it may be | ||
| hard-coded to `http` if unavailable. `:method` can be hard-coded to `POST` if |
There was a problem hiding this comment.
As we've discussed, in C-core, we still have some vestigal support for GET and PUT. We should make sure no one is actually using that and then remove it if possible. If not, we'll have to find some way to make it work safely with this policy.
CC @yashykt
This now actually matches Envoy. It is unclear where the original definition was derived from.
|
Everyone, be aware that I made a significant change to principal_name upon noticing it was not Envoy behavior. The previous text was a significant departure from what the proto said, so I'm surprised somebody else didn't notice. |
|
Everyone (esp @yashykt, @YifeiZhuang, @zasweq), I further updated |
A36 had "if you don't use RouteConfiguration" language but didn't say what to do if "you do use RouteConfiguration" so was incomplete. Adding the language there is less confusing and easier to find than adding it to A41-xds-rbac
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
This looks like it's getting close!
| `:method`, `:scheme`, `:authority`, and`:path` matchers and they must match the | ||
| values received on-the-wire independent of whether they are stored in Metadata | ||
| or in separate APIs. It is unspecified whether hop-to-hop headers are matched. | ||
| The `header` field is not entirely 1:1 with gRPC Metadata, in part because which |
There was a problem hiding this comment.
Some of the behavior described here is going to be a bit of a challenge for C-core, since the xDS HTTP filters will sit higher in the stack than the http_server filter, which removes a lot of the fields that we want to see here. In particular, it removes :method and Content-type, so they will no longer be visible by the time we get to the RBAC filter. CC @yashykt
Can you remind me how Java and Go handle these headers? Do they simply get returned to the application? CC @dfawley
There was a problem hiding this comment.
:method will be hard-coded to POST in Java. Content-Type happens to still be present in Metadata in Java. Netty does validate it on a low-level, but doesn't remove it because 1) it doesn't remove any headers and 2) the higher levels would need it if we added support for the +proto/+json part.
There was a problem hiding this comment.
In Go, we currently strip :method (we validate it is POST), but content-type is passed through to the application.
There was a problem hiding this comment.
I think we've come up with a reasonable way to handle :method in C-core, but I'm not yet sure how we're going to handle content-type. We need to think about this some more.
| since gRPC generally doesn't observe this header yet the RBAC policy could. | ||
| Options: deny such requests, move Host header to :authority, drop Host header. | ||
| Host and :authority could also disagree, and that needs to be handled. | ||
| The core gRPC implementation (not just xDS or RBAC) must observe both :authority |
There was a problem hiding this comment.
@yashykt We'll need to make sure C-core is behaving correctly here.
|
@yashykt, @YifeiZhuang, @zasweq, the decision about how to handle broken RDS was made. We'll handle it at the L7 layer, which should be easy to implement. See the recent commit I pushed. We can ship without the drain grace time configuration option so let's focus on the other parts first, but we'll want to add it eventually. It should be really easy. |
markdroth
left a comment
markdroth
left a comment
There was a problem hiding this comment.
Overall, this looks very good! My comments are mainly wording nits.
| `:method`, `:scheme`, `:authority`, and`:path` matchers and they must match the | ||
| values received on-the-wire independent of whether they are stored in Metadata | ||
| or in separate APIs. It is unspecified whether hop-to-hop headers are matched. | ||
| The `header` field is not entirely 1:1 with gRPC Metadata, in part because which |
There was a problem hiding this comment.
I think we've come up with a reasonable way to handle :method in C-core, but I'm not yet sure how we're going to handle content-type. We need to think about this some more.
| the concatenation of the values along with a `,` (comma, no added spaces) | ||
| separator, as permitted by HTTP and gRPC. The `Content-Type` provided by the | ||
| client must be used; not a hard-coded value. Binary headers are represented in | ||
| their base64-encoded form, although we rarely expect binary header matchers |
There was a problem hiding this comment.
Hmmm... What if the C-core true-binary metadata extension is being used? In that case, the binary header will not actually be base64-encoded. Do we just match against the binary format in that case, or do we need to base64-encode it just to check the match?
There was a problem hiding this comment.
The semantics of binary headers in HTTP/2 is they are normal base64 when unsupported. So yes, you'd need to match against the base64-encoded version. We don't really think binary header matching will be common, so we should optimize the code for simplicity and not performance.
There was a problem hiding this comment.
Makes sense.
@yashykt Please note that this is something we'll need to handle in our implementation.
| There is no requirement that the RBAC engine have a specialized API; it could | ||
| simply be an interceptor. | ||
|
|
||
| The [RBAC policy][] has many fields. All current (Envoy-implemented) fields will |
There was a problem hiding this comment.
there is an ongoing work to support dynamic typed proto matcher in RBAC, it is currently used to support upstream IP/port but the general framework allows other usages in the future, not sure if this is something the gRPC RBAC wants to support, see more details in envoyproxy/envoy#17645
There was a problem hiding this comment.
Interesting. We may want a followup change that NACKs if the TypedExtensionConfig is set, simply because having ignored fields in matchers can cause havoc. But I don't think we have much of a need for that feature itself, at least yet.
RBAC here is server-side only, so "upstream IP/port" doesn't make sense. We could have client-side support, but it seems "upstream IP/port" has to be for a more conventional reverse proxy usecase instead of Envoy as a sidecar. I'd have to dig deeper to see if it can work load balancing, but it seems that specific extension is probably not too interesting to us.
|
|
||
| The `header` field is not entirely 1:1 with gRPC Metadata, in part because which | ||
| HTTP headers are present in Metadata is not 100% consistent cross-language. | ||
| For this design, `headers` can include `:method`, `:authority`, and `:path` |
There was a problem hiding this comment.
does the :path goes through the same URL normalization done by Envoy? There are quite some normalization (e.g. resolve dot, dot-dot, remove multiple-forward-slashes, decode some of the percent encoded characters, etc.).
Some of the normalization are always needed and some other are configurable, we have seen multiple CVEs due to the mishandling of the path normalization.
There was a problem hiding this comment.
No normalizations are done on the path. But no normalizations are done by the grpc service either. gRPC essentially just does string-equality to match RPC methods. (It is possible for applications to have some freedom here, but nobody really does differently.)
Envoy has a bigger problem here since the HTTP library processing the request in the application is a different one than in Envoy. That isn't really a problem in a proxyless environment, as long as we make sure RBAC runs with the same path that the service would see.
| names is used in the _RBAC policy_. | ||
|
|
||
| The core gRPC implementation (not just xDS or RBAC) must observe both :authority | ||
| and Host headers. If :authority is missing, Host must be renamed to :authority. |
There was a problem hiding this comment.
also note the match on host header should be case-insensitive, the match on other header should be case-sensitive.
There was a problem hiding this comment.
Isn't case sensitivity defined by the matcher (StringMatcher.ignore_case), not the implementation or specific header being matched? I'd agree if you are saying that it is probably a configuration mistake if a matcher for a host was case sensitive. But that doesn't seem part of this specification.
You aren't suggesting that Envoy hard-coded different case matching behavior based on the header name for things like HeaderMatcher.exact_match, right? I've honestly not looked much at that sort of detail for HeaderMatcher as we had pre-existing support because of Routes and most things but StringMatcher are deprecated.
There was a problem hiding this comment.
I don't see any case-insensitivity code in HeaderMatcher for the old-style matchers:
https://github.com/envoyproxy/envoy/blob/327149fdc141ac7e057fb2c37fcf5f2e32047475/source/common/http/header_utility.cc#L152
It really just seems like users should set StringMatcher.ignore_case=true for headers like :authority.
|
|
||
| The [RBAC policy][] has many fields. All current (Envoy-implemented) fields will | ||
| be considered in gRPC. However, some fields may have pre-determined values or | ||
| behavior. At this time, if the `RBAC.action` is `Action.LOG` then the policy |
There was a problem hiding this comment.
A general question, does the gRPC allows specifying multiple RBAC config? One common use case in Envoy is to deploy multiple RBAC filter with different actions, for example, the 1st RBAC config with action DENY and the 2nd RBAC filter with action ALLOW.
There was a problem hiding this comment.
Yes. We allow multiple filters and filters can be repeated, just like in Envoy. That is covered in https://github.com/grpc/proposal/blob/master/A39-xds-http-filters.md
| [RBAC policy]: https://github.com/envoyproxy/envoy/blob/10c17a7cd90b013c38dfbfbf715d3c24fdd0477c/api/envoy/config/rbac/v3/rbac.proto | ||
| [RBAC network filter]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/network/rbac/v3/rbac.proto | ||
| [envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto | ||
|
|
There was a problem hiding this comment.
Another general question, does gRPC support RBAC in shadow mode? which is to evaluate the rules and record the result but do not enforce it. This is useful in trying out a RBAC config in production without rejecting any actual requests.
See api/envoy/extensions/filters/http/rbac/v3/rbac.proto
There was a problem hiding this comment.
No, not as part of this. I've made a change to mention only RBAC.rules is supported in the RBAC message.
We are aware of it and agree it is really helpful. But it'll need some stats plumbing that is missing.
| [RBAC network filter]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/network/rbac/v3/rbac.proto | ||
| [envoy.extensions.filters.listener.proxy_protocol.v3.ProxyProtocol]: https://github.com/envoyproxy/envoy/blob/main/api/envoy/extensions/filters/listener/proxy_protocol/v3/proxy_protocol.proto | ||
|
|
||
| ### Temporary environment variable protection |
There was a problem hiding this comment.
Another general question, does the gRPC RBAC emit the same stats for the evaluation result (e.g. allowed, denied number of requests), those are useful stats for quickly checking the status of the RBAC filter in Envoy.
There was a problem hiding this comment.
There are no stats as part of this. There have been some other discussions about stats (outside of RBAC) as stats are getting fleshed out. But as of yet, filters aren't contributing to stats. gRPC stats will not be the same as Envoy stats, but we'll obviously be aware of the Envoy stats and want similar stats for the really helpful cases.
These were forgotten before merging grpc#237.
These were forgotten before merging #237.
9 participants
I think it is far enough along to be useful at least to understand which pieces are getting solid and what is up-in-the-air. But I'm not yet ready to elicit reviews and I'm also not sending out a notification to grpc-io yet. But feel free to add comments.
This does differ from the internal C design slightly (although the more I consider things, the more extra cases I'm finding), like with remote_ip. And I'm noticing some interesting details on what might be involved to keep fields unsupported (like with the MetadataMatcher). I hope we can avoid processing MetadataMatcher, but it'll need more investigation.
CC @markdroth, @dfawley, @yashykt, @YifeiZhuang, @zasweq