envoyproxy / envoy

Cloud-native high-performance edge/middle/service proxy
https://www.envoyproxy.io
Apache License 2.0
24.74k stars 4.75k forks source link

Add a new ABAC filter #18064

Open wjtracey opened 3 years ago

wjtracey commented 3 years ago

Title: Add a new ABAC filter.

Proposal:

Goals:

Alternatives considered:

htuch@ is the maintainer sponsor of this work.

htuch commented 3 years ago

FWIW I'm strongly in favor of either an ABAC filter or a radical revisting of the RBAC filter.

I think the existing RBAC filter is not super well suited to the kinds of advanced ABAC cases above, and we have the ongoing issue of the RBAC filter having two languages (CEL and proto3) which don't fully track each other; it's already an overloaded filter which probably doesn't need more complexity.

This is a feature that would be fully invested in from the Google side as a core Envoy extension.

When it comes to chaining and conditional filter execution, we can possibly leverage generic matchers @snowp.

Somewhat related discussion in the earlier RBAC-CEL issue https://github.com/envoyproxy/envoy/issues/6751. It's clear that if you look at it from the right direction, the existing RBAC CEL support is a form of ABAC, but significantly impoverished as per OP.

CC @lizan @mattklein123 @yangminzhu @kyessenov @ramaraochavali

moderation commented 3 years ago

https://tailscale.com/blog/rbac-like-it-was-meant-to-be/ is a really good read. This sent me down the rabbit hole of RBAC, ABAC, Google's Zanzibar etc. I collected some links at https://pinboard.in/u:moderation/t:rbac. My concern about CEL is that it doesn't appear to have generated much outside Google community. Firebase still seems like the main usage.

Open Policy Agent's Rego and Oso's Polar are both derivations of Datalog and the primary complaint seems to be learning curve. There are some project that wrap OPA and use YAML. YAML is terrible but using it to define ABAC rules seems somehow even worse (WSJ: Bank exposes customer data due to indentation error in ABAC policy that uses YAML). Oso has a Rust library. Casbin has a C++ implementation

yangminzhu commented 3 years ago

I think there are two things in the proposal:

1) Make the Envoy access control feature more flexible and expressive to support all the different things as you mentioned;

2) Have a separate ABAC filter to implement it instead of changing the current RBAC filter;

For 1, I agree, it's good to have more capability as long as it's useful and really needed by users.

For 2, I think Envoy should have a single access control filter that is probably neither ABAC nor RBAC. It should just be a general access control (or authorization) filter that is flexible enough to be used as either ABAC-style or RBAC-style by the control plane.

The ABAC, RBAC (or any other) access control style makes more sense in the control plane API when used directly by end users, but not so much in the data plane API that is mainly consumed by control plane and usually translated from a different higher-level user-facing API.

For example, Today's Istio authorization policy is not RBAC at all but is translated and supported by the Envoy RBAC filter. If you look at the early history of the RBAC filter, it was originally introduced to support the Istio RBAC policy (already deprecated). I think the lesson here is that Envoy should just have a general access control (authorization) filter and let the control plane to decide how to use it.

Assuming we really need the new filter, I suggest we add it as a general access control (or authorization) filter that is able to support all the current use cases of RBAC filter, and then deprecate the RBAC filter, the control plane can migrate to use the new filter smoothly.

In the long term, there should be only one access control filter in Envoy that needs to be supported and maintained. A single filter reduces the maintenance, vulnerability handling, documentation and extensibility (new feature) overhead and improves the user experience a lot.

htuch commented 3 years ago

I'm in agreement here, but I see a long tail of RBAC users who probably will not want to move. One of the biggest mistakes we have in RBAC is it's actually not a single filter and it already has the maintenance, documentation, vulnerability management and extensibility issues you point to @yangminzhu. We have two parallel filters with independent languages squashed together, one providing a CEL ABAC-like filter and another providing a more limited RBAC.

I think it would be 100% doable to move all the CEL use cases from the existing RBAC to an ABAC filter, which can be the canonical "access control filter" and then leave RBAC for those folks who want the non-CEL experience, deprecating CEL there. It would be strictly no worse than what exists today, since we do have two filters, which are "in name only" a single filter.

If you want a good example of this, see https://github.com/envoyproxy/envoy/pull/17645. We basically are building a non-CEL only extensibility mechanism. This seems really confusing and will lead to further divergence of CEL and non-CEL use cases, in particular when we also end up with https://github.com/envoyproxy/envoy/issues/18063.

ejona86 commented 3 years ago

I don't know why "ABAC" is being mentioned; I don't see anything ABAC here. The larger piece seems a rework of the rule structure to allow greater mixing of ALLOW/DENY. And then there is the briefly-mentioned by seemingly very important setMetadata.

At the very least, can we not call it ABAC? RBAC wasn't role-based (it was subject-based; there is no mapping from subject to role nor request to needed permissions). And it seems like ABAC isn't attribute-based (attributes are not attached to subjects/objects/etc in some DB and then the policy processes attributes; Envoy attributes/metadata != ABAC).

Envoy is just implementing an authz engine, whereas RBAC, ABAC, MAC, and the other acronyms are describing systems. For example, a subject Ejona is given the role Administrator and a resource has the ACL that Administrator has Modify permission. Envoy's authz rules are not structured anything close to that. But you can take an RBAC system and produce an authz policy that Envoy can use, by expanding out all the subjects that have a role and such.

With ABAC, the company creates a filter chain of ... -> ABAC0 -> ExpensiveCustomFilter -> ABAC1 ->... where ABAC0 and ABAC1 use the same ruleset.

