jwt_authn: Add header_to_metadata by dio · Pull Request #18140 · envoyproxy/envoy

dio · 2021-09-16T08:50:04Z

Commit Message:

This patch adds the header_to_metadata field to the JwtProvider config to allow setting the extracted header of a successfully verified JWT to dynamic metadata.

Additional Description:
Risk Level: Low, a new feature
Testing: Added
Docs Changes: Added
Release Notes: Added
Platform-Specific Features: N/A
Fixes #18138

This patch adds header_to_metadata field to JwtProvider config to allow setting the extracted header of a successfully verified JWT to dynamic metadata. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

repokitteh-read-only · 2021-09-16T08:50:07Z

As a reminder, PRs marked as draft will not be automatically assigned reviewers,
or be handled by maintainer-oncall triage.

Please mark your PR as ready when you want it to be reviewed!

🐱

Caused by: #18140 was opened by dio.

see: more, trace.

repokitteh-read-only · 2021-09-16T08:50:12Z

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to api/envoy/.
envoyproxy/api-shepherds assignee is @mattklein123
CC @envoyproxy/api-watchers: FYI only for changes made to api/envoy/.

🐱

Caused by: #18140 was opened by dio.

see: more, trace.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

lizfeed · 2021-09-16T16:06:09Z

Thanks for the quick response! I've verified I can log this new metadata field in this release. I use envoy_data_plane, so I can also verify that the protobufs are generated correctly at least for python.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

yangminzhu · 2021-09-16T21:09:04Z

+  //       kid: EF71iSaosbC5C4tC6Syq1Gm647M
+  //       alg: PS256
+  //
+  string header_in_metadata = 14;


what if payload_in_metadata is the same as header_in_metadata? Is it allowed?

it should not be allowed, please add comment on this restriction.

Added a warning section. Please let me know if we want to validate this when checking the config.

hmm, the current comment says ... is not allowed ... which I would expect the validation is going to reject the config, or you can just update the comment to be more accurate (it's not suggested due to potential override but can still be used (won't be rejected in validation) if the user is sure it won't have any override for their use case).

@yangminzhu I updated as you suggested.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

yangminzhu · 2021-09-17T01:20:45Z

+  //       kid: EF71iSaosbC5C4tC6Syq1Gm647M
+  //       alg: PS256
+  //
+  string header_in_metadata = 14;


hmm, the current comment says ... is not allowed ... which I would expect the validation is going to reject the config, or you can just update the comment to be more accurate (it's not suggested due to potential override but can still be used (won't be rejected in validation) if the user is sure it won't have any override for their use case).

yangminzhu · 2021-09-17T01:23:12Z

  // The active span for the request
  Tracing::Span* parent_span_{&Tracing::NullSpan::instance()};
-  // the callback function to set payload
+  // The callback function called to set the extracted payload and header from a verified JWT.


set_payload_cb_ should have a more generic name as it now sets both payload and header.

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio · 2021-09-17T04:40:34Z

@qiwzhang @yangminzhu I have a question, re:

envoy/source/extensions/filters/http/jwt_authn/verifier.h

Lines 29 to 34 in a3cc673

    
               /** 
        
                * Successfully verified JWT payload are stored in the struct with its 
        
                * *fields* containing **issuer** as keys and **payload** as string values 
        
                * This function is called before onComplete() function. 
        
                * It will not be called if no payload to write. 
        
                */

. Do you think this is still correct? I'm not sure I understand there is a use-case when we do this (storing the payload as a string).

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

mattklein123 · 2021-09-20T15:45:12Z

Will wait for review/approve from @yangminzhu or @qiwzhang before looking, thanks.

/wait-any

yangminzhu · 2021-09-21T00:34:33Z

@qiwzhang @yangminzhu I have a question, re:

envoy/source/extensions/filters/http/jwt_authn/verifier.h

Lines 29 to 34 in a3cc673

/**

* Successfully verified JWT payload are stored in the struct with its

* *fields* containing **issuer** as keys and **payload** as string values

* This function is called before onComplete() function.

* It will not be called if no payload to write.

*/

. Do you think this is still correct? I'm not sure I understand there is a use-case when we do this (storing the payload as a string).

storing the payload as a string is to pass the payload to backend application without the signature so that the backend can not reuse the JWT token.

dio added 2 commits September 16, 2021 01:43

jwt_authn: Add header_to_metadata

ab3f002

This patch adds header_to_metadata field to JwtProvider config to allow setting the extracted header of a successfully verified JWT to dynamic metadata. Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Merge remote-tracking branch 'upstream/main' into jwt-auth-18138

f84ee04

repokitteh-read-only Bot added the api label Sep 16, 2021

repokitteh-read-only Bot assigned mattklein123 Sep 16, 2021

dio mentioned this pull request Sep 16, 2021

No way to retrieve JWT kid as metadata #18138

Closed

Add header only test

470b80b

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Fix format

da95901

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio marked this pull request as ready for review September 16, 2021 20:52

dio requested a review from lizan as a code owner September 16, 2021 20:52

dio commented Sep 16, 2021

View reviewed changes

Comment thread source/extensions/filters/http/jwt_authn/verifier.cc Outdated

Fix

214eb30

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

yangminzhu reviewed Sep 16, 2021

View reviewed changes

dio added 4 commits September 16, 2021 16:19

Use only one callback

ef8a93f

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Review comments

66e63ca

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Rename

f01742a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

Minimize diffs

6433ca6

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio requested review from qiwzhang and yangminzhu September 16, 2021 23:52

yangminzhu reviewed Sep 17, 2021

View reviewed changes

qiwzhang previously approved these changes Sep 17, 2021

View reviewed changes

Rename payload to extracted data

311fc4a

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio dismissed qiwzhang’s stale review via 311fc4a September 17, 2021 04:38

dio added 2 commits September 16, 2021 22:12

Fix

4c58b67

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

One more thing

276d2d5

Signed-off-by: Dhi Aurrahman <dio@rockybars.com>

dio requested review from qiwzhang and yangminzhu September 17, 2021 23:54

mattklein123 assigned qiwzhang and yangminzhu Sep 20, 2021

repokitteh-read-only Bot added the waiting:any label Sep 20, 2021

qiwzhang approved these changes Sep 20, 2021

View reviewed changes

repokitteh-read-only Bot removed the waiting:any label Sep 20, 2021

yangminzhu approved these changes Sep 21, 2021

View reviewed changes

mattklein123 approved these changes Sep 21, 2021

View reviewed changes

repokitteh-read-only Bot removed the api label Sep 21, 2021

mattklein123 merged commit 25622ce into envoyproxy:main Sep 21, 2021

tyxia mentioned this pull request Jun 22, 2024

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

Merged

Conversation

dio commented Sep 16, 2021

Uh oh!

repokitteh-read-only Bot commented Sep 16, 2021

Uh oh!

repokitteh-read-only Bot commented Sep 16, 2021

Uh oh!

lizfeed commented Sep 16, 2021

Uh oh!

Uh oh!

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!

dio commented Sep 17, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mattklein123 commented Sep 20, 2021

Uh oh!

yangminzhu commented Sep 21, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

dio commented Sep 17, 2021 •

edited

Loading