jwt_authn: write JWT payload to dynamic metadata by qiwzhang · Pull Request #4707 · envoyproxy/envoy

qiwzhang · 2018-10-12T17:39:00Z

Description: Use dynamicMetadata in the StreamInfo to pass all successfully verified JWT payloads to other HTTP filters.

Risk Level: Low
Testing: Add unit-tests

lizan

Can you update the PR description to the original one? It will be in the commit message when we merge.

lizan · 2018-10-12T19:31:44Z

Left over from #4698 (comment)

Parse JSON payload into a Struct and use that as value.

qiwzhang · 2018-10-12T22:40:52Z

It was #4698
It was accidentally closed.

lizan · 2018-10-12T23:25:38Z

Can payload_pairs_ just be a struct and you insert to it here. So setPayload doesn't have to iterate and copy them once again?

lizan · 2018-10-12T23:30:54Z

In a second thought, should we make payload_in_metadata as a string, so the key will be specified there? It is more consistent with forward_header configuration. And issuer is already in the payload iss. It will be easier to RBAC rules that can do StringMatcher over issuer rather than use PathSegment (exact match only).

Hi @lizan I did not understand. Could you clarify your proposal? What format should the payload be stored in dynamic_metadata?

If we just store the json string payload, how RBAC rules can use its individual field, such as "iss", "aud"?

I meant make payload_in_meatadata config as a string instead of a bool, like forward_payload_header. Then use that string as a key.

The benefit of that is the key will be specified by config, then when you configure MetadataMatcher you can do StringMatch against value of iss, but if issuer is struct key it couldn't.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

qiwzhang · 2018-10-16T16:30:43Z

@diemtvu, @yangminzhu and me discussed if we want to use recent introduced FilterState to pass payload and decided to continue to use dynamicMetadata with following reasons:

FilterState may not be ready. It is not clear where to put these data object class definitions.
dynamicMetadata is not going to go away soon. RBAC filter is still using dynamicMetadata,

If RBAC filter decides not supporting dynamicMetadata, this fitler will switch to use FilterState.

lizan · 2018-10-16T17:44:27Z

Using dynamic metadata is totally fine and they exists along with FilterState. You cannot add a FilterState without modifying RBAC filter to support it so in this case it should be dynamic metadata.

potatop · 2018-10-16T19:07:33Z

Sorry, I was out last week.

potatop · 2018-10-16T19:11:36Z

@@ -19,6 +19,8 @@ typedef std::unique_ptr<Authenticator> AuthenticatorPtr;

 typedef std::function<void(const ::google::jwt_verify::Status& status)> AuthenticatorCallback;


Not sure about this func callback now. It made sense when it is just the status that needs to be returned, but now every new thing that needs to be returned will get a new func callback?

Agreed that it is not ideal. The best solution is to define a callback interface that has addPayload() and onComplete() call, and later can add call for new data. I did try to explore this route, it is not easy; which object should implement such interface, maybe ContextImpl, it is good for addPayload, but not good for onComplete() since OnComplete can be called per Verifier which there could be many, not just one.

So I settled down with two call_back functions for now, it is easier to pass in two lambda functions.

qiwzhang · 2018-10-18T20:32:45Z

@lizan could you help to merge this?

lizan · 2018-10-18T20:40:58Z

@htuch can we merge this? I think we talked about potential deprecation of metadata but I don't think that should block this?

htuch

Yeah, it should be reasonable to accept this PR assuming the authors are comfortable that we might change the dynamic metadata interface soon.

htuch · 2018-10-19T19:31:39Z