The dance to get attributeObtainedFromExpensiveCustomFilterLogic working right seems brittle. You have to get two policies and the custom filter to all agree. If they don't, you have a security vulnerability.

ABAC0 and ABAC1 will not have the same ruleset. Or if they do, it is dangerous. ABAC0 needs to ALLOW attributeObtainedFromExpensiveCustomFilterLogic to be missing. But ABAC1 should require it.

~ExpensiveCustomFilter will need to have a copy of the ABAC logic in order to determine whether attributeObtainedFromExpensiveCustomFilterLogic is necessary, in this case identity == 'i2' since that happens to be in the ABAC rules. It isn't as simple as "load attributeObtainedFromExpensiveCustomFilterLogic if identity == i2" in a sort of static way since that approach could be done unconditionally before ABAC0 even runs which is possible today. You could structure something here with Envoy metadata where the rule sets needs_attributeObtainedFromExpensiveCustomFilterLogic to be seen by the filter, but it seems like a house of cards.~ Edit: I think I see now how the original proposal was handling this in the same way I suggested below. I was thrown off by setMetadata. I think I see a way to define setMetadata to give ABAC-style processing, but I need to make some assumptions.


I propose an alternative that still is along the lines of this proposal: the authz policy can be in a special mode that runs and makes a list of all attributes accessed that were not present. When in that mode, the policy would always allow (there are other options here, but they require things like ternary logic). Then filters run and can populate any "unknown" attributes by name. And then a second authz filter runs with the same authz policy, except in "normal" mode.

Note that these rough approaches are not well-suited to short-circuiting. If you have attributeObtainedFromExpensiveCustomFilterLogic == 'foo' || otherExpensiveAttribute == 'bar' both expensive attributes will be calculated. You could process the authz policy 3, 4, or 5 items to avoid loading the expensive attributes, but obviously that scales poorly. For precise short-circuiting, I believe the authz filter must call out directly to other code.

htuch commented 3 years ago

I think we're less interested in arguing semantics; we can go with whatever name is preferred, but clearly this is intended to support ABAC (and in general, policies that are CEL based are generally supporting some form of ABAC).

Regarding short-cut logic, this is a really good point, we have some scope to work with the design here, and ultimately I think we will need to put together a design document with alternatives considered. It's possible to nest Envoy filters when carrying out match decisions, we have some examples of this today such as the generic matchers that @snowp developed for https://github.com/envoyproxy/envoy/issues/12968.

I think if we can get in-principle agreement on the right way to think about extending the existing RBAC filter vs. writing a new filter, we can move to a design doc for issues like this.

htuch commented 3 years ago

Was just chatting with @mattklein123 and he proposed an alternative, similar to the routing generic matchers, that we think might be able to address concerns raised by folks while preserving @wjtracey original proposal. Essentially we do:

  1. Create a new Access Control filter (as per @yangminzhu). This is neutral to RBAC/ABAC terminology as per @ejona86 and @moderation considerations.
  2. The rule/priority logic proposed by @wjtracey is the basis for this engine. A series of prioritized rules with (match, action) structure. Support for lazy and short circuit evaluation is included, as is support to be able to pause evaluation, defer to other filters/extensions for expensive attributes that require off-proxy compute.
  3. The matchers are themselves extension points (similar to @snowp generic matchers). So, we can have CEL as one pluggable matcher, a syntax similar to existing RBAC as an alternative, OPA syntax as another etc.

This would move us to a place where we could sustainably manage multiple match languages via extensions, we would remove any strong claims about RBAC/ABAC that are confusing, and still support the original goal of prioritized (match, action) rules with lazy CEL attribute evaluation.

WDYT?

mattklein123 commented 3 years ago

+1 to @htuch's summary of my idea. :)

ejona86 commented 3 years ago

I think I figured out how SetMetadata was intended to work, so ABAC is more a fitting name than I thought earlier. But still, +1 to "access control" or a similar generic/fuzzy term since it is really the policy that is ABAC. Pausing evaluation sounds like a fine approach.

I can't say I really understand the point of making the languages extension points, but sure, that's fine. The language is a sizable percentage of the total filter, so I don't see the advantage of language extension over a separate, e.g., OpaFilter. An extension mechanism does allow mix-n-match of languages, which could be useful during a migration or such. An extension point would make it harder to reuse "context" objects across rules (those objects that convert from Envoy/misc internals to the language's internals), but the impact of that will probably vary per-implementation.

geokb commented 3 years ago

The current use of CEL inside the conditions field makes the RBAC filter technically ABAC imo. While I appreciate the authors' seeking to improve the RBAC filter into a more generalized context-aware policy engine point that can evaluate OPA, CEL, etc... I personally think for any non-trivial application it would make much more sense to just use the ext-authz to do complex policy engine evaluation. Why? because the most crucial point imo here is that most non-trivial systems (I am working on one myself that I hope I can open source within a few weeks) use much more system-specific information that's fed as input to the policy engine beyond of just what the Envoy instance is aware of (i.e. Envoy's metadata and the request attributes). Unless Envoy adds its own Redis-like store that can be controlled in a xDS-like fashion where the stored data is used as input to the policy engine along with the metadata and request attributes, I think it would be better to keeping focusing more on the improvement of Envoy's extensibility (e.g. ext_authz, wasm, ext_proc, etc...).

github-actions[bot] commented 2 years ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days. It will be closed in the next 7 days unless it is tagged "help wanted" or "no stalebot" or other activity occurs. Thank you for your contributions.