ext_authz: add grpc_service field on the per-route filter#40169
ext_authz: add grpc_service field on the per-route filter#40169agrawroh merged 17 commits intoenvoyproxy:mainfrom
Conversation
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
0f6aadb to
cad4ca6
Compare
|
/retest |
1aa7326 to
c9b2026
Compare
mathetake
left a comment
There was a problem hiding this comment.
Generally I agree this would be useful, but I wonder if this would come with a security implication. If the route-recalculating filter comes after this per-route ext_authz, then it can somehow bypass the authz for a specific route. For example, let's say we have two routes A and B and each one has its own authz server. When a request comes in, say, route A got selected. Then ext_authz filter does the auth logic for that route A. Then some filter after authz change clears the route cache and then changes the destination route to route B. That means the request bypasses the auth for route B by using auth for route A.
I am not saying that this would be common thing but i guess at least we should document this implication?
Thanks, @mathetake. It's a great point and I agree that we should document it. The scenario you described also exists today as technically an incoming request can match on a Route A with per-route ExtAuthZ override to disable the filter, clear the route cache using LUA in a subsequent filter and make it match on a Route B which requires ExtAuthZ. Same risk of privilege escalation. |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
c9b2026 to
465e6a6
Compare
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for this new feature. One comment to start the review.
|
|
||
| // Set a different gRPC service for this route than the default. | ||
| // This allows different routes to use different external authorization service backends. | ||
| // | ||
| // .. note:: | ||
| // | ||
| // This setting is only applied to a filter configured with a | ||
| // :ref:`grpc_service<envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>`. | ||
| // If the filter is configured with an ``http_service``, this field is ignored. | ||
| config.core.v3.GrpcService grpc_service = 4; |
There was a problem hiding this comment.
Could we support http_service override here also? Thanks.
There was a problem hiding this comment.
@wbpcode Yes, i think we can support HTTP as well. We internally use gRPC and have this use-case so I started with gRPC first. I can create a follow-up for HTTP next if that sounds good.
There was a problem hiding this comment.
I think the behavior may be unexpected? From you comment of API, the grpc_service will only override the filter level grpc_service. But we may expect it override any http_service or http_service?
|
/wait |
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the contribution and I add some comments to both API and the impl. :)
|
|
||
| // Set a different gRPC service for this route than the default. | ||
| // This allows different routes to use different external authorization service backends. | ||
| // | ||
| // .. note:: | ||
| // | ||
| // This setting is only applied to a filter configured with a | ||
| // :ref:`grpc_service<envoy_v3_api_field_extensions.filters.http.ext_authz.v3.ExtAuthz.grpc_service>`. | ||
| // If the filter is configured with an ``http_service``, this field is ignored. | ||
| config.core.v3.GrpcService grpc_service = 4; |
There was a problem hiding this comment.
I think the behavior may be unexpected? From you comment of API, the grpc_service will only override the filter level grpc_service. But we may expect it override any http_service or http_service?
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for your update. And some comments are added.
/lgtm api
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
wbpcode
left a comment
There was a problem hiding this comment.
Thanks for the update. Overall is fine. I actually have a thought, but up to you. May be we cann create a function to load the configuration (grpc_service, http_service, encode_raw_headers) then return a lambda. This lambda will create a client instance when it's executed. Then, the config and per route config could share the tool function.
That would make our code much simple to read and may have better performance because avoiding unnecessary copies (although doesn't matter because the network consume the largest time)
| // Create a temporary ExtAuthz config with the HTTP service for the ClientConfig constructor. | ||
| envoy::extensions::filters::http::ext_authz::v3::ExtAuthz temp_config; | ||
| temp_config.mutable_http_service()->CopyFrom(http_service); |
There was a problem hiding this comment.
I think maybe we should update the constructor of ClientConfig to accept HttpService only to avoid the this copy. But it's not in a hurry. The auth is slow anyway.
|
/wait |
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
|
The failing test is |
…voyproxy#40169)" This reverts commit 8c03b2a.
…voyproxy#40169)" This reverts commit 8c03b2a. Signed-off-by: Ryan Northey <ryan@synca.io>
…#40169) ## Description This PR adds support for per-route gRPC service override in the `ext_authz` HTTP filter, allowing different routes to use different external authorization backends. Routes would now be able to specify a different authorization service by configuring `grpc_service` in the per-route `check_settings`. --- Commit Message: ext_authz: add grpc_service field on the per-route filter Additional Description: Add a new `grpc_service` field on the per-route ExtAuthZ filter to be able to override the AuthService backend on a per-route basis. Risk Level: Low Testing: Added Unit & Integration Tests Docs Changes: Added Release Notes: Added --------- Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com> Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
…#40169) ## Description This PR adds support for per-route gRPC service override in the `ext_authz` HTTP filter, allowing different routes to use different external authorization backends. Routes would now be able to specify a different authorization service by configuring `grpc_service` in the per-route `check_settings`. --- Commit Message: ext_authz: add grpc_service field on the per-route filter Additional Description: Add a new `grpc_service` field on the per-route ExtAuthZ filter to be able to override the AuthService backend on a per-route basis. Risk Level: Low Testing: Added Unit & Integration Tests Docs Changes: Added Release Notes: Added --------- Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com> Signed-off-by: Melissa Ginaldi <mginaldi@google.com>
…#40169) ## Description This PR adds support for per-route gRPC service override in the `ext_authz` HTTP filter, allowing different routes to use different external authorization backends. Routes would now be able to specify a different authorization service by configuring `grpc_service` in the per-route `check_settings`. --- Commit Message: ext_authz: add grpc_service field on the per-route filter Additional Description: Add a new `grpc_service` field on the per-route ExtAuthZ filter to be able to override the AuthService backend on a per-route basis. Risk Level: Low Testing: Added Unit & Integration Tests Docs Changes: Added Release Notes: Added --------- Signed-off-by: Rohit Agrawal <rohit.agrawal@databricks.com>
Description
This PR adds support for per-route gRPC service override in the
ext_authzHTTP filter, allowing different routes to use different external authorization backends. Routes would now be able to specify a different authorization service by configuringgrpc_servicein the per-routecheck_settings.Commit Message: ext_authz: add grpc_service field on the per-route filter
Additional Description: Add a new
grpc_servicefield on the per-route ExtAuthZ filter to be able to override the AuthService backend on a per-route basis.Risk Level: Low
Testing: Added Unit & Integration Tests
Docs Changes: Added
Release Notes: Added