ext_authz: Use 403 as default for denied response#18010
Merged
mattklein123 merged 6 commits intoenvoyproxy:mainfrom Sep 13, 2021
dio:fix-ext_authz-unknown-denied-response-status
Merged
ext_authz: Use 403 as default for denied response#18010mattklein123 merged 6 commits intoenvoyproxy:mainfrom dio:fix-ext_authz-unknown-denied-response-status
mattklein123 merged 6 commits intoenvoyproxy:mainfrom
dio:fix-ext_authz-unknown-denied-response-status
Conversation
Before this, when a gRPC server sends out DeniedResponse as a check response for a request but without setting the HttpResponse.DeniedResponse.Status, HTTP ext_authz filter translates that as "0" (empty/unknown HTTP status code). This patch makes sure we reply with a valid 403 Forbidden HTTP status code (the current default status code for denied response). Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
Member
Author
|
/assign @esmet |
yanavlasov
reviewed
Sep 7, 2021
Contributor
yanavlasov
left a comment
yanavlasov
left a comment
There was a problem hiding this comment.
Should we make the status to not be required with this change?
/wait
Contributor
|
Lets update the comment on status to make it extra clear that the default is 403. The language only sort of suggests this. |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Contributor
|
/lgtm api |
yanavlasov
previously approved these changes
Sep 8, 2021
Member
Author
|
Thanks, @yanavlasov, @esmet, and @markdroth for the review! |
Contributor
|
Looks good, and I think it needs main merged in to fix the linux_x64 api_compat job. |
…wn-denied-response-status
Member
Author
|
cc. @envoyproxy/senior-maintainers if you want to take a look. Thanks! |
mattklein123
requested changes
Sep 10, 2021
Member
mattklein123
left a comment
mattklein123
left a comment
There was a problem hiding this comment.
LGTM with small doc nit, thanks.
/wait
| * dynamic forward proxy: fixing a validation bug where san and sni checks were not applied setting :ref:`http_protocol_options <envoy_v3_api_msg_extensions.upstreams.http.v3.HttpProtocolOptions>` via :ref:`typed_extension_protocol_options <envoy_v3_api_field_config.cluster.v3.Cluster.typed_extension_protocol_options>`. | ||
| * ext_authz: fix the ext_authz filter to correctly merge multiple same headers using the ',' as separator in the check request to the external authorization service. | ||
| * ext_authz: the network ext_authz filter now correctly sets dynamic metdata returned by the authorization service for non-OK responses. This behavior now matches the http ext_authz filter. | ||
| * ext_authz: fix the HTTP ext_authz filter to response with ``403 Forbidden`` when a gRPC auth server sends a denied check respond with empty HTTP status code. |
Member
There was a problem hiding this comment.
Suggested change
| * ext_authz: fix the HTTP ext_authz filter to response with ``403 Forbidden`` when a gRPC auth server sends a denied check respond with empty HTTP status code. | |
| * ext_authz: fix the HTTP ext_authz filter to respond with ``403 Forbidden`` when a gRPC auth server sends a denied check response with an empty HTTP status code. |
Signed-off-by: Dhi Aurrahman <dio@rockybars.com>
mattklein123
approved these changes
Sep 13, 2021
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Commit Message: Before this, when a gRPC server sends out
DeniedResponseas a check response for a request but without setting theHttpResponse.DeniedResponse.Status, HTTPext_authzfilter translates that as0(empty/unknown HTTP status code). This patch makes sure we reply with a valid403 ForbiddenHTTP status code (the current default status code for denied response).Additional Description: When building a minimal gRPC auth server I found that it is a bit confusing for
CheckResponse_DeniedResponsewhen you have already setCheckResponse.Statuswithcode.Code_PERMISSION_DENIEDbut then you addedCheckResponse.HttpResponse. In the current code, it is required to setCheckResponse.HttpResponse.Status, if not it will be sending Unknown (0) status code.envoy/api/envoy/service/auth/v3/external_auth.proto
Lines 49 to 51 in cc1d41e
The above docs excerpt suggests that if we skip on specifying it, it will be 403.
Risk Level: Low
Testing: Added
Docs Changes: N/A
Release Notes: Added
Platform Specific Features: N/A
Signed-off-by: Dhi Aurrahman dio@rockybars.com