istio · rshriram · Aug 9, 2018 · Aug 8, 2018 · Aug 8, 2018 · Aug 8, 2018
@@ -19,7 +19,7 @@
 #include <map>
 #include <string>
 
-#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<std::string, std::string> *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.

@@ -20,7 +20,7 @@
 #include <map>
 #include <string>
 
-#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<std::string, ::std::string>& 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();
+      }
     }
   }
 

@@ -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);

@@ -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

@@ -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,14 +143,19 @@ 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) {
         EXPECT_STREQ("401", headers.Status()->value().c_str());
       }));
   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;

@@ -38,8 +38,9 @@ const std::set<std::string> 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<std::string, std::string>* 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

@@ -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<Logger::Id::filter> {
  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<std::string, std::string>* 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_;
 };

@@ -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.

@@ -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