diff --git a/include/istio/control/http/check_data.h b/include/istio/control/http/check_data.h index 318f409b717..10889524518 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 nullptr. + 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..eae9836f4d0 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,24 @@ 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. + switch (field.second.kind_case()) { + case google::protobuf::Value::kStringValue: + (*entries)[field.first] = field.second.string_value(); + break; + default: + break; + } } } diff --git a/src/envoy/http/authn/http_filter.cc b/src/envoy/http/authn/http_filter.cc index ddb0ed4f680..f1f372fa960 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,11 +70,8 @@ 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. + // 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 0ba682b5007..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/http/authn/http_filter_test.cc b/src/envoy/http/authn/http_filter_test.cc index eab12be2dc7..ae529f9d7d1 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,6 +121,7 @@ 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)); EXPECT_CALL(decoder_callbacks_, encodeHeaders_(_, _)) .Times(1) @@ -128,7 +130,8 @@ 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 +143,10 @@ 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 +154,8 @@ 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 +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)); 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::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..88f2f455e7f 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..ed5cce45509 100644 --- a/src/envoy/http/mixer/check_data.h +++ b/src/envoy/http/mixer/check_data.h @@ -17,7 +17,9 @@ #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 "google/protobuf/struct.pb.h" #include "include/istio/control/http/controller.h" #include "src/istio/authn/context.pb.h" @@ -28,7 +30,9 @@ 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 +60,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..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_->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 +211,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 +278,8 @@ 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..dad2300041f 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..cc97855cda9 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,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 diff --git a/src/istio/control/http/attributes_builder.cc b/src/istio/control/http/attributes_builder.cc index 2650c196836..43be284049a 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,35 @@ 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 &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/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..f85e1487c3b 100644 --- a/src/istio/control/http/mock_check_data.h +++ b/src/istio/control/http/mock_check_data.h @@ -17,6 +17,7 @@ #define ISTIO_CONTROL_HTTP_MOCK_CHECK_DATA_H #include "gmock/gmock.h" +#include "google/protobuf/struct.pb.h" #include "include/istio/control/http/check_data.h" namespace istio { @@ -41,8 +42,8 @@ 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)); }; diff --git a/test/integration/istio_http_integration_test.cc b/test/integration/istio_http_integration_test.cc index a6f75b59ee7..60447eb9001 100644 --- a/test/integration/istio_http_integration_test.cc +++ b/test/integration/istio_http_integration_test.cc @@ -75,6 +75,10 @@ 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"; @@ -314,12 +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("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); @@ -332,12 +336,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("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());