From 1378f9402b572ef00fcee458cfaf8c78423f61de Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Thu, 2 Aug 2018 17:48:26 -0700 Subject: [PATCH 1/8] Using dynamicMetadata to pass data between filters instead of headers --- include/istio/utils/BUILD | 8 ++ include/istio/utils/filter_names.h | 34 +++++ src/envoy/http/authn/BUILD | 3 + src/envoy/http/authn/authenticator_base.cc | 11 +- .../http/authn/authenticator_base_test.cc | 118 ++++++++++-------- src/envoy/http/authn/authn_utils.cc | 25 +--- src/envoy/http/authn/authn_utils.h | 9 +- src/envoy/http/authn/authn_utils_test.cc | 38 ++---- src/envoy/http/authn/filter_context.cc | 1 - src/envoy/http/authn/filter_context.h | 17 +-- src/envoy/http/authn/filter_context_test.cc | 2 +- src/envoy/http/authn/http_filter.cc | 24 +--- src/envoy/http/authn/http_filter.h | 3 - src/envoy/http/authn/http_filter_factory.cc | 8 +- .../http/authn/origin_authenticator_test.cc | 10 +- .../http/authn/peer_authenticator_test.cc | 9 +- src/envoy/http/jwt_auth/BUILD | 2 + src/envoy/http/jwt_auth/http_filter.cc | 5 + src/envoy/http/jwt_auth/http_filter.h | 4 + .../http/jwt_auth/http_filter_factory.cc | 3 +- .../http_filter_integration_test.cc | 29 +++-- src/envoy/http/jwt_auth/jwt_authenticator.cc | 25 ++-- src/envoy/http/jwt_auth/jwt_authenticator.h | 1 + .../http/jwt_auth/jwt_authenticator_test.cc | 55 ++++---- src/istio/utils/BUILD | 11 ++ src/istio/utils/filter_names.cc | 26 ++++ 26 files changed, 269 insertions(+), 212 deletions(-) create mode 100644 include/istio/utils/filter_names.h create mode 100644 src/istio/utils/filter_names.cc diff --git a/include/istio/utils/BUILD b/include/istio/utils/BUILD index f343c5a51e7..1c31b7180d2 100644 --- a/include/istio/utils/BUILD +++ b/include/istio/utils/BUILD @@ -42,3 +42,11 @@ cc_library( ], visibility = ["//visibility:public"], ) + +cc_library( + name = "filter_names_header", + hdrs = [ + "filter_names.h", + ], + visibility = ["//visibility:public"], +) \ No newline at end of file diff --git a/include/istio/utils/filter_names.h b/include/istio/utils/filter_names.h new file mode 100644 index 00000000000..a821325b34f --- /dev/null +++ b/include/istio/utils/filter_names.h @@ -0,0 +1,34 @@ +/* Copyright 2018 Istio Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#ifndef ISTIO_UTILS_FILTER_NAMES_H +#define ISTIO_UTILS_FILTER_NAMES_H + +#include + +namespace istio { +namespace utils { + +// These are name of filters that currently output data to dynamicMetadata (by convention, under the +// the entry using filter name itself as key). Define them here for easy access. +struct FilterName { + static const char kJwt[]; + static const char kAuthentication[]; +}; + +} // namespace utils +} // namespace istio + +#endif // ISTIO_UTILS_FILTER_NAMES_H diff --git a/src/envoy/http/authn/BUILD b/src/envoy/http/authn/BUILD index 6c9efa35324..17e2d3fc6dd 100644 --- a/src/envoy/http/authn/BUILD +++ b/src/envoy/http/authn/BUILD @@ -66,6 +66,7 @@ envoy_cc_library( "//src/envoy/utils:utils_lib", "//src/istio/authn:context_proto", "@envoy//source/exe:envoy_common_lib", + "//src/istio/utils:filter_names_lib", ], ) @@ -85,6 +86,7 @@ envoy_cc_test( deps = [ ":authenticator", ":test_utils", + "@envoy//test/mocks/http:http_mocks", "@envoy//test/test_common:utility_lib", ], ) @@ -97,6 +99,7 @@ envoy_cc_test( ":authenticator", ":test_utils", "@envoy//test/mocks/network:network_mocks", + "@envoy//test/mocks/request_info:request_info_mocks", "@envoy//test/mocks/ssl:ssl_mocks", "@envoy//test/test_common:utility_lib", ], diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index d53f8cc0119..0ec6c78b827 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -15,6 +15,7 @@ #include "src/envoy/http/authn/authenticator_base.h" #include "common/common/assert.h" +#include "common/config/metadata.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" @@ -64,8 +65,6 @@ bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, } bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { - Envoy::Http::HeaderMap& header = *filter_context()->headers(); - auto iter = filter_context()->filter_config().jwt_output_payload_locations().find( jwt.issuer()); @@ -76,9 +75,11 @@ bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { return false; } - LowerCaseString header_key(iter->second); - return AuthnUtils::GetJWTPayloadFromHeaders(header, header_key, - payload->mutable_jwt()); + const auto& value = Envoy::Config::Metadata::metadataValue(filter_context()->request_info().dynamicMetadata(), "jwt-auth", iter->second); + if (!value.string_value().empty()) { + return AuthnUtils::ProcessJwtPayload(value.string_value(), payload->mutable_jwt()); + } + return false; } } // namespace AuthN diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 454e40e19c4..4bf7c889248 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -20,12 +20,14 @@ #include "gmock/gmock.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/network/mocks.h" +#include "test/mocks/request_info/mocks.h" #include "test/mocks/ssl/mocks.h" using google::protobuf::util::MessageDifferencer; using istio::authn::Payload; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; using testing::NiceMock; +using testing::StrictMock; using testing::Return; namespace iaapi = istio::authentication::v1alpha1; @@ -60,11 +62,11 @@ class ValidateX509Test : public testing::TestWithParam, public: virtual ~ValidateX509Test() {} - Http::TestHeaderMapImpl request_headers_{}; + NiceMock request_info_{}; NiceMock connection_{}; NiceMock ssl_{}; FilterConfig filter_config_{}; - FilterContext filter_context_{&request_headers_, &connection_, + FilterContext filter_context_{&request_info_, &connection_, istio::envoy::config::filter::http::authn:: v2alpha1::FilterConfig::default_instance()}; @@ -162,30 +164,17 @@ class ValidateJwtTest : public testing::Test, public: virtual ~ValidateJwtTest() {} - Http::TestHeaderMapImpl request_headers_{}; + StrictMock request_info_{}; NiceMock connection_{}; - NiceMock ssl_{}; + // NiceMock ssl_{}; FilterConfig filter_config_{}; - FilterContext filter_context_{&request_headers_, &connection_, - istio::envoy::config::filter::http::authn:: - v2alpha1::FilterConfig::default_instance()}; - + FilterContext filter_context_{&request_info_, &connection_, filter_config_}; 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_; @@ -193,8 +182,7 @@ class ValidateJwtTest : public testing::Test, Payload default_payload_; }; -// TODO: more tests for Jwt. -TEST_F(ValidateJwtTest, ValidateJwtWithNoIstioAuthnConfig) { +TEST_F(ValidateJwtTest, NoIstioAuthnConfig) { jwt_.set_issuer("issuer@foo.com"); // authenticator_ has empty Istio authn config // When there is empty Istio authn config, validateJwt() should return @@ -206,7 +194,6 @@ TEST_F(ValidateJwtTest, ValidateJwtWithNoIstioAuthnConfig) { TEST_F(ValidateJwtTest, NoIssuer) { // no issuer in jwt google::protobuf::util::JsonParseOptions options; - FilterConfig filter_config; JsonStringToMessage( R"({ "jwt_output_payload_locations": @@ -215,11 +202,7 @@ TEST_F(ValidateJwtTest, NoIssuer) { } } )", - &filter_config, options); - Http::TestHeaderMapImpl empty_request_headers{}; - FilterContext filter_context{&empty_request_headers, &connection_, - filter_config}; - MockAuthenticatorBase authenticator{&filter_context}; + &filter_config_, options); // When there is no issuer in the JWT config, validateJwt() should return // nullptr and failure. @@ -227,12 +210,9 @@ TEST_F(ValidateJwtTest, NoIssuer) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(ValidateJwtTest, EmptyJwtOutputPayloadLocations) { +TEST_F(ValidateJwtTest, OutputPayloadLocationNotDefine) { jwt_.set_issuer("issuer@foo.com"); - Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( - kSecIstioAuthUserInfoHeaderKey, kSecIstioAuthUserinfoHeaderValue); google::protobuf::util::JsonParseOptions options; - FilterConfig filter_config; JsonStringToMessage( R"({ "jwt_output_payload_locations": @@ -240,10 +220,8 @@ TEST_F(ValidateJwtTest, EmptyJwtOutputPayloadLocations) { } } )", - &filter_config, options); - FilterContext filter_context{&request_headers_with_jwt, &connection_, - filter_config}; - MockAuthenticatorBase authenticator{&filter_context}; + &filter_config_, options); + // authenticator has empty jwt_output_payload_locations in Istio authn config // When there is no matching jwt_output_payload_locations for the issuer in // the Istio authn config, validateJwt() should return nullptr and failure. @@ -251,10 +229,9 @@ TEST_F(ValidateJwtTest, EmptyJwtOutputPayloadLocations) { EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(ValidateJwtTest, NoJwtInHeader) { +TEST_F(ValidateJwtTest, NoJwtPayloadOutput) { jwt_.set_issuer("issuer@foo.com"); google::protobuf::util::JsonParseOptions options; - FilterConfig filter_config; JsonStringToMessage( R"({ "jwt_output_payload_locations": @@ -263,23 +240,18 @@ TEST_F(ValidateJwtTest, NoJwtInHeader) { } } )", - &filter_config, options); - Http::TestHeaderMapImpl empty_request_headers{}; - FilterContext filter_context{&empty_request_headers, &connection_, - filter_config}; - MockAuthenticatorBase authenticator{&filter_context}; + &filter_config_, options); + EXPECT_CALL(request_info_, dynamicMetadata()); + // When there is no JWT in the HTTP header, validateJwt() should return // nullptr and failure. EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } -TEST_F(ValidateJwtTest, JwtInHeader) { +TEST_F(ValidateJwtTest, HasJwtPayloadOutputButNoDataForKey) { jwt_.set_issuer("issuer@foo.com"); - Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( - "sec-istio-auth-jwt-output", kSecIstioAuthUserinfoHeaderValue); google::protobuf::util::JsonParseOptions options; - FilterConfig filter_config; JsonStringToMessage( R"({ "jwt_output_payload_locations": @@ -288,10 +260,56 @@ TEST_F(ValidateJwtTest, JwtInHeader) { } } )", - &filter_config, options); - FilterContext filter_context{&request_headers_with_jwt, &connection_, - filter_config}; - MockAuthenticatorBase authenticator{&filter_context}; + &filter_config_, options); + + (*request_info_.metadata_.mutable_filter_metadata())["jwt-awuth"].MergeFrom( + MessageUtil::keyValueStruct("foo", "bar")); + EXPECT_CALL(request_info_, dynamicMetadata()); + + // When there is no JWT in the HTTP header, validateJwt() should return + // nullptr and failure. + EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); + EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); +} + +TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { + jwt_.set_issuer("issuer@foo.com"); + google::protobuf::util::JsonParseOptions options; + JsonStringToMessage( + R"({ + "jwt_output_payload_locations": + { + "issuer@foo.com": "sec-istio-auth-jwt-output" + } + } + )", + &filter_config_, options); + + (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( + MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", "bad-data")); + EXPECT_CALL(request_info_, dynamicMetadata()); + + EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); + EXPECT_TRUE(MessageDifferencer::Equivalent(*payload_, default_payload_)); +} + +TEST_F(ValidateJwtTest, JwtPayloadAvailable) { + jwt_.set_issuer("issuer@foo.com"); + google::protobuf::util::JsonParseOptions options; + JsonStringToMessage( + R"({ + "jwt_output_payload_locations": + { + "issuer@foo.com": "sec-istio-auth-jwt-output" + } + } + )", + &filter_config_, options); + + (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( + MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", kSecIstioAuthUserinfoHeaderValue)); + EXPECT_CALL(request_info_, dynamicMetadata()); + Payload expected_payload; JsonStringToMessage( R"({ @@ -311,7 +329,7 @@ TEST_F(ValidateJwtTest, JwtInHeader) { )", &expected_payload, options); - EXPECT_TRUE(authenticator.validateJwt(jwt_, payload_)); + EXPECT_TRUE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, *payload_)); } diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 78c232e3108..7cb0b268e82 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/src/envoy/http/authn/authn_utils.cc @@ -50,27 +50,8 @@ void ExtractJwtAudience( } }; // namespace -// Retrieve the JwtPayload from the HTTP headers with the key -bool AuthnUtils::GetJWTPayloadFromHeaders( - const HeaderMap& headers, const LowerCaseString& jwt_payload_key, +bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str, istio::authn::JwtPayload* payload) { - const HeaderEntry* entry = headers.get(jwt_payload_key); - if (!entry) { - ENVOY_LOG(debug, "No JWT payload {} in the header", jwt_payload_key.get()); - return false; - } - std::string value(entry->value().c_str(), entry->value().size()); - // JwtAuth::Base64UrlDecode() is different from Base64::decode(). - std::string payload_str = JwtAuth::Base64UrlDecode(value); - // Return an empty string if Base64 decode fails. - if (payload_str.empty()) { - ENVOY_LOG(error, "Invalid {} header, invalid base64: {}", - 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; try { json_obj = Json::Factory::loadFromString(payload_str); @@ -80,6 +61,10 @@ bool AuthnUtils::GetJWTPayloadFromHeaders( return false; } + *payload->mutable_raw_claims() = payload_str; + ::google::protobuf::Map< ::std::string, ::std::string>* claims = + payload->mutable_claims(); + // Extract claims json_obj->iterate( [payload](const std::string& key, const Json::Object& obj) -> bool { diff --git a/src/envoy/http/authn/authn_utils.h b/src/envoy/http/authn/authn_utils.h index e156f5eb2c3..ce8801f30c5 100644 --- a/src/envoy/http/authn/authn_utils.h +++ b/src/envoy/http/authn/authn_utils.h @@ -28,12 +28,9 @@ namespace AuthN { // AuthnUtils class provides utility functions used for authentication. class AuthnUtils : public Logger::Loggable { public: - // Retrieve the JWT payload from the HTTP header into the output payload map - // Return true if parsing the header payload key succeeds. - // Otherwise, return false. - static bool GetJWTPayloadFromHeaders(const HeaderMap& headers, - const LowerCaseString& jwt_payload_key, - istio::authn::JwtPayload* payload); + // Parse JWT payload string (which typically is the output from jwt filter) and populate JwtPayload object. + // Return true if input string can be parsed successfully. Otherwise, return false. + static bool ProcessJwtPayload(const std::string& jwt_payload_str, istio::authn::JwtPayload* payload); }; } // namespace AuthN diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index 3663476ab0d..4ef0b8fcd10 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -27,7 +27,6 @@ namespace Istio { namespace AuthN { namespace { -const LowerCaseString kSecIstioAuthUserInfoHeaderKey("sec-istio-auth-userinfo"); const std::string kSecIstioAuthUserinfoHeaderValue = R"( { @@ -58,20 +57,8 @@ const std::string kSecIstioAuthUserInfoHeaderWithTwoAudValue = } )"; -Http::TestHeaderMapImpl CreateTestHeaderMap(const LowerCaseString& 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.get(), value_base64}}; -} - TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { JwtPayload payload, expected_payload; - Http::TestHeaderMapImpl request_headers_with_jwt = CreateTestHeaderMap( - kSecIstioAuthUserInfoHeaderKey, kSecIstioAuthUserinfoHeaderValue); ASSERT_TRUE(Protobuf::TextFormat::ParseFromString( R"( user: "issuer@foo.com/sub@foo.com" @@ -95,19 +82,15 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { raw_claims: ")" + StringUtil::escape(kSecIstioAuthUserinfoHeaderValue) + R"(")", &expected_payload)); - // The payload returned from GetJWTPayloadFromHeaders() should be the same as + // The payload returned from ProcessJwtPayload() should be the same as // the expected. - bool ret = AuthnUtils::GetJWTPayloadFromHeaders( - request_headers_with_jwt, kSecIstioAuthUserInfoHeaderKey, &payload); + bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserinfoHeaderValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } -TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithNoAudTest) { +TEST(AuthnUtilsTest, ProcessJwtPayloadWithNoAudTest) { JwtPayload payload, expected_payload; - Http::TestHeaderMapImpl request_headers_with_jwt = - CreateTestHeaderMap(kSecIstioAuthUserInfoHeaderKey, - kSecIstioAuthUserInfoHeaderWithNoAudValue); ASSERT_TRUE(Protobuf::TextFormat::ParseFromString( R"( user: "issuer@foo.com/sub@foo.com" @@ -127,20 +110,16 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithNoAudTest) { StringUtil::escape(kSecIstioAuthUserInfoHeaderWithNoAudValue) + R"(")", &expected_payload)); - // The payload returned from GetJWTPayloadFromHeaders() should be the same as + // The payload returned from ProcessJwtPayload() should be the same as // the expected. When there is no aud, the aud is not saved in the payload // and claims. - bool ret = AuthnUtils::GetJWTPayloadFromHeaders( - request_headers_with_jwt, kSecIstioAuthUserInfoHeaderKey, &payload); + bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserInfoHeaderWithNoAudValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } -TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithTwoAudTest) { +TEST(AuthnUtilsTest, ProcessJwtPayloadWithTwoAudTest) { JwtPayload payload, expected_payload; - Http::TestHeaderMapImpl request_headers_with_jwt = - CreateTestHeaderMap(kSecIstioAuthUserInfoHeaderKey, - kSecIstioAuthUserInfoHeaderWithTwoAudValue); ASSERT_TRUE(Protobuf::TextFormat::ParseFromString( R"( user: "issuer@foo.com/sub@foo.com" @@ -163,11 +142,10 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderWithTwoAudTest) { R"(")", &expected_payload)); - // The payload returned from GetJWTPayloadFromHeaders() should be the same as + // The payload returned from ProcessJwtPayload() should be the same as // the expected. When the aud is a string array, the aud is not saved in the // claims. - bool ret = AuthnUtils::GetJWTPayloadFromHeaders( - request_headers_with_jwt, kSecIstioAuthUserInfoHeaderKey, &payload); + bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserInfoHeaderWithTwoAudValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } diff --git a/src/envoy/http/authn/filter_context.cc b/src/envoy/http/authn/filter_context.cc index ac72ae8bc89..474bd00f469 100644 --- a/src/envoy/http/authn/filter_context.cc +++ b/src/envoy/http/authn/filter_context.cc @@ -70,7 +70,6 @@ void FilterContext::setPrincipal(const iaapi::PrincipalBinding& binding) { return; } } - } // namespace AuthN } // namespace Istio } // namespace Http diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 7907479d55f..9359a24a67a 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -18,7 +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/http/header_map.h" +#include "envoy/request_info/request_info.h" #include "envoy/network/connection.h" #include "src/istio/authn/context.pb.h" @@ -32,10 +33,10 @@ namespace AuthN { class FilterContext : public Logger::Loggable { public: FilterContext( - HeaderMap* headers, const Network::Connection* connection, + Envoy::RequestInfo::RequestInfo* request_info, const Network::Connection* connection, const istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig& filter_config) - : headers_(headers), + : request_info_(request_info), connection_(connection), filter_config_(filter_config) {} virtual ~FilterContext() {} @@ -56,8 +57,8 @@ class FilterContext : public Logger::Loggable { // Returns the authentication result. const istio::authn::Result& authenticationResult() { return result_; } - // Accessor to headers. - HeaderMap* headers() { return headers_; } + // Accessor to request_info. + const Envoy::RequestInfo::RequestInfo& request_info() const { return *request_info_; } // Accessor to connection const Network::Connection* connection() { return connection_; } // Accessor to the filter config @@ -66,9 +67,11 @@ class FilterContext : public Logger::Loggable { return filter_config_; } + bool getJwtPayload(const std::string& key, std::string* payload); + private: - // Pointer to the headers of the request. - HeaderMap* headers_; + // Pointer to the request info. + Envoy::RequestInfo::RequestInfo* request_info_; // Pointer to network connection of the request. const Network::Connection* connection_; diff --git a/src/envoy/http/authn/filter_context_test.cc b/src/envoy/http/authn/filter_context_test.cc index c00f3514ea0..4d54141b5a5 100644 --- a/src/envoy/http/authn/filter_context_test.cc +++ b/src/envoy/http/authn/filter_context_test.cc @@ -32,7 +32,7 @@ class FilterContextTest : public testing::Test { public: virtual ~FilterContextTest() {} - // This test suit does not use headers nor connection, so ok to use null for + // This test suit does not use request_info nor connection, so ok to use null for // them. FilterContext filter_context_{nullptr, nullptr, istio::envoy::config::filter::http::authn:: diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 27e14b8a088..b8cf15e8f82 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -21,6 +21,7 @@ #include "src/envoy/http/authn/peer_authenticator.h" #include "src/envoy/utils/authn.h" #include "src/envoy/utils/utils.h" +#include "include/istio/utils/filter_names.h" using istio::authn::Payload; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; @@ -41,21 +42,19 @@ void AuthenticationFilter::onDestroy() { ENVOY_LOG(debug, "Called AuthenticationFilter : {}", __func__); } -FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, - bool) { +FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { ENVOY_LOG(debug, "AuthenticationFilter::decodeHeaders with config\n{}", filter_config_.DebugString()); state_ = State::PROCESSING; filter_context_.reset(new Istio::AuthN::FilterContext( - &headers, decoder_callbacks_->connection(), filter_config_)); + &decoder_callbacks_->requestInfo(), decoder_callbacks_->connection(), filter_config_)); Payload payload; if (!filter_config_.policy().peer_is_optional() && !createPeerAuthenticator(filter_context_.get())->run(&payload)) { rejectRequest("Peer authentication failed."); - removeJwtPayloadFromHeaders(); return FilterHeadersStatus::StopIteration; } @@ -63,10 +62,6 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, filter_config_.policy().origin_is_optional() || createOriginAuthenticator(filter_context_.get())->run(&payload); - // After Istio authn, the JWT headers consumed by Istio authn should be - // removed. - removeJwtPayloadFromHeaders(); - if (!success) { rejectRequest("Origin authentication failed."); return FilterHeadersStatus::StopIteration; @@ -74,28 +69,17 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, // Put authentication result to headers. if (filter_context_ != nullptr) { - // TODO(yangminzhu): Remove the header and only use the metadata to pass the - // attributes. - Utils::Authentication::SaveResultToHeader( - filter_context_->authenticationResult(), filter_context_->headers()); - // Save auth results in the metadata, could be later used by RBAC filter. ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); - decoder_callbacks_->requestInfo().setDynamicMetadata("istio_authn", data); + decoder_callbacks_->requestInfo().setDynamicMetadata(istio::utils::FilterName::kAuthentication, data); ENVOY_LOG(debug, "Saved Dynamic Metadata:\n{}", data.DebugString()); } state_ = State::COMPLETE; return FilterHeadersStatus::Continue; } -void AuthenticationFilter::removeJwtPayloadFromHeaders() { - for (auto const iter : filter_config_.jwt_output_payload_locations()) { - filter_context_->headers()->remove(LowerCaseString(iter.second)); - } -} - FilterDataStatus AuthenticationFilter::decodeData(Buffer::Instance&, bool) { if (state_ == State::PROCESSING) { return FilterDataStatus::StopIterationAndWatermark; diff --git a/src/envoy/http/authn/http_filter.h b/src/envoy/http/authn/http_filter.h index c7a29a02a8e..a710cfc0559 100644 --- a/src/envoy/http/authn/http_filter.h +++ b/src/envoy/http/authn/http_filter.h @@ -63,9 +63,6 @@ class AuthenticationFilter : public StreamDecoderFilter, createOriginAuthenticator(Istio::AuthN::FilterContext* filter_context); - // Removes output JWT valiation from headers. - void removeJwtPayloadFromHeaders(); - private: // Store the config. const istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig& diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index 3fa6182f504..bcc5e29c518 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -19,6 +19,7 @@ #include "google/protobuf/util/json_util.h" #include "src/envoy/http/authn/http_filter.h" #include "src/envoy/utils/utils.h" +#include "include/istio/utils/filter_names.h" using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; @@ -26,11 +27,6 @@ namespace Envoy { namespace Server { namespace Configuration { -namespace { -// The name for the Istio authentication filter. -const std::string kAuthnFactoryName("istio_authn"); -} // namespace - class AuthnFilterConfig : public NamedHttpFilterConfigFactory, public Logger::Loggable { public: @@ -65,7 +61,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, return ProtobufTypes::MessagePtr{new FilterConfig}; } - std::string name() override { return kAuthnFactoryName; } + std::string name() override { return istio::utils::FilterName::kAuthentication; } private: Http::FilterFactoryCb createFilterFactory(const FilterConfig& config_pb) { diff --git a/src/envoy/http/authn/origin_authenticator_test.cc b/src/envoy/http/authn/origin_authenticator_test.cc index 8aeaa85215f..2e16ceafd6e 100644 --- a/src/envoy/http/authn/origin_authenticator_test.cc +++ b/src/envoy/http/authn/origin_authenticator_test.cc @@ -15,13 +15,13 @@ #include "src/envoy/http/authn/origin_authenticator.h" #include "authentication/v1alpha1/policy.pb.h" -#include "common/http/header_map_impl.h" #include "common/protobuf/protobuf.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" #include "test/test_common/utility.h" +#include "test/mocks/request_info/mocks.h" namespace iaapi = istio::authentication::v1alpha1; @@ -90,8 +90,8 @@ class MockOriginAuthenticator : public OriginAuthenticator { class OriginAuthenticatorTest : public testing::TestWithParam { public: - OriginAuthenticatorTest() - : request_headers_{{":method", "GET"}, {":path", "/"}} {} + OriginAuthenticatorTest() {} + // : request_headers_{{":method", "GET"}, {":path", "/"}} {} virtual ~OriginAuthenticatorTest() {} void SetUp() override { @@ -121,8 +121,8 @@ class OriginAuthenticatorTest : public testing::TestWithParam { protected: std::unique_ptr> authenticator_; - Http::TestHeaderMapImpl request_headers_; - FilterContext filter_context_{&request_headers_, nullptr, + StrictMock request_info_{}; + FilterContext filter_context_{&request_info_, nullptr, istio::envoy::config::filter::http::authn:: v2alpha1::FilterConfig::default_instance()}; iaapi::Policy policy_; diff --git a/src/envoy/http/authn/peer_authenticator_test.cc b/src/envoy/http/authn/peer_authenticator_test.cc index d0cc95b6f6a..58fab4f9762 100644 --- a/src/envoy/http/authn/peer_authenticator_test.cc +++ b/src/envoy/http/authn/peer_authenticator_test.cc @@ -15,13 +15,13 @@ #include "src/envoy/http/authn/peer_authenticator.h" #include "authentication/v1alpha1/policy.pb.h" -#include "common/http/header_map_impl.h" #include "common/protobuf/protobuf.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" #include "test/test_common/utility.h" +#include "test/mocks/request_info/mocks.h" namespace iaapi = istio::authentication::v1alpha1; @@ -52,8 +52,7 @@ class MockPeerAuthenticator : public PeerAuthenticator { class PeerAuthenticatorTest : public testing::Test { public: - PeerAuthenticatorTest() - : request_headers_{{":method", "GET"}, {":path", "/"}} {} + PeerAuthenticatorTest() {} virtual ~PeerAuthenticatorTest() {} void createAuthenticator() { @@ -67,8 +66,8 @@ class PeerAuthenticatorTest : public testing::Test { protected: std::unique_ptr> authenticator_; - Http::TestHeaderMapImpl request_headers_; - FilterContext filter_context_{&request_headers_, nullptr, + StrictMock request_info_{}; + FilterContext filter_context_{&request_info_, nullptr, istio::envoy::config::filter::http::authn:: v2alpha1::FilterConfig::default_instance()}; diff --git a/src/envoy/http/jwt_auth/BUILD b/src/envoy/http/jwt_auth/BUILD index a0f80bf43e4..b1f9537f1e8 100644 --- a/src/envoy/http/jwt_auth/BUILD +++ b/src/envoy/http/jwt_auth/BUILD @@ -70,6 +70,7 @@ envoy_cc_library( deps = [ ":jwt_authenticator_lib", "@envoy//source/exe:envoy_common_lib", + "//src/istio/utils:filter_names_lib", ], ) @@ -80,6 +81,7 @@ envoy_cc_library( deps = [ ":http_filter_lib", "@envoy//source/exe:envoy_common_lib", + "//src/istio/utils:filter_names_lib", ], ) diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 405cc611bab..128aabd0b3c 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -18,6 +18,7 @@ #include "common/http/message_impl.h" #include "common/http/utility.h" #include "envoy/http/async_client.h" +#include "include/istio/utils/filter_names.h" #include #include @@ -72,6 +73,10 @@ void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { } } +void JwtVerificationFilter::savePayload(const std::string& key, const std::string& payload) { + decoder_callbacks_->requestInfo().setDynamicMetadata(istio::utils::FilterName::kJwt, MessageUtil::keyValueStruct(key, payload)); +} + FilterDataStatus JwtVerificationFilter::decodeData(Buffer::Instance&, bool) { if (state_ == Calling) { return FilterDataStatus::StopIterationAndWatermark; diff --git a/src/envoy/http/jwt_auth/http_filter.h b/src/envoy/http/jwt_auth/http_filter.h index 838605faa6c..bcf8c583a54 100644 --- a/src/envoy/http/jwt_auth/http_filter.h +++ b/src/envoy/http/jwt_auth/http_filter.h @@ -47,6 +47,10 @@ class JwtVerificationFilter : public StreamDecoderFilter, // To be called when its Verify() call is completed. void onDone(const JwtAuth::Status& status) override; + // the function for JwtAuth::Authenticator::Callbacks interface. + // To be called when Jwt validation success to save payload for future use. + void savePayload(const std::string& key, const std::string& payload) override; + // The callback funcion. StreamDecoderFilterCallbacks* decoder_callbacks_; // The auth object. diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 7c3b11fd86d..0feaa3cf620 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -17,6 +17,7 @@ #include "google/protobuf/util/json_util.h" #include "src/envoy/http/jwt_auth/auth_store.h" #include "src/envoy/http/jwt_auth/http_filter.h" +#include "include/istio/utils/filter_names.h" using ::istio::envoy::config::filter::http::jwt_auth::v2alpha1:: JwtAuthentication; @@ -46,7 +47,7 @@ class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { return ProtobufTypes::MessagePtr{new JwtAuthentication}; } - std::string name() override { return "jwt-auth"; } + std::string name() override { return istio::utils::FilterName::kJwt; } private: Http::FilterFactoryCb createFilter(const JwtAuthentication& proto_config, 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 a5ae802d76d..c32f74a2402 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 @@ -260,11 +260,13 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, RSASuccess1) { "xfP590ACPyXrivtsxg"; auto expected_headers = BaseRequestHeaders(); - expected_headers.addCopy( - kJwtVerificationResultHeaderKey, - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" - "dEBleGFtcGxlLmNvbSIsImF1ZCI6ImV4YW1wbGVfc2VydmljZSIs" - "ImV4cCI6MjAwMTAwMTAwMX0"); + // TODO: JWT payload is not longer output to header. Find way to verify that data equivalent to + // this is added to dynamicMetadata. + // expected_headers.addCopy( + // kJwtVerificationResultHeaderKey, + // "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" + // "dEBleGFtcGxlLmNvbSIsImF1ZCI6ImV4YW1wbGVfc2VydmljZSIs" + // "ImV4cCI6MjAwMTAwMTAwMX0"); TestVerification(createHeaders(kJwtNoKid), "", createIssuerHeaders(), kPublicKeyRSA, true, expected_headers, ""); @@ -283,11 +285,13 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, ES256Success1) { "T9ubWvRvNGGYOTuJ8T17Db68Qk3T8UNTK5lzfR_mw"; auto expected_headers = BaseRequestHeaders(); - expected_headers.addCopy(kJwtVerificationResultHeaderKey, - "eyJpc3MiOiJo" - "dHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtc" - "GxlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbX" - "BsZV9zZXJ2aWNlIn0"); + // TODO: JWT payload is not longer output to header. Find way to verify that data equivalent to + // this is added to dynamicMetadata. + // expected_headers.addCopy(kJwtVerificationResultHeaderKey, + // "eyJpc3MiOiJo" + // "dHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtc" + // "GxlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbX" + // "BsZV9zZXJ2aWNlIn0"); TestVerification(createHeaders(kJwtEC), "", createIssuerHeaders(), kPublicKeyEC, true, expected_headers, ""); @@ -380,11 +384,6 @@ TEST_P(JwtVerificationFilterIntegrationTestWithInjectedJwtResult, request_stream_backend->waitForEndStream(*dispatcher_); EXPECT_TRUE(request_stream_backend->complete()); - // With sanitization, the headers received by the backend should not - // contain the injected JWT verification header. - EXPECT_TRUE(request_stream_backend->headers().get( - kJwtVerificationResultHeaderKey) == nullptr); - response->waitForEndStream(); codec_client->close(); fake_upstream_connection_backend->close(); diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index 56b21356136..bd75321a5f5 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -60,14 +60,14 @@ void JwtAuthenticator::Verify(HeaderMap& headers, callback_ = callback; // Sanitize the JWT verification result in the HTTP headers - for (const auto& rule : store_.config().rules()) { - if (!rule.forward_payload_header().empty()) { - ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", - rule.forward_payload_header()); - const LowerCaseString key(rule.forward_payload_header()); - headers.remove(key); - } - } + // for (const auto& rule : store_.config().rules()) { + // if (!rule.forward_payload_header().empty()) { + // ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", + // rule.forward_payload_header()); + // const LowerCaseString key(rule.forward_payload_header()); + // headers.remove(key); + // } + // } ENVOY_LOG(debug, "Jwt authentication starts"); std::vector> tokens; @@ -206,9 +206,12 @@ void JwtAuthenticator::VerifyKey(const PubkeyCacheItem& issuer_item) { } if (!issuer_item.jwt_config().forward_payload_header().empty()) { - const LowerCaseString key( - issuer_item.jwt_config().forward_payload_header()); - headers_->addCopy(key, jwt_->PayloadStrBase64Url()); + // TODO: can we save as proto or json object directly? + callback_->savePayload(issuer_item.jwt_config().forward_payload_header(), + jwt_->PayloadStr()); + // const LowerCaseString key( + // issuer_item.jwt_config().forward_payload_header()); + // headers_->addCopy(key, jwt_->PayloadStrBase64Url()); } if (!issuer_item.jwt_config().forward()) { diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.h b/src/envoy/http/jwt_auth/jwt_authenticator.h index 748b5752799..8d8513d2795 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.h +++ b/src/envoy/http/jwt_auth/jwt_authenticator.h @@ -36,6 +36,7 @@ class JwtAuthenticator : public Logger::Loggable, public: virtual ~Callbacks() {} virtual void onDone(const Status& status) PURE; + virtual void savePayload(const std::string& key, const std::string& payload) PURE; }; void Verify(HeaderMap& headers, Callbacks* callback); diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index ae090943251..21a878f2dfe 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -89,7 +89,7 @@ const std::string kPublicKey = "}]}"; // Keep this same as forward_payload_header field in the config below. -const char kOutputHeadersKey[] = "test-output"; +const char kJwtPayloadOutputKey[] = "test-output"; // A good JSON config. const char kExampleConfig[] = R"( { @@ -216,6 +216,11 @@ const std::string kGoodToken = "EprqSZUzi_ZzzYzqBNVhIJujcNWij7JRra2sXXiSAfKjtxHQoxrX8n4V1ySWJ3_1T" "H_cJcdfS_RKP7YgXRWC0L16PNF5K7iqRqmjKALNe83ZFnFIw"; +// Payload output for kGoodToken. +const std::string kGoodTokenPayload = + "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"example_service\"}"; + // Payload: // {"iss":"https://example.com","sub":"test@example.com","aud":"http://example_service/","exp":2001001001} const std::string kGoodTokenAudHasProtocolScheme = @@ -228,6 +233,11 @@ const std::string kGoodTokenAudHasProtocolScheme = "EpfDR3lAXSaug1NE22zX_tm0d9JnC5ZrIk3kwmPJPrnAS2_9RKTQW2e2skpAT8dUV" "T5aSpQxJmWIkyp4PKWmH6h4H2INS7hWyASZdX4oW-R0PMy3FAd8D6Y8740A"; +// Payload output for kGoodTokenAudHasProtocolScheme. +const std::string kGoodTokenAudHasProtocolSchemePayload = + "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"http://example_service/\"}"; + // Payload: // {"iss":"https://example.com","sub":"test@example.com","aud":"https://example_service1/","exp":2001001001} const std::string kGoodTokenAudService1 = @@ -240,6 +250,11 @@ const std::string kGoodTokenAudService1 = "nzvwse8DINafa5kOhBmQcrIADiOyTVC1IqcOvaftVcS4MTkTeCyzfsqcNQ-VeNPKY" "3e6wTe9brxbii-IPZFNY-1osQNnfCtYpEDjfvMjwHTielF-b55xq_tUwuqaaQ"; +// Payload output for kGoodTokenAudService1. +const std::string kGoodTokenAudService1Payload = + "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"https://example_service1/\"}"; + // Payload: // {"iss":"https://example.com","sub":"test@example.com","aud":"http://example_service2","exp":2001001001} const std::string kGoodTokenAudService2 = @@ -252,11 +267,16 @@ const std::string kGoodTokenAudService2 = "Thwt7f3DTmHOMBeO_3xrLOOZgNtuXipqupkp9sb-DcCRdSokoFpGSTibvV_8RwkQo" "W2fdqw_ZD7WOe4sTcK27Uma9exclisHVxzJJbQOW82WdPQGicYaR_EajYzA"; +// Payload output for kGoodTokenAudService2. +const std::string kGoodTokenAudService2Payload = + "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"http://example_service2\"}"; } // namespace class MockJwtAuthenticatorCallbacks : public JwtAuthenticator::Callbacks { public: MOCK_METHOD1(onDone, void(const Status &status)); + MOCK_METHOD2(savePayload, void(const std::string& key, const std::string& payload)); }; class JwtAuthenticatorTest : public ::testing::Test { @@ -318,13 +338,9 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTandCache) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" - "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" - "aWNlIn0"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); } @@ -349,13 +365,10 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoAlg) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" - "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" - "aWNlIn0"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); @@ -382,13 +395,10 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoKid) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcG" - "xlLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiZXhhbXBsZV9zZXJ2" - "aWNlIn0"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); @@ -408,13 +418,10 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudHasProtocolSchemePayload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" - "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cDovL2V4YW1wbG" - "Vfc2VydmljZS8ifQ"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); @@ -434,13 +441,10 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService1) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudService1Payload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" - "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cHM6Ly9leGFtcG" - "xlX3NlcnZpY2UxLyJ9"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); @@ -460,13 +464,10 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService2) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudService2Payload)); auth_->Verify(headers, &mock_cb); - EXPECT_EQ(headers.get_(kOutputHeadersKey), - "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtcGx" - "lLmNvbSIsImV4cCI6MjAwMTAwMTAwMSwiYXVkIjoiaHR0cDovL2V4YW1wbG" - "Vfc2VydmljZTIifQ"); // Verify the token is removed. EXPECT_FALSE(headers.Authorization()); @@ -489,6 +490,7 @@ TEST_F(JwtAuthenticatorTest, TestForwardJwt) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); @@ -752,6 +754,7 @@ TEST_F(JwtAuthenticatorTest, TestInlineJwks) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); EXPECT_EQ(mock_pubkey.called_count(), 0); diff --git a/src/istio/utils/BUILD b/src/istio/utils/BUILD index a1ec20a2851..ab192312ab1 100644 --- a/src/istio/utils/BUILD +++ b/src/istio/utils/BUILD @@ -77,3 +77,14 @@ cc_library( "//include/istio/utils:attribute_names_header", ], ) + +cc_library( + name = "filter_names_lib", + srcs = [ + "filter_names.cc", + ], + visibility = ["//visibility:public"], + deps = [ + "//include/istio/utils:filter_names_header", + ], +) \ No newline at end of file diff --git a/src/istio/utils/filter_names.cc b/src/istio/utils/filter_names.cc new file mode 100644 index 00000000000..c122946b25a --- /dev/null +++ b/src/istio/utils/filter_names.cc @@ -0,0 +1,26 @@ +/* Copyright 2018 Istio Authors. All Rights Reserved. + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +#include "include/istio/utils/filter_names.h" + +namespace istio { +namespace utils { + +// TODO: using more standard naming, e.g istio.jwt, istio.authn +const char FilterName::kJwt[] = "jwt-auth"; +const char FilterName::kAuthentication[] = "istio_authn"; + +} // namespace utils +} // namespace istio From 465ee984629c770f334a65ec9adeaf80e029d72a Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Thu, 2 Aug 2018 17:48:52 -0700 Subject: [PATCH 2/8] Lint --- include/istio/utils/filter_names.h | 5 ++-- src/envoy/http/authn/authenticator_base.cc | 7 ++++-- .../http/authn/authenticator_base_test.cc | 9 +++---- src/envoy/http/authn/authn_utils.cc | 2 +- src/envoy/http/authn/authn_utils.h | 8 ++++--- src/envoy/http/authn/authn_utils_test.cc | 9 ++++--- src/envoy/http/authn/filter_context.h | 9 ++++--- src/envoy/http/authn/filter_context_test.cc | 4 ++-- src/envoy/http/authn/http_filter.cc | 8 ++++--- src/envoy/http/authn/http_filter_factory.cc | 6 +++-- .../http/authn/origin_authenticator_test.cc | 4 ++-- .../http/authn/peer_authenticator_test.cc | 2 +- src/envoy/http/jwt_auth/http_filter.cc | 7 ++++-- .../http/jwt_auth/http_filter_factory.cc | 2 +- .../http_filter_integration_test.cc | 8 +++---- src/envoy/http/jwt_auth/jwt_authenticator.h | 3 ++- .../http/jwt_auth/jwt_authenticator_test.cc | 24 ++++++++++++------- 17 files changed, 73 insertions(+), 44 deletions(-) diff --git a/include/istio/utils/filter_names.h b/include/istio/utils/filter_names.h index a821325b34f..8f7b54a5e19 100644 --- a/include/istio/utils/filter_names.h +++ b/include/istio/utils/filter_names.h @@ -21,8 +21,9 @@ namespace istio { namespace utils { -// These are name of filters that currently output data to dynamicMetadata (by convention, under the -// the entry using filter name itself as key). Define them here for easy access. +// These are name of filters that currently output data to dynamicMetadata (by +// convention, under the the entry using filter name itself as key). Define them +// here for easy access. struct FilterName { static const char kJwt[]; static const char kAuthentication[]; diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index 0ec6c78b827..da71cb29976 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -75,9 +75,12 @@ bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { return false; } - const auto& value = Envoy::Config::Metadata::metadataValue(filter_context()->request_info().dynamicMetadata(), "jwt-auth", iter->second); + const auto& value = Envoy::Config::Metadata::metadataValue( + filter_context()->request_info().dynamicMetadata(), "jwt-auth", + iter->second); if (!value.string_value().empty()) { - return AuthnUtils::ProcessJwtPayload(value.string_value(), payload->mutable_jwt()); + return AuthnUtils::ProcessJwtPayload(value.string_value(), + payload->mutable_jwt()); } return false; } diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 4bf7c889248..21c0c84ff8f 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -27,8 +27,8 @@ using google::protobuf::util::MessageDifferencer; using istio::authn::Payload; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; using testing::NiceMock; -using testing::StrictMock; using testing::Return; +using testing::StrictMock; namespace iaapi = istio::authentication::v1alpha1; @@ -263,7 +263,7 @@ TEST_F(ValidateJwtTest, HasJwtPayloadOutputButNoDataForKey) { &filter_config_, options); (*request_info_.metadata_.mutable_filter_metadata())["jwt-awuth"].MergeFrom( - MessageUtil::keyValueStruct("foo", "bar")); + MessageUtil::keyValueStruct("foo", "bar")); EXPECT_CALL(request_info_, dynamicMetadata()); // When there is no JWT in the HTTP header, validateJwt() should return @@ -286,7 +286,7 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { &filter_config_, options); (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( - MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", "bad-data")); + MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", "bad-data")); EXPECT_CALL(request_info_, dynamicMetadata()); EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); @@ -307,7 +307,8 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailable) { &filter_config_, options); (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( - MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", kSecIstioAuthUserinfoHeaderValue)); + MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", + kSecIstioAuthUserinfoHeaderValue)); EXPECT_CALL(request_info_, dynamicMetadata()); Payload expected_payload; diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 7cb0b268e82..98a1940f16e 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/src/envoy/http/authn/authn_utils.cc @@ -51,7 +51,7 @@ void ExtractJwtAudience( }; // namespace bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str, - istio::authn::JwtPayload* payload) { + istio::authn::JwtPayload* payload) { Envoy::Json::ObjectSharedPtr json_obj; try { json_obj = Json::Factory::loadFromString(payload_str); diff --git a/src/envoy/http/authn/authn_utils.h b/src/envoy/http/authn/authn_utils.h index ce8801f30c5..023b03e8f10 100644 --- a/src/envoy/http/authn/authn_utils.h +++ b/src/envoy/http/authn/authn_utils.h @@ -28,9 +28,11 @@ namespace AuthN { // AuthnUtils class provides utility functions used for authentication. class AuthnUtils : public Logger::Loggable { public: - // Parse JWT payload string (which typically is the output from jwt filter) and populate JwtPayload object. - // Return true if input string can be parsed successfully. Otherwise, return false. - static bool ProcessJwtPayload(const std::string& jwt_payload_str, istio::authn::JwtPayload* payload); + // Parse JWT payload string (which typically is the output from jwt filter) + // and populate JwtPayload object. Return true if input string can be parsed + // successfully. Otherwise, return false. + static bool ProcessJwtPayload(const std::string& jwt_payload_str, + istio::authn::JwtPayload* payload); }; } // namespace AuthN diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index 4ef0b8fcd10..710a30e5bda 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -84,7 +84,8 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { &expected_payload)); // The payload returned from ProcessJwtPayload() should be the same as // the expected. - bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserinfoHeaderValue, &payload); + bool ret = + AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserinfoHeaderValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } @@ -113,7 +114,8 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithNoAudTest) { // The payload returned from ProcessJwtPayload() should be the same as // the expected. When there is no aud, the aud is not saved in the payload // and claims. - bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserInfoHeaderWithNoAudValue, &payload); + bool ret = AuthnUtils::ProcessJwtPayload( + kSecIstioAuthUserInfoHeaderWithNoAudValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } @@ -145,7 +147,8 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithTwoAudTest) { // The payload returned from ProcessJwtPayload() should be the same as // the expected. When the aud is a string array, the aud is not saved in the // claims. - bool ret = AuthnUtils::ProcessJwtPayload(kSecIstioAuthUserInfoHeaderWithTwoAudValue, &payload); + bool ret = AuthnUtils::ProcessJwtPayload( + kSecIstioAuthUserInfoHeaderWithTwoAudValue, &payload); EXPECT_TRUE(ret); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload)); } diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 9359a24a67a..40aaca1ac00 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -19,8 +19,8 @@ #include "common/common/logger.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" // #include "envoy/http/header_map.h" -#include "envoy/request_info/request_info.h" #include "envoy/network/connection.h" +#include "envoy/request_info/request_info.h" #include "src/istio/authn/context.pb.h" namespace Envoy { @@ -33,7 +33,8 @@ namespace AuthN { class FilterContext : public Logger::Loggable { public: FilterContext( - Envoy::RequestInfo::RequestInfo* request_info, const Network::Connection* connection, + Envoy::RequestInfo::RequestInfo* request_info, + const Network::Connection* connection, const istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig& filter_config) : request_info_(request_info), @@ -58,7 +59,9 @@ class FilterContext : public Logger::Loggable { const istio::authn::Result& authenticationResult() { return result_; } // Accessor to request_info. - const Envoy::RequestInfo::RequestInfo& request_info() const { return *request_info_; } + const Envoy::RequestInfo::RequestInfo& request_info() const { + return *request_info_; + } // Accessor to connection const Network::Connection* connection() { return connection_; } // Accessor to the filter config diff --git a/src/envoy/http/authn/filter_context_test.cc b/src/envoy/http/authn/filter_context_test.cc index 4d54141b5a5..86adb87e25f 100644 --- a/src/envoy/http/authn/filter_context_test.cc +++ b/src/envoy/http/authn/filter_context_test.cc @@ -32,8 +32,8 @@ class FilterContextTest : public testing::Test { public: virtual ~FilterContextTest() {} - // This test suit does not use request_info nor connection, so ok to use null for - // them. + // This test suit does not use request_info nor connection, so ok to use null + // for them. FilterContext filter_context_{nullptr, nullptr, istio::envoy::config::filter::http::authn:: v2alpha1::FilterConfig::default_instance()}; diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index b8cf15e8f82..131f114218c 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -17,11 +17,11 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/http/utility.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" +#include "include/istio/utils/filter_names.h" #include "src/envoy/http/authn/origin_authenticator.h" #include "src/envoy/http/authn/peer_authenticator.h" #include "src/envoy/utils/authn.h" #include "src/envoy/utils/utils.h" -#include "include/istio/utils/filter_names.h" using istio::authn::Payload; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; @@ -48,7 +48,8 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { state_ = State::PROCESSING; filter_context_.reset(new Istio::AuthN::FilterContext( - &decoder_callbacks_->requestInfo(), decoder_callbacks_->connection(), filter_config_)); + &decoder_callbacks_->requestInfo(), decoder_callbacks_->connection(), + filter_config_)); Payload payload; @@ -73,7 +74,8 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); - decoder_callbacks_->requestInfo().setDynamicMetadata(istio::utils::FilterName::kAuthentication, data); + decoder_callbacks_->requestInfo().setDynamicMetadata( + istio::utils::FilterName::kAuthentication, data); ENVOY_LOG(debug, "Saved Dynamic Metadata:\n{}", data.DebugString()); } state_ = State::COMPLETE; diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index bcc5e29c518..45d72ae0da5 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -17,9 +17,9 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" #include "google/protobuf/util/json_util.h" +#include "include/istio/utils/filter_names.h" #include "src/envoy/http/authn/http_filter.h" #include "src/envoy/utils/utils.h" -#include "include/istio/utils/filter_names.h" using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; @@ -61,7 +61,9 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, return ProtobufTypes::MessagePtr{new FilterConfig}; } - std::string name() override { return istio::utils::FilterName::kAuthentication; } + std::string name() override { + return istio::utils::FilterName::kAuthentication; + } private: Http::FilterFactoryCb createFilterFactory(const FilterConfig& config_pb) { diff --git a/src/envoy/http/authn/origin_authenticator_test.cc b/src/envoy/http/authn/origin_authenticator_test.cc index 2e16ceafd6e..5fc1a8a42b7 100644 --- a/src/envoy/http/authn/origin_authenticator_test.cc +++ b/src/envoy/http/authn/origin_authenticator_test.cc @@ -20,8 +20,8 @@ #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" -#include "test/test_common/utility.h" #include "test/mocks/request_info/mocks.h" +#include "test/test_common/utility.h" namespace iaapi = istio::authentication::v1alpha1; @@ -91,7 +91,7 @@ class MockOriginAuthenticator : public OriginAuthenticator { class OriginAuthenticatorTest : public testing::TestWithParam { public: OriginAuthenticatorTest() {} - // : request_headers_{{":method", "GET"}, {":path", "/"}} {} + // : request_headers_{{":method", "GET"}, {":path", "/"}} {} virtual ~OriginAuthenticatorTest() {} void SetUp() override { diff --git a/src/envoy/http/authn/peer_authenticator_test.cc b/src/envoy/http/authn/peer_authenticator_test.cc index 58fab4f9762..bc539c5e2c0 100644 --- a/src/envoy/http/authn/peer_authenticator_test.cc +++ b/src/envoy/http/authn/peer_authenticator_test.cc @@ -20,8 +20,8 @@ #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" -#include "test/test_common/utility.h" #include "test/mocks/request_info/mocks.h" +#include "test/test_common/utility.h" namespace iaapi = istio::authentication::v1alpha1; diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 128aabd0b3c..99cb850bdd1 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -73,8 +73,11 @@ void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { } } -void JwtVerificationFilter::savePayload(const std::string& key, const std::string& payload) { - decoder_callbacks_->requestInfo().setDynamicMetadata(istio::utils::FilterName::kJwt, MessageUtil::keyValueStruct(key, payload)); +void JwtVerificationFilter::savePayload(const std::string& key, + const std::string& payload) { + decoder_callbacks_->requestInfo().setDynamicMetadata( + istio::utils::FilterName::kJwt, + MessageUtil::keyValueStruct(key, payload)); } FilterDataStatus JwtVerificationFilter::decodeData(Buffer::Instance&, bool) { diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 0feaa3cf620..25748cf6be5 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -15,9 +15,9 @@ #include "envoy/registry/registry.h" #include "google/protobuf/util/json_util.h" +#include "include/istio/utils/filter_names.h" #include "src/envoy/http/jwt_auth/auth_store.h" #include "src/envoy/http/jwt_auth/http_filter.h" -#include "include/istio/utils/filter_names.h" using ::istio::envoy::config::filter::http::jwt_auth::v2alpha1:: JwtAuthentication; 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 c32f74a2402..4a4d223faa4 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 @@ -260,8 +260,8 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, RSASuccess1) { "xfP590ACPyXrivtsxg"; auto expected_headers = BaseRequestHeaders(); - // TODO: JWT payload is not longer output to header. Find way to verify that data equivalent to - // this is added to dynamicMetadata. + // TODO: JWT payload is not longer output to header. Find way to verify that + // data equivalent to this is added to dynamicMetadata. // expected_headers.addCopy( // kJwtVerificationResultHeaderKey, // "eyJpc3MiOiJodHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVz" @@ -285,8 +285,8 @@ TEST_P(JwtVerificationFilterIntegrationTestWithJwks, ES256Success1) { "T9ubWvRvNGGYOTuJ8T17Db68Qk3T8UNTK5lzfR_mw"; auto expected_headers = BaseRequestHeaders(); - // TODO: JWT payload is not longer output to header. Find way to verify that data equivalent to - // this is added to dynamicMetadata. + // TODO: JWT payload is not longer output to header. Find way to verify that + // data equivalent to this is added to dynamicMetadata. // expected_headers.addCopy(kJwtVerificationResultHeaderKey, // "eyJpc3MiOiJo" // "dHRwczovL2V4YW1wbGUuY29tIiwic3ViIjoidGVzdEBleGFtc" diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.h b/src/envoy/http/jwt_auth/jwt_authenticator.h index 8d8513d2795..2796cf1ce01 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.h +++ b/src/envoy/http/jwt_auth/jwt_authenticator.h @@ -36,7 +36,8 @@ class JwtAuthenticator : public Logger::Loggable, public: virtual ~Callbacks() {} virtual void onDone(const Status& status) PURE; - virtual void savePayload(const std::string& key, const std::string& payload) PURE; + virtual void savePayload(const std::string& key, + const std::string& payload) PURE; }; void Verify(HeaderMap& headers, Callbacks* callback); diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index 21a878f2dfe..9574e54d2a3 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -218,7 +218,8 @@ const std::string kGoodToken = // Payload output for kGoodToken. const std::string kGoodTokenPayload = - "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "{\"iss\":\"https://" + "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," "\"aud\":\"example_service\"}"; // Payload: @@ -235,7 +236,8 @@ const std::string kGoodTokenAudHasProtocolScheme = // Payload output for kGoodTokenAudHasProtocolScheme. const std::string kGoodTokenAudHasProtocolSchemePayload = - "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "{\"iss\":\"https://" + "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," "\"aud\":\"http://example_service/\"}"; // Payload: @@ -252,7 +254,8 @@ const std::string kGoodTokenAudService1 = // Payload output for kGoodTokenAudService1. const std::string kGoodTokenAudService1Payload = - "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "{\"iss\":\"https://" + "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," "\"aud\":\"https://example_service1/\"}"; // Payload: @@ -269,14 +272,16 @@ const std::string kGoodTokenAudService2 = // Payload output for kGoodTokenAudService2. const std::string kGoodTokenAudService2Payload = - "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "{\"iss\":\"https://" + "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," "\"aud\":\"http://example_service2\"}"; } // namespace class MockJwtAuthenticatorCallbacks : public JwtAuthenticator::Callbacks { public: MOCK_METHOD1(onDone, void(const Status &status)); - MOCK_METHOD2(savePayload, void(const std::string& key, const std::string& payload)); + MOCK_METHOD2(savePayload, + void(const std::string &key, const std::string &payload)); }; class JwtAuthenticatorTest : public ::testing::Test { @@ -418,7 +423,8 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudHasProtocolSchemePayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, + kGoodTokenAudHasProtocolSchemePayload)); auth_->Verify(headers, &mock_cb); @@ -441,7 +447,8 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService1) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudService1Payload)); + EXPECT_CALL(mock_cb, + savePayload(kJwtPayloadOutputKey, kGoodTokenAudService1Payload)); auth_->Verify(headers, &mock_cb); @@ -464,7 +471,8 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService2) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenAudService2Payload)); + EXPECT_CALL(mock_cb, + savePayload(kJwtPayloadOutputKey, kGoodTokenAudService2Payload)); auth_->Verify(headers, &mock_cb); From cddbb664930f8954c3139489882efe40460de64d Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Thu, 2 Aug 2018 20:33:32 -0700 Subject: [PATCH 3/8] Populate authn result to dynamic data only. --- src/envoy/http/authn/filter_context.h | 5 ++++ src/envoy/http/authn/http_filter.cc | 2 +- src/envoy/http/authn/http_filter_test.cc | 33 ++++++++++++++++++++---- src/envoy/utils/BUILD | 1 + src/envoy/utils/authn.cc | 11 ++++++++ src/envoy/utils/authn.h | 5 ++++ 6 files changed, 51 insertions(+), 6 deletions(-) diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 40aaca1ac00..b21542266a9 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -62,6 +62,11 @@ class FilterContext : public Logger::Loggable { const Envoy::RequestInfo::RequestInfo& request_info() const { return *request_info_; } + + Envoy::RequestInfo::RequestInfo* mutable_request_info() { + return request_info_; + } + // Accessor to connection const Network::Connection* connection() { return connection_; } // Accessor to the filter config diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 131f114218c..9cca171ec90 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -74,7 +74,7 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); - decoder_callbacks_->requestInfo().setDynamicMetadata( + filter_context_->mutable_request_info()->setDynamicMetadata( istio::utils::FilterName::kAuthentication, data); ENVOY_LOG(debug, "Saved Dynamic Metadata:\n{}", data.DebugString()); } diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index a71d60aeb56..f47192e8c09 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -16,6 +16,7 @@ #include "src/envoy/http/authn/http_filter.h" #include "common/common/base64.h" #include "common/http/header_map_impl.h" +#include "common/request_info/request_info_impl.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" #include "gmock/gmock.h" #include "gtest/gtest.h" @@ -33,6 +34,7 @@ using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; using testing::Invoke; using testing::NiceMock; using testing::StrictMock; +using testing::ReturnRef; using testing::_; namespace iaapi = istio::authentication::v1alpha1; @@ -116,6 +118,8 @@ TEST_F(AuthenticationFilterTest, PeerFail) { EXPECT_CALL(filter_, createPeerAuthenticator(_)) .Times(1) .WillOnce(Invoke(createAlwaysFailAuthenticator)); + RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); + EXPECT_CALL(decoder_callbacks_, requestInfo()).WillOnce(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -152,13 +156,32 @@ TEST_F(AuthenticationFilterTest, AllPass) { EXPECT_CALL(filter_, createOriginAuthenticator(_)) .Times(1) .WillOnce(Invoke(createAlwaysPassAuthenticator)); + RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); + EXPECT_CALL(decoder_callbacks_, requestInfo()).WillOnce(ReturnRef(request_info)); + EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers_, true)); - Result authn; - EXPECT_TRUE( - Utils::Authentication::FetchResultFromHeader(request_headers_, &authn)); - EXPECT_TRUE(TestUtility::protoEqual( - TestUtilities::AuthNResultFromString(R"(peer_user: "foo")"), authn)); + + EXPECT_EQ(1, request_info.dynamicMetadata().filter_metadata_size()); + const auto* data = Utils::Authentication::GetResultFromRequestInfo(request_info); + ASSERT_TRUE(data); + + ProtobufWkt::Struct expected_data; + ASSERT_TRUE( + Protobuf::TextFormat::ParseFromString(R"( + fields { + key: "source.principal" + value { + string_value: "foo" + } + } + fields { + key: "source.user" + value { + string_value: "foo" + } + })", &expected_data)); + EXPECT_TRUE(TestUtility::protoEqual(expected_data, *data)); } TEST_F(AuthenticationFilterTest, IgnoreBothFail) { diff --git a/src/envoy/utils/BUILD b/src/envoy/utils/BUILD index 4b30761be45..c4f3cd9228e 100644 --- a/src/envoy/utils/BUILD +++ b/src/envoy/utils/BUILD @@ -35,6 +35,7 @@ envoy_cc_library( "//include/istio/utils:attribute_names_header", "//src/istio/authn:context_proto", "//src/istio/utils:attribute_names_lib", + "//src/istio/utils:filter_names_lib", "@envoy//source/exe:envoy_common_lib", ], ) diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index a9b03bd67df..409b5a8dce6 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -17,6 +17,7 @@ #include "common/common/base64.h" #include "include/istio/utils/attribute_names.h" #include "src/istio/authn/context.pb.h" +#include "include/istio/utils/filter_names.h" using istio::authn::Result; @@ -53,6 +54,7 @@ bool Authentication::SaveResultToHeader(const istio::authn::Result& result, void Authentication::SaveAuthAttributesToStruct( const istio::authn::Result& result, ::google::protobuf::Struct& data) { + // TODO(diemvu): Refactor istio::authn::Result this conversion can be removed. if (!result.principal().empty()) { setKeyValue(data, istio::utils::AttributeName::kRequestAuthPrincipal, result.principal()); @@ -103,6 +105,15 @@ bool Authentication::FetchResultFromHeader(const Http::HeaderMap& headers, return result->ParseFromString(Base64::decode(value)); } +const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo(const RequestInfo::RequestInfo& request_info) { + const auto& metadata = request_info.dynamicMetadata(); + const auto& iter = metadata.filter_metadata().find(istio::utils::FilterName::kAuthentication); + if (iter == metadata.filter_metadata().end()) { + return nullptr; + } + return &(iter->second); +} + void Authentication::ClearResultInHeader(Http::HeaderMap* headers) { headers->remove(kAuthenticationOutputHeaderLocation); } diff --git a/src/envoy/utils/authn.h b/src/envoy/utils/authn.h index 395eb88a817..8bbb4e4f09c 100644 --- a/src/envoy/utils/authn.h +++ b/src/envoy/utils/authn.h @@ -17,6 +17,7 @@ #include "envoy/http/header_map.h" #include "google/protobuf/struct.pb.h" #include "src/istio/authn/context.pb.h" +#include "envoy/request_info/request_info.h" namespace Envoy { namespace Utils { @@ -40,6 +41,10 @@ class Authentication : public Logger::Loggable { static bool FetchResultFromHeader(const Http::HeaderMap& headers, istio::authn::Result* result); + // Returns a pointer to the authentication result from request info, if + // available. Otherwise, return nullptrl + static const ProtobufWkt::Struct* GetResultFromRequestInfo(const RequestInfo::RequestInfo& request_info); + // Clears authentication result in header, if exist. static void ClearResultInHeader(Http::HeaderMap* headers); From b823de42ecc88eb6b747de665a4753902c533634 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Fri, 3 Aug 2018 01:03:07 -0700 Subject: [PATCH 4/8] Integration test for authn --- src/envoy/http/authn/BUILD | 6 +- src/envoy/http/authn/authenticator_base.cc | 3 +- src/envoy/http/authn/filter_context.h | 1 - src/envoy/http/authn/http_filter.cc | 3 +- .../authn/http_filter_integration_test.cc | 150 +++++++++++------- 5 files changed, 100 insertions(+), 63 deletions(-) diff --git a/src/envoy/http/authn/BUILD b/src/envoy/http/authn/BUILD index 17e2d3fc6dd..00a524eed4a 100644 --- a/src/envoy/http/authn/BUILD +++ b/src/envoy/http/authn/BUILD @@ -46,6 +46,7 @@ envoy_cc_library( "//src/envoy/http/jwt_auth:jwt_lib", "//src/envoy/utils:utils_lib", "//src/istio/authn:context_proto", + "//src/istio/utils:filter_names_lib", ], ) @@ -161,6 +162,9 @@ envoy_cc_test( repository = "@envoy", deps = [ ":filter_lib", - "@envoy//test/integration:http_integration_lib", + #"//src/envoy/http/jwt_auth:http_filter_factory", + "@envoy//source/common/common:utility_lib", + "@envoy//test/integration:http_protocol_integration_lib", + "//src/istio/utils:filter_names_lib", ], ) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index da71cb29976..e033cf09f01 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -18,6 +18,7 @@ #include "common/config/metadata.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" +#include "include/istio/utils/filter_names.h" using istio::authn::Payload; @@ -76,7 +77,7 @@ bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { } const auto& value = Envoy::Config::Metadata::metadataValue( - filter_context()->request_info().dynamicMetadata(), "jwt-auth", + filter_context()->request_info().dynamicMetadata(), istio::utils::FilterName::kJwt, iter->second); if (!value.string_value().empty()) { return AuthnUtils::ProcessJwtPayload(value.string_value(), diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index b21542266a9..e53dce46379 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -18,7 +18,6 @@ #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 "envoy/request_info/request_info.h" #include "src/istio/authn/context.pb.h" diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 9cca171ec90..d3ff9ef1f50 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -47,6 +47,7 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { filter_config_.DebugString()); state_ = State::PROCESSING; + std::cout << "oooooooooooooooooo " << decoder_callbacks_->requestInfo().dynamicMetadata().DebugString() << "\n"; filter_context_.reset(new Istio::AuthN::FilterContext( &decoder_callbacks_->requestInfo(), decoder_callbacks_->connection(), filter_config_)); @@ -74,7 +75,7 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); - filter_context_->mutable_request_info()->setDynamicMetadata( + decoder_callbacks_->requestInfo().setDynamicMetadata( istio::utils::FilterName::kAuthentication, data); ENVOY_LOG(debug, "Saved Dynamic Metadata:\n{}", data.DebugString()); } diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 80416e135e0..f9a86311982 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -15,7 +15,11 @@ #include "common/common/base64.h" #include "src/istio/authn/context.pb.h" -#include "test/integration/http_integration.h" +#include "test/integration/http_protocol_integration.h" +#include "include/istio/utils/filter_names.h" +#include "fmt/printf.h" +#include "extensions/filters/http/well_known_names.h" +#include "common/common/utility.h" using google::protobuf::util::MessageDifferencer; using istio::authn::Payload; @@ -33,55 +37,33 @@ const std::string kSecIstioAuthUserinfoHeaderValue = "U0MjA1fQ=="; const Envoy::Http::LowerCaseString kSecIstioAuthnPayloadHeaderKey( "sec-istio-authn-payload"); +typedef HttpProtocolIntegrationTest AuthenticationFilterIntegrationTest; -class AuthenticationFilterIntegrationTest - : public HttpIntegrationTest, - public testing::TestWithParam { - public: - AuthenticationFilterIntegrationTest() - : HttpIntegrationTest(Http::CodecClient::Type::HTTP1, GetParam()), - default_request_headers_{ - {":method", "GET"}, {":path", "/"}, {":authority", "host"}}, - request_headers_with_jwt_{{":method", "GET"}, - {":path", "/"}, - {":authority", "host"}, - {kSecIstioAuthUserInfoHeaderKey, - kSecIstioAuthUserinfoHeaderValue}} {} - - void SetUp() override { - fake_upstreams_.emplace_back( - new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); - registerPort("upstream_0", - fake_upstreams_.back()->localAddress()->ip()->port()); - fake_upstreams_.emplace_back( - new FakeUpstream(0, FakeHttpConnection::Type::HTTP1, version_)); - registerPort("upstream_1", - fake_upstreams_.back()->localAddress()->ip()->port()); +const Http::TestHeaderMapImpl kSimpleRequestHeader{ + { + {":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, } - - void TearDown() override { - test_server_.reset(); - fake_upstream_connection_.reset(); - fake_upstreams_.clear(); - } - - protected: - Http::TestHeaderMapImpl default_request_headers_; - Http::TestHeaderMapImpl request_headers_with_jwt_; }; -INSTANTIATE_TEST_CASE_P( - IpVersions, AuthenticationFilterIntegrationTest, - testing::ValuesIn(TestEnvironment::getIpVersionsForTest())); +INSTANTIATE_TEST_CASE_P(Protocols, AuthenticationFilterIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(AuthenticationFilterIntegrationTest, EmptyPolicy) { - createTestServer("src/envoy/http/authn/testdata/envoy_empty.conf", {"http"}); + config_helper_.addFilter("name: istio_authn"); + initialize(); + // setUpServer("src/envoy/http/authn/testdata/envoy_empty.conf"; codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto response = - codec_client_->makeHeaderOnlyRequest(default_request_headers_); + codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Wait for request to upstream[0] (backend) - waitForNextUpstreamRequest(0); + waitForNextUpstreamRequest(); + // Send backend response. upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); @@ -92,15 +74,20 @@ TEST_P(AuthenticationFilterIntegrationTest, EmptyPolicy) { } TEST_P(AuthenticationFilterIntegrationTest, SourceMTlsFail) { - createTestServer("src/envoy/http/authn/testdata/envoy_peer_mtls.conf", - {"http"}); + config_helper_.addFilter(R"( + name: istio_authn + config: + policy: + peers: + - mtls: {})"); + initialize(); // AuthN filter use MTls, but request doesn't have certificate, request // would be rejected. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto response = - codec_client_->makeHeaderOnlyRequest(default_request_headers_); + codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Request is rejected, there will be no upstream request (thus no // waitForNextUpstreamRequest). @@ -112,16 +99,25 @@ TEST_P(AuthenticationFilterIntegrationTest, SourceMTlsFail) { // TODO (diemtvu/lei-tang): add test for MTls success. TEST_P(AuthenticationFilterIntegrationTest, OriginJwtRequiredHeaderNoJwtFail) { - createTestServer( - "src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf", - {"http"}); + config_helper_.addFilter(R"( + name: istio_authn + config: + policy: + origins: + - jwt: + issuer: 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com + jwks_uri: http://localhost:8081/ + jwt_output_payload_locations: [ + 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com: sec-istio-auth-userinfo + ])"); + initialize(); // The AuthN filter requires JWT, but request doesn't have JWT, request // would be rejected. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto response = - codec_client_->makeHeaderOnlyRequest(default_request_headers_); + codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Request is rejected, there will be no upstream request (thus no // waitForNextUpstreamRequest). @@ -130,20 +126,58 @@ TEST_P(AuthenticationFilterIntegrationTest, OriginJwtRequiredHeaderNoJwtFail) { EXPECT_STREQ("401", response->headers().Status()->value().c_str()); } +std::string MakeHeaderToMetadataConfig() { + const std::string payload = + "{\"iss\":\"https://" + "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"example_service\"}"; +/* + const char payload[] = R"({ + "iss": "https://example.com", + "sub": "test@example.com", + "exp": 2001001001, + "aud": "example_service" + })"; +*/ + //ProtobufWkt::Struct pfc = MessageUtil::keyValueStruct("sec-istio-auth-userinfo", payload); + return fmt::sprintf(R"( + name: %s + config: + request_rules: + - header: x-sec-istio-auth-userinfo + on_header_missing: + metadata_namespace: %s + key: sec-istio-auth-userinfo + value: "%s" + type: STRING + )", Extensions::HttpFilters::HttpFilterNames::get().HeaderToMetadata, istio::utils::FilterName::kJwt, StringUtil::escape(payload)); +} TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { - createTestServer( - "src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf", - {"http"}); + config_helper_.addFilter(R"( + name: istio_authn + config: + jwt_output_payload_locations: { + 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com: sec-istio-auth-userinfo + } + policy: + origins: + - jwt: + issuer: 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com + jwks_uri: http://localhost:8081/ + )"); + config_helper_.addFilter(MakeHeaderToMetadataConfig()); + initialize(); // The AuthN filter requires JWT. The http request contains validated JWT and // the authentication should succeed. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); auto response = - codec_client_->makeHeaderOnlyRequest(request_headers_with_jwt_); + codec_client_->makeRequestWithBody(kSimpleRequestHeader, 1024); + // codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Wait for request to upstream[0] (backend) - waitForNextUpstreamRequest(0); + waitForNextUpstreamRequest(); // Send backend response. upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); @@ -153,6 +187,7 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { EXPECT_STREQ("200", response->headers().Status()->value().c_str()); } +/* TEST_P(AuthenticationFilterIntegrationTest, CheckConsumedJwtHeadersAreRemoved) { const Envoy::Http::LowerCaseString header_location( "location-to-read-jwt-result"); @@ -175,10 +210,9 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckConsumedJwtHeadersAreRemoved) { {"location-to-read-jwt-result", jwt_header_base64}}; // In this config, the JWT verification result for "issuer@foo.com" is in the // header "location-to-read-jwt-result" - createTestServer( + setUpServer( "src/envoy/http/authn/testdata/" - "envoy_jwt_with_output_header_location.conf", - {"http"}); + "envoy_jwt_with_output_header_location.conf"); // The AuthN filter requires JWT and the http request contains validated JWT. // In this case, the authentication should succeed and an authn result // should be generated. @@ -197,9 +231,7 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckConsumedJwtHeadersAreRemoved) { } TEST_P(AuthenticationFilterIntegrationTest, CheckAuthnResultIsExpected) { - createTestServer( - "src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf", - {"http"}); + setUpServer("src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf"); // The AuthN filter requires JWT and the http request contains validated JWT. // In this case, the authentication should succeed and an authn result @@ -247,6 +279,6 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckAuthnResultIsExpected) { // is non-deterministic. Thus, MessageDifferencer::Equals() is used. EXPECT_TRUE(MessageDifferencer::Equals(expected_result, result)); } - +*/ } // namespace } // namespace Envoy From abadf5bccc9c548852a29b5b5915e7a741fff641 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Fri, 3 Aug 2018 12:06:48 -0700 Subject: [PATCH 5/8] Clean up and verify all tests --- src/envoy/http/authn/BUILD | 4 +- src/envoy/http/authn/authenticator_base.cc | 21 +- .../http/authn/authenticator_base_test.cc | 90 ++---- src/envoy/http/authn/filter_context.cc | 31 +++ src/envoy/http/authn/filter_context.h | 31 +-- src/envoy/http/authn/filter_context_test.cc | 7 +- src/envoy/http/authn/http_filter.cc | 12 +- .../authn/http_filter_integration_test.cc | 263 ++++++------------ src/envoy/http/authn/http_filter_test.cc | 17 +- .../http/authn/origin_authenticator_test.cc | 12 +- .../http/authn/peer_authenticator_test.cc | 10 +- src/envoy/http/jwt_auth/jwt_authenticator.cc | 13 +- .../http/jwt_auth/jwt_authenticator_test.cc | 30 +- src/envoy/utils/authn.cc | 8 +- src/envoy/utils/authn.h | 5 +- 15 files changed, 230 insertions(+), 324 deletions(-) diff --git a/src/envoy/http/authn/BUILD b/src/envoy/http/authn/BUILD index 00a524eed4a..06cd2378512 100644 --- a/src/envoy/http/authn/BUILD +++ b/src/envoy/http/authn/BUILD @@ -100,7 +100,8 @@ envoy_cc_test( ":authenticator", ":test_utils", "@envoy//test/mocks/network:network_mocks", - "@envoy//test/mocks/request_info:request_info_mocks", + # "@envoy//test/mocks/request_info:request_info_mocks", + "//src/istio/utils:filter_names_lib", "@envoy//test/mocks/ssl:ssl_mocks", "@envoy//test/test_common:utility_lib", ], @@ -162,7 +163,6 @@ envoy_cc_test( repository = "@envoy", deps = [ ":filter_lib", - #"//src/envoy/http/jwt_auth:http_filter_factory", "@envoy//source/common/common:utility_lib", "@envoy//test/integration:http_protocol_integration_lib", "//src/istio/utils:filter_names_lib", diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index e033cf09f01..e3922449187 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -16,9 +16,9 @@ #include "src/envoy/http/authn/authenticator_base.h" #include "common/common/assert.h" #include "common/config/metadata.h" +#include "include/istio/utils/filter_names.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" -#include "include/istio/utils/filter_names.h" using istio::authn::Payload; @@ -66,22 +66,9 @@ bool AuthenticatorBase::validateX509(const iaapi::MutualTls& mtls, } bool AuthenticatorBase::validateJwt(const iaapi::Jwt& jwt, Payload* payload) { - auto iter = - filter_context()->filter_config().jwt_output_payload_locations().find( - jwt.issuer()); - if (iter == - filter_context()->filter_config().jwt_output_payload_locations().end()) { - ENVOY_LOG(warn, "No JWT payload header location is found for the issuer {}", - jwt.issuer()); - return false; - } - - const auto& value = Envoy::Config::Metadata::metadataValue( - filter_context()->request_info().dynamicMetadata(), istio::utils::FilterName::kJwt, - iter->second); - if (!value.string_value().empty()) { - return AuthnUtils::ProcessJwtPayload(value.string_value(), - payload->mutable_jwt()); + std::string jwt_payload; + if (filter_context()->getJwtPayload(jwt.issuer(), &jwt_payload)) { + return AuthnUtils::ProcessJwtPayload(jwt_payload, payload->mutable_jwt()); } return false; } diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 21c0c84ff8f..7ac99079155 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -20,7 +20,9 @@ #include "gmock/gmock.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/network/mocks.h" -#include "test/mocks/request_info/mocks.h" +//#include "test/mocks/request_info/mocks.h" +#include "envoy/api/v2/core/base.pb.h" +#include "include/istio/utils/filter_names.h" #include "test/mocks/ssl/mocks.h" using google::protobuf::util::MessageDifferencer; @@ -38,7 +40,6 @@ namespace Istio { namespace AuthN { namespace { -const std::string kSecIstioAuthUserInfoHeaderKey = "sec-istio-auth-userinfo"; const std::string kSecIstioAuthUserinfoHeaderValue = R"( { @@ -62,13 +63,13 @@ class ValidateX509Test : public testing::TestWithParam, public: virtual ~ValidateX509Test() {} - NiceMock request_info_{}; NiceMock connection_{}; NiceMock ssl_{}; FilterConfig filter_config_{}; - FilterContext filter_context_{&request_info_, &connection_, - istio::envoy::config::filter::http::authn:: - v2alpha1::FilterConfig::default_instance()}; + FilterContext filter_context_{ + envoy::api::v2::core::Metadata::default_instance(), &connection_, + istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig:: + default_instance()}; MockAuthenticatorBase authenticator_{&filter_context_}; @@ -164,11 +165,13 @@ class ValidateJwtTest : public testing::Test, public: virtual ~ValidateJwtTest() {} - StrictMock request_info_{}; + // StrictMock request_info_{}; + envoy::api::v2::core::Metadata dynamic_metadata_; NiceMock connection_{}; // NiceMock ssl_{}; FilterConfig filter_config_{}; - FilterContext filter_context_{&request_info_, &connection_, filter_config_}; + FilterContext filter_context_{dynamic_metadata_, &connection_, + filter_config_}; MockAuthenticatorBase authenticator_{&filter_context_}; void SetUp() override { payload_ = new Payload(); } @@ -231,63 +234,30 @@ TEST_F(ValidateJwtTest, OutputPayloadLocationNotDefine) { TEST_F(ValidateJwtTest, NoJwtPayloadOutput) { jwt_.set_issuer("issuer@foo.com"); - google::protobuf::util::JsonParseOptions options; - JsonStringToMessage( - R"({ - "jwt_output_payload_locations": - { - "issuer@foo.com": "sec-istio-auth-jwt-output" - } - } - )", - &filter_config_, options); - EXPECT_CALL(request_info_, dynamicMetadata()); - // When there is no JWT in the HTTP header, validateJwt() should return - // nullptr and failure. + // When there is no JWT in request info dynamic metadata, validateJwt() should + // return nullptr and failure. EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } TEST_F(ValidateJwtTest, HasJwtPayloadOutputButNoDataForKey) { jwt_.set_issuer("issuer@foo.com"); - google::protobuf::util::JsonParseOptions options; - JsonStringToMessage( - R"({ - "jwt_output_payload_locations": - { - "issuer@foo.com": "sec-istio-auth-jwt-output" - } - } - )", - &filter_config_, options); - (*request_info_.metadata_.mutable_filter_metadata())["jwt-awuth"].MergeFrom( - MessageUtil::keyValueStruct("foo", "bar")); - EXPECT_CALL(request_info_, dynamicMetadata()); + (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + .MergeFrom(MessageUtil::keyValueStruct("foo", "bar")); - // When there is no JWT in the HTTP header, validateJwt() should return - // nullptr and failure. + // When there is no JWT payload for given issuer in request info dynamic + // metadata, validateJwt() should return nullptr and failure. EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equals(*payload_, default_payload_)); } TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { jwt_.set_issuer("issuer@foo.com"); - google::protobuf::util::JsonParseOptions options; - JsonStringToMessage( - R"({ - "jwt_output_payload_locations": - { - "issuer@foo.com": "sec-istio-auth-jwt-output" - } - } - )", - &filter_config_, options); - - (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( - MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", "bad-data")); - EXPECT_CALL(request_info_, dynamicMetadata()); + (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + .MergeFrom(MessageUtil::keyValueStruct("issuer@foo.com", "bad-data")); + // EXPECT_CALL(request_info_, dynamicMetadata()); EXPECT_FALSE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equivalent(*payload_, default_payload_)); @@ -295,21 +265,9 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { TEST_F(ValidateJwtTest, JwtPayloadAvailable) { jwt_.set_issuer("issuer@foo.com"); - google::protobuf::util::JsonParseOptions options; - JsonStringToMessage( - R"({ - "jwt_output_payload_locations": - { - "issuer@foo.com": "sec-istio-auth-jwt-output" - } - } - )", - &filter_config_, options); - - (*request_info_.metadata_.mutable_filter_metadata())["jwt-auth"].MergeFrom( - MessageUtil::keyValueStruct("sec-istio-auth-jwt-output", - kSecIstioAuthUserinfoHeaderValue)); - EXPECT_CALL(request_info_, dynamicMetadata()); + (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + .MergeFrom(MessageUtil::keyValueStruct("issuer@foo.com", + kSecIstioAuthUserinfoHeaderValue)); Payload expected_payload; JsonStringToMessage( @@ -328,7 +286,7 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailable) { } } )", - &expected_payload, options); + &expected_payload, google::protobuf::util::JsonParseOptions{}); EXPECT_TRUE(authenticator_.validateJwt(jwt_, payload_)); EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, *payload_)); diff --git a/src/envoy/http/authn/filter_context.cc b/src/envoy/http/authn/filter_context.cc index 474bd00f469..e5e7f7e5e60 100644 --- a/src/envoy/http/authn/filter_context.cc +++ b/src/envoy/http/authn/filter_context.cc @@ -14,6 +14,7 @@ */ #include "src/envoy/http/authn/filter_context.h" +#include "include/istio/utils/filter_names.h" #include "src/envoy/utils/utils.h" using istio::authn::Payload; @@ -70,6 +71,36 @@ void FilterContext::setPrincipal(const iaapi::PrincipalBinding& binding) { return; } } + +bool FilterContext::getJwtPayload(const std::string& issuer, + std::string* payload) const { + // auto location_iter = + // filter_config_.jwt_output_payload_locations().find(issuer); + // if (location_iter == filter_config_.jwt_output_payload_locations().end()) { + // ENVOY_LOG(warn, "No JWT payload output location is found for the issuer + // {}", issuer); return false; + // } + const auto filter_it = + dynamic_metadata_.filter_metadata().find(istio::utils::FilterName::kJwt); + if (filter_it == dynamic_metadata_.filter_metadata().end()) { + ENVOY_LOG(debug, "No dynamic_metadata found for filter {}", + istio::utils::FilterName::kJwt); + return false; + } + + const auto& data_struct = filter_it->second; + const auto entry_it = data_struct.fields().find(issuer); + if (entry_it == data_struct.fields().end()) { + return false; + } + if (entry_it->second.string_value().empty()) { + return false; + } + + *payload = entry_it->second.string_value(); + return true; +} + } // namespace AuthN } // namespace Istio } // namespace Http diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index e53dce46379..3265783eb3e 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -17,9 +17,9 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/common/logger.h" +#include "envoy/api/v2/core/base.pb.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" #include "envoy/network/connection.h" -#include "envoy/request_info/request_info.h" #include "src/istio/authn/context.pb.h" namespace Envoy { @@ -27,16 +27,16 @@ namespace Http { namespace Istio { namespace AuthN { -// FilterContext holds inputs, such as request header and connection and -// result data for authentication process. +// FilterContext holds inputs, such as request dynamic metadata and connection +// and result data for authentication process. class FilterContext : public Logger::Loggable { public: FilterContext( - Envoy::RequestInfo::RequestInfo* request_info, + const envoy::api::v2::core::Metadata& dynamic_metadata, const Network::Connection* connection, const istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig& filter_config) - : request_info_(request_info), + : dynamic_metadata_(dynamic_metadata), connection_(connection), filter_config_(filter_config) {} virtual ~FilterContext() {} @@ -57,15 +57,10 @@ class FilterContext : public Logger::Loggable { // Returns the authentication result. const istio::authn::Result& authenticationResult() { return result_; } - // Accessor to request_info. - const Envoy::RequestInfo::RequestInfo& request_info() const { - return *request_info_; + // Acessor to dynamic metadata. + const envoy::api::v2::core::Metadata& dynamic_metadata() const { + return dynamic_metadata_; } - - Envoy::RequestInfo::RequestInfo* mutable_request_info() { - return request_info_; - } - // Accessor to connection const Network::Connection* connection() { return connection_; } // Accessor to the filter config @@ -74,11 +69,15 @@ class FilterContext : public Logger::Loggable { return filter_config_; } - bool getJwtPayload(const std::string& key, std::string* payload); + // Gets JWT payload (output from JWT filter) for given issuer. If non-empty + // payload found, returns true and set the output payload string. Otherwise, + // returns false. + bool getJwtPayload(const std::string& issuer, std::string* payload) const; private: - // Pointer to the request info. - Envoy::RequestInfo::RequestInfo* request_info_; + // Const reference to request info dynamic metadata. This provides data that + // output from other filters, e.g JWT. + const envoy::api::v2::core::Metadata& dynamic_metadata_; // Pointer to network connection of the request. const Network::Connection* connection_; diff --git a/src/envoy/http/authn/filter_context_test.cc b/src/envoy/http/authn/filter_context_test.cc index 86adb87e25f..6fb89866e15 100644 --- a/src/envoy/http/authn/filter_context_test.cc +++ b/src/envoy/http/authn/filter_context_test.cc @@ -14,6 +14,7 @@ */ #include "src/envoy/http/authn/filter_context.h" +#include "envoy/api/v2/core/base.pb.h" #include "src/envoy/http/authn/test_utils.h" #include "test/test_common/utility.h" @@ -32,9 +33,9 @@ class FilterContextTest : public testing::Test { public: virtual ~FilterContextTest() {} - // This test suit does not use request_info nor connection, so ok to use null - // for them. - FilterContext filter_context_{nullptr, nullptr, + envoy::api::v2::core::Metadata metadata_; + // This test suit does not use connection, so ok to use null for it. + FilterContext filter_context_{metadata_, nullptr, istio::envoy::config::filter::http::authn:: v2alpha1::FilterConfig::default_instance()}; diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index d3ff9ef1f50..0b369832a27 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -42,15 +42,15 @@ void AuthenticationFilter::onDestroy() { ENVOY_LOG(debug, "Called AuthenticationFilter : {}", __func__); } -FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { +FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, + bool) { ENVOY_LOG(debug, "AuthenticationFilter::decodeHeaders with config\n{}", filter_config_.DebugString()); state_ = State::PROCESSING; - std::cout << "oooooooooooooooooo " << decoder_callbacks_->requestInfo().dynamicMetadata().DebugString() << "\n"; filter_context_.reset(new Istio::AuthN::FilterContext( - &decoder_callbacks_->requestInfo(), decoder_callbacks_->connection(), - filter_config_)); + decoder_callbacks_->requestInfo().dynamicMetadata(), + decoder_callbacks_->connection(), filter_config_)); Payload payload; @@ -71,6 +71,10 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { // Put authentication result to headers. if (filter_context_ != nullptr) { + // TODO(diemvu): Remove the header and only use the metadata to pass the + // attributes. + Utils::Authentication::SaveResultToHeader( + filter_context_->authenticationResult(), &headers); // Save auth results in the metadata, could be later used by RBAC filter. ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index f9a86311982..3b6f4753b6d 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -14,12 +14,12 @@ */ #include "common/common/base64.h" +#include "common/common/utility.h" +#include "extensions/filters/http/well_known_names.h" +#include "fmt/printf.h" +#include "include/istio/utils/filter_names.h" #include "src/istio/authn/context.pb.h" #include "test/integration/http_protocol_integration.h" -#include "include/istio/utils/filter_names.h" -#include "fmt/printf.h" -#include "extensions/filters/http/well_known_names.h" -#include "common/common/utility.h" using google::protobuf::util::MessageDifferencer; using istio::authn::Payload; @@ -27,41 +27,71 @@ using istio::authn::Result; namespace Envoy { namespace { -const std::string kSecIstioAuthUserInfoHeaderKey = "sec-istio-auth-userinfo"; -const std::string kSecIstioAuthUserinfoHeaderValue = - "eyJpc3MiOiI2Mjg2NDU3NDE4ODEtbm9hYml1MjNmNWE4bThvdmQ4dWN2Njk4bGo3OH" - "Z2MGxAZGV2ZWxvcGVyLmdzZXJ2aWNlYWNjb3VudC5jb20iLCJzdWIiOiI2Mjg2NDU3" - "NDE4ODEtbm9hYml1MjNmNWE4bThvdmQ4dWN2Njk4bGo3OHZ2MGxAZGV2ZWxvcGVyLm" - "dzZXJ2aWNlYWNjb3VudC5jb20iLCJhdWQiOiJib29rc3RvcmUtZXNwLWVjaG8uY2xv" - "dWRlbmRwb2ludHNhcGlzLmNvbSIsImlhdCI6MTUxMjc1NDIwNSwiZXhwIjo1MTEyNz" - "U0MjA1fQ=="; -const Envoy::Http::LowerCaseString kSecIstioAuthnPayloadHeaderKey( + +static const Envoy::Http::LowerCaseString kSecIstioAuthnPayloadHeaderKey( "sec-istio-authn-payload"); -typedef HttpProtocolIntegrationTest AuthenticationFilterIntegrationTest; -const Http::TestHeaderMapImpl kSimpleRequestHeader{ - { - {":method", "GET"}, - {":path", "/"}, - {":scheme", "http"}, - {":authority", "host"}, - {"x-forwarded-for", "10.0.0.1"}, - } -}; +// Default request for testing. +static const Http::TestHeaderMapImpl kSimpleRequestHeader{{ + {":method", "GET"}, + {":path", "/"}, + {":scheme", "http"}, + {":authority", "host"}, + {"x-forwarded-for", "10.0.0.1"}, +}}; -INSTANTIATE_TEST_CASE_P(Protocols, AuthenticationFilterIntegrationTest, - testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), - HttpProtocolIntegrationTest::protocolTestParamsToString); +// Keep the same as issuer in the policy below. +static const char kJwtIssuer[] = "some@issuer"; + +static const char kAuthnFilterWithJwt[] = R"( + name: istio_authn + config: + policy: + origins: + - jwt: + issuer: some@issuer + jwks_uri: http://localhost:8081/)"; + +// Payload data to inject. Note the iss claim intentionally set different from +// kJwtIssuer. +static const char kMockJwtPayload[] = + "{\"iss\":\"https://example.com\"," + "\"sub\":\"test@example.com\",\"exp\":2001001001," + "\"aud\":\"example_service\"}"; +// Returns a simple header-to-metadata filter config that can be used to inject +// data into request info dynamic metadata for testing. +std::string MakeHeaderToMetadataConfig() { + return fmt::sprintf( + R"( + name: %s + config: + request_rules: + - header: x-mock-metadata-injection + on_header_missing: + metadata_namespace: %s + key: %s + value: "%s" + type: STRING + )", + Extensions::HttpFilters::HttpFilterNames::get().HeaderToMetadata, + istio::utils::FilterName::kJwt, kJwtIssuer, + StringUtil::escape(kMockJwtPayload)); +} + +typedef HttpProtocolIntegrationTest AuthenticationFilterIntegrationTest; + +INSTANTIATE_TEST_CASE_P( + Protocols, AuthenticationFilterIntegrationTest, + testing::ValuesIn(HttpProtocolIntegrationTest::getProtocolTestParams()), + HttpProtocolIntegrationTest::protocolTestParamsToString); TEST_P(AuthenticationFilterIntegrationTest, EmptyPolicy) { config_helper_.addFilter("name: istio_authn"); initialize(); - // setUpServer("src/envoy/http/authn/testdata/envoy_empty.conf"; codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = - codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); - // Wait for request to upstream[0] (backend) + auto response = codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); + // Wait for request to upstream (backend) waitForNextUpstreamRequest(); // Send backend response. @@ -86,8 +116,7 @@ TEST_P(AuthenticationFilterIntegrationTest, SourceMTlsFail) { // would be rejected. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = - codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); + auto response = codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Request is rejected, there will be no upstream request (thus no // waitForNextUpstreamRequest). @@ -99,25 +128,14 @@ TEST_P(AuthenticationFilterIntegrationTest, SourceMTlsFail) { // TODO (diemtvu/lei-tang): add test for MTls success. TEST_P(AuthenticationFilterIntegrationTest, OriginJwtRequiredHeaderNoJwtFail) { - config_helper_.addFilter(R"( - name: istio_authn - config: - policy: - origins: - - jwt: - issuer: 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com - jwks_uri: http://localhost:8081/ - jwt_output_payload_locations: [ - 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com: sec-istio-auth-userinfo - ])"); + config_helper_.addFilter(kAuthnFilterWithJwt); initialize(); // The AuthN filter requires JWT, but request doesn't have JWT, request // would be rejected. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = - codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); + auto response = codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); // Request is rejected, there will be no upstream request (thus no // waitForNextUpstreamRequest). @@ -126,45 +144,8 @@ TEST_P(AuthenticationFilterIntegrationTest, OriginJwtRequiredHeaderNoJwtFail) { EXPECT_STREQ("401", response->headers().Status()->value().c_str()); } -std::string MakeHeaderToMetadataConfig() { - const std::string payload = - "{\"iss\":\"https://" - "example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001," - "\"aud\":\"example_service\"}"; -/* - const char payload[] = R"({ - "iss": "https://example.com", - "sub": "test@example.com", - "exp": 2001001001, - "aud": "example_service" - })"; -*/ - //ProtobufWkt::Struct pfc = MessageUtil::keyValueStruct("sec-istio-auth-userinfo", payload); - return fmt::sprintf(R"( - name: %s - config: - request_rules: - - header: x-sec-istio-auth-userinfo - on_header_missing: - metadata_namespace: %s - key: sec-istio-auth-userinfo - value: "%s" - type: STRING - )", Extensions::HttpFilters::HttpFilterNames::get().HeaderToMetadata, istio::utils::FilterName::kJwt, StringUtil::escape(payload)); -} TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { - config_helper_.addFilter(R"( - name: istio_authn - config: - jwt_output_payload_locations: { - 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com: sec-istio-auth-userinfo - } - policy: - origins: - - jwt: - issuer: 628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com - jwks_uri: http://localhost:8081/ - )"); + config_helper_.addFilter(kAuthnFilterWithJwt); config_helper_.addFilter(MakeHeaderToMetadataConfig()); initialize(); @@ -172,12 +153,10 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { // the authentication should succeed. codec_client_ = makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = - codec_client_->makeRequestWithBody(kSimpleRequestHeader, 1024); - // codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); + auto response = codec_client_->makeHeaderOnlyRequest(kSimpleRequestHeader); - // Wait for request to upstream[0] (backend) - waitForNextUpstreamRequest(); + // Wait for request to upstream (backend) + waitForNextUpstreamRequest(); // Send backend response. upstream_request_->encodeHeaders(Http::TestHeaderMapImpl{{":status", "200"}}, true); @@ -185,100 +164,42 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); -} - -/* -TEST_P(AuthenticationFilterIntegrationTest, CheckConsumedJwtHeadersAreRemoved) { - const Envoy::Http::LowerCaseString header_location( - "location-to-read-jwt-result"); - const std::string jwt_header = - R"( - { - "iss": "issuer@foo.com", - "sub": "sub@foo.com", - "aud": "aud1", - "non-string-will-be-ignored": 1512754205, - "some-other-string-claims": "some-claims-kept" - } - )"; - std::string jwt_header_base64 = - Base64::encode(jwt_header.c_str(), jwt_header.size()); - Http::TestHeaderMapImpl request_headers_with_jwt_at_specified_location{ - {":method", "GET"}, - {":path", "/"}, - {":authority", "host"}, - {"location-to-read-jwt-result", jwt_header_base64}}; - // In this config, the JWT verification result for "issuer@foo.com" is in the - // header "location-to-read-jwt-result" - setUpServer( - "src/envoy/http/authn/testdata/" - "envoy_jwt_with_output_header_location.conf"); - // The AuthN filter requires JWT and the http request contains validated JWT. - // In this case, the authentication should succeed and an authn result - // should be generated. - codec_client_ = - makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = codec_client_->makeHeaderOnlyRequest( - request_headers_with_jwt_at_specified_location); - - // Wait for request to upstream[0] (backend) - waitForNextUpstreamRequest(0); - response->waitForEndStream(); - - // After Istio authn, the JWT headers consumed by Istio authn should have - // been removed. - EXPECT_TRUE(nullptr == upstream_request_->headers().get(header_location)); -} - -TEST_P(AuthenticationFilterIntegrationTest, CheckAuthnResultIsExpected) { - setUpServer("src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf"); - - // The AuthN filter requires JWT and the http request contains validated JWT. - // In this case, the authentication should succeed and an authn result - // should be generated. - codec_client_ = - makeHttpConnection(makeClientConnection((lookupPort("http")))); - auto response = - codec_client_->makeHeaderOnlyRequest(request_headers_with_jwt_); - // Wait for request to upstream[0] (backend) - waitForNextUpstreamRequest(0); - response->waitForEndStream(); - - // Authn result should be as expected + // Verify authn result in header. + // TODO(diemvu): find a way to verify data in request info as the use of + // header for authentication result will be removed soon. const Envoy::Http::HeaderString &header_value = upstream_request_->headers().get(kSecIstioAuthnPayloadHeaderKey)->value(); std::string value_base64(header_value.c_str(), header_value.size()); const std::string value = Base64::decode(value_base64); Result result; - google::protobuf::util::JsonParseOptions options; - Result expected_result; + EXPECT_TRUE(result.ParseFromString(value)); - bool parse_ret = result.ParseFromString(value); - EXPECT_TRUE(parse_ret); - JsonStringToMessage( - R"( - { - "origin": { - "user": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com/628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com", - "audiences": [ - "bookstore-esp-echo.cloudendpointsapis.com" - ], - "presenter": "", - "claims": { - "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}" + Result expected_result; + ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(R"( + origin { + user: "https://example.com/test@example.com" + audiences: "example_service" + claims { + key: "aud" + value: "example_service" } - } - )", - &expected_result, options); + claims { + key: "iss" + value: "https://example.com" + } + claims { + key: "sub" + value: "test@example.com" + } + raw_claims: "{\"iss\":\"https://example.com\",\"sub\":\"test@example.com\",\"exp\":2001001001,\"aud\":\"example_service\"}" + })", + &expected_result)); + // Note: TestUtility::protoEqual() uses SerializeAsString() and the output // is non-deterministic. Thus, MessageDifferencer::Equals() is used. EXPECT_TRUE(MessageDifferencer::Equals(expected_result, result)); } -*/ + } // namespace } // namespace Envoy diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index f47192e8c09..eab12be2dc7 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -33,8 +33,8 @@ using istio::authn::Result; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; using testing::Invoke; using testing::NiceMock; -using testing::StrictMock; using testing::ReturnRef; +using testing::StrictMock; using testing::_; namespace iaapi = istio::authentication::v1alpha1; @@ -119,7 +119,8 @@ TEST_F(AuthenticationFilterTest, PeerFail) { .Times(1) .WillOnce(Invoke(createAlwaysFailAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); - EXPECT_CALL(decoder_callbacks_, requestInfo()).WillOnce(ReturnRef(request_info)); + EXPECT_CALL(decoder_callbacks_, requestInfo()) + .WillRepeatedly(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -157,18 +158,19 @@ TEST_F(AuthenticationFilterTest, AllPass) { .Times(1) .WillOnce(Invoke(createAlwaysPassAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); - EXPECT_CALL(decoder_callbacks_, requestInfo()).WillOnce(ReturnRef(request_info)); + EXPECT_CALL(decoder_callbacks_, requestInfo()) + .WillRepeatedly(ReturnRef(request_info)); EXPECT_EQ(Http::FilterHeadersStatus::Continue, filter_.decodeHeaders(request_headers_, true)); EXPECT_EQ(1, request_info.dynamicMetadata().filter_metadata_size()); - const auto* data = Utils::Authentication::GetResultFromRequestInfo(request_info); + const auto *data = + Utils::Authentication::GetResultFromRequestInfo(request_info); ASSERT_TRUE(data); ProtobufWkt::Struct expected_data; - ASSERT_TRUE( - Protobuf::TextFormat::ParseFromString(R"( + ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(R"( fields { key: "source.principal" value { @@ -180,7 +182,8 @@ TEST_F(AuthenticationFilterTest, AllPass) { value { string_value: "foo" } - })", &expected_data)); + })", + &expected_data)); EXPECT_TRUE(TestUtility::protoEqual(expected_data, *data)); } diff --git a/src/envoy/http/authn/origin_authenticator_test.cc b/src/envoy/http/authn/origin_authenticator_test.cc index 5fc1a8a42b7..0a4211692f0 100644 --- a/src/envoy/http/authn/origin_authenticator_test.cc +++ b/src/envoy/http/authn/origin_authenticator_test.cc @@ -16,11 +16,11 @@ #include "src/envoy/http/authn/origin_authenticator.h" #include "authentication/v1alpha1/policy.pb.h" #include "common/protobuf/protobuf.h" +#include "envoy/api/v2/core/base.pb.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" -#include "test/mocks/request_info/mocks.h" #include "test/test_common/utility.h" namespace iaapi = istio::authentication::v1alpha1; @@ -91,7 +91,6 @@ class MockOriginAuthenticator : public OriginAuthenticator { class OriginAuthenticatorTest : public testing::TestWithParam { public: OriginAuthenticatorTest() {} - // : request_headers_{{":method", "GET"}, {":path", "/"}} {} virtual ~OriginAuthenticatorTest() {} void SetUp() override { @@ -121,10 +120,11 @@ class OriginAuthenticatorTest : public testing::TestWithParam { protected: std::unique_ptr> authenticator_; - StrictMock request_info_{}; - FilterContext filter_context_{&request_info_, nullptr, - istio::envoy::config::filter::http::authn:: - v2alpha1::FilterConfig::default_instance()}; + // envoy::api::v2::core::Metadata metadata_; + FilterContext filter_context_{ + envoy::api::v2::core::Metadata::default_instance(), nullptr, + istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig:: + default_instance()}; iaapi::Policy policy_; Payload* payload_; diff --git a/src/envoy/http/authn/peer_authenticator_test.cc b/src/envoy/http/authn/peer_authenticator_test.cc index bc539c5e2c0..60f04a827ec 100644 --- a/src/envoy/http/authn/peer_authenticator_test.cc +++ b/src/envoy/http/authn/peer_authenticator_test.cc @@ -16,11 +16,11 @@ #include "src/envoy/http/authn/peer_authenticator.h" #include "authentication/v1alpha1/policy.pb.h" #include "common/protobuf/protobuf.h" +#include "envoy/api/v2/core/base.pb.h" #include "gmock/gmock.h" #include "gtest/gtest.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/http/mocks.h" -#include "test/mocks/request_info/mocks.h" #include "test/test_common/utility.h" namespace iaapi = istio::authentication::v1alpha1; @@ -66,10 +66,10 @@ class PeerAuthenticatorTest : public testing::Test { protected: std::unique_ptr> authenticator_; - StrictMock request_info_{}; - FilterContext filter_context_{&request_info_, nullptr, - istio::envoy::config::filter::http::authn:: - v2alpha1::FilterConfig::default_instance()}; + FilterContext filter_context_{ + envoy::api::v2::core::Metadata::default_instance(), nullptr, + istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig:: + default_instance()}; iaapi::Policy policy_; Payload* payload_; diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index bd75321a5f5..d0b46eea19e 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -205,14 +205,11 @@ void JwtAuthenticator::VerifyKey(const PubkeyCacheItem& issuer_item) { return; } - if (!issuer_item.jwt_config().forward_payload_header().empty()) { - // TODO: can we save as proto or json object directly? - callback_->savePayload(issuer_item.jwt_config().forward_payload_header(), - jwt_->PayloadStr()); - // const LowerCaseString key( - // issuer_item.jwt_config().forward_payload_header()); - // headers_->addCopy(key, jwt_->PayloadStrBase64Url()); - } + // TODO: can we save as proto or json object directly? + // User the issuer as the entry key for simplicity. The forward_payload_header + // field can be removed or replace by a boolean (to make `save` is + // conditional) + callback_->savePayload(issuer_item.jwt_config().issuer(), jwt_->PayloadStr()); if (!issuer_item.jwt_config().forward()) { // Remove JWT from headers. diff --git a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc index 9574e54d2a3..e3acc734f89 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator_test.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator_test.cc @@ -88,8 +88,8 @@ const std::string kPublicKey = " \"kid\": \"b3319a147514df7ee5e4bcdee51350cc890cc89e\"" "}]}"; -// Keep this same as forward_payload_header field in the config below. -const char kJwtPayloadOutputKey[] = "test-output"; +// Keep this same as issuer field in the config below. +const char kJwtIssuer[] = "https://example.com"; // A good JSON config. const char kExampleConfig[] = R"( { @@ -343,7 +343,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTandCache) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); // Verify the token is removed. @@ -370,7 +370,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoAlg) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); @@ -400,7 +400,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTPubkeyNoKid) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); @@ -423,8 +423,8 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, - kGoodTokenAudHasProtocolSchemePayload)); + EXPECT_CALL(mock_cb, + savePayload(kJwtIssuer, kGoodTokenAudHasProtocolSchemePayload)); auth_->Verify(headers, &mock_cb); @@ -447,8 +447,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService1) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, - savePayload(kJwtPayloadOutputKey, kGoodTokenAudService1Payload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenAudService1Payload)); auth_->Verify(headers, &mock_cb); @@ -471,8 +470,7 @@ TEST_F(JwtAuthenticatorTest, TestOkJWTAudService2) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, - savePayload(kJwtPayloadOutputKey, kGoodTokenAudService2Payload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenAudService2Payload)); auth_->Verify(headers, &mock_cb); @@ -498,7 +496,7 @@ TEST_F(JwtAuthenticatorTest, TestForwardJwt) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); @@ -730,7 +728,9 @@ TEST_F(JwtAuthenticatorTest, TestOnDestroy) { } TEST_F(JwtAuthenticatorTest, TestNoForwardPayloadHeader) { - // In this config, there is no forward_payload_header + // The flag (forward_payload_header) is deprecated and have no impact. The + // current behavior is always save JWT payload to request info (dynamic + // metadata). In this config, there is no forward_payload_header. SetupConfig(kExampleConfigWithoutForwardPayloadHeader); MockUpstream mock_pubkey(mock_cm_, kPublicKey); auto headers = TestHeaderMapImpl{{"Authorization", "Bearer " + kGoodToken}}; @@ -738,6 +738,8 @@ TEST_F(JwtAuthenticatorTest, TestNoForwardPayloadHeader) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); + // Note savePayload is still being called, as explain above. + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); // Test when forward_payload_header is not set, nothing added to headers. @@ -762,7 +764,7 @@ TEST_F(JwtAuthenticatorTest, TestInlineJwks) { EXPECT_CALL(mock_cb, onDone(_)).WillOnce(Invoke([](const Status &status) { ASSERT_EQ(status, Status::OK); })); - EXPECT_CALL(mock_cb, savePayload(kJwtPayloadOutputKey, kGoodTokenPayload)); + EXPECT_CALL(mock_cb, savePayload(kJwtIssuer, kGoodTokenPayload)); auth_->Verify(headers, &mock_cb); EXPECT_EQ(mock_pubkey.called_count(), 0); diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index 409b5a8dce6..0fe3af1e011 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -16,8 +16,8 @@ #include "src/envoy/utils/authn.h" #include "common/common/base64.h" #include "include/istio/utils/attribute_names.h" -#include "src/istio/authn/context.pb.h" #include "include/istio/utils/filter_names.h" +#include "src/istio/authn/context.pb.h" using istio::authn::Result; @@ -105,9 +105,11 @@ bool Authentication::FetchResultFromHeader(const Http::HeaderMap& headers, return result->ParseFromString(Base64::decode(value)); } -const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo(const RequestInfo::RequestInfo& request_info) { +const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo( + const RequestInfo::RequestInfo& request_info) { const auto& metadata = request_info.dynamicMetadata(); - const auto& iter = metadata.filter_metadata().find(istio::utils::FilterName::kAuthentication); + const auto& iter = metadata.filter_metadata().find( + istio::utils::FilterName::kAuthentication); if (iter == metadata.filter_metadata().end()) { return nullptr; } diff --git a/src/envoy/utils/authn.h b/src/envoy/utils/authn.h index 8bbb4e4f09c..eae74256610 100644 --- a/src/envoy/utils/authn.h +++ b/src/envoy/utils/authn.h @@ -15,9 +15,9 @@ #include "common/common/logger.h" #include "envoy/http/header_map.h" +#include "envoy/request_info/request_info.h" #include "google/protobuf/struct.pb.h" #include "src/istio/authn/context.pb.h" -#include "envoy/request_info/request_info.h" namespace Envoy { namespace Utils { @@ -43,7 +43,8 @@ class Authentication : public Logger::Loggable { // Returns a pointer to the authentication result from request info, if // available. Otherwise, return nullptrl - static const ProtobufWkt::Struct* GetResultFromRequestInfo(const RequestInfo::RequestInfo& request_info); + static const ProtobufWkt::Struct* GetResultFromRequestInfo( + const RequestInfo::RequestInfo& request_info); // Clears authentication result in header, if exist. static void ClearResultInHeader(Http::HeaderMap* headers); From fd51c41b520a44b8586e1632d9f4d40d3fc6cd69 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Fri, 3 Aug 2018 12:08:57 -0700 Subject: [PATCH 6/8] Remove unused test configs --- .../authn/http_filter_integration_test.cc | 3 +- .../http/authn/testdata/envoy_empty.conf | 85 ---------------- ...envoy_jwt_with_output_header_location.conf | 99 ------------------- .../testdata/envoy_origin_jwt_authn_only.conf | 99 ------------------- .../http/authn/testdata/envoy_peer_mtls.conf | 94 ------------------ 5 files changed, 1 insertion(+), 379 deletions(-) delete mode 100644 src/envoy/http/authn/testdata/envoy_empty.conf delete mode 100644 src/envoy/http/authn/testdata/envoy_jwt_with_output_header_location.conf delete mode 100644 src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf delete mode 100644 src/envoy/http/authn/testdata/envoy_peer_mtls.conf diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 3b6f4753b6d..398d6864dbc 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -71,8 +71,7 @@ std::string MakeHeaderToMetadataConfig() { metadata_namespace: %s key: %s value: "%s" - type: STRING - )", + type: STRING)", Extensions::HttpFilters::HttpFilterNames::get().HeaderToMetadata, istio::utils::FilterName::kJwt, kJwtIssuer, StringUtil::escape(kMockJwtPayload)); diff --git a/src/envoy/http/authn/testdata/envoy_empty.conf b/src/envoy/http/authn/testdata/envoy_empty.conf deleted file mode 100644 index c9f244f91ce..00000000000 --- a/src/envoy/http/authn/testdata/envoy_empty.conf +++ /dev/null @@ -1,85 +0,0 @@ -{ - "listeners": [ - { - "address": "tcp://{{ ip_loopback_address }}:0", - "bind_to_port": true, - "filters": [ - { - "type": "read", - "name": "http_connection_manager", - "config": { - "codec_type": "auto", - "stat_prefix": "ingress_http", - "route_config": { - "virtual_hosts": [ - { - "name": "backend", - "domains": ["*"], - "routes": [ - { - "prefix": "/", - "cluster": "backend_service" - } - ] - } - ] - }, - "access_log": [ - { - "path": "/dev/null" - } - ], - "filters": [ - { - "type": "decoder", - "name": "istio_authn", - "config": {} - }, - { - "type": "decoder", - "name": "router", - "config": {} - } - ] - } - } - ] - } - ], - "admin": { - "access_log_path": "/dev/null", - "address": "tcp://{{ ip_loopback_address }}:0" - }, - "cluster_manager": { - "clusters": [ - { - "name": "backend_service", - "connect_timeout_ms": 5000, - "type": "static", - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_0 }}" - } - ] - }, - { - "name": "example_issuer", - "connect_timeout_ms": 5000, - "type": "static", - "circuit_breakers": { - "default": { - "max_pending_requests": 10000, - "max_requests": 10000 - } - }, - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_1 }}" - } - ] - } - ] - } -} diff --git a/src/envoy/http/authn/testdata/envoy_jwt_with_output_header_location.conf b/src/envoy/http/authn/testdata/envoy_jwt_with_output_header_location.conf deleted file mode 100644 index 0a0c6e8cdd8..00000000000 --- a/src/envoy/http/authn/testdata/envoy_jwt_with_output_header_location.conf +++ /dev/null @@ -1,99 +0,0 @@ -{ - "listeners": [ - { - "address": "tcp://{{ ip_loopback_address }}:0", - "bind_to_port": true, - "filters": [ - { - "type": "read", - "name": "http_connection_manager", - "config": { - "codec_type": "auto", - "stat_prefix": "ingress_http", - "route_config": { - "virtual_hosts": [ - { - "name": "backend", - "domains": ["*"], - "routes": [ - { - "prefix": "/", - "cluster": "backend_service" - } - ] - } - ] - }, - "access_log": [ - { - "path": "/dev/null" - } - ], - "filters": [ - { - "type": "decoder", - "name": "istio_authn", - "config": { - "policy": { - "origins": [ - { - "jwt": { - "issuer": "issuer@foo.com", - "jwks_uri": "http://localhost:8081/" - } - } - ] - }, - "jwt_output_payload_locations": { - "issuer@foo.com": "location-to-read-jwt-result" - } - } - }, - { - "type": "decoder", - "name": "router", - "config": {} - } - ] - } - } - ] - } - ], - "admin": { - "access_log_path": "/dev/null", - "address": "tcp://{{ ip_loopback_address }}:0" - }, - "cluster_manager": { - "clusters": [ - { - "name": "backend_service", - "connect_timeout_ms": 5000, - "type": "static", - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_0 }}" - } - ] - }, - { - "name": "example_issuer", - "connect_timeout_ms": 5000, - "type": "static", - "circuit_breakers": { - "default": { - "max_pending_requests": 10000, - "max_requests": 10000 - } - }, - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_1 }}" - } - ] - } - ] - } -} diff --git a/src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf b/src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf deleted file mode 100644 index 5806fec16d2..00000000000 --- a/src/envoy/http/authn/testdata/envoy_origin_jwt_authn_only.conf +++ /dev/null @@ -1,99 +0,0 @@ -{ - "listeners": [ - { - "address": "tcp://{{ ip_loopback_address }}:0", - "bind_to_port": true, - "filters": [ - { - "type": "read", - "name": "http_connection_manager", - "config": { - "codec_type": "auto", - "stat_prefix": "ingress_http", - "route_config": { - "virtual_hosts": [ - { - "name": "backend", - "domains": ["*"], - "routes": [ - { - "prefix": "/", - "cluster": "backend_service" - } - ] - } - ] - }, - "access_log": [ - { - "path": "/dev/null" - } - ], - "filters": [ - { - "type": "decoder", - "name": "istio_authn", - "config": { - "policy": { - "origins": [ - { - "jwt": { - "issuer": "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com", - "jwks_uri": "http://localhost:8081/" - } - } - ] - }, - "jwt_output_payload_locations": { - "628645741881-noabiu23f5a8m8ovd8ucv698lj78vv0l@developer.gserviceaccount.com": "sec-istio-auth-userinfo" - } - } - }, - { - "type": "decoder", - "name": "router", - "config": {} - } - ] - } - } - ] - } - ], - "admin": { - "access_log_path": "/dev/null", - "address": "tcp://{{ ip_loopback_address }}:0" - }, - "cluster_manager": { - "clusters": [ - { - "name": "backend_service", - "connect_timeout_ms": 5000, - "type": "static", - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_0 }}" - } - ] - }, - { - "name": "example_issuer", - "connect_timeout_ms": 5000, - "type": "static", - "circuit_breakers": { - "default": { - "max_pending_requests": 10000, - "max_requests": 10000 - } - }, - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_1 }}" - } - ] - } - ] - } -} diff --git a/src/envoy/http/authn/testdata/envoy_peer_mtls.conf b/src/envoy/http/authn/testdata/envoy_peer_mtls.conf deleted file mode 100644 index 993de548707..00000000000 --- a/src/envoy/http/authn/testdata/envoy_peer_mtls.conf +++ /dev/null @@ -1,94 +0,0 @@ -{ - "listeners": [ - { - "address": "tcp://{{ ip_loopback_address }}:0", - "bind_to_port": true, - "filters": [ - { - "type": "read", - "name": "http_connection_manager", - "config": { - "codec_type": "auto", - "stat_prefix": "ingress_http", - "route_config": { - "virtual_hosts": [ - { - "name": "backend", - "domains": ["*"], - "routes": [ - { - "prefix": "/", - "cluster": "backend_service" - } - ] - } - ] - }, - "access_log": [ - { - "path": "/dev/null" - } - ], - "filters": [ - { - "type": "decoder", - "name": "istio_authn", - "config": { - "policy": { - "peers": [ - { - "mtls": { - } - } - ] - } - } - }, - { - "type": "decoder", - "name": "router", - "config": {} - } - ] - } - } - ] - } - ], - "admin": { - "access_log_path": "/dev/null", - "address": "tcp://{{ ip_loopback_address }}:0" - }, - "cluster_manager": { - "clusters": [ - { - "name": "backend_service", - "connect_timeout_ms": 5000, - "type": "static", - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_0 }}" - } - ] - }, - { - "name": "example_issuer", - "connect_timeout_ms": 5000, - "type": "static", - "circuit_breakers": { - "default": { - "max_pending_requests": 10000, - "max_requests": 10000 - } - }, - "lb_type": "round_robin", - "hosts": [ - { - "url": "tcp://{{ ip_loopback_address }}:{{ upstream_1 }}" - } - ] - } - ] - } -} From b317d0aed822188bde8547f8b2b50214ff6b3975 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Mon, 6 Aug 2018 09:58:40 -0700 Subject: [PATCH 7/8] Address reviews --- include/istio/utils/BUILD | 8 -------- src/envoy/http/authn/BUILD | 9 ++++----- src/envoy/http/authn/authenticator_base.cc | 2 +- src/envoy/http/authn/authenticator_base_test.cc | 9 ++++----- src/envoy/http/authn/filter_context.cc | 12 +++--------- src/envoy/http/authn/filter_context.h | 4 ---- src/envoy/http/authn/http_filter.cc | 4 ++-- src/envoy/http/authn/http_filter_factory.cc | 4 ++-- .../http/authn/http_filter_integration_test.cc | 4 ++-- src/envoy/http/jwt_auth/BUILD | 4 ++-- src/envoy/http/jwt_auth/http_filter.cc | 4 ++-- src/envoy/http/jwt_auth/http_filter_factory.cc | 4 ++-- src/envoy/http/jwt_auth/jwt_authenticator.cc | 10 ---------- src/envoy/utils/BUILD | 14 +++++++++++++- src/envoy/utils/authn.cc | 4 ++-- src/{istio => envoy}/utils/filter_names.cc | 15 ++++++++------- .../istio => src/envoy}/utils/filter_names.h | 17 +++++++---------- src/istio/utils/BUILD | 11 ----------- 18 files changed, 54 insertions(+), 85 deletions(-) rename src/{istio => envoy}/utils/filter_names.cc (74%) rename {include/istio => src/envoy}/utils/filter_names.h (73%) diff --git a/include/istio/utils/BUILD b/include/istio/utils/BUILD index 1c31b7180d2..92bb7e084bd 100644 --- a/include/istio/utils/BUILD +++ b/include/istio/utils/BUILD @@ -41,12 +41,4 @@ cc_library( "attribute_names.h", ], visibility = ["//visibility:public"], -) - -cc_library( - name = "filter_names_header", - hdrs = [ - "filter_names.h", - ], - visibility = ["//visibility:public"], ) \ No newline at end of file diff --git a/src/envoy/http/authn/BUILD b/src/envoy/http/authn/BUILD index 06cd2378512..3f4c3f296cd 100644 --- a/src/envoy/http/authn/BUILD +++ b/src/envoy/http/authn/BUILD @@ -46,7 +46,7 @@ envoy_cc_library( "//src/envoy/http/jwt_auth:jwt_lib", "//src/envoy/utils:utils_lib", "//src/istio/authn:context_proto", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", ], ) @@ -67,7 +67,7 @@ envoy_cc_library( "//src/envoy/utils:utils_lib", "//src/istio/authn:context_proto", "@envoy//source/exe:envoy_common_lib", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", ], ) @@ -100,8 +100,7 @@ envoy_cc_test( ":authenticator", ":test_utils", "@envoy//test/mocks/network:network_mocks", - # "@envoy//test/mocks/request_info:request_info_mocks", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", "@envoy//test/mocks/ssl:ssl_mocks", "@envoy//test/test_common:utility_lib", ], @@ -165,6 +164,6 @@ envoy_cc_test( ":filter_lib", "@envoy//source/common/common:utility_lib", "@envoy//test/integration:http_protocol_integration_lib", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", ], ) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index e3922449187..ae3d0437711 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -16,7 +16,7 @@ #include "src/envoy/http/authn/authenticator_base.h" #include "common/common/assert.h" #include "common/config/metadata.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/authn_utils.h" #include "src/envoy/utils/utils.h" diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 7ac99079155..9b25aa975ab 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -20,9 +20,8 @@ #include "gmock/gmock.h" #include "src/envoy/http/authn/test_utils.h" #include "test/mocks/network/mocks.h" -//#include "test/mocks/request_info/mocks.h" #include "envoy/api/v2/core/base.pb.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "test/mocks/ssl/mocks.h" using google::protobuf::util::MessageDifferencer; @@ -244,7 +243,7 @@ TEST_F(ValidateJwtTest, NoJwtPayloadOutput) { TEST_F(ValidateJwtTest, HasJwtPayloadOutputButNoDataForKey) { jwt_.set_issuer("issuer@foo.com"); - (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + (*dynamic_metadata_.mutable_filter_metadata())[Utils::IstioFilterName::kJwt] .MergeFrom(MessageUtil::keyValueStruct("foo", "bar")); // When there is no JWT payload for given issuer in request info dynamic @@ -255,7 +254,7 @@ TEST_F(ValidateJwtTest, HasJwtPayloadOutputButNoDataForKey) { TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { jwt_.set_issuer("issuer@foo.com"); - (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + (*dynamic_metadata_.mutable_filter_metadata())[Utils::IstioFilterName::kJwt] .MergeFrom(MessageUtil::keyValueStruct("issuer@foo.com", "bad-data")); // EXPECT_CALL(request_info_, dynamicMetadata()); @@ -265,7 +264,7 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailableWithBadData) { TEST_F(ValidateJwtTest, JwtPayloadAvailable) { jwt_.set_issuer("issuer@foo.com"); - (*dynamic_metadata_.mutable_filter_metadata())[istio::utils::FilterName::kJwt] + (*dynamic_metadata_.mutable_filter_metadata())[Utils::IstioFilterName::kJwt] .MergeFrom(MessageUtil::keyValueStruct("issuer@foo.com", kSecIstioAuthUserinfoHeaderValue)); diff --git a/src/envoy/http/authn/filter_context.cc b/src/envoy/http/authn/filter_context.cc index e5e7f7e5e60..38bf952db00 100644 --- a/src/envoy/http/authn/filter_context.cc +++ b/src/envoy/http/authn/filter_context.cc @@ -14,7 +14,7 @@ */ #include "src/envoy/http/authn/filter_context.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/utils/utils.h" using istio::authn::Payload; @@ -74,17 +74,11 @@ void FilterContext::setPrincipal(const iaapi::PrincipalBinding& binding) { bool FilterContext::getJwtPayload(const std::string& issuer, std::string* payload) const { - // auto location_iter = - // filter_config_.jwt_output_payload_locations().find(issuer); - // if (location_iter == filter_config_.jwt_output_payload_locations().end()) { - // ENVOY_LOG(warn, "No JWT payload output location is found for the issuer - // {}", issuer); return false; - // } const auto filter_it = - dynamic_metadata_.filter_metadata().find(istio::utils::FilterName::kJwt); + dynamic_metadata_.filter_metadata().find(Utils::IstioFilterName::kJwt); if (filter_it == dynamic_metadata_.filter_metadata().end()) { ENVOY_LOG(debug, "No dynamic_metadata found for filter {}", - istio::utils::FilterName::kJwt); + Utils::IstioFilterName::kJwt); return false; } diff --git a/src/envoy/http/authn/filter_context.h b/src/envoy/http/authn/filter_context.h index 3265783eb3e..0b5e060f98e 100644 --- a/src/envoy/http/authn/filter_context.h +++ b/src/envoy/http/authn/filter_context.h @@ -57,10 +57,6 @@ class FilterContext : public Logger::Loggable { // Returns the authentication result. const istio::authn::Result& authenticationResult() { return result_; } - // Acessor to dynamic metadata. - const envoy::api::v2::core::Metadata& dynamic_metadata() const { - return dynamic_metadata_; - } // Accessor to connection const Network::Connection* connection() { return connection_; } // Accessor to the filter config diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 0b369832a27..7d41cbef6fe 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -17,7 +17,7 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/http/utility.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/origin_authenticator.h" #include "src/envoy/http/authn/peer_authenticator.h" #include "src/envoy/utils/authn.h" @@ -80,7 +80,7 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); decoder_callbacks_->requestInfo().setDynamicMetadata( - istio::utils::FilterName::kAuthentication, data); + Utils::IstioFilterName::kAuthentication, data); ENVOY_LOG(debug, "Saved Dynamic Metadata:\n{}", data.DebugString()); } state_ = State::COMPLETE; diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index 45d72ae0da5..bc992b1d625 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -17,7 +17,7 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" #include "google/protobuf/util/json_util.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/http_filter.h" #include "src/envoy/utils/utils.h" @@ -62,7 +62,7 @@ class AuthnFilterConfig : public NamedHttpFilterConfigFactory, } std::string name() override { - return istio::utils::FilterName::kAuthentication; + return Utils::IstioFilterName::kAuthentication; } private: diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 398d6864dbc..0ba682b5007 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -17,7 +17,7 @@ #include "common/common/utility.h" #include "extensions/filters/http/well_known_names.h" #include "fmt/printf.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/istio/authn/context.pb.h" #include "test/integration/http_protocol_integration.h" @@ -73,7 +73,7 @@ std::string MakeHeaderToMetadataConfig() { value: "%s" type: STRING)", Extensions::HttpFilters::HttpFilterNames::get().HeaderToMetadata, - istio::utils::FilterName::kJwt, kJwtIssuer, + Utils::IstioFilterName::kJwt, kJwtIssuer, StringUtil::escape(kMockJwtPayload)); } diff --git a/src/envoy/http/jwt_auth/BUILD b/src/envoy/http/jwt_auth/BUILD index b1f9537f1e8..eca24ac187f 100644 --- a/src/envoy/http/jwt_auth/BUILD +++ b/src/envoy/http/jwt_auth/BUILD @@ -70,7 +70,7 @@ envoy_cc_library( deps = [ ":jwt_authenticator_lib", "@envoy//source/exe:envoy_common_lib", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", ], ) @@ -81,7 +81,7 @@ envoy_cc_library( deps = [ ":http_filter_lib", "@envoy//source/exe:envoy_common_lib", - "//src/istio/utils:filter_names_lib", + "//src/envoy/utils:filter_names_lib", ], ) diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 99cb850bdd1..7037165979a 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -18,7 +18,7 @@ #include "common/http/message_impl.h" #include "common/http/utility.h" #include "envoy/http/async_client.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include #include @@ -76,7 +76,7 @@ void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { void JwtVerificationFilter::savePayload(const std::string& key, const std::string& payload) { decoder_callbacks_->requestInfo().setDynamicMetadata( - istio::utils::FilterName::kJwt, + Utils::IstioFilterName::kJwt, MessageUtil::keyValueStruct(key, payload)); } diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 25748cf6be5..6cf54309c60 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -15,7 +15,7 @@ #include "envoy/registry/registry.h" #include "google/protobuf/util/json_util.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/jwt_auth/auth_store.h" #include "src/envoy/http/jwt_auth/http_filter.h" @@ -47,7 +47,7 @@ class JwtVerificationFilterConfig : public NamedHttpFilterConfigFactory { return ProtobufTypes::MessagePtr{new JwtAuthentication}; } - std::string name() override { return istio::utils::FilterName::kJwt; } + std::string name() override { return Utils::IstioFilterName::kJwt; } private: Http::FilterFactoryCb createFilter(const JwtAuthentication& proto_config, diff --git a/src/envoy/http/jwt_auth/jwt_authenticator.cc b/src/envoy/http/jwt_auth/jwt_authenticator.cc index d0b46eea19e..c47b8b6d036 100644 --- a/src/envoy/http/jwt_auth/jwt_authenticator.cc +++ b/src/envoy/http/jwt_auth/jwt_authenticator.cc @@ -59,16 +59,6 @@ void JwtAuthenticator::Verify(HeaderMap& headers, headers_ = &headers; callback_ = callback; - // Sanitize the JWT verification result in the HTTP headers - // for (const auto& rule : store_.config().rules()) { - // if (!rule.forward_payload_header().empty()) { - // ENVOY_LOG(debug, "Sanitize JWT authentication output header {}", - // rule.forward_payload_header()); - // const LowerCaseString key(rule.forward_payload_header()); - // headers.remove(key); - // } - // } - ENVOY_LOG(debug, "Jwt authentication starts"); std::vector> tokens; store_.token_extractor().Extract(headers, &tokens); diff --git a/src/envoy/utils/BUILD b/src/envoy/utils/BUILD index c4f3cd9228e..adf671a1b20 100644 --- a/src/envoy/utils/BUILD +++ b/src/envoy/utils/BUILD @@ -35,7 +35,7 @@ envoy_cc_library( "//include/istio/utils:attribute_names_header", "//src/istio/authn:context_proto", "//src/istio/utils:attribute_names_lib", - "//src/istio/utils:filter_names_lib", + ":filter_names_lib", "@envoy//source/exe:envoy_common_lib", ], ) @@ -90,3 +90,15 @@ envoy_cc_test( "@envoy//test/test_common:utility_lib", ], ) + + +cc_library( + name = "filter_names_lib", + srcs = [ + "filter_names.cc", + ], + hdrs = [ + "filter_names.h", + ], + visibility = ["//visibility:public"], +) \ No newline at end of file diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index 0fe3af1e011..ef12463fef7 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -16,7 +16,7 @@ #include "src/envoy/utils/authn.h" #include "common/common/base64.h" #include "include/istio/utils/attribute_names.h" -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" #include "src/istio/authn/context.pb.h" using istio::authn::Result; @@ -109,7 +109,7 @@ const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo( const RequestInfo::RequestInfo& request_info) { const auto& metadata = request_info.dynamicMetadata(); const auto& iter = metadata.filter_metadata().find( - istio::utils::FilterName::kAuthentication); + Utils::IstioFilterName::kAuthentication); if (iter == metadata.filter_metadata().end()) { return nullptr; } diff --git a/src/istio/utils/filter_names.cc b/src/envoy/utils/filter_names.cc similarity index 74% rename from src/istio/utils/filter_names.cc rename to src/envoy/utils/filter_names.cc index c122946b25a..89b2a124090 100644 --- a/src/istio/utils/filter_names.cc +++ b/src/envoy/utils/filter_names.cc @@ -13,14 +13,15 @@ * limitations under the License. */ -#include "include/istio/utils/filter_names.h" +#include "src/envoy/utils/filter_names.h" + +namespace Envoy { +namespace Utils { -namespace istio { -namespace utils { // TODO: using more standard naming, e.g istio.jwt, istio.authn -const char FilterName::kJwt[] = "jwt-auth"; -const char FilterName::kAuthentication[] = "istio_authn"; +const char IstioFilterName::kJwt[] = "jwt-auth"; +const char IstioFilterName::kAuthentication[] = "istio_authn"; -} // namespace utils -} // namespace istio +} // namespace Utils +} // namespace Envoy diff --git a/include/istio/utils/filter_names.h b/src/envoy/utils/filter_names.h similarity index 73% rename from include/istio/utils/filter_names.h rename to src/envoy/utils/filter_names.h index 8f7b54a5e19..de828197b82 100644 --- a/include/istio/utils/filter_names.h +++ b/src/envoy/utils/filter_names.h @@ -13,23 +13,20 @@ * limitations under the License. */ -#ifndef ISTIO_UTILS_FILTER_NAMES_H -#define ISTIO_UTILS_FILTER_NAMES_H +#pragma once #include -namespace istio { -namespace utils { +namespace Envoy { +namespace Utils { -// These are name of filters that currently output data to dynamicMetadata (by +// These are name of (Istio) filters that currently output data to dynamicMetadata (by // convention, under the the entry using filter name itself as key). Define them // here for easy access. -struct FilterName { +struct IstioFilterName { static const char kJwt[]; static const char kAuthentication[]; }; -} // namespace utils -} // namespace istio - -#endif // ISTIO_UTILS_FILTER_NAMES_H +} // namespace Utils +} // namespace Envoy \ No newline at end of file diff --git a/src/istio/utils/BUILD b/src/istio/utils/BUILD index ab192312ab1..a1ec20a2851 100644 --- a/src/istio/utils/BUILD +++ b/src/istio/utils/BUILD @@ -77,14 +77,3 @@ cc_library( "//include/istio/utils:attribute_names_header", ], ) - -cc_library( - name = "filter_names_lib", - srcs = [ - "filter_names.cc", - ], - visibility = ["//visibility:public"], - deps = [ - "//include/istio/utils:filter_names_header", - ], -) \ No newline at end of file From 430a29d44bb188fb66336cdf4fe08d508725c5de Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Mon, 6 Aug 2018 09:59:27 -0700 Subject: [PATCH 8/8] Lint --- src/envoy/http/authn/authenticator_base.cc | 2 +- src/envoy/http/authn/authenticator_base_test.cc | 4 ++-- src/envoy/http/authn/http_filter.cc | 2 +- src/envoy/http/authn/http_filter_factory.cc | 2 +- src/envoy/http/jwt_auth/http_filter.cc | 3 +-- src/envoy/http/jwt_auth/http_filter_factory.cc | 2 +- src/envoy/utils/authn.cc | 4 ++-- src/envoy/utils/filter_names.cc | 1 - src/envoy/utils/filter_names.h | 6 +++--- 9 files changed, 12 insertions(+), 14 deletions(-) diff --git a/src/envoy/http/authn/authenticator_base.cc b/src/envoy/http/authn/authenticator_base.cc index ae3d0437711..9262d249e25 100644 --- a/src/envoy/http/authn/authenticator_base.cc +++ b/src/envoy/http/authn/authenticator_base.cc @@ -16,8 +16,8 @@ #include "src/envoy/http/authn/authenticator_base.h" #include "common/common/assert.h" #include "common/config/metadata.h" -#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/authn_utils.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/utils/utils.h" using istio::authn::Payload; diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 9b25aa975ab..068e5db1edb 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -16,12 +16,12 @@ #include "src/envoy/http/authn/authenticator_base.h" #include "common/common/base64.h" #include "common/protobuf/protobuf.h" +#include "envoy/api/v2/core/base.pb.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" #include "gmock/gmock.h" #include "src/envoy/http/authn/test_utils.h" -#include "test/mocks/network/mocks.h" -#include "envoy/api/v2/core/base.pb.h" #include "src/envoy/utils/filter_names.h" +#include "test/mocks/network/mocks.h" #include "test/mocks/ssl/mocks.h" using google::protobuf::util::MessageDifferencer; diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 7d41cbef6fe..ddb0ed4f680 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -17,10 +17,10 @@ #include "authentication/v1alpha1/policy.pb.h" #include "common/http/utility.h" #include "envoy/config/filter/http/authn/v2alpha1/config.pb.h" -#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/origin_authenticator.h" #include "src/envoy/http/authn/peer_authenticator.h" #include "src/envoy/utils/authn.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/utils/utils.h" using istio::authn::Payload; diff --git a/src/envoy/http/authn/http_filter_factory.cc b/src/envoy/http/authn/http_filter_factory.cc index bc992b1d625..221cc78a6e5 100644 --- a/src/envoy/http/authn/http_filter_factory.cc +++ b/src/envoy/http/authn/http_filter_factory.cc @@ -17,8 +17,8 @@ #include "envoy/registry/registry.h" #include "envoy/server/filter_config.h" #include "google/protobuf/util/json_util.h" -#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/authn/http_filter.h" +#include "src/envoy/utils/filter_names.h" #include "src/envoy/utils/utils.h" using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; diff --git a/src/envoy/http/jwt_auth/http_filter.cc b/src/envoy/http/jwt_auth/http_filter.cc index 7037165979a..40a50e9ffb5 100644 --- a/src/envoy/http/jwt_auth/http_filter.cc +++ b/src/envoy/http/jwt_auth/http_filter.cc @@ -76,8 +76,7 @@ void JwtVerificationFilter::onDone(const JwtAuth::Status& status) { void JwtVerificationFilter::savePayload(const std::string& key, const std::string& payload) { decoder_callbacks_->requestInfo().setDynamicMetadata( - Utils::IstioFilterName::kJwt, - MessageUtil::keyValueStruct(key, payload)); + Utils::IstioFilterName::kJwt, MessageUtil::keyValueStruct(key, payload)); } FilterDataStatus JwtVerificationFilter::decodeData(Buffer::Instance&, bool) { diff --git a/src/envoy/http/jwt_auth/http_filter_factory.cc b/src/envoy/http/jwt_auth/http_filter_factory.cc index 6cf54309c60..06daf039fb0 100644 --- a/src/envoy/http/jwt_auth/http_filter_factory.cc +++ b/src/envoy/http/jwt_auth/http_filter_factory.cc @@ -15,9 +15,9 @@ #include "envoy/registry/registry.h" #include "google/protobuf/util/json_util.h" -#include "src/envoy/utils/filter_names.h" #include "src/envoy/http/jwt_auth/auth_store.h" #include "src/envoy/http/jwt_auth/http_filter.h" +#include "src/envoy/utils/filter_names.h" using ::istio::envoy::config::filter::http::jwt_auth::v2alpha1:: JwtAuthentication; diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index ef12463fef7..6bb73230263 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -108,8 +108,8 @@ bool Authentication::FetchResultFromHeader(const Http::HeaderMap& headers, const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo( const RequestInfo::RequestInfo& request_info) { const auto& metadata = request_info.dynamicMetadata(); - const auto& iter = metadata.filter_metadata().find( - Utils::IstioFilterName::kAuthentication); + const auto& iter = + metadata.filter_metadata().find(Utils::IstioFilterName::kAuthentication); if (iter == metadata.filter_metadata().end()) { return nullptr; } diff --git a/src/envoy/utils/filter_names.cc b/src/envoy/utils/filter_names.cc index 89b2a124090..2626766b5c2 100644 --- a/src/envoy/utils/filter_names.cc +++ b/src/envoy/utils/filter_names.cc @@ -18,7 +18,6 @@ namespace Envoy { namespace Utils { - // TODO: using more standard naming, e.g istio.jwt, istio.authn const char IstioFilterName::kJwt[] = "jwt-auth"; const char IstioFilterName::kAuthentication[] = "istio_authn"; diff --git a/src/envoy/utils/filter_names.h b/src/envoy/utils/filter_names.h index de828197b82..64b79dfff2c 100644 --- a/src/envoy/utils/filter_names.h +++ b/src/envoy/utils/filter_names.h @@ -20,9 +20,9 @@ namespace Envoy { namespace Utils { -// These are name of (Istio) filters that currently output data to dynamicMetadata (by -// convention, under the the entry using filter name itself as key). Define them -// here for easy access. +// These are name of (Istio) filters that currently output data to +// dynamicMetadata (by convention, under the the entry using filter name itself +// as key). Define them here for easy access. struct IstioFilterName { static const char kJwt[]; static const char kAuthentication[];