+      MessageUtil::loadFromJson(jwt_->payload_str_, payload_pb);
+      set_payload_cb_(provider.payload_in_metadata(), payload_pb);
+    } catch (EnvoyException ex) {
+      ENVOY_LOG(warn, "Error in converting payload for issuer {}, error: {}", jwt_->iss_,


Is there a test for this?

htuch · 2018-10-19T19:32:01Z

+      ProtobufWkt::Struct payload_pb;
+      MessageUtil::loadFromJson(jwt_->payload_str_, payload_pb);
+      set_payload_cb_(provider.payload_in_metadata(), payload_pb);
+    } catch (EnvoyException ex) {


In general, you can't throw exceptions on the data path..

I don't think we have policy/style guid not throw exception on data path. It's true in general envoy core doesn't catch data path exceptions, but as long as it is caught by filter itself it should be OK? I know some extensions does this a lot (e.g thrift proxy).

If the filter catches it's OK, but it does make reasoning about behavior harder. It would significantly simplify the understandability of exception safety to readers to avoid this.

Use RELEASE_ASSERT to replace this try and catch. The error should not happen.

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

htuch

Looks good, some last nits and a question on the interface.

htuch · 2018-10-22T22:40:34Z

+  // The value is the *protobuf::Struct*. The value of this field will be the key for its *fields*
+  // and the value is the *protobuf::Struct* converted from JWT JSON payload.
+  //
+  // For example, if palyload_in_metadata is *my_payload*:


Nit: typo palyload

Done. Thanks

htuch · 2018-10-22T22:40:52Z

+
+  // If non empty, successfully verified JWT payloads will be written to StreamInfo DynamicMetadata
+  // in the format as: *namespace* is the jwt_authn filter name as **envoy.filters.http.jwt_authn**
+  // The value is the *protobuf::Struct*. The value of this field will be the key for its *fields*


This is a pretty confusing interface. Why not put this under a fixed key like payload_metadata?

You may have multiple JWT entries in a single request and they go to different scope.

htuch · 2018-10-22T22:41:59Z

+    const auto status =
+        Protobuf::util::JsonStringToMessage(jwt_->payload_str_, &payload_pb, options);
+    // payload_str_ have been verified as valid JSON already.
+    // All valid JSON should be able to parse into a portobuf Struct.


Nit: typo "portobuf" (although I like it).

Done. Thanks

htuch · 2018-10-22T22:46:13Z

+  // which set an empty payload struct for each issuer.
+  static ProtobufWkt::Struct getExpectedPayload(const std::vector<std::string>& issuers) {
+    ProtobufWkt::Struct struct_obj;
+    auto fields = struct_obj.mutable_fields();


Nit: auto*

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

htuch

Thanks!

qiwzhang requested a review from lizan as a code owner October 12, 2018 17:39

lizan suggested changes Oct 12, 2018

View reviewed changes

qiwzhang force-pushed the jwt_dynamic_metadadta branch from ef71dd5 to a105167 Compare October 12, 2018 22:37

lizan suggested changes Oct 12, 2018

View reviewed changes

lizan self-assigned this Oct 13, 2018

qiwzhang added 8 commits October 15, 2018 23:46

jwt_authn: write JWT payload to dynamic metadata

74b52c5

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Fix format

47950cd

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Fix asan test failure

35dd05d

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

fix typo

3621582

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Store payload in protobuf::Struct

1ac7010

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

move setPayload to ContextImpl

93e9582

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

save into struct directly

ad88420

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

Change payload_in_metadata to string

cd0e317

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

qiwzhang force-pushed the jwt_dynamic_metadadta branch from 13515a3 to cd0e317 Compare October 15, 2018 23:47

lizan previously approved these changes Oct 16, 2018

View reviewed changes

potatop reviewed Oct 16, 2018

View reviewed changes

lizan requested a review from htuch October 18, 2018 20:40

htuch suggested changes Oct 19, 2018

View reviewed changes

use RELEASE_ASSERT to replace try catch

6d985f3

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

qiwzhang dismissed lizan’s stale review via 6d985f3 October 19, 2018 21:04

lizan mentioned this pull request Oct 20, 2018

style: formalize exception policy on the data path. #4793

Merged

htuch suggested changes Oct 22, 2018

View reviewed changes

fix typo

1b41bbb

Signed-off-by: Wayne Zhang <qiwzhang@google.com>

htuch approved these changes Oct 23, 2018

View reviewed changes

htuch merged commit 2399402 into envoyproxy:master Oct 23, 2018

qiwzhang deleted the jwt_dynamic_metadadta branch October 23, 2018 15:45

cstrahan mentioned this pull request Aug 16, 2019

Question. RBAC on jwt token #7913

Closed

tyxia mentioned this pull request Jun 22, 2024

jwt_authn: Set metadata irrespective of success/failure of JWT Verification #34335

Merged

		@@ -19,6 +19,8 @@ typedef std::unique_ptr<Authenticator> AuthenticatorPtr;

		typedef std::function<void(const ::google::jwt_verify::Status& status)> AuthenticatorCallback;

Conversation

qiwzhang commented Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan 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!

qiwzhang commented Oct 12, 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!

qiwzhang Oct 12, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

qiwzhang commented Oct 16, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lizan commented Oct 16, 2018

Uh oh!

potatop commented Oct 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

qiwzhang commented Oct 18, 2018

Uh oh!

lizan commented Oct 18, 2018

Uh oh!

htuch 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!

lizan Oct 19, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

htuch 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!

htuch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

qiwzhang commented Oct 12, 2018 •

edited

Loading

qiwzhang Oct 12, 2018 •

edited

Loading

qiwzhang commented Oct 16, 2018 •

edited

Loading

lizan Oct 19, 2018 •

edited

Loading