diff --git a/api/envoy/extensions/filters/http/jwt_authn/v3/config.proto b/api/envoy/extensions/filters/http/jwt_authn/v3/config.proto index a79e3382d6334..db81f847d7bae 100644 --- a/api/envoy/extensions/filters/http/jwt_authn/v3/config.proto +++ b/api/envoy/extensions/filters/http/jwt_authn/v3/config.proto @@ -258,6 +258,37 @@ message RemoteJwks { // * Jwks is ready when the requests come, not need to wait for the Jwks fetching. // JwksAsyncFetch async_fetch = 3; + + // Retry policy for fetching Jwks. optional. turned off by default. + // + // For example: + // + // .. code-block:: yaml + // + // retry_policy: + // retry_back_off: + // base_interval: 0.01s + // max_interval: 20s + // num_retries: 10 + // + // will yield a randomized truncated exponential backoff policy with an initial delay of 10ms + // 10 maximum attempts spaced at most 20s seconds. + // + // .. code-block:: yaml + // + // retry_policy: + // num_retries:1 + // + // uses the default :ref:`retry backoff strategy `. + // with the default base interval is 1000 milliseconds. and the default maximum interval of 10 times the base interval. + // + // if num_retries is omitted, the default is to allow only one retry. + // + // + // If enabled, the retry policy will apply to all Jwks fetching approaches, e.g. on demand or asynchronously in background. + // + // + config.core.v3.RetryPolicy retry_policy = 4; } // Fetch Jwks asynchronously in the main thread when the filter config is parsed. diff --git a/api/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto b/api/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto index 82f6bef04eae4..3a65b6a64cc8c 100644 --- a/api/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto +++ b/api/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto @@ -258,6 +258,37 @@ message RemoteJwks { // * Jwks is ready when the requests come, not need to wait for the Jwks fetching. // JwksAsyncFetch async_fetch = 3; + + // Retry policy for fetching Jwks. optional. turned off by default. + // + // For example: + // + // .. code-block:: yaml + // + // retry_policy: + // retry_back_off: + // base_interval: 0.01s + // max_interval: 20s + // num_retries: 10 + // + // will yield a randomized truncated exponential backoff policy with an initial delay of 10ms + // 10 maximum attempts spaced at most 20s seconds. + // + // .. code-block:: yaml + // + // retry_policy: + // num_retries:1 + // + // uses the default :ref:`retry backoff strategy `. + // with the default base interval is 1000 milliseconds. and the default maximum interval of 10 times the base interval. + // + // if num_retries is omitted, the default is to allow only one retry. + // + // + // If enabled, the retry policy will apply to all Jwks fetching approaches, e.g. on demand or asynchronously in background. + // + // + config.core.v4alpha.RetryPolicy retry_policy = 4; } // Fetch Jwks asynchronously in the main thread when the filter config is parsed. diff --git a/envoy/http/async_client.h b/envoy/http/async_client.h index 81058137ce6a3..2e1917aaa413c 100644 --- a/envoy/http/async_client.h +++ b/envoy/http/async_client.h @@ -213,6 +213,12 @@ class AsyncClient { return *this; } + // this should be done with setBufferedBodyForRetry=true ? + StreamOptions& setRetryPolicy(const envoy::config::route::v3::RetryPolicy& p) { + retry_policy = p; + return *this; + } + // For gmock test bool operator==(const StreamOptions& src) const { return timeout == src.timeout && buffer_body_for_retry == src.buffer_body_for_retry && @@ -239,6 +245,8 @@ class AsyncClient { ParentContext parent_context; envoy::config::core::v3::Metadata metadata; + + absl::optional retry_policy; }; /** @@ -274,6 +282,10 @@ class AsyncClient { StreamOptions::setMetadata(m); return *this; } + RequestOptions& setRetryPolicy(const envoy::config::route::v3::RetryPolicy& p) { + StreamOptions::setRetryPolicy(p); + return *this; + } RequestOptions& setParentSpan(Tracing::Span& parent_span) { parent_span_ = &parent_span; return *this; diff --git a/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v3/config.proto b/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v3/config.proto index a79e3382d6334..db81f847d7bae 100644 --- a/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v3/config.proto +++ b/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v3/config.proto @@ -258,6 +258,37 @@ message RemoteJwks { // * Jwks is ready when the requests come, not need to wait for the Jwks fetching. // JwksAsyncFetch async_fetch = 3; + + // Retry policy for fetching Jwks. optional. turned off by default. + // + // For example: + // + // .. code-block:: yaml + // + // retry_policy: + // retry_back_off: + // base_interval: 0.01s + // max_interval: 20s + // num_retries: 10 + // + // will yield a randomized truncated exponential backoff policy with an initial delay of 10ms + // 10 maximum attempts spaced at most 20s seconds. + // + // .. code-block:: yaml + // + // retry_policy: + // num_retries:1 + // + // uses the default :ref:`retry backoff strategy `. + // with the default base interval is 1000 milliseconds. and the default maximum interval of 10 times the base interval. + // + // if num_retries is omitted, the default is to allow only one retry. + // + // + // If enabled, the retry policy will apply to all Jwks fetching approaches, e.g. on demand or asynchronously in background. + // + // + config.core.v3.RetryPolicy retry_policy = 4; } // Fetch Jwks asynchronously in the main thread when the filter config is parsed. diff --git a/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto b/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto index 82f6bef04eae4..3a65b6a64cc8c 100644 --- a/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto +++ b/generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto @@ -258,6 +258,37 @@ message RemoteJwks { // * Jwks is ready when the requests come, not need to wait for the Jwks fetching. // JwksAsyncFetch async_fetch = 3; + + // Retry policy for fetching Jwks. optional. turned off by default. + // + // For example: + // + // .. code-block:: yaml + // + // retry_policy: + // retry_back_off: + // base_interval: 0.01s + // max_interval: 20s + // num_retries: 10 + // + // will yield a randomized truncated exponential backoff policy with an initial delay of 10ms + // 10 maximum attempts spaced at most 20s seconds. + // + // .. code-block:: yaml + // + // retry_policy: + // num_retries:1 + // + // uses the default :ref:`retry backoff strategy `. + // with the default base interval is 1000 milliseconds. and the default maximum interval of 10 times the base interval. + // + // if num_retries is omitted, the default is to allow only one retry. + // + // + // If enabled, the retry policy will apply to all Jwks fetching approaches, e.g. on demand or asynchronously in background. + // + // + config.core.v4alpha.RetryPolicy retry_policy = 4; } // Fetch Jwks asynchronously in the main thread when the filter config is parsed. diff --git a/source/common/http/async_client_impl.cc b/source/common/http/async_client_impl.cc index 5c09a9f6cddfc..f1638e928d71b 100644 --- a/source/common/http/async_client_impl.cc +++ b/source/common/http/async_client_impl.cc @@ -19,7 +19,6 @@ const std::vector> AsyncStreamImpl::NullRateLimitPolicy::rate_limit_policy_entry_; const AsyncStreamImpl::NullHedgePolicy AsyncStreamImpl::RouteEntryImpl::hedge_policy_; const AsyncStreamImpl::NullRateLimitPolicy AsyncStreamImpl::RouteEntryImpl::rate_limit_policy_; -const AsyncStreamImpl::NullRetryPolicy AsyncStreamImpl::RouteEntryImpl::retry_policy_; const Router::InternalRedirectPolicyImpl AsyncStreamImpl::RouteEntryImpl::internal_redirect_policy_; const std::vector AsyncStreamImpl::RouteEntryImpl::shadow_policies_; const AsyncStreamImpl::NullVirtualHost AsyncStreamImpl::RouteEntryImpl::virtual_host_; @@ -86,7 +85,7 @@ AsyncStreamImpl::AsyncStreamImpl(AsyncClientImpl& parent, AsyncClient::StreamCal stream_info_(Protocol::Http11, parent.dispatcher().timeSource(), nullptr), tracing_config_(Tracing::EgressConfig::get()), route_(std::make_shared(parent_.cluster_->name(), options.timeout, - options.hash_policy)), + options.hash_policy, options.retry_policy)), send_xff_(options.send_xff) { stream_info_.dynamicMetadata().MergeFrom(options.metadata); diff --git a/source/common/http/async_client_impl.h b/source/common/http/async_client_impl.h index d7193564ef6d9..5f45c023b3075 100644 --- a/source/common/http/async_client_impl.h +++ b/source/common/http/async_client_impl.h @@ -35,6 +35,8 @@ #include "source/common/common/empty_string.h" #include "source/common/common/linked_object.h" #include "source/common/http/message_impl.h" +#include "source/common/protobuf/message_validator_impl.h" +#include "source/common/router/config_impl.h" #include "source/common/router/router.h" #include "source/common/stream_info/stream_info_impl.h" #include "source/common/tracing/http_tracer_impl.h" @@ -211,11 +213,19 @@ class AsyncStreamImpl : public AsyncClient::Stream, RouteEntryImpl( const std::string& cluster_name, const absl::optional& timeout, const Protobuf::RepeatedPtrField& - hash_policy) + hash_policy, + const absl::optional& retry_policy) : cluster_name_(cluster_name), timeout_(timeout) { if (!hash_policy.empty()) { hash_policy_ = std::make_unique(hash_policy); } + if (retry_policy.has_value()) { + // ProtobufMessage::getStrictValidationVisitor() ? how often do we do this? + retry_policy_ = std::make_unique( + retry_policy.value(), ProtobufMessage::getNullValidationVisitor()); + } else { + retry_policy_ = std::make_unique(); + } } // Router::RouteEntry @@ -243,7 +253,7 @@ class AsyncStreamImpl : public AsyncClient::Stream, return Upstream::ResourcePriority::Default; } const Router::RateLimitPolicy& rateLimitPolicy() const override { return rate_limit_policy_; } - const Router::RetryPolicy& retryPolicy() const override { return retry_policy_; } + const Router::RetryPolicy& retryPolicy() const override { return *retry_policy_; } const Router::InternalRedirectPolicy& internalRedirectPolicy() const override { return internal_redirect_policy_; } @@ -307,9 +317,10 @@ class AsyncStreamImpl : public AsyncClient::Stream, const Router::RouteEntry::UpgradeMap& upgradeMap() const override { return upgrade_map_; } const std::string& routeName() const override { return route_name_; } std::unique_ptr hash_policy_; + std::unique_ptr retry_policy_; + static const NullHedgePolicy hedge_policy_; static const NullRateLimitPolicy rate_limit_policy_; - static const NullRetryPolicy retry_policy_; static const Router::InternalRedirectPolicyImpl internal_redirect_policy_; static const std::vector shadow_policies_; static const NullVirtualHost virtual_host_; @@ -330,8 +341,9 @@ class AsyncStreamImpl : public AsyncClient::Stream, RouteImpl(const std::string& cluster_name, const absl::optional& timeout, const Protobuf::RepeatedPtrField& - hash_policy) - : route_entry_(cluster_name, timeout, hash_policy) {} + hash_policy, + const absl::optional& retry_policy) + : route_entry_(cluster_name, timeout, hash_policy, retry_policy) {} // Router::Route const Router::DirectResponseEntry* directResponseEntry() const override { return nullptr; } diff --git a/source/extensions/filters/http/common/BUILD b/source/extensions/filters/http/common/BUILD index b8cffa1c6366d..1f619d332ea9b 100644 --- a/source/extensions/filters/http/common/BUILD +++ b/source/extensions/filters/http/common/BUILD @@ -38,6 +38,7 @@ envoy_cc_library( "//envoy/upstream:cluster_manager_interface", "//source/common/http:utility_lib", "@envoy_api//envoy/config/core/v3:pkg_cc_proto", + "@envoy_api//envoy/extensions/filters/http/jwt_authn/v3:pkg_cc_proto", ], ) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index b5f0048b10211..4b2e50594fd13 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -5,59 +5,127 @@ #include "source/common/common/enum_to_int.h" #include "source/common/http/headers.h" #include "source/common/http/utility.h" +#include "source/common/protobuf/utility.h" #include "jwt_verify_lib/status.h" +using envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks; + namespace Envoy { namespace Extensions { namespace HttpFilters { namespace Common { namespace { +// Parameters of the jittered backoff strategy. +static constexpr uint32_t RetryInitialDelayMilliseconds = 1000; +static constexpr uint32_t RetryMaxDelayMilliseconds = 10 * 1000; + +/** + * @details construct an envoy.config.route.v3.RetryPolicy protobuf message + * from a less feature rich envoy.config.core.v3.RetryPolicy one. + * + * this is about limiting the user's possibilities. + * just doing truncated exponential backoff + * + * the upstream.use_retry feature flag will need to be turned on (default) + * for this to work. + * + * @param retry policy from the RemoteJwks proto + * @return a retry policy usable by the http async client. + */ +envoy::config::route::v3::RetryPolicy +adaptRetryPolicy(const envoy::config::core::v3::RetryPolicy& core_retry_policy) { + envoy::config::route::v3::RetryPolicy route_retry_policy; + + uint64_t base_interval_ms = RetryInitialDelayMilliseconds; + uint64_t max_interval_ms = RetryMaxDelayMilliseconds; + + if (core_retry_policy.has_retry_back_off()) { + const auto& core_back_off = core_retry_policy.retry_back_off(); + + base_interval_ms = PROTOBUF_GET_MS_REQUIRED(core_back_off, base_interval); + + max_interval_ms = + PROTOBUF_GET_MS_OR_DEFAULT(core_back_off, max_interval, base_interval_ms * 10); + + if (max_interval_ms < base_interval_ms) { + throw EnvoyException("max_interval must be greater than or equal to the base_interval"); + } + } + + route_retry_policy.mutable_num_retries()->set_value( + PROTOBUF_GET_WRAPPED_OR_DEFAULT(core_retry_policy, num_retries, 1)); + + auto* route_mutable_back_off = route_retry_policy.mutable_retry_back_off(); + + route_mutable_back_off->mutable_base_interval()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(base_interval_ms)); + route_mutable_back_off->mutable_max_interval()->CopyFrom( + Protobuf::util::TimeUtil::MillisecondsToDuration(max_interval_ms)); + + // set all the other fields with appropriate values. + route_retry_policy.set_retry_on("5xx,gateway-error,connect-failure,reset"); + route_retry_policy.mutable_per_try_timeout()->CopyFrom( + route_retry_policy.retry_back_off().max_interval()); + + return route_retry_policy; +} class JwksFetcherImpl : public JwksFetcher, public Logger::Loggable, public Http::AsyncClient::Callbacks { public: - JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } + JwksFetcherImpl(Upstream::ClusterManager& cm, const RemoteJwks& remote_jwks) + : cm_(cm), remote_jwks_(remote_jwks) { + ENVOY_LOG(trace, "{}", __func__); + + if (remote_jwks_.has_retry_policy()) { + route_retry_policy_ = adaptRetryPolicy(remote_jwks_.retry_policy()); + } + } ~JwksFetcherImpl() override { cancel(); } void cancel() final { if (request_ && !complete_) { request_->cancel(); - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", uri_->uri()); + ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", remote_jwks_.http_uri().uri()); } reset(); } - void fetch(const envoy::config::core::v3::HttpUri& uri, Tracing::Span& parent_span, - JwksFetcher::JwksReceiver& receiver) override { + void fetch(Tracing::Span& parent_span, JwksFetcher::JwksReceiver& receiver) override { ENVOY_LOG(trace, "{}", __func__); ASSERT(!receiver_); complete_ = false; receiver_ = &receiver; - uri_ = &uri; // Check if cluster is configured, fail the request if not. - const auto thread_local_cluster = cm_.getThreadLocalCluster(uri.cluster()); + const auto thread_local_cluster = cm_.getThreadLocalCluster(remote_jwks_.http_uri().cluster()); if (thread_local_cluster == nullptr) { ENVOY_LOG(error, "{}: fetch pubkey [uri = {}] failed: [cluster = {}] is not configured", - __func__, uri.uri(), uri.cluster()); + __func__, remote_jwks_.http_uri().uri(), remote_jwks_.http_uri().cluster()); complete_ = true; receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); reset(); return; } - Http::RequestMessagePtr message = Http::Utility::prepareHeaders(uri); + Http::RequestMessagePtr message = Http::Utility::prepareHeaders(remote_jwks_.http_uri()); message->headers().setReferenceMethod(Http::Headers::get().MethodValues.Get); - ENVOY_LOG(debug, "fetch pubkey from [uri = {}]: start", uri_->uri()); + ENVOY_LOG(debug, "fetch pubkey from [uri = {}]: start", remote_jwks_.http_uri().uri()); auto options = Http::AsyncClient::RequestOptions() .setTimeout(std::chrono::milliseconds( - DurationUtil::durationToMilliseconds(uri.timeout()))) + DurationUtil::durationToMilliseconds(remote_jwks_.http_uri().timeout()))) .setParentSpan(parent_span) .setChildSpanName("JWT Remote PubKey Fetch"); + + if (remote_jwks_.has_retry_policy()) { + options.setRetryPolicy(route_retry_policy_.value()); + options.setBufferBodyForRetry(true); + } + request_ = thread_local_cluster->httpAsyncClient().send(std::move(message), *this, options); } @@ -66,26 +134,27 @@ class JwksFetcherImpl : public JwksFetcher, ENVOY_LOG(trace, "{}", __func__); complete_ = true; const uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); + const std::string& uri = remote_jwks_.http_uri().uri(); if (status_code == enumToInt(Http::Code::OK)) { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: success", __func__, uri_->uri()); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: success", __func__, uri); if (response->body().length() != 0) { const auto body = response->bodyAsString(); auto jwks = google::jwt_verify::Jwks::createFrom(body, google::jwt_verify::Jwks::Type::JWKS); if (jwks->getStatus() == google::jwt_verify::Status::Ok) { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: succeeded", __func__, uri_->uri()); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: succeeded", __func__, uri); receiver_->onJwksSuccess(std::move(jwks)); } else { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: invalid jwks", __func__, uri_->uri()); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: invalid jwks", __func__, uri); receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks); } } else { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: body is empty", __func__, uri_->uri()); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: body is empty", __func__, uri); receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } } else { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: response status code {}", __func__, - uri_->uri(), status_code); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: response status code {}", __func__, uri, + status_code); receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } reset(); @@ -93,8 +162,8 @@ class JwksFetcherImpl : public JwksFetcher, void onFailure(const Http::AsyncClient::Request&, Http::AsyncClient::FailureReason reason) override { - ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, uri_->uri(), - enumToInt(reason)); + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, + remote_jwks_.http_uri().uri(), enumToInt(reason)); complete_ = true; receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); reset(); @@ -106,19 +175,25 @@ class JwksFetcherImpl : public JwksFetcher, Upstream::ClusterManager& cm_; bool complete_{}; JwksFetcher::JwksReceiver* receiver_{}; - const envoy::config::core::v3::HttpUri* uri_{}; + const RemoteJwks& remote_jwks_; Http::AsyncClient::Request* request_{}; + // http async client uses richer semantics than the ones allowed in RemoteJwks + // envoy.config.route.v3.RetryPolicy vs envoy.config.core.v3.RetryPolicy + // mapping is done in constructor and reused. + absl::optional route_retry_policy_{absl::nullopt}; + void reset() { request_ = nullptr; receiver_ = nullptr; - uri_ = nullptr; } }; } // namespace -JwksFetcherPtr JwksFetcher::create(Upstream::ClusterManager& cm) { - return std::make_unique(cm); +JwksFetcherPtr JwksFetcher::create( + Upstream::ClusterManager& cm, + const envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks& remote_jwks) { + return std::make_unique(cm, remote_jwks); } } // namespace Common } // namespace HttpFilters diff --git a/source/extensions/filters/http/common/jwks_fetcher.h b/source/extensions/filters/http/common/jwks_fetcher.h index a6d99f65b36ac..7579bcb141e13 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.h +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -2,6 +2,7 @@ #include "envoy/common/pure.h" #include "envoy/config/core/v3/http_uri.pb.h" +#include "envoy/extensions/filters/http/jwt_authn/v3/config.pb.h" #include "envoy/upstream/cluster_manager.h" #include "jwt_verify_lib/jwks.h" @@ -57,19 +58,25 @@ class JwksFetcher { * i.e. from the invocation of `fetch()` until either * a callback or `cancel()` is invoked, no * additional `fetch()` may be issued. - * @param uri the uri to retrieve the jwks from. + * the URI to to fetch is to be obtained at construction time + * for example from RemoteJwks configuration + * * @param parent_span the active span to create children under * @param receiver the receiver of the fetched JWKS or error. + * + * */ - virtual void fetch(const envoy::config::core::v3::HttpUri& uri, Tracing::Span& parent_span, - JwksReceiver& receiver) PURE; + virtual void fetch(Tracing::Span& parent_span, JwksReceiver& receiver) PURE; /* * Factory method for creating a JwksFetcher. * @param cm the cluster manager to use during Jwks retrieval + * @param remote_jwks the definition of the remote Jwks source * @return a JwksFetcher instance */ - static JwksFetcherPtr create(Upstream::ClusterManager& cm); + static JwksFetcherPtr + create(Upstream::ClusterManager& cm, + const envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks& remote_jwks); }; } // namespace Common } // namespace HttpFilters diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 151d6a7fa1286..f32bfac5dd78d 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -211,9 +211,9 @@ void AuthenticatorImpl::startVerify() { // jwks fetching can be shared by two requests. if (jwks_data_->getJwtProvider().has_remote_jwks()) { if (!fetcher_) { - fetcher_ = create_jwks_fetcher_cb_(cm_); + fetcher_ = create_jwks_fetcher_cb_(cm_, jwks_data_->getJwtProvider().remote_jwks()); } - fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), *parent_span_, *this); + fetcher_->fetch(*parent_span_, *this); return; } // No valid keys for this issuer. This may happen as a result of incorrect local diff --git a/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc b/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc index ddd7350d44d36..2c870576c0f15 100644 --- a/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc +++ b/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.cc @@ -54,8 +54,8 @@ void JwksAsyncFetcher::fetch() { } ENVOY_LOG(debug, "{}: started", debug_name_); - fetcher_ = create_fetcher_fn_(context_.clusterManager()); - fetcher_->fetch(remote_jwks_.http_uri(), Tracing::NullSpan::instance(), *this); + fetcher_ = create_fetcher_fn_(context_.clusterManager(), remote_jwks_); + fetcher_->fetch(Tracing::NullSpan::instance(), *this); } void JwksAsyncFetcher::handleFetchDone() { diff --git a/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.h b/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.h index 0d6247394ebac..d8483296ee265 100644 --- a/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.h +++ b/source/extensions/filters/http/jwt_authn/jwks_async_fetcher.h @@ -18,7 +18,8 @@ namespace JwtAuthn { /** * CreateJwksFetcherCb is a callback interface for creating a JwksFetcher instance. */ -using CreateJwksFetcherCb = std::function; +using CreateJwksFetcherCb = std::function; /** * JwksDoneFetched is a callback interface to set a Jwks when fetch is done. */ diff --git a/test/common/http/async_client_impl_test.cc b/test/common/http/async_client_impl_test.cc index ea81164f1cc42..4e5bfe53b0db1 100644 --- a/test/common/http/async_client_impl_test.cc +++ b/test/common/http/async_client_impl_test.cc @@ -1545,45 +1545,84 @@ TEST_F(AsyncClientImplTest, DumpState) { // Must not be in anonymous namespace for friend to work. class AsyncClientImplUnitTest : public testing::Test { public: - AsyncStreamImpl::RouteImpl route_impl_{ + std::unique_ptr route_impl_{new AsyncStreamImpl::RouteImpl( "foo", absl::nullopt, - Protobuf::RepeatedPtrField()}; + Protobuf::RepeatedPtrField(), + absl::nullopt)}; AsyncStreamImpl::NullVirtualHost vhost_; AsyncStreamImpl::NullConfig config_; + + void setupRouteImpl(const std::string& yaml_config) { + envoy::config::route::v3::RetryPolicy retry_policy; + + TestUtility::loadFromYaml(yaml_config, retry_policy); + + route_impl_ = std::make_unique( + "foo", absl::nullopt, + Protobuf::RepeatedPtrField(), + std::move(retry_policy)); + } }; // Test the extended fake route that AsyncClient uses. -TEST_F(AsyncClientImplUnitTest, RouteImplInitTest) { - EXPECT_EQ(nullptr, route_impl_.decorator()); - EXPECT_EQ(nullptr, route_impl_.tracingConfig()); - EXPECT_EQ(nullptr, route_impl_.perFilterConfig("")); - EXPECT_EQ(Code::InternalServerError, route_impl_.routeEntry()->clusterNotFoundResponseCode()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->corsPolicy()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->hashPolicy()); - EXPECT_EQ(1, route_impl_.routeEntry()->hedgePolicy().initialRequests()); - EXPECT_EQ(0, route_impl_.routeEntry()->hedgePolicy().additionalRequestChance().numerator()); - EXPECT_FALSE(route_impl_.routeEntry()->hedgePolicy().hedgeOnPerTryTimeout()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->metadataMatchCriteria()); - EXPECT_TRUE(route_impl_.routeEntry()->rateLimitPolicy().empty()); - EXPECT_TRUE(route_impl_.routeEntry()->rateLimitPolicy().getApplicableRateLimit(0).empty()); - EXPECT_EQ(absl::nullopt, route_impl_.routeEntry()->idleTimeout()); - EXPECT_EQ(absl::nullopt, route_impl_.routeEntry()->grpcTimeoutOffset()); - EXPECT_TRUE(route_impl_.routeEntry()->opaqueConfig().empty()); - EXPECT_TRUE(route_impl_.routeEntry()->includeVirtualHostRateLimits()); - EXPECT_TRUE(route_impl_.routeEntry()->metadata().filter_metadata().empty()); - EXPECT_EQ(nullptr, - route_impl_.routeEntry()->typedMetadata().get("bar")); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->perFilterConfig("bar")); - EXPECT_TRUE(route_impl_.routeEntry()->upgradeMap().empty()); - EXPECT_EQ(false, route_impl_.routeEntry()->internalRedirectPolicy().enabled()); - EXPECT_TRUE(route_impl_.routeEntry()->shadowPolicies().empty()); - EXPECT_TRUE(route_impl_.routeEntry()->virtualHost().rateLimitPolicy().empty()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->virtualHost().corsPolicy()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->virtualHost().perFilterConfig("bar")); - EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().includeAttemptCountInRequest()); - EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().includeAttemptCountInResponse()); - EXPECT_FALSE(route_impl_.routeEntry()->virtualHost().routeConfig().usesVhds()); - EXPECT_EQ(nullptr, route_impl_.routeEntry()->tlsContextMatchCriteria()); +TEST_F(AsyncClientImplUnitTest, NullRouteImplInitTest) { + + auto& route_entry = *(route_impl_->routeEntry()); + + EXPECT_EQ(nullptr, route_impl_->decorator()); + EXPECT_EQ(nullptr, route_impl_->tracingConfig()); + EXPECT_EQ(nullptr, route_impl_->perFilterConfig("")); + EXPECT_EQ(Code::InternalServerError, route_entry.clusterNotFoundResponseCode()); + EXPECT_EQ(nullptr, route_entry.corsPolicy()); + EXPECT_EQ(nullptr, route_entry.hashPolicy()); + EXPECT_EQ(1, route_entry.hedgePolicy().initialRequests()); + EXPECT_EQ(0, route_entry.hedgePolicy().additionalRequestChance().numerator()); + EXPECT_FALSE(route_entry.hedgePolicy().hedgeOnPerTryTimeout()); + EXPECT_EQ(nullptr, route_entry.metadataMatchCriteria()); + EXPECT_TRUE(route_entry.rateLimitPolicy().empty()); + EXPECT_TRUE(route_entry.rateLimitPolicy().getApplicableRateLimit(0).empty()); + EXPECT_EQ(absl::nullopt, route_entry.idleTimeout()); + EXPECT_EQ(absl::nullopt, route_entry.grpcTimeoutOffset()); + EXPECT_TRUE(route_entry.opaqueConfig().empty()); + EXPECT_TRUE(route_entry.includeVirtualHostRateLimits()); + EXPECT_TRUE(route_entry.metadata().filter_metadata().empty()); + EXPECT_EQ(nullptr, route_entry.typedMetadata().get("bar")); + EXPECT_EQ(nullptr, route_entry.perFilterConfig("bar")); + EXPECT_TRUE(route_entry.upgradeMap().empty()); + EXPECT_EQ(false, route_entry.internalRedirectPolicy().enabled()); + EXPECT_TRUE(route_entry.shadowPolicies().empty()); + EXPECT_TRUE(route_entry.virtualHost().rateLimitPolicy().empty()); + EXPECT_EQ(nullptr, route_entry.virtualHost().corsPolicy()); + EXPECT_EQ(nullptr, route_entry.virtualHost().perFilterConfig("bar")); + EXPECT_FALSE(route_entry.virtualHost().includeAttemptCountInRequest()); + EXPECT_FALSE(route_entry.virtualHost().includeAttemptCountInResponse()); + EXPECT_FALSE(route_entry.virtualHost().routeConfig().usesVhds()); + EXPECT_EQ(nullptr, route_entry.tlsContextMatchCriteria()); +} + +TEST_F(AsyncClientImplUnitTest, RouteImplInitTestWithRetryPolicy) { + + const std::string yaml = R"EOF( +per_try_timeout: 30s +num_retries: 10 +retry_on: 5xx,gateway-error,connect-failure,reset +retry_back_off: + base_interval: 0.01s + max_interval: 30s +)EOF"; + + setupRouteImpl(yaml); + + auto& route_entry = *(route_impl_->routeEntry()); + + EXPECT_EQ(route_entry.retryPolicy().numRetries(), 10); + EXPECT_EQ(route_entry.retryPolicy().perTryTimeout(), std::chrono::seconds(30)); + EXPECT_EQ(Router::RetryPolicy::RETRY_ON_CONNECT_FAILURE | Router::RetryPolicy::RETRY_ON_5XX | + Router::RetryPolicy::RETRY_ON_GATEWAY_ERROR | Router::RetryPolicy::RETRY_ON_RESET, + route_entry.retryPolicy().retryOn()); + + EXPECT_EQ(route_entry.retryPolicy().baseInterval(), std::chrono::milliseconds(10)); + EXPECT_EQ(route_entry.retryPolicy().maxInterval(), std::chrono::seconds(30)); } TEST_F(AsyncClientImplUnitTest, NullConfig) { diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index aad2ac07b636c..09a90ddbd00fc 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -12,8 +12,7 @@ #include "test/mocks/server/factory_context.h" #include "test/test_common/utility.h" -using envoy::config::core::v3::HttpUri; - +using envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks; namespace Envoy { namespace Extensions { namespace HttpFilters { @@ -43,117 +42,115 @@ const char publicKey[] = R"( } )"; -const std::string JwksUri = R"( -uri: https://pubkey_server/pubkey_path -cluster: pubkey_cluster -timeout: - seconds: 5 +const std::string config = R"( +http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 )"; class JwksFetcherTest : public testing::Test { public: - void SetUp() override { - TestUtility::loadFromYaml(JwksUri, uri_); + void setupFetcher(const std::string& config_str) { + TestUtility::loadFromYaml(config_str, remote_jwks_); mock_factory_ctx_.cluster_manager_.initializeThreadLocalClusters({"pubkey_cluster"}); + fetcher_ = JwksFetcher::create(mock_factory_ctx_.cluster_manager_, remote_jwks_); + EXPECT_TRUE(fetcher_ != nullptr); } - HttpUri uri_; + RemoteJwks remote_jwks_; testing::NiceMock mock_factory_ctx_; + std::unique_ptr fetcher_; NiceMock parent_span_; }; // Test findByIssuer TEST_F(JwksFetcherTest, TestGetSuccess) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", publicKey); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)); EXPECT_CALL(receiver, onJwksError(testing::_)).Times(0); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); } TEST_F(JwksFetcherTest, TestGet400) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "400", "invalid"); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::Network)); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); } TEST_F(JwksFetcherTest, TestGetNoBody) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", ""); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::Network)); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); } TEST_F(JwksFetcherTest, TestGetInvalidJwks) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", "invalid"); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks)); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); } TEST_F(JwksFetcherTest, TestHttpFailure) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, Http::AsyncClient::FailureReason::Reset); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::Network)); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); } TEST_F(JwksFetcherTest, TestCancel) { // Setup + setupFetcher(config); Http::MockAsyncClientRequest request( &(mock_factory_ctx_.cluster_manager_.thread_local_cluster_.async_client_)); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, &request); MockJwksReceiver receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); - EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(request, cancel()); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); EXPECT_CALL(receiver, onJwksError(testing::_)).Times(0); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); // Proper cancel - fetcher->cancel(); + fetcher_->cancel(); // Re-entrant cancel - fetcher->cancel(); + fetcher_->cancel(); } TEST_F(JwksFetcherTest, TestSpanPassedDown) { // Setup + setupFetcher(config); MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", publicKey); NiceMock receiver; - std::unique_ptr fetcher(JwksFetcher::create(mock_factory_ctx_.cluster_manager_)); // Expectations for span EXPECT_CALL(mock_factory_ctx_.cluster_manager_.thread_local_cluster_.async_client_, @@ -167,7 +164,121 @@ TEST_F(JwksFetcherTest, TestSpanPassedDown) { })); // Act - fetcher->fetch(uri_, parent_span_, receiver); + fetcher_->fetch(parent_span_, receiver); +} +struct RetryingParameters { + RetryingParameters(const std::string& config, uint32_t n, int64_t base_ms, int64_t max_ms) + : config_(config), expected_num_retries_(n), expected_backoff_base_interval_ms_(base_ms), + expected_backoff_max_interval_ms_(max_ms) {} + + std::string config_; + + uint32_t expected_num_retries_; + int64_t expected_backoff_base_interval_ms_; + int64_t expected_backoff_max_interval_ms_; +}; + +class JwksFetcherRetryingTest : public testing::TestWithParam { +public: + void setupFetcher(const std::string& config_str) { + TestUtility::loadFromYaml(config_str, remote_jwks_); + mock_factory_ctx_.cluster_manager_.initializeThreadLocalClusters({"pubkey_cluster"}); + fetcher_ = JwksFetcher::create(mock_factory_ctx_.cluster_manager_, remote_jwks_); + EXPECT_TRUE(fetcher_ != nullptr); + } + + RemoteJwks remote_jwks_; + testing::NiceMock mock_factory_ctx_; + std::unique_ptr fetcher_; + NiceMock parent_span_; +}; + +INSTANTIATE_TEST_SUITE_P(Retrying, JwksFetcherRetryingTest, + testing::Values(RetryingParameters{R"( +http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 +retry_policy: + retry_back_off: + base_interval: 0.1s + max_interval: 32s + num_retries: 10 +)", + 10, 100, 32000}, + RetryingParameters{R"( +http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 +retry_policy: {} +)", + 1, 1000, 10000}, + RetryingParameters{R"( +http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 +retry_policy: + num_retries: 2 +)", + 2, 1000, 10000})); + +TEST_P(JwksFetcherRetryingTest, TestCompleteRetryPolicy) { + + // Setup + setupFetcher(GetParam().config_); + MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", publicKey); + NiceMock receiver; + + // Expectations for envoy.config.core.v3.RetryPolicy to envoy.config.route.v3.RetryPolicy + // used by async client. + // execution deep down in async_client_'s route entry implementation + // is not exercised here, just the configuration adaptation. + EXPECT_CALL(mock_factory_ctx_.cluster_manager_.thread_local_cluster_.async_client_, + send_(_, _, _)) + .WillOnce(Invoke( + [](Http::RequestMessagePtr&, Http::AsyncClient::Callbacks&, + const Http::AsyncClient::RequestOptions& options) -> Http::AsyncClient::Request* { + RetryingParameters const& rp = GetParam(); + + EXPECT_TRUE(options.retry_policy.has_value()); + EXPECT_TRUE(options.buffer_body_for_retry); + EXPECT_TRUE(options.retry_policy.value().has_num_retries()); + EXPECT_EQ(PROTOBUF_GET_WRAPPED_REQUIRED(options.retry_policy.value(), num_retries), + rp.expected_num_retries_); + + EXPECT_TRUE(options.retry_policy.value().has_retry_back_off()); + EXPECT_TRUE(options.retry_policy.value().retry_back_off().has_base_interval()); + EXPECT_EQ(PROTOBUF_GET_MS_REQUIRED(options.retry_policy.value().retry_back_off(), + base_interval), + rp.expected_backoff_base_interval_ms_); + EXPECT_TRUE(options.retry_policy.value().retry_back_off().has_max_interval()); + EXPECT_EQ(PROTOBUF_GET_MS_REQUIRED(options.retry_policy.value().retry_back_off(), + max_interval), + rp.expected_backoff_max_interval_ms_); + + EXPECT_TRUE(options.retry_policy.value().has_per_try_timeout()); + EXPECT_LE(PROTOBUF_GET_MS_REQUIRED(options.retry_policy.value().retry_back_off(), + max_interval), + PROTOBUF_GET_MS_REQUIRED(options.retry_policy.value(), per_try_timeout)); + + const std::string& retry_on = options.retry_policy.value().retry_on(); + std::set retry_on_modes = absl::StrSplit(retry_on, ','); + + EXPECT_EQ(retry_on_modes.count("5xx"), 1); + EXPECT_EQ(retry_on_modes.count("gateway-error"), 1); + EXPECT_EQ(retry_on_modes.count("connect-failure"), 1); + EXPECT_EQ(retry_on_modes.count("reset"), 1); + + return nullptr; + })); + + // Act + fetcher_->fetch(parent_span_, receiver); } } // namespace diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h index 3aa0bb3b2b04d..35d10d1203215 100644 --- a/test/extensions/filters/http/common/mock.h +++ b/test/extensions/filters/http/common/mock.h @@ -17,9 +17,7 @@ namespace Common { class MockJwksFetcher : public JwksFetcher { public: MOCK_METHOD(void, cancel, ()); - MOCK_METHOD(void, fetch, - (const envoy::config::core::v3::HttpUri& uri, Tracing::Span& parent_span, - JwksReceiver& receiver)); + MOCK_METHOD(void, fetch, (Tracing::Span & parent_span, JwksReceiver& receiver)); }; // A mock HTTP upstream. diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index 0168f1ce58aab..41224121bbe97 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -16,6 +16,7 @@ #include "gtest/gtest.h" using envoy::extensions::filters::http::jwt_authn::v3::JwtAuthentication; +using envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks; using Envoy::Extensions::HttpFilters::Common::JwksFetcher; using Envoy::Extensions::HttpFilters::Common::JwksFetcherPtr; using Envoy::Extensions::HttpFilters::Common::MockJwksFetcher; @@ -47,7 +48,8 @@ class AuthenticatorTest : public testing::Test { fetcher_.reset(raw_fetcher_); auth_ = Authenticator::create( check_audience, provider, allow_failed, allow_missing, filter_config_->getJwksCache(), - filter_config_->cm(), [this](Upstream::ClusterManager&) { return std::move(fetcher_); }, + filter_config_->cm(), + [this](Upstream::ClusterManager&, const RemoteJwks&) { return std::move(fetcher_); }, filter_config_->timeSource()); jwks_ = Jwks::createFrom(PublicKey, Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); @@ -91,9 +93,8 @@ class AuthenticatorTest : public testing::Test { // This test validates a good JWT authentication with a remote Jwks. // It also verifies Jwks cache with 10 JWT authentications, but only one Jwks fetch. TEST_F(AuthenticatorTest, TestOkJWTandCache) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -116,9 +117,8 @@ TEST_F(AuthenticatorTest, TestCompletePaddingInJwtPayload) { (*proto_config_.mutable_providers())[std::string(ProviderName)].set_pad_forward_payload_header( true); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -134,9 +134,8 @@ TEST_F(AuthenticatorTest, TestForwardJwt) { // Config forward_jwt flag (*proto_config_.mutable_providers())[std::string(ProviderName)].set_forward(true); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -161,9 +160,8 @@ TEST_F(AuthenticatorTest, TestSetPayload) { (*proto_config_.mutable_providers())[std::string(ProviderName)].set_payload_in_metadata( "my_payload"); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -182,9 +180,8 @@ TEST_F(AuthenticatorTest, TestSetPayload) { // This test verifies the Jwt with non existing kid TEST_F(AuthenticatorTest, TestJwtWithNonExistKid) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -198,7 +195,7 @@ TEST_F(AuthenticatorTest, TestJwtWithNonExistKid) { // Jwt "iss" is "other.com", it doesn't match "issuer" specified in JwtProvider. // The verification fails with JwtUnknownIssuer. TEST_F(AuthenticatorTest, TestWrongIssuer) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); // Token with other issuer should fail. Http::TestRequestHeaderMapImpl headers{ {"Authorization", "Bearer " + std::string(OtherGoodToken)}}; @@ -216,9 +213,8 @@ TEST_F(AuthenticatorTest, TestWrongIssuerOKWithProvider) { provider.clear_audiences(); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -238,9 +234,8 @@ TEST_F(AuthenticatorTest, TestWrongIssuerOKWithoutProvider) { // use authenticator without a valid provider createAuthenticator(nullptr, absl::nullopt); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -256,9 +251,8 @@ TEST_F(AuthenticatorTest, TestJwtWithoutIssWithValidProvider) { jwks_ = Jwks::createFrom(ES256PublicKey, Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -274,7 +268,7 @@ TEST_F(AuthenticatorTest, TestJwtWithoutIssWithValidProvider) { // When "allow_missing" or "allow_failed" is used, authenticator doesn't have a valid provider. TEST_F(AuthenticatorTest, TestJwtWithoutIssWithoutValidProvider) { createAuthenticator(nullptr, absl::nullopt); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{ {"Authorization", "Bearer " + std::string(ES256WithoutIssToken)}}; @@ -291,9 +285,8 @@ TEST_F(AuthenticatorTest, TestJwtWithoutIssWithValidProviderNotIssuer) { jwks_ = Jwks::createFrom(ES256PublicKey, Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -314,9 +307,8 @@ TEST_F(AuthenticatorTest, TestJwtWithoutIssWithoutValidProviderNotIssuer) { jwks_ = Jwks::createFrom(ES256PublicKey, Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -327,7 +319,7 @@ TEST_F(AuthenticatorTest, TestJwtWithoutIssWithoutValidProviderNotIssuer) { // This test verifies if Jwt is missing, proper status is called. TEST_F(AuthenticatorTest, TestMissedJWT) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); // Empty headers. Http::TestRequestHeaderMapImpl headers{}; @@ -337,7 +329,7 @@ TEST_F(AuthenticatorTest, TestMissedJWT) { // Test multiple tokens; the one from query parameter is bad, verification should fail. TEST_F(AuthenticatorTest, TestMultipleJWTOneBadFromQuery) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)); // headers with multiple tokens: one good, one bad Http::TestRequestHeaderMapImpl headers{ @@ -350,7 +342,7 @@ TEST_F(AuthenticatorTest, TestMultipleJWTOneBadFromQuery) { // Test multiple tokens; the one from header is bad, verification should fail. TEST_F(AuthenticatorTest, TestMultipleJWTOneBadFromHeader) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)); // headers with multiple tokens: one good, one bad Http::TestRequestHeaderMapImpl headers{ @@ -363,7 +355,7 @@ TEST_F(AuthenticatorTest, TestMultipleJWTOneBadFromHeader) { // Test multiple tokens; all are good, verification is ok. TEST_F(AuthenticatorTest, TestMultipleJWTAllGood) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)); // headers with multiple tokens: all are good Http::TestRequestHeaderMapImpl headers{ @@ -378,7 +370,7 @@ TEST_F(AuthenticatorTest, TestMultipleJWTAllGood) { TEST_F(AuthenticatorTest, TestMultipleJWTOneBadAllowFails) { createAuthenticator(nullptr, absl::make_optional(ProviderName), /*allow_failed=*/true, /*all_missing=*/false); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)); // headers with multiple tokens: one good, one bad Http::TestRequestHeaderMapImpl headers{ @@ -393,7 +385,7 @@ TEST_F(AuthenticatorTest, TestMultipleJWTOneBadAllowFails) { TEST_F(AuthenticatorTest, TestAllowMissingWithEmptyHeader) { createAuthenticator(nullptr, absl::make_optional(ProviderName), /*allow_failed=*/false, /*all_missing=*/true); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); // Empty headers Http::TestRequestHeaderMapImpl headers{}; @@ -403,7 +395,7 @@ TEST_F(AuthenticatorTest, TestAllowMissingWithEmptyHeader) { // This test verifies if Jwt is invalid, JwtBadFormat status is returned. TEST_F(AuthenticatorTest, TestInvalidJWT) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); std::string token = "invalidToken"; Http::TestRequestHeaderMapImpl headers{{"Authorization", "Bearer " + token}}; @@ -412,7 +404,7 @@ TEST_F(AuthenticatorTest, TestInvalidJWT) { // This test verifies if Authorization header has invalid prefix, JwtMissed status is returned TEST_F(AuthenticatorTest, TestInvalidPrefix) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{{"Authorization", "Bearer-invalid"}}; expectVerifyStatus(Status::JwtMissed, headers); @@ -430,7 +422,7 @@ TEST_F(AuthenticatorTest, TestNonExpiringJWT) { // This test verifies when a JWT is expired, JwtExpired status is returned. TEST_F(AuthenticatorTest, TestExpiredJWT) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{{"Authorization", "Bearer " + std::string(ExpiredToken)}}; expectVerifyStatus(Status::JwtExpired, headers); @@ -443,9 +435,8 @@ TEST_F(AuthenticatorTest, TestExpiredJWTWithABigClockSkew) { provider.set_clock_skew_seconds(1205005587); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -455,7 +446,7 @@ TEST_F(AuthenticatorTest, TestExpiredJWTWithABigClockSkew) { // This test verifies when a JWT is not yet valid, JwtNotYetValid status is returned. TEST_F(AuthenticatorTest, TestNotYetValidJWT) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{ {"Authorization", "Bearer " + std::string(NotYetValidToken)}}; @@ -469,7 +460,7 @@ TEST_F(AuthenticatorTest, TestInvalidLocalJwks) { provider.mutable_local_jwks()->set_inline_string("invalid"); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{{"Authorization", "Bearer " + std::string(GoodToken)}}; expectVerifyStatus(Status::JwksNoValidKeys, headers); @@ -477,7 +468,7 @@ TEST_F(AuthenticatorTest, TestInvalidLocalJwks) { // This test verifies when a JWT is with invalid audience, JwtAudienceNotAllowed is returned. TEST_F(AuthenticatorTest, TestNonMatchAudJWT) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{ {"Authorization", "Bearer " + std::string(InvalidAudToken)}}; @@ -490,7 +481,7 @@ TEST_F(AuthenticatorTest, TestIssuerNotFound) { (*proto_config_.mutable_providers())[std::string(ProviderName)].set_issuer("other_issuer"); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)).Times(0); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)).Times(0); Http::TestRequestHeaderMapImpl headers{{"Authorization", "Bearer " + std::string(GoodToken)}}; expectVerifyStatus(Status::JwtUnknownIssuer, headers); @@ -498,9 +489,8 @@ TEST_F(AuthenticatorTest, TestIssuerNotFound) { // This test verifies that when Jwks fetching fails, JwksFetchFail status is returned. TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks); })); @@ -515,7 +505,7 @@ TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { // onComplete() callback should not be called, but internal request->cancel() should be called. // Most importantly, no crash. TEST_F(AuthenticatorTest, TestOnDestroy) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)); + EXPECT_CALL(*raw_fetcher_, fetch(_, _)); // Cancel is called once. EXPECT_CALL(*raw_fetcher_, cancel()); @@ -537,9 +527,8 @@ TEST_F(AuthenticatorTest, TestNoForwardPayloadHeader) { auto& provider0 = (*proto_config_.mutable_providers())[std::string(ProviderName)]; provider0.clear_forward_payload_header(); createAuthenticator(); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -562,9 +551,8 @@ TEST_F(AuthenticatorTest, TestAllowFailedMultipleTokens) { } createAuthenticator(nullptr, absl::nullopt, /*allow_failed=*/true); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -609,10 +597,9 @@ TEST_F(AuthenticatorTest, TestAllowFailedMultipleIssuers) { header->set_value_prefix("Bearer "); createAuthenticator(nullptr, absl::nullopt, /*allow_failed=*/true); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) .Times(2) - .WillRepeatedly(Invoke([](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + .WillRepeatedly(Invoke([](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { ::google::jwt_verify::JwksPtr jwks = Jwks::createFrom(PublicKey, Jwks::JWKS); EXPECT_TRUE(jwks->getStatus() == Status::Ok); receiver.onJwksSuccess(std::move(jwks)); @@ -636,9 +623,8 @@ TEST_F(AuthenticatorTest, TestCustomCheckAudience) { auto check_audience = std::make_unique<::google::jwt_verify::CheckAudience>( std::vector{"invalid_service"}); createAuthenticator(check_audience.get()); - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([this](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([this](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { receiver.onJwksSuccess(std::move(jwks_)); })); @@ -652,9 +638,8 @@ TEST_F(AuthenticatorTest, TestCustomCheckAudience) { // This test verifies that when invalid JWKS is fetched, an JWKS error status is returned. TEST_F(AuthenticatorTest, TestInvalidPubkeyKey) { - EXPECT_CALL(*raw_fetcher_, fetch(_, _, _)) - .WillOnce(Invoke([](const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksFetcher::JwksReceiver& receiver) { + EXPECT_CALL(*raw_fetcher_, fetch(_, _)) + .WillOnce(Invoke([](Tracing::Span&, JwksFetcher::JwksReceiver& receiver) { auto jwks = Jwks::createFrom(PublicKey, Jwks::PEM); receiver.onJwksSuccess(std::move(jwks)); })); diff --git a/test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc b/test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc index 7973c40f05790..5322b96dbe9f1 100644 --- a/test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc +++ b/test/extensions/filters/http/jwt_authn/jwks_async_fetcher_test.cc @@ -23,10 +23,7 @@ class MockJwksFetcher : public Common::JwksFetcher { MockJwksFetcher(SaveJwksReceiverFn receiver_fn) : receiver_fn_(receiver_fn) {} void cancel() override {} - void fetch(const envoy::config::core::v3::HttpUri&, Tracing::Span&, - JwksReceiver& receiver) override { - receiver_fn_(receiver); - } + void fetch(Tracing::Span&, JwksReceiver& receiver) override { receiver_fn_(receiver); } private: SaveJwksReceiverFn receiver_fn_; @@ -66,7 +63,7 @@ class JwksAsyncFetcherTest : public testing::TestWithParam { async_fetcher_ = std::make_unique( config_, context_, - [this](Upstream::ClusterManager&) { + [this](Upstream::ClusterManager&, const RemoteJwks&) { return std::make_unique( [this](Common::JwksFetcher::JwksReceiver& receiver) { fetch_receiver_array_.push_back(&receiver); diff --git a/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc b/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc index 1c98f2ec7606d..d6a1c725bc521 100644 --- a/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc +++ b/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc @@ -12,6 +12,7 @@ #include "test/test_common/utility.h" using envoy::extensions::filters::http::jwt_authn::v3::JwtAuthentication; +using envoy::extensions::filters::http::jwt_authn::v3::RemoteJwks; using ::google::jwt_verify::Status; using ::testing::MockFunction; @@ -31,7 +32,7 @@ class JwksCacheTest : public testing::Test { void SetUp() override { // fetcher is only called at async_fetch. In this test, it is never called. - EXPECT_CALL(mock_fetcher_, Call(_)).Times(0); + EXPECT_CALL(mock_fetcher_, Call(_, _)).Times(0); setupCache(ExampleConfig); jwks_ = google::jwt_verify::Jwks::createFrom(PublicKey, google::jwt_verify::Jwks::JWKS); } @@ -44,7 +45,7 @@ class JwksCacheTest : public testing::Test { JwtAuthentication config_; JwksCachePtr cache_; google::jwt_verify::JwksPtr jwks_; - MockFunction mock_fetcher_; + MockFunction mock_fetcher_; NiceMock context_; JwtAuthnFilterStats stats_; };