Extract JWT claims as string lists under request.auth.claims[] for RBAC by lei-tang · Pull Request #1906 · istio/proxy

lei-tang · 2018-08-09T23:07:56Z

What this PR does / why we need it: Extract JWT claims as string lists under request.auth.claims[] for RBAC.

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #

Special notes for your reviewer:

Release note:

istio-testing · 2018-08-09T23:07:59Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: jimmycyj

If they are not already assigned, you can assign the PR to them by writing /assign @jimmycyj in a comment when ready.

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

* Add the groups JWT claims to the attribute request.auth.groups * Fix lint errors * Simplify the code * Fix lint error * Simplify the code * Add a test * Fix the test error

lei-tang · 2018-08-09T23:09:02Z

/cc @liminw

lei-tang · 2018-08-09T23:09:10Z

/cc @diemtvu

lei-tang · 2018-08-09T23:09:21Z

/cc @qiwzhang

qiwzhang · 2018-08-09T23:20:35Z

src/envoy/utils/authn.cc

+              [istio::utils::AttributeName::kAuthDerivedClaims]
+                  .mutable_struct_value();
+      ::google::protobuf::ListValue* value =
+          (*s->mutable_fields())["groups"].mutable_list_value();


Please use new styple for loop

for (const auto& group : origin.groups())

diemtvu

