authz: RBAC HTTP filter + utilities#3455
Conversation
|
@rodaine can you take a look at CI issues then I will take a first pass? |
rodaine
force-pushed
the
rbac-filter
branch
2 times, most recently
from
38552be to
614d1dd
Compare
|
Ready to go, @mattklein123. |
There was a problem hiding this comment.
While I review this rest of this change, the indent here is funny, and I'm almost certain we used to run clang-format over the proto files which would have fixed this. I also would have thought that #3450 would have fixed not running clang-format, so I suspect there is something else going on here. Do you mind taking a quick look to see if you can figure out if/whether clang-format is running on these files? cc @htuch @alyssawilk
There was a problem hiding this comment.
Oh. hmm. I think the issue is because .proto is in the doc suffix list: c5d715a#diff-2e4b5720c93b5c27bb0c8c7c4312aaa8R17
I think we want to remove it from there and treat it as code?
There was a problem hiding this comment.
That'd be fine, but it requires some workarounds (e.g. to avoid the C++ namespace checks in .proto) which I didn't have time for. It shouldn't be a ton of work though.
There was a problem hiding this comment.
OK no problem I can fix this later today.
There was a problem hiding this comment.
An RBAC role is a set of permissions. Each permission defines an allowed action. So by definition, it is "OR" relationship between permissions. And within a single permission, the rules are "AND"ed together. Hence,
- We do not need top-level "or_rules" in a permission, because if you define "A OR B" within a permission, it is better to create two permissions "A" and "B".
- We do not have to specify "and_rules" explicitly because within a permission, the rules are always ANDed.
- I see that you have changed the semantics of "attribute-->a list of allowed values" to specific or_rules (attribute==value1 or attribute==value2). So the example you showed (removed top-level "and_rules" based on the above comment).
permissions:
- header: { name: ":method", exact_match: "GET" }
header: { name: ":path", regex_match: "/products(/.*)?" }
or_rules:
rules:
- destination_port: 80
- destination_port: 443
vs. attribute->a list of allowed values model.
permissions:
- header: { name: ":method", exact_match: "GET" }
header: { name: ":path", regex_match: "/products(/.*)?" }
destination_port: [80, 443]
Regarding 3, just from user experience perspective, I feel that the second (attribute->a list of allowed value) presentation is simpler. What do you think? But if it creates difficulty/confusion in implementation, I am fine either way.
Same comments apply to "principals". By definition, it is "OR" relationship between different principals. Within a single "principal", the rules are "AND"ed together. Hence, the three points I mentioned above apply to "principals" fields too.
There was a problem hiding this comment.
I believe it's been discussed before, but the ergonomics of the configs here are for the Envoy's implementation itself, not so much the user. User-friendlier structures can be easily morphed to this model.
As for why there is support for AND/OR similar to the original implementation of the auth protos, it's to make it very explicit of what the intent is of how the rules within a permission/principal apply. While I agree from a user's perspective that the top-level and_rules are redundant, it makes it clear how each sub-rule is evaluated and leaves nothing up to convention. The existence of the and/or_rules is not mutually exclusive with your approach of splitting out each individual permission/principal within a policy, but it does have the potential to eliminate an explosion of rules which could be more succinctly described with composed ANDs/ORs.
This form, IMO, strikes a balance between the original intent as you proposed, plus makes it easier and clearer how it should be interpreted by the engine, as well as provides flexibility for use cases that go beyond a typical RBAC design.
There was a problem hiding this comment.
Thanks for the explanation. Per our offline discussion, I am fine with the proto changes.
There was a problem hiding this comment.
If "any" is unset or set to false, and there is no permission defined, what is the behavior?
There was a problem hiding this comment.
The oneof has a required validation rule. You would not be able to emit a permission/principal without a rule set. This was intended to make the intent explicit (instead of by convention).
There was a problem hiding this comment.
Same question here. If "any" is unset or set to false, and no principal is defined, what is the behavior?
There was a problem hiding this comment.
"any" cannot be set to false, the validation rule precludes it.
There was a problem hiding this comment.
The paths, methods and conditions (destination_ports, destination_ips) in the old design are all repeated field, is this still supported in the new design?
Also the same to the header in principal.
There was a problem hiding this comment.
yes, just not as repeated fields to make the relationship explicit. It's certainly more verbose, but still supports all the same behaviors as the previous design while being more flexible.
There was a problem hiding this comment.
To confirm my understanding, the top level policy match is always OR?
and if the action is ALLOW but the request is not matched (which means all the policies are not matched), then the request will be rejected, and vice-versa?
Thanks!
There was a problem hiding this comment.
that's correct:
A request is authorized IFF:
- Action=ALLOWED && a policy matches
- Action=DENY && no policy matches
A policy matches IFF:
- One (or more) permission matches the action
- One (or more) principal matches the downstream
There was a problem hiding this comment.
where is the service field in the new design?
There was a problem hiding this comment.
it was removed because it's source is unclear from Envoy's perspective. To get the same effect as what was previously described in the comment, use a header matcher on the "x-envoy-downstream-cluster" header
|
Thanks for the change, Chris! |
|
Going to split out the proto changes into a separate PR to facilitate review and unblock @yangminzhu /Istio folks! |
|
@mattklein123 rebased with changes from #3477 |
There was a problem hiding this comment.
How would this support other authentication method, like JWT-based authentication?
@diemtvu
There was a problem hiding this comment.
currently it does not but we can extend this in a later PR to other mechanisms. I am already iffy on this existing behavior (which I ported from the ext authz filter), and it can most certainly be improved.
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Overall, really great work @rodaine. Well done. I will nominate you to be on the Go++ steering committee. 😉.
One substantial high level comment and some nits, once we sort out the high level comment I will look at the tests. Thank you!
There was a problem hiding this comment.
This comment is not going to make you happy, but I'm wondering what you think about maintaining the bifurcated variants of allowed/matches through this code. If you look at what we did in ratelimit, we basically use the same call points whether it's HTTP or not (similar for access logging). There are two ways to do this:
- Allow headers to be nullptr (switch to const raw pointer)
- Synthetically pass a static const empty http headers for calls from the eventual network filter. This makes the matching code even simpler since there needs to be no null checking anywhere in the engine.
I would vote that we get rid of the split functions everywhere, and do number 2 above. Given this PR just touches HTTP, it will always be populated with a real header block. The eventual network filter can just pass a static empty head block as I already mentioned. I think this will delete a bunch of code. Thoughts?
There was a problem hiding this comment.
Going with (2). Easy enough!
There was a problem hiding this comment.
nit: for function decls, spell out the parameter name (this will end up getting caught by clang-tidy on google import which we don't run in public CI). (Same elsewhere.)
There was a problem hiding this comment.
del empty destructors, same elsewhere.
There was a problem hiding this comment.
Please use the new FactoryBase plumbing (see other examples)
There was a problem hiding this comment.
"envoy.filters.http.rbac" (see note above)
There was a problem hiding this comment.
Per https://google.github.io/styleguide/cppguide.html#General_Naming_Rules
I'd be inclined to rename RBAC where possible with AccessControlEngine or RouteBasedAccessControlEngine. For folks not familiar with the acronym they're going to have to look it up where most folks reading "RouteBasedAccessControlEngine" will have a pretty good idea of what it does :-)
This is totally optional - given the config is rbac - you can totally argue that we leave this rbac for consistency :-)
There was a problem hiding this comment.
certainly don't mind making it clearer!
There was a problem hiding this comment.
Comments please. Also consider more verbose naming. engine_disabled_ vs allow_if_matched_?
There was a problem hiding this comment.
nit: hate to be that person, but can we change the language to be "safe list"/"block list" instead? Parathesis definitely help, but would like to change the language as well to be inclusive. thanks!
|
Comments addressed, @mattklein123 @alyssawilk @lita! |
|
@rodaine taking another pass now but FYI needs master merge. |
|
@rodaine also please add a release note also. Thank you! |
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
Great work, just some random comments, release notes, master merge, then I think ready to ship!
There was a problem hiding this comment.
You can delete this function, it's implemented by the base class.
test/config/utility.cc
Outdated
There was a problem hiding this comment.
nit: given this is only going to be used in an extension integration test, I would not add this to test/config/utility.cc. Just define in your extension integration test and use there.
There was a problem hiding this comment.
All of these tests should be defined using envoy_extension_cc_test(). I think I see why you didn't use it here, since there is no clear extension that owns it if it gets used from multiple filters. I would for now have it be owned by the HTTP rbac filter and put in a TODO That we might need to make a test owner a list of multiple owners so that if either the network or HTTP filter gets included we test this stuff.
There was a problem hiding this comment.
I was being consistent with the other libs in the common namespace. I'll make this change and add the TODO.
There was a problem hiding this comment.
Also adding a envoy_extension_cc_mock with similar mechanics.
There was a problem hiding this comment.
nit: for perf reasons constructor/destructor should go in cc file (see other examples, gmock docs).
There was a problem hiding this comment.
👍
(For any code archeologists in the future: https://github.com/google/googletest/blob/master/googlemock/docs/CookBook.md#making-the-compilation-faster)
There was a problem hiding this comment.
This indicates that RBAC filter is actually checking whether or not SSL connection is set up. It might be OK for now. In the long run, our preference is to leave the authentication check to a separate authentication filter. Also, once JWT authentication filter is present, just checking SSL connection won't be enough. I just want to leave some notes here for future consideration.
There was a problem hiding this comment.
Yeah we can totally iterate on this going forward, this just matches the existing behavior from ext_authz. I'd like to see the name field deprecated on the Authorization proto and replaced with specific sources for this data.
Signed-off-by: Chris Roche <croche@lyft.com>
Signed-off-by: Chris Roche <croche@lyft.com>
Signed-off-by: Chris Roche <croche@lyft.com>
Signed-off-by: Chris Roche <croche@lyft.com>
| return true; | ||
| } | ||
|
|
||
| std::string principal = ssl->uriSanPeerCertificate(); |
There was a problem hiding this comment.
For Istio, my understanding is local RBAC filter is going to chain with authN filter, which already fished out identiy for both mtls/JWT cases and set principle in request header; so here may need to use principle from header directly.
https://github.com/istio/proxy/blob/master/src/envoy/http/authn/filter_context.cc#L56
https://github.com/istio/proxy/blob/master/src/envoy/http/authn/http_filter.cc#L79
8 participants
This patch adds a new RBAC authz HTTP filter. The engine component is independent of the filter itself, so it can be shared with the network-level filter (coming in a future PR). This patch also reworks the proto configs for the RBAC rules a bit. The previous structure made it ambiguous if rules should be applied with a logical AND or OR. As well, rules of different types could not previously be mixed.
Risk Level: Medium (new filter + minor touchups to existing shared code)
Testing: unit + integration
Docs: updated with new landing page for the filter
Release Notes: updated
Signed-off-by: Chris Roche croche@lyft.com