From f9accf55db01367215619e249f1630b359e30f85 Mon Sep 17 00:00:00 2001 From: bibby Date: Mon, 1 Jun 2020 23:25:50 -0400 Subject: [PATCH 01/13] first successful run Signed-off-by: bibby --- .../network/redis_proxy/v2/redis_proxy.proto | 6 ++- .../network/redis_proxy/v3/redis_proxy.proto | 6 ++- .../network/redis_proxy/v2/redis_proxy.proto | 6 ++- .../network/redis_proxy/v3/redis_proxy.proto | 6 ++- .../clusters/redis/redis_cluster.cc | 5 ++- .../extensions/clusters/redis/redis_cluster.h | 1 + .../filters/network/common/redis/client.h | 5 ++- .../network/common/redis/client_impl.cc | 13 +++++-- .../network/common/redis/client_impl.h | 5 ++- .../filters/network/common/redis/utility.cc | 12 ++++++ .../filters/network/common/redis/utility.h | 1 + .../network/redis_proxy/command_splitter.h | 2 +- .../redis_proxy/command_splitter_impl.cc | 7 +++- .../filters/network/redis_proxy/config.h | 18 ++++++++- .../network/redis_proxy/conn_pool_impl.cc | 7 ++-- .../network/redis_proxy/conn_pool_impl.h | 1 + .../network/redis_proxy/proxy_filter.cc | 11 ++++-- .../network/redis_proxy/proxy_filter.h | 7 +++- .../extensions/health_checkers/redis/redis.cc | 4 +- .../extensions/health_checkers/redis/redis.h | 1 + .../clusters/redis/redis_cluster_test.cc | 2 +- .../network/common/redis/client_impl_test.cc | 38 +++++++++++++++---- .../filters/network/common/redis/mocks.h | 2 +- .../redis_proxy/command_lookup_speed_test.cc | 2 +- .../redis_proxy/conn_pool_impl_test.cc | 5 ++- .../filters/network/redis_proxy/mocks.h | 1 + .../network/redis_proxy/proxy_filter_test.cc | 6 +-- .../health_checkers/redis/redis_test.cc | 2 +- 28 files changed, 141 insertions(+), 41 deletions(-) diff --git a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index caca630fd297d..b8a5ef87d75be 100644 --- a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto +++ b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto @@ -24,7 +24,7 @@ option (udpa.annotations.file_status).package_version_status = FROZEN; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 7] +// [#next-free-field: 8] message RedisProxy { // Redis connection pool settings. // [#next-free-field: 9] @@ -233,6 +233,8 @@ message RedisProxy { // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no password is set" error will be returned. api.v2.core.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + + api.v2.core.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } // RedisProtocolOptions specifies Redis upstream protocol options. This object is used in @@ -242,4 +244,6 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. api.v2.core.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + + api.v2.core.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index 143bd4da65e12..aac50eb2d8df2 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -22,7 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 7] +// [#next-free-field: 8] message RedisProxy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.redis_proxy.v2.RedisProxy"; @@ -234,6 +234,8 @@ message RedisProxy { // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no password is set" error will be returned. config.core.v3.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + + config.core.v3.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } // RedisProtocolOptions specifies Redis upstream protocol options. This object is used in @@ -246,4 +248,6 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. config.core.v3.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + + config.core.v3.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index caca630fd297d..b8a5ef87d75be 100644 --- a/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto +++ b/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto @@ -24,7 +24,7 @@ option (udpa.annotations.file_status).package_version_status = FROZEN; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 7] +// [#next-free-field: 8] message RedisProxy { // Redis connection pool settings. // [#next-free-field: 9] @@ -233,6 +233,8 @@ message RedisProxy { // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no password is set" error will be returned. api.v2.core.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + + api.v2.core.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } // RedisProtocolOptions specifies Redis upstream protocol options. This object is used in @@ -242,4 +244,6 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. api.v2.core.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + + api.v2.core.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index b9ca387f4ca55..b724456523431 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -22,7 +22,7 @@ option (udpa.annotations.file_status).package_version_status = ACTIVE; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 7] +// [#next-free-field: 8] message RedisProxy { option (udpa.annotations.versioning).previous_message_type = "envoy.config.filter.network.redis_proxy.v2.RedisProxy"; @@ -230,6 +230,8 @@ message RedisProxy { // AUTH, but no password is set" error will be returned. config.core.v3.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + config.core.v3.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; + string hidden_envoy_deprecated_cluster = 2 [deprecated = true, (envoy.annotations.disallowed_by_default) = true]; } @@ -244,4 +246,6 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. config.core.v3.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + + config.core.v3.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/source/extensions/clusters/redis/redis_cluster.cc b/source/extensions/clusters/redis/redis_cluster.cc index 6c0e04bdfbc1c..38dbccc60e6af 100644 --- a/source/extensions/clusters/redis/redis_cluster.cc +++ b/source/extensions/clusters/redis/redis_cluster.cc @@ -45,6 +45,8 @@ RedisCluster::RedisCluster( : Config::Utility::translateClusterHosts(cluster.hidden_envoy_deprecated_hosts())), local_info_(factory_context.localInfo()), random_(factory_context.random()), redis_discovery_session_(*this, redis_client_factory), lb_factory_(std::move(lb_factory)), + auth_username_( + NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authUsername(info(), api)), auth_password_( NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authPassword(info(), api)), cluster_name_(cluster.name()), @@ -278,7 +280,8 @@ void RedisCluster::RedisDiscoverySession::startResolveRedis() { client = std::make_unique(*this); client->host_ = current_host_address_; client->client_ = client_factory_.create(host, dispatcher_, *this, redis_command_stats_, - parent_.info()->statsScope(), parent_.auth_password_); + parent_.info()->statsScope(), parent_.auth_username_, + parent_.auth_password_); client->client_->addConnectionCallbacks(*client); } diff --git a/source/extensions/clusters/redis/redis_cluster.h b/source/extensions/clusters/redis/redis_cluster.h index f716960385c8a..ce0c32dd800e2 100644 --- a/source/extensions/clusters/redis/redis_cluster.h +++ b/source/extensions/clusters/redis/redis_cluster.h @@ -276,6 +276,7 @@ class RedisCluster : public Upstream::BaseDynamicClusterImpl { Upstream::HostVector hosts_; Upstream::HostMap all_hosts_; + const std::string auth_username_; const std::string auth_password_; const std::string cluster_name_; const Common::Redis::ClusterRefreshManagerSharedPtr refresh_manager_; diff --git a/source/extensions/filters/network/common/redis/client.h b/source/extensions/filters/network/common/redis/client.h index b420438ac55f2..0c8a15cb65fc5 100644 --- a/source/extensions/filters/network/common/redis/client.h +++ b/source/extensions/filters/network/common/redis/client.h @@ -106,7 +106,7 @@ class Client : public Event::DeferredDeletable { * Initialize the connection. Issue the auth command and readonly command as needed. * @param auth password for upstream host. */ - virtual void initialize(const std::string& auth_password) PURE; + virtual void initialize(const std::string& auth_username, const std::string& auth_password) PURE; }; using ClientPtr = std::unique_ptr; @@ -206,7 +206,8 @@ class ClientFactory { virtual ClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher, const Config& config, const RedisCommandStatsSharedPtr& redis_command_stats, - Stats::Scope& scope, const std::string& auth_password) PURE; + Stats::Scope& scope, const std::string& auth_username, + const std::string& auth_password) PURE; }; } // namespace Client diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index 9643b725a0111..c54285f8dc407 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -285,8 +285,12 @@ void ClientImpl::PendingRequest::cancel() { canceled_ = true; } -void ClientImpl::initialize(const std::string& auth_password) { - if (!auth_password.empty()) { +void ClientImpl::initialize(const std::string& auth_username, const std::string& auth_password) { + if (!auth_username.empty()) { + // Send an AUTH acl command to the upstream server with username. + Utility::AuthRequest auth_request(auth_username, auth_password); + makeRequest(auth_request, null_pool_callbacks); + } else if (!auth_password.empty()) { // Send an AUTH command to the upstream server. Utility::AuthRequest auth_request(auth_password); makeRequest(auth_request, null_pool_callbacks); @@ -304,10 +308,11 @@ ClientFactoryImpl ClientFactoryImpl::instance_; ClientPtr ClientFactoryImpl::create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher, const Config& config, const RedisCommandStatsSharedPtr& redis_command_stats, - Stats::Scope& scope, const std::string& auth_password) { + Stats::Scope& scope, const std::string& auth_username, + const std::string& auth_password) { ClientPtr client = ClientImpl::create(host, dispatcher, EncoderPtr{new EncoderImpl()}, decoder_factory_, config, redis_command_stats, scope); - client->initialize(auth_password); + client->initialize(auth_username, auth_password); return client; } diff --git a/source/extensions/filters/network/common/redis/client_impl.h b/source/extensions/filters/network/common/redis/client_impl.h index 5d7bcb182ea87..ad5b6231ffb79 100644 --- a/source/extensions/filters/network/common/redis/client_impl.h +++ b/source/extensions/filters/network/common/redis/client_impl.h @@ -87,7 +87,7 @@ class ClientImpl : public Client, public DecoderCallbacks, public Network::Conne PoolRequest* makeRequest(const RespValue& request, ClientCallbacks& callbacks) override; bool active() override { return !pending_requests_.empty(); } void flushBufferAndResetTimer(); - void initialize(const std::string& auth_password) override; + void initialize(const std::string& auth_username, const std::string& auth_password) override; private: friend class RedisClientImplTest; @@ -151,7 +151,8 @@ class ClientFactoryImpl : public ClientFactory { // RedisProxy::ConnPool::ClientFactoryImpl ClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher& dispatcher, const Config& config, const RedisCommandStatsSharedPtr& redis_command_stats, - Stats::Scope& scope, const std::string& auth_password) override; + Stats::Scope& scope, const std::string& auth_username, + const std::string& auth_password) override; static ClientFactoryImpl instance_; diff --git a/source/extensions/filters/network/common/redis/utility.cc b/source/extensions/filters/network/common/redis/utility.cc index c652addb3e12d..773196dd70e21 100644 --- a/source/extensions/filters/network/common/redis/utility.cc +++ b/source/extensions/filters/network/common/redis/utility.cc @@ -19,6 +19,18 @@ AuthRequest::AuthRequest(const std::string& password) { asArray().swap(values); } +AuthRequest::AuthRequest(const std::string& username, const std::string& password) { + std::vector values(3); + values[0].type(RespType::BulkString); + values[0].asString() = "auth"; + values[1].type(RespType::BulkString); + values[1].asString() = username; + values[2].type(RespType::BulkString); + values[2].asString() = password; + type(RespType::Array); + asArray().swap(values); +} + RespValuePtr makeError(const std::string& error) { Common::Redis::RespValuePtr response(new RespValue()); response->type(Common::Redis::RespType::Error); diff --git a/source/extensions/filters/network/common/redis/utility.h b/source/extensions/filters/network/common/redis/utility.h index b2e77b8e94ab4..ca5774d2d3a6c 100644 --- a/source/extensions/filters/network/common/redis/utility.h +++ b/source/extensions/filters/network/common/redis/utility.h @@ -13,6 +13,7 @@ namespace Utility { class AuthRequest : public Redis::RespValue { public: + AuthRequest(const std::string& username, const std::string& password); AuthRequest(const std::string& password); }; diff --git a/source/extensions/filters/network/redis_proxy/command_splitter.h b/source/extensions/filters/network/redis_proxy/command_splitter.h index 5e1248d3b5001..f51825106e7b4 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter.h @@ -44,7 +44,7 @@ class SplitCallbacks { * Called when an authentication command has been received. * @param password supplies the AUTH password provided by the downstream client. */ - virtual void onAuth(const std::string& password) PURE; + virtual void onAuth(const std::string& username, const std::string& password) PURE; /** * Called when the response is ready. diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc index adfbf7ff9fbe1..4db7de6a378d4 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -439,7 +439,12 @@ SplitRequestPtr InstanceImpl::makeRequest(Common::Redis::RespValuePtr&& request, onInvalidRequest(callbacks); return nullptr; } - callbacks.onAuth(request->asArray()[1].asString()); + if (request->asArray().size() == 3) { + callbacks.onAuth(request->asArray()[1].asString(), request->asArray()[2].asString()); + } else { + callbacks.onAuth("", request->asArray()[1].asString()); + } + return nullptr; } diff --git a/source/extensions/filters/network/redis_proxy/config.h b/source/extensions/filters/network/redis_proxy/config.h index e13d0cda331ef..cbb1866018f49 100644 --- a/source/extensions/filters/network/redis_proxy/config.h +++ b/source/extensions/filters/network/redis_proxy/config.h @@ -24,12 +24,27 @@ class ProtocolOptionsConfigImpl : public Upstream::ProtocolOptionsConfig { ProtocolOptionsConfigImpl( const envoy::extensions::filters::network::redis_proxy::v3::RedisProtocolOptions& proto_config) - : auth_password_(proto_config.auth_password()) {} + : auth_username_(proto_config.auth_username()), auth_password_(proto_config.auth_password()) { + } + + std::string authUsername(Api::Api& api) const { + return Config::DataSource::read(auth_username_, true, api); + } std::string authPassword(Api::Api& api) const { return Config::DataSource::read(auth_password_, true, api); } + static const std::string authUsername(const Upstream::ClusterInfoConstSharedPtr info, + Api::Api& api) { + auto options = info->extensionProtocolOptionsTyped( + NetworkFilterNames::get().RedisProxy); + if (options) { + return options->authUsername(api); + } + return EMPTY_STRING; + } + static const std::string authPassword(const Upstream::ClusterInfoConstSharedPtr info, Api::Api& api) { auto options = info->extensionProtocolOptionsTyped( @@ -41,6 +56,7 @@ class ProtocolOptionsConfigImpl : public Upstream::ProtocolOptionsConfig { } private: + envoy::config::core::v3::DataSource auth_username_; envoy::config::core::v3::DataSource auth_password_; }; diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc index 9bc15b0f14a6d..b446901c60720 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.cc @@ -74,6 +74,7 @@ InstanceImpl::ThreadLocalPool::ThreadLocalPool(InstanceImpl& parent, Event::Disp cluster_update_handle_ = parent_.cm_.addThreadLocalClusterUpdateCallbacks(*this); Upstream::ThreadLocalCluster* cluster = parent_.cm_.get(cluster_name_); if (cluster != nullptr) { + auth_username_ = ProtocolOptionsConfigImpl::authUsername(cluster->info(), parent_.api_); auth_password_ = ProtocolOptionsConfigImpl::authPassword(cluster->info(), parent_.api_); onClusterAddOrUpdateNonVirtual(*cluster); } @@ -214,9 +215,9 @@ InstanceImpl::ThreadLocalPool::threadLocalActiveClient(Upstream::HostConstShared if (!client) { client = std::make_unique(*this); client->host_ = host; - client->redis_client_ = parent_.client_factory_.create(host, dispatcher_, parent_.config_, - parent_.redis_command_stats_, - *parent_.stats_scope_, auth_password_); + client->redis_client_ = parent_.client_factory_.create( + host, dispatcher_, parent_.config_, parent_.redis_command_stats_, *parent_.stats_scope_, + auth_username_, auth_password_); client->redis_client_->addConnectionCallbacks(*client); } return client; diff --git a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h index 6dcb695efac80..6fa31f2ca0ea8 100644 --- a/source/extensions/filters/network/redis_proxy/conn_pool_impl.h +++ b/source/extensions/filters/network/redis_proxy/conn_pool_impl.h @@ -157,6 +157,7 @@ class InstanceImpl : public Instance { std::unordered_map client_map_; Envoy::Common::CallbackHandle* host_set_member_update_cb_handle_{}; std::unordered_map host_address_map_; + std::string auth_username_; std::string auth_password_; std::list created_via_redirect_hosts_; std::list clients_to_drain_; diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index 7782485d5ec9a..9b38c47b4d5f9 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -23,6 +23,8 @@ ProxyFilterConfig::ProxyFilterConfig( : drain_decision_(drain_decision), runtime_(runtime), stat_prefix_(fmt::format("redis.{}.", config.stat_prefix())), stats_(generateStats(stat_prefix_, scope)), + downstream_auth_username_( + Config::DataSource::read(config.downstream_auth_username(), true, api)), downstream_auth_password_( Config::DataSource::read(config.downstream_auth_password(), true, api)) {} @@ -38,7 +40,8 @@ ProxyFilter::ProxyFilter(Common::Redis::DecoderFactory& factory, config_(config) { config_->stats_.downstream_cx_total_.inc(); config_->stats_.downstream_cx_active_.inc(); - connection_allowed_ = config_->downstream_auth_password_.empty(); + connection_allowed_ = + config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty(); } ProxyFilter::~ProxyFilter() { @@ -79,12 +82,14 @@ void ProxyFilter::onEvent(Network::ConnectionEvent event) { } } -void ProxyFilter::onAuth(PendingRequest& request, const std::string& password) { +void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, + const std::string& password) { Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; if (config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); response->asString() = "ERR Client sent AUTH, but no password is set"; - } else if (password == config_->downstream_auth_password_) { + } else if (username == config_->downstream_auth_username_ && + password == config_->downstream_auth_password_) { response->type(Common::Redis::RespType::SimpleString); response->asString() = "OK"; connection_allowed_ = true; diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.h b/source/extensions/filters/network/redis_proxy/proxy_filter.h index 23ebd3e0f039b..5825ef31ed1bd 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.h +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.h @@ -57,6 +57,7 @@ class ProxyFilterConfig { const std::string stat_prefix_; const std::string redis_drain_close_runtime_key_{"redis.drain_close_enabled"}; ProxyStats stats_; + const std::string downstream_auth_username_; const std::string downstream_auth_password_; private: @@ -99,7 +100,9 @@ class ProxyFilter : public Network::ReadFilter, // RedisProxy::CommandSplitter::SplitCallbacks bool connectionAllowed() override { return parent_.connectionAllowed(); } - void onAuth(const std::string& password) override { parent_.onAuth(*this, password); } + void onAuth(const std::string& username, const std::string& password) override { + parent_.onAuth(*this, username, password); + } void onResponse(Common::Redis::RespValuePtr&& value) override { parent_.onResponse(*this, std::move(value)); } @@ -109,7 +112,7 @@ class ProxyFilter : public Network::ReadFilter, CommandSplitter::SplitRequestPtr request_handle_; }; - void onAuth(PendingRequest& request, const std::string& password); + void onAuth(PendingRequest& request, const std::string& username, const std::string& password); void onResponse(PendingRequest& request, Common::Redis::RespValuePtr&& value); Common::Redis::DecoderPtr decoder_; diff --git a/source/extensions/health_checkers/redis/redis.cc b/source/extensions/health_checkers/redis/redis.cc index d508193445da8..96d8766d180e0 100644 --- a/source/extensions/health_checkers/redis/redis.cc +++ b/source/extensions/health_checkers/redis/redis.cc @@ -19,6 +19,8 @@ RedisHealthChecker::RedisHealthChecker( Extensions::NetworkFilters::Common::Redis::Client::ClientFactory& client_factory) : HealthCheckerImplBase(cluster, config, dispatcher, runtime, random, std::move(event_logger)), client_factory_(client_factory), key_(redis_config.key()), + auth_username_( + NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authUsername(cluster.info(), api)), auth_password_(NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authPassword( cluster.info(), api)) { if (!key_.empty()) { @@ -65,7 +67,7 @@ void RedisHealthChecker::RedisActiveHealthCheckSession::onInterval() { if (!client_) { client_ = parent_.client_factory_.create( host_, parent_.dispatcher_, *this, redis_command_stats_, - parent_.cluster_.info()->statsScope(), parent_.auth_password_); + parent_.cluster_.info()->statsScope(), parent_.auth_username_, parent_.auth_password_); client_->addConnectionCallbacks(*this); } diff --git a/source/extensions/health_checkers/redis/redis.h b/source/extensions/health_checkers/redis/redis.h index 6284c475eda59..808c8ff26a7e4 100644 --- a/source/extensions/health_checkers/redis/redis.h +++ b/source/extensions/health_checkers/redis/redis.h @@ -125,6 +125,7 @@ class RedisHealthChecker : public Upstream::HealthCheckerImplBase { Extensions::NetworkFilters::Common::Redis::Client::ClientFactory& client_factory_; Type type_; const std::string key_; + const std::string auth_username_; const std::string auth_password_; }; diff --git a/test/extensions/clusters/redis/redis_cluster_test.cc b/test/extensions/clusters/redis/redis_cluster_test.cc index 6b9a87ab778a3..5936773382ad9 100644 --- a/test/extensions/clusters/redis/redis_cluster_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_test.cc @@ -65,7 +65,7 @@ class RedisClusterTest : public testing::Test, create(Upstream::HostConstSharedPtr host, Event::Dispatcher&, const Extensions::NetworkFilters::Common::Redis::Client::Config&, const Extensions::NetworkFilters::Common::Redis::RedisCommandStatsSharedPtr&, - Stats::Scope&, const std::string&) override { + Stats::Scope&, const std::string&, const std::string&) override { EXPECT_EQ(22120, host->address()->ip()->port()); return Extensions::NetworkFilters::Common::Redis::Client::ClientPtr{ create_(host->address()->asString())}; diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc index c9028a1da42ac..669005b914ead 100644 --- a/test/extensions/filters/network/common/redis/client_impl_test.cc +++ b/test/extensions/filters/network/common/redis/client_impl_test.cc @@ -116,7 +116,7 @@ class RedisClientImplTest : public testing::Test, Common::Redis::RespValue readonly_request = Utility::ReadOnlyRequest::instance(); EXPECT_CALL(*encoder_, encode(Eq(readonly_request), _)); EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); - client_->initialize(auth_password_); + client_->initialize(auth_username_, auth_password_); EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_total_.value()); EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_active_.value()); @@ -143,6 +143,7 @@ class RedisClientImplTest : public testing::Test, NiceMock stats_; Stats::ScopePtr stats_scope_; Common::Redis::RedisCommandStatsSharedPtr redis_command_stats_; + std::string auth_username_; std::string auth_password_; }; @@ -290,7 +291,7 @@ TEST_F(RedisClientImplTest, Basic) { setup(); - client_->initialize(auth_password_); + client_->initialize(auth_username_, auth_password_); Common::Redis::RespValue request1; MockClientCallbacks callbacks1; @@ -370,7 +371,7 @@ TEST_F(RedisClientImplTest, CommandStatsDisabledSingleRequest) { setup(); - client_->initialize(auth_password_); + client_->initialize(auth_username_, auth_password_); std::string get_command = "get"; @@ -426,7 +427,7 @@ TEST_F(RedisClientImplTest, CommandStatsEnabledTwoRequests) { setup(std::make_unique()); - client_->initialize(auth_password_); + client_->initialize(auth_username_, auth_password_); std::string get_command = "get"; @@ -511,7 +512,29 @@ TEST_F(RedisClientImplTest, InitializedWithAuthPassword) { Utility::AuthRequest auth_request(auth_password_); EXPECT_CALL(*encoder_, encode(Eq(auth_request), _)); EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); - client_->initialize(auth_password_); + client_->initialize(auth_username_, auth_password_); + + EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_total_.value()); + EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_active_.value()); + EXPECT_EQ(1UL, host_->stats_.rq_total_.value()); + EXPECT_EQ(1UL, host_->stats_.rq_active_.value()); + + EXPECT_CALL(*upstream_connection_, close(Network::ConnectionCloseType::NoFlush)); + EXPECT_CALL(*connect_or_op_timer_, disableTimer()); + client_->close(); +} + +TEST_F(RedisClientImplTest, InitializedWithAuthUsernamePassword) { + InSequence s; + + setup(); + + auth_username_ = "testing username"; + auth_password_ = "testing password"; + Utility::AuthRequest auth_request(auth_username_, auth_password_); + EXPECT_CALL(*encoder_, encode(Eq(auth_request), _)); + EXPECT_CALL(*flush_timer_, enabled()).WillOnce(Return(false)); + client_->initialize(auth_username_, auth_password_); EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_total_.value()); EXPECT_EQ(1UL, host_->cluster_.stats_.upstream_rq_active_.value()); @@ -1188,9 +1211,10 @@ TEST(RedisClientFactoryImplTest, Basic) { Stats::IsolatedStoreImpl stats_; auto redis_command_stats = Common::Redis::RedisCommandStats::createRedisCommandStats(stats_.symbolTable()); + const std::string auth_username; const std::string auth_password; - ClientPtr client = - factory.create(host, dispatcher, config, redis_command_stats, stats_, auth_password); + ClientPtr client = factory.create(host, dispatcher, config, redis_command_stats, stats_, + auth_username, auth_password); client->close(); } } // namespace Client diff --git a/test/extensions/filters/network/common/redis/mocks.h b/test/extensions/filters/network/common/redis/mocks.h index 0561c7bb57e0a..4f8f11bdaa4b5 100644 --- a/test/extensions/filters/network/common/redis/mocks.h +++ b/test/extensions/filters/network/common/redis/mocks.h @@ -87,7 +87,7 @@ class MockClient : public Client { MOCK_METHOD(void, close, ()); MOCK_METHOD(PoolRequest*, makeRequest_, (const Common::Redis::RespValue& request, ClientCallbacks& callbacks)); - MOCK_METHOD(void, initialize, (const std::string& password)); + MOCK_METHOD(void, initialize, (const std::string& username, const std::string& password)); std::list callbacks_; std::list client_callbacks_; diff --git a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc index a6ccc4f1b59d2..5ab03c3e04a76 100644 --- a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc @@ -27,7 +27,7 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { ~NoOpSplitCallbacks() override = default; bool connectionAllowed() override { return true; } - void onAuth(const std::string&) override {} + void onAuth(const std::string&, const std::string&) override {} void onResponse(Common::Redis::RespValuePtr&&) override {} }; diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index 639a9b8313c88..fe66069887139 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -199,8 +199,9 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client Common::Redis::Client::ClientPtr create(Upstream::HostConstSharedPtr host, Event::Dispatcher&, const Common::Redis::Client::Config&, const Common::Redis::RedisCommandStatsSharedPtr&, - Stats::Scope&, const std::string& password) override { - EXPECT_EQ(auth_password_, password); + Stats::Scope&, const std::string& username, + const std::string& password) override { + EXPECT_EQ(username, password); return Common::Redis::Client::ClientPtr{create_(host)}; } diff --git a/test/extensions/filters/network/redis_proxy/mocks.h b/test/extensions/filters/network/redis_proxy/mocks.h index 5bb208bfa9019..4d5aa47f82be2 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.h +++ b/test/extensions/filters/network/redis_proxy/mocks.h @@ -100,6 +100,7 @@ class MockSplitCallbacks : public SplitCallbacks { void onResponse(Common::Redis::RespValuePtr&& value) override { onResponse_(value); } MOCK_METHOD(bool, connectionAllowed, ()); + MOCK_METHOD(void, onAuth, (const std::string& username, const std::string& password)); MOCK_METHOD(void, onAuth, (const std::string& password)); MOCK_METHOD(void, onResponse_, (Common::Redis::RespValuePtr & value)); }; diff --git a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc index 72cebf97fcd28..1267e68087165 100644 --- a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc @@ -301,7 +301,7 @@ TEST_F(RedisProxyFilterTest, AuthWhenNotRequired) { error->asString() = "ERR Client sent AUTH, but no password is set"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*error)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("foo"); + callbacks.onAuth("", "foo"); // callbacks cannot be accessed now. EXPECT_TRUE(filter_->connectionAllowed()); return nullptr; @@ -344,7 +344,7 @@ TEST_F(RedisProxyFilterWithAuthPasswordTest, AuthPasswordCorrect) { reply->asString() = "OK"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("somepassword"); + callbacks.onAuth("", "somepassword"); // callbacks cannot be accessed now. EXPECT_TRUE(filter_->connectionAllowed()); return nullptr; @@ -371,7 +371,7 @@ TEST_F(RedisProxyFilterWithAuthPasswordTest, AuthPasswordIncorrect) { reply->asString() = "ERR invalid password"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("wrongpassword"); + callbacks.onAuth("", "wrongpassword"); // callbacks cannot be accessed now. EXPECT_FALSE(filter_->connectionAllowed()); return nullptr; diff --git a/test/extensions/health_checkers/redis/redis_test.cc b/test/extensions/health_checkers/redis/redis_test.cc index 9d413879998ca..7b1b850f0033f 100644 --- a/test/extensions/health_checkers/redis/redis_test.cc +++ b/test/extensions/health_checkers/redis/redis_test.cc @@ -157,7 +157,7 @@ class RedisHealthCheckerTest create(Upstream::HostConstSharedPtr, Event::Dispatcher&, const Extensions::NetworkFilters::Common::Redis::Client::Config&, const Extensions::NetworkFilters::Common::Redis::RedisCommandStatsSharedPtr&, - Stats::Scope&, const std::string&) override { + Stats::Scope&, const std::string&, const std::string&) override { return Extensions::NetworkFilters::Common::Redis::Client::ClientPtr{create_()}; } From f8413e1a922e82e842e176db1965360943f792fc Mon Sep 17 00:00:00 2001 From: bibby Date: Tue, 2 Jun 2020 15:11:35 -0400 Subject: [PATCH 02/13] Add unit tests and address code review - Added unit tests for ACL type auths - Added an overloaded version of onAuth rather than replacing the existing version - Updated error messages to be acl specific where applicable to assist in debugging Signed-off-by: bibby --- .../network/redis_proxy/command_splitter.h | 7 + .../redis_proxy/command_splitter_impl.cc | 2 +- .../network/redis_proxy/proxy_filter.cc | 23 +- .../network/redis_proxy/proxy_filter.h | 2 + .../redis/redis_cluster_integration_test.cc | 83 ++++++- .../redis_proxy/command_lookup_speed_test.cc | 1 + .../redis_proxy/conn_pool_impl_test.cc | 5 +- .../filters/network/redis_proxy/mocks.h | 2 +- .../network/redis_proxy/proxy_filter_test.cc | 154 ++++++++++++- .../redis_proxy_integration_test.cc | 216 +++++++++++++++++- 10 files changed, 469 insertions(+), 26 deletions(-) diff --git a/source/extensions/filters/network/redis_proxy/command_splitter.h b/source/extensions/filters/network/redis_proxy/command_splitter.h index f51825106e7b4..b5d12a83411d1 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter.h @@ -44,6 +44,13 @@ class SplitCallbacks { * Called when an authentication command has been received. * @param password supplies the AUTH password provided by the downstream client. */ + virtual void onAuth(const std::string& password) PURE; + + /** + * Called when an authentication acl command has been received. + * @param username supplies the AUTH username provided by the downstream client. + * @param password supplies the AUTH password provided by the downstream client. + */ virtual void onAuth(const std::string& username, const std::string& password) PURE; /** diff --git a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc index 4db7de6a378d4..a5bd89588f51a 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc +++ b/source/extensions/filters/network/redis_proxy/command_splitter_impl.cc @@ -442,7 +442,7 @@ SplitRequestPtr InstanceImpl::makeRequest(Common::Redis::RespValuePtr&& request, if (request->asArray().size() == 3) { callbacks.onAuth(request->asArray()[1].asString(), request->asArray()[2].asString()); } else { - callbacks.onAuth("", request->asArray()[1].asString()); + callbacks.onAuth(request->asArray()[1].asString()); } return nullptr; diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index 9b38c47b4d5f9..de75a706d3f93 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -82,12 +82,29 @@ void ProxyFilter::onEvent(Network::ConnectionEvent event) { } } -void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, - const std::string& password) { +void ProxyFilter::onAuth(PendingRequest& request, const std::string& password) { Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; if (config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); response->asString() = "ERR Client sent AUTH, but no password is set"; + } else if (password == config_->downstream_auth_password_) { + response->type(Common::Redis::RespType::SimpleString); + response->asString() = "OK"; + connection_allowed_ = true; + } else { + response->type(Common::Redis::RespType::Error); + response->asString() = "ERR invalid password"; + connection_allowed_ = false; + } + request.onResponse(std::move(response)); +} + +void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, + const std::string& password) { + Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; + if (config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty()) { + response->type(Common::Redis::RespType::Error); + response->asString() = "ERR Client sent AUTH, but no acl is set"; } else if (username == config_->downstream_auth_username_ && password == config_->downstream_auth_password_) { response->type(Common::Redis::RespType::SimpleString); @@ -95,7 +112,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, connection_allowed_ = true; } else { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR invalid password"; + response->asString() = "ERR invalid acl auth"; connection_allowed_ = false; } request.onResponse(std::move(response)); diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.h b/source/extensions/filters/network/redis_proxy/proxy_filter.h index 5825ef31ed1bd..1694a2a0640e9 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.h +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.h @@ -100,6 +100,7 @@ class ProxyFilter : public Network::ReadFilter, // RedisProxy::CommandSplitter::SplitCallbacks bool connectionAllowed() override { return parent_.connectionAllowed(); } + void onAuth(const std::string& password) override { parent_.onAuth(*this, password); } void onAuth(const std::string& username, const std::string& password) override { parent_.onAuth(*this, username, password); } @@ -112,6 +113,7 @@ class ProxyFilter : public Network::ReadFilter, CommandSplitter::SplitRequestPtr request_handle_; }; + void onAuth(PendingRequest& request, const std::string& password); void onAuth(PendingRequest& request, const std::string& username, const std::string& password); void onResponse(PendingRequest& request, Common::Redis::RespValuePtr&& value); diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 8746c86bbe5ff..7a468fa699053 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -122,6 +122,19 @@ const std::string& testConfigWithAuth() { )EOF"); } +// This is the basic redis_proxy configuration with an upstream +// authentication acl username and password specified. + +const std::string& testConfigWithAuthAcl() { + CONSTRUCT_ON_FIRST_USE(std::string, testConfig() + R"EOF( + typed_extension_protocol_options: + envoy.filters.network.redis_proxy: + "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions + auth_username: { inline_string: someusername } + auth_password: { inline_string: somepassword } +)EOF"); +} + // This function encodes commands as an array of bulkstrings as transmitted by Redis clients to // Redis servers, according to the Redis protocol. std::string makeBulkStringArray(std::vector&& command_strings) { @@ -199,7 +212,7 @@ class RedisClusterIntegrationTest : public testing::TestWithParamwaitForData(auth_command.size() + request.size(), &proxy_to_server)); // The original request should be the same as the data received by the server. @@ -244,13 +260,14 @@ class RedisClusterIntegrationTest : public testing::TestWithParamclose(); EXPECT_TRUE(fake_upstream_connection->close()); } void expectCallClusterSlot(int stream_index, std::string& response, + const std::string& auth_username = "", const std::string& auth_password = "") { std::string cluster_slot_request = makeBulkStringArray({"CLUSTER", "SLOTS"}); @@ -264,10 +281,18 @@ class RedisClusterIntegrationTest : public testing::TestWithParamwaitForData(cluster_slot_request.size(), &proxied_cluster_slot_request)); EXPECT_EQ(cluster_slot_request, proxied_cluster_slot_request); - } else { + } else if (auth_username.empty()) { std::string auth_request = makeBulkStringArray({"auth", auth_password}); std::string ok = "+OK\r\n"; + EXPECT_TRUE(fake_upstream_connection_->waitForData( + auth_request.size() + cluster_slot_request.size(), &proxied_cluster_slot_request)); + EXPECT_EQ(auth_request + cluster_slot_request, proxied_cluster_slot_request); + EXPECT_TRUE(fake_upstream_connection_->write(ok)); + } else { + std::string auth_request = makeBulkStringArray({"auth", auth_username, auth_password}); + std::string ok = "+OK\r\n"; + EXPECT_TRUE(fake_upstream_connection_->waitForData( auth_request.size() + cluster_slot_request.size(), &proxied_cluster_slot_request)); EXPECT_EQ(auth_request + cluster_slot_request, proxied_cluster_slot_request); @@ -354,6 +379,13 @@ class RedisClusterWithAuthIntegrationTest : public RedisClusterIntegrationTest { : RedisClusterIntegrationTest(config, num_upstreams) {} }; +class RedisClusterWithAuthAclIntegrationTest : public RedisClusterIntegrationTest { +public: + RedisClusterWithAuthAclIntegrationTest(const std::string& config = testConfigWithAuthAcl(), + int num_upstreams = 2) + : RedisClusterIntegrationTest(config, num_upstreams) {} +}; + class RedisClusterWithReadPolicyIntegrationTest : public RedisClusterIntegrationTest { public: RedisClusterWithReadPolicyIntegrationTest(const std::string& config = testConfigWithReadPolicy(), @@ -376,6 +408,10 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterWithAuthIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); +INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterWithAuthAclIntegrationTest, + testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), + TestUtility::ipTestParamsToString); + // This test sends a simple "get foo" command from a fake // downstream client through the proxy to a fake upstream // Redis cluster with a single slot with master and replica. @@ -530,7 +566,41 @@ TEST_P(RedisClusterWithAuthIntegrationTest, SingleSlotMasterReplica) { on_server_init_function_ = [this]() { std::string cluster_slot_response = singleSlotMasterReplica( fake_upstreams_[0]->localAddress()->ip(), fake_upstreams_[1]->localAddress()->ip()); - expectCallClusterSlot(0, cluster_slot_response, "somepassword"); + expectCallClusterSlot(0, cluster_slot_response, "", "somepassword"); + }; + + initialize(); + + IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); + FakeRawConnectionPtr fake_upstream_connection; + + roundtripToUpstreamStep(fake_upstreams_[random_index_], makeBulkStringArray({"get", "foo"}), + "$3\r\nbar\r\n", redis_client, fake_upstream_connection, "", + "somepassword"); + + redis_client->close(); + EXPECT_TRUE(fake_upstream_connection->close()); +} + +// This test sends a simple "get foo" command from a fake +// downstream client through the proxy to a fake upstream +// Redis cluster with a single slot with master and replica. +// The fake server sends a valid response back to the client. +// The request and response should make it through the envoy +// proxy server code unchanged. +// +// In this scenario, the fake server will receive 2 auth acl commands: +// one as part of a topology discovery connection (before sending a +// "cluster slots" command), and one to authenticate the connection +// that carries the "get foo" request. + +TEST_P(RedisClusterWithAuthAclIntegrationTest, SingleSlotMasterReplica) { + random_index_ = 0; + + on_server_init_function_ = [this]() { + std::string cluster_slot_response = singleSlotMasterReplica( + fake_upstreams_[0]->localAddress()->ip(), fake_upstreams_[1]->localAddress()->ip()); + expectCallClusterSlot(0, cluster_slot_response, "someusername", "somepassword"); }; initialize(); @@ -539,7 +609,8 @@ TEST_P(RedisClusterWithAuthIntegrationTest, SingleSlotMasterReplica) { FakeRawConnectionPtr fake_upstream_connection; roundtripToUpstreamStep(fake_upstreams_[random_index_], makeBulkStringArray({"get", "foo"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection, "somepassword"); + "$3\r\nbar\r\n", redis_client, fake_upstream_connection, "someusername", + "somepassword"); redis_client->close(); EXPECT_TRUE(fake_upstream_connection->close()); diff --git a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc index 5ab03c3e04a76..edf29c9730921 100644 --- a/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc +++ b/test/extensions/filters/network/redis_proxy/command_lookup_speed_test.cc @@ -27,6 +27,7 @@ class NoOpSplitCallbacks : public CommandSplitter::SplitCallbacks { ~NoOpSplitCallbacks() override = default; bool connectionAllowed() override { return true; } + void onAuth(const std::string&) override {} void onAuth(const std::string&, const std::string&) override {} void onResponse(Common::Redis::RespValuePtr&&) override {} }; diff --git a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc index fe66069887139..f3350db7cd5f8 100644 --- a/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc +++ b/test/extensions/filters/network/redis_proxy/conn_pool_impl_test.cc @@ -84,6 +84,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client read_policy_), api_, std::move(store), redis_command_stats, cluster_refresh_manager_); // Set the authentication password for this connection pool. + conn_pool_impl->tls_->getTyped().auth_username_ = auth_username_; conn_pool_impl->tls_->getTyped().auth_password_ = auth_password_; conn_pool_ = std::move(conn_pool_impl); test_address_ = Network::Utility::resolveUrl("tcp://127.0.0.1:3000"); @@ -201,7 +202,8 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client const Common::Redis::RedisCommandStatsSharedPtr&, Stats::Scope&, const std::string& username, const std::string& password) override { - EXPECT_EQ(username, password); + EXPECT_EQ(auth_username_, username); + EXPECT_EQ(auth_password_, password); return Common::Redis::Client::ClientPtr{create_(host)}; } @@ -274,6 +276,7 @@ class RedisConnPoolImplTest : public testing::Test, public Common::Redis::Client Upstream::ClusterUpdateCallbacks* update_callbacks_{}; Common::Redis::Client::MockClient* client_{}; Network::Address::InstanceConstSharedPtr test_address_; + std::string auth_username_; std::string auth_password_; NiceMock api_; envoy::extensions::filters::network::redis_proxy::v3::RedisProxy::ConnPoolSettings::ReadPolicy diff --git a/test/extensions/filters/network/redis_proxy/mocks.h b/test/extensions/filters/network/redis_proxy/mocks.h index 4d5aa47f82be2..b093ad35b9b94 100644 --- a/test/extensions/filters/network/redis_proxy/mocks.h +++ b/test/extensions/filters/network/redis_proxy/mocks.h @@ -100,8 +100,8 @@ class MockSplitCallbacks : public SplitCallbacks { void onResponse(Common::Redis::RespValuePtr&& value) override { onResponse_(value); } MOCK_METHOD(bool, connectionAllowed, ()); - MOCK_METHOD(void, onAuth, (const std::string& username, const std::string& password)); MOCK_METHOD(void, onAuth, (const std::string& password)); + MOCK_METHOD(void, onAuth, (const std::string& username, const std::string& password)); MOCK_METHOD(void, onResponse_, (Common::Redis::RespValuePtr & value)); }; diff --git a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc index 1267e68087165..215e2ee4d876a 100644 --- a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc @@ -63,6 +63,7 @@ TEST_F(RedisProxyFilterConfigTest, Normal) { parseProtoFromYaml(yaml_string); ProxyFilterConfig config(proto_config, store_, drain_decision_, runtime_, api_); EXPECT_EQ("redis.foo.", config.stat_prefix_); + EXPECT_TRUE(config.downstream_auth_username_.empty()); EXPECT_TRUE(config.downstream_auth_password_.empty()); } @@ -93,6 +94,27 @@ TEST_F(RedisProxyFilterConfigTest, DownstreamAuthPasswordSet) { EXPECT_EQ(config.downstream_auth_password_, "somepassword"); } +TEST_F(RedisProxyFilterConfigTest, DownstreamAuthAclSet) { + const std::string yaml_string = R"EOF( + prefix_routes: + catch_all_route: + cluster: fake_cluster + stat_prefix: foo + settings: + op_timeout: 0.01s + downstream_auth_username: + inline_string: someusername + downstream_auth_password: + inline_string: somepassword + )EOF"; + + envoy::extensions::filters::network::redis_proxy::v3::RedisProxy proto_config = + parseProtoFromYaml(yaml_string); + ProxyFilterConfig config(proto_config, store_, drain_decision_, runtime_, api_); + EXPECT_EQ(config.downstream_auth_username_, "someusername"); + EXPECT_EQ(config.downstream_auth_password_, "somepassword"); +} + class RedisProxyFilterTest : public testing::Test, public Common::Redis::DecoderFactory { public: static constexpr const char* DefaultConfig = R"EOF( @@ -301,7 +323,34 @@ TEST_F(RedisProxyFilterTest, AuthWhenNotRequired) { error->asString() = "ERR Client sent AUTH, but no password is set"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*error)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("", "foo"); + callbacks.onAuth("foo"); + // callbacks cannot be accessed now. + EXPECT_TRUE(filter_->connectionAllowed()); + return nullptr; + })); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(fake_data, false)); +} + +TEST_F(RedisProxyFilterTest, AuthAclWhenNotRequired) { + InSequence s; + + Buffer::OwnedImpl fake_data; + Common::Redis::RespValuePtr request(new Common::Redis::RespValue()); + EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void { + decoder_callbacks_->onRespValue(std::move(request)); + })); + EXPECT_CALL(splitter_, makeRequest_(Ref(*request), _)) + .WillOnce( + Invoke([&](const Common::Redis::RespValue&, + CommandSplitter::SplitCallbacks& callbacks) -> CommandSplitter::SplitRequest* { + EXPECT_TRUE(callbacks.connectionAllowed()); + Common::Redis::RespValuePtr error(new Common::Redis::RespValue()); + error->type(Common::Redis::RespType::Error); + error->asString() = "ERR Client sent AUTH, but no acl is set"; + EXPECT_CALL(*encoder_, encode(Eq(ByRef(*error)), _)); + EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); + callbacks.onAuth("foo", "bar"); // callbacks cannot be accessed now. EXPECT_TRUE(filter_->connectionAllowed()); return nullptr; @@ -344,7 +393,7 @@ TEST_F(RedisProxyFilterWithAuthPasswordTest, AuthPasswordCorrect) { reply->asString() = "OK"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("", "somepassword"); + callbacks.onAuth("somepassword"); // callbacks cannot be accessed now. EXPECT_TRUE(filter_->connectionAllowed()); return nullptr; @@ -371,7 +420,106 @@ TEST_F(RedisProxyFilterWithAuthPasswordTest, AuthPasswordIncorrect) { reply->asString() = "ERR invalid password"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); - callbacks.onAuth("", "wrongpassword"); + callbacks.onAuth("wrongpassword"); + // callbacks cannot be accessed now. + EXPECT_FALSE(filter_->connectionAllowed()); + return nullptr; + })); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(fake_data, false)); +} + +const std::string downstream_auth_acl_config = R"EOF( +prefix_routes: + catch_all_route: + cluster: fake_cluster +stat_prefix: foo +settings: + op_timeout: 0.01s +downstream_auth_username: + inline_string: someusername +downstream_auth_password: + inline_string: somepassword +)EOF"; + +class RedisProxyFilterWithAuthAclTest : public RedisProxyFilterTest { +public: + RedisProxyFilterWithAuthAclTest() : RedisProxyFilterTest(downstream_auth_acl_config) {} +}; + +TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclCorrect) { + InSequence s; + + Buffer::OwnedImpl fake_data; + Common::Redis::RespValuePtr request(new Common::Redis::RespValue()); + EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void { + decoder_callbacks_->onRespValue(std::move(request)); + })); + EXPECT_CALL(splitter_, makeRequest_(Ref(*request), _)) + .WillOnce( + Invoke([&](const Common::Redis::RespValue&, + CommandSplitter::SplitCallbacks& callbacks) -> CommandSplitter::SplitRequest* { + EXPECT_FALSE(callbacks.connectionAllowed()); + Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); + reply->type(Common::Redis::RespType::SimpleString); + reply->asString() = "OK"; + EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); + EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); + callbacks.onAuth("someusername", "somepassword"); + // callbacks cannot be accessed now. + EXPECT_TRUE(filter_->connectionAllowed()); + return nullptr; + })); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(fake_data, false)); +} + +TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclUsernameIncorrect) { + InSequence s; + + Buffer::OwnedImpl fake_data; + Common::Redis::RespValuePtr request(new Common::Redis::RespValue()); + EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void { + decoder_callbacks_->onRespValue(std::move(request)); + })); + EXPECT_CALL(splitter_, makeRequest_(Ref(*request), _)) + .WillOnce( + Invoke([&](const Common::Redis::RespValue&, + CommandSplitter::SplitCallbacks& callbacks) -> CommandSplitter::SplitRequest* { + EXPECT_FALSE(callbacks.connectionAllowed()); + Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); + reply->type(Common::Redis::RespType::Error); + reply->asString() = "ERR invalid acl auth"; + EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); + EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); + callbacks.onAuth("wrongusername", "somepassword"); + // callbacks cannot be accessed now. + EXPECT_FALSE(filter_->connectionAllowed()); + return nullptr; + })); + + EXPECT_EQ(Network::FilterStatus::Continue, filter_->onData(fake_data, false)); +} + +TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclPasswordIncorrect) { + InSequence s; + + Buffer::OwnedImpl fake_data; + Common::Redis::RespValuePtr request(new Common::Redis::RespValue()); + EXPECT_CALL(*decoder_, decode(Ref(fake_data))).WillOnce(Invoke([&](Buffer::Instance&) -> void { + decoder_callbacks_->onRespValue(std::move(request)); + })); + EXPECT_CALL(splitter_, makeRequest_(Ref(*request), _)) + .WillOnce( + Invoke([&](const Common::Redis::RespValue&, + CommandSplitter::SplitCallbacks& callbacks) -> CommandSplitter::SplitRequest* { + EXPECT_FALSE(callbacks.connectionAllowed()); + Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); + reply->type(Common::Redis::RespType::Error); + reply->asString() = "ERR invalid acl auth"; + EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); + EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); + callbacks.onAuth("someusername", "wrongpassword"); // callbacks cannot be accessed now. EXPECT_FALSE(filter_->connectionAllowed()); return nullptr; diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index b1e33acfe8203..b7bafeff1fe1d 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -273,6 +273,95 @@ const std::string CONFIG_WITH_ROUTES_AND_AUTH_PASSWORDS = R"EOF( cluster: cluster_2 )EOF"; +const std::string CONFIG_WITH_DOWNSTREAM_AUTH_ACL_SET = CONFIG + R"EOF( + downstream_auth_username: { inline_string: someusername } + downstream_auth_password: { inline_string: somepassword } +)EOF"; + +const std::string CONFIG_WITH_ROUTES_AND_AUTH_ACLS = R"EOF( +admin: + access_log_path: /dev/null + address: + socket_address: + address: 127.0.0.1 + port_value: 0 +static_resources: + clusters: + - name: cluster_0 + type: STATIC + typed_extension_protocol_options: + envoy.filters.network.redis_proxy: + "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions + auth_username: { inline_string: cluster_0_username } + auth_password: { inline_string: cluster_0_password } + lb_policy: RANDOM + load_assignment: + cluster_name: cluster_0 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + - name: cluster_1 + type: STATIC + lb_policy: RANDOM + typed_extension_protocol_options: + envoy.filters.network.redis_proxy: + "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions + auth_username: { inline_string: cluster_1_username } + auth_password: { inline_string: cluster_1_password } + load_assignment: + cluster_name: cluster_1 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 1 + - name: cluster_2 + type: STATIC + typed_extension_protocol_options: + envoy.filters.network.redis_proxy: + "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions + auth_username: { inline_string: cluster_2_username } + auth_password: { inline_string: cluster_2_password } + lb_policy: RANDOM + load_assignment: + cluster_name: cluster_2 + endpoints: + - lb_endpoints: + - endpoint: + address: + socket_address: + address: 127.0.0.1 + port_value: 2 + listeners: + name: listener_0 + address: + socket_address: + address: 127.0.0.1 + port_value: 0 + filter_chains: + filters: + name: redis + typed_config: + "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProxy + stat_prefix: redis_stats + settings: + op_timeout: 5s + prefix_routes: + catch_all_route: + cluster: cluster_0 + routes: + - prefix: "foo:" + cluster: cluster_1 + - prefix: "baz:" + cluster: cluster_2 +)EOF"; + // This function encodes commands as an array of bulkstrings as transmitted by Redis clients to // Redis servers, according to the Redis protocol. std::string makeBulkStringArray(std::vector&& command_strings) { @@ -360,12 +449,13 @@ class RedisProxyIntegrationTest : public testing::TestWithParamclearData(); redis_client->write(request); expectUpstreamRequestResponse(upstream, request, response, fake_upstream_connection, - auth_password); + auth_username, auth_password); redis_client->waitForData(response); // The original response should be received by the fake Redis client. @@ -494,7 +606,8 @@ void RedisProxyIntegrationTest::roundtripToUpstreamStep( void RedisProxyIntegrationTest::expectUpstreamRequestResponse( FakeUpstreamPtr& upstream, const std::string& request, const std::string& response, - FakeRawConnectionPtr& fake_upstream_connection, const std::string& auth_password) { + FakeRawConnectionPtr& fake_upstream_connection, const std::string& auth_username, + const std::string& auth_password) { std::string proxy_to_server; bool expect_auth_command = false; std::string ok = "+OK\r\n"; @@ -504,7 +617,9 @@ void RedisProxyIntegrationTest::expectUpstreamRequestResponse( EXPECT_TRUE(upstream->waitForRawConnection(fake_upstream_connection)); } if (expect_auth_command) { - std::string auth_command = makeBulkStringArray({"auth", auth_password}); + std::string auth_command = (auth_username.empty()) + ? makeBulkStringArray({"auth", auth_password}) + : makeBulkStringArray({"auth", auth_username, auth_password}); EXPECT_TRUE(fake_upstream_connection->waitForData(auth_command.size() + request.size(), &proxy_to_server)); // The original request should be the same as the data received by the server. @@ -527,7 +642,8 @@ void RedisProxyIntegrationTest::simpleRoundtripToUpstream(FakeUpstreamPtr& upstr IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); FakeRawConnectionPtr fake_upstream_connection; - roundtripToUpstreamStep(upstream, request, response, redis_client, fake_upstream_connection, ""); + roundtripToUpstreamStep(upstream, request, response, redis_client, fake_upstream_connection, "", + ""); EXPECT_TRUE(fake_upstream_connection->close()); redis_client->close(); @@ -626,10 +742,11 @@ TEST_P(RedisProxyWithCommandStatsIntegrationTest, MGETRequestAndResponse) { // Make GET request to upstream (MGET is turned into GETs for upstream) FakeUpstreamPtr& upstream = fake_upstreams_[0]; FakeRawConnectionPtr fake_upstream_connection; + std::string auth_username = ""; std::string auth_password = ""; std::string upstream_request = makeBulkStringArray({"get", "foo"}); expectUpstreamRequestResponse(upstream, upstream_request, upstream_response, - fake_upstream_connection, auth_password); + fake_upstream_connection, auth_username, auth_password); // Downstream response for MGET redis_client->waitForData(downstream_response); @@ -678,6 +795,16 @@ TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoPasswordSet) { "-ERR Client sent AUTH, but no password is set\r\n"); } +// This test sends an AUTH acl command from the fake downstream client to +// the Envoy proxy. Envoy will respond with a no-password-set error since +// no downstream_auth_username and downstream_auth_password has been set for the filter. + +TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoAclSet) { + initialize(); + simpleProxyResponse(makeBulkStringArray({"auth", "someusername", "somepassword"}), + "-ERR Client sent AUTH, but no acl is set\r\n"); +} + // This test sends a simple Redis command to a sequence of fake upstream // Redis servers. The first server replies with a MOVED or ASK redirection // error that specifies the second upstream server in the static configuration @@ -915,7 +1042,7 @@ TEST_P(RedisProxyWithDownstreamAuthIntegrationTest, ErrorsUntilCorrectPasswordSe proxyResponseStep(makeBulkStringArray({"auth", "somepassword"}), "+OK\r\n", redis_client); roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n", - redis_client, fake_upstream_connection, ""); + redis_client, fake_upstream_connection, "", ""); EXPECT_TRUE(fake_upstream_connection->close()); redis_client->close(); @@ -932,16 +1059,16 @@ TEST_P(RedisProxyWithRoutesAndAuthPasswordsIntegrationTest, TransparentAuthentic // roundtrip to cluster_0 (catch_all route) roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), "$3\r\nbar\r\n", - redis_client, fake_upstream_connection[0], "cluster_0_password"); + redis_client, fake_upstream_connection[0], "", "cluster_0_password"); // roundtrip to cluster_1 (prefix "foo:" route) roundtripToUpstreamStep(fake_upstreams_[1], makeBulkStringArray({"get", "foo:123"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection[1], + "$3\r\nbar\r\n", redis_client, fake_upstream_connection[1], "", "cluster_1_password"); // roundtrip to cluster_2 (prefix "baz:" route) roundtripToUpstreamStep(fake_upstreams_[2], makeBulkStringArray({"get", "baz:123"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection[2], + "$3\r\nbar\r\n", redis_client, fake_upstream_connection[2], "", "cluster_2_password"); EXPECT_TRUE(fake_upstream_connection[0]->close()); @@ -950,6 +1077,73 @@ TEST_P(RedisProxyWithRoutesAndAuthPasswordsIntegrationTest, TransparentAuthentic redis_client->close(); } +// This test verifies that a client connection cannot issue a command to an upstream +// server until it supplies a valid Redis AUTH command when downstream_auth_username and +// downstream_auth_password is set for the redis_proxy filter. It also verifies the errors sent by +// the proxy when no username or password or the wrong username or password is received. + +TEST_P(RedisProxyWithDownstreamAuthAclIntegrationTest, ErrorsUntilCorrectAclSent) { + initialize(); + + IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); + FakeRawConnectionPtr fake_upstream_connection; + + proxyResponseStep(makeBulkStringArray({"get", "foo"}), "-NOAUTH Authentication required.\r\n", + redis_client); + + std::stringstream error_response; + error_response << "-" << RedisCmdSplitter::Response::get().InvalidRequest << "\r\n"; + proxyResponseStep(makeBulkStringArray({"auth"}), error_response.str(), redis_client); + + proxyResponseStep(makeBulkStringArray({"auth", "wrongusername", "somepassword"}), + "-ERR invalid acl auth\r\n", redis_client); + + proxyResponseStep(makeBulkStringArray({"auth", "someusername", "wrongpassword"}), + "-ERR invalid acl auth\r\n", redis_client); + + proxyResponseStep(makeBulkStringArray({"get", "foo"}), "-NOAUTH Authentication required.\r\n", + redis_client); + + proxyResponseStep(makeBulkStringArray({"auth", "someusername", "somepassword"}), "+OK\r\n", + redis_client); + + roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n", + redis_client, fake_upstream_connection, "", ""); + + EXPECT_TRUE(fake_upstream_connection->close()); + redis_client->close(); +} + +// This test verifies that upstream server connections are transparently authenticated if an +// auth_username and auth_password is specified for each cluster. + +TEST_P(RedisProxyWithRoutesAndAuthAclsIntegrationTest, TransparentAuthentication) { + initialize(); + + IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); + std::array fake_upstream_connection; + + // roundtrip to cluster_0 (catch_all route) + roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), "$3\r\nbar\r\n", + redis_client, fake_upstream_connection[0], "cluster_0_username", + "cluster_0_password"); + + // roundtrip to cluster_1 (prefix "foo:" route) + roundtripToUpstreamStep(fake_upstreams_[1], makeBulkStringArray({"get", "foo:123"}), + "$3\r\nbar\r\n", redis_client, fake_upstream_connection[1], + "cluster_1_username", "cluster_1_password"); + + // roundtrip to cluster_2 (prefix "baz:" route) + roundtripToUpstreamStep(fake_upstreams_[2], makeBulkStringArray({"get", "baz:123"}), + "$3\r\nbar\r\n", redis_client, fake_upstream_connection[2], + "cluster_2_username", "cluster_2_password"); + + EXPECT_TRUE(fake_upstream_connection[0]->close()); + EXPECT_TRUE(fake_upstream_connection[1]->close()); + EXPECT_TRUE(fake_upstream_connection[2]->close()); + redis_client->close(); +} + TEST_P(RedisProxyWithMirrorsIntegrationTest, MirroredCatchAllRequest) { initialize(); From 171fdca9f09ed01484895eac3a8cdad4d37ec7f1 Mon Sep 17 00:00:00 2001 From: bibby Date: Tue, 2 Jun 2020 15:35:34 -0400 Subject: [PATCH 03/13] fix test name Signed-off-by: bibby --- .../extensions/filters/network/common/redis/client_impl_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/network/common/redis/client_impl_test.cc b/test/extensions/filters/network/common/redis/client_impl_test.cc index 669005b914ead..d5402aeaa6dbf 100644 --- a/test/extensions/filters/network/common/redis/client_impl_test.cc +++ b/test/extensions/filters/network/common/redis/client_impl_test.cc @@ -524,7 +524,7 @@ TEST_F(RedisClientImplTest, InitializedWithAuthPassword) { client_->close(); } -TEST_F(RedisClientImplTest, InitializedWithAuthUsernamePassword) { +TEST_F(RedisClientImplTest, InitializedWithAuthAcl) { InSequence s; setup(); From 8b61479d05f9bfc3c5f8c084acad6c7a15343912 Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 3 Jun 2020 10:24:04 -0400 Subject: [PATCH 04/13] Added ACL to the dictionary and fixed casing and some comments/error messages Signed-off-by: bibby --- .../filters/network/common/redis/client_impl.cc | 2 +- .../filters/network/redis_proxy/command_splitter.h | 4 ++-- .../filters/network/redis_proxy/proxy_filter.cc | 4 ++-- .../redis/redis_cluster_integration_test.cc | 4 ++-- .../network/redis_proxy/proxy_filter_test.cc | 6 +++--- .../redis_proxy/redis_proxy_integration_test.cc | 13 +++++++------ tools/spelling/spelling_dictionary.txt | 1 + 7 files changed, 18 insertions(+), 16 deletions(-) diff --git a/source/extensions/filters/network/common/redis/client_impl.cc b/source/extensions/filters/network/common/redis/client_impl.cc index c54285f8dc407..cadfa95a13716 100644 --- a/source/extensions/filters/network/common/redis/client_impl.cc +++ b/source/extensions/filters/network/common/redis/client_impl.cc @@ -287,7 +287,7 @@ void ClientImpl::PendingRequest::cancel() { void ClientImpl::initialize(const std::string& auth_username, const std::string& auth_password) { if (!auth_username.empty()) { - // Send an AUTH acl command to the upstream server with username. + // Send an AUTH command to the upstream server with username and password. Utility::AuthRequest auth_request(auth_username, auth_password); makeRequest(auth_request, null_pool_callbacks); } else if (!auth_password.empty()) { diff --git a/source/extensions/filters/network/redis_proxy/command_splitter.h b/source/extensions/filters/network/redis_proxy/command_splitter.h index b5d12a83411d1..e03d0a92e1377 100644 --- a/source/extensions/filters/network/redis_proxy/command_splitter.h +++ b/source/extensions/filters/network/redis_proxy/command_splitter.h @@ -41,13 +41,13 @@ class SplitCallbacks { virtual bool connectionAllowed() PURE; /** - * Called when an authentication command has been received. + * Called when an authentication command has been received with a password. * @param password supplies the AUTH password provided by the downstream client. */ virtual void onAuth(const std::string& password) PURE; /** - * Called when an authentication acl command has been received. + * Called when an authentication command has been received with a username and password. * @param username supplies the AUTH username provided by the downstream client. * @param password supplies the AUTH password provided by the downstream client. */ diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index de75a706d3f93..da63286f02304 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -104,7 +104,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; if (config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR Client sent AUTH, but no acl is set"; + response->asString() = "ERR Client sent AUTH, but no username or password is set"; } else if (username == config_->downstream_auth_username_ && password == config_->downstream_auth_password_) { response->type(Common::Redis::RespType::SimpleString); @@ -112,7 +112,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, connection_allowed_ = true; } else { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR invalid acl auth"; + response->asString() = "ERR invalid auth"; connection_allowed_ = false; } request.onResponse(std::move(response)); diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 7a468fa699053..4e40091a35701 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -123,7 +123,7 @@ const std::string& testConfigWithAuth() { } // This is the basic redis_proxy configuration with an upstream -// authentication acl username and password specified. +// authentication username and password specified. const std::string& testConfigWithAuthAcl() { CONSTRUCT_ON_FIRST_USE(std::string, testConfig() + R"EOF( @@ -589,7 +589,7 @@ TEST_P(RedisClusterWithAuthIntegrationTest, SingleSlotMasterReplica) { // The request and response should make it through the envoy // proxy server code unchanged. // -// In this scenario, the fake server will receive 2 auth acl commands: +// In this scenario, the fake server will receive 2 auth commands with username and password: // one as part of a topology discovery connection (before sending a // "cluster slots" command), and one to authenticate the connection // that carries the "get foo" request. diff --git a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc index 215e2ee4d876a..61f8d7b82e17d 100644 --- a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc @@ -347,7 +347,7 @@ TEST_F(RedisProxyFilterTest, AuthAclWhenNotRequired) { EXPECT_TRUE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr error(new Common::Redis::RespValue()); error->type(Common::Redis::RespType::Error); - error->asString() = "ERR Client sent AUTH, but no acl is set"; + error->asString() = "ERR Client sent AUTH, but no ACL is set"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*error)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("foo", "bar"); @@ -489,7 +489,7 @@ TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclUsernameIncorrect) { EXPECT_FALSE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); reply->type(Common::Redis::RespType::Error); - reply->asString() = "ERR invalid acl auth"; + reply->asString() = "ERR invalid ACL auth"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("wrongusername", "somepassword"); @@ -516,7 +516,7 @@ TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclPasswordIncorrect) { EXPECT_FALSE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); reply->type(Common::Redis::RespType::Error); - reply->asString() = "ERR invalid acl auth"; + reply->asString() = "ERR invalid ACL auth"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("someusername", "wrongpassword"); diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index b7bafeff1fe1d..c0d0ca40d65b3 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -795,14 +795,15 @@ TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoPasswordSet) { "-ERR Client sent AUTH, but no password is set\r\n"); } -// This test sends an AUTH acl command from the fake downstream client to -// the Envoy proxy. Envoy will respond with a no-password-set error since -// no downstream_auth_username and downstream_auth_password has been set for the filter. +// This test sends an AUTH command from the fake downstream client to +// the Envoy proxy with username and password. Envoy will respond with a +// no-acl-set error since no downstream_auth_username and downstream_auth_password +// has been set for the filter. TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoAclSet) { initialize(); simpleProxyResponse(makeBulkStringArray({"auth", "someusername", "somepassword"}), - "-ERR Client sent AUTH, but no acl is set\r\n"); + "-ERR Client sent AUTH, but no ACL is set\r\n"); } // This test sends a simple Redis command to a sequence of fake upstream @@ -1096,10 +1097,10 @@ TEST_P(RedisProxyWithDownstreamAuthAclIntegrationTest, ErrorsUntilCorrectAclSent proxyResponseStep(makeBulkStringArray({"auth"}), error_response.str(), redis_client); proxyResponseStep(makeBulkStringArray({"auth", "wrongusername", "somepassword"}), - "-ERR invalid acl auth\r\n", redis_client); + "-ERR invalid ACL auth\r\n", redis_client); proxyResponseStep(makeBulkStringArray({"auth", "someusername", "wrongpassword"}), - "-ERR invalid acl auth\r\n", redis_client); + "-ERR invalid ACL auth\r\n", redis_client); proxyResponseStep(makeBulkStringArray({"get", "foo"}), "-NOAUTH Authentication required.\r\n", redis_client); diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index c59db7754d409..03b2d0df55758 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -4,6 +4,7 @@ # are allowed for any otherwise correctly spelled word. ABI ACK +ACL AES AFAICT ALPN From 7f5f27d15ce6c1d1c1c626b445988750cf0eef44 Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 3 Jun 2020 11:43:35 -0400 Subject: [PATCH 05/13] update proto coments, fix error message mismatch Signed-off-by: bibby --- .../filters/network/redis_proxy/v3/redis_proxy.proto | 12 ++++++++++++ .../filters/network/redis_proxy/v3/redis_proxy.proto | 12 ++++++++++++ .../filters/network/redis_proxy/proxy_filter.cc | 4 ++-- 3 files changed, 26 insertions(+), 2 deletions(-) diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index aac50eb2d8df2..ac03660cca5d1 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -235,6 +235,16 @@ message RedisProxy { // AUTH, but no password is set" error will be returned. config.core.v3.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + // If a username is provided an ACL style AUTH command will be required with a username and password. + // Authenticate Redis client connections locally by forcing downstream clients to issue a `Redis + // AUTH command `_ with this username and the downstream_auth_password + // before enabling any other command. If an AUTH command's username and password matches this username + // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH + // command username or password does not match this username or the downstream_auth_password, then an + // "ERR invalid ACL auth" error will be returned. If any other command is received before AUTH when this + // password is set, then a "NOAUTH Authentication required." error response will be sent to the + // client. If an AUTH command is received when the password is not set, then an "ERR Client sent + // AUTH, but no ACL is set" error will be returned. config.core.v3.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } @@ -249,5 +259,7 @@ message RedisProtocolOptions { // `_ in the server's configuration file. config.core.v3.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + // Upstream server username as defined by the `user` directive + // `_ in the server's configuration file. config.core.v3.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index b724456523431..631ee5950029e 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -230,6 +230,16 @@ message RedisProxy { // AUTH, but no password is set" error will be returned. config.core.v3.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; + // If a username is provided an ACL style AUTH command will be required with a username and password. + // Authenticate Redis client connections locally by forcing downstream clients to issue a `Redis + // AUTH command `_ with this username and the downstream_auth_password + // before enabling any other command. If an AUTH command's username and password matches this username + // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH + // command username or password does not match this username or the downstream_auth_password, then an + // "ERR invalid ACL auth" error will be returned. If any other command is received before AUTH when this + // password is set, then a "NOAUTH Authentication required." error response will be sent to the + // client. If an AUTH command is received when the password is not set, then an "ERR Client sent + // AUTH, but no ACL is set" error will be returned. config.core.v3.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; string hidden_envoy_deprecated_cluster = 2 @@ -247,5 +257,7 @@ message RedisProtocolOptions { // `_ in the server's configuration file. config.core.v3.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; + // Upstream server username as defined by the `user` directive + // `_ in the server's configuration file. config.core.v3.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index da63286f02304..ba6e1bb6b02f2 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -104,7 +104,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; if (config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR Client sent AUTH, but no username or password is set"; + response->asString() = "ERR Client sent AUTH, but no ACL is set"; } else if (username == config_->downstream_auth_username_ && password == config_->downstream_auth_password_) { response->type(Common::Redis::RespType::SimpleString); @@ -112,7 +112,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, connection_allowed_ = true; } else { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR invalid auth"; + response->asString() = "ERR invalid ACL auth"; connection_allowed_ = false; } request.onResponse(std::move(response)); From de25fec5ad025856317345b3df98dabe1c05c3d4 Mon Sep 17 00:00:00 2001 From: bibby Date: Thu, 4 Jun 2020 10:45:49 -0400 Subject: [PATCH 06/13] Remove the downstream_auth_username from the v2 API - remove the downstream_auth_username from the API - update the tests to account for the change Signed-off-by: bibby --- .../network/redis_proxy/v2/redis_proxy.proto | 6 +- .../network/redis_proxy/v2/redis_proxy.proto | 6 +- .../extensions/health_checkers/redis/redis.cc | 4 +- .../extensions/health_checkers/redis/redis.h | 1 - .../redis/redis_cluster_integration_test.cc | 58 ------ .../redis_proxy_integration_test.cc | 187 ------------------ 6 files changed, 3 insertions(+), 259 deletions(-) diff --git a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index b8a5ef87d75be..caca630fd297d 100644 --- a/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto +++ b/api/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto @@ -24,7 +24,7 @@ option (udpa.annotations.file_status).package_version_status = FROZEN; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 8] +// [#next-free-field: 7] message RedisProxy { // Redis connection pool settings. // [#next-free-field: 9] @@ -233,8 +233,6 @@ message RedisProxy { // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no password is set" error will be returned. api.v2.core.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; - - api.v2.core.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } // RedisProtocolOptions specifies Redis upstream protocol options. This object is used in @@ -244,6 +242,4 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. api.v2.core.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; - - api.v2.core.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto b/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto index b8a5ef87d75be..caca630fd297d 100644 --- a/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto +++ b/generated_api_shadow/envoy/config/filter/network/redis_proxy/v2/redis_proxy.proto @@ -24,7 +24,7 @@ option (udpa.annotations.file_status).package_version_status = FROZEN; // Redis Proxy :ref:`configuration overview `. // [#extension: envoy.filters.network.redis_proxy] -// [#next-free-field: 8] +// [#next-free-field: 7] message RedisProxy { // Redis connection pool settings. // [#next-free-field: 9] @@ -233,8 +233,6 @@ message RedisProxy { // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no password is set" error will be returned. api.v2.core.DataSource downstream_auth_password = 6 [(udpa.annotations.sensitive) = true]; - - api.v2.core.DataSource downstream_auth_username = 7 [(udpa.annotations.sensitive) = true]; } // RedisProtocolOptions specifies Redis upstream protocol options. This object is used in @@ -244,6 +242,4 @@ message RedisProtocolOptions { // Upstream server password as defined by the `requirepass` directive // `_ in the server's configuration file. api.v2.core.DataSource auth_password = 1 [(udpa.annotations.sensitive) = true]; - - api.v2.core.DataSource auth_username = 2 [(udpa.annotations.sensitive) = true]; } diff --git a/source/extensions/health_checkers/redis/redis.cc b/source/extensions/health_checkers/redis/redis.cc index 96d8766d180e0..2f403b9742e58 100644 --- a/source/extensions/health_checkers/redis/redis.cc +++ b/source/extensions/health_checkers/redis/redis.cc @@ -19,8 +19,6 @@ RedisHealthChecker::RedisHealthChecker( Extensions::NetworkFilters::Common::Redis::Client::ClientFactory& client_factory) : HealthCheckerImplBase(cluster, config, dispatcher, runtime, random, std::move(event_logger)), client_factory_(client_factory), key_(redis_config.key()), - auth_username_( - NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authUsername(cluster.info(), api)), auth_password_(NetworkFilters::RedisProxy::ProtocolOptionsConfigImpl::authPassword( cluster.info(), api)) { if (!key_.empty()) { @@ -67,7 +65,7 @@ void RedisHealthChecker::RedisActiveHealthCheckSession::onInterval() { if (!client_) { client_ = parent_.client_factory_.create( host_, parent_.dispatcher_, *this, redis_command_stats_, - parent_.cluster_.info()->statsScope(), parent_.auth_username_, parent_.auth_password_); + parent_.cluster_.info()->statsScope(), "", parent_.auth_password_); client_->addConnectionCallbacks(*this); } diff --git a/source/extensions/health_checkers/redis/redis.h b/source/extensions/health_checkers/redis/redis.h index 808c8ff26a7e4..6284c475eda59 100644 --- a/source/extensions/health_checkers/redis/redis.h +++ b/source/extensions/health_checkers/redis/redis.h @@ -125,7 +125,6 @@ class RedisHealthChecker : public Upstream::HealthCheckerImplBase { Extensions::NetworkFilters::Common::Redis::Client::ClientFactory& client_factory_; Type type_; const std::string key_; - const std::string auth_username_; const std::string auth_password_; }; diff --git a/test/extensions/clusters/redis/redis_cluster_integration_test.cc b/test/extensions/clusters/redis/redis_cluster_integration_test.cc index 4e40091a35701..bd535a3cf0520 100644 --- a/test/extensions/clusters/redis/redis_cluster_integration_test.cc +++ b/test/extensions/clusters/redis/redis_cluster_integration_test.cc @@ -122,19 +122,6 @@ const std::string& testConfigWithAuth() { )EOF"); } -// This is the basic redis_proxy configuration with an upstream -// authentication username and password specified. - -const std::string& testConfigWithAuthAcl() { - CONSTRUCT_ON_FIRST_USE(std::string, testConfig() + R"EOF( - typed_extension_protocol_options: - envoy.filters.network.redis_proxy: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions - auth_username: { inline_string: someusername } - auth_password: { inline_string: somepassword } -)EOF"); -} - // This function encodes commands as an array of bulkstrings as transmitted by Redis clients to // Redis servers, according to the Redis protocol. std::string makeBulkStringArray(std::vector&& command_strings) { @@ -379,13 +366,6 @@ class RedisClusterWithAuthIntegrationTest : public RedisClusterIntegrationTest { : RedisClusterIntegrationTest(config, num_upstreams) {} }; -class RedisClusterWithAuthAclIntegrationTest : public RedisClusterIntegrationTest { -public: - RedisClusterWithAuthAclIntegrationTest(const std::string& config = testConfigWithAuthAcl(), - int num_upstreams = 2) - : RedisClusterIntegrationTest(config, num_upstreams) {} -}; - class RedisClusterWithReadPolicyIntegrationTest : public RedisClusterIntegrationTest { public: RedisClusterWithReadPolicyIntegrationTest(const std::string& config = testConfigWithReadPolicy(), @@ -408,10 +388,6 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterWithAuthIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -INSTANTIATE_TEST_SUITE_P(IpVersions, RedisClusterWithAuthAclIntegrationTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); - // This test sends a simple "get foo" command from a fake // downstream client through the proxy to a fake upstream // Redis cluster with a single slot with master and replica. @@ -582,40 +558,6 @@ TEST_P(RedisClusterWithAuthIntegrationTest, SingleSlotMasterReplica) { EXPECT_TRUE(fake_upstream_connection->close()); } -// This test sends a simple "get foo" command from a fake -// downstream client through the proxy to a fake upstream -// Redis cluster with a single slot with master and replica. -// The fake server sends a valid response back to the client. -// The request and response should make it through the envoy -// proxy server code unchanged. -// -// In this scenario, the fake server will receive 2 auth commands with username and password: -// one as part of a topology discovery connection (before sending a -// "cluster slots" command), and one to authenticate the connection -// that carries the "get foo" request. - -TEST_P(RedisClusterWithAuthAclIntegrationTest, SingleSlotMasterReplica) { - random_index_ = 0; - - on_server_init_function_ = [this]() { - std::string cluster_slot_response = singleSlotMasterReplica( - fake_upstreams_[0]->localAddress()->ip(), fake_upstreams_[1]->localAddress()->ip()); - expectCallClusterSlot(0, cluster_slot_response, "someusername", "somepassword"); - }; - - initialize(); - - IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); - FakeRawConnectionPtr fake_upstream_connection; - - roundtripToUpstreamStep(fake_upstreams_[random_index_], makeBulkStringArray({"get", "foo"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection, "someusername", - "somepassword"); - - redis_client->close(); - EXPECT_TRUE(fake_upstream_connection->close()); -} - // This test show the test proxy's multi-stage response to an error from an upstream fake // redis server. The proxy will connect to the first fake upstream server to rediscover the // cluster's topology using a "cluster slots" command. diff --git a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc index c0d0ca40d65b3..6df533cef7bd6 100644 --- a/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc +++ b/test/extensions/filters/network/redis_proxy/redis_proxy_integration_test.cc @@ -273,95 +273,6 @@ const std::string CONFIG_WITH_ROUTES_AND_AUTH_PASSWORDS = R"EOF( cluster: cluster_2 )EOF"; -const std::string CONFIG_WITH_DOWNSTREAM_AUTH_ACL_SET = CONFIG + R"EOF( - downstream_auth_username: { inline_string: someusername } - downstream_auth_password: { inline_string: somepassword } -)EOF"; - -const std::string CONFIG_WITH_ROUTES_AND_AUTH_ACLS = R"EOF( -admin: - access_log_path: /dev/null - address: - socket_address: - address: 127.0.0.1 - port_value: 0 -static_resources: - clusters: - - name: cluster_0 - type: STATIC - typed_extension_protocol_options: - envoy.filters.network.redis_proxy: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions - auth_username: { inline_string: cluster_0_username } - auth_password: { inline_string: cluster_0_password } - lb_policy: RANDOM - load_assignment: - cluster_name: cluster_0 - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: 127.0.0.1 - port_value: 0 - - name: cluster_1 - type: STATIC - lb_policy: RANDOM - typed_extension_protocol_options: - envoy.filters.network.redis_proxy: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions - auth_username: { inline_string: cluster_1_username } - auth_password: { inline_string: cluster_1_password } - load_assignment: - cluster_name: cluster_1 - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: 127.0.0.1 - port_value: 1 - - name: cluster_2 - type: STATIC - typed_extension_protocol_options: - envoy.filters.network.redis_proxy: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProtocolOptions - auth_username: { inline_string: cluster_2_username } - auth_password: { inline_string: cluster_2_password } - lb_policy: RANDOM - load_assignment: - cluster_name: cluster_2 - endpoints: - - lb_endpoints: - - endpoint: - address: - socket_address: - address: 127.0.0.1 - port_value: 2 - listeners: - name: listener_0 - address: - socket_address: - address: 127.0.0.1 - port_value: 0 - filter_chains: - filters: - name: redis - typed_config: - "@type": type.googleapis.com/envoy.config.filter.network.redis_proxy.v2.RedisProxy - stat_prefix: redis_stats - settings: - op_timeout: 5s - prefix_routes: - catch_all_route: - cluster: cluster_0 - routes: - - prefix: "foo:" - cluster: cluster_1 - - prefix: "baz:" - cluster: cluster_2 -)EOF"; - // This function encodes commands as an array of bulkstrings as transmitted by Redis clients to // Redis servers, according to the Redis protocol. std::string makeBulkStringArray(std::vector&& command_strings) { @@ -513,24 +424,12 @@ class RedisProxyWithDownstreamAuthIntegrationTest : public RedisProxyIntegration : RedisProxyIntegrationTest(CONFIG_WITH_DOWNSTREAM_AUTH_PASSWORD_SET, 2) {} }; -class RedisProxyWithDownstreamAuthAclIntegrationTest : public RedisProxyIntegrationTest { -public: - RedisProxyWithDownstreamAuthAclIntegrationTest() - : RedisProxyIntegrationTest(CONFIG_WITH_DOWNSTREAM_AUTH_ACL_SET, 2) {} -}; - class RedisProxyWithRoutesAndAuthPasswordsIntegrationTest : public RedisProxyIntegrationTest { public: RedisProxyWithRoutesAndAuthPasswordsIntegrationTest() : RedisProxyIntegrationTest(CONFIG_WITH_ROUTES_AND_AUTH_PASSWORDS, 3) {} }; -class RedisProxyWithRoutesAndAuthAclsIntegrationTest : public RedisProxyIntegrationTest { -public: - RedisProxyWithRoutesAndAuthAclsIntegrationTest() - : RedisProxyIntegrationTest(CONFIG_WITH_ROUTES_AND_AUTH_ACLS, 3) {} -}; - class RedisProxyWithMirrorsIntegrationTest : public RedisProxyIntegrationTest { public: RedisProxyWithMirrorsIntegrationTest() : RedisProxyIntegrationTest(CONFIG_WITH_MIRROR, 6) {} @@ -562,18 +461,10 @@ INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyWithDownstreamAuthIntegrationTest testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyWithDownstreamAuthAclIntegrationTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); - INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyWithRoutesAndAuthPasswordsIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); -INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyWithRoutesAndAuthAclsIntegrationTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), - TestUtility::ipTestParamsToString); - INSTANTIATE_TEST_SUITE_P(IpVersions, RedisProxyWithMirrorsIntegrationTest, testing::ValuesIn(TestEnvironment::getIpVersionsForTest()), TestUtility::ipTestParamsToString); @@ -795,17 +686,6 @@ TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoPasswordSet) { "-ERR Client sent AUTH, but no password is set\r\n"); } -// This test sends an AUTH command from the fake downstream client to -// the Envoy proxy with username and password. Envoy will respond with a -// no-acl-set error since no downstream_auth_username and downstream_auth_password -// has been set for the filter. - -TEST_P(RedisProxyIntegrationTest, DownstreamAuthWhenNoAclSet) { - initialize(); - simpleProxyResponse(makeBulkStringArray({"auth", "someusername", "somepassword"}), - "-ERR Client sent AUTH, but no ACL is set\r\n"); -} - // This test sends a simple Redis command to a sequence of fake upstream // Redis servers. The first server replies with a MOVED or ASK redirection // error that specifies the second upstream server in the static configuration @@ -1078,73 +958,6 @@ TEST_P(RedisProxyWithRoutesAndAuthPasswordsIntegrationTest, TransparentAuthentic redis_client->close(); } -// This test verifies that a client connection cannot issue a command to an upstream -// server until it supplies a valid Redis AUTH command when downstream_auth_username and -// downstream_auth_password is set for the redis_proxy filter. It also verifies the errors sent by -// the proxy when no username or password or the wrong username or password is received. - -TEST_P(RedisProxyWithDownstreamAuthAclIntegrationTest, ErrorsUntilCorrectAclSent) { - initialize(); - - IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); - FakeRawConnectionPtr fake_upstream_connection; - - proxyResponseStep(makeBulkStringArray({"get", "foo"}), "-NOAUTH Authentication required.\r\n", - redis_client); - - std::stringstream error_response; - error_response << "-" << RedisCmdSplitter::Response::get().InvalidRequest << "\r\n"; - proxyResponseStep(makeBulkStringArray({"auth"}), error_response.str(), redis_client); - - proxyResponseStep(makeBulkStringArray({"auth", "wrongusername", "somepassword"}), - "-ERR invalid ACL auth\r\n", redis_client); - - proxyResponseStep(makeBulkStringArray({"auth", "someusername", "wrongpassword"}), - "-ERR invalid ACL auth\r\n", redis_client); - - proxyResponseStep(makeBulkStringArray({"get", "foo"}), "-NOAUTH Authentication required.\r\n", - redis_client); - - proxyResponseStep(makeBulkStringArray({"auth", "someusername", "somepassword"}), "+OK\r\n", - redis_client); - - roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "foo"}), "$3\r\nbar\r\n", - redis_client, fake_upstream_connection, "", ""); - - EXPECT_TRUE(fake_upstream_connection->close()); - redis_client->close(); -} - -// This test verifies that upstream server connections are transparently authenticated if an -// auth_username and auth_password is specified for each cluster. - -TEST_P(RedisProxyWithRoutesAndAuthAclsIntegrationTest, TransparentAuthentication) { - initialize(); - - IntegrationTcpClientPtr redis_client = makeTcpConnection(lookupPort("redis_proxy")); - std::array fake_upstream_connection; - - // roundtrip to cluster_0 (catch_all route) - roundtripToUpstreamStep(fake_upstreams_[0], makeBulkStringArray({"get", "toto"}), "$3\r\nbar\r\n", - redis_client, fake_upstream_connection[0], "cluster_0_username", - "cluster_0_password"); - - // roundtrip to cluster_1 (prefix "foo:" route) - roundtripToUpstreamStep(fake_upstreams_[1], makeBulkStringArray({"get", "foo:123"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection[1], - "cluster_1_username", "cluster_1_password"); - - // roundtrip to cluster_2 (prefix "baz:" route) - roundtripToUpstreamStep(fake_upstreams_[2], makeBulkStringArray({"get", "baz:123"}), - "$3\r\nbar\r\n", redis_client, fake_upstream_connection[2], - "cluster_2_username", "cluster_2_password"); - - EXPECT_TRUE(fake_upstream_connection[0]->close()); - EXPECT_TRUE(fake_upstream_connection[1]->close()); - EXPECT_TRUE(fake_upstream_connection[2]->close()); - redis_client->close(); -} - TEST_P(RedisProxyWithMirrorsIntegrationTest, MirroredCatchAllRequest) { initialize(); From 6d93798d7e61532f154e895d2a9ac2f81c04cc82 Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 10 Jun 2020 10:59:26 -0400 Subject: [PATCH 07/13] Addressing Code Review comments - Update ACL error message to match what Redis returns - Add handling for empty username and "default" being synonymous in Redis 6 Signed-off-by: bibby --- .../filters/network/redis_proxy/v3/redis_proxy.proto | 2 +- .../filters/network/redis_proxy/v3/redis_proxy.proto | 2 +- .../filters/network/redis_proxy/proxy_filter.cc | 10 ++++++++-- .../filters/network/redis_proxy/proxy_filter_test.cc | 6 +++--- 4 files changed, 13 insertions(+), 7 deletions(-) diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index ac03660cca5d1..b30dd878f568d 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -241,7 +241,7 @@ message RedisProxy { // before enabling any other command. If an AUTH command's username and password matches this username // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH // command username or password does not match this username or the downstream_auth_password, then an - // "ERR invalid ACL auth" error will be returned. If any other command is received before AUTH when this + // "WRONGPASS invalid username-password pair" error will be returned. If any other command is received before AUTH when this // password is set, then a "NOAUTH Authentication required." error response will be sent to the // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no ACL is set" error will be returned. diff --git a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index 631ee5950029e..df815be341811 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -236,7 +236,7 @@ message RedisProxy { // before enabling any other command. If an AUTH command's username and password matches this username // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH // command username or password does not match this username or the downstream_auth_password, then an - // "ERR invalid ACL auth" error will be returned. If any other command is received before AUTH when this + // "WRONGPASS invalid username-password pair" error will be returned. If any other command is received before AUTH when this // password is set, then a "NOAUTH Authentication required." error response will be sent to the // client. If an AUTH command is received when the password is not set, then an "ERR Client sent // AUTH, but no ACL is set" error will be returned. diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index ba6e1bb6b02f2..136fd4c6a196a 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -104,7 +104,13 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, Common::Redis::RespValuePtr response{new Common::Redis::RespValue()}; if (config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR Client sent AUTH, but no ACL is set"; + response->asString() = "ERR Client sent AUTH, but no username-password pair is set"; + } else if ("" == config_->downstream_auth_username_ && username == "default" && + password == config_->downstream_auth_password_) { + // empty username and "default" are synonymous in Redis 6 ACLs + response->type(Common::Redis::RespType::SimpleString); + response->asString() = "OK"; + connection_allowed_ = true; } else if (username == config_->downstream_auth_username_ && password == config_->downstream_auth_password_) { response->type(Common::Redis::RespType::SimpleString); @@ -112,7 +118,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, connection_allowed_ = true; } else { response->type(Common::Redis::RespType::Error); - response->asString() = "ERR invalid ACL auth"; + response->asString() = "WRONGPASS invalid username-password pair"; connection_allowed_ = false; } request.onResponse(std::move(response)); diff --git a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc index 61f8d7b82e17d..f094c02b665a0 100644 --- a/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc +++ b/test/extensions/filters/network/redis_proxy/proxy_filter_test.cc @@ -347,7 +347,7 @@ TEST_F(RedisProxyFilterTest, AuthAclWhenNotRequired) { EXPECT_TRUE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr error(new Common::Redis::RespValue()); error->type(Common::Redis::RespType::Error); - error->asString() = "ERR Client sent AUTH, but no ACL is set"; + error->asString() = "ERR Client sent AUTH, but no username-password pair is set"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*error)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("foo", "bar"); @@ -489,7 +489,7 @@ TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclUsernameIncorrect) { EXPECT_FALSE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); reply->type(Common::Redis::RespType::Error); - reply->asString() = "ERR invalid ACL auth"; + reply->asString() = "WRONGPASS invalid username-password pair"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("wrongusername", "somepassword"); @@ -516,7 +516,7 @@ TEST_F(RedisProxyFilterWithAuthAclTest, AuthAclPasswordIncorrect) { EXPECT_FALSE(callbacks.connectionAllowed()); Common::Redis::RespValuePtr reply(new Common::Redis::RespValue()); reply->type(Common::Redis::RespType::Error); - reply->asString() = "ERR invalid ACL auth"; + reply->asString() = "WRONGPASS invalid username-password pair"; EXPECT_CALL(*encoder_, encode(Eq(ByRef(*reply)), _)); EXPECT_CALL(filter_callbacks_.connection_, write(_, _)); callbacks.onAuth("someusername", "wrongpassword"); From 4fb45017167f098185859b4a4571aec04855515e Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 10 Jun 2020 12:22:00 -0400 Subject: [PATCH 08/13] Adding release notes for Redis ACL support to the current version Signed-off-by: bibby --- docs/root/version_history/current.rst | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 7d4fcd9a3f5df..2547e17c37f67 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -72,6 +72,7 @@ New Features in :ref:`client_features` field. * network filters: added a :ref:`postgres proxy filter `. * network filters: added a :ref:`rocketmq proxy filter `. +* redis: added acl support :ref:`downstream_auth_username ` for downstream client ACL authentication, and :ref:`auth_username ` to configure authentication usernames for upstream Redis 6+ server clusters with ACL enabled. * request_id: added to :ref:`always_set_request_id_in_response setting ` to set :ref:`x-request-id ` header in response even if tracing is not forced. From fa05664e6bde802a81d9917052c81999312f4483 Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 10 Jun 2020 12:23:25 -0400 Subject: [PATCH 09/13] fix a typo Signed-off-by: bibby --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 2547e17c37f67..9b546320ec7b6 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -72,7 +72,7 @@ New Features in :ref:`client_features` field. * network filters: added a :ref:`postgres proxy filter `. * network filters: added a :ref:`rocketmq proxy filter `. -* redis: added acl support :ref:`downstream_auth_username ` for downstream client ACL authentication, and :ref:`auth_username ` to configure authentication usernames for upstream Redis 6+ server clusters with ACL enabled. +* redis: added acl support :ref:`downstream_auth_username ` for downstream client ACL authentication, and :ref:`auth_username ` to configure authentication usernames for upstream Redis 6+ server clusters with ACL enabled. * request_id: added to :ref:`always_set_request_id_in_response setting ` to set :ref:`x-request-id ` header in response even if tracing is not forced. From c7b97097c5975c5dcce01c571a50571028fdc459 Mon Sep 17 00:00:00 2001 From: bibby Date: Wed, 10 Jun 2020 16:11:13 -0400 Subject: [PATCH 10/13] add WRONGPASS to the spelling dictionary Signed-off-by: bibby --- tools/spelling/spelling_dictionary.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/spelling/spelling_dictionary.txt b/tools/spelling/spelling_dictionary.txt index 03b2d0df55758..68e64535a80e3 100644 --- a/tools/spelling/spelling_dictionary.txt +++ b/tools/spelling/spelling_dictionary.txt @@ -336,6 +336,7 @@ WASM WAVM WIP WKT +WRONGPASS WRR WS WSA From 2b177a945f255cf6bec637bc24c2ea22f93fce1e Mon Sep 17 00:00:00 2001 From: bibby Date: Thu, 11 Jun 2020 14:34:32 -0400 Subject: [PATCH 11/13] Check for config_->downstream_auth_username_.empty() rather than == config_->downstream_auth_username_ Signed-off-by: bibby --- source/extensions/filters/network/redis_proxy/proxy_filter.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/source/extensions/filters/network/redis_proxy/proxy_filter.cc b/source/extensions/filters/network/redis_proxy/proxy_filter.cc index 136fd4c6a196a..aa2f558cc51af 100644 --- a/source/extensions/filters/network/redis_proxy/proxy_filter.cc +++ b/source/extensions/filters/network/redis_proxy/proxy_filter.cc @@ -105,7 +105,7 @@ void ProxyFilter::onAuth(PendingRequest& request, const std::string& username, if (config_->downstream_auth_username_.empty() && config_->downstream_auth_password_.empty()) { response->type(Common::Redis::RespType::Error); response->asString() = "ERR Client sent AUTH, but no username-password pair is set"; - } else if ("" == config_->downstream_auth_username_ && username == "default" && + } else if (config_->downstream_auth_username_.empty() && username == "default" && password == config_->downstream_auth_password_) { // empty username and "default" are synonymous in Redis 6 ACLs response->type(Common::Redis::RespType::SimpleString); From 0037bf2b635951e4c3b507a5875d7cbf5de52776 Mon Sep 17 00:00:00 2001 From: bibby Date: Fri, 12 Jun 2020 11:36:16 -0400 Subject: [PATCH 12/13] Add ** around the proximal reference to downstream_auth_password Signed-off-by: bibby --- .../filters/network/redis_proxy/v3/redis_proxy.proto | 6 +++--- .../filters/network/redis_proxy/v3/redis_proxy.proto | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index b30dd878f568d..658ac1c16b8ca 100644 --- a/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/api/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -237,10 +237,10 @@ message RedisProxy { // If a username is provided an ACL style AUTH command will be required with a username and password. // Authenticate Redis client connections locally by forcing downstream clients to issue a `Redis - // AUTH command `_ with this username and the downstream_auth_password + // AUTH command `_ with this username and the *downstream_auth_password* // before enabling any other command. If an AUTH command's username and password matches this username - // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH - // command username or password does not match this username or the downstream_auth_password, then an + // and the *downstream_auth_password* , an "OK" response will be returned to the client. If the AUTH + // command username or password does not match this username or the *downstream_auth_password*, then an // "WRONGPASS invalid username-password pair" error will be returned. If any other command is received before AUTH when this // password is set, then a "NOAUTH Authentication required." error response will be sent to the // client. If an AUTH command is received when the password is not set, then an "ERR Client sent diff --git a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto index df815be341811..098f5f4a2ea97 100644 --- a/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto +++ b/generated_api_shadow/envoy/extensions/filters/network/redis_proxy/v3/redis_proxy.proto @@ -232,10 +232,10 @@ message RedisProxy { // If a username is provided an ACL style AUTH command will be required with a username and password. // Authenticate Redis client connections locally by forcing downstream clients to issue a `Redis - // AUTH command `_ with this username and the downstream_auth_password + // AUTH command `_ with this username and the *downstream_auth_password* // before enabling any other command. If an AUTH command's username and password matches this username - // and the downstream_auth_password , an "OK" response will be returned to the client. If the AUTH - // command username or password does not match this username or the downstream_auth_password, then an + // and the *downstream_auth_password* , an "OK" response will be returned to the client. If the AUTH + // command username or password does not match this username or the *downstream_auth_password*, then an // "WRONGPASS invalid username-password pair" error will be returned. If any other command is received before AUTH when this // password is set, then a "NOAUTH Authentication required." error response will be sent to the // client. If an AUTH command is received when the password is not set, then an "ERR Client sent From cf0bcad4cc4f1736c9b077c1e1de880d2e29864d Mon Sep 17 00:00:00 2001 From: bibby Date: Tue, 23 Jun 2020 11:37:07 -0400 Subject: [PATCH 13/13] Fix ordering (alpha) Signed-off-by: bibby --- docs/root/version_history/current.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/root/version_history/current.rst b/docs/root/version_history/current.rst index 17cc5ea9ad213..08ccc6e673f53 100644 --- a/docs/root/version_history/current.rst +++ b/docs/root/version_history/current.rst @@ -97,8 +97,8 @@ New Features * metrics service: added added :ref:`API version ` to explicitly set the version of gRPC service endpoint and message to be used. * network filters: added a :ref:`postgres proxy filter `. * network filters: added a :ref:`rocketmq proxy filter `. -* redis: added acl support :ref:`downstream_auth_username ` for downstream client ACL authentication, and :ref:`auth_username ` to configure authentication usernames for upstream Redis 6+ server clusters with ACL enabled. * ratelimit: added :ref:`API version ` to explicitly set the version of gRPC service endpoint and message to be used. +* redis: added acl support :ref:`downstream_auth_username ` for downstream client ACL authentication, and :ref:`auth_username ` to configure authentication usernames for upstream Redis 6+ server clusters with ACL enabled. * request_id: added to :ref:`always_set_request_id_in_response setting ` to set :ref:`x-request-id ` header in response even if tracing is not forced.