Replace Envoy RBAC's CEL AST with human-readable expression

[TristonianJones]

Title: Replace CEL AST with human-readable expression

Description: The Envoy RBAC API exposes the google/api/expr/v1alpha1 abstract syntax tree for a parsed CEL expression, but if this AST were replaced with a human-readable CEL expression string this would provide key benefits for the API:

• Improved readability and diffing of expressions in config files.
• Greater flexibility in optimizations and alternative representations of CEL ASTs.
• Improved signals of correctness.

Since the creation of the API, a CEL C++ parser has been created / fuzzed / hardened, and could easily be incorporated into the Envoy runtime. Over time, if/when a CEL C++ type-checker is created, the signals of correctness could be further improved.

As of this moment, it is easy to construct ASTs by hand which might be valid CEL programs. Given the sensitivity of using CEL in a security context, restricting ASTs to only those which are at least syntactically valid would be a worth-while security improvement as well.

The only downside of such a change is the CEL expressions would need to be parsed which would add some additional overhead to updating the configuration of an Envoy instance; however, since configuration changes are likely infrequent, the impact of the added parse overhead would likely be nominal.

Relevant Links: https://github.com/google/cel-cpp/tree/master/parser

[TristonianJones]

@htuch @markdroth @kyessenov I think this is strongly worth considering given that it will reduce the API surface complexity, improve the correctness of configurations, and allow us to make improvements in expression representation (performance) over time.

[kyessenov]

Text representation has been exposed in rate limiting descriptor: https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/rate_limit_descriptors/expr/v3/expr.proto. It is a oneof between text and parsed. In RBAC, the field condition is taken to be the parsed version. Is there any preference how to name the field holding the text condition?

[yangminzhu]

+1, I think this will make it much easier to use the CEL expression in RBAC.

The checked_condition was added sometime ago to replace the condition, is it still going to be implemented (e.g. as one_of) or replaced by the text expression proposed in this issue? cc @jiangtaoli2016

And I think malformed expression is more likely to happen when users are directly writing the text expression, in this case, the RBAC filter should fallback to a deny-all-traffic mode until the expression is fixed by the user (I think we currently do not have this but it's also less likely to have malformed expression in the AST form).

[jiangtaoli2016]

It makes sense to keep both checked_condition and text expression, and removed condition (AST). User can use checked_condition for better efficiency.

[TristonianJones]

The are a few concerns with keeping the CheckedExpr visible on the API:

1. There are no guarantees that the CheckedExpr is actually a valid AST. I can construct invalid ones by hand.
2. We have started making AST optimizations which means semantically equivalent CheckedExpr values may not be protobuf equivalent.
3. The textual expression will reap the benefit of software upgrades to Envoy for the CEL parser (and eventually type-checker) whereas the AST form will not.

Since there is not yet a type-checker for CEL in C++, perhaps it makes sense to leave the CheckedExpr, but I'm not sure it's a good API choice.

[htuch]

@TristonianJones having text CEL makes sense to me as an option. We've discussed before the idea of separating out type checking and parsing in a config pipeline from the Envoy-side work as an architectural feature. https://github.com/envoyproxy/envoy/issues/15631#issuecomment-806151392 seems to indicate the thinking here might be changing?

[github-actions[bot]]

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.

[github-actions[bot]]

This issue has been automatically closed because it has not had activity in the last 37 days. If this issue is still valid, please ping a maintainer and ask them to label it as "help wanted" or "no stalebot". Thank you for your contributions.