rbac: Add support for specifying a HTTP status code for RBAC denial responses #34126
Conversation
…esponses Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
|
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
|
Rationale for this change is documented in the associated issue. |
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
Signed-off-by: Jacob Neil Taylor <jacobneiltaylor@canva.com>
|
Ping @markdroth for api shepherding |
|
/lgtm api |
|
Does this need to be assigned to @yanavlasov ? |
| if (deny_status == 0) { | ||
| return Http::Code::Forbidden; | ||
| } | ||
| return static_cast<Http::Code>(deny_status); |
There was a problem hiding this comment.
This is undefined behavior since you're casting from uint32_t to enum class over uint16_t. Maybe use
envoy/api/envoy/type/v3/http_status.proto
Line 138 in e543e1c
| @@ -65,6 +72,7 @@ RoleBasedAccessControlFilterConfig::RoleBasedAccessControlFilterConfig( | |||
| proto_config.shadow_rules_stat_prefix(), scope)), | |||
| shadow_rules_stat_prefix_(proto_config.shadow_rules_stat_prefix()), | |||
| per_rule_stats_(proto_config.track_per_rule_stats()), | |||
| deny_status_(getDenyStatus(proto_config.deny_status())), | |||
There was a problem hiding this comment.
Maybe explicitly do a static cast to uint16_t for the argument? I think the overflow never really happens but the sanitizers will probably complain if you implicitly convert values.
Commit Message: "rbac: Add support for specifying a HTTP status code for RBAC denial responses"
Additional Description: Added
deny_statusfield toextensions.filters.http.rbac.v3.RBACto allow the HTTP status code for the RBAC denial response to be configured.Risk Level: low
Testing: Added unit and integration tests
Docs Changes: Updated comments in proto file.
Release Notes: Added :ref:
deny_status <envoy_v3_api_msg_extensions.filters.http.rbac.v3.RBAC.deny_status>to allow the HTTP status code for the RBAC denial response to be configured.Platform Specific Features: N/A
Fixes: #34127