Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
A41: xDS RBAC Support #237
A41: xDS RBAC Support #237
Changes from all commits
65ddc86
a9f7442
48c3a2c
203a547
f782dae
2a30ba6
0eb14ca
bdece2b
be39479
aa777e8
f3486d6
88f00d5
27ba988
2ab6d4a
6228f60
392960e
a9d5591
b9cbab4
08bed38
2a5b289
09a2cc8
7416b2e
f502d56
ace1abd
75c2c22
1469642
583ac3a
0736ffd
c5ee903
1bce180
e847336
ed508ce
17fdb3a
8b5d8a2
947d441
13ee759
5c0c73c
d207a54
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
andContent-type
, so they will no longer be visible by the time we get to the RBAC filter. CC @yashyktCan 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
:method
will be hard-coded toPOST
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In Go, we currently strip
:method
(we validate it isPOST
), butcontent-type
is passed through to the application.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 handlecontent-type
. We need to think about this some more.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense.
@yashykt Please note that this is something we'll need to handle in our implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@yashykt We'll need to make sure C-core is behaving correctly here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.