From 79147949545b0e59f4b6ecb0c0a14494aebe9618 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Fri, 17 Aug 2018 14:45:30 +0100 Subject: [PATCH 01/22] Creating JwksFetcher interface and impl JwksFetcher wraps up HTTP acquisition of JWKS strings converting them into a concrete type on the way. JwksFetcher is reusable so can be used in a wider context. Tests updated and fixed where necessary. Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 4 + bazel/repository_locations.bzl | 2 +- source/extensions/filters/http/common/BUILD | 14 + .../filters/http/common/jwks_fetcher.cc | 91 ++++++ .../filters/http/common/jwks_fetcher.h | 63 ++++ .../extensions/filters/http/jwt_authn/BUILD | 1 + .../filters/http/jwt_authn/authenticator.cc | 107 +++---- .../filters/http/jwt_authn/authenticator.h | 4 +- .../filters/http/jwt_authn/filter_factory.cc | 5 +- .../filters/http/jwt_authn/jwks_cache.cc | 25 +- .../filters/http/jwt_authn/jwks_cache.h | 5 +- test/extensions/filters/http/common/BUILD | 38 +++ .../filters/http/common/jwks_fetcher_test.cc | 134 +++++++++ test/extensions/filters/http/common/mock.h | 19 ++ .../http/{jwt_authn => common}/test_common.h | 17 +- test/extensions/filters/http/jwt_authn/BUILD | 17 +- .../http/jwt_authn/authenticator_test.cc | 273 ++++++------------ .../http/jwt_authn/filter_factory_test.cc | 6 +- .../http/jwt_authn/filter_integration_test.cc | 24 +- .../filters/http/jwt_authn/jwks_cache_test.cc | 20 +- 20 files changed, 549 insertions(+), 320 deletions(-) create mode 100644 source/extensions/filters/http/common/jwks_fetcher.cc create mode 100644 source/extensions/filters/http/common/jwks_fetcher.h create mode 100644 test/extensions/filters/http/common/BUILD create mode 100644 test/extensions/filters/http/common/jwks_fetcher_test.cc create mode 100644 test/extensions/filters/http/common/mock.h rename test/extensions/filters/http/{jwt_authn => common}/test_common.h (88%) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 50656e238e39a..089bdc2b592c9 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -464,6 +464,10 @@ def _com_google_absl(): name = "abseil_symbolize", actual = "@com_google_absl//absl/debugging:symbolize", ) + native.bind( + name = "abseil_time", + actual = "@com_google_absl//absl/time:time", + ) def _com_google_protobuf(): _repository_impl("com_google_protobuf") diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index f2c8e82a82ae7..91ed947d19aa1 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -70,7 +70,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/google/googleapis", ), com_github_google_jwt_verify = dict( - commit = "4eb9e96485b71e00d43acc7207501caafb085b4a", + commit = "66792a057ec54e4b75c6a2eeda4e98220bd12a9a", remote = "https://github.com/google/jwt_verify_lib", ), com_github_nodejs_http_parser = dict( diff --git a/source/extensions/filters/http/common/BUILD b/source/extensions/filters/http/common/BUILD index ad080a666e93c..f2b17633bc88c 100644 --- a/source/extensions/filters/http/common/BUILD +++ b/source/extensions/filters/http/common/BUILD @@ -23,3 +23,17 @@ envoy_cc_library( "//include/envoy/server:filter_config_interface", ], ) + +envoy_cc_library( + name = "jwks_fetcher_lib", + srcs = ["jwks_fetcher.cc"], + hdrs = ["jwks_fetcher.h"], + external_deps = [ + "jwt_verify_lib", + ], + deps = [ + "//include/envoy/upstream:cluster_manager_interface", + "//source/common/http:utility_lib", + "@envoy_api//envoy/api/v2/core:http_uri_cc", + ], +) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc new file mode 100644 index 0000000000000..b9bf1e0568f18 --- /dev/null +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -0,0 +1,91 @@ +#include "extensions/filters/http/common/jwks_fetcher.h" + +#include "common/common/enum_to_int.h" +#include "common/http/headers.h" +#include "common/http/utility.h" + +#include "jwt_verify_lib/status.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Common { +namespace { +class JwksFetcherImpl : public JwksFetcher, + public Logger::Loggable, + public Http::AsyncClient::Callbacks { +private: + Upstream::ClusterManager& cm_; + JwksFetcher::JwksReceiver* receiver_ = nullptr; + const ::envoy::api::v2::core::HttpUri* uri_ = nullptr; + Http::AsyncClient::Request* request_ = nullptr; + +public: + JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } + + void close() { + if (request_) { + request_->cancel(); + request_ = nullptr; + ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", uri_->uri()); + } + } + + void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver* receiver) { + ENVOY_LOG(trace, "{}", __func__); + receiver_ = receiver; + uri_ = &uri; + Http::MessagePtr message = Http::Utility::prepareHeaders(uri); + message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); + ENVOY_LOG(debug, "fetch pubkey from [uri = {}]: start", uri_->uri()); + request_ = + cm_.httpAsyncClientForCluster(uri.cluster()) + .send(std::move(message), *this, + std::chrono::milliseconds(DurationUtil::durationToMilliseconds(uri.timeout()))); + } + + // HTTP async receive methods + void onSuccess(Http::MessagePtr&& response) { + ENVOY_LOG(trace, "{}", __func__); + request_ = nullptr; + const uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); + if (status_code == enumToInt(Http::Code::OK)) { + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: success", __func__, uri_->uri()); + if (response->body()) { + const auto len = response->body()->length(); + const auto body = std::string(static_cast(response->body()->linearize(len)), len); + 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()); + receiver_->onJwksSuccess(std::move(jwks)); + } else { + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: invalid jwks", __func__, uri_->uri()); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks); + } + } else { + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: body is empty", __func__, uri_->uri()); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + } + } else { + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: response status code {}", __func__, + uri_->uri(), status_code); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + } + } + + void onFailure(Http::AsyncClient::FailureReason reason) { + ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, uri_->uri(), + enumToInt(reason)); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + } +}; +} // namespace + +JwksFetcher::JwksFetcherPtr JwksFetcher::create(Upstream::ClusterManager& cm) { + return JwksFetcherPtr(new JwksFetcherImpl(cm)); +} +} // namespace Common +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/common/jwks_fetcher.h b/source/extensions/filters/http/common/jwks_fetcher.h new file mode 100644 index 0000000000000..9f68308f1ee44 --- /dev/null +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -0,0 +1,63 @@ +#pragma once + +#include "envoy/api/v2/core/http_uri.pb.h" +#include "envoy/common/pure.h" +#include "envoy/upstream/cluster_manager.h" + +#include "jwt_verify_lib/jwks.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Common { + +class JwksFetcher { +public: + typedef std::unique_ptr JwksFetcherPtr; + + class JwksReceiver { + public: + enum class Failure { + unknown, + network, + invalid_jwks, + }; + + virtual ~JwksReceiver(){}; + /* + * Successful retrieval callback. + * of the returned JWKS object. + * @param jwks the JWKS object retrieved. + */ + virtual void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) PURE; + /* + * Retrieval error callback. + * * @param reason the failure reason. + */ + virtual void onJwksError(Failure reason) PURE; + }; + + virtual ~JwksFetcher(){}; + + /* + * Close/stop any inflight request. + */ + virtual void close() PURE; + + /* + * Retrieve a JWKS resource from a remote HTTP host. + * @param uri the uri to retrieve the jwks from. + */ + virtual void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver* receiver) PURE; + + /* + * Factory method for creating a JwksFetcher. + * @param cm the cluster manager to use during Jwks retrieval + * @return a JwksFetcher instance + */ + static JwksFetcherPtr create(Upstream::ClusterManager& cm); +}; +} // namespace Common +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/source/extensions/filters/http/jwt_authn/BUILD b/source/extensions/filters/http/jwt_authn/BUILD index 88a9b691d99fb..d7e1b572fcfbb 100644 --- a/source/extensions/filters/http/jwt_authn/BUILD +++ b/source/extensions/filters/http/jwt_authn/BUILD @@ -46,6 +46,7 @@ envoy_cc_library( "//include/envoy/server:filter_config_interface", "//include/envoy/stats:stats_macros", "//source/common/http:message_lib", + "//source/extensions/filters/http/common:jwks_fetcher_lib", ], ) diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 4dc5ee50e22f2..f218c33388f0d 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -24,29 +24,23 @@ namespace { */ class AuthenticatorImpl : public Logger::Loggable, public Authenticator, - public Http::AsyncClient::Callbacks { + public Common::JwksFetcher::JwksReceiver { public: - AuthenticatorImpl(FilterConfigSharedPtr config) : config_(config) {} + AuthenticatorImpl(FilterConfigSharedPtr config, Common::JwksFetcher::JwksFetcherPtr& fetcher) + : config_(config), fetcher_(std::move(fetcher)) {} + // Following functions are for JwksFetcher::JwksReceiver interface + void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) override; + void onJwksError(Failure reason) override; // Following functions are for Authenticator interface void verify(Http::HeaderMap& headers, Authenticator::Callbacks* callback) override; void onDestroy() override; void sanitizePayloadHeaders(Http::HeaderMap& headers) const override; private: - // Fetch a remote public key. - void fetchRemoteJwks(); - - // Following two functions are for AyncClient::Callbacks - void onSuccess(Http::MessagePtr&& response) override; - void onFailure(Http::AsyncClient::FailureReason) override; - // Verify with a specific public key. void verifyKey(); - // Handle the public key fetch done event. - void onFetchRemoteJwksDone(const std::string& jwks_str); - // Calls the callback with status. void doneWithStatus(const Status& status); @@ -56,6 +50,9 @@ class AuthenticatorImpl : public Logger::Loggable, // The config object. FilterConfigSharedPtr config_; + // The Jwks fetcher object + Common::JwksFetcher::JwksFetcherPtr fetcher_; + // The token data JwtLocationConstPtr token_; // The JWT object. @@ -115,13 +112,19 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback return; } + // TODO: Cross-platform-wise the below unix_timestamp code is wrong as the + // epoch is not guaranteed to be defined as the unix epoch. We should use + // the abseil time functionality instead. + // TODO: We should use the jwt_verify_lib to check the validity of a JWT. // Check "exp" claim. const auto unix_timestamp = std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch()) .count(); - // NOTE: Service account tokens generally don't have an expiration time (due to being long lived) - // and defaulted to 0 by google::jwt_verify library but are still valid. - if (jwt_.exp_ > 0 && jwt_.exp_ < unix_timestamp) { + if (jwt_.nbf_ > unix_timestamp) { + doneWithStatus(Status::JwtNotYetValid); + return; + } + if (jwt_.exp && jwt_.exp_ < unix_timestamp) { doneWithStatus(Status::JwtExpired); return; } @@ -137,7 +140,8 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback return; } - if (jwks_data_->getJwksObj() != nullptr && !jwks_data_->isExpired()) { + auto jwks_obj = jwks_data_->getJwksObj(); + if (jwks_obj != nullptr && !jwks_data_->isExpired()) { verifyKey(); return; } @@ -146,65 +150,17 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback // If request 1 triggers a remote jwks fetching, but is not yet replied when the request 2 // of using the same jwks comes. The request 2 will trigger another remote fetching for the // jwks. This can be optimized; the same remote jwks fetching can be shared by two requrests. - fetchRemoteJwks(); -} - -void AuthenticatorImpl::fetchRemoteJwks() { - const auto& http_uri = jwks_data_->getJwtProvider().remote_jwks().http_uri(); - - Http::MessagePtr message = Http::Utility::prepareHeaders(http_uri); - message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); - - if (config_->cm().get(http_uri.cluster()) == nullptr) { - doneWithStatus(Status::JwksFetchFail); - return; - } - - uri_ = http_uri.uri(); - ENVOY_LOG(debug, "fetch pubkey from [uri = {}]: start", uri_); - request_ = config_->cm() - .httpAsyncClientForCluster(http_uri.cluster()) - .send(std::move(message), *this, - std::chrono::milliseconds( - DurationUtil::durationToMilliseconds(http_uri.timeout()))); -} - -void AuthenticatorImpl::onSuccess(Http::MessagePtr&& response) { - request_ = nullptr; - const uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); - if (status_code == enumToInt(Http::Code::OK)) { - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: success", uri_); - if (response->body()) { - const auto len = response->body()->length(); - const auto body = std::string(static_cast(response->body()->linearize(len)), len); - onFetchRemoteJwksDone(body); - return; - } else { - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: body is empty", uri_); - } + if (jwks_data_->getJwtProvider().has_remote_jwks()) { + fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), this); } else { - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: response status code {}", uri_, status_code); - } - doneWithStatus(Status::JwksFetchFail); -} - -void AuthenticatorImpl::onFailure(Http::AsyncClient::FailureReason) { - request_ = nullptr; - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: failed", uri_); - doneWithStatus(Status::JwksFetchFail); -} - -void AuthenticatorImpl::onDestroy() { - if (request_ != nullptr) { - request_->cancel(); - request_ = nullptr; - ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", uri_); + // No valid keys for this issuer. This may happen as a result of incorrect local + // JWKS configuration. + doneWithStatus(Status::JwksNoValidKeys); } } -// Handle the public key fetch done event. -void AuthenticatorImpl::onFetchRemoteJwksDone(const std::string& jwks_str) { - const Status status = jwks_data_->setRemoteJwks(jwks_str); +void AuthenticatorImpl::onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { + const Status status = jwks_data_->setRemoteJwks(std::move(jwks))->getStatus(); if (status != Status::Ok) { doneWithStatus(status); } else { @@ -212,6 +168,10 @@ void AuthenticatorImpl::onFetchRemoteJwksDone(const std::string& jwks_str) { } } +void AuthenticatorImpl::onJwksError(Failure) { doneWithStatus(Status::JwksFetchFail); } + +void AuthenticatorImpl::onDestroy() { fetcher_->close(); } + // Verify with a specific public key. void AuthenticatorImpl::verifyKey() { const Status status = ::google::jwt_verify::verifyJwt(jwt_, *jwks_data_->getJwksObj()); @@ -249,8 +209,9 @@ void AuthenticatorImpl::doneWithStatus(const Status& status) { } // namespace -AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config) { - return std::make_unique(config); +AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config, + Common::JwksFetcher::JwksFetcherPtr& fetcher) { + return std::make_unique(config, fetcher); } } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/authenticator.h b/source/extensions/filters/http/jwt_authn/authenticator.h index 8e3e2273dc9d5..2904f0fe26645 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.h +++ b/source/extensions/filters/http/jwt_authn/authenticator.h @@ -1,5 +1,6 @@ #pragma once +#include "extensions/filters/http/common/jwks_fetcher.h" #include "extensions/filters/http/jwt_authn/filter_config.h" #include "jwt_verify_lib/status.h" @@ -35,7 +36,8 @@ class Authenticator { virtual void sanitizePayloadHeaders(Http::HeaderMap& headers) const PURE; // Authenticator factory function. - static AuthenticatorPtr create(FilterConfigSharedPtr config); + static AuthenticatorPtr create(FilterConfigSharedPtr config, + Common::JwksFetcher::JwksFetcherPtr& fetcher); }; } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/filter_factory.cc b/source/extensions/filters/http/jwt_authn/filter_factory.cc index 4e7a7e622ca7e..bfa9cf131ea46 100644 --- a/source/extensions/filters/http/jwt_authn/filter_factory.cc +++ b/source/extensions/filters/http/jwt_authn/filter_factory.cc @@ -46,8 +46,9 @@ FilterFactory::createFilterFactoryFromProtoTyped(const JwtAuthentication& proto_ validateJwtConfig(proto_config); auto filter_config = std::make_shared(proto_config, prefix, context); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - callbacks.addStreamDecoderFilter( - std::make_shared(filter_config->stats(), Authenticator::create(filter_config))); + auto jwks_fetcher = Common::JwksFetcher::create(filter_config->cm()); + callbacks.addStreamDecoderFilter(std::make_shared( + filter_config->stats(), Authenticator::create(filter_config, jwks_fetcher))); }; } diff --git a/source/extensions/filters/http/jwt_authn/jwks_cache.cc b/source/extensions/filters/http/jwt_authn/jwks_cache.cc index f26735c4f4186..3648113dbc304 100644 --- a/source/extensions/filters/http/jwt_authn/jwks_cache.cc +++ b/source/extensions/filters/http/jwt_authn/jwks_cache.cc @@ -34,12 +34,13 @@ class JwksDataImpl : public JwksCache::JwksData, public Logger::LoggablegetStatus() != Status::Ok) { ENVOY_LOG(warn, "Invalid inline jwks for issuer: {}, jwks: {}", jwt_provider_.issuer(), inline_jwks); + jwks_obj_.reset(nullptr); } } } @@ -54,8 +55,8 @@ class JwksDataImpl : public JwksCache::JwksData, public Logger::Loggable= expiration_time_; } - Status setRemoteJwks(const std::string& jwks_str) override { - return setKey(jwks_str, getRemoteJwksExpirationTime()); + const ::google::jwt_verify::Jwks* setRemoteJwks(::google::jwt_verify::JwksPtr&& jwks) override { + return setKey(std::move(jwks), getRemoteJwksExpirationTime()); } private: @@ -71,15 +72,11 @@ class JwksDataImpl : public JwksCache::JwksData, public Logger::LoggablegetStatus() != Status::Ok) { - return jwks_obj->getStatus(); - } - jwks_obj_ = std::move(jwks_obj); + const ::google::jwt_verify::Jwks* setKey(::google::jwt_verify::JwksPtr&& jwks, + std::chrono::steady_clock::time_point expire) { + jwks_obj_ = std::move(jwks); expiration_time_ = expire; - return Status::Ok; + return jwks_obj_.get(); } // The jwt provider config. diff --git a/source/extensions/filters/http/jwt_authn/jwks_cache.h b/source/extensions/filters/http/jwt_authn/jwks_cache.h index 570b9c00d3f15..46d697f03e8b4 100644 --- a/source/extensions/filters/http/jwt_authn/jwks_cache.h +++ b/source/extensions/filters/http/jwt_authn/jwks_cache.h @@ -53,8 +53,9 @@ class JwksCache { // Return true if jwks object is expired. virtual bool isExpired() const PURE; - // Set a remote Jwks string. - virtual ::google::jwt_verify::Status setRemoteJwks(const std::string& jwks_str) PURE; + // Set a remote Jwks. + virtual const ::google::jwt_verify::Jwks* + setRemoteJwks(::google::jwt_verify::JwksPtr&& jwks) PURE; }; // Lookup issuer cache map. The cache only stores Jwks specified in the config. diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD new file mode 100644 index 0000000000000..16a7eb0e64809 --- /dev/null +++ b/test/extensions/filters/http/common/BUILD @@ -0,0 +1,38 @@ +licenses(["notice"]) # Apache 2 + +load( + "//bazel:envoy_build_system.bzl", + "envoy_cc_library", + "envoy_package", +) +load( + "//test/extensions:extensions_build_system.bzl", + "envoy_extension_cc_test", +) + +envoy_package() + +envoy_cc_library( + name = "test_common_lib", + hdrs = ["test_common.h"], +) + +envoy_cc_library( + name = "mock_lib", + hdrs = ["mock.h"], +) + +envoy_extension_cc_test( + name = "jwks_fetcher_test", + srcs = [ + "jwks_fetcher_test.cc", + ], + extension_name = "envoy.filters.http.jwt_authn", + deps = [ + ":test_common_lib", + "//source/extensions/filters/http/common:jwks_fetcher_lib", + "//test/mocks/http:http_mocks", + "//test/mocks/server:server_mocks", + "//test/test_common:utility_lib", + ], +) diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc new file mode 100644 index 0000000000000..f5415bced2c7f --- /dev/null +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -0,0 +1,134 @@ +#include +#include + +#include "common/http/message_impl.h" +#include "common/protobuf/utility.h" + +#include "extensions/filters/http/common/jwks_fetcher.h" + +#include "test/extensions/filters/http/common/test_common.h" +#include "test/mocks/http/mocks.h" +#include "test/mocks/server/mocks.h" +#include "test/test_common/utility.h" + +using ::envoy::api::v2::core::HttpUri; +using ::google::jwt_verify::Status; + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Common { +namespace { + +const std::string JwksUri = R"( +http_uri: + uri: https://pubkey_server/pubkey_path + cluster: pubkey_cluster + timeout: + seconds: 5 +)"; + +class JwksFetcherTest : public ::testing::Test { +public: + void SetUp() { MessageUtil::loadFromYaml(JwksUri, uri_); } + HttpUri uri_; + testing::NiceMock mock_factory_ctx_; +}; + +// A mock HTTP upstream with response body. +class MockUpstream { +public: + MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& status, + const std::string& response_body) + : request_(&mock_cm.async_client_), status_(status), response_body_(response_body) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke([this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) + -> Http::AsyncClient::Request* { + Http::MessagePtr response_message(new Http::ResponseMessageImpl( + Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", status_}}})); + if (response_body_.length()) { + response_message->body().reset(new Buffer::OwnedImpl(response_body_)); + } else { + response_message->body().reset(nullptr); + } + cb.onSuccess(std::move(response_message)); + return &request_; + })); + } + +private: + Http::MockAsyncClientRequest request_; + std::string status_; + std::string response_body_; +}; + +class MockJwksReceiver : public JwksFetcher::JwksReceiver { +public: + /* GoogleMock does handle r-value references hence the below construction. + * Expectations and assertions should be made on onJwksSuccessImpl in place + * of onJwksSuccess. + */ + void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { onJwksSuccessImpl(jwks); } + MOCK_METHOD1(onJwksSuccessImpl, void(google::jwt_verify::JwksPtr& jwks)); + MOCK_METHOD1(onJwksError, void(JwksFetcher::JwksReceiver::Failure reason)); +}; + +// Test findByIssuer +TEST_F(JwksFetcherTest, TestGetSuccess) { + // Setup + 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::_)).Times(1); + EXPECT_CALL(receiver, onJwksError(testing::_)).Times(0); + + // Act + fetcher->fetch(uri_, &receiver); +} + +TEST_F(JwksFetcherTest, TestGet400) { + // Setup + 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)).Times(1); + + // Act + fetcher->fetch(uri_, &receiver); +} + +TEST_F(JwksFetcherTest, TestGetNoBody) { + // Setup + 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)).Times(1); + + // Act + fetcher->fetch(uri_, &receiver); +} + +TEST_F(JwksFetcherTest, TestGetInvalidJwks) { + // Setup + 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::invalid_jwks)).Times(1); + + // Act + fetcher->fetch(uri_, &receiver); +} + +} // namespace +} // namespace Common +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h new file mode 100644 index 0000000000000..b0018981551c6 --- /dev/null +++ b/test/extensions/filters/http/common/mock.h @@ -0,0 +1,19 @@ +#include "extensions/filters/http/common/jwks_fetcher.h" + +#include "gmock/gmock.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Common { + +class MockJwksFetcher : public JwksFetcher { +public: + MOCK_METHOD0(close, void()); + MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver* receiver)); +}; + +} // namespace Common +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy diff --git a/test/extensions/filters/http/jwt_authn/test_common.h b/test/extensions/filters/http/common/test_common.h similarity index 88% rename from test/extensions/filters/http/jwt_authn/test_common.h rename to test/extensions/filters/http/common/test_common.h index 3568d4ef68d6f..4ce0a3fcc1db1 100644 --- a/test/extensions/filters/http/jwt_authn/test_common.h +++ b/test/extensions/filters/http/common/test_common.h @@ -3,7 +3,7 @@ namespace Envoy { namespace Extensions { namespace HttpFilters { -namespace JwtAuthn { +namespace Common { // A good public key const char PublicKey[] = R"( @@ -85,6 +85,19 @@ const char ExpiredToken[] = "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJodH "V1AdwgX_5n3SmQTacVN0WcSgk6YJRZG6VE8PjxZP9bEameBmbSB0810giKRpdTU1-" "RJtjq6aCSTD4CYXtW38T5uko4V-S4zifK3BXeituUTebkgoA"; +// An NotYetValid token +// {"iss":"https://example.com","sub":"test@example.com","aud":"example_service","nbf":9223372036854775807} +const char NotYetValidToken[] = + "eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9." + "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGxlLmNvbSIsImF1" + "ZCI6ImV4YW1wbGVfc2VydmljZSIsIm5iZiI6OTIyMzM3MjAzNjg1NDc3NTgwN30K" + ".izDa6aHNgbsbeRzucE0baXIP7SXOrgopYQ" + "ALLFAsKq_N0GvOyqpAZA9nwCAhqCkeKWcL-9gbQe3XJa0KN3FPa2NbW4ChenIjmf2" + "QYXOuOQaDu9QRTdHEY2Y4mRy6DiTZAsBHWGA71_cLX-rzTSO_8aC8eIqdHo898oJw" + "3E8ISKdryYjayb9X3wtF6KLgNomoD9_nqtOkliuLElD8grO0qHKI1xQurGZNaoeyi" + "V1AdwgX_5n3SmQTacVN0WcSgk6YJRZG6VE8PjxZP9bEameBmbSB0810giKRpdTU1-" + "RJtjq6aCSTD4CYXtW38T5uko4V-S4zifK3BXeituUTebkgoA"; + // A token with aud as invalid_service // Payload: // {"iss":"https://example.com","sub":"test@example.com","aud":"invalid_service","exp":2001001001} @@ -118,7 +131,7 @@ const char ExpectedPayloadValue[] = "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3V "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" "aWNlIn0"; -} // namespace JwtAuthn +} // namespace Common } // namespace HttpFilters } // namespace Extensions } // namespace Envoy diff --git a/test/extensions/filters/http/jwt_authn/BUILD b/test/extensions/filters/http/jwt_authn/BUILD index bfc7cd47ccf67..84acd9a9ef832 100644 --- a/test/extensions/filters/http/jwt_authn/BUILD +++ b/test/extensions/filters/http/jwt_authn/BUILD @@ -12,11 +12,6 @@ load( envoy_package() -envoy_cc_library( - name = "test_common_lib", - hdrs = ["test_common.h"], -) - envoy_cc_library( name = "mock_lib", hdrs = ["mock.h"], @@ -55,10 +50,9 @@ envoy_extension_cc_test( ], extension_name = "envoy.filters.http.jwt_authn", deps = [ - ":test_common_lib", "//source/extensions/filters/http/jwt_authn:config", + "//test/extensions/filters/http/common:test_common_lib", "//test/mocks/server:server_mocks", - "//test/test_common:utility_lib", ], ) @@ -69,8 +63,9 @@ envoy_extension_cc_test( ], extension_name = "envoy.filters.http.jwt_authn", deps = [ - ":test_common_lib", + "//source/extensions/filters/http/common:jwks_fetcher_lib", "//source/extensions/filters/http/jwt_authn:jwks_cache_lib", + "//test/extensions/filters/http/common:test_common_lib", "//test/test_common:utility_lib", ], ) @@ -83,8 +78,10 @@ envoy_extension_cc_test( extension_name = "envoy.filters.http.jwt_authn", deps = [ ":mock_lib", - ":test_common_lib", + "//source/extensions/filters/http/common:jwks_fetcher_lib", "//source/extensions/filters/http/jwt_authn:authenticator_lib", + "//test/extensions/filters/http/common:mock_lib", + "//test/extensions/filters/http/common:test_common_lib", "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", ], @@ -95,9 +92,9 @@ envoy_extension_cc_test( srcs = ["filter_integration_test.cc"], extension_name = "envoy.filters.http.jwt_authn", deps = [ - ":test_common_lib", "//source/extensions/filters/http/jwt_authn:config", "//test/config:utility_lib", + "//test/extensions/filters/http/common:test_common_lib", "//test/integration:http_protocol_integration_lib", ], ) diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index 02abd237488e6..df26c8f8c6e3a 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -1,10 +1,12 @@ #include "common/http/message_impl.h" #include "common/protobuf/utility.h" +#include "extensions/filters/http/common/jwks_fetcher.h" #include "extensions/filters/http/jwt_authn/authenticator.h" +#include "test/extensions/filters/http/common/mock.h" +#include "test/extensions/filters/http/common/test_common.h" #include "test/extensions/filters/http/jwt_authn/mock.h" -#include "test/extensions/filters/http/jwt_authn/test_common.h" #include "test/mocks/server/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" @@ -16,6 +18,18 @@ using ::google::jwt_verify::Status; using ::testing::_; using ::testing::Invoke; using ::testing::NiceMock; +using ::testing::_; +using Envoy::Extensions::HttpFilters::Common::ExampleConfig; +using Envoy::Extensions::HttpFilters::Common::ExpectedPayloadValue; +using Envoy::Extensions::HttpFilters::Common::ExpiredToken; +using Envoy::Extensions::HttpFilters::Common::GoodToken; +using Envoy::Extensions::HttpFilters::Common::InvalidAudToken; +using Envoy::Extensions::HttpFilters::Common::JwksFetcher; +using Envoy::Extensions::HttpFilters::Common::MockJwksFetcher; +using Envoy::Extensions::HttpFilters::Common::NonExistKidToken; +using Envoy::Extensions::HttpFilters::Common::NotYetValidToken; +using Envoy::Extensions::HttpFilters::Common::ProviderName; +using Envoy::Extensions::HttpFilters::Common::PublicKey; namespace Envoy { namespace Extensions { @@ -31,46 +45,31 @@ class AuthenticatorTest : public ::testing::Test { void CreateAuthenticator() { filter_config_ = ::std::make_shared(proto_config_, "", mock_factory_ctx_); - auth_ = Authenticator::create(filter_config_); + fetcher_ = new MockJwksFetcher; + fetcherPtr_.reset(fetcher_); + auth_ = Authenticator::create(filter_config_, fetcherPtr_); + jwks_ = ::google::jwt_verify::Jwks::createFrom(PublicKey, ::google::jwt_verify::Jwks::JWKS); + EXPECT_TRUE(jwks_->getStatus() == Status::Ok); } JwtAuthentication proto_config_; FilterConfigSharedPtr filter_config_; - std::unique_ptr auth_; + MockJwksFetcher* fetcher_; + JwksFetcher::JwksFetcherPtr fetcherPtr_; + AuthenticatorPtr auth_; + ::google::jwt_verify::JwksPtr jwks_; NiceMock mock_factory_ctx_; MockAuthenticatorCallbacks mock_cb_; }; -// A mock HTTP upstream with response body. -class MockUpstream { -public: - MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& response_body) - : request_(&mock_cm.async_client_), response_body_(response_body) { - ON_CALL(mock_cm.async_client_, send_(_, _, _)) - .WillByDefault(Invoke([this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) - -> Http::AsyncClient::Request* { - Http::MessagePtr response_message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - response_message->body().reset(new Buffer::OwnedImpl(response_body_)); - cb.onSuccess(std::move(response_message)); - called_count_++; - return &request_; - })); - } - - int called_count() const { return called_count_; } - -private: - Http::MockAsyncClientRequest request_; - std::string response_body_; - int called_count_{}; -}; - // 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) { - MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, PublicKey); + EXPECT_CALL(*fetcher_, fetch(_, _)) + .WillOnce(Invoke( + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { + receiver->onJwksSuccess(std::move(jwks_)); + })); // Test OK pubkey and its cache for (int i = 0; i < 10; i++) { @@ -87,17 +86,18 @@ TEST_F(AuthenticatorTest, TestOkJWTandCache) { // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); } - - EXPECT_EQ(mock_pubkey.called_count(), 1); } // This test verifies the Jwt is forwarded if "forward" flag is set. TEST_F(AuthenticatorTest, TestForwardJwt) { - // Confit forward_jwt flag + // Confirm forward_jwt flag (*proto_config_.mutable_providers())[std::string(ProviderName)].set_forward(true); CreateAuthenticator(); - - MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, PublicKey); + EXPECT_CALL(*fetcher_, fetch(_, _)) + .WillOnce(Invoke( + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { + receiver->onJwksSuccess(std::move(jwks_)); + })); // Test OK pubkey and its cache auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; @@ -115,8 +115,11 @@ TEST_F(AuthenticatorTest, TestForwardJwt) { // This test verifies the Jwt with non existing kid TEST_F(AuthenticatorTest, TestJwtWithNonExistKid) { - MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, PublicKey); - + EXPECT_CALL(*fetcher_, fetch(_, _)) + .WillOnce(Invoke( + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { + receiver->onJwksSuccess(std::move(jwks_)); + })); // Test OK pubkey and its cache auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(NonExistKidToken)}}; @@ -131,7 +134,8 @@ TEST_F(AuthenticatorTest, TestJwtWithNonExistKid) { // This test verifies if Jwt is missing, proper status is called. TEST_F(AuthenticatorTest, TestMissedJWT) { - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); + MockAuthenticatorCallbacks mock_cb; EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { ASSERT_EQ(status, Status::JwtMissed); })); @@ -143,7 +147,7 @@ TEST_F(AuthenticatorTest, TestMissedJWT) { // This test verifies if Jwt is invalid, JwtBadFormat status is returned. TEST_F(AuthenticatorTest, TestInvalidJWT) { - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { ASSERT_EQ(status, Status::JwtBadFormat); })); @@ -155,7 +159,7 @@ TEST_F(AuthenticatorTest, TestInvalidJWT) { // This test verifies if Authorization header has invalid prefix, JwtMissed status is returned TEST_F(AuthenticatorTest, TestInvalidPrefix) { - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { ASSERT_EQ(status, Status::JwtMissed); })); @@ -179,7 +183,7 @@ TEST_F(AuthenticatorTest, TestNonExpiringJWT) { // This test verifies when a JWT is expired, JwtExpired status is returned. TEST_F(AuthenticatorTest, TestExpiredJWT) { - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { ASSERT_EQ(status, Status::JwtExpired); })); @@ -188,43 +192,53 @@ TEST_F(AuthenticatorTest, TestExpiredJWT) { auth_->verify(headers, &mock_cb_); } -// This test verifies when a JWT is with invalid audience, JwtAudienceNotAllowed is returned. -TEST_F(AuthenticatorTest, TestNonMatchAudJWT) { - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); +// This test verifies when a JWT is not yet valid, JwtNotYetValid status is returned. +TEST_F(AuthenticatorTest, TestNotYetValidJWT) { + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwtAudienceNotAllowed); + ASSERT_EQ(status, Status::JwtNotYetValid); })); auto headers = - Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(InvalidAudToken)}}; + Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(NotYetValidToken)}}; auth_->verify(headers, &mock_cb_); } -// This test verifies when invalid cluster is configured for remote jwks, JwksFetchFail is returned. -TEST_F(AuthenticatorTest, TestWrongCluster) { - // Get returns nullptr - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, get(_)) - .WillOnce(Invoke([](const std::string& cluster) -> Upstream::ThreadLocalCluster* { - EXPECT_EQ(cluster, "pubkey_cluster"); - return nullptr; - })); +// This test verifies when an inline JWKS is misconfigured, JwksNoValidKeys is returns +TEST_F(AuthenticatorTest, TestInvalidLocalJwks) { + auto& provider = (*proto_config_.mutable_providers())[std::string(ProviderName)]; + provider.clear_remote_jwks(); + provider.mutable_local_jwks()->set_inline_string("invalid"); + CreateAuthenticator(); - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwksFetchFail); + ASSERT_EQ(status, Status::JwksNoValidKeys); })); auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; auth_->verify(headers, &mock_cb_); } +// This test verifies when a JWT is with invalid audience, JwtAudienceNotAllowed is returned. +TEST_F(AuthenticatorTest, TestNonMatchAudJWT) { + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); + EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { + ASSERT_EQ(status, Status::JwtAudienceNotAllowed); + })); + + auto headers = + Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(InvalidAudToken)}}; + auth_->verify(headers, &mock_cb_); +} + // This test verifies when Jwt issuer is not configured, JwtUnknownIssuer is returned. TEST_F(AuthenticatorTest, TestIssuerNotFound) { // Create a config with an other issuer. (*proto_config_.mutable_providers())[std::string(ProviderName)].set_issuer("other_issuer"); CreateAuthenticator(); - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)).Times(0); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { ASSERT_EQ(status, Status::JwtUnknownIssuer); })); @@ -235,27 +249,10 @@ TEST_F(AuthenticatorTest, TestIssuerNotFound) { // This test verifies that when Jwks fetching fails, JwksFetchFail status is returned. TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { - NiceMock async_client; - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)) - .WillOnce(Invoke([&](const std::string& cluster) -> Http::AsyncClient& { - EXPECT_EQ(cluster, "pubkey_cluster"); - return async_client; - })); - - Http::MockAsyncClientRequest request(&async_client); - Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(async_client, send_(_, _, _)) - .WillOnce(Invoke( - [&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":path", "/pubkey_path"}, - {":authority", "pubkey_server"}, - {":method", "GET"}, - }), - message->headers()); - callbacks = &cb; - return &request; + EXPECT_CALL(*fetcher_, fetch(_, _)) + .WillOnce( + Invoke([](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { + receiver->onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks); })); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { @@ -267,122 +264,14 @@ TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { Http::MessagePtr response_message(new Http::ResponseMessageImpl( Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "401"}}})); - callbacks->onSuccess(std::move(response_message)); -} - -// This test verifies that when Jwks fetching return empty body -TEST_F(AuthenticatorTest, TestPubkeyFetchFailWithEmptyBody) { - Http::MockAsyncClientRequest request(&mock_factory_ctx_.cluster_manager_.async_client_); - Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(mock_factory_ctx_.cluster_manager_.async_client_, send_(_, _, _)) - .WillOnce(Invoke( - [&](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - callbacks = &cb; - return &request; - })); - - EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwksFetchFail); - })); - - auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; - auth_->verify(headers, &mock_cb_); - - Http::MessagePtr response_message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - callbacks->onSuccess(std::move(response_message)); -} - -// This test verifies that when Jwks fetching returned with onFailure -TEST_F(AuthenticatorTest, TestPubkeyFetchFailWithOnFailure) { - Http::MockAsyncClientRequest request(&mock_factory_ctx_.cluster_manager_.async_client_); - Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(mock_factory_ctx_.cluster_manager_.async_client_, send_(_, _, _)) - .WillOnce(Invoke( - [&](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - callbacks = &cb; - return &request; - })); - - EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwksFetchFail); - })); - - auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; - auth_->verify(headers, &mock_cb_); - - callbacks->onFailure(Http::AsyncClient::FailureReason::Reset); -} - -// This test verifies when an invalid Jwks is fetched, JwksParseError status is returned. -TEST_F(AuthenticatorTest, TestInvalidPubkey) { - NiceMock async_client; - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)) - .WillOnce(Invoke([&](const std::string& cluster) -> Http::AsyncClient& { - EXPECT_EQ(cluster, "pubkey_cluster"); - return async_client; - })); - - Http::MockAsyncClientRequest request(&async_client); - Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(async_client, send_(_, _, _)) - .WillOnce(Invoke( - [&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":path", "/pubkey_path"}, - {":authority", "pubkey_server"}, - {":method", "GET"}, - }), - message->headers()); - callbacks = &cb; - return &request; - })); - - EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwksParseError); - })); - - auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; - auth_->verify(headers, &mock_cb_); - - Http::MessagePtr response_message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", "200"}}})); - response_message->body().reset(new Buffer::OwnedImpl("invalid publik key")); - callbacks->onSuccess(std::move(response_message)); } // This test verifies when a Jwks fetching is not completed yet, but onDestory() is called, -// onComplete() callback should not be called, but internal request->cancel() should be called. +// onComplete() callback should not be called, but internal fetcher_->close() should be called. // Most importantly, no crash. TEST_F(AuthenticatorTest, TestOnDestroy) { - NiceMock async_client; - EXPECT_CALL(mock_factory_ctx_.cluster_manager_, httpAsyncClientForCluster(_)) - .WillOnce(Invoke([&](const std::string& cluster) -> Http::AsyncClient& { - EXPECT_EQ(cluster, "pubkey_cluster"); - return async_client; - })); - - Http::MockAsyncClientRequest request(&async_client); - Http::AsyncClient::Callbacks* callbacks; - EXPECT_CALL(async_client, send_(_, _, _)) - .WillOnce(Invoke( - [&](Http::MessagePtr& message, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - EXPECT_EQ((Http::TestHeaderMapImpl{ - {":path", "/pubkey_path"}, - {":authority", "pubkey_server"}, - {":method", "GET"}, - }), - message->headers()); - callbacks = &cb; - return &request; - })); - - // Cancel is called once. - EXPECT_CALL(request, cancel()).Times(1); + EXPECT_CALL(*fetcher_, fetch(_, _)).Times(1); + EXPECT_CALL(*fetcher_, close()).Times(1); // onComplete() should not be called. EXPECT_CALL(mock_cb_, onComplete(_)).Times(0); @@ -400,8 +289,12 @@ TEST_F(AuthenticatorTest, TestNoForwardPayloadHeader) { auto& provider0 = (*proto_config_.mutable_providers())[std::string(ProviderName)]; provider0.clear_forward_payload_header(); CreateAuthenticator(); + EXPECT_CALL(*fetcher_, fetch(_, _)) + .WillOnce(Invoke( + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { + receiver->onJwksSuccess(std::move(jwks_)); + })); - MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, PublicKey); auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; MockAuthenticatorCallbacks mock_cb; EXPECT_CALL(mock_cb, onComplete(_)).WillOnce(Invoke([](const Status& status) { diff --git a/test/extensions/filters/http/jwt_authn/filter_factory_test.cc b/test/extensions/filters/http/jwt_authn/filter_factory_test.cc index dfb2c94ff5842..565f12cd3fe4b 100644 --- a/test/extensions/filters/http/jwt_authn/filter_factory_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_factory_test.cc @@ -2,7 +2,7 @@ #include "extensions/filters/http/jwt_authn/filter_factory.h" -#include "test/extensions/filters/http/jwt_authn/test_common.h" +#include "test/extensions/filters/http/common/test_common.h" #include "test/mocks/server/mocks.h" #include "gmock/gmock.h" @@ -21,7 +21,7 @@ namespace JwtAuthn { TEST(HttpJwtAuthnFilterFactoryTest, GoodRemoteJwks) { FilterFactory factory; ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto(); - MessageUtil::loadFromYaml(ExampleConfig, *proto_config); + MessageUtil::loadFromYaml(Common::ExampleConfig, *proto_config); NiceMock context; @@ -35,7 +35,7 @@ TEST(HttpJwtAuthnFilterFactoryTest, GoodLocalJwks) { JwtAuthentication proto_config; auto& provider = (*proto_config.mutable_providers())["provider"]; provider.set_issuer("issuer"); - provider.mutable_local_jwks()->set_inline_string(PublicKey); + provider.mutable_local_jwks()->set_inline_string(Common::PublicKey); NiceMock context; FilterFactory factory; diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 536d373f21aaf..1f79a6e8385f1 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -2,7 +2,7 @@ #include "extensions/filters/http/well_known_names.h" -#include "test/extensions/filters/http/jwt_authn/test_common.h" +#include "test/extensions/filters/http/common/test_common.h" #include "test/integration/http_protocol_integration.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; @@ -16,13 +16,13 @@ namespace { std::string getFilterConfig(bool use_local_jwks) { JwtAuthentication proto_config; - MessageUtil::loadFromYaml(ExampleConfig, proto_config); + MessageUtil::loadFromYaml(Common::ExampleConfig, proto_config); if (use_local_jwks) { - auto& provider0 = (*proto_config.mutable_providers())[std::string(ProviderName)]; + auto& provider0 = (*proto_config.mutable_providers())[std::string(Common::ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); - local_jwks->set_inline_string(PublicKey); + local_jwks->set_inline_string(Common::PublicKey); } HttpFilter filter; @@ -49,19 +49,17 @@ TEST_P(LocalJwksIntegrationTest, WithGoodToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(GoodToken)}, + {"Authorization", "Bearer " + std::string(Common::GoodToken)}, }); waitForNextUpstreamRequest(); const auto* payload_entry = upstream_request_->headers().get(Http::LowerCaseString("sec-istio-auth-userinfo")); EXPECT_TRUE(payload_entry != nullptr); - EXPECT_EQ(payload_entry->value().getStringView(), ExpectedPayloadValue); + EXPECT_EQ(payload_entry->value().getStringView(), Common::ExpectedPayloadValue); // Verify the token is removed. EXPECT_FALSE(upstream_request_->headers().Authorization()); - upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); - response->waitForEndStream(); ASSERT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); @@ -79,7 +77,7 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(ExpiredToken)}, + {"Authorization", "Bearer " + std::string(Common::ExpiredToken)}, }); response->waitForEndStream(); @@ -180,17 +178,17 @@ TEST_P(RemoteJwksIntegrationTest, WithGoodToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(GoodToken)}, + {"Authorization", "Bearer " + std::string(Common::GoodToken)}, }); - waitForJwksResponse("200", PublicKey); + waitForJwksResponse("200", Common::PublicKey); waitForNextUpstreamRequest(); const auto* payload_entry = upstream_request_->headers().get(Http::LowerCaseString("sec-istio-auth-userinfo")); EXPECT_TRUE(payload_entry != nullptr); - EXPECT_EQ(payload_entry->value().getStringView(), ExpectedPayloadValue); + EXPECT_EQ(payload_entry->value().getStringView(), Common::ExpectedPayloadValue); // Verify the token is removed. EXPECT_FALSE(upstream_request_->headers().Authorization()); @@ -215,7 +213,7 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedJwks) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(GoodToken)}, + {"Authorization", "Bearer " + std::string(Common::GoodToken)}, }); // Fails the jwks fetching. 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 a679043628aaf..5bdbb644bbf6b 100644 --- a/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc +++ b/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc @@ -5,7 +5,7 @@ #include "extensions/filters/http/jwt_authn/jwks_cache.h" -#include "test/extensions/filters/http/jwt_authn/test_common.h" +#include "test/extensions/filters/http/common/test_common.h" #include "test/test_common/utility.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; @@ -20,12 +20,14 @@ namespace { class JwksCacheTest : public ::testing::Test { public: void SetUp() { - MessageUtil::loadFromYaml(ExampleConfig, config_); + MessageUtil::loadFromYaml(Common::ExampleConfig, config_); cache_ = JwksCache::create(config_); + jwks_ = google::jwt_verify::Jwks::createFrom(Common::PublicKey, google::jwt_verify::Jwks::JWKS); } JwtAuthentication config_; JwksCachePtr cache_; + google::jwt_verify::JwksPtr jwks_; }; // Test findByIssuer @@ -36,7 +38,7 @@ TEST_F(JwksCacheTest, TestFindByIssuer) { // Test setRemoteJwks and its expiration TEST_F(JwksCacheTest, TestSetRemoteJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; // Set cache_duration to 1 second to test expiration provider0.mutable_remote_jwks()->mutable_cache_duration()->set_seconds(1); cache_ = JwksCache::create(config_); @@ -44,7 +46,7 @@ TEST_F(JwksCacheTest, TestSetRemoteJwks) { auto jwks = cache_->findByIssuer("https://example.com"); EXPECT_TRUE(jwks->getJwksObj() == nullptr); - EXPECT_EQ(jwks->setRemoteJwks(PublicKey), Status::Ok); + EXPECT_EQ(jwks->setRemoteJwks(std::move(jwks_))->getStatus(), Status::Ok); EXPECT_FALSE(jwks->getJwksObj() == nullptr); EXPECT_FALSE(jwks->isExpired()); @@ -55,7 +57,7 @@ TEST_F(JwksCacheTest, TestSetRemoteJwks) { // Test setRemoteJwks and use default cache duration. TEST_F(JwksCacheTest, TestSetRemoteJwksWithDefaultCacheDuration) { - auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; // Clear cache_duration to use default one. provider0.mutable_remote_jwks()->clear_cache_duration(); cache_ = JwksCache::create(config_); @@ -63,17 +65,17 @@ TEST_F(JwksCacheTest, TestSetRemoteJwksWithDefaultCacheDuration) { auto jwks = cache_->findByIssuer("https://example.com"); EXPECT_TRUE(jwks->getJwksObj() == nullptr); - EXPECT_EQ(jwks->setRemoteJwks(PublicKey), Status::Ok); + EXPECT_EQ(jwks->setRemoteJwks(std::move(jwks_))->getStatus(), Status::Ok); EXPECT_FALSE(jwks->getJwksObj() == nullptr); EXPECT_FALSE(jwks->isExpired()); } // Test a good local jwks TEST_F(JwksCacheTest, TestGoodInlineJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); - local_jwks->set_inline_string(PublicKey); + local_jwks->set_inline_string(Common::PublicKey); cache_ = JwksCache::create(config_); @@ -84,7 +86,7 @@ TEST_F(JwksCacheTest, TestGoodInlineJwks) { // Test a bad local jwks TEST_F(JwksCacheTest, TestBadInlineJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); local_jwks->set_inline_string("BAD-JWKS"); From d3fbad5861f7fdee1906bdb86a783d737e8ee3d8 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Fri, 24 Aug 2018 10:47:22 +0100 Subject: [PATCH 02/22] Changes from original PR review. - Changes from PR review. - Added missing tests for JwksFetcher::cancel(). - Added comment and TODO around a potential bug in jwt_authn::authenticator. Signed-off-by: Nick A. Smith --- .../filters/http/common/jwks_fetcher.cc | 4 +- .../filters/http/common/jwks_fetcher.h | 14 +++++-- .../filters/http/jwt_authn/authenticator.cc | 24 ++++++----- .../filters/http/jwt_authn/authenticator.h | 3 +- .../filters/http/jwt_authn/filter_factory.cc | 2 +- .../filters/http/common/jwks_fetcher_test.cc | 40 ++++++++++++++++--- test/extensions/filters/http/common/mock.h | 2 +- .../http/jwt_authn/authenticator_test.cc | 17 ++++---- .../http/jwt_authn/filter_integration_test.cc | 2 +- 9 files changed, 73 insertions(+), 35 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index b9bf1e0568f18..ce148bbb77e21 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -23,7 +23,7 @@ class JwksFetcherImpl : public JwksFetcher, public: JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } - void close() { + void cancel() { if (request_) { request_->cancel(); request_ = nullptr; @@ -82,7 +82,7 @@ class JwksFetcherImpl : public JwksFetcher, }; } // namespace -JwksFetcher::JwksFetcherPtr JwksFetcher::create(Upstream::ClusterManager& cm) { +JwksFetcherPtr JwksFetcher::create(Upstream::ClusterManager& cm) { return JwksFetcherPtr(new JwksFetcherImpl(cm)); } } // namespace Common diff --git a/source/extensions/filters/http/common/jwks_fetcher.h b/source/extensions/filters/http/common/jwks_fetcher.h index 9f68308f1ee44..19fb50587433c 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.h +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -11,10 +11,16 @@ namespace Extensions { namespace HttpFilters { namespace Common { +class JwksFetcher; +typedef std::unique_ptr JwksFetcherPtr; +/** + * JwksFetcher interface can be used to retrieve remote JWKS + * (https://tools.ietf.org/html/rfc7517) data structures returning a concrete, + * type-safe representation. An instance of this interface is designed to + * retrieve a single JWKS and should not be re-used to fetch further instances. + */ class JwksFetcher { public: - typedef std::unique_ptr JwksFetcherPtr; - class JwksReceiver { public: enum class Failure { @@ -40,9 +46,9 @@ class JwksFetcher { virtual ~JwksFetcher(){}; /* - * Close/stop any inflight request. + * Cancel any inflight request. */ - virtual void close() PURE; + virtual void cancel() PURE; /* * Retrieve a JWKS resource from a remote HTTP host. diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index f218c33388f0d..12d0183623d77 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -26,7 +26,7 @@ class AuthenticatorImpl : public Logger::Loggable, public Authenticator, public Common::JwksFetcher::JwksReceiver { public: - AuthenticatorImpl(FilterConfigSharedPtr config, Common::JwksFetcher::JwksFetcherPtr& fetcher) + AuthenticatorImpl(FilterConfigSharedPtr config, Common::JwksFetcherPtr&& fetcher) : config_(config), fetcher_(std::move(fetcher)) {} // Following functions are for JwksFetcher::JwksReceiver interface @@ -51,7 +51,7 @@ class AuthenticatorImpl : public Logger::Loggable, FilterConfigSharedPtr config_; // The Jwks fetcher object - Common::JwksFetcher::JwksFetcherPtr fetcher_; + Common::JwksFetcherPtr fetcher_; // The token data JwtLocationConstPtr token_; @@ -64,11 +64,6 @@ class AuthenticatorImpl : public Logger::Loggable, Http::HeaderMap* headers_{}; // The on_done function. Authenticator::Callbacks* callback_{}; - - // The pending uri_, only used for logging. - std::string uri_; - // The pending remote request so it can be canceled. - Http::AsyncClient::Request* request_{}; }; void AuthenticatorImpl::sanitizePayloadHeaders(Http::HeaderMap& headers) const { @@ -124,7 +119,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback doneWithStatus(Status::JwtNotYetValid); return; } - if (jwt_.exp && jwt_.exp_ < unix_timestamp) { + if (jwt_.exp_ && jwt_.exp_ < unix_timestamp) { doneWithStatus(Status::JwtExpired); return; } @@ -142,6 +137,13 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback auto jwks_obj = jwks_data_->getJwksObj(); if (jwks_obj != nullptr && !jwks_data_->isExpired()) { + // TODO: It would seem there's a window of error whereby if the JWT issuer + // has started signing with a new key that's not in our cache, then the + // verification will fail even though the JWT is valid. A simple fix + // would be to check the JWS kid header field; if present check we have + // the key cached, if we do proceed to verify else try a new JWKS retrieval. + // JWTs without a kid header field in the JWS we might be best to get each + // time? This all only matters for remote JWKS. verifyKey(); return; } @@ -170,7 +172,7 @@ void AuthenticatorImpl::onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { void AuthenticatorImpl::onJwksError(Failure) { doneWithStatus(Status::JwksFetchFail); } -void AuthenticatorImpl::onDestroy() { fetcher_->close(); } +void AuthenticatorImpl::onDestroy() { fetcher_->cancel(); } // Verify with a specific public key. void AuthenticatorImpl::verifyKey() { @@ -210,8 +212,8 @@ void AuthenticatorImpl::doneWithStatus(const Status& status) { } // namespace AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config, - Common::JwksFetcher::JwksFetcherPtr& fetcher) { - return std::make_unique(config, fetcher); + Common::JwksFetcherPtr&& fetcher) { + return std::make_unique(config, std::move(fetcher)); } } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/authenticator.h b/source/extensions/filters/http/jwt_authn/authenticator.h index 2904f0fe26645..6160bbb2452ea 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.h +++ b/source/extensions/filters/http/jwt_authn/authenticator.h @@ -36,8 +36,7 @@ class Authenticator { virtual void sanitizePayloadHeaders(Http::HeaderMap& headers) const PURE; // Authenticator factory function. - static AuthenticatorPtr create(FilterConfigSharedPtr config, - Common::JwksFetcher::JwksFetcherPtr& fetcher); + static AuthenticatorPtr create(FilterConfigSharedPtr config, Common::JwksFetcherPtr&& fetcher); }; } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/filter_factory.cc b/source/extensions/filters/http/jwt_authn/filter_factory.cc index bfa9cf131ea46..6dc0ce454c8e4 100644 --- a/source/extensions/filters/http/jwt_authn/filter_factory.cc +++ b/source/extensions/filters/http/jwt_authn/filter_factory.cc @@ -48,7 +48,7 @@ FilterFactory::createFilterFactoryFromProtoTyped(const JwtAuthentication& proto_ return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { auto jwks_fetcher = Common::JwksFetcher::create(filter_config->cm()); callbacks.addStreamDecoderFilter(std::make_shared( - filter_config->stats(), Authenticator::create(filter_config, jwks_fetcher))); + filter_config->stats(), Authenticator::create(filter_config, std::move(jwks_fetcher)))); }; } diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index f5415bced2c7f..84e7101f7a247 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -21,11 +21,10 @@ namespace Common { namespace { const std::string JwksUri = R"( -http_uri: - uri: https://pubkey_server/pubkey_path - cluster: pubkey_cluster - timeout: - seconds: 5 +uri: https://pubkey_server/pubkey_path +cluster: pubkey_cluster +timeout: + seconds: 5 )"; class JwksFetcherTest : public ::testing::Test { @@ -57,6 +56,18 @@ class MockUpstream { })); } + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke([request](Http::MessagePtr&, Http::AsyncClient::Callbacks&, + const absl::optional&) + -> Http::AsyncClient::Request* { + Http::MessagePtr response_message( + new Http::ResponseMessageImpl(Http::HeaderMapPtr{new Http::TestHeaderMapImpl{}})); + return request; + })); + } + private: Http::MockAsyncClientRequest request_; std::string status_; @@ -127,6 +138,25 @@ TEST_F(JwksFetcherTest, TestGetInvalidJwks) { fetcher->fetch(uri_, &receiver); } +TEST_F(JwksFetcherTest, TestCancel) { + // Setup + Http::MockAsyncClientRequest request(&(mock_factory_ctx_.cluster_manager_.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()).Times(1); + EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); + EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks)).Times(0); + + // Act + fetcher->fetch(uri_, &receiver); + // Proper cancel + fetcher->cancel(); + // Re-entrant cancel + fetcher->cancel(); +} + } // namespace } // namespace Common } // namespace HttpFilters diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h index b0018981551c6..def27e58b4244 100644 --- a/test/extensions/filters/http/common/mock.h +++ b/test/extensions/filters/http/common/mock.h @@ -9,7 +9,7 @@ namespace Common { class MockJwksFetcher : public JwksFetcher { public: - MOCK_METHOD0(close, void()); + MOCK_METHOD0(cancel, void()); MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver* receiver)); }; diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index df26c8f8c6e3a..957171c8050d7 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -14,22 +14,23 @@ #include "gtest/gtest.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; -using ::google::jwt_verify::Status; -using ::testing::_; -using ::testing::Invoke; -using ::testing::NiceMock; -using ::testing::_; using Envoy::Extensions::HttpFilters::Common::ExampleConfig; using Envoy::Extensions::HttpFilters::Common::ExpectedPayloadValue; using Envoy::Extensions::HttpFilters::Common::ExpiredToken; using Envoy::Extensions::HttpFilters::Common::GoodToken; using Envoy::Extensions::HttpFilters::Common::InvalidAudToken; using Envoy::Extensions::HttpFilters::Common::JwksFetcher; +using Envoy::Extensions::HttpFilters::Common::JwksFetcherPtr; using Envoy::Extensions::HttpFilters::Common::MockJwksFetcher; using Envoy::Extensions::HttpFilters::Common::NonExistKidToken; +using Envoy::Extensions::HttpFilters::Common::NonExpiringToken; using Envoy::Extensions::HttpFilters::Common::NotYetValidToken; using Envoy::Extensions::HttpFilters::Common::ProviderName; using Envoy::Extensions::HttpFilters::Common::PublicKey; +using ::google::jwt_verify::Status; +using ::testing::_; +using ::testing::Invoke; +using ::testing::NiceMock; namespace Envoy { namespace Extensions { @@ -47,7 +48,7 @@ class AuthenticatorTest : public ::testing::Test { filter_config_ = ::std::make_shared(proto_config_, "", mock_factory_ctx_); fetcher_ = new MockJwksFetcher; fetcherPtr_.reset(fetcher_); - auth_ = Authenticator::create(filter_config_, fetcherPtr_); + auth_ = Authenticator::create(filter_config_, std::move(fetcherPtr_)); jwks_ = ::google::jwt_verify::Jwks::createFrom(PublicKey, ::google::jwt_verify::Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); } @@ -55,7 +56,7 @@ class AuthenticatorTest : public ::testing::Test { JwtAuthentication proto_config_; FilterConfigSharedPtr filter_config_; MockJwksFetcher* fetcher_; - JwksFetcher::JwksFetcherPtr fetcherPtr_; + JwksFetcherPtr fetcherPtr_; AuthenticatorPtr auth_; ::google::jwt_verify::JwksPtr jwks_; NiceMock mock_factory_ctx_; @@ -271,7 +272,7 @@ TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { // Most importantly, no crash. TEST_F(AuthenticatorTest, TestOnDestroy) { EXPECT_CALL(*fetcher_, fetch(_, _)).Times(1); - EXPECT_CALL(*fetcher_, close()).Times(1); + EXPECT_CALL(*fetcher_, cancel()).Times(1); // onComplete() should not be called. EXPECT_CALL(mock_cb_, onComplete(_)).Times(0); diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 1f79a6e8385f1..2b51a7d14af4c 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -96,7 +96,7 @@ TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(ExpiredToken)}, + {"Authorization", "Bearer " + std::string(Common::ExpiredToken)}, }); response->waitForEndStream(); From c90ebf7ffaa4cb9eee6771b35496a5b0cc11a82b Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Fri, 24 Aug 2018 12:27:16 +0100 Subject: [PATCH 03/22] Fixing bazel/repositories.bzl formatting Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 089bdc2b592c9..94dd34233142a 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -465,9 +465,9 @@ def _com_google_absl(): actual = "@com_google_absl//absl/debugging:symbolize", ) native.bind( - name = "abseil_time", - actual = "@com_google_absl//absl/time:time", - ) + name = "abseil_time", + actual = "@com_google_absl//absl/time:time", + ) def _com_google_protobuf(): _repository_impl("com_google_protobuf") From 1b2e6c764ff61f40803b32cc828ede71e7a11023 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Tue, 28 Aug 2018 21:13:46 +0100 Subject: [PATCH 04/22] Fixes from PR Review - Removed unused response_message variable - use of std::make_unique - Fix enum inline with style guide - Added missing param documentation - Added comments re: nbf and exp JWT fields and their default values - Used reference instead of pointer for non-null function param Signed-off-by: Nick A. Smith --- .../filters/http/common/jwks_fetcher.cc | 14 ++++++------- .../filters/http/common/jwks_fetcher.h | 9 +++++---- .../filters/http/jwt_authn/authenticator.cc | 6 +++++- .../filters/http/common/jwks_fetcher_test.cc | 20 +++++++++---------- test/extensions/filters/http/common/mock.h | 2 +- .../http/jwt_authn/authenticator_test.cc | 20 +++++++++---------- 6 files changed, 37 insertions(+), 34 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index ce148bbb77e21..b99f15b231c7d 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -31,9 +31,9 @@ class JwksFetcherImpl : public JwksFetcher, } } - void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver* receiver) { + void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) { ENVOY_LOG(trace, "{}", __func__); - receiver_ = receiver; + receiver_ = &receiver; uri_ = &uri; Http::MessagePtr message = Http::Utility::prepareHeaders(uri); message->headers().insertMethod().value().setReference(Http::Headers::get().MethodValues.Get); @@ -61,29 +61,29 @@ class JwksFetcherImpl : public JwksFetcher, receiver_->onJwksSuccess(std::move(jwks)); } else { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: invalid jwks", __func__, uri_->uri()); - receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks); } } else { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: body is empty", __func__, uri_->uri()); - receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } } else { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: response status code {}", __func__, uri_->uri(), status_code); - receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } } void onFailure(Http::AsyncClient::FailureReason reason) { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, uri_->uri(), enumToInt(reason)); - receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::network); + receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } }; } // namespace JwksFetcherPtr JwksFetcher::create(Upstream::ClusterManager& cm) { - return JwksFetcherPtr(new JwksFetcherImpl(cm)); + return std::make_unique(cm); } } // 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 19fb50587433c..748ac0943d054 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.h +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -24,9 +24,9 @@ class JwksFetcher { class JwksReceiver { public: enum class Failure { - unknown, - network, - invalid_jwks, + Unknown, + Network, + InvalidJwks, }; virtual ~JwksReceiver(){}; @@ -53,8 +53,9 @@ class JwksFetcher { /* * Retrieve a JWKS resource from a remote HTTP host. * @param uri the uri to retrieve the jwks from. + * @param receiver the receiver of the fetched JWKS or error. */ - virtual void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver* receiver) PURE; + virtual void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver& receiver) PURE; /* * Factory method for creating a JwksFetcher. diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 12d0183623d77..041e7566d407b 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -115,10 +115,14 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback const auto unix_timestamp = std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch()) .count(); + // If the nbf claim does *not* appear in the JWT, then the nbf field is defaulted + // to 0. if (jwt_.nbf_ > unix_timestamp) { doneWithStatus(Status::JwtNotYetValid); return; } + // If the exp claim does *not* appear in the JWT then the exp field is defaulted + // to 0. if (jwt_.exp_ && jwt_.exp_ < unix_timestamp) { doneWithStatus(Status::JwtExpired); return; @@ -153,7 +157,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback // of using the same jwks comes. The request 2 will trigger another remote fetching for the // jwks. This can be optimized; the same remote jwks fetching can be shared by two requrests. if (jwks_data_->getJwtProvider().has_remote_jwks()) { - fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), this); + fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), *this); } else { // No valid keys for this issuer. This may happen as a result of incorrect local // JWKS configuration. diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index 84e7101f7a247..2f00a2dc1c0ff 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -62,8 +62,6 @@ class MockUpstream { .WillByDefault(testing::Invoke([request](Http::MessagePtr&, Http::AsyncClient::Callbacks&, const absl::optional&) -> Http::AsyncClient::Request* { - Http::MessagePtr response_message( - new Http::ResponseMessageImpl(Http::HeaderMapPtr{new Http::TestHeaderMapImpl{}})); return request; })); } @@ -96,7 +94,7 @@ TEST_F(JwksFetcherTest, TestGetSuccess) { EXPECT_CALL(receiver, onJwksError(testing::_)).Times(0); // Act - fetcher->fetch(uri_, &receiver); + fetcher->fetch(uri_, receiver); } TEST_F(JwksFetcherTest, TestGet400) { @@ -106,10 +104,10 @@ TEST_F(JwksFetcherTest, TestGet400) { 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)).Times(1); + EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::Network)).Times(1); // Act - fetcher->fetch(uri_, &receiver); + fetcher->fetch(uri_, receiver); } TEST_F(JwksFetcherTest, TestGetNoBody) { @@ -119,10 +117,10 @@ TEST_F(JwksFetcherTest, TestGetNoBody) { 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)).Times(1); + EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::Network)).Times(1); // Act - fetcher->fetch(uri_, &receiver); + fetcher->fetch(uri_, receiver); } TEST_F(JwksFetcherTest, TestGetInvalidJwks) { @@ -132,10 +130,10 @@ TEST_F(JwksFetcherTest, TestGetInvalidJwks) { 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::invalid_jwks)).Times(1); + EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks)).Times(1); // Act - fetcher->fetch(uri_, &receiver); + fetcher->fetch(uri_, receiver); } TEST_F(JwksFetcherTest, TestCancel) { @@ -147,10 +145,10 @@ TEST_F(JwksFetcherTest, TestCancel) { EXPECT_TRUE(fetcher != nullptr); EXPECT_CALL(request, cancel()).Times(1); EXPECT_CALL(receiver, onJwksSuccessImpl(testing::_)).Times(0); - EXPECT_CALL(receiver, onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks)).Times(0); + EXPECT_CALL(receiver, onJwksError(testing::_)).Times(0); // Act - fetcher->fetch(uri_, &receiver); + fetcher->fetch(uri_, receiver); // Proper cancel fetcher->cancel(); // Re-entrant cancel diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h index def27e58b4244..f04335a061793 100644 --- a/test/extensions/filters/http/common/mock.h +++ b/test/extensions/filters/http/common/mock.h @@ -10,7 +10,7 @@ namespace Common { class MockJwksFetcher : public JwksFetcher { public: MOCK_METHOD0(cancel, void()); - MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver* receiver)); + MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver& receiver)); }; } // namespace Common diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index 957171c8050d7..84d106d68d2da 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -68,8 +68,8 @@ class AuthenticatorTest : public ::testing::Test { TEST_F(AuthenticatorTest, TestOkJWTandCache) { EXPECT_CALL(*fetcher_, fetch(_, _)) .WillOnce(Invoke( - [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { - receiver->onJwksSuccess(std::move(jwks_)); + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver& receiver) { + receiver.onJwksSuccess(std::move(jwks_)); })); // Test OK pubkey and its cache @@ -96,8 +96,8 @@ TEST_F(AuthenticatorTest, TestForwardJwt) { CreateAuthenticator(); EXPECT_CALL(*fetcher_, fetch(_, _)) .WillOnce(Invoke( - [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { - receiver->onJwksSuccess(std::move(jwks_)); + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver& receiver) { + receiver.onJwksSuccess(std::move(jwks_)); })); // Test OK pubkey and its cache @@ -118,8 +118,8 @@ TEST_F(AuthenticatorTest, TestForwardJwt) { TEST_F(AuthenticatorTest, TestJwtWithNonExistKid) { EXPECT_CALL(*fetcher_, fetch(_, _)) .WillOnce(Invoke( - [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { - receiver->onJwksSuccess(std::move(jwks_)); + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver& receiver) { + receiver.onJwksSuccess(std::move(jwks_)); })); // Test OK pubkey and its cache auto headers = @@ -252,8 +252,8 @@ TEST_F(AuthenticatorTest, TestIssuerNotFound) { TEST_F(AuthenticatorTest, TestPubkeyFetchFail) { EXPECT_CALL(*fetcher_, fetch(_, _)) .WillOnce( - Invoke([](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { - receiver->onJwksError(JwksFetcher::JwksReceiver::Failure::invalid_jwks); + Invoke([](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver& receiver) { + receiver.onJwksError(JwksFetcher::JwksReceiver::Failure::InvalidJwks); })); EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { @@ -292,8 +292,8 @@ TEST_F(AuthenticatorTest, TestNoForwardPayloadHeader) { CreateAuthenticator(); EXPECT_CALL(*fetcher_, fetch(_, _)) .WillOnce(Invoke( - [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver* receiver) { - receiver->onJwksSuccess(std::move(jwks_)); + [this](const ::envoy::api::v2::core::HttpUri&, JwksFetcher::JwksReceiver& receiver) { + receiver.onJwksSuccess(std::move(jwks_)); })); auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(GoodToken)}}; From 6dbc828f259b77eddcb24127c89970785d428ba7 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Tue, 28 Aug 2018 21:25:50 +0100 Subject: [PATCH 05/22] check-format fixes Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/jwks_fetcher_test.cc | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index 2f00a2dc1c0ff..1de69e3e2574b 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -61,9 +61,7 @@ class MockUpstream { ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) .WillByDefault(testing::Invoke([request](Http::MessagePtr&, Http::AsyncClient::Callbacks&, const absl::optional&) - -> Http::AsyncClient::Request* { - return request; - })); + -> Http::AsyncClient::Request* { return request; })); } private: From 6ca047114d144585e74ddc0d63b1f24b52759411 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 30 Aug 2018 12:00:46 +0100 Subject: [PATCH 06/22] PR review changes The most major change is to swap JwksFetcher creation to be just-in-time using a lambda callback. Moved test_common.h from test/extensions/filters/http/common back to test/extensions/filters/http/jwt_authn. Most others changes are minor. Signed-off-by: Nick A. Smith --- .../filters/http/jwt_authn/authenticator.cc | 19 ++++++++---- .../filters/http/jwt_authn/authenticator.h | 10 +++++-- .../filters/http/jwt_authn/filter_factory.cc | 3 +- test/extensions/filters/http/common/BUILD | 6 ---- .../filters/http/common/jwks_fetcher_test.cc | 30 +++++++++++++++---- test/extensions/filters/http/jwt_authn/BUILD | 13 +++++--- .../http/jwt_authn/authenticator_test.cc | 19 +++--------- .../http/jwt_authn/filter_factory_test.cc | 6 ++-- .../http/jwt_authn/filter_integration_test.cc | 24 +++++++-------- .../filters/http/jwt_authn/jwks_cache_test.cc | 16 +++++----- .../http/{common => jwt_authn}/test_common.h | 4 +-- 11 files changed, 86 insertions(+), 64 deletions(-) rename test/extensions/filters/http/{common => jwt_authn}/test_common.h (99%) diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 041e7566d407b..1de6b0fce79fe 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -26,8 +26,8 @@ class AuthenticatorImpl : public Logger::Loggable, public Authenticator, public Common::JwksFetcher::JwksReceiver { public: - AuthenticatorImpl(FilterConfigSharedPtr config, Common::JwksFetcherPtr&& fetcher) - : config_(config), fetcher_(std::move(fetcher)) {} + AuthenticatorImpl(FilterConfigSharedPtr config, CreateJwksFetcherCb createJwksFetcherCb) + : config_(config), createJwksFetcherCb_(createJwksFetcherCb) {} // Following functions are for JwksFetcher::JwksReceiver interface void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) override; @@ -50,6 +50,9 @@ class AuthenticatorImpl : public Logger::Loggable, // The config object. FilterConfigSharedPtr config_; + // The callback used to create a JwksFetcher instance. + CreateJwksFetcherCb createJwksFetcherCb_; + // The Jwks fetcher object Common::JwksFetcherPtr fetcher_; @@ -157,6 +160,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback // of using the same jwks comes. The request 2 will trigger another remote fetching for the // jwks. This can be optimized; the same remote jwks fetching can be shared by two requrests. if (jwks_data_->getJwtProvider().has_remote_jwks()) { + fetcher_ = createJwksFetcherCb_(config_->cm()); fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), *this); } else { // No valid keys for this issuer. This may happen as a result of incorrect local @@ -176,7 +180,12 @@ void AuthenticatorImpl::onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { void AuthenticatorImpl::onJwksError(Failure) { doneWithStatus(Status::JwksFetchFail); } -void AuthenticatorImpl::onDestroy() { fetcher_->cancel(); } +void AuthenticatorImpl::onDestroy() { + if (fetcher_) { + fetcher_->cancel(); + fetcher_.reset(nullptr); + } +} // Verify with a specific public key. void AuthenticatorImpl::verifyKey() { @@ -216,8 +225,8 @@ void AuthenticatorImpl::doneWithStatus(const Status& status) { } // namespace AuthenticatorPtr Authenticator::create(FilterConfigSharedPtr config, - Common::JwksFetcherPtr&& fetcher) { - return std::make_unique(config, std::move(fetcher)); + CreateJwksFetcherCb createJwksFetcherCb) { + return std::make_unique(config, createJwksFetcherCb); } } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/authenticator.h b/source/extensions/filters/http/jwt_authn/authenticator.h index 6160bbb2452ea..937f6c3f5aab1 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.h +++ b/source/extensions/filters/http/jwt_authn/authenticator.h @@ -1,4 +1,5 @@ #pragma once +#include #include "extensions/filters/http/common/jwks_fetcher.h" #include "extensions/filters/http/jwt_authn/filter_config.h" @@ -14,9 +15,13 @@ class Authenticator; typedef std::unique_ptr AuthenticatorPtr; /** - * Authenticator object to handle all JWT authentication flow. + * CreateJwksFetcherCb is a callback interface for creating a JwksFetcher instance. */ +typedef std::function CreateJwksFetcherCb; +/** + * Authenticator object to handle all JWT authentication flow. + */ class Authenticator { public: virtual ~Authenticator() {} @@ -36,7 +41,8 @@ class Authenticator { virtual void sanitizePayloadHeaders(Http::HeaderMap& headers) const PURE; // Authenticator factory function. - static AuthenticatorPtr create(FilterConfigSharedPtr config, Common::JwksFetcherPtr&& fetcher); + static AuthenticatorPtr create(FilterConfigSharedPtr config, + CreateJwksFetcherCb createJwksFetcherCb); }; } // namespace JwtAuthn diff --git a/source/extensions/filters/http/jwt_authn/filter_factory.cc b/source/extensions/filters/http/jwt_authn/filter_factory.cc index 6dc0ce454c8e4..ecec7bf035059 100644 --- a/source/extensions/filters/http/jwt_authn/filter_factory.cc +++ b/source/extensions/filters/http/jwt_authn/filter_factory.cc @@ -46,9 +46,8 @@ FilterFactory::createFilterFactoryFromProtoTyped(const JwtAuthentication& proto_ validateJwtConfig(proto_config); auto filter_config = std::make_shared(proto_config, prefix, context); return [filter_config](Http::FilterChainFactoryCallbacks& callbacks) -> void { - auto jwks_fetcher = Common::JwksFetcher::create(filter_config->cm()); callbacks.addStreamDecoderFilter(std::make_shared( - filter_config->stats(), Authenticator::create(filter_config, std::move(jwks_fetcher)))); + filter_config->stats(), Authenticator::create(filter_config, Common::JwksFetcher::create))); }; } diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD index 16a7eb0e64809..103f14765dcd9 100644 --- a/test/extensions/filters/http/common/BUILD +++ b/test/extensions/filters/http/common/BUILD @@ -12,11 +12,6 @@ load( envoy_package() -envoy_cc_library( - name = "test_common_lib", - hdrs = ["test_common.h"], -) - envoy_cc_library( name = "mock_lib", hdrs = ["mock.h"], @@ -29,7 +24,6 @@ envoy_extension_cc_test( ], extension_name = "envoy.filters.http.jwt_authn", deps = [ - ":test_common_lib", "//source/extensions/filters/http/common:jwks_fetcher_lib", "//test/mocks/http:http_mocks", "//test/mocks/server:server_mocks", diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index 1de69e3e2574b..a2ddadd0bcf0a 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -6,7 +6,6 @@ #include "extensions/filters/http/common/jwks_fetcher.h" -#include "test/extensions/filters/http/common/test_common.h" #include "test/mocks/http/mocks.h" #include "test/mocks/server/mocks.h" #include "test/test_common/utility.h" @@ -20,6 +19,29 @@ namespace HttpFilters { namespace Common { namespace { +const char publicKey[] = R"( +{ + "keys": [ + { + "kty": "RSA", + "alg": "RS256", + "use": "sig", + "kid": "62a93512c9ee4c7f8067b5a216dade2763d32a47", + "n": "up97uqrF9MWOPaPkwSaBeuAPLOr9FKcaWGdVEGzQ4f3Zq5WKVZowx9TCBxmImNJ1qmUi13pB8otwM_l5lfY1AFBMxVbQCUXntLovhDaiSvYp4wGDjFzQiYA-pUq8h6MUZBnhleYrkU7XlCBwNVyN8qNMkpLA7KFZYz-486GnV2NIJJx_4BGa3HdKwQGxi2tjuQsQvao5W4xmSVaaEWopBwMy2QmlhSFQuPUpTaywTqUcUq_6SfAHhZ4IDa_FxEd2c2z8gFGtfst9cY3lRYf-c_ZdboY3mqN9Su3-j3z5r2SHWlhB_LNAjyWlBGsvbGPlTqDziYQwZN4aGsqVKQb9Vw", + "e": "AQAB" + }, + { + "kty": "RSA", + "alg": "RS256", + "use": "sig", + "kid": "b3319a147514df7ee5e4bcdee51350cc890cc89e", + "n": "up97uqrF9MWOPaPkwSaBeuAPLOr9FKcaWGdVEGzQ4f3Zq5WKVZowx9TCBxmImNJ1qmUi13pB8otwM_l5lfY1AFBMxVbQCUXntLovhDaiSvYp4wGDjFzQiYA-pUq8h6MUZBnhleYrkU7XlCBwNVyN8qNMkpLA7KFZYz-486GnV2NIJJx_4BGa3HdKwQGxi2tjuQsQvao5W4xmSVaaEWopBwMy2QmlhSFQuPUpTaywTqUcUq_6SfAHhZ4IDa_FxEd2c2z8gFGtfst9cY3lRYf-c_ZdboY3mqN9Su3-j3z5r2SHWlhB_LNAjyWlBGsvbGPlTqDziYQwZN4aGsqVKQb9Vw", + "e": "AQAB" + } + ] +} +)"; + const std::string JwksUri = R"( uri: https://pubkey_server/pubkey_path cluster: pubkey_cluster @@ -59,9 +81,7 @@ class MockUpstream { MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) : request_(&mock_cm.async_client_) { ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Invoke([request](Http::MessagePtr&, Http::AsyncClient::Callbacks&, - const absl::optional&) - -> Http::AsyncClient::Request* { return request; })); + .WillByDefault(testing::Return(request)); } private: @@ -84,7 +104,7 @@ class MockJwksReceiver : public JwksFetcher::JwksReceiver { // Test findByIssuer TEST_F(JwksFetcherTest, TestGetSuccess) { // Setup - MockUpstream mock_pubkey(mock_factory_ctx_.cluster_manager_, "200", PublicKey); + 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); diff --git a/test/extensions/filters/http/jwt_authn/BUILD b/test/extensions/filters/http/jwt_authn/BUILD index 84acd9a9ef832..0e90d2c0ecc2d 100644 --- a/test/extensions/filters/http/jwt_authn/BUILD +++ b/test/extensions/filters/http/jwt_authn/BUILD @@ -12,6 +12,11 @@ load( envoy_package() +envoy_cc_library( + name = "test_common_lib", + hdrs = ["test_common.h"], +) + envoy_cc_library( name = "mock_lib", hdrs = ["mock.h"], @@ -51,7 +56,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.http.jwt_authn", deps = [ "//source/extensions/filters/http/jwt_authn:config", - "//test/extensions/filters/http/common:test_common_lib", + "//test/extensions/filters/http/jwt_authn:test_common_lib", "//test/mocks/server:server_mocks", ], ) @@ -65,7 +70,7 @@ envoy_extension_cc_test( deps = [ "//source/extensions/filters/http/common:jwks_fetcher_lib", "//source/extensions/filters/http/jwt_authn:jwks_cache_lib", - "//test/extensions/filters/http/common:test_common_lib", + "//test/extensions/filters/http/jwt_authn:test_common_lib", "//test/test_common:utility_lib", ], ) @@ -81,7 +86,7 @@ envoy_extension_cc_test( "//source/extensions/filters/http/common:jwks_fetcher_lib", "//source/extensions/filters/http/jwt_authn:authenticator_lib", "//test/extensions/filters/http/common:mock_lib", - "//test/extensions/filters/http/common:test_common_lib", + "//test/extensions/filters/http/jwt_authn:test_common_lib", "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", ], @@ -94,7 +99,7 @@ envoy_extension_cc_test( deps = [ "//source/extensions/filters/http/jwt_authn:config", "//test/config:utility_lib", - "//test/extensions/filters/http/common:test_common_lib", + "//test/extensions/filters/http/jwt_authn:test_common_lib", "//test/integration:http_protocol_integration_lib", ], ) diff --git a/test/extensions/filters/http/jwt_authn/authenticator_test.cc b/test/extensions/filters/http/jwt_authn/authenticator_test.cc index 84d106d68d2da..f6c2d90bc7666 100644 --- a/test/extensions/filters/http/jwt_authn/authenticator_test.cc +++ b/test/extensions/filters/http/jwt_authn/authenticator_test.cc @@ -5,8 +5,8 @@ #include "extensions/filters/http/jwt_authn/authenticator.h" #include "test/extensions/filters/http/common/mock.h" -#include "test/extensions/filters/http/common/test_common.h" #include "test/extensions/filters/http/jwt_authn/mock.h" +#include "test/extensions/filters/http/jwt_authn/test_common.h" #include "test/mocks/server/mocks.h" #include "test/mocks/upstream/mocks.h" #include "test/test_common/utility.h" @@ -14,19 +14,9 @@ #include "gtest/gtest.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; -using Envoy::Extensions::HttpFilters::Common::ExampleConfig; -using Envoy::Extensions::HttpFilters::Common::ExpectedPayloadValue; -using Envoy::Extensions::HttpFilters::Common::ExpiredToken; -using Envoy::Extensions::HttpFilters::Common::GoodToken; -using Envoy::Extensions::HttpFilters::Common::InvalidAudToken; using Envoy::Extensions::HttpFilters::Common::JwksFetcher; using Envoy::Extensions::HttpFilters::Common::JwksFetcherPtr; using Envoy::Extensions::HttpFilters::Common::MockJwksFetcher; -using Envoy::Extensions::HttpFilters::Common::NonExistKidToken; -using Envoy::Extensions::HttpFilters::Common::NonExpiringToken; -using Envoy::Extensions::HttpFilters::Common::NotYetValidToken; -using Envoy::Extensions::HttpFilters::Common::ProviderName; -using Envoy::Extensions::HttpFilters::Common::PublicKey; using ::google::jwt_verify::Status; using ::testing::_; using ::testing::Invoke; @@ -48,7 +38,8 @@ class AuthenticatorTest : public ::testing::Test { filter_config_ = ::std::make_shared(proto_config_, "", mock_factory_ctx_); fetcher_ = new MockJwksFetcher; fetcherPtr_.reset(fetcher_); - auth_ = Authenticator::create(filter_config_, std::move(fetcherPtr_)); + auth_ = Authenticator::create( + filter_config_, [this](Upstream::ClusterManager&) { return std::move(fetcherPtr_); }); jwks_ = ::google::jwt_verify::Jwks::createFrom(PublicKey, ::google::jwt_verify::Jwks::JWKS); EXPECT_TRUE(jwks_->getStatus() == Status::Ok); } @@ -196,9 +187,7 @@ TEST_F(AuthenticatorTest, TestExpiredJWT) { // This test verifies when a JWT is not yet valid, JwtNotYetValid status is returned. TEST_F(AuthenticatorTest, TestNotYetValidJWT) { EXPECT_CALL(*fetcher_, fetch(_, _)).Times(0); - EXPECT_CALL(mock_cb_, onComplete(_)).WillOnce(Invoke([](const Status& status) { - ASSERT_EQ(status, Status::JwtNotYetValid); - })); + EXPECT_CALL(mock_cb_, onComplete(Status::JwtNotYetValid)).Times(1); auto headers = Http::TestHeaderMapImpl{{"Authorization", "Bearer " + std::string(NotYetValidToken)}}; diff --git a/test/extensions/filters/http/jwt_authn/filter_factory_test.cc b/test/extensions/filters/http/jwt_authn/filter_factory_test.cc index 565f12cd3fe4b..dfb2c94ff5842 100644 --- a/test/extensions/filters/http/jwt_authn/filter_factory_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_factory_test.cc @@ -2,7 +2,7 @@ #include "extensions/filters/http/jwt_authn/filter_factory.h" -#include "test/extensions/filters/http/common/test_common.h" +#include "test/extensions/filters/http/jwt_authn/test_common.h" #include "test/mocks/server/mocks.h" #include "gmock/gmock.h" @@ -21,7 +21,7 @@ namespace JwtAuthn { TEST(HttpJwtAuthnFilterFactoryTest, GoodRemoteJwks) { FilterFactory factory; ProtobufTypes::MessagePtr proto_config = factory.createEmptyConfigProto(); - MessageUtil::loadFromYaml(Common::ExampleConfig, *proto_config); + MessageUtil::loadFromYaml(ExampleConfig, *proto_config); NiceMock context; @@ -35,7 +35,7 @@ TEST(HttpJwtAuthnFilterFactoryTest, GoodLocalJwks) { JwtAuthentication proto_config; auto& provider = (*proto_config.mutable_providers())["provider"]; provider.set_issuer("issuer"); - provider.mutable_local_jwks()->set_inline_string(Common::PublicKey); + provider.mutable_local_jwks()->set_inline_string(PublicKey); NiceMock context; FilterFactory factory; diff --git a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc index 2b51a7d14af4c..3440c215d207f 100644 --- a/test/extensions/filters/http/jwt_authn/filter_integration_test.cc +++ b/test/extensions/filters/http/jwt_authn/filter_integration_test.cc @@ -2,7 +2,7 @@ #include "extensions/filters/http/well_known_names.h" -#include "test/extensions/filters/http/common/test_common.h" +#include "test/extensions/filters/http/jwt_authn/test_common.h" #include "test/integration/http_protocol_integration.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; @@ -16,13 +16,13 @@ namespace { std::string getFilterConfig(bool use_local_jwks) { JwtAuthentication proto_config; - MessageUtil::loadFromYaml(Common::ExampleConfig, proto_config); + MessageUtil::loadFromYaml(ExampleConfig, proto_config); if (use_local_jwks) { - auto& provider0 = (*proto_config.mutable_providers())[std::string(Common::ProviderName)]; + auto& provider0 = (*proto_config.mutable_providers())[std::string(ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); - local_jwks->set_inline_string(Common::PublicKey); + local_jwks->set_inline_string(PublicKey); } HttpFilter filter; @@ -49,14 +49,14 @@ TEST_P(LocalJwksIntegrationTest, WithGoodToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(Common::GoodToken)}, + {"Authorization", "Bearer " + std::string(GoodToken)}, }); waitForNextUpstreamRequest(); const auto* payload_entry = upstream_request_->headers().get(Http::LowerCaseString("sec-istio-auth-userinfo")); EXPECT_TRUE(payload_entry != nullptr); - EXPECT_EQ(payload_entry->value().getStringView(), Common::ExpectedPayloadValue); + EXPECT_EQ(payload_entry->value().getStringView(), ExpectedPayloadValue); // Verify the token is removed. EXPECT_FALSE(upstream_request_->headers().Authorization()); upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); @@ -77,7 +77,7 @@ TEST_P(LocalJwksIntegrationTest, ExpiredToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(Common::ExpiredToken)}, + {"Authorization", "Bearer " + std::string(ExpiredToken)}, }); response->waitForEndStream(); @@ -96,7 +96,7 @@ TEST_P(LocalJwksIntegrationTest, ExpiredTokenHeadReply) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(Common::ExpiredToken)}, + {"Authorization", "Bearer " + std::string(ExpiredToken)}, }); response->waitForEndStream(); @@ -178,17 +178,17 @@ TEST_P(RemoteJwksIntegrationTest, WithGoodToken) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(Common::GoodToken)}, + {"Authorization", "Bearer " + std::string(GoodToken)}, }); - waitForJwksResponse("200", Common::PublicKey); + waitForJwksResponse("200", PublicKey); waitForNextUpstreamRequest(); const auto* payload_entry = upstream_request_->headers().get(Http::LowerCaseString("sec-istio-auth-userinfo")); EXPECT_TRUE(payload_entry != nullptr); - EXPECT_EQ(payload_entry->value().getStringView(), Common::ExpectedPayloadValue); + EXPECT_EQ(payload_entry->value().getStringView(), ExpectedPayloadValue); // Verify the token is removed. EXPECT_FALSE(upstream_request_->headers().Authorization()); @@ -213,7 +213,7 @@ TEST_P(RemoteJwksIntegrationTest, FetchFailedJwks) { {":path", "/"}, {":scheme", "http"}, {":authority", "host"}, - {"Authorization", "Bearer " + std::string(Common::GoodToken)}, + {"Authorization", "Bearer " + std::string(GoodToken)}, }); // Fails the jwks fetching. 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 5bdbb644bbf6b..4610e5d86e8b4 100644 --- a/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc +++ b/test/extensions/filters/http/jwt_authn/jwks_cache_test.cc @@ -5,7 +5,7 @@ #include "extensions/filters/http/jwt_authn/jwks_cache.h" -#include "test/extensions/filters/http/common/test_common.h" +#include "test/extensions/filters/http/jwt_authn/test_common.h" #include "test/test_common/utility.h" using ::envoy::config::filter::http::jwt_authn::v2alpha::JwtAuthentication; @@ -20,9 +20,9 @@ namespace { class JwksCacheTest : public ::testing::Test { public: void SetUp() { - MessageUtil::loadFromYaml(Common::ExampleConfig, config_); + MessageUtil::loadFromYaml(ExampleConfig, config_); cache_ = JwksCache::create(config_); - jwks_ = google::jwt_verify::Jwks::createFrom(Common::PublicKey, google::jwt_verify::Jwks::JWKS); + jwks_ = google::jwt_verify::Jwks::createFrom(PublicKey, google::jwt_verify::Jwks::JWKS); } JwtAuthentication config_; @@ -38,7 +38,7 @@ TEST_F(JwksCacheTest, TestFindByIssuer) { // Test setRemoteJwks and its expiration TEST_F(JwksCacheTest, TestSetRemoteJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; // Set cache_duration to 1 second to test expiration provider0.mutable_remote_jwks()->mutable_cache_duration()->set_seconds(1); cache_ = JwksCache::create(config_); @@ -57,7 +57,7 @@ TEST_F(JwksCacheTest, TestSetRemoteJwks) { // Test setRemoteJwks and use default cache duration. TEST_F(JwksCacheTest, TestSetRemoteJwksWithDefaultCacheDuration) { - auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; // Clear cache_duration to use default one. provider0.mutable_remote_jwks()->clear_cache_duration(); cache_ = JwksCache::create(config_); @@ -72,10 +72,10 @@ TEST_F(JwksCacheTest, TestSetRemoteJwksWithDefaultCacheDuration) { // Test a good local jwks TEST_F(JwksCacheTest, TestGoodInlineJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); - local_jwks->set_inline_string(Common::PublicKey); + local_jwks->set_inline_string(PublicKey); cache_ = JwksCache::create(config_); @@ -86,7 +86,7 @@ TEST_F(JwksCacheTest, TestGoodInlineJwks) { // Test a bad local jwks TEST_F(JwksCacheTest, TestBadInlineJwks) { - auto& provider0 = (*config_.mutable_providers())[std::string(Common::ProviderName)]; + auto& provider0 = (*config_.mutable_providers())[std::string(ProviderName)]; provider0.clear_remote_jwks(); auto local_jwks = provider0.mutable_local_jwks(); local_jwks->set_inline_string("BAD-JWKS"); diff --git a/test/extensions/filters/http/common/test_common.h b/test/extensions/filters/http/jwt_authn/test_common.h similarity index 99% rename from test/extensions/filters/http/common/test_common.h rename to test/extensions/filters/http/jwt_authn/test_common.h index 4ce0a3fcc1db1..8cad51b755085 100644 --- a/test/extensions/filters/http/common/test_common.h +++ b/test/extensions/filters/http/jwt_authn/test_common.h @@ -3,7 +3,7 @@ namespace Envoy { namespace Extensions { namespace HttpFilters { -namespace Common { +namespace JwtAuthn { // A good public key const char PublicKey[] = R"( @@ -131,7 +131,7 @@ const char ExpectedPayloadValue[] = "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3V "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" "aWNlIn0"; -} // namespace Common +} // namespace JwtAuthn } // namespace HttpFilters } // namespace Extensions } // namespace Envoy From 498c858b5db712b3d83955956aa010f870c7617d Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 30 Aug 2018 21:40:56 +0100 Subject: [PATCH 07/22] Added comment describing the dependency on abseil time Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 2 ++ 1 file changed, 2 insertions(+) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 94dd34233142a..ff0d966317ff2 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -464,6 +464,8 @@ def _com_google_absl(): name = "abseil_symbolize", actual = "@com_google_absl//absl/debugging:symbolize", ) + # abseil_time is an indirect dependency required by the direct dependency + # jwt_verify_lib. native.bind( name = "abseil_time", actual = "@com_google_absl//absl/time:time", From af9175eb12ec72d7df8eaefd7f2e4e601d1e75ff Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 30 Aug 2018 21:57:17 +0100 Subject: [PATCH 08/22] More style fixing :( Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 1 + 1 file changed, 1 insertion(+) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index ff0d966317ff2..6ddfc2dc095bc 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -464,6 +464,7 @@ def _com_google_absl(): name = "abseil_symbolize", actual = "@com_google_absl//absl/debugging:symbolize", ) + # abseil_time is an indirect dependency required by the direct dependency # jwt_verify_lib. native.bind( From 51c4d1bc1b79d5f0d55b0cf1b27bc05fce5c2044 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 30 Aug 2018 22:38:56 +0100 Subject: [PATCH 09/22] Another go at fixing format of bazel/repositories.bzl Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index 6ddfc2dc095bc..f27165da23acd 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -465,8 +465,8 @@ def _com_google_absl(): actual = "@com_google_absl//absl/debugging:symbolize", ) - # abseil_time is an indirect dependency required by the direct dependency - # jwt_verify_lib. + # Require abseil_time as an indirect dependency as it is needed by the + # direct dependency jwt_verify_lib. native.bind( name = "abseil_time", actual = "@com_google_absl//absl/time:time", From f477905d189f9f46d721464e98760c23b8ce4ca2 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 30 Aug 2018 23:44:57 +0100 Subject: [PATCH 10/22] Run tools/check_format.py fix Check_format.py made some unwanted changes to certain lines in baxel/repositories.bzl that had not been modified. I've reverted those lines in the hope they will take. Signed-off-by: Nick A. Smith --- bazel/repositories.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repositories.bzl b/bazel/repositories.bzl index f27165da23acd..4610d2335b25e 100644 --- a/bazel/repositories.bzl +++ b/bazel/repositories.bzl @@ -464,7 +464,7 @@ def _com_google_absl(): name = "abseil_symbolize", actual = "@com_google_absl//absl/debugging:symbolize", ) - + # Require abseil_time as an indirect dependency as it is needed by the # direct dependency jwt_verify_lib. native.bind( From 8fa1dc8adac5ead867656685a9255f1ca0dd5eca Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Tue, 4 Sep 2018 21:24:09 +0100 Subject: [PATCH 11/22] Fixed from PR review Mainly small fixes and better documentation. Signed-off-by: Nick A. Smith --- .../filters/http/common/jwks_fetcher.cc | 24 ++++++++++++------- .../filters/http/common/jwks_fetcher.h | 2 +- 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index b99f15b231c7d..507dca2572c12 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -11,24 +11,19 @@ namespace Extensions { namespace HttpFilters { namespace Common { namespace { + class JwksFetcherImpl : public JwksFetcher, public Logger::Loggable, public Http::AsyncClient::Callbacks { -private: - Upstream::ClusterManager& cm_; - JwksFetcher::JwksReceiver* receiver_ = nullptr; - const ::envoy::api::v2::core::HttpUri* uri_ = nullptr; - Http::AsyncClient::Request* request_ = nullptr; - public: JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } void cancel() { if (request_) { request_->cancel(); - request_ = nullptr; ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", uri_->uri()); } + reset(); } void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) { @@ -47,7 +42,6 @@ class JwksFetcherImpl : public JwksFetcher, // HTTP async receive methods void onSuccess(Http::MessagePtr&& response) { ENVOY_LOG(trace, "{}", __func__); - request_ = nullptr; const uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); if (status_code == enumToInt(Http::Code::OK)) { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: success", __func__, uri_->uri()); @@ -72,12 +66,26 @@ class JwksFetcherImpl : public JwksFetcher, uri_->uri(), status_code); receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); } + reset(); } void onFailure(Http::AsyncClient::FailureReason reason) { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, uri_->uri(), enumToInt(reason)); receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); + reset(); + } + +private: + Upstream::ClusterManager& cm_; + JwksFetcher::JwksReceiver* receiver_{}; + const envoy::api::v2::core::HttpUri* uri_{}; + Http::AsyncClient::Request* request_{}; + + void reset() { + request_ = nullptr; + receiver_ = nullptr; + uri_ = nullptr; } }; } // namespace diff --git a/source/extensions/filters/http/common/jwks_fetcher.h b/source/extensions/filters/http/common/jwks_fetcher.h index 748ac0943d054..16189281af821 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.h +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -17,7 +17,7 @@ typedef std::unique_ptr JwksFetcherPtr; * JwksFetcher interface can be used to retrieve remote JWKS * (https://tools.ietf.org/html/rfc7517) data structures returning a concrete, * type-safe representation. An instance of this interface is designed to - * retrieve a single JWKS and should not be re-used to fetch further instances. + * retrieve one JWKS at a time. */ class JwksFetcher { public: From 2d18a010866ad1c73ee3f192241a7521176ccf48 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Wed, 5 Sep 2018 06:28:06 +0100 Subject: [PATCH 12/22] Added missing unit tests, tidying Changes from final PR review to include missing unit testand fix commenting Signed-off-by: Nick A. Smith --- .../filters/http/common/jwks_fetcher.cc | 1 + .../filters/http/common/jwks_fetcher.h | 9 +++++-- .../filters/http/jwt_authn/authenticator.cc | 10 +++---- .../filters/http/common/jwks_fetcher_test.cc | 26 +++++++++++++++++++ 4 files changed, 39 insertions(+), 7 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index 507dca2572c12..de7b9ad1034dd 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -28,6 +28,7 @@ class JwksFetcherImpl : public JwksFetcher, void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) { ENVOY_LOG(trace, "{}", __func__); + ASSERT(!receiver_); receiver_ = &receiver; uri_ = &uri; Http::MessagePtr message = Http::Utility::prepareHeaders(uri); diff --git a/source/extensions/filters/http/common/jwks_fetcher.h b/source/extensions/filters/http/common/jwks_fetcher.h index 16189281af821..c76a76c8948b6 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.h +++ b/source/extensions/filters/http/common/jwks_fetcher.h @@ -24,8 +24,9 @@ class JwksFetcher { class JwksReceiver { public: enum class Failure { - Unknown, + /* A network error occured causing JWKS retrieval failure. */ Network, + /* A failure occured when trying to parse the retrieved JWKS data. */ InvalidJwks, }; @@ -46,12 +47,16 @@ class JwksFetcher { virtual ~JwksFetcher(){}; /* - * Cancel any inflight request. + * Cancel any in-flight request. */ virtual void cancel() PURE; /* * Retrieve a JWKS resource from a remote HTTP host. + * At most one outstanding request may be in-flight, + * 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. * @param receiver the receiver of the fetched JWKS or error. */ diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 1de6b0fce79fe..2a08b8cba7dce 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -110,10 +110,10 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback return; } - // TODO: Cross-platform-wise the below unix_timestamp code is wrong as the + // TODO(qiwzhang): Cross-platform-wise the below unix_timestamp code is wrong as the // epoch is not guaranteed to be defined as the unix epoch. We should use - // the abseil time functionality instead. - // TODO: We should use the jwt_verify_lib to check the validity of a JWT. + // the abseil time functionality instead or use the jwt_verify_lib to check + // the validity of a JWT. // Check "exp" claim. const auto unix_timestamp = std::chrono::duration_cast( std::chrono::system_clock::now().time_since_epoch()) @@ -126,7 +126,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback } // If the exp claim does *not* appear in the JWT then the exp field is defaulted // to 0. - if (jwt_.exp_ && jwt_.exp_ < unix_timestamp) { + if (jwt_.exp_ > 0 && jwt_.exp_ < unix_timestamp) { doneWithStatus(Status::JwtExpired); return; } @@ -144,7 +144,7 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback auto jwks_obj = jwks_data_->getJwksObj(); if (jwks_obj != nullptr && !jwks_data_->isExpired()) { - // TODO: It would seem there's a window of error whereby if the JWT issuer + // TODO(qiwzhang): It would seem there's a window of error whereby if the JWT issuer // has started signing with a new key that's not in our cache, then the // verification will fail even though the JWT is valid. A simple fix // would be to check the JWS kid header field; if present check we have diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index a2ddadd0bcf0a..d673d74b6399a 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -78,6 +78,18 @@ class MockUpstream { })); } + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::AsyncClient::FailureReason reason) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke( + [this, reason]( + Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) -> Http::AsyncClient::Request* { + cb.onFailure(reason); + return &request_; + })); + } + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) : request_(&mock_cm.async_client_) { ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) @@ -154,6 +166,20 @@ TEST_F(JwksFetcherTest, TestGetInvalidJwks) { fetcher->fetch(uri_, receiver); } +TEST_F(JwksFetcherTest, TestHttpFailure) { + // Setup + 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)).Times(1); + + // Act + fetcher->fetch(uri_, receiver); +} + TEST_F(JwksFetcherTest, TestCancel) { // Setup Http::MockAsyncClientRequest request(&(mock_factory_ctx_.cluster_manager_.async_client_)); From 1d9281e19350cf22933e0f969cbc77746bb44a7a Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 6 Sep 2018 14:20:21 +0100 Subject: [PATCH 13/22] Added state machine for handling immediate JwksFetch returns. Signed-off-by: Nick A. Smith --- source/extensions/filters/http/common/jwks_fetcher.cc | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index de7b9ad1034dd..666efcb5869d5 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -18,8 +18,12 @@ class JwksFetcherImpl : public JwksFetcher, public: JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } + ~JwksFetcherImpl() { + cancel(); + } + void cancel() { - if (request_) { + if (request_ && !complete_) { request_->cancel(); ENVOY_LOG(debug, "fetch pubkey [uri = {}]: canceled", uri_->uri()); } @@ -29,6 +33,7 @@ class JwksFetcherImpl : public JwksFetcher, void fetch(const ::envoy::api::v2::core::HttpUri& uri, JwksFetcher::JwksReceiver& receiver) { ENVOY_LOG(trace, "{}", __func__); ASSERT(!receiver_); + complete_ = false; receiver_ = &receiver; uri_ = &uri; Http::MessagePtr message = Http::Utility::prepareHeaders(uri); @@ -43,6 +48,7 @@ class JwksFetcherImpl : public JwksFetcher, // HTTP async receive methods void onSuccess(Http::MessagePtr&& response) { ENVOY_LOG(trace, "{}", __func__); + complete_ = true; const uint64_t status_code = Http::Utility::getResponseStatus(response->headers()); if (status_code == enumToInt(Http::Code::OK)) { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: success", __func__, uri_->uri()); @@ -73,12 +79,14 @@ class JwksFetcherImpl : public JwksFetcher, void onFailure(Http::AsyncClient::FailureReason reason) { ENVOY_LOG(debug, "{}: fetch pubkey [uri = {}]: network error {}", __func__, uri_->uri(), enumToInt(reason)); + complete_ = true; receiver_->onJwksError(JwksFetcher::JwksReceiver::Failure::Network); reset(); } private: Upstream::ClusterManager& cm_; + bool complete_ {}; JwksFetcher::JwksReceiver* receiver_{}; const envoy::api::v2::core::HttpUri* uri_{}; Http::AsyncClient::Request* request_{}; From 9d277be68c8a54408afe11174a2673c0a9de51f8 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 6 Sep 2018 14:42:20 +0100 Subject: [PATCH 14/22] Style fixes Signed-off-by: Nick A. Smith --- source/extensions/filters/http/common/jwks_fetcher.cc | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index 666efcb5869d5..7c76b085c178a 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -1,9 +1,9 @@ -#include "extensions/filters/http/common/jwks_fetcher.h" - #include "common/common/enum_to_int.h" #include "common/http/headers.h" #include "common/http/utility.h" +#include "extensions/filters/http/common/jwks_fetcher.h" + #include "jwt_verify_lib/status.h" namespace Envoy { @@ -18,9 +18,7 @@ class JwksFetcherImpl : public JwksFetcher, public: JwksFetcherImpl(Upstream::ClusterManager& cm) : cm_(cm) { ENVOY_LOG(trace, "{}", __func__); } - ~JwksFetcherImpl() { - cancel(); - } + ~JwksFetcherImpl() { cancel(); } void cancel() { if (request_ && !complete_) { @@ -86,7 +84,7 @@ class JwksFetcherImpl : public JwksFetcher, private: Upstream::ClusterManager& cm_; - bool complete_ {}; + bool complete_{}; JwksFetcher::JwksReceiver* receiver_{}; const envoy::api::v2::core::HttpUri* uri_{}; Http::AsyncClient::Request* request_{}; From 4c772000613e30b2295c3da7cc63070d6a9fcd9b Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 6 Sep 2018 22:32:15 +0100 Subject: [PATCH 15/22] Changed JwtAuthN::Authenticator to re-use a JwksFetcher. This should fix any use-after-free errors Signed-off-by: Nick A. Smith --- source/extensions/filters/http/common/jwks_fetcher.cc | 4 ++-- source/extensions/filters/http/jwt_authn/authenticator.cc | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/source/extensions/filters/http/common/jwks_fetcher.cc b/source/extensions/filters/http/common/jwks_fetcher.cc index 7c76b085c178a..b46f72cacb939 100644 --- a/source/extensions/filters/http/common/jwks_fetcher.cc +++ b/source/extensions/filters/http/common/jwks_fetcher.cc @@ -1,9 +1,9 @@ +#include "extensions/filters/http/common/jwks_fetcher.h" + #include "common/common/enum_to_int.h" #include "common/http/headers.h" #include "common/http/utility.h" -#include "extensions/filters/http/common/jwks_fetcher.h" - #include "jwt_verify_lib/status.h" namespace Envoy { diff --git a/source/extensions/filters/http/jwt_authn/authenticator.cc b/source/extensions/filters/http/jwt_authn/authenticator.cc index 2a08b8cba7dce..9b558de3b422c 100644 --- a/source/extensions/filters/http/jwt_authn/authenticator.cc +++ b/source/extensions/filters/http/jwt_authn/authenticator.cc @@ -79,6 +79,7 @@ void AuthenticatorImpl::sanitizePayloadHeaders(Http::HeaderMap& headers) const { } void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callbacks* callback) { + ASSERT(!callback_); headers_ = &headers; callback_ = callback; @@ -160,7 +161,9 @@ void AuthenticatorImpl::verify(Http::HeaderMap& headers, Authenticator::Callback // of using the same jwks comes. The request 2 will trigger another remote fetching for the // jwks. This can be optimized; the same remote jwks fetching can be shared by two requrests. if (jwks_data_->getJwtProvider().has_remote_jwks()) { - fetcher_ = createJwksFetcherCb_(config_->cm()); + if (!fetcher_) { + fetcher_ = createJwksFetcherCb_(config_->cm()); + } fetcher_->fetch(jwks_data_->getJwtProvider().remote_jwks().http_uri(), *this); } else { // No valid keys for this issuer. This may happen as a result of incorrect local @@ -183,7 +186,6 @@ void AuthenticatorImpl::onJwksError(Failure) { doneWithStatus(Status::JwksFetchF void AuthenticatorImpl::onDestroy() { if (fetcher_) { fetcher_->cancel(); - fetcher_.reset(nullptr); } } From 4bb61d26dd43ad315d4a882ca3f5c0d141304406 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Fri, 7 Sep 2018 21:40:57 +0100 Subject: [PATCH 16/22] Refactored unique_ptr pass by reference We now pass by const ptr. Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/jwks_fetcher_test.cc | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index d673d74b6399a..55c3d1f30c25a 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -108,8 +108,8 @@ class MockJwksReceiver : public JwksFetcher::JwksReceiver { * Expectations and assertions should be made on onJwksSuccessImpl in place * of onJwksSuccess. */ - void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { onJwksSuccessImpl(jwks); } - MOCK_METHOD1(onJwksSuccessImpl, void(google::jwt_verify::JwksPtr& jwks)); + void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { onJwksSuccessImpl(jwks.get()); } + MOCK_METHOD1(onJwksSuccessImpl, void(const google::jwt_verify::Jwks* jwks)); MOCK_METHOD1(onJwksError, void(JwksFetcher::JwksReceiver::Failure reason)); }; From 8c1c2949f204a79449185a033047519b7b8fa4cc Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Mon, 10 Sep 2018 12:50:59 +0100 Subject: [PATCH 17/22] Change of const pointer to const reference inline with policy. Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/jwks_fetcher_test.cc | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index 55c3d1f30c25a..15955a64c4d08 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -108,8 +108,11 @@ class MockJwksReceiver : public JwksFetcher::JwksReceiver { * Expectations and assertions should be made on onJwksSuccessImpl in place * of onJwksSuccess. */ - void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { onJwksSuccessImpl(jwks.get()); } - MOCK_METHOD1(onJwksSuccessImpl, void(const google::jwt_verify::Jwks* jwks)); + void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { + ASSERT(jwks); + onJwksSuccessImpl(*jwks.get()); + } + MOCK_METHOD1(onJwksSuccessImpl, void(const google::jwt_verify::Jwks& jwks)); MOCK_METHOD1(onJwksError, void(JwksFetcher::JwksReceiver::Failure reason)); }; From 17ddf46b5fefb57e5e675b78f13b035fee81bc60 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Mon, 10 Sep 2018 15:56:04 +0100 Subject: [PATCH 18/22] Fixing bazel/repository_locations.bzl Signed-off-by: Nick A. Smith --- bazel/repository_locations.bzl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/bazel/repository_locations.bzl b/bazel/repository_locations.bzl index 0091d94632a59..d7f770952b9b4 100644 --- a/bazel/repository_locations.bzl +++ b/bazel/repository_locations.bzl @@ -75,7 +75,7 @@ REPOSITORY_LOCATIONS = dict( remote = "https://github.com/google/googleapis", ), com_github_google_jwt_verify = dict( - commit = "66792a057ec54e4b75c6a2eeda4e98220bd12a9a", # 2018-08-17 + commit = "66792a057ec54e4b75c6a2eeda4e98220bd12a9a", # 2018-08-17 remote = "https://github.com/google/jwt_verify_lib", ), com_github_nodejs_http_parser = dict( From 8038924f0ef07a970e6186180cf10bbe380c4559 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Tue, 11 Sep 2018 16:32:47 +0100 Subject: [PATCH 19/22] Refactoring location of mocks for re-usability Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/BUILD | 1 + .../filters/http/common/jwks_fetcher_test.cc | 62 +------------------ test/extensions/filters/http/common/mock.h | 62 +++++++++++++++++++ 3 files changed, 64 insertions(+), 61 deletions(-) diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD index 103f14765dcd9..9083739f0c4ca 100644 --- a/test/extensions/filters/http/common/BUILD +++ b/test/extensions/filters/http/common/BUILD @@ -25,6 +25,7 @@ envoy_extension_cc_test( extension_name = "envoy.filters.http.jwt_authn", deps = [ "//source/extensions/filters/http/common:jwks_fetcher_lib", + "//test/extensions/filters/http/common:mock_lib", "//test/mocks/http:http_mocks", "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", diff --git a/test/extensions/filters/http/common/jwks_fetcher_test.cc b/test/extensions/filters/http/common/jwks_fetcher_test.cc index 15955a64c4d08..46ce0063b1c0e 100644 --- a/test/extensions/filters/http/common/jwks_fetcher_test.cc +++ b/test/extensions/filters/http/common/jwks_fetcher_test.cc @@ -6,8 +6,8 @@ #include "extensions/filters/http/common/jwks_fetcher.h" +#include "test/extensions/filters/http/common/mock.h" #include "test/mocks/http/mocks.h" -#include "test/mocks/server/mocks.h" #include "test/test_common/utility.h" using ::envoy::api::v2::core::HttpUri; @@ -56,66 +56,6 @@ class JwksFetcherTest : public ::testing::Test { testing::NiceMock mock_factory_ctx_; }; -// A mock HTTP upstream with response body. -class MockUpstream { -public: - MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& status, - const std::string& response_body) - : request_(&mock_cm.async_client_), status_(status), response_body_(response_body) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Invoke([this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) - -> Http::AsyncClient::Request* { - Http::MessagePtr response_message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", status_}}})); - if (response_body_.length()) { - response_message->body().reset(new Buffer::OwnedImpl(response_body_)); - } else { - response_message->body().reset(nullptr); - } - cb.onSuccess(std::move(response_message)); - return &request_; - })); - } - - MockUpstream(Upstream::MockClusterManager& mock_cm, Http::AsyncClient::FailureReason reason) - : request_(&mock_cm.async_client_) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Invoke( - [this, reason]( - Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - cb.onFailure(reason); - return &request_; - })); - } - - MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) - : request_(&mock_cm.async_client_) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Return(request)); - } - -private: - Http::MockAsyncClientRequest request_; - std::string status_; - std::string response_body_; -}; - -class MockJwksReceiver : public JwksFetcher::JwksReceiver { -public: - /* GoogleMock does handle r-value references hence the below construction. - * Expectations and assertions should be made on onJwksSuccessImpl in place - * of onJwksSuccess. - */ - void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { - ASSERT(jwks); - onJwksSuccessImpl(*jwks.get()); - } - MOCK_METHOD1(onJwksSuccessImpl, void(const google::jwt_verify::Jwks& jwks)); - MOCK_METHOD1(onJwksError, void(JwksFetcher::JwksReceiver::Failure reason)); -}; - // Test findByIssuer TEST_F(JwksFetcherTest, TestGetSuccess) { // Setup diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h index f04335a061793..5895e09a0f109 100644 --- a/test/extensions/filters/http/common/mock.h +++ b/test/extensions/filters/http/common/mock.h @@ -1,5 +1,7 @@ #include "extensions/filters/http/common/jwks_fetcher.h" +#include "test/mocks/server/mocks.h" + #include "gmock/gmock.h" namespace Envoy { @@ -13,6 +15,66 @@ class MockJwksFetcher : public JwksFetcher { MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver& receiver)); }; +// A mock HTTP upstream with response body. +class MockUpstream { +public: + MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& status, + const std::string& response_body) + : request_(&mock_cm.async_client_), status_(status), response_body_(response_body) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke([this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) + -> Http::AsyncClient::Request* { + Http::MessagePtr response_message(new Http::ResponseMessageImpl( + Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", status_}}})); + if (response_body_.length()) { + response_message->body().reset(new Buffer::OwnedImpl(response_body_)); + } else { + response_message->body().reset(nullptr); + } + cb.onSuccess(std::move(response_message)); + return &request_; + })); + } + + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::AsyncClient::FailureReason reason) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke( + [this, reason]( + Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) -> Http::AsyncClient::Request* { + cb.onFailure(reason); + return &request_; + })); + } + + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Return(request)); + } + +private: + Http::MockAsyncClientRequest request_; + std::string status_; + std::string response_body_; +}; + +class MockJwksReceiver : public JwksFetcher::JwksReceiver { +public: + /* GoogleMock does handle r-value references hence the below construction. + * Expectations and assertions should be made on onJwksSuccessImpl in place + * of onJwksSuccess. + */ + void onJwksSuccess(google::jwt_verify::JwksPtr&& jwks) { + ASSERT(jwks); + onJwksSuccessImpl(*jwks.get()); + } + MOCK_METHOD1(onJwksSuccessImpl, void(const google::jwt_verify::Jwks& jwks)); + MOCK_METHOD1(onJwksError, void(JwksFetcher::JwksReceiver::Failure reason)); +}; + } // namespace Common } // namespace HttpFilters } // namespace Extensions From 1457fb1b4cccb8513c218c1675c30030d6338843 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Wed, 12 Sep 2018 21:38:08 +0100 Subject: [PATCH 20/22] Moved Mock constructors from header to translation unit Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/BUILD | 9 +++- test/extensions/filters/http/common/mock.cc | 48 ++++++++++++++++++++ test/extensions/filters/http/common/mock.h | 49 ++++++--------------- 3 files changed, 68 insertions(+), 38 deletions(-) create mode 100644 test/extensions/filters/http/common/mock.cc diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD index 9083739f0c4ca..3061b6951fa51 100644 --- a/test/extensions/filters/http/common/BUILD +++ b/test/extensions/filters/http/common/BUILD @@ -8,13 +8,19 @@ load( load( "//test/extensions:extensions_build_system.bzl", "envoy_extension_cc_test", + "envoy_cc_test_library", ) envoy_package() -envoy_cc_library( +envoy_cc_test_library( name = "mock_lib", + srcs = ["mock.cc"], hdrs = ["mock.h"], + deps = [ + "//source/extensions/filters/http/common:jwks_fetcher_lib", + "//test/mocks/server:server_mocks", + ], ) envoy_extension_cc_test( @@ -27,7 +33,6 @@ envoy_extension_cc_test( "//source/extensions/filters/http/common:jwks_fetcher_lib", "//test/extensions/filters/http/common:mock_lib", "//test/mocks/http:http_mocks", - "//test/mocks/server:server_mocks", "//test/test_common:utility_lib", ], ) diff --git a/test/extensions/filters/http/common/mock.cc b/test/extensions/filters/http/common/mock.cc new file mode 100644 index 0000000000000..3229f39cc5076 --- /dev/null +++ b/test/extensions/filters/http/common/mock.cc @@ -0,0 +1,48 @@ +#include "test/extensions/filters/http/common/mock.h" + +namespace Envoy { +namespace Extensions { +namespace HttpFilters { +namespace Common { +MockUpstream::MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& status, + const std::string& response_body) + : request_(&mock_cm.async_client_), status_(status), response_body_(response_body) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke( + [this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) -> Http::AsyncClient::Request* { + Http::MessagePtr response_message(new Http::ResponseMessageImpl( + Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", status_}}})); + if (response_body_.length()) { + response_message->body().reset(new Buffer::OwnedImpl(response_body_)); + } else { + response_message->body().reset(nullptr); + } + cb.onSuccess(std::move(response_message)); + return &request_; + })); +} + +MockUpstream::MockUpstream(Upstream::MockClusterManager& mock_cm, + Http::AsyncClient::FailureReason reason) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Invoke( + [this, reason]( + Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, + const absl::optional&) -> Http::AsyncClient::Request* { + cb.onFailure(reason); + return &request_; + })); +} + +MockUpstream::MockUpstream(Upstream::MockClusterManager& mock_cm, + Http::MockAsyncClientRequest* request) + : request_(&mock_cm.async_client_) { + ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) + .WillByDefault(testing::Return(request)); +} +} // namespace Common +} // namespace HttpFilters +} // namespace Extensions +} // namespace Envoy \ No newline at end of file diff --git a/test/extensions/filters/http/common/mock.h b/test/extensions/filters/http/common/mock.h index 5895e09a0f109..4d48f1b931704 100644 --- a/test/extensions/filters/http/common/mock.h +++ b/test/extensions/filters/http/common/mock.h @@ -15,45 +15,22 @@ class MockJwksFetcher : public JwksFetcher { MOCK_METHOD2(fetch, void(const ::envoy::api::v2::core::HttpUri& uri, JwksReceiver& receiver)); }; -// A mock HTTP upstream with response body. +// A mock HTTP upstream. class MockUpstream { public: + /** + * Mock upstream which returns a given response body. + */ MockUpstream(Upstream::MockClusterManager& mock_cm, const std::string& status, - const std::string& response_body) - : request_(&mock_cm.async_client_), status_(status), response_body_(response_body) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Invoke([this](Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) - -> Http::AsyncClient::Request* { - Http::MessagePtr response_message(new Http::ResponseMessageImpl( - Http::HeaderMapPtr{new Http::TestHeaderMapImpl{{":status", status_}}})); - if (response_body_.length()) { - response_message->body().reset(new Buffer::OwnedImpl(response_body_)); - } else { - response_message->body().reset(nullptr); - } - cb.onSuccess(std::move(response_message)); - return &request_; - })); - } - - MockUpstream(Upstream::MockClusterManager& mock_cm, Http::AsyncClient::FailureReason reason) - : request_(&mock_cm.async_client_) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Invoke( - [this, reason]( - Http::MessagePtr&, Http::AsyncClient::Callbacks& cb, - const absl::optional&) -> Http::AsyncClient::Request* { - cb.onFailure(reason); - return &request_; - })); - } - - MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request) - : request_(&mock_cm.async_client_) { - ON_CALL(mock_cm.async_client_, send_(testing::_, testing::_, testing::_)) - .WillByDefault(testing::Return(request)); - } + const std::string& response_body); + /** + * Mock upstream which returns a given failure. + */ + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::AsyncClient::FailureReason reason); + /** + * Mock upstream which returns the given request. + */ + MockUpstream(Upstream::MockClusterManager& mock_cm, Http::MockAsyncClientRequest* request); private: Http::MockAsyncClientRequest request_; From f95e51eeefb42afb14b314fddfd16316e801d879 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Wed, 12 Sep 2018 22:14:02 +0100 Subject: [PATCH 21/22] Fixing formatting of BUILD file Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/BUILD | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD index 3061b6951fa51..bed76bea54fcd 100644 --- a/test/extensions/filters/http/common/BUILD +++ b/test/extensions/filters/http/common/BUILD @@ -15,8 +15,12 @@ envoy_package() envoy_cc_test_library( name = "mock_lib", - srcs = ["mock.cc"], - hdrs = ["mock.h"], + srcs = [ + "mock.cc", + ], + hdrs = [ + "mock.h", + ], deps = [ "//source/extensions/filters/http/common:jwks_fetcher_lib", "//test/mocks/server:server_mocks", From 18b6b9da5b3627018991f94ed38d85180833d4f6 Mon Sep 17 00:00:00 2001 From: "Nick A. Smith" Date: Thu, 13 Sep 2018 21:20:23 +0100 Subject: [PATCH 22/22] Fixing format of t/e/f/h/c/BUILD Signed-off-by: Nick A. Smith --- test/extensions/filters/http/common/BUILD | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/extensions/filters/http/common/BUILD b/test/extensions/filters/http/common/BUILD index bed76bea54fcd..58f30f2f2d607 100644 --- a/test/extensions/filters/http/common/BUILD +++ b/test/extensions/filters/http/common/BUILD @@ -7,8 +7,8 @@ load( ) load( "//test/extensions:extensions_build_system.bzl", - "envoy_extension_cc_test", "envoy_cc_test_library", + "envoy_extension_cc_test", ) envoy_package()