From d7028f501c5061a50b30efe468fc2f4ac68159a9 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Mon, 30 Apr 2018 19:24:33 -0700 Subject: [PATCH 01/12] Update Envoy sha to d338e45e (#1659) --- WORKSPACE | 2 +- istio.deps | 2 +- src/envoy/http/authn/filter_context.h | 2 ++ src/envoy/http/jwt_auth/token_extractor.h | 1 + src/envoy/http/mixer/filter.cc | 4 +++- src/envoy/http/mixer/filter.h | 1 + src/envoy/http/mixer/report_data.h | 7 +++++-- 7 files changed, 14 insertions(+), 5 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index 4d1dc388e04..c6a795745f0 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -38,7 +38,7 @@ git_repository( ) # When updating envoy sha manually please update the sha in istio.deps file also -ENVOY_SHA = "d55f4990889da5d6874ab70ec5f4e5e0a9053160" +ENVOY_SHA = "d338e45e31be628f19c895003d0aeee6be18d32f" http_archive( name = "envoy", diff --git a/istio.deps b/istio.deps index 333616a7fcb..0a62a53ff55 100644 --- a/istio.deps +++ b/istio.deps @@ -13,6 +13,6 @@ "repoName": "envoyproxy/envoy", "prodBranch": "master", "file": "WORKSPACE", - "lastStableSHA": "d55f4990889da5d6874ab70ec5f4e5e0a9053160" + "lastStableSHA": "d338e45e31be628f19c895003d0aeee6be18d32f" } ] \ No newline at end of file diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 39a3c79b763..7907479d55f 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -18,6 +18,8 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/common/logger.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" +#include "envoy/http/header_map.h" +#include "envoy/network/connection.h" #include "src/istio/authn/context.pb.h" namespace Envoy { diff --git a/src/envoy/http/jwt_auth/token_extractor.h b/src/envoy/http/jwt_auth/token_extractor.h index da8a74ad459..4efd808afa4 100644 --- a/src/envoy/http/jwt_auth/token_extractor.h +++ b/src/envoy/http/jwt_auth/token_extractor.h @@ -17,6 +17,7 @@ #include "common/common/logger.h" #include "envoy/config/filter/http/jwt_authn/v2alpha/config.pb.h" +#include "envoy/http/header_map.h" namespace Envoy { namespace Http { diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index a2c67664143..2c186bdd9c5 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -211,6 +211,7 @@ void Filter::onDestroy() { void Filter::log(const HeaderMap* request_headers, const HeaderMap* response_headers, + const HeaderMap* response_trailers, const RequestInfo::RequestInfo& request_info) { ENVOY_LOG(debug, "Called Mixer::Filter : {}", __func__); if (!handler_) { @@ -227,7 +228,8 @@ void Filter::log(const HeaderMap* request_headers, handler_->ExtractRequestAttributes(&check_data); } // response trailer header is not counted to response total size. - ReportData report_data(response_headers, request_info, request_total_size_); + ReportData report_data(response_headers, response_trailers, request_info, + request_total_size_); handler_->Report(&report_data); } diff --git a/src/envoy/http/mixer/filter.h b/src/envoy/http/mixer/filter.h index 9b38bb54ccc..6c31add3993 100644 --- a/src/envoy/http/mixer/filter.h +++ b/src/envoy/http/mixer/filter.h @@ -55,6 +55,7 @@ class Filter : public Http::StreamDecoderFilter, // Called when the request is completed. virtual void log(const HeaderMap* request_headers, const HeaderMap* response_headers, + const HeaderMap* response_trailers, const RequestInfo::RequestInfo& request_info) override; private: diff --git a/src/envoy/http/mixer/report_data.h b/src/envoy/http/mixer/report_data.h index 7cd16e5c317..46658b60b5f 100644 --- a/src/envoy/http/mixer/report_data.h +++ b/src/envoy/http/mixer/report_data.h @@ -36,8 +36,8 @@ class ReportData : public ::istio::control::http::ReportData { uint64_t request_total_size_; public: - ReportData(const HeaderMap *headers, const RequestInfo::RequestInfo &info, - uint64_t request_total_size) + ReportData(const HeaderMap *headers, const HeaderMap *response_trailers, + const RequestInfo::RequestInfo &info, uint64_t request_total_size) : headers_(headers), info_(info), response_total_size_(info.bytesSent()), @@ -45,6 +45,9 @@ class ReportData : public ::istio::control::http::ReportData { if (headers != nullptr) { response_total_size_ += headers->byteSize(); } + if (response_trailers != nullptr) { + response_total_size_ += response_trailers->byteSize(); + } } std::map GetResponseHeaders() const override { From 55ac70643268b4e06470b43b9d78a36f0fc5d32e Mon Sep 17 00:00:00 2001 From: Quanjie Lin <32855694+quanjielin@users.noreply.github.com> Date: Tue, 1 May 2018 10:36:36 -0700 Subject: [PATCH 02/12] ignore verification if peer/origin optional fields are set (#1660) * ignore verfication if peer/origin optional fields are set --- src/envoy/http/authn/http_filter.cc | 4 +++- src/envoy/http/authn/http_filter_test.cc | 27 ++++++++++++++++++++---- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 75e172ef87b..e8fe41be1d2 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -51,12 +51,14 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, Payload payload; - if (!createPeerAuthenticator(filter_context_.get())->run(&payload)) { + if (!filter_config_.policy().peer_is_optional() && + !createPeerAuthenticator(filter_context_.get())->run(&payload)) { rejectRequest("Peer authentication failed."); return FilterHeadersStatus::StopIteration; } bool success = + filter_config_.policy().origin_is_optional() || createOriginAuthenticator(filter_context_.get())->run(&payload); // After Istio authn, the JWT headers consumed by Istio authn should be diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index 0c82a5e6959..79b39d7e76a 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -35,12 +35,19 @@ using testing::NiceMock; using testing::StrictMock; using testing::_; +namespace iaapi = istio::authentication::v1alpha1; + namespace Envoy { namespace Http { namespace Istio { namespace AuthN { namespace { +const char ingoreBothPolicy[] = R"( + peer_is_optional: true + origin_is_optional: true +)"; + // Create a fake authenticator for test. This authenticator do nothing except // making the authentication fail. std::unique_ptr createAlwaysFailAuthenticator( @@ -74,8 +81,9 @@ class MockAuthenticationFilter : public AuthenticationFilter { public: // We'll use fake authenticator for test, so policy is not really needed. Use // default config for simplicity. - MockAuthenticationFilter() - : AuthenticationFilter(FilterConfig::default_instance()) {} + MockAuthenticationFilter(const FilterConfig& filter_config) + : AuthenticationFilter(filter_config) {} + ~MockAuthenticationFilter(){}; MOCK_METHOD1(createPeerAuthenticator, @@ -95,9 +103,11 @@ class AuthenticationFilterTest : public testing::Test { } protected: - StrictMock filter_; - NiceMock decoder_callbacks_; + FilterConfig filter_config_ = FilterConfig::default_instance(); + Http::TestHeaderMapImpl request_headers_; + StrictMock filter_{filter_config_}; + NiceMock decoder_callbacks_; }; TEST_F(AuthenticationFilterTest, PeerFail) { @@ -151,6 +161,15 @@ TEST_F(AuthenticationFilterTest, AllPass) { TestUtilities::AuthNResultFromString(R"(peer_user: "foo")"), authn)); } +TEST_F(AuthenticationFilterTest, IgnoreBothFail) { + iaapi::Policy policy_; + ASSERT_TRUE( + Protobuf::TextFormat::ParseFromString(ingoreBothPolicy, &policy_)); + *filter_config_.mutable_policy() = policy_; + EXPECT_EQ(Http::FilterHeadersStatus::Continue, + filter_.decodeHeaders(request_headers_, true)); +} + } // namespace } // namespace AuthN } // namespace Istio From 178406d5c1bdebbfbc673134fcb7a7e7784593a7 Mon Sep 17 00:00:00 2001 From: Pengyuan Bian <34738376+bianpengyuan@users.noreply.github.com> Date: Wed, 2 May 2018 09:56:04 -0700 Subject: [PATCH 03/12] Create a child span for mixer check call (#1610) Automatic merge from submit-queue. Create a child span for mixer check call --- src/envoy/http/mixer/control.cc | 4 +-- src/envoy/http/mixer/control.h | 2 +- src/envoy/http/mixer/filter.cc | 3 +- src/envoy/utils/grpc_transport.cc | 53 ++++++------------------------- src/envoy/utils/grpc_transport.h | 8 ++--- src/envoy/utils/mixer_control.cc | 6 ++-- 6 files changed, 21 insertions(+), 55 deletions(-) diff --git a/src/envoy/http/mixer/control.cc b/src/envoy/http/mixer/control.cc index fb8fd8bedb2..0ce9e327415 100644 --- a/src/envoy/http/mixer/control.cc +++ b/src/envoy/http/mixer/control.cc @@ -42,8 +42,8 @@ Control::Control(const Config& config, Upstream::ClusterManager& cm, } Utils::CheckTransport::Func Control::GetCheckTransport( - const HeaderMap* headers) { - return Utils::CheckTransport::GetFunc(*check_client_factory_, headers); + Tracing::Span& parent_span) { + return Utils::CheckTransport::GetFunc(*check_client_factory_, parent_span); } // Call controller to get statistics. diff --git a/src/envoy/http/mixer/control.h b/src/envoy/http/mixer/control.h index aacf783c512..07740a4df9e 100644 --- a/src/envoy/http/mixer/control.h +++ b/src/envoy/http/mixer/control.h @@ -41,7 +41,7 @@ class Control final : public ThreadLocal::ThreadLocalObject { ::istio::control::http::Controller* controller() { return controller_.get(); } // Create a per-request Check transport function. - Utils::CheckTransport::Func GetCheckTransport(const HeaderMap* headers); + Utils::CheckTransport::Func GetCheckTransport(Tracing::Span& parent_span); private: // Call controller to get statistics. diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index 2c186bdd9c5..597ec5af51c 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -133,7 +133,8 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { HeaderUpdate header_update(&headers); headers_ = &headers; cancel_check_ = handler_->Check( - &check_data, &header_update, control_.GetCheckTransport(&headers), + &check_data, &header_update, + control_.GetCheckTransport(decoder_callbacks_->activeSpan()), [this](const Status& status) { completeCheck(status); }); initiating_call_ = false; diff --git a/src/envoy/utils/grpc_transport.cc b/src/envoy/utils/grpc_transport.cc index fe7923a2a48..8c2829f13ae 100644 --- a/src/envoy/utils/grpc_transport.cc +++ b/src/envoy/utils/grpc_transport.cc @@ -25,59 +25,23 @@ namespace { // gRPC request timeout const std::chrono::milliseconds kGrpcRequestTimeoutMs(5000); -// HTTP trace headers that should pass to gRPC metadata from origin request. -// x-request-id is added for easy debugging. -const Http::LowerCaseString kRequestId("x-request-id"); -const Http::LowerCaseString kB3TraceId("x-b3-traceid"); -const Http::LowerCaseString kB3SpanId("x-b3-spanid"); -const Http::LowerCaseString kB3ParentSpanId("x-b3-parentspanid"); -const Http::LowerCaseString kB3Sampled("x-b3-sampled"); -const Http::LowerCaseString kB3Flags("x-b3-flags"); -const Http::LowerCaseString kOtSpanContext("x-ot-span-context"); - -inline void CopyHeaderEntry(const Http::HeaderEntry *entry, - const Http::LowerCaseString &key, - Http::HeaderMap &headers) { - if (entry) { - std::string val(entry->value().c_str(), entry->value().size()); - headers.addReferenceKey(key, val); - } -} - } // namespace template GrpcTransport::GrpcTransport( Grpc::AsyncClientPtr async_client, const RequestType &request, - const Http::HeaderMap *headers, ResponseType *response, + ResponseType *response, Tracing::Span &parent_span, istio::mixerclient::DoneFunc on_done) : async_client_(std::move(async_client)), - headers_(headers), response_(response), on_done_(on_done), request_(async_client_->send( - descriptor(), request, *this, Tracing::NullSpan::instance(), + descriptor(), request, *this, parent_span, absl::optional(kGrpcRequestTimeoutMs))) { ENVOY_LOG(debug, "Sending {} request: {}", descriptor().name(), request.DebugString()); } -template -void GrpcTransport::onCreateInitialMetadata( - Http::HeaderMap &metadata) { - if (!headers_) return; - - CopyHeaderEntry(headers_->RequestId(), kRequestId, metadata); - CopyHeaderEntry(headers_->XB3TraceId(), kB3TraceId, metadata); - CopyHeaderEntry(headers_->XB3SpanId(), kB3SpanId, metadata); - CopyHeaderEntry(headers_->XB3ParentSpanId(), kB3ParentSpanId, metadata); - CopyHeaderEntry(headers_->XB3Sampled(), kB3Sampled, metadata); - CopyHeaderEntry(headers_->XB3Flags(), kB3Flags, metadata); - - // This one is NOT inline, need to do linar search. - CopyHeaderEntry(headers_->get(kOtSpanContext), kOtSpanContext, metadata); -} - template void GrpcTransport::onSuccess( std::unique_ptr &&response, Tracing::Span &) { @@ -107,12 +71,13 @@ void GrpcTransport::Cancel() { template typename GrpcTransport::Func GrpcTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers) { - return [&factory, headers](const RequestType &request, ResponseType *response, - istio::mixerclient::DoneFunc on_done) + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span) { + return [&factory, &parent_span](const RequestType &request, + ResponseType *response, + istio::mixerclient::DoneFunc on_done) -> istio::mixerclient::CancelFunc { auto transport = new GrpcTransport( - factory.create(), request, headers, response, on_done); + factory.create(), request, response, parent_span, on_done); return [transport]() { transport->Cancel(); }; }; } @@ -137,9 +102,9 @@ const google::protobuf::MethodDescriptor &ReportTransport::descriptor() { // explicitly instantiate CheckTransport and ReportTransport template CheckTransport::Func CheckTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); template ReportTransport::Func ReportTransport::GetFunc( - Grpc::AsyncClientFactory &factory, const Http::HeaderMap *headers); + Grpc::AsyncClientFactory &factory, Tracing::Span &parent_span); } // namespace Utils } // namespace Envoy diff --git a/src/envoy/utils/grpc_transport.h b/src/envoy/utils/grpc_transport.h index 2cb60834bec..bda56b6621f 100644 --- a/src/envoy/utils/grpc_transport.h +++ b/src/envoy/utils/grpc_transport.h @@ -39,14 +39,13 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, istio::mixerclient::DoneFunc on_done)>; static Func GetFunc(Grpc::AsyncClientFactory& factory, - const Http::HeaderMap* headers = nullptr); + Tracing::Span& parent_span); GrpcTransport(Grpc::AsyncClientPtr async_client, const RequestType& request, - const Http::HeaderMap* headers, ResponseType* response, + ResponseType* response, Tracing::Span& parent_span, istio::mixerclient::DoneFunc on_done); - // Grpc::AsyncRequestCallbacks - void onCreateInitialMetadata(Http::HeaderMap& metadata) override; + void onCreateInitialMetadata(Http::HeaderMap&) override {} void onSuccess(std::unique_ptr&& response, Tracing::Span& span) override; @@ -60,7 +59,6 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, static const google::protobuf::MethodDescriptor& descriptor(); Grpc::AsyncClientPtr async_client_; - const Http::HeaderMap* headers_; ResponseType* response_; ::istio::mixerclient::DoneFunc on_done_; Grpc::AsyncRequest* request_{}; diff --git a/src/envoy/utils/mixer_control.cc b/src/envoy/utils/mixer_control.cc index 01ccaac74b5..082dde704b5 100644 --- a/src/envoy/utils/mixer_control.cc +++ b/src/envoy/utils/mixer_control.cc @@ -61,8 +61,10 @@ void CreateEnvironment(Event::Dispatcher &dispatcher, Grpc::AsyncClientFactory &check_client_factory, Grpc::AsyncClientFactory &report_client_factory, ::istio::mixerclient::Environment *env) { - env->check_transport = CheckTransport::GetFunc(check_client_factory, nullptr); - env->report_transport = ReportTransport::GetFunc(report_client_factory); + env->check_transport = CheckTransport::GetFunc(check_client_factory, + Tracing::NullSpan::instance()); + env->report_transport = ReportTransport::GetFunc( + report_client_factory, Tracing::NullSpan::instance()); env->timer_create_func = [&dispatcher](std::function timer_cb) -> std::unique_ptr<::istio::mixerclient::Timer> { From e3dc84e10ab4229f7793308ae6c075239b92a80c Mon Sep 17 00:00:00 2001 From: istio-bot Date: Wed, 2 May 2018 18:25:53 -0700 Subject: [PATCH 04/12] Update_Dependencies (#1692) --- istio.deps | 2 -- 1 file changed, 2 deletions(-) diff --git a/istio.deps b/istio.deps index 0a62a53ff55..dccaf244e85 100644 --- a/istio.deps +++ b/istio.deps @@ -3,7 +3,6 @@ "_comment": "", "name": "ISTIO_API", "repoName": "api", - "prodBranch": "master", "file": "repositories.bzl", "lastStableSHA": "793db0cb1a2e33012166f1d6ee5918596b8199d4" }, @@ -11,7 +10,6 @@ "_comment": "", "name": "ENVOY_SHA", "repoName": "envoyproxy/envoy", - "prodBranch": "master", "file": "WORKSPACE", "lastStableSHA": "d338e45e31be628f19c895003d0aeee6be18d32f" } From 2ec70769602dfc47b5ac66f9665b703f226247a5 Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Fri, 4 May 2018 14:45:06 -0700 Subject: [PATCH 05/12] Update Envoy sha to 2b2c299144600fb9e525d21aabf39bf48e64fb1f (#1734) --- WORKSPACE | 4 +--- istio.deps | 2 +- src/envoy/http/authn/http_filter_factory.cc | 10 +++++----- src/envoy/http/jwt_auth/http_filter_factory.cc | 12 ++++++------ .../http_filter_integration_test.cc | 2 -- src/envoy/http/mixer/filter_factory.cc | 18 ++++++++++-------- src/envoy/tcp/mixer/filter_factory.cc | 10 +++++----- 7 files changed, 28 insertions(+), 30 deletions(-) diff --git a/WORKSPACE b/WORKSPACE index c6a795745f0..b864fa6513a 100644 --- a/WORKSPACE +++ b/WORKSPACE @@ -38,7 +38,7 @@ git_repository( ) # When updating envoy sha manually please update the sha in istio.deps file also -ENVOY_SHA = "d338e45e31be628f19c895003d0aeee6be18d32f" +ENVOY_SHA = "2b2c299144600fb9e525d21aabf39bf48e64fb1f" http_archive( name = "envoy", @@ -60,8 +60,6 @@ load("@com_lyft_protoc_gen_validate//bazel:go_proto_library.bzl", "go_proto_repo go_proto_repositories(shared=0) go_rules_dependencies() go_register_toolchains() -load("@io_bazel_rules_go//proto:def.bzl", "proto_register_toolchains") -proto_register_toolchains() load("@bazel_tools//tools/build_defs/repo:git.bzl", "git_repository") git_repository( diff --git a/istio.deps b/istio.deps index dccaf244e85..e1de0566466 100644 --- a/istio.deps +++ b/istio.deps @@ -11,6 +11,6 @@ "name": "ENVOY_SHA", "repoName": "envoyproxy/envoy", "file": "WORKSPACE", - "lastStableSHA": "d338e45e31be628f19c895003d0aeee6be18d32f" + "lastStableSHA": "2b2c299144600fb9e525d21aabf39bf48e64fb1f" } ] \ No newline at end of file diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index 18ce537248b..dcf344fac78 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -34,9 +34,9 @@ const std::string kAuthnFactoryName("istio_authn"); class AuthnFilterConfig : public NamedHttpFilterConfigFactory, public Logger::Loggable { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext&) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config, + const std::string&, + FactoryContext&) override { ENVOY_LOG(debug, "Called AuthnFilterConfig : {}", __func__); google::protobuf::util::Status status = @@ -54,7 +54,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, } } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string&, FactoryContext&) override { filter_config_ = dynamic_cast(proto_config); @@ -69,7 +69,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, std::string name() override { return kAuthnFactoryName; } private: - HttpFilterFactoryCb createFilter() { + Http::FilterFactoryCb createFilter() { ENVOY_LOG(debug, "Called AuthnFilterConfig : {}", __func__); return [&](Http::FilterChainFactoryCallbacks& callbacks) -> void { diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 4656902e786..018b8a65788 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -27,15 +27,15 @@ namespace Configuration { class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config, - const std::string&, - FactoryContext& context) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config, + const std::string&, + FactoryContext& context) override { JwtAuthentication proto_config; MessageUtil::loadFromJson(config.asJsonString(), proto_config); return createFilter(proto_config, context); } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string&, FactoryContext& context) override { return createFilter( @@ -51,8 +51,8 @@ class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { std::string name() override { return "jwt-auth"; } private: - HttpFilterFactoryCb createFilter(const JwtAuthentication& proto_config, - FactoryContext& context) { + Http::FilterFactoryCb createFilter(const JwtAuthentication& proto_config, + FactoryContext& context) { auto store_factory = std::make_shared( proto_config, context); Upstream::ClusterManager& cm = context.clusterManager(); diff --git a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc index c750f014629..5d53ba3cff3 100644 --- a/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc +++ b/src/envoy/http/jwt_auth/integration_test/http_filter_integration_test.cc @@ -358,8 +358,6 @@ INSTANTIATE_TEST_CASE_P( TEST_P(JwtVerificationFilterIntegrationTestWithInjectedJwtResult, InjectedJwtResultSanitized) { - // Issuer is not called by passing empty pubkey. - std::string pubkey = ""; // Create a request without JWT. // With allow_missing_or_failed option being true, a request without JWT // will reach the backend. This is to test the injected JWT result. diff --git a/src/envoy/http/mixer/filter_factory.cc b/src/envoy/http/mixer/filter_factory.cc index f8a5165a342..4f045d479ba 100644 --- a/src/envoy/http/mixer/filter_factory.cc +++ b/src/envoy/http/mixer/filter_factory.cc @@ -30,9 +30,9 @@ namespace Configuration { class MixerConfigFactory : public NamedHttpFilterConfigFactory { public: - HttpFilterFactoryCb createFilterFactory(const Json::Object& config_json, - const std::string& prefix, - FactoryContext& context) override { + Http::FilterFactoryCb createFilterFactory(const Json::Object& config_json, + const std::string& prefix, + FactoryContext& context) override { HttpClientConfig config_pb; if (!Utils::ReadV2Config(config_json, &config_pb) && !Utils::ReadV1Config(config_json, &config_pb)) { @@ -42,7 +42,7 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { return createFilterFactory(config_pb, prefix, context); } - HttpFilterFactoryCb createFilterFactoryFromProto( + Http::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& proto_config, const std::string& prefix, FactoryContext& context) override { return createFilterFactory( @@ -58,7 +58,9 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { } Router::RouteSpecificFilterConfigConstSharedPtr - createRouteSpecificFilterConfig(const Protobuf::Message& config) override { + createRouteSpecificFilterConfig( + const Protobuf::Message& config, + Envoy::Server::Configuration::FactoryContext&) override { auto obj = std::make_shared(); // TODO: use downcastAndValidate once client_config.proto adds validate // rules. @@ -70,9 +72,9 @@ class MixerConfigFactory : public NamedHttpFilterConfigFactory { std::string name() override { return "mixer"; } private: - HttpFilterFactoryCb createFilterFactory(const HttpClientConfig& config_pb, - const std::string&, - FactoryContext& context) { + Http::FilterFactoryCb createFilterFactory(const HttpClientConfig& config_pb, + const std::string&, + FactoryContext& context) { std::unique_ptr config_obj( new Http::Mixer::Config(config_pb)); auto control_factory = std::make_shared( diff --git a/src/envoy/tcp/mixer/filter_factory.cc b/src/envoy/tcp/mixer/filter_factory.cc index 7290226c1f8..fe9732a8865 100644 --- a/src/envoy/tcp/mixer/filter_factory.cc +++ b/src/envoy/tcp/mixer/filter_factory.cc @@ -26,8 +26,8 @@ namespace Configuration { class FilterFactory : public NamedNetworkFilterConfigFactory { public: - NetworkFilterFactoryCb createFilterFactory(const Json::Object& config_json, - FactoryContext& context) override { + Network::FilterFactoryCb createFilterFactory( + const Json::Object& config_json, FactoryContext& context) override { TcpClientConfig config_pb; if (!Utils::ReadV2Config(config_json, &config_pb) && !Utils::ReadV1Config(config_json, &config_pb)) { @@ -37,7 +37,7 @@ class FilterFactory : public NamedNetworkFilterConfigFactory { return createFilterFactory(config_pb, context); } - NetworkFilterFactoryCb createFilterFactoryFromProto( + Network::FilterFactoryCb createFilterFactoryFromProto( const Protobuf::Message& config, FactoryContext& context) override { return createFilterFactory(dynamic_cast(config), context); @@ -50,8 +50,8 @@ class FilterFactory : public NamedNetworkFilterConfigFactory { std::string name() override { return "mixer"; } private: - NetworkFilterFactoryCb createFilterFactory(const TcpClientConfig& config_pb, - FactoryContext& context) { + Network::FilterFactoryCb createFilterFactory(const TcpClientConfig& config_pb, + FactoryContext& context) { std::unique_ptr config_obj( new Tcp::Mixer::Config(config_pb)); From cf72117a96cd19e5a4f18d7049f25886f252cbba Mon Sep 17 00:00:00 2001 From: istio-bot Date: Sun, 6 May 2018 21:00:15 -0700 Subject: [PATCH 06/12] Update_Dependencies (#1736) --- istio.deps | 2 +- repositories.bzl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/istio.deps b/istio.deps index e1de0566466..aee82e911dd 100644 --- a/istio.deps +++ b/istio.deps @@ -4,7 +4,7 @@ "name": "ISTIO_API", "repoName": "api", "file": "repositories.bzl", - "lastStableSHA": "793db0cb1a2e33012166f1d6ee5918596b8199d4" + "lastStableSHA": "4eac2c3d96dcbe2b56006e6bdfa643d160695da8" }, { "_comment": "", diff --git a/repositories.bzl b/repositories.bzl index ddb728d0074..b9944d93543 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -113,7 +113,7 @@ cc_library( actual = "@googletest_git//:googletest_prod", ) -ISTIO_API = "793db0cb1a2e33012166f1d6ee5918596b8199d4" +ISTIO_API = "4eac2c3d96dcbe2b56006e6bdfa643d160695da8" def mixerapi_repositories(bind=True): BUILD = """ From 42faac24eec913b4c9983003734bf658754e429d Mon Sep 17 00:00:00 2001 From: Wayne Zhang Date: Tue, 8 May 2018 15:41:18 -0700 Subject: [PATCH 07/12] Strip debug info for release (#1738) --- tools/bazel.rc | 1 + 1 file changed, 1 insertion(+) diff --git a/tools/bazel.rc b/tools/bazel.rc index 0038dfd79ff..4ce27056965 100644 --- a/tools/bazel.rc +++ b/tools/bazel.rc @@ -49,6 +49,7 @@ test --test_env=HEAPCHECK=normal --test_env=PPROF_PATH # Release builds build:release -c opt +build:release --strip=always # Add compile option for all C++ files build --cxxopt -Wnon-virtual-dtor From 87907acb81ba8adc009f18e5c499c8d1842e18c8 Mon Sep 17 00:00:00 2001 From: Lizan Zhou Date: Wed, 9 May 2018 14:25:49 -0700 Subject: [PATCH 08/12] hardcode authority header for mixer temporarily (#1740) --- src/envoy/utils/grpc_transport.h | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/envoy/utils/grpc_transport.h b/src/envoy/utils/grpc_transport.h index bda56b6621f..41bbffe5407 100644 --- a/src/envoy/utils/grpc_transport.h +++ b/src/envoy/utils/grpc_transport.h @@ -45,7 +45,12 @@ class GrpcTransport : public Grpc::TypedAsyncRequestCallbacks, ResponseType* response, Tracing::Span& parent_span, istio::mixerclient::DoneFunc on_done); - void onCreateInitialMetadata(Http::HeaderMap&) override {} + void onCreateInitialMetadata(Http::HeaderMap& metadata) override { + // We generate cluster name contains invalid characters, so override the + // authority header temorarily until it can be specified via CDS. + // See https://github.com/envoyproxy/envoy/issues/3297 for details. + metadata.Host()->value("mixer", 5); + } void onSuccess(std::unique_ptr&& response, Tracing::Span& span) override; From 61c5a39b38c11cca47bc16f8d1f30797c290ea52 Mon Sep 17 00:00:00 2001 From: Diem Vu <25132401+diemtvu@users.noreply.github.com> Date: Wed, 9 May 2018 15:41:57 -0700 Subject: [PATCH 09/12] Use mTLS mode enum to decide authentication result. (#1733) * Update_Dependencies (#1657) * Use mTLS mode enum to decide authentication result. * Clang * Use NOT_REACHED macros for default case. * Clang --- istio.deps | 2 +- src/envoy/http/authn/authenticator_base.cc | 26 ++- .../http/authn/authenticator_base_test.cc | 162 ++++++++---------- 3 files changed, 93 insertions(+), 97 deletions(-) diff --git a/istio.deps b/istio.deps index aee82e911dd..1a9c3861b3b 100644 --- a/istio.deps +++ b/istio.deps @@ -13,4 +13,4 @@ "file": "WORKSPACE", "lastStableSHA": "2b2c299144600fb9e525d21aabf39bf48e64fb1f" } -] \ No newline at end of file +] diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 02ee0e796e0..a2dba3de4cc 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -14,6 +14,7 @@ */ #include "src/envoy/http/authn/authenticator_base.h" +#include "common/common/assert.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" @@ -34,16 +35,31 @@ AuthenticatorBase::~AuthenticatorBase() {} bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, Payload* payload) const { const Network::Connection* connection = filter_context_.connection(); - if (connection == nullptr || connection->ssl() == nullptr) { - // Not a TLS connection + if (connection == nullptr) { + // It's wrong if connection does not exist. return false; } - - bool has_user = + // Always try to get principal and set to output if available. + const bool has_user = + connection->ssl() != nullptr && connection->ssl()->peerCertificatePresented() && Utils::GetSourceUser(connection, payload->mutable_x509()->mutable_user()); - return has_user || mtls.allow_tls(); + // Return value depend on mode: + // - PERMISSIVE: plaintext connection is acceptable, thus return true + // regardless. + // - TLS_PERMISSIVE: tls connection is required, but certificate is optional. + // - STRICT: must be TLS with valid certificate. + switch (mtls.mode()) { + case iaapi::MutualTls::PERMISSIVE: + return true; + case iaapi::MutualTls::TLS_PERMISSIVE: + return connection->ssl() != nullptr; + case iaapi::MutualTls::STRICT: + return has_user; + default: + NOT_REACHED; + } } bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index e9b9885e33f..f223c0e9433 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -55,10 +55,10 @@ class MockAuthenticatorBase : public AuthenticatorBase { MOCK_METHOD1(run, bool(Payload*)); }; -class AuthenticatorBaseTest : public testing::Test, - public Logger::Loggable { +class ValidateX509Test : public testing::TestWithParam, + public Logger::Loggable { public: - virtual ~AuthenticatorBaseTest() {} + virtual ~ValidateX509Test() {} Http::TestHeaderMapImpl request_headers_{}; NiceMock connection_{}; @@ -70,7 +70,10 @@ class AuthenticatorBaseTest : public testing::Test, MockAuthenticatorBase authenticator_{&filter_context_}; - void SetUp() override { payload_ = new Payload(); } + void SetUp() override { + mtls_params_.set_mode(GetParam()); + payload_ = new Payload(); + } void TearDown() override { delete (payload_); } @@ -81,51 +84,34 @@ class AuthenticatorBaseTest : public testing::Test, Payload default_payload_; }; -Http::TestHeaderMapImpl CreateTestHeaderMap(const std::string& header_key, - const std::string& header_value) { - // The base64 encoding is done through Base64::encode(). - // If the test input has special chars, may need to use the counterpart of - // Base64UrlDecode(). - std::string value_base64 = - Base64::encode(header_value.c_str(), header_value.size()); - return Http::TestHeaderMapImpl{{header_key, value_base64}}; -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnPlaintextConnection) { - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); - EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); -} - -TEST_F(AuthenticatorBaseTest, ValidateTlsOnPlaintextConnection) { - mtls_params_.set_allow_tls(true); // allow TLS connection - // When requiring TLS, plaintext connection should fail. - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); +TEST_P(ValidateX509Test, PlaintextConnection) { + // Should return false except mode is PERMISSIVE (accept plaintext) + if (GetParam() == iaapi::MutualTls::PERMISSIVE) { + EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); + } else { + EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + } EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithNoPeerCert) { +TEST_P(ValidateX509Test, SslConnectionWithNoPeerCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) .WillOnce(Return(false)); - EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + // Should return false except mode is PERMISSIVE (accept plaintext) or + // TLS_PERMISSIVE + if (GetParam() == iaapi::MutualTls::PERMISSIVE || + GetParam() == iaapi::MutualTls::TLS_PERMISSIVE) { + EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); + } else { + EXPECT_FALSE(authenticator_.validateX509(mtls_params_, payload_)); + } EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithNoPeerCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(false)); - - // When client certificate is not present on TLS, authentication should still - // succeed. - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -137,20 +123,7 @@ TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerCert) { EXPECT_EQ(payload_->x509().user(), "foo"); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithPeerCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()).Times(1).WillOnce(Return("foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS, authenticated attribute should - // be extracted. - EXPECT_EQ(payload_->x509().user(), "foo"); -} - -TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerSpiffeCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerSpiffeCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -165,23 +138,7 @@ TEST_F(AuthenticatorBaseTest, ValidateMtlsOnSslConnectionWithPeerSpiffeCert) { EXPECT_EQ(payload_->x509().user(), "foo"); } -TEST_F(AuthenticatorBaseTest, ValidateTlsOnSslConnectionWithPeerSpiffeCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()) - .Times(1) - .WillOnce(Return("spiffe://foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS, authenticated attribute should - // be extracted. - EXPECT_EQ(payload_->x509().user(), "foo"); -} - -TEST_F(AuthenticatorBaseTest, - ValidateMtlsOnSslConnectionWithPeerMalformedSpiffeCert) { +TEST_P(ValidateX509Test, SslConnectionWithPeerMalformedSpiffeCert) { EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); EXPECT_CALL(Const(ssl_), peerCertificatePresented()) .Times(1) @@ -198,26 +155,49 @@ TEST_F(AuthenticatorBaseTest, EXPECT_EQ(payload_->x509().user(), "spiffe:foo"); } -TEST_F(AuthenticatorBaseTest, - ValidateTlsOnSslConnectionWithPeerMalformedSpiffeCert) { - mtls_params_.set_allow_tls(true); // allow TLS connection - EXPECT_CALL(Const(connection_), ssl()).WillRepeatedly(Return(&ssl_)); - EXPECT_CALL(Const(ssl_), peerCertificatePresented()) - .Times(1) - .WillOnce(Return(true)); - EXPECT_CALL(ssl_, uriSanPeerCertificate()) - .Times(1) - .WillOnce(Return("spiffe:foo")); - EXPECT_TRUE(authenticator_.validateX509(mtls_params_, payload_)); - // When client certificate is present on TLS and the spiffe subject format is - // wrong - // ("spiffe:foo" instead of "spiffe://foo"), the user attribute should be - // extracted. - EXPECT_EQ(payload_->x509().user(), "spiffe:foo"); -} +INSTANTIATE_TEST_CASE_P(ValidateX509Tests, ValidateX509Test, + testing::Values(iaapi::MutualTls::STRICT, + iaapi::MutualTls::TLS_PERMISSIVE, + iaapi::MutualTls::PERMISSIVE)); + +class ValidateJwtTest : public testing::Test, + public Logger::Loggable { + public: + virtual ~ValidateJwtTest() {} + + Http::TestHeaderMapImpl request_headers_{}; + NiceMock connection_{}; + NiceMock ssl_{}; + FilterConfig filter_config_{}; + FilterContext filter_context_{&request_headers_, &connection_, + istio::envoy::config::filter::http::authn:: + v2alpha1::FilterConfig::default_instance()}; + + MockAuthenticatorBase authenticator_{&filter_context_}; + + void SetUp() override { payload_ = new Payload(); } + + void TearDown() override { delete (payload_); } + + Http::TestHeaderMapImpl CreateTestHeaderMap(const std::string& header_key, + const std::string& header_value) { + // The base64 encoding is done through Base64::encode(). + // If the test input has special chars, may need to use the counterpart of + // Base64UrlDecode(). + std::string value_base64 = + Base64::encode(header_value.c_str(), header_value.size()); + return Http::TestHeaderMapImpl{{header_key, value_base64}}; + } + + protected: + iaapi::MutualTls mtls_params_; + iaapi::Jwt jwt_; + Payload* payload_; + Payload default_payload_; +}; // TODO: more tests for Jwt. -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIstioAuthnConfig) { +TEST_F(ValidateJwtTest, ValidateJwtWithNoIstioAuthnConfig) { jwt_.set_issuer("issuer@foo.com"); // authenticator_ has empty Istio authn config // When there is empty Istio authn config, validateJwt() should return @@ -226,7 +206,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIstioAuthnConfig) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIssuer) { +TEST_F(ValidateJwtTest, NoIssuer) { // no issuer in jwt google::protobuf::util::JsonParseOptions options; FilterConfig filter_config; @@ -250,7 +230,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoIssuer) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithEmptyJwtOutputPayloadLocations) { +TEST_F(ValidateJwtTest, EmptyJwtOutputPayloadLocations) { jwt_.set_issuer("issuer@foo.com"); Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( kSecIstioAuthUserInfoHeaderKey, kSecIstioAuthUserinfoHeaderValue); @@ -274,7 +254,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithEmptyJwtOutputPayloadLocations) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoJwtInHeader) { +TEST_F(ValidateJwtTest, NoJwtInHeader) { jwt_.set_issuer("issuer@foo.com"); google::protobuf::util::JsonParseOptions options; FilterConfig filter_config; @@ -297,7 +277,7 @@ TEST_F(AuthenticatorBaseTest, ValidateJwtWithNoJwtInHeader) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(AuthenticatorBaseTest, ValidateJwtWithJwtInHeader) { +TEST_F(ValidateJwtTest, JwtInHeader) { jwt_.set_issuer("issuer@foo.com"); Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( "sec-istio-auth-jwt-output", kSecIstioAuthUserinfoHeaderValue); From f0d5123d3d2b0e2d6ffc11845052db4f084652f0 Mon Sep 17 00:00:00 2001 From: istio-bot Date: Wed, 9 May 2018 17:04:03 -0700 Subject: [PATCH 10/12] Update_Dependencies (#1739) --- istio.deps | 2 +- repositories.bzl | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/istio.deps b/istio.deps index 1a9c3861b3b..9ec2873ac03 100644 --- a/istio.deps +++ b/istio.deps @@ -4,7 +4,7 @@ "name": "ISTIO_API", "repoName": "api", "file": "repositories.bzl", - "lastStableSHA": "4eac2c3d96dcbe2b56006e6bdfa643d160695da8" + "lastStableSHA": "636ea68d62bf143ee9f8067a31a7824804613440" }, { "_comment": "", diff --git a/repositories.bzl b/repositories.bzl index b9944d93543..bf9e1e7d501 100644 --- a/repositories.bzl +++ b/repositories.bzl @@ -113,7 +113,7 @@ cc_library( actual = "@googletest_git//:googletest_prod", ) -ISTIO_API = "4eac2c3d96dcbe2b56006e6bdfa643d160695da8" +ISTIO_API = "636ea68d62bf143ee9f8067a31a7824804613440" def mixerapi_repositories(bind=True): BUILD = """ From c105b23de8800c4ec7e970973efdf22a2ca37ca0 Mon Sep 17 00:00:00 2001 From: Diem Vu <25132401+diemtvu@users.noreply.github.com> Date: Fri, 11 May 2018 09:26:31 -0700 Subject: [PATCH 11/12] Sending full JWT claims as json string via new attribute request.auth.raw_claims (#1744) * Add raw_claims attribute to hold all original claims from JWT * Clang --- src/envoy/http/authn/authenticator_base_test.cc | 3 ++- src/envoy/http/authn/authn_utils.cc | 1 + src/envoy/http/authn/authn_utils_test.cc | 12 +++++++++--- src/envoy/http/authn/http_filter_integration_test.cc | 3 ++- src/istio/authn/context.proto | 5 +++++ src/istio/control/attribute_names.cc | 1 + src/istio/control/attribute_names.h | 1 + src/istio/control/http/attributes_builder.cc | 4 ++++ src/istio/control/http/attributes_builder_test.cc | 9 +++++++++ 9 files changed, 34 insertions(+), 5 deletions(-) diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index f223c0e9433..9c6d69c6510 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -307,7 +307,8 @@ TEST_F(ValidateJwtTest, JwtInHeader) { "iss": "issuer@foo.com", "sub": "sub@foo.com", "some-other-string-claims": "some-claims-kept" - } + }, + raw_claims: "\n {\n \"iss\": \"issuer@foo.com\",\n \"sub\": \"sub@foo.com\",\n \"aud\": \"aud1\",\n \"non-string-will-be-ignored\": 1512754205,\n \"some-other-string-claims\": \"some-claims-kept\"\n }\n " } } )", diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 2dc57bfed20..78c232e3108 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/src/envoy/http/authn/authn_utils.cc @@ -68,6 +68,7 @@ bool AuthnUtils::GetJWTPayloadFromHeaders( jwt_payload_key.get(), value); return false; } + *payload->mutable_raw_claims() = payload_str; ::google::protobuf::Map< ::std::string, ::std::string>* claims = payload->mutable_claims(); Envoy::Json::ObjectSharedPtr json_obj; diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index fe9f3105445..3663476ab0d 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -14,6 +14,7 @@ */ #include "src/envoy/http/authn/authn_utils.h" #include "common/common/base64.h" +#include "common/common/utility.h" #include "src/envoy/http/authn/test_utils.h" #include "test/test_common/utility.h" @@ -91,7 +92,8 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserinfoHeaderValue) + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as // the expected. @@ -121,7 +123,9 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithNoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserInfoHeaderWithNoAudValue) + + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as // the expected. When there is no aud, the aud is not saved in the payload @@ -154,7 +158,9 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithTwoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } - )", + raw_claims: ")" + + StringUtil::escape(kSecIstioAuthUserInfoHeaderWithTwoAudValue) + + R"(")", &expected_payload)); // The payload returned from GetJWTPayloadFromHeaders() should be the same as diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 3ec0c1197e1..5690ef7766c 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -214,7 +214,8 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckAuthnResultIsExpected) { "aud": "bookstore-esp-echo.cloudendpointsapis.com", "iss": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com", "sub": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com" - } + }, + raw_claims: "{\"iss\":\"628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com\",\"sub\":\"628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com\",\"aud\":\"bookstore-esp-echo.cloudendpointsapis.com\",\"iat\":1512754205,\"exp\":5112754205}" } } )", diff --git a/src/istio/authn/context.proto b/src/istio/authn/context.proto index d87385b5d0f..5ea3fdb67d5 100644 --- a/src/istio/authn/context.proto +++ b/src/istio/authn/context.proto @@ -35,6 +35,11 @@ message JwtPayload { // Only raw JWT string claims are kept. map claims = 5; + + // All original claims in JsonString format, which can be parsed into json + // object (map) to access other claims that not cover with the string claims + // map above. + string raw_claims = 6; } // Container to hold authenticated attributes from X509 (mTLS). diff --git a/src/istio/control/attribute_names.cc b/src/istio/control/attribute_names.cc index 3f0326a84af..953957998a9 100644 --- a/src/istio/control/attribute_names.cc +++ b/src/istio/control/attribute_names.cc @@ -78,6 +78,7 @@ const char AttributeName::kRequestAuthPrincipal[] = "request.auth.principal"; const char AttributeName::kRequestAuthAudiences[] = "request.auth.audiences"; const char AttributeName::kRequestAuthPresenter[] = "request.auth.presenter"; const char AttributeName::kRequestAuthClaims[] = "request.auth.claims"; +const char AttributeName::kRequestAuthRawClaims[] = "request.auth.raw_claims"; } // namespace control } // namespace istio diff --git a/src/istio/control/attribute_names.h b/src/istio/control/attribute_names.h index 09bb1488e25..2ed3dfdf46a 100644 --- a/src/istio/control/attribute_names.h +++ b/src/istio/control/attribute_names.h @@ -85,6 +85,7 @@ struct AttributeName { static const char kRequestAuthAudiences[]; static const char kRequestAuthPresenter[]; static const char kRequestAuthClaims[]; + static const char kRequestAuthRawClaims[]; }; } // namespace control diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 4aac9a7d468..d85bc4f2bcf 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -89,6 +89,10 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { builder.AddProtobufStringMap(AttributeName::kRequestAuthClaims, origin.claims()); } + if (!origin.raw_claims().empty()) { + builder.AddString(AttributeName::kRequestAuthRawClaims, + origin.raw_claims()); + } } return; } diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 958e4d3f5a2..9ac9019442e 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -398,6 +398,7 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithAuthNResult) { "thisisemail@email.com"; (*result->mutable_origin()->mutable_claims())["iat"] = "1512754205"; (*result->mutable_origin()->mutable_claims())["exp"] = "5112754205"; + result->mutable_origin()->set_raw_claims("test_raw_claims"); return true; })); @@ -414,6 +415,14 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithAuthNResult) { Attributes expected_attributes; ASSERT_TRUE( TextFormat::ParseFromString(kCheckAttributes, &expected_attributes)); + // kCheckAttributes is also used in TestCheckAttributes, which is a deprecated + // way to construct mixer attribute (it was a fallback when authn filter is + // not available, which can be removed after 0.8). For now, modifying expected + // data manually for this test. + (*expected_attributes + .mutable_attributes())[AttributeName::kRequestAuthRawClaims] + .set_string_value("test_raw_claims"); + EXPECT_TRUE( MessageDifferencer::Equals(request.attributes, expected_attributes)); } From 2325b6f036803c6342eec7f98392ecf48c80de34 Mon Sep 17 00:00:00 2001 From: istio-bot Date: Mon, 14 May 2018 10:27:08 -0700 Subject: [PATCH 12/12] Update_Dependencies (#1749) --- istio.deps | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/istio.deps b/istio.deps index 9ec2873ac03..13503371a98 100644 --- a/istio.deps +++ b/istio.deps @@ -13,4 +13,4 @@ "file": "WORKSPACE", "lastStableSHA": "2b2c299144600fb9e525d21aabf39bf48e64fb1f" } -] +] \ No newline at end of file