From ba72d6333d43459821dd5fe6e99cb8e9936a974d Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 15:55:47 -0700 Subject: [PATCH 1/8] Use dynamic metadata to for authentication filter output. --- include/istio/control/http/check_data.h | 13 +-- include/istio/utils/attributes_builder.h | 16 ++-- src/envoy/http/authn/http_filter.cc | 7 +- .../authn/http_filter_integration_test.cc | 68 +++++++-------- src/envoy/http/authn/http_filter_test.cc | 14 +-- src/envoy/http/mixer/check_data.cc | 41 +-------- src/envoy/http/mixer/check_data.h | 10 +-- src/envoy/http/mixer/filter.cc | 9 +- src/envoy/utils/authn.cc | 46 +--------- src/envoy/utils/authn.h | 39 ++------- src/envoy/utils/authn_test.cc | 87 +++++++------------ src/istio/control/http/attributes_builder.cc | 63 ++++++-------- .../istio_http_integration_test.cc | 8 +- 13 files changed, 143 insertions(+), 278 deletions(-) diff --git a/include/istio/control/http/check_data.h b/include/istio/control/http/check_data.h index 318f409b717..426feaa623a 100644 --- a/include/istio/control/http/check_data.h +++ b/include/istio/control/http/check_data.h @@ -19,7 +19,7 @@ #include #include -#include "src/istio/authn/context.pb.h" +#include "google/protobuf/struct.pb.h" namespace istio { namespace control { @@ -81,14 +81,9 @@ class CheckData { virtual bool FindCookie(const std::string &name, std::string *value) const = 0; - // If the request has a JWT token and it is verified, get its payload as - // string map, and return true. Otherwise return false. - virtual bool GetJWTPayload( - std::map *payload) const = 0; - - // If the request has authentication result in header, parses data into the - // output result; returns true if success. Otherwise, returns false. - virtual bool GetAuthenticationResult(istio::authn::Result *result) const = 0; + // Returns a pointer to the authentication result from request info dynamic metadata, if + // available. Otherwise, returns nullptrl + virtual const ::google::protobuf::Struct* GetAuthenticationResult() const = 0; }; // An interfact to update request HTTP headers with Istio attributes. diff --git a/include/istio/utils/attributes_builder.h b/include/istio/utils/attributes_builder.h index da124977414..afcebaeeaab 100644 --- a/include/istio/utils/attributes_builder.h +++ b/include/istio/utils/attributes_builder.h @@ -20,7 +20,7 @@ #include #include -#include "google/protobuf/map.h" +#include "google/protobuf/struct.pb.h" #include "mixer/v1/attributes.pb.h" namespace istio { @@ -89,18 +89,20 @@ class AttributesBuilder { } } - void AddProtobufStringMap( - const std::string& key, - const google::protobuf::Map& string_map) { - if (string_map.empty()) { + void AddProtoStructStringMap(const std::string& key, + const google::protobuf::Struct& struct_map) { + if (struct_map.fields().empty()) { return; } auto entries = (*attributes_->mutable_attributes())[key] .mutable_string_map_value() ->mutable_entries(); entries->clear(); - for (const auto& map_it : string_map) { - (*entries)[map_it.first] = map_it.second; + for (const auto& field : struct_map.fields()) { + // Ignore all fields that are not string. + if (!field.second.string_value().empty()) { + (*entries)[field.first] = field.second.string_value(); + } } } diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index ddb0ed4f680..bc34f0fef71 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -42,8 +42,7 @@ 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; @@ -71,10 +70,6 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap& headers, // 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 0ba682b5007..382c2572549 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -164,40 +164,40 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - // 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; - EXPECT_TRUE(result.ParseFromString(value)); - - 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" - } - 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)); +// // 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; +// EXPECT_TRUE(result.ParseFromString(value)); + +// 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" +// } +// 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 diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index eab12be2dc7..6a0eab43d0d 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -31,6 +31,7 @@ using Envoy::Http::Istio::AuthN::FilterContext; using istio::authn::Payload; using istio::authn::Result; using istio::envoy::config::filter::http::authn::v2alpha1::FilterConfig; +using testing::AtLeast; using testing::Invoke; using testing::NiceMock; using testing::ReturnRef; @@ -120,7 +121,7 @@ TEST_F(AuthenticationFilterTest, PeerFail) { .WillOnce(Invoke(createAlwaysFailAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_CALL(decoder_callbacks_, requestInfo()) - .WillRepeatedly(ReturnRef(request_info)); + .Times(AtLeast(1)).WillRepeatedly(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -128,7 +129,7 @@ TEST_F(AuthenticationFilterTest, PeerFail) { })); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers_, true)); - EXPECT_FALSE(Utils::Authentication::HasResultInHeader(request_headers_)); + EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata(request_info.dynamicMetadata())); } TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { @@ -140,6 +141,9 @@ TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { EXPECT_CALL(filter_, createOriginAuthenticator(_)) .Times(1) .WillOnce(Invoke(createAlwaysFailAuthenticator)); + RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); + EXPECT_CALL(decoder_callbacks_, requestInfo()) + .Times(AtLeast(1)).WillRepeatedly(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -147,7 +151,7 @@ TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { })); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers_, true)); - EXPECT_FALSE(Utils::Authentication::HasResultInHeader(request_headers_)); + EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata(request_info.dynamicMetadata())); } TEST_F(AuthenticationFilterTest, AllPass) { @@ -159,14 +163,14 @@ TEST_F(AuthenticationFilterTest, AllPass) { .WillOnce(Invoke(createAlwaysPassAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_CALL(decoder_callbacks_, requestInfo()) - .WillRepeatedly(ReturnRef(request_info)); + .Times(AtLeast(1)).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); + Utils::Authentication::GetResultFromMetadata(request_info.dynamicMetadata()); ASSERT_TRUE(data); ProtobufWkt::Struct expected_data; diff --git a/src/envoy/http/mixer/check_data.cc b/src/envoy/http/mixer/check_data.cc index 22f0b3360c7..e86fd0e793f 100644 --- a/src/envoy/http/mixer/check_data.cc +++ b/src/envoy/http/mixer/check_data.cc @@ -38,8 +38,9 @@ const std::set RequestHeaderExclusives = { } // namespace CheckData::CheckData(const HeaderMap& headers, + const envoy::api::v2::core::Metadata& metadata, const Network::Connection* connection) - : headers_(headers), connection_(connection) { + : headers_(headers), metadata_(metadata), connection_(connection) { if (headers_.Path()) { query_params_ = Utility::parseQueryString(std::string( headers_.Path()->value().c_str(), headers_.Path()->value().size())); @@ -166,42 +167,8 @@ bool CheckData::FindCookie(const std::string& name, std::string* value) const { return false; } -bool CheckData::GetJWTPayload( - std::map* payload) const { - const HeaderEntry* entry = - headers_.get(JwtAuth::JwtAuthenticator::JwtPayloadKey()); - if (!entry) { - return false; - } - std::string value(entry->value().c_str(), entry->value().size()); - 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: {}", - JwtAuth::JwtAuthenticator::JwtPayloadKey().get(), value); - return false; - } - try { - auto json_obj = Json::Factory::loadFromString(payload_str); - json_obj->iterate( - [payload](const std::string& key, const Json::Object& obj) -> bool { - // will throw execption if value type is not string. - try { - (*payload)[key] = obj.asString(); - } catch (...) { - } - return true; - }); - } catch (...) { - ENVOY_LOG(error, "Invalid {} header, invalid json: {}", - JwtAuth::JwtAuthenticator::JwtPayloadKey().get(), payload_str); - return false; - } - return true; -} - -bool CheckData::GetAuthenticationResult(istio::authn::Result* result) const { - return Utils::Authentication::FetchResultFromHeader(headers_, result); +const ::google::protobuf::Struct* CheckData::GetAuthenticationResult() const { + return Utils::Authentication::GetResultFromMetadata(metadata_); } } // namespace Mixer diff --git a/src/envoy/http/mixer/check_data.h b/src/envoy/http/mixer/check_data.h index 85b3eef3994..0695d91e6b6 100644 --- a/src/envoy/http/mixer/check_data.h +++ b/src/envoy/http/mixer/check_data.h @@ -17,9 +17,11 @@ #include "common/common/logger.h" #include "common/http/utility.h" +#include "envoy/api/v2/core/base.pb.h" #include "envoy/http/header_map.h" #include "include/istio/control/http/controller.h" #include "src/istio/authn/context.pb.h" +#include "google/protobuf/struct.pb.h" namespace Envoy { namespace Http { @@ -28,7 +30,7 @@ namespace Mixer { class CheckData : public ::istio::control::http::CheckData, public Logger::Loggable { public: - CheckData(const HeaderMap& headers, const Network::Connection* connection); + CheckData(const HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, const Network::Connection* connection); // Find "x-istio-attributes" headers, if found base64 decode // its value and remove it from the headers. @@ -56,13 +58,11 @@ class CheckData : public ::istio::control::http::CheckData, bool FindCookie(const std::string& name, std::string* value) const override; - bool GetJWTPayload( - std::map* payload) const override; - - bool GetAuthenticationResult(istio::authn::Result* result) const override; + const ::google::protobuf::Struct* GetAuthenticationResult() const override; private: const HeaderMap& headers_; + const envoy::api::v2::core::Metadata& metadata_; const Network::Connection* connection_; Utility::QueryParams query_params_; }; diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index ca08d36e145..5d5ac20d81d 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -131,7 +131,7 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { state_ = Calling; initiating_call_ = true; - CheckData check_data(headers, decoder_callbacks_->connection()); + CheckData check_data(headers, decoder_callbacks_->requestInfo().dynamicMetadata(), decoder_callbacks_->connection()); Utils::HeaderUpdate header_update(&headers); headers_ = &headers; cancel_check_ = handler_->Check( @@ -209,11 +209,6 @@ void Filter::completeCheck(const CheckResponseInfo& info) { auto status = info.response_status; ENVOY_LOG(debug, "Called Mixer::Filter : check complete {}", status.ToString()); - // Remove Istio authentication header after Check() is completed - if (nullptr != headers_) { - Envoy::Utils::Authentication::ClearResultInHeader(headers_); - } - // This stream has been reset, abort the callback. if (state_ == Responded) { return; @@ -281,7 +276,7 @@ void Filter::log(const HeaderMap* request_headers, ReadPerRouteConfig(request_info.routeEntry(), &config); handler_ = control_.controller()->CreateRequestHandler(config); - CheckData check_data(*request_headers, nullptr); + CheckData check_data(*request_headers, request_info.dynamicMetadata(), nullptr); handler_->ExtractRequestAttributes(&check_data); } // response trailer header is not counted to response total size. diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index 8f90471b4be..a28f8ceba8f 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -25,10 +25,6 @@ namespace Envoy { namespace Utils { namespace { -// The HTTP header to save authentication result. -const Http::LowerCaseString kAuthenticationOutputHeaderLocation( - "sec-istio-authn-payload"); - // Helper function to set a key/value pair into Struct. static void setKeyValue(::google::protobuf::Struct& data, std::string key, std::string value) { @@ -37,21 +33,6 @@ static void setKeyValue(::google::protobuf::Struct& data, std::string key, } // namespace -bool Authentication::SaveResultToHeader(const istio::authn::Result& result, - Http::HeaderMap* headers) { - if (HasResultInHeader(*headers)) { - ENVOY_LOG(warn, - "Authentication result already exist in header. Cannot save"); - return false; - } - - std::string payload_data; - result.SerializeToString(&payload_data); - headers->addCopy(kAuthenticationOutputHeaderLocation, - Base64::encode(payload_data.c_str(), payload_data.size())); - return true; -} - void Authentication::SaveAuthAttributesToStruct( const istio::authn::Result& result, ::google::protobuf::Struct& data) { // TODO(diemvu): Refactor istio::authn::Result this conversion can be removed. @@ -104,19 +85,8 @@ void Authentication::SaveAuthAttributesToStruct( } } -bool Authentication::FetchResultFromHeader(const Http::HeaderMap& headers, - istio::authn::Result* result) { - const auto entry = headers.get(kAuthenticationOutputHeaderLocation); - if (entry == nullptr) { - return false; - } - std::string value(entry->value().c_str(), entry->value().size()); - return result->ParseFromString(Base64::decode(value)); -} - -const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo( - const RequestInfo::RequestInfo& request_info) { - const auto& metadata = request_info.dynamicMetadata(); +const ProtobufWkt::Struct* Authentication::GetResultFromMetadata( + const envoy::api::v2::core::Metadata& metadata) { const auto& iter = metadata.filter_metadata().find(Utils::IstioFilterName::kAuthentication); if (iter == metadata.filter_metadata().end()) { @@ -125,17 +95,5 @@ const ProtobufWkt::Struct* Authentication::GetResultFromRequestInfo( return &(iter->second); } -void Authentication::ClearResultInHeader(Http::HeaderMap* headers) { - headers->remove(kAuthenticationOutputHeaderLocation); -} - -bool Authentication::HasResultInHeader(const Http::HeaderMap& headers) { - return headers.get(kAuthenticationOutputHeaderLocation) != nullptr; -} - -const Http::LowerCaseString& Authentication::GetHeaderLocation() { - return kAuthenticationOutputHeaderLocation; -} - } // namespace Utils } // namespace Envoy diff --git a/src/envoy/utils/authn.h b/src/envoy/utils/authn.h index eae74256610..18f340eb1ce 100644 --- a/src/envoy/utils/authn.h +++ b/src/envoy/utils/authn.h @@ -14,8 +14,8 @@ */ #include "common/common/logger.h" -#include "envoy/http/header_map.h" -#include "envoy/request_info/request_info.h" +#include "common/protobuf/protobuf.h" +#include "envoy/api/v2/core/base.pb.h" #include "google/protobuf/struct.pb.h" #include "src/istio/authn/context.pb.h" @@ -24,41 +24,16 @@ namespace Utils { class Authentication : public Logger::Loggable { public: - // Saves (authentication) result to header with proper encoding (base64). The - // location is internal implementation detail, and is chosen to avoid possible - // collision. If header already contains data in that location, function will - // returns false and data is *not* overwritten. - static bool SaveResultToHeader(const istio::authn::Result& result, - Http::HeaderMap* headers); // Save authentication attributes into the data Struct. static void SaveAuthAttributesToStruct(const istio::authn::Result& result, ::google::protobuf::Struct& data); - // Looks up authentication result data in the header. If data is available, - // decodes and output result proto. Returns false if data is not available, or - // in bad format. - 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); - - // Returns true if there is header entry at thelocation that is used to store - // authentication result. (function does not check for validity of the data - // though). - static bool HasResultInHeader(const Http::HeaderMap& headers); - - private: - // Return the header location key. For testing purpose only. - static const Http::LowerCaseString& GetHeaderLocation(); - - friend class AuthenticationTest; + // Returns a pointer to the authentication result from metadata. Typically, the input metadata is + // the request info's dynamic metadata. Authentication result, if available, is stored under + // authentication filter metdata. Returns nullptr if there is no data for that filter. + static const ProtobufWkt::Struct* GetResultFromMetadata( + const envoy::api::v2::core::Metadata& metadata); }; } // namespace Utils diff --git a/src/envoy/utils/authn_test.cc b/src/envoy/utils/authn_test.cc index 1fd142641dd..2ac9e59ed4e 100644 --- a/src/envoy/utils/authn_test.cc +++ b/src/envoy/utils/authn_test.cc @@ -15,7 +15,6 @@ #include "src/envoy/utils/authn.h" #include "common/protobuf/protobuf.h" -#include "envoy/http/header_map.h" #include "include/istio/utils/attribute_names.h" #include "src/istio/authn/context.pb.h" #include "test/test_common/utility.h" @@ -31,31 +30,9 @@ class AuthenticationTest : public testing::Test { test_result_.set_principal("foo"); test_result_.set_peer_user("bar"); } - - const Http::LowerCaseString& GetHeaderLocation() { - return Authentication::GetHeaderLocation(); - } - Http::TestHeaderMapImpl request_headers_{}; Result test_result_; }; -TEST_F(AuthenticationTest, SaveEmptyResult) { - EXPECT_FALSE(Authentication::HasResultInHeader(request_headers_)); - EXPECT_TRUE(Authentication::SaveResultToHeader(Result{}, &request_headers_)); - EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); - const auto entry = request_headers_.get(GetHeaderLocation()); - EXPECT_TRUE(entry != nullptr); - EXPECT_EQ("", entry->value().getStringView()); -} - -TEST_F(AuthenticationTest, SaveSomeResult) { - EXPECT_TRUE( - Authentication::SaveResultToHeader(test_result_, &request_headers_)); - const auto entry = request_headers_.get(GetHeaderLocation()); - EXPECT_TRUE(entry != nullptr); - EXPECT_EQ("CgNmb28SA2Jhcg==", entry->value().getStringView()); -} - TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { istio::authn::Result result; ::google::protobuf::Struct data; @@ -124,38 +101,38 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { "rawclaim"); } -TEST_F(AuthenticationTest, ResultAlreadyExist) { - request_headers_.addCopy(GetHeaderLocation(), "somedata"); - EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); - EXPECT_FALSE(Authentication::SaveResultToHeader(Result{}, &request_headers_)); - EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); - const auto entry = request_headers_.get(GetHeaderLocation()); - EXPECT_TRUE(entry != nullptr); - EXPECT_EQ("somedata", entry->value().getStringView()); -} - -TEST_F(AuthenticationTest, FetchResultNotExit) { - Result result; - EXPECT_FALSE( - Authentication::FetchResultFromHeader(request_headers_, &result)); -} - -TEST_F(AuthenticationTest, FetchResultBadFormat) { - request_headers_.addCopy(GetHeaderLocation(), "somedata"); - EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); - Result result; - EXPECT_FALSE( - Authentication::FetchResultFromHeader(request_headers_, &result)); -} - -TEST_F(AuthenticationTest, FetchResult) { - EXPECT_TRUE( - Authentication::SaveResultToHeader(test_result_, &request_headers_)); - Result fetch_result; - EXPECT_TRUE( - Authentication::FetchResultFromHeader(request_headers_, &fetch_result)); - EXPECT_TRUE(TestUtility::protoEqual(test_result_, fetch_result)); -} +// TEST_F(AuthenticationTest, ResultAlreadyExist) { +// request_headers_.addCopy(GetHeaderLocation(), "somedata"); +// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); +// EXPECT_FALSE(Authentication::SaveResultToHeader(Result{}, &request_headers_)); +// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); +// const auto entry = request_headers_.get(GetHeaderLocation()); +// EXPECT_TRUE(entry != nullptr); +// EXPECT_EQ("somedata", entry->value().getStringView()); +// } + +// TEST_F(AuthenticationTest, FetchResultNotExit) { +// Result result; +// EXPECT_FALSE( +// Authentication::FetchResultFromHeader(request_headers_, &result)); +// } + +// TEST_F(AuthenticationTest, FetchResultBadFormat) { +// request_headers_.addCopy(GetHeaderLocation(), "somedata"); +// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); +// Result result; +// EXPECT_FALSE( +// Authentication::FetchResultFromHeader(request_headers_, &result)); +// } + +// TEST_F(AuthenticationTest, FetchResult) { +// EXPECT_TRUE( +// Authentication::SaveResultToHeader(test_result_, &request_headers_)); +// Result fetch_result; +// EXPECT_TRUE( +// Authentication::FetchResultFromHeader(request_headers_, &fetch_result)); +// EXPECT_TRUE(TestUtility::protoEqual(test_result_, fetch_result)); +// } } // namespace Utils } // namespace Envoy diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 2650c196836..af6013834b7 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -15,6 +15,8 @@ #include "src/istio/control/http/attributes_builder.h" +#include + #include "include/istio/utils/attribute_names.h" #include "include/istio/utils/attributes_builder.h" #include "include/istio/utils/status.h" @@ -74,45 +76,34 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { builder.AddString(utils::AttributeName::kDestinationPrincipal, destination_principal); } - - istio::authn::Result authn_result; - if (check_data->GetAuthenticationResult(&authn_result)) { - if (!authn_result.principal().empty()) { - builder.AddString(utils::AttributeName::kRequestAuthPrincipal, - authn_result.principal()); - } - if (!authn_result.peer_user().empty()) { - // TODO(diemtvu): remove kSourceUser once migration to source.principal is - // over. https://github.com/istio/istio/issues/4689 - builder.AddString(utils::AttributeName::kSourceUser, - authn_result.peer_user()); - builder.AddString(utils::AttributeName::kSourcePrincipal, - authn_result.peer_user()); - } - if (authn_result.has_origin()) { - const auto &origin = authn_result.origin(); - if (!origin.audiences().empty()) { - // TODO(diemtvu): this should be send as repeated field once mixer - // support string_list (https://github.com/istio/istio/issues/2802) For - // now, just use the first value. - builder.AddString(utils::AttributeName::kRequestAuthAudiences, - origin.audiences(0)); - } - if (!origin.presenter().empty()) { - builder.AddString(utils::AttributeName::kRequestAuthPresenter, - origin.presenter()); - } - if (!origin.claims().empty()) { - builder.AddProtobufStringMap(utils::AttributeName::kRequestAuthClaims, - origin.claims()); - } - if (!origin.raw_claims().empty()) { - builder.AddString(utils::AttributeName::kRequestAuthRawClaims, - origin.raw_claims()); + static const std::set kAuthenticationStringAttributes = { + utils::AttributeName::kRequestAuthPrincipal, + utils::AttributeName::kSourceUser, + utils::AttributeName::kSourcePrincipal, + utils::AttributeName::kRequestAuthAudiences, + utils::AttributeName::kRequestAuthPresenter, + utils::AttributeName::kRequestAuthRawClaims, + }; + const auto* authn_result = check_data->GetAuthenticationResult(); + if (authn_result != nullptr) { + // Not all data in authentication results need to be sent to mixer (e.g + // groups), so we need to iterate on pre-approved attributes only. for + // (const auto &field : authn_result->fields()) { + for (const auto &attribute : kAuthenticationStringAttributes) { + const auto &iter = authn_result->fields().find(attribute); + if (iter != authn_result->fields().end() && + !iter->second.string_value().empty()) { + builder.AddString(attribute, iter->second.string_value()); } } + + // Add string-map attribute (kRequestAuthClaims) + const auto &claims = authn_result->fields().find(utils::AttributeName::kRequestAuthClaims); + if (claims != authn_result->fields().end()) { + builder.AddProtoStructStringMap(utils::AttributeName::kRequestAuthClaims, claims->second.struct_value()); + } } -} // namespace http +} void AttributesBuilder::ExtractForwardedAttributes(CheckData *check_data) { std::string forwarded_data; diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index a6f75b59ee7..f54d1a73146 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -75,6 +75,9 @@ constexpr char kBadToken[] = constexpr char kExpectedPrincipal[] = "testing@secure.istio.io/testing@secure.istio.io"; +constexpr char kExpectedRawClaims[] = + "{\"exp\":4685989700,\"foo\":\"bar\",\"iat\":1532389700,\"iss\":\"testing@secure.istio.io\"," + "\"sub\":\"testing@secure.istio.io\"}"; constexpr char kDestinationUID[] = "dest.pod.123"; constexpr char kSourceUID[] = "src.pod.xyz"; constexpr char kTelemetryBackend[] = "telemetry-backend"; @@ -318,8 +321,10 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { check_request.attributes().words(), ::testing::AllOf(Contains(kDestinationUID), Contains("10.0.0.1"), Contains(kExpectedPrincipal), + Contains(kExpectedRawClaims), Contains("testing@secure.istio.io"), Contains("sub"), - Contains("iss"), Contains("foo"), Contains("bar"))); + Contains("iss"), Contains("foo"), Contains("bar"), + Contains("c"))); sendPolicyResponse(); waitForNextUpstreamRequest(0); @@ -336,6 +341,7 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { report_request.default_words(), ::testing::AllOf(Contains(kDestinationUID), Contains("10.0.0.1"), Contains(kExpectedPrincipal), + Contains(kExpectedRawClaims), Contains("testing@secure.istio.io"), Contains("sub"), Contains("iss"), Contains("foo"), Contains("bar"))); sendTelemetryResponse(); From 325942d0f48999bcbce7ce467fbc7e7dbdee850c Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 16:06:52 -0700 Subject: [PATCH 2/8] Clean up comments. --- src/envoy/http/authn/http_filter.cc | 2 +- .../authn/http_filter_integration_test.cc | 35 ------------------- src/envoy/utils/authn_test.cc | 33 ----------------- 3 files changed, 1 insertion(+), 69 deletions(-) diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index bc34f0fef71..67669084c25 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -70,7 +70,7 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { // Put authentication result to headers. if (filter_context_ != nullptr) { - // Save auth results in the metadata, could be later used by RBAC filter. + // Save auth results in the metadata, could be used later by RBAC and/or mixer filter. ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); diff --git a/src/envoy/http/authn/http_filter_integration_test.cc b/src/envoy/http/authn/http_filter_integration_test.cc index 382c2572549..a404e07d2aa 100644 --- a/src/envoy/http/authn/http_filter_integration_test.cc +++ b/src/envoy/http/authn/http_filter_integration_test.cc @@ -163,41 +163,6 @@ TEST_P(AuthenticationFilterIntegrationTest, CheckValidJwtPassAuthentication) { response->waitForEndStream(); EXPECT_TRUE(response->complete()); EXPECT_STREQ("200", response->headers().Status()->value().c_str()); - -// // 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; -// EXPECT_TRUE(result.ParseFromString(value)); - -// 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" -// } -// 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 diff --git a/src/envoy/utils/authn_test.cc b/src/envoy/utils/authn_test.cc index 2ac9e59ed4e..cc97855cda9 100644 --- a/src/envoy/utils/authn_test.cc +++ b/src/envoy/utils/authn_test.cc @@ -101,38 +101,5 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { "rawclaim"); } -// TEST_F(AuthenticationTest, ResultAlreadyExist) { -// request_headers_.addCopy(GetHeaderLocation(), "somedata"); -// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); -// EXPECT_FALSE(Authentication::SaveResultToHeader(Result{}, &request_headers_)); -// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); -// const auto entry = request_headers_.get(GetHeaderLocation()); -// EXPECT_TRUE(entry != nullptr); -// EXPECT_EQ("somedata", entry->value().getStringView()); -// } - -// TEST_F(AuthenticationTest, FetchResultNotExit) { -// Result result; -// EXPECT_FALSE( -// Authentication::FetchResultFromHeader(request_headers_, &result)); -// } - -// TEST_F(AuthenticationTest, FetchResultBadFormat) { -// request_headers_.addCopy(GetHeaderLocation(), "somedata"); -// EXPECT_TRUE(Authentication::HasResultInHeader(request_headers_)); -// Result result; -// EXPECT_FALSE( -// Authentication::FetchResultFromHeader(request_headers_, &result)); -// } - -// TEST_F(AuthenticationTest, FetchResult) { -// EXPECT_TRUE( -// Authentication::SaveResultToHeader(test_result_, &request_headers_)); -// Result fetch_result; -// EXPECT_TRUE( -// Authentication::FetchResultFromHeader(request_headers_, &fetch_result)); -// EXPECT_TRUE(TestUtility::protoEqual(test_result_, fetch_result)); -// } - } // namespace Utils } // namespace Envoy From 3f0bf9e86652d5cd9a7e1e8f602f706f0b9de795 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 16:14:33 -0700 Subject: [PATCH 3/8] Lint --- include/istio/control/http/check_data.h | 6 ++--- src/envoy/http/authn/http_filter.cc | 3 ++- src/envoy/http/authn/http_filter_test.cc | 19 ++++++++----- src/envoy/http/mixer/check_data.cc | 2 +- src/envoy/http/mixer/check_data.h | 6 +++-- src/envoy/http/mixer/filter.cc | 7 +++-- src/envoy/utils/authn.h | 8 +++--- src/istio/control/http/attributes_builder.cc | 20 +++++++------- .../istio_http_integration_test.cc | 27 +++++++++---------- 9 files changed, 55 insertions(+), 43 deletions(-) diff --git a/include/istio/control/http/check_data.h b/include/istio/control/http/check_data.h index 426feaa623a..3e11bff7758 100644 --- a/include/istio/control/http/check_data.h +++ b/include/istio/control/http/check_data.h @@ -81,9 +81,9 @@ class CheckData { virtual bool FindCookie(const std::string &name, std::string *value) const = 0; - // Returns a pointer to the authentication result from request info dynamic metadata, if - // available. Otherwise, returns nullptrl - virtual const ::google::protobuf::Struct* GetAuthenticationResult() const = 0; + // Returns a pointer to the authentication result from request info dynamic + // metadata, if available. Otherwise, returns nullptrl + virtual const ::google::protobuf::Struct *GetAuthenticationResult() const = 0; }; // An interfact to update request HTTP headers with Istio attributes. diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index 67669084c25..f1f372fa960 100644 --- a/src/envoy/http/authn/http_filter.cc +++ b/src/envoy/http/authn/http_filter.cc @@ -70,7 +70,8 @@ FilterHeadersStatus AuthenticationFilter::decodeHeaders(HeaderMap&, bool) { // Put authentication result to headers. if (filter_context_ != nullptr) { - // Save auth results in the metadata, could be used later by RBAC and/or mixer filter. + // Save auth results in the metadata, could be used later by RBAC and/or + // mixer filter. ProtobufWkt::Struct data; Utils::Authentication::SaveAuthAttributesToStruct( filter_context_->authenticationResult(), data); diff --git a/src/envoy/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index 6a0eab43d0d..ae529f9d7d1 100644 --- a/src/envoy/http/authn/http_filter_test.cc +++ b/src/envoy/http/authn/http_filter_test.cc @@ -121,7 +121,8 @@ TEST_F(AuthenticationFilterTest, PeerFail) { .WillOnce(Invoke(createAlwaysFailAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_CALL(decoder_callbacks_, requestInfo()) - .Times(AtLeast(1)).WillRepeatedly(ReturnRef(request_info)); + .Times(AtLeast(1)) + .WillRepeatedly(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -129,7 +130,8 @@ TEST_F(AuthenticationFilterTest, PeerFail) { })); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers_, true)); - EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata(request_info.dynamicMetadata())); + EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata( + request_info.dynamicMetadata())); } TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { @@ -143,7 +145,8 @@ TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { .WillOnce(Invoke(createAlwaysFailAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_CALL(decoder_callbacks_, requestInfo()) - .Times(AtLeast(1)).WillRepeatedly(ReturnRef(request_info)); + .Times(AtLeast(1)) + .WillRepeatedly(ReturnRef(request_info)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) .WillOnce(testing::Invoke([](Http::HeaderMap &headers, bool) { @@ -151,7 +154,8 @@ TEST_F(AuthenticationFilterTest, PeerPassOrginFail) { })); EXPECT_EQ(Http::FilterHeadersStatus::StopIteration, filter_.decodeHeaders(request_headers_, true)); - EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata(request_info.dynamicMetadata())); + EXPECT_FALSE(Utils::Authentication::GetResultFromMetadata( + request_info.dynamicMetadata())); } TEST_F(AuthenticationFilterTest, AllPass) { @@ -163,14 +167,15 @@ TEST_F(AuthenticationFilterTest, AllPass) { .WillOnce(Invoke(createAlwaysPassAuthenticator)); RequestInfo::RequestInfoImpl request_info(Http::Protocol::Http2); EXPECT_CALL(decoder_callbacks_, requestInfo()) - .Times(AtLeast(1)).WillRepeatedly(ReturnRef(request_info)); + .Times(AtLeast(1)) + .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::GetResultFromMetadata(request_info.dynamicMetadata()); + const auto *data = Utils::Authentication::GetResultFromMetadata( + request_info.dynamicMetadata()); ASSERT_TRUE(data); ProtobufWkt::Struct expected_data; diff --git a/src/envoy/http/mixer/check_data.cc b/src/envoy/http/mixer/check_data.cc index e86fd0e793f..88f2f455e7f 100644 --- a/src/envoy/http/mixer/check_data.cc +++ b/src/envoy/http/mixer/check_data.cc @@ -168,7 +168,7 @@ bool CheckData::FindCookie(const std::string& name, std::string* value) const { } const ::google::protobuf::Struct* CheckData::GetAuthenticationResult() const { - return Utils::Authentication::GetResultFromMetadata(metadata_); + return Utils::Authentication::GetResultFromMetadata(metadata_); } } // namespace Mixer diff --git a/src/envoy/http/mixer/check_data.h b/src/envoy/http/mixer/check_data.h index 0695d91e6b6..ed5cce45509 100644 --- a/src/envoy/http/mixer/check_data.h +++ b/src/envoy/http/mixer/check_data.h @@ -19,9 +19,9 @@ #include "common/http/utility.h" #include "envoy/api/v2/core/base.pb.h" #include "envoy/http/header_map.h" +#include "google/protobuf/struct.pb.h" #include "include/istio/control/http/controller.h" #include "src/istio/authn/context.pb.h" -#include "google/protobuf/struct.pb.h" namespace Envoy { namespace Http { @@ -30,7 +30,9 @@ namespace Mixer { class CheckData : public ::istio::control::http::CheckData, public Logger::Loggable { public: - CheckData(const HeaderMap& headers, const envoy::api::v2::core::Metadata& metadata, const Network::Connection* connection); + CheckData(const HeaderMap& headers, + const envoy::api::v2::core::Metadata& metadata, + const Network::Connection* connection); // Find "x-istio-attributes" headers, if found base64 decode // its value and remove it from the headers. diff --git a/src/envoy/http/mixer/filter.cc b/src/envoy/http/mixer/filter.cc index 5d5ac20d81d..8c55fd4552a 100644 --- a/src/envoy/http/mixer/filter.cc +++ b/src/envoy/http/mixer/filter.cc @@ -131,7 +131,9 @@ FilterHeadersStatus Filter::decodeHeaders(HeaderMap& headers, bool) { state_ = Calling; initiating_call_ = true; - CheckData check_data(headers, decoder_callbacks_->requestInfo().dynamicMetadata(), decoder_callbacks_->connection()); + CheckData check_data(headers, + decoder_callbacks_->requestInfo().dynamicMetadata(), + decoder_callbacks_->connection()); Utils::HeaderUpdate header_update(&headers); headers_ = &headers; cancel_check_ = handler_->Check( @@ -276,7 +278,8 @@ void Filter::log(const HeaderMap* request_headers, ReadPerRouteConfig(request_info.routeEntry(), &config); handler_ = control_.controller()->CreateRequestHandler(config); - CheckData check_data(*request_headers, request_info.dynamicMetadata(), nullptr); + CheckData check_data(*request_headers, request_info.dynamicMetadata(), + nullptr); handler_->ExtractRequestAttributes(&check_data); } // response trailer header is not counted to response total size. diff --git a/src/envoy/utils/authn.h b/src/envoy/utils/authn.h index 18f340eb1ce..dad2300041f 100644 --- a/src/envoy/utils/authn.h +++ b/src/envoy/utils/authn.h @@ -24,14 +24,14 @@ namespace Utils { class Authentication : public Logger::Loggable { public: - // Save authentication attributes into the data Struct. static void SaveAuthAttributesToStruct(const istio::authn::Result& result, ::google::protobuf::Struct& data); - // Returns a pointer to the authentication result from metadata. Typically, the input metadata is - // the request info's dynamic metadata. Authentication result, if available, is stored under - // authentication filter metdata. Returns nullptr if there is no data for that filter. + // Returns a pointer to the authentication result from metadata. Typically, + // the input metadata is the request info's dynamic metadata. Authentication + // result, if available, is stored under authentication filter metdata. + // Returns nullptr if there is no data for that filter. static const ProtobufWkt::Struct* GetResultFromMetadata( const envoy::api::v2::core::Metadata& metadata); }; diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index af6013834b7..600fc8dace8 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -77,14 +77,14 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { destination_principal); } static const std::set kAuthenticationStringAttributes = { - utils::AttributeName::kRequestAuthPrincipal, - utils::AttributeName::kSourceUser, - utils::AttributeName::kSourcePrincipal, - utils::AttributeName::kRequestAuthAudiences, - utils::AttributeName::kRequestAuthPresenter, - utils::AttributeName::kRequestAuthRawClaims, + utils::AttributeName::kRequestAuthPrincipal, + utils::AttributeName::kSourceUser, + utils::AttributeName::kSourcePrincipal, + utils::AttributeName::kRequestAuthAudiences, + utils::AttributeName::kRequestAuthPresenter, + utils::AttributeName::kRequestAuthRawClaims, }; - const auto* authn_result = check_data->GetAuthenticationResult(); + const auto *authn_result = check_data->GetAuthenticationResult(); if (authn_result != nullptr) { // Not all data in authentication results need to be sent to mixer (e.g // groups), so we need to iterate on pre-approved attributes only. for @@ -98,9 +98,11 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { } // Add string-map attribute (kRequestAuthClaims) - const auto &claims = authn_result->fields().find(utils::AttributeName::kRequestAuthClaims); + const auto &claims = + authn_result->fields().find(utils::AttributeName::kRequestAuthClaims); if (claims != authn_result->fields().end()) { - builder.AddProtoStructStringMap(utils::AttributeName::kRequestAuthClaims, claims->second.struct_value()); + builder.AddProtoStructStringMap(utils::AttributeName::kRequestAuthClaims, + claims->second.struct_value()); } } } diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index f54d1a73146..1b5c7683193 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -76,7 +76,8 @@ constexpr char kBadToken[] = constexpr char kExpectedPrincipal[] = "testing@secure.istio.io/testing@secure.istio.io"; constexpr char kExpectedRawClaims[] = - "{\"exp\":4685989700,\"foo\":\"bar\",\"iat\":1532389700,\"iss\":\"testing@secure.istio.io\"," + "{\"exp\":4685989700,\"foo\":\"bar\",\"iat\":1532389700,\"iss\":\"testing@" + "secure.istio.io\"," "\"sub\":\"testing@secure.istio.io\"}"; constexpr char kDestinationUID[] = "dest.pod.123"; constexpr char kSourceUID[] = "src.pod.xyz"; @@ -319,12 +320,11 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { // Check request should see authn attributes. EXPECT_THAT( check_request.attributes().words(), - ::testing::AllOf(Contains(kDestinationUID), Contains("10.0.0.1"), - Contains(kExpectedPrincipal), - Contains(kExpectedRawClaims), - Contains("testing@secure.istio.io"), Contains("sub"), - Contains("iss"), Contains("foo"), Contains("bar"), - Contains("c"))); + ::testing::AllOf( + Contains(kDestinationUID), Contains("10.0.0.1"), + Contains(kExpectedPrincipal), Contains(kExpectedRawClaims), + Contains("testing@secure.istio.io"), Contains("sub"), Contains("iss"), + Contains("foo"), Contains("bar"), Contains("c"))); sendPolicyResponse(); waitForNextUpstreamRequest(0); @@ -337,13 +337,12 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { ::istio::mixer::v1::ReportRequest report_request; waitForTelemetryRequest(&report_request); // Report request should also see the same authn attributes. - EXPECT_THAT( - report_request.default_words(), - ::testing::AllOf(Contains(kDestinationUID), Contains("10.0.0.1"), - Contains(kExpectedPrincipal), - Contains(kExpectedRawClaims), - Contains("testing@secure.istio.io"), Contains("sub"), - Contains("iss"), Contains("foo"), Contains("bar"))); + EXPECT_THAT(report_request.default_words(), + ::testing::AllOf( + Contains(kDestinationUID), Contains("10.0.0.1"), + Contains(kExpectedPrincipal), Contains(kExpectedRawClaims), + Contains("testing@secure.istio.io"), Contains("sub"), + Contains("iss"), Contains("foo"), Contains("bar"))); sendTelemetryResponse(); EXPECT_TRUE(response->complete()); From 1357d76d488a24471f0ae07ae2caa9624ba72f4e Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 16:43:36 -0700 Subject: [PATCH 4/8] Reviews --- include/istio/control/http/check_data.h | 2 +- include/istio/utils/attributes_builder.h | 7 +++++-- src/istio/control/http/attributes_builder.cc | 3 +-- 3 files changed, 7 insertions(+), 5 deletions(-) diff --git a/include/istio/control/http/check_data.h b/include/istio/control/http/check_data.h index 3e11bff7758..10889524518 100644 --- a/include/istio/control/http/check_data.h +++ b/include/istio/control/http/check_data.h @@ -82,7 +82,7 @@ class CheckData { std::string *value) const = 0; // Returns a pointer to the authentication result from request info dynamic - // metadata, if available. Otherwise, returns nullptrl + // metadata, if available. Otherwise, returns nullptr. virtual const ::google::protobuf::Struct *GetAuthenticationResult() const = 0; }; diff --git a/include/istio/utils/attributes_builder.h b/include/istio/utils/attributes_builder.h index afcebaeeaab..ae934988abe 100644 --- a/include/istio/utils/attributes_builder.h +++ b/include/istio/utils/attributes_builder.h @@ -100,8 +100,11 @@ class AttributesBuilder { entries->clear(); for (const auto& field : struct_map.fields()) { // Ignore all fields that are not string. - if (!field.second.string_value().empty()) { - (*entries)[field.first] = field.second.string_value(); + switch (field.second.kind_case()) { + case google::protobuf::Value::kStringValue: + (*entries)[field.first] = field.second.string_value(); + break; + default: break; } } } diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 600fc8dace8..43be284049a 100644 --- a/src/istio/control/http/attributes_builder.cc +++ b/src/istio/control/http/attributes_builder.cc @@ -87,8 +87,7 @@ void AttributesBuilder::ExtractAuthAttributes(CheckData *check_data) { const auto *authn_result = check_data->GetAuthenticationResult(); if (authn_result != nullptr) { // Not all data in authentication results need to be sent to mixer (e.g - // groups), so we need to iterate on pre-approved attributes only. for - // (const auto &field : authn_result->fields()) { + // groups), so we need to iterate on pre-approved attributes only. for (const auto &attribute : kAuthenticationStringAttributes) { const auto &iter = authn_result->fields().find(attribute); if (iter != authn_result->fields().end() && From 9e80b592941f311201abd4066440d65614741277 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 17:00:12 -0700 Subject: [PATCH 5/8] Lint --- include/istio/utils/attributes_builder.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/include/istio/utils/attributes_builder.h b/include/istio/utils/attributes_builder.h index ae934988abe..eae9836f4d0 100644 --- a/include/istio/utils/attributes_builder.h +++ b/include/istio/utils/attributes_builder.h @@ -104,7 +104,8 @@ class AttributesBuilder { case google::protobuf::Value::kStringValue: (*entries)[field.first] = field.second.string_value(); break; - default: break; + default: + break; } } } From 53c009488ca3a169804d886cc033f157e1b9d616 Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 17:43:11 -0700 Subject: [PATCH 6/8] Fix test --- .../control/http/attributes_builder_test.cc | 132 +++++++++++++++--- src/istio/control/http/mock_check_data.h | 4 +- 2 files changed, 112 insertions(+), 24 deletions(-) diff --git a/src/istio/control/http/attributes_builder_test.cc b/src/istio/control/http/attributes_builder_test.cc index 7a0f7c31c12..677cfb885a1 100644 --- a/src/istio/control/http/attributes_builder_test.cc +++ b/src/istio/control/http/attributes_builder_test.cc @@ -40,9 +40,7 @@ namespace { MATCHER_P(EqualsAttribute, expected, "") { const auto matched = MessageDifferencer::Equals(arg, expected); if (!matched) { - std::string out_str; - TextFormat::PrintToString(arg, &out_str); - GOOGLE_LOG(INFO) << "\n===" << out_str << "==="; + GOOGLE_LOG(INFO) << arg.DebugString() << " vs " << expected.DebugString(); } return matched; } @@ -344,6 +342,107 @@ attributes { } )"; +constexpr char kAuthenticationResultStruct[] = R"( +fields { + key: "request.auth.audiences" + value { + string_value: "thisisaud" + } +} +fields { + key: "request.auth.claims" + value { + struct_value { + fields { + key: "iss" + value { + string_value: "thisisiss" + } + } + fields { + key: "sub" + value { + string_value: "thisissub" + } + } + fields { + key: "aud" + value { + string_value: "thisisaud" + } + } + fields { + key: "azp" + value { + string_value: "thisisazp" + } + } + fields { + key: "email" + value { + string_value: "thisisemail@email.com" + } + } + fields { + key: "iat" + value { + string_value: "1512754205" + } + } + fields { + key: "exp" + value { + string_value: "5112754205" + } + } + } + } +} +fields { + key: "request.auth.groups" + value { + list_value { + values { + string_value: "group1" + } + values { + string_value: "group2" + } + } + } +} +fields { + key: "request.auth.presenter" + value { + string_value: "thisisazp" + } +} +fields { + key: "request.auth.principal" + value { + string_value: "thisisiss/thisissub" + } +} +fields { + key: "request.auth.raw_claims" + value { + string_value: "test_raw_claims" + } +} +fields { + key: "source.principal" + value { + string_value: "test_user" + } +} +fields { + key: "source.user" + value { + string_value: "test_user" + } +} +)"; + void ClearContextTime(const std::string &name, RequestContext *request) { // Override timestamp with - utils::AttributesBuilder builder(&request->attributes); @@ -436,8 +535,8 @@ TEST(AttributesBuilderTest, TestCheckAttributesWithoutAuthnFilter) { } return false; })); - EXPECT_CALL(mock_data, GetAuthenticationResult(_)) - .WillOnce(testing::Return(false)); + EXPECT_CALL(mock_data, GetAuthenticationResult()) + .WillOnce(testing::Return(nullptr)); RequestContext request; AttributesBuilder builder(&request); @@ -495,23 +594,12 @@ TEST(AttributesBuilderTest, TestCheckAttributes) { } return false; })); - EXPECT_CALL(mock_data, GetAuthenticationResult(_)) - .WillOnce(Invoke([](istio::authn::Result *result) -> bool { - result->set_principal("thisisiss/thisissub"); - result->set_peer_user("test_user"); - result->mutable_origin()->add_audiences("thisisaud"); - result->mutable_origin()->set_presenter("thisisazp"); - (*result->mutable_origin()->mutable_claims())["iss"] = "thisisiss"; - (*result->mutable_origin()->mutable_claims())["sub"] = "thisissub"; - (*result->mutable_origin()->mutable_claims())["aud"] = "thisisaud"; - (*result->mutable_origin()->mutable_claims())["azp"] = "thisisazp"; - (*result->mutable_origin()->mutable_claims())["email"] = - "thisisemail@email.com"; - (*result->mutable_origin()->mutable_claims())["iat"] = "1512754205"; - (*result->mutable_origin()->mutable_claims())["exp"] = "5112754205"; - result->mutable_origin()->set_raw_claims("test_raw_claims"); - return true; - })); + google::protobuf::Struct authn_result; + ASSERT_TRUE( + TextFormat::ParseFromString(kAuthenticationResultStruct, &authn_result)); + + EXPECT_CALL(mock_data, GetAuthenticationResult()) + .WillOnce(testing::Return(&authn_result)); RequestContext request; AttributesBuilder builder(&request); diff --git a/src/istio/control/http/mock_check_data.h b/src/istio/control/http/mock_check_data.h index 399b15457c4..58d5cbabfa3 100644 --- a/src/istio/control/http/mock_check_data.h +++ b/src/istio/control/http/mock_check_data.h @@ -18,6 +18,7 @@ #include "gmock/gmock.h" #include "include/istio/control/http/check_data.h" +#include "google/protobuf/struct.pb.h" namespace istio { namespace control { @@ -41,8 +42,7 @@ class MockCheckData : public CheckData { bool(const std::string &name, std::string *value)); MOCK_CONST_METHOD1(GetJWTPayload, bool(std::map *payload)); - MOCK_CONST_METHOD1(GetAuthenticationResult, - bool(istio::authn::Result *result)); + MOCK_CONST_METHOD0(GetAuthenticationResult, const ::google::protobuf::Struct*()); MOCK_CONST_METHOD0(IsMutualTLS, bool()); MOCK_CONST_METHOD1(GetRequestedServerName, bool(std::string *name)); }; From 16cf731e4194e67a41db3f8f56505cae4515ce0b Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 21:14:22 -0700 Subject: [PATCH 7/8] Remove mis-type --- test/integration/istio_http_integration_test.cc | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index 1b5c7683193..8d11925592a 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -324,7 +324,7 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { Contains(kDestinationUID), Contains("10.0.0.1"), Contains(kExpectedPrincipal), Contains(kExpectedRawClaims), Contains("testing@secure.istio.io"), Contains("sub"), Contains("iss"), - Contains("foo"), Contains("bar"), Contains("c"))); + Contains("foo"), Contains("bar"))); sendPolicyResponse(); waitForNextUpstreamRequest(0); From 9c0f2a45c80d9d15e847efd47443349b4bdd3f1f Mon Sep 17 00:00:00 2001 From: Diem Vu Date: Wed, 8 Aug 2018 22:37:39 -0700 Subject: [PATCH 8/8] Lint --- src/istio/control/http/mock_check_data.h | 5 +++-- test/integration/istio_http_integration_test.cc | 13 ++++++------- 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/istio/control/http/mock_check_data.h b/src/istio/control/http/mock_check_data.h index 58d5cbabfa3..f85e1487c3b 100644 --- a/src/istio/control/http/mock_check_data.h +++ b/src/istio/control/http/mock_check_data.h @@ -17,8 +17,8 @@ #define ISTIO_CONTROL_HTTP_MOCK_CHECK_DATA_H #include "gmock/gmock.h" -#include "include/istio/control/http/check_data.h" #include "google/protobuf/struct.pb.h" +#include "include/istio/control/http/check_data.h" namespace istio { namespace control { @@ -42,7 +42,8 @@ class MockCheckData : public CheckData { bool(const std::string &name, std::string *value)); MOCK_CONST_METHOD1(GetJWTPayload, bool(std::map *payload)); - MOCK_CONST_METHOD0(GetAuthenticationResult, const ::google::protobuf::Struct*()); + MOCK_CONST_METHOD0(GetAuthenticationResult, + const ::google::protobuf::Struct *()); MOCK_CONST_METHOD0(IsMutualTLS, bool()); MOCK_CONST_METHOD1(GetRequestedServerName, bool(std::string *name)); }; diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index 8d11925592a..60447eb9001 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -318,13 +318,12 @@ TEST_P(IstioHttpIntegrationTest, GoodJwt) { ::istio::mixer::v1::CheckRequest check_request; waitForPolicyRequest(&check_request); // Check request should see authn attributes. - EXPECT_THAT( - check_request.attributes().words(), - ::testing::AllOf( - Contains(kDestinationUID), Contains("10.0.0.1"), - Contains(kExpectedPrincipal), Contains(kExpectedRawClaims), - Contains("testing@secure.istio.io"), Contains("sub"), Contains("iss"), - Contains("foo"), Contains("bar"))); + EXPECT_THAT(check_request.attributes().words(), + ::testing::AllOf( + Contains(kDestinationUID), Contains("10.0.0.1"), + Contains(kExpectedPrincipal), Contains(kExpectedRawClaims), + Contains("testing@secure.istio.io"), Contains("sub"), + Contains("iss"), Contains("foo"), Contains("bar"))); sendPolicyResponse(); waitForNextUpstreamRequest(0);