Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
istio-testing merged 2 commits into from
Aug 23, 2018
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9f34dcd
Authn uses protobuf.Struct to store claims and add list support for RBAC
lei-tang Aug 9, 2018
5b5ad27
Change based on the review comments
lei-tang Aug 17, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion include/istio/utils/attribute_names.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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[];
Expand Down
16 changes: 15 additions & 1 deletion include/istio/utils/attributes_builder.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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++) {
Copy link

@liminw liminw Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you use for (const auto& value : field.second.list_value().values()) ?

Also, do you want to put the logic of converting a ListValue to comma separated string into a util function?

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"int i" here is used to avoid adding a comma to the last list item. Since the for loop here of six lines is only used at other places, it is not placed in a separate util function.

Copy link
Contributor

@diemtvu diemtvu Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May be using string join utils under https://github.com/envoyproxy/envoy/blob/master/source/common/common/utility.h#L322

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

join() input argument is a vectorstd::string: static std::string join(const std::vectorstd::string& source, const std::string& delimiter). To use join(), field.second needs to be converted to vectorstd::string. I have not found a one-liner to do the conversion.

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;
}
Expand Down
14 changes: 7 additions & 7 deletions src/envoy/http/authn/authenticator_base_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
}
Expand Down Expand Up @@ -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 ",
}
}
)",
Expand Down
105 changes: 40 additions & 65 deletions src/envoy/http/authn/authn_utils.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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
Expand All @@ -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
Copy link
Contributor

@yangminzhu yangminzhu Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of looping over the JSON object manually, why not use ParseJsonMessage() to parse the whole json string to the claim field? So that you could simplify the ProcessJwtPayload() a lot and also don't need ExtractStringList() any more.

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yangminzhu The output of ParseJsonMessage() is of the type Message:
Status ParseJsonMessage(const std::string& json, Message* output). Here the claims is of the type protobuf.Struct. Can you provide details of how to use ParseJsonMessage() to parse the json object into protobuf.Struct? One more thing, if the json object is parsed into the claims Struct as different types (e.g., strings, string lists, structs, and etc), how does the consumer of the claims interpret the type of a claim (since the type for a claim value is not only a string list anymore)?

Copy link
Contributor

@yangminzhu yangminzhu Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Struct is a subclass of Message, so it should be able to pass to ParseJsonMessage() directly.

I'm curious where are we using the claims field other than in authn_utils.cc? The type is changed and it's seems mostly used in this file directly.

@diemtvu WDYT?

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The consumers of the claims protobuf.Struct include the producers of dynamic metadata and tests in the Istio proxy repo. If the claims become a map from string key to protobuf.Struct value, every consumer of the claims needs to add some code to verify the type of the claim value before accessing a claim. In contrast, with current implementation, the contract between the producer of the claims and the consumers is that a claim value must be of the string list type and a consumer directly reads a claim value as a string list without extra code to verify the type of a claim.

Copy link
Contributor

@yangminzhu yangminzhu Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The claims's type is already changed to Struct so why it matters about how you construct it? If the constructed Struct doesn't have a string or string list as your implicit contract here, the consumer still get the same value as today when using the .string_value() method on the Struct. So there shouldn't have any changes on consumer side once its stored in dynamic metadata.

Copy link
Contributor

@diemtvu diemtvu Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can explore, but I do think ParseJsonMessage, even if it works with Struct, may has some limitation. For example, in our approach, we want to have consistent representation for string + string array. I'm ok with this custom conversion for now, and find a better utils later (or refactor them to a util function)

Copy link
Contributor

@yangminzhu yangminzhu Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you point me to one of the consumer side code that needs to change if you use ParseJsonMessage()?

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following is a few examples on the consumer side code that need to change if use ParseJsonMessage():

  • In istio/proxy/src/envoy/http/authn/authn_utils.cc:
    payload->set_user(
    (*claims)["iss"].list_value().values().Get(0).string_value() + "/" +
    (*claims)["sub"].list_value().values().Get(0).string_value());
    payload->set_presenter(
    (*claims)["azp"].list_value().values().Get(0).string_value());

Copy link
Contributor Author

@lei-tang lei-tang Aug 17, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another example on the consumer side code that need to change if use ParseJsonMessage():
// 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());
}
}

Copy link
Contributor

@yangminzhu yangminzhu Aug 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are just right in this function, I'm trying to see if there are any consumer code outside this file and I will be supersized if it is consuming the claim field the same way as the examples, i.e. getting string directly from the list.

Anyway, I'm ok if you would like to go with the current approach, we can discuss more and refactor later.

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;
}

Expand Down
155 changes: 117 additions & 38 deletions src/envoy/http/authn/authn_utils_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"(")",
Expand All @@ -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) +
Expand All @@ -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));
}
Expand Down
Loading