diff --git a/include/istio/utils/attribute_names.h b/include/istio/utils/attribute_names.h index 84e7415837d..3c59cb4f6bf 100644 --- a/include/istio/utils/attribute_names.h +++ b/include/istio/utils/attribute_names.h @@ -90,7 +90,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..eb25b3bbd0a 100644 --- a/include/istio/utils/attributes_builder.h +++ b/include/istio/utils/attributes_builder.h @@ -99,11 +99,20 @@ 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) { + // Only uses the first item in the list as string + // TODO (lei-tang): the items in the list may be converted into a + // comma separated string + (*entries)[field.first] = + field.second.list_value().values().Get(0).string_value(); + } + 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..9bcfbc7d16c 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,20 @@ 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" }, - 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 ", + "claim_string_lists": { + "aud": {"list": ["aud1", "aud2"]}, + "iss": {"list": ["issuer@foo.com"]}, + "some-other-string-claims": {"list": ["some-claims-kept"]}, + "sub": {"list": ["sub@foo.com"]}, + }, } } )", diff --git a/src/envoy/http/authn/authn_utils.cc b/src/envoy/http/authn/authn_utils.cc index 952114b9810..b5fe3f8c886 100644 --- a/src/envoy/http/authn/authn_utils.cc +++ b/src/envoy/http/authn/authn_utils.cc @@ -25,9 +25,6 @@ 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( @@ -52,24 +49,22 @@ void ExtractJwtAudience( } } -// Extract JWT groups into the JwtPayload. +// Extract JWT claim as a string list // This function should to be called after the claims are extracted. -void ExtractJwtGroups( - const Envoy::Json::Object& obj, +void ExtractStringList( + const std::string& key, 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. + std::vector& list) { // First, try as string if (claims.count(key) > 0) { - payload->add_groups(claims.at(key)); + list.push_back(claims.at(key)); return; } // 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 @@ -92,7 +87,7 @@ bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str, ::google::protobuf::Map< ::std::string, ::std::string>* claims = payload->mutable_claims(); - // Extract claims + // Extract claims as strings json_obj->iterate( [payload](const std::string& key, const Json::Object& obj) -> bool { ::google::protobuf::Map< ::std::string, ::std::string>* claims = @@ -110,8 +105,7 @@ bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str, // 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); + // Build user if (claims->count("iss") > 0 && claims->count("sub") > 0) { payload->set_user((*claims)["iss"] + "/" + (*claims)["sub"]); @@ -120,6 +114,18 @@ bool AuthnUtils::ProcessJwtPayload(const std::string& payload_str, if (claims->count("azp") > 0) { payload->set_presenter((*claims)["azp"]); } + + // Extract claims as lists of strings for RBAC list matcher + json_obj->iterate([json_obj, payload, claims](const std::string& key, + const Json::Object&) -> bool { + auto* claim_lists = payload->mutable_claim_string_lists(); + std::vector list; + ExtractStringList(key, *json_obj, *claims, list); + for (auto s : list) { + (*claim_lists)[key].add_list(s); + } + return true; + }); return true; } diff --git a/src/envoy/http/authn/authn_utils_test.cc b/src/envoy/http/authn/authn_utils_test.cc index 710a30e5bda..b211c56acee 100644 --- a/src/envoy/http/authn/authn_utils_test.cc +++ b/src/envoy/http/authn/authn_utils_test.cc @@ -79,6 +79,22 @@ TEST(AuthnUtilsTest, GetJwtPayloadFromHeaderTest) { key: "some-other-string-claims" value: "some-claims-kept" } + claim_string_lists { + key: "aud" + value: {list: ["aud1"]} + } + claim_string_lists { + key: "iss" + value: {list: ["issuer@foo.com"]} + } + claim_string_lists { + key: "sub" + value: {list: ["sub@foo.com"]} + } + claim_string_lists { + key: "some-other-string-claims" + value: {list: ["some-claims-kept"]} + } raw_claims: ")" + StringUtil::escape(kSecIstioAuthUserinfoHeaderValue) + R"(")", &expected_payload)); @@ -107,6 +123,18 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithNoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } + claim_string_lists { + key: "iss" + value: {list: ["issuer@foo.com"]} + } + claim_string_lists { + key: "sub" + value: {list: ["sub@foo.com"]} + } + claim_string_lists { + key: "some-other-string-claims" + value: {list: ["some-claims-kept"]} + } raw_claims: ")" + StringUtil::escape(kSecIstioAuthUserInfoHeaderWithNoAudValue) + R"(")", @@ -139,16 +167,32 @@ TEST(AuthnUtilsTest, ProcessJwtPayloadWithTwoAudTest) { key: "some-other-string-claims" value: "some-claims-kept" } + claim_string_lists { + key: "aud" + value: {list: ["aud1", "aud2"]} + } + claim_string_lists { + key: "iss" + value: {list: ["issuer@foo.com"]} + } + claim_string_lists { + key: "sub" + value: {list: ["sub@foo.com"]} + } + claim_string_lists { + key: "some-other-string-claims" + value: {list: ["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 a28f8ceba8f..905605e13b7 100644 --- a/src/envoy/utils/authn.cc +++ b/src/envoy/utils/authn.cc @@ -57,15 +57,6 @@ 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()); @@ -82,6 +73,20 @@ void Authentication::SaveAuthAttributesToStruct( setKeyValue(data, istio::utils::AttributeName::kRequestAuthRawClaims, origin.raw_claims()); } + // Store the claims for RBAC + if (!origin.claim_string_lists().empty()) { + ::google::protobuf::Struct* s = + (*data.mutable_fields()) + [istio::utils::AttributeName::kRequestAuthClaims] + .mutable_struct_value(); + for (auto const& e : origin.claim_string_lists()) { + ::google::protobuf::ListValue* value = + (*s->mutable_fields())[e.first].mutable_list_value(); + for (const auto& v : e.second.list()) { + value->add_values()->set_string_value(v); + } + } + } } } diff --git a/src/envoy/utils/authn_test.cc b/src/envoy/utils/authn_test.cc index cc97855cda9..ab68390b066 100644 --- a/src/envoy/utils/authn_test.cc +++ b/src/envoy/utils/authn_test.cc @@ -46,8 +46,8 @@ TEST_F(AuthenticationTest, SaveAuthAttributesToStruct) { origin->add_audiences("audiences0"); origin->add_audiences("audiences1"); origin->set_presenter("presenter"); - origin->add_groups("group1"); - origin->add_groups("group2"); + (*origin->mutable_claim_string_lists())["groups"].add_list("group1"); + (*origin->mutable_claim_string_lists())["groups"].add_list("group2"); auto claim = origin->mutable_claims(); (*claim)["key1"] = "value1"; (*claim)["key2"] = "value2"; @@ -72,21 +72,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); diff --git a/src/istio/authn/context.proto b/src/istio/authn/context.proto index b9537ee8490..1922180054c 100644 --- a/src/istio/authn/context.proto +++ b/src/istio/authn/context.proto @@ -41,9 +41,8 @@ message JwtPayload { // map above. string raw_claims = 6; - // The groups claim in the JWT. - // Example: [‘group1’, ‘group2’] - repeated string groups = 7; + // Claims in JWT as lists of strings + map claim_string_lists = 7; } // Container to hold authenticated attributes from X509 (mTLS). @@ -74,3 +73,8 @@ message Result { // JwtPayload for origin authenticated attributes. JwtPayload origin = 3; } + +message StringList { + // A list of strings + repeated string list = 1; +} diff --git a/src/istio/utils/attribute_names.cc b/src/istio/utils/attribute_names.cc index d42e0666b38..d2c976985bb 100644 --- a/src/istio/utils/attribute_names.cc +++ b/src/istio/utils/attribute_names.cc @@ -85,7 +85,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";