diff --git a/include/istio/utils/attribute_names.h b/include/istio/utils/attribute_names.h index 67c6df64c98..e2ed204a4e6 100644 --- a/include/istio/utils/attribute_names.h +++ b/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[]; diff --git a/include/istio/utils/attributes_builder.h b/include/istio/utils/attributes_builder.h index eae9836f4d0..82e71e89747 100644 --- a/include/istio/utils/attributes_builder.h +++ b/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; } diff --git a/src/envoy/http/authn/authenticator_base_test.cc b/src/envoy/http/authn/authenticator_base_test.cc index 068e5db1edb..39662318296 100644 --- a/src/envoy/http/authn/authenticator_base_test.cc +++ b/src/envoy/http/authn/authenticator_base_test.cc @@ -44,7 +44,7 @@ const std::string kSecIstioAuthUserinfoHeaderValue = { "iss": "issuer@foo.com", "sub": "sub@foo.com", - "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": "issuer@foo.com/sub@foo.com", - "audiences": ["aud1"], + "audiences": ["aud1", "aud2"], "presenter": "", "claims": { - "aud": "aud1", - "iss": "issuer@foo.com", - "sub": "sub@foo.com", - "some-other-string-claims": "some-claims-kept" + "aud": ["aud1", "aud2"], + "iss": ["issuer@foo.com"], + "some-other-string-claims": ["some-claims-kept"], + "sub": ["sub@foo.com"], }, - raw_claims: "\n {\n \"iss\": \"issuer@foo.com\",\n \"sub\": \"sub@foo.com\",\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\": \"issuer@foo.com\",\n \"sub\": \"sub@foo.com\",\n \"aud\": [\"aud1\", \"aud2\"],\n \"non-string-will-be-ignored\": 1512754205,\n \"some-other-string-claims\": \"some-claims-kept\"\n }\n ", } } )", diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 952114b9810..88d04327b94 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/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* 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 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 group_vector = obj.getStringArray(key); - for (const std::string group : group_vector) { - payload->add_groups(group); + std::vector 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 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; } diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index 710a30e5bda..6c79a6d73b4 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -63,21 +63,47 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { R"( user: "issuer@foo.com/sub@foo.com" audiences: ["aud1"] - claims { - key: "aud" - value: "aud1" - } - claims { - key: "iss" - value: "issuer@foo.com" - } - claims { - key: "sub" - value: "sub@foo.com" - } - 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: "issuer@foo.com" + } + } + } + } + fields: { + key: "sub" + value: { + list_value: { + values: { + string_value: "sub@foo.com" + } + } + } + } + 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: "issuer@foo.com/sub@foo.com" - claims { - key: "iss" - value: "issuer@foo.com" - } - claims { - key: "sub" - value: "sub@foo.com" - } - claims { - key: "some-other-string-claims" - value: "some-claims-kept" + claims: { + fields: { + key: "iss" + value: { + list_value: { + values: { + string_value: "issuer@foo.com" + } + } + } + } + fields: { + key: "sub" + value: { + list_value: { + values: { + string_value: "sub@foo.com" + } + } + } + } + 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: "issuer@foo.com/sub@foo.com" audiences: "aud1" audiences: "aud2" - claims { - key: "iss" - value: "issuer@foo.com" - } - claims { - key: "sub" - value: "sub@foo.com" - } - 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: "issuer@foo.com" + } + } + } + } + fields: { + key: "sub" + value: { + list_value: { + values: { + string_value: "sub@foo.com" + } + } + } + } + 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)); } diff --git a/src/envoy/utils/authn.cc b/src/envoy/utils/authn.cc index 9c915eaca3d..f64abcc791d 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -63,26 +63,14 @@ void Authentication::SaveAuthAttributesToStruct( setKeyValue(data, istio::utils::AttributeName::kRequestAuthAudiences, origin.audiences(0)); } - if (!origin.groups().empty()) { - ::google::protobuf::ListValue* value; - value = (*data.mutable_fields()) - [istio::utils::AttributeName::kRequestAuthGroups] - .mutable_list_value(); - for (int i = 0; i < origin.groups().size(); i++) { - value->add_values()->set_string_value(origin.groups(i)); - } - } if (!origin.presenter().empty()) { setKeyValue(data, istio::utils::AttributeName::kRequestAuthPresenter, origin.presenter()); } - if (!origin.claims().empty()) { - auto s = (*data.mutable_fields()) - [istio::utils::AttributeName::kRequestAuthClaims] - .mutable_struct_value(); - for (const auto& pair : origin.claims()) { - setKeyValue(*s, pair.first, pair.second); - } + if (!origin.claims().fields().empty()) { + *((*data.mutable_fields()) + [istio::utils::AttributeName::kRequestAuthClaims] + .mutable_struct_value()) = origin.claims(); } if (!origin.raw_claims().empty()) { setKeyValue(data, istio::utils::AttributeName::kRequestAuthRawClaims, diff --git a/src/envoy/utils/authn_test.cc b/src/envoy/utils/authn_test.cc index 93ab445c623..fee32f310c7 100644 --- a/src/envoy/utils/authn_test.cc +++ b/src/envoy/utils/authn_test.cc @@ -46,11 +46,14 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { origin->add_audiences("audiences0"); origin->add_audiences("audiences1"); origin->set_presenter("presenter"); - origin->add_groups("group1"); - origin->add_groups("group2"); - auto claim = origin->mutable_claims(); - (*claim)["key1"] = "value1"; - (*claim)["key2"] = "value2"; + (*origin->mutable_claims()->mutable_fields())["groups"] + .mutable_list_value() + ->add_values() + ->set_string_value("group1"); + (*origin->mutable_claims()->mutable_fields())["groups"] + .mutable_list_value() + ->add_values() + ->set_string_value("group2"); origin->set_raw_claims("rawclaim"); Authentication::SaveAuthAttributesToStruct(result, data); @@ -76,28 +79,26 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { .string_value(), "audiences0"); EXPECT_EQ(data.fields() - .at(istio::utils::AttributeName::kRequestAuthGroups) + .at(istio::utils::AttributeName::kRequestAuthPresenter) + .string_value(), + "presenter"); + + auto auth_claims = + data.fields().at(istio::utils::AttributeName::kRequestAuthClaims); + EXPECT_EQ(auth_claims.struct_value() + .fields() + .at("groups") .list_value() .values(0) .string_value(), "group1"); - EXPECT_EQ(data.fields() - .at(istio::utils::AttributeName::kRequestAuthGroups) + EXPECT_EQ(auth_claims.struct_value() + .fields() + .at("groups") .list_value() .values(1) .string_value(), "group2"); - EXPECT_EQ(data.fields() - .at(istio::utils::AttributeName::kRequestAuthPresenter) - .string_value(), - "presenter"); - - auto actual_claim = - data.fields().at(istio::utils::AttributeName::kRequestAuthClaims); - EXPECT_EQ(actual_claim.struct_value().fields().at("key1").string_value(), - "value1"); - EXPECT_EQ(actual_claim.struct_value().fields().at("key2").string_value(), - "value2"); EXPECT_EQ(data.fields() .at(istio::utils::AttributeName::kRequestAuthRawClaims) diff --git a/src/istio/authn/BUILD b/src/istio/authn/BUILD index b87c9b7bde4..79f3c18011c 100644 --- a/src/istio/authn/BUILD +++ b/src/istio/authn/BUILD @@ -25,4 +25,5 @@ load( envoy_proto_library( name = "context_proto", srcs = ["context.proto"], + external_deps = ["well_known_protos"], ) diff --git a/src/istio/authn/context.proto b/src/istio/authn/context.proto index b9537ee8490..d092132fef4 100644 --- a/src/istio/authn/context.proto +++ b/src/istio/authn/context.proto @@ -16,6 +16,8 @@ syntax = "proto3"; package istio.authn; +import "google/protobuf/struct.proto"; + // Container to hold authenticated attributes from JWT. message JwtPayload { // This is a string of the issuer (iss) and subject (sub) claims within a @@ -33,17 +35,15 @@ message JwtPayload { // id. Example 123456789012.my-svc.com string presenter = 3; - // Only raw JWT string claims are kept. - map claims = 5; + // JWT claims stored as protobuf.Struct + // Only string and string-list claims are extracted into claims. + // A string claim is stored as a string list of one item. + google.protobuf.Struct claims = 5; // All original claims in JsonString format, which can be parsed into json // object (map) to access other claims that not cover with the string claims // map above. string raw_claims = 6; - - // The groups claim in the JWT. - // Example: [‘group1’, ‘group2’] - repeated string groups = 7; } // Container to hold authenticated attributes from X509 (mTLS). diff --git a/src/istio/utils/attribute_names.cc b/src/istio/utils/attribute_names.cc index eead2de2f58..d7c6a55127d 100644 --- a/src/istio/utils/attribute_names.cc +++ b/src/istio/utils/attribute_names.cc @@ -86,7 +86,6 @@ const char AttributeName::kQuotaCacheHit[] = "quota.cache_hit"; // Authentication attributes const char AttributeName::kRequestAuthPrincipal[] = "request.auth.principal"; const char AttributeName::kRequestAuthAudiences[] = "request.auth.audiences"; -const char AttributeName::kRequestAuthGroups[] = "request.auth.groups"; const char AttributeName::kRequestAuthPresenter[] = "request.auth.presenter"; const char AttributeName::kRequestAuthClaims[] = "request.auth.claims"; const char AttributeName::kRequestAuthRawClaims[] = "request.auth.raw_claims";