-
Notifications
You must be signed in to change notification settings - Fork 4.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
basic_auth: add support for per-route filter #33335
Conversation
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to |
// Disable the basic auth filter for this particular vhost or route. | ||
// If disabled is specified in multiple per-filter-configs, the most specific one will be used. | ||
bool disabled = 1 [(validate.rules).bool.const = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Prefer the general way to disable a filter in specific route or vh, see https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/http/http_filters#route-based-filter-chain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My implementation follows the approach with disabled
field, because BasicAuth is enabled on all routes by default. What do you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can check the doc I posted above. It have provided example about how to disable a filter on any route or vh. It a general feature that Envoy provided and should be used preferably.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I meant that I read the doc, but I don't understand what you mean, because another option is to make filter disabled on routes by default and enable it selectively, but this approach would break existing configs. "Prefer the general way" - I did not find term "general" in that doc, so I don't know what you mean.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is exactly what I implemented.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a general feature of Envoy to disable a filter in specific route. And all filters could use it directly. The config envoy.config.route.v3.FilterConfig
is a common wrapper that will be handled with special logic.
You needn't to add a new disabled
field in your filter's config.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay... Now I understand it and will remove this field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Check this doc, you can get more detail about the wrapper and the only one drawback of this mechanism https://www.envoyproxy.io/docs/envoy/latest/api-v3/config/route/v3/route_components.proto#config-route-v3-filterconfig
cc @zhaohuabing as code owner |
eaddeb3
to
7449b1b
Compare
bool disabled = 1 [(validate.rules).bool.const = true]; | ||
|
||
// Username-password pairs for this route. | ||
config.core.v3.DataSource users = 2 [(validate.rules).message.required = true, (udpa.annotations.sensitive) = true]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I will have to add a wrapper struct for users
and forward_username_header
to make these fields mutually exclusive with disabled
, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do you think that forward_username_header
should be supported by per-route filter? Or should it be general setting for the filter?
"@type": type.googleapis.com/envoy.extensions.filters.http.basic_auth.v3.BasicAuthPerRoute | ||
users: | ||
inline_string: |- | ||
admin:{SHA}0DPiKuNIrrVmD8IUCuw1hQxNqZc= |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: could we use the same masked password as in the previous example?
admin:{SHA}0DPiKuNIrrVmD8IUCuw1hQxNqZc= | |
admin:{SHA}hashed_admin_password | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
This looks good. Just a tiny nit on the doc. |
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
faca8b5
to
b302d4b
Compare
I'm sorry for the force-push, but a conflict in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you contribution and some comments are added.
/wait
changelogs/current.yaml
Outdated
@@ -463,6 +463,9 @@ new_features: | |||
Added maximum gRPC message size that is allowed to be received in Envoy gRPC. If a message over this limit is received, | |||
the gRPC stream is terminated with the RESOURCE_EXHAUSTED error. This limit is applied to individual messages in the | |||
streaming response and not the total size of streaming response. Defaults to 0, which means unlimited. | |||
- area: filters | |||
change: | | |||
Added :ref:`the Basic Auth per-route filter <envoy_v3_api_msg_extensions.filters.http.basic_auth.v3.BasicAuthPerRoute>`. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added :ref:`per-route configuration support to the Basic Auth filter <envoy_v3_api_msg_extensions.filters.http.basic_auth.v3.BasicAuthPerRoute>`.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
initialize(); | ||
} | ||
|
||
void initializePerRouteFilter(std::string yaml_config) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: const std::string& yaml_config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done
auto users = config_->users(); | ||
if (route_specific_settings != nullptr) { | ||
users = route_specific_settings->users(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think a reference wrapper or something should be used here to avoid to copy the whole user map. It's too expensive for a single request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I missed that due to auto
and I didn't notice that it's not UserMap&
as the method return type. I changed users
to be a pointer.
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
@@ -85,6 +84,16 @@ Http::FilterHeadersStatus BasicAuthFilter::decodeHeaders(Http::RequestHeaderMap& | |||
return Http::FilterHeadersStatus::Continue; | |||
} | |||
|
|||
bool BasicAuthFilter::validateUser(const UserMap* users, absl::string_view username, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for you quick response. Please just keep using const UserMap& users
in this method because it should never be null. :)
/wait
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, done
const UserMap* users = &config_->users(); | ||
if (route_specific_settings != nullptr) { | ||
users = &route_specific_settings->users(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Prefer reference_wrapper because it never be null. But pointer is also OK if you prefer it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not prefer pointers. I used the pointer to reassign users
inside the if statement, and I couldn't do that with reference_wrapper
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think reference_wraper
also could do the reassign? But anyway, it's fine.
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
LGTM.Thanks. |
Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Head branch was pushed to by a user without write access
@wbpcode I had to fix release notes format, could you approve again? |
* basic_auth: add per-route filter Signed-off-by: Jacek Ewertowski <jewertow@redhat.com> --------- Signed-off-by: Jacek Ewertowski <jewertow@redhat.com>
Commit Message: "basic_auth: add support for per-route filter"
Additional Description:
Risk Level: medium
Testing: unit, integration, manual
Docs Changes: added
Release Notes: added
Platform Specific Features: -
Fixes #32550
Here you can find configuration and steps for manual tests.