Authn uses protobuf.Struct to store claims and add list support for R…

…BAC (#1925)

* Authn uses protobuf.Struct to store claims and add list support for RBAC

- Change authn to use protobuf.Struct to store claims
- Add list support for RBAC

* Change based on the review comments

include/istio/utils/attribute_names.h

@@ -91,7 +91,6 @@ struct AttributeName {
   // Authentication attributes
   static const char kRequestAuthPrincipal[];
   static const char kRequestAuthAudiences[];
-  static const char kRequestAuthGroups[];
   static const char kRequestAuthPresenter[];
   static const char kRequestAuthClaims[];
   static const char kRequestAuthRawClaims[];

include/istio/utils/attributes_builder.h

@@ -99,11 +99,25 @@ class AttributesBuilder {
                        ->mutable_entries();
     entries->clear();
     for (const auto& field : struct_map.fields()) {
-      // Ignore all fields that are not string.
+      // Ignore all fields that are not string or string list.
       switch (field.second.kind_case()) {
         case google::protobuf::Value::kStringValue:
           (*entries)[field.first] = field.second.string_value();
           break;
+        case google::protobuf::Value::kListValue:
+          if (field.second.list_value().values_size() > 0) {
+            // The items in the list is converted into a
+            // comma separated string
+            std::string s;
+            for (int i = 0; i < field.second.list_value().values_size(); i++) {
+              s += field.second.list_value().values().Get(i).string_value();
+              if (i + 1 < field.second.list_value().values_size()) {
+                s += ",";
+              }
+            }
+            (*entries)[field.first] = s;
+          }
+          break;
         default:
           break;
       }

src/envoy/http/authn/authenticator_base_test.cc

@@ -44,7 +44,7 @@ const std::string kSecIstioAuthUserinfoHeaderValue =
      {
        "iss": "[email protected]",
        "sub": "[email protected]",
-       "aud": "aud1",
+       "aud": ["aud1", "aud2"],
        "non-string-will-be-ignored": 1512754205,
        "some-other-string-claims": "some-claims-kept"
      }
@@ -273,15 +273,15 @@ TEST_F(ValidateJwtTest, JwtPayloadAvailable) {
       R"({
              "jwt": {
                "user": "[email protected]/[email protected]",
-               "audiences": ["aud1"],
+               "audiences": ["aud1", "aud2"],
                "presenter": "",
                "claims": {
-                 "aud": "aud1",
-                 "iss": "[email protected]",
-                 "sub": "[email protected]",
-                 "some-other-string-claims": "some-claims-kept"
+                 "aud": ["aud1", "aud2"],
+                 "iss": ["[email protected]"],
+                 "some-other-string-claims": ["some-claims-kept"],
+                 "sub": ["[email protected]"],
                },
-               raw_claims: "\n     {\n       \"iss\": \"[email protected]\",\n       \"sub\": \"[email protected]\",\n       \"aud\": \"aud1\",\n       \"non-string-will-be-ignored\": 1512754205,\n       \"some-other-string-claims\": \"some-claims-kept\"\n     }\n   "
+               "raw_claims": "\n     {\n       \"iss\": \"[email protected]\",\n       \"sub\": \"[email protected]\",\n       \"aud\": [\"aud1\", \"aud2\"],\n       \"non-string-will-be-ignored\": 1512754205,\n       \"some-other-string-claims\": \"some-claims-kept\"\n     }\n   ",
              }
            }
         )",

src/envoy/http/authn/authn_utils.cc

@@ -15,6 +15,7 @@
 
 #include "authn_utils.h"
 #include "common/json/json_loader.h"
+#include "google/protobuf/struct.pb.h"
 #include "src/envoy/http/jwt_auth/jwt.h"
 
 namespace Envoy {
@@ -25,51 +26,23 @@ namespace {
 // The JWT audience key name
 static const std::string kJwtAudienceKey = "aud";
 
-// The JWT groups key name
-static const std::string kJwtGroupsKey = "groups";
-
-// Extract JWT audience into the JwtPayload.
-// This function should to be called after the claims are extracted.
-void ExtractJwtAudience(
-    const Envoy::Json::Object& obj,
-    const ::google::protobuf::Map< ::std::string, ::std::string>& claims,
-    istio::authn::JwtPayload* payload) {
-  const std::string& key = kJwtAudienceKey;
-  // "aud" can be either string array or string.
+// Extract JWT claim as a string list.
+// This function only extracts string and string list claims.
+// A string claim is extracted as a string list of 1 item.
+void ExtractStringList(const std::string& key, const Envoy::Json::Object& obj,
+                       std::vector<std::string>* list) {
   // First, try as string
-  if (claims.count(key) > 0) {
-    payload->add_audiences(claims.at(key));
-    return;
-  }
-  // Next, try as string array
   try {
-    std::vector<std::string> aud_vector = obj.getStringArray(key);
-    for (const std::string aud : aud_vector) {
-      payload->add_audiences(aud);
-    }
+    // Try as string, will throw execption if object type is not string.
+    list->push_back(obj.getString(key));
   } catch (Json::Exception& e) {
-    // Not convertable to string array
-  }
-}
-
-// Extract JWT groups into the JwtPayload.
-// This function should to be called after the claims are extracted.
-void ExtractJwtGroups(
-    const Envoy::Json::Object& obj,
-    const ::google::protobuf::Map< ::std::string, ::std::string>& claims,
-    istio::authn::JwtPayload* payload) {
-  const std::string& key = kJwtGroupsKey;
-  // "groups" can be either string array or string.
-  // First, try as string
-  if (claims.count(key) > 0) {
-    payload->add_groups(claims.at(key));
-    return;
+    // Not convertable to string
   }
   // Next, try as string array
   try {
-    std::vector<std::string> group_vector = obj.getStringArray(key);
-    for (const std::string group : group_vector) {
-      payload->add_groups(group);
+    std::vector<std::string> vector = obj.getStringArray(key);
+    for (const std::string v : vector) {
+      list->push_back(v);
     }
   } catch (Json::Exception& e) {
     // Not convertable to string array
@@ -89,37 +62,39 @@ bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str,
   }
 
   *payload->mutable_raw_claims() = payload_str;
-  ::google::protobuf::Map< ::std::string, ::std::string>* claims =
-      payload->mutable_claims();
 
-  // Extract claims
-  json_obj->iterate(
-      [payload](const std::string& key, const Json::Object& obj) -> bool {
-        ::google::protobuf::Map< ::std::string, ::std::string>* claims =
-            payload->mutable_claims();
-        // In current implementation, only string objects are extracted into
-        // claims. If call obj.asJsonString(), will get "panic: not reached"
-        // from json_loader.cc.
-        try {
-          // Try as string, will throw execption if object type is not string.
-          (*claims)[key] = obj.asString();
-        } catch (Json::Exception& e) {
-        }
-        return true;
-      });
-  // Extract audience
-  // ExtractJwtAudience() should be called after claims are extracted.
-  ExtractJwtAudience(*json_obj, payload->claims(), payload);
-  // ExtractJwtGroups() should be called after claims are extracted.
-  ExtractJwtGroups(*json_obj, payload->claims(), payload);
+  auto claims = payload->mutable_claims()->mutable_fields();
+  // Extract claims as string lists
+  json_obj->iterate([json_obj, claims](const std::string& key,
+                                       const Json::Object&) -> bool {
+    // In current implementation, only string/string list objects are extracted
+    std::vector<std::string> list;
+    ExtractStringList(key, *json_obj, &list);
+    for (auto s : list) {
+      (*claims)[key].mutable_list_value()->add_values()->set_string_value(s);
+    }
+    return true;
+  });
+  // Copy audience to the audience in context.proto
+  if (claims->find(kJwtAudienceKey) != claims->end()) {
+    for (const auto& v : (*claims)[kJwtAudienceKey].list_value().values()) {
+      payload->add_audiences(v.string_value());
+    }
+  }
+
   // Build user
-  if (claims->count("iss") > 0 && claims->count("sub") > 0) {
-    payload->set_user((*claims)["iss"] + "/" + (*claims)["sub"]);
+  if (claims->find("iss") != claims->end() &&
+      claims->find("sub") != claims->end()) {
+    payload->set_user(
+        (*claims)["iss"].list_value().values().Get(0).string_value() + "/" +
+        (*claims)["sub"].list_value().values().Get(0).string_value());
   }
   // Build authorized presenter (azp)
-  if (claims->count("azp") > 0) {
-    payload->set_presenter((*claims)["azp"]);
+  if (claims->find("azp") != claims->end()) {
+    payload->set_presenter(
+        (*claims)["azp"].list_value().values().Get(0).string_value());
   }
+
   return true;
 }
 

src/envoy/http/authn/authn_utils_test.cc

@@ -63,21 +63,47 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) {
       R"(
       user: "[email protected]/[email protected]"
       audiences: ["aud1"]
-      claims {
-        key: "aud"
-        value: "aud1"
-      }
-      claims {
-        key: "iss"
-        value: "[email protected]"
-      }
-      claims {
-        key: "sub"
-        value: "[email protected]"
-      }
-      claims {
-        key: "some-other-string-claims"
-        value: "some-claims-kept"
+      claims: {
+        fields: {
+          key: "aud"
+          value: {
+            list_value: {
+              values: {
+                string_value: "aud1"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "iss"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "sub"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "some-other-string-claims"
+          value: {
+            list_value: {
+              values: {
+                string_value: "some-claims-kept"
+              }
+            }
+          }
+        }
       }
       raw_claims: ")" +
           StringUtil::escape(kSecIstioAuthUserinfoHeaderValue) + R"(")",
@@ -95,17 +121,37 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithNoAudTest) {
   ASSERT_TRUE(Protobuf::TextFormat::ParseFromString(
       R"(
       user: "[email protected]/[email protected]"
-      claims {
-        key: "iss"
-        value: "[email protected]"
-      }
-      claims {
-        key: "sub"
-        value: "[email protected]"
-      }
-      claims {
-        key: "some-other-string-claims"
-        value: "some-claims-kept"
+      claims: {
+        fields: {
+          key: "iss"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "sub"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "some-other-string-claims"
+          value: {
+            list_value: {
+              values: {
+                string_value: "some-claims-kept"
+              }
+            }
+          }
+        }
       }
       raw_claims: ")" +
           StringUtil::escape(kSecIstioAuthUserInfoHeaderWithNoAudValue) +
@@ -127,28 +173,61 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithTwoAudTest) {
       user: "[email protected]/[email protected]"
       audiences: "aud1"
       audiences: "aud2"
-      claims {
-        key: "iss"
-        value: "[email protected]"
-      }
-      claims {
-        key: "sub"
-        value: "[email protected]"
-      }
-      claims {
-        key: "some-other-string-claims"
-        value: "some-claims-kept"
+      claims: {
+        fields: {
+          key: "aud"
+          value: {
+            list_value: {
+              values: {
+                string_value: "aud1"
+              }
+              values: {
+                string_value: "aud2"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "iss"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "sub"
+          value: {
+            list_value: {
+              values: {
+                string_value: "[email protected]"
+              }
+            }
+          }
+        }
+        fields: {
+          key: "some-other-string-claims"
+          value: {
+            list_value: {
+              values: {
+                string_value: "some-claims-kept"
+              }
+            }
+          }
+        }
       }
       raw_claims: ")" +
           StringUtil::escape(kSecIstioAuthUserInfoHeaderWithTwoAudValue) +
           R"(")",
       &expected_payload));
-
   // The payload returned from ProcessJwtPayload() should be the same as
   // the expected. When the aud is a string array, the aud is not saved in the
   // claims.
   bool ret = AuthnUtils::ProcessJwtPayload(
       kSecIstioAuthUserInfoHeaderWithTwoAudValue, &payload);
+
   EXPECT_TRUE(ret);
   EXPECT_TRUE(MessageDifferencer::Equals(expected_payload, payload));
 }