Overall, I think we should change the way we populate and passing claims. First, we should change the type under JwtPayload to use protobuf::Struct (https://github.com/istio/proxy/blob/master/src/istio/authn/context.proto#L37), so we it's more flexible to keep all claims, instead of just string. There may be an util to convert Json object to proto struct, if you are lucky. Though it's should be ok to write one, and 'normalize' the type (e.g always keep as string array for certain types).

After that, in this part of the code, you simply need to set the 'request.auth.claims' to that struct without any modification.

lei-tang · 2018-08-14T00:38:14Z

/cc @wattli

lei-tang · 2018-08-14T00:50:32Z

This PR puts the groups claim (e.g., "groups: group1") in a JWT under the key request.auth.claims[groups] in the dynamic metadata (this is to be consistent with istio/istio#7747). The change of JwtPayload in context.proto to use protobuf::Struct can be implemented in a separate PR.

liminw · 2018-08-14T17:46:48Z

src/envoy/utils/authn.cc

@@ -58,12 +58,14 @@ void Authentication::SaveAuthAttributesToStruct(
                  origin.audiences(0));
    }
    if (!origin.groups().empty()) {


Can we do it for all claims instead of just "groups"?

A JWT may contain various claims, including user defined ones (e.g., department, machine-model, IMEI and etc). Probably we can place all claims in a JWT into request.auth.claims[] or we can all non-well-known claims in a JWT into request.auth.claims[]. But such a PR can be a separate PR to keep this PR small and simple.

In the Pilot PR istio/istio#7747 you just checked in, all claims are matched using ListMatcher. If you don't create ListValue for all claims here, RBAC will break.

Per discussions, in this PR, JWT claims are extracted as string lists for RBAC list matcher. A new PR https://github.com/istio/proxy/issues/1920 has been created to change the "claims" to a protobuf struct.

yangminzhu

I recommend to do #1920 right in this PR.
The code and logic in ProcessJwtPayload() is already complex and seems quite hack to me. It's better to avoid making it more complex and the claims_string_list seems a very specific solution.

liminw · 2018-08-16T13:53:10Z

include/istio/utils/attributes_builder.h

+          if (field.second.list_value().values_size() > 0) {
+            // Only uses the first item in the list as string
+            (*entries)[field.first] =
+                field.second.list_value().values().Get(0).string_value();


We need to make them comma separated strings. If this is not fixed now, please leave a TODO here.

liminw · 2018-08-16T13:56:06Z

src/envoy/http/authn/authenticator_base_test.cc

                 "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\",\n       \"non-string-will-be-ignored\": 1512754205,\n       \"some-other-string-claims\": \"some-claims-kept\"\n     }\n   ",


In the test case, you may want to have claims that have more than one string entries.

Done: added a claim of two string entries.

liminw · 2018-08-16T13:57:12Z

src/envoy/http/authn/authn_utils.cc

-void ExtractJwtGroups(
-    const Envoy::Json::Object& obj,
+void ExtractStringList(
+    const std::string key, const Envoy::Json::Object& obj,


const std::string&

liminw · 2018-08-16T18:55:12Z

src/envoy/http/authn/authn_utils.cc

      payload->mutable_claims();

-  // Extract claims
+  // Extract claims as strings


Since you are doing it for all claims, do you want to replace the logic here with what you had for ExtractStringList()?

ExtractStringList() extracts a string list whereas the code here only extracts a string. So the logic for ExtractStringList() is not applied here.

I mean, you can apply the same logic, first try as a string, then string list, here. With the current way, you actually do it twice, first cast all claims to string in this function, then cast them to string list in ExtractStringList function.

Before ExtractStringList(key, *json_obj, *claims, list) is called, claims must have been extracted. Hence the separation of two loops (one at line 93, the other at line 123).

lei-tang · 2018-08-16T19:34:43Z

/retest

yangminzhu · 2018-08-17T00:51:09Z

src/envoy/http/authn/authn_utils.cc

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


Do you still need kJwtGroupsKey in line 29?

No, I have removed it. Thanks.

lei-tang · 2018-08-17T06:55:57Z

This PR is replaced by #1925, which use protobuf::Struct for storing claims.

istio-testing · 2018-08-30T17:14:09Z

@lei-tang: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

lei-tang · 2018-08-30T17:15:35Z

This PR is closed as it has been replaced by #1925.

istio-testing requested review from JimmyCYJ and linsun August 9, 2018 23:07

googlebot added the cla: yes Set by the Google CLA bot to indicate the author of a PR has signed the Google CLA. label Aug 9, 2018

istio-testing requested a review from liminw August 9, 2018 23:09

istio-testing requested a review from diemtvu August 9, 2018 23:09

istio-testing requested a review from qiwzhang August 9, 2018 23:09

lei-tang force-pushed the update_group_output branch from d8eda04 to a87542f Compare August 9, 2018 23:16

lei-tang mentioned this pull request Aug 9, 2018

RBAC: handle all request.auth.claims[] using RBAC list matcher istio/istio#7747

Merged

qiwzhang reviewed Aug 9, 2018

View reviewed changes

diemtvu reviewed Aug 13, 2018

View reviewed changes

lei-tang force-pushed the update_group_output branch from df22cea to 1549b46 Compare August 14, 2018 00:15

lei-tang changed the title ~~Reorganize the groups claim under auth.derived.claims~~ Organize the groups claim in request.auth.claims[groups] Aug 14, 2018

lei-tang force-pushed the update_group_output branch from 1549b46 to 47d2ea4 Compare August 14, 2018 00:21

istio-testing requested a review from wattli August 14, 2018 00:38

liminw reviewed Aug 14, 2018

View reviewed changes

lei-tang changed the title ~~Organize the groups claim in request.auth.claims[groups]~~ Extract JWT claims as string lists under request.auth.claims[] for RBAC Aug 14, 2018

lei-tang force-pushed the update_group_output branch from e795b7c to 4b603c0 Compare August 15, 2018 00:23

yangminzhu reviewed Aug 15, 2018

View reviewed changes

lei-tang force-pushed the update_group_output branch from dc4e421 to 80fbc16 Compare August 15, 2018 04:55

lei-tang added 4 commits August 15, 2018 10:16

Organize the groups claim in request.auth.claims[groups]

3e00200

Extract claims as string lists for RBAC

dfd4566

Fix http/authn/authn_utils_test

6295f68

Fix authn/authenticator_base_test

84a4fe3

Fix test/integration/istio_http_integration_test.cc

7434c90

lei-tang force-pushed the update_group_output branch from ec5ae71 to 7434c90 Compare August 16, 2018 02:07

liminw reviewed Aug 16, 2018

View reviewed changes

Add a test case for a claim of a list of strings

97f2bc9

yangminzhu reviewed Aug 17, 2018

View reviewed changes

Remove unused variable

67ee2fa

istio-testing added the needs-rebase Indicates a PR needs to be rebased before being merged label Aug 30, 2018

lei-tang closed this Aug 30, 2018

Conversation

lei-tang commented Aug 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

istio-testing commented Aug 9, 2018

Uh oh!

lei-tang commented Aug 9, 2018

Uh oh!

lei-tang commented Aug 9, 2018

Uh oh!

lei-tang commented Aug 9, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

diemtvu left a comment

Choose a reason for hiding this comment

Uh oh!

lei-tang commented Aug 14, 2018

Uh oh!

lei-tang commented Aug 14, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yangminzhu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lei-tang commented Aug 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lei-tang commented Aug 17, 2018

Uh oh!

istio-testing commented Aug 30, 2018

Uh oh!

lei-tang commented Aug 30, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

lei-tang commented Aug 9, 2018 •

edited

Loading