jwt_authn: Support extraction of JWT from Cookies in JWT Extension by theshubhamp · Pull Request #17721 · envoyproxy/envoy

theshubhamp · 2021-08-15T18:47:18Z

Support extraction of JWT from Cookies in JWT Extension

Added "from_cookies" config directive to jwt_authn that enables JWT extraction from request cookies.

Risk Level: low
Testing: unit tests
Docs Changes: Updated docs/root/configuration/http/http_filters/jwt_authn_filter.rst
Release Notes: Updated docs/root/version_history/current.rst
Platform Specific Features: None

Fixes #17424

Signed-off-by: Shubham Patil theshubhamp@gmail.com

Added "from_cookies" config directive to jwt_authn that enables JWT extraction from request cookies. Testing: unit tests Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

repokitteh-read-only · 2021-08-15T18:47:21Z

Hi @theshubhamp, welcome and thank you for your contribution.

We will try to review your Pull Request as quickly as possible.

In the meantime, please take a look at the contribution guidelines if you have not done so already.

🐱

Caused by: #17721 was opened by theshubhamp.

see: more, trace.

repokitteh-read-only · 2021-08-15T18:47:25Z

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

🐱

Caused by: #17721 was opened by theshubhamp.

see: more, trace.

theshubhamp · 2021-08-15T18:51:26Z

Running tools/code_format/check_format.py fix made a few formatting changes related to my change. I have left them out of this PR.

	modified:   api/envoy/config/filter/http/jwt_authn/v2alpha/config.proto
	modified:   api/envoy/extensions/filters/http/jwt_authn/v3/config.proto
	modified:   api/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto
	modified:   generated_api_shadow/envoy/config/filter/http/jwt_authn/v2alpha/config.proto
	modified:   generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v3/config.proto
	modified:   generated_api_shadow/envoy/extensions/filters/http/jwt_authn/v4alpha/config.proto
	modified:   test/common/http/header_map_impl_test.cc

Example:

LMK if these should be included.

theshubhamp · 2021-08-15T18:55:12Z

source/common/http/utility.cc

  return EMPTY_STRING;
 }

+std::map<std::string, std::string> Utility::parseCookies(const RequestHeaderMap& headers) {


This is very similar to the existing code in source/common/http/utility.cc #L257-L282

Could not use it as-is because that'd scan over cookies multiple times. Any suggestions on merging them together ?

Perhaps you could introduce something like void forEachCookie(const Http::HeaderMap::GetResult cookie_headers, std::function<bool(...)> fn) where fn is a closure dealing with a cookie and returning false to stop iteration.

Thanks for the suggestion! Was able to get this working locally 👍

I would prefer to do this change in a follow-up PR. I hope that's okay.

Yes, a follow-up would be Ok. Just put a TODO note to the function.

The other places using parseCookieValue would be benefit from this logic if you see how oauth2 filter use it.

https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/oauth2/filter.cc#L141-L143

I would suggest you to land the refactoring before this PR.

Cool, I'll do that first

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

rojkov

Thank you! Added a couple of suggestions.

rojkov · 2021-08-16T07:31:48Z

source/common/http/utility.cc

  return EMPTY_STRING;
 }

+std::map<std::string, std::string> Utility::parseCookies(const RequestHeaderMap& headers) {


Perhaps you could introduce something like void forEachCookie(const Http::HeaderMap::GetResult cookie_headers, std::function<bool(...)> fn) where fn is a closure dealing with a cookie and returning false to stop iteration.

test/extensions/filters/http/jwt_authn/extractor_test.cc

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

qiwzhang

LGTM on jwt_authn extractor code . Thanks.

theshubhamp · 2021-08-16T18:01:55Z

Thanks @rojkov for your review! Commented on your suggestions, PTAL.

Thanks @qiwzhang for your approval! Unfortunately it got reset because of a small test data change I just pushed. PTAL again.

rojkov · 2021-08-17T06:03:08Z

/retest

repokitteh-read-only · 2021-08-17T06:03:12Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17721 (comment) was created by @rojkov.

see: more, trace.

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp · 2021-08-17T17:59:58Z

Pushed a commit ^ that adds TODO for cookie iterator improvements suggested in the review.

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp · 2021-08-18T02:35:37Z

/retest

repokitteh-read-only · 2021-08-18T02:35:40Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17721 (comment) was created by @theshubhamp.

see: more, trace.

rojkov · 2021-08-18T07:10:42Z

/retest

repokitteh-read-only · 2021-08-18T07:10:45Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17721 (comment) was created by @rojkov.

see: more, trace.

rojkov

Thanks! Looks good to me

theshubhamp · 2021-08-18T18:41:49Z

cc: @lizan, this PR requires a mandatory review from you. PTAL whenever you can!

lizan

/wait

lizan · 2021-08-20T17:53:54Z

api/envoy/extensions/filters/http/jwt_authn/v3/config.proto

+
+  // JWT is sent in a cookie. `from_cookies` represents the cookie names to extract from.
+  //
+  // For example, if config is:
+  //
+  // .. code-block:: yaml
+  //
+  //   from_cookies:
+  //   - auth-token
+  //
+  // Then JWT will be extracted from `auth-token` cookie in the request.
+  //
+  repeated string from_cookies = 13;


Move this block after from_params.

This doesn't have to be ordered by field tag.

Done, thanks for pointing this out

lizan · 2021-08-20T18:19:18Z

source/common/http/utility.cc

  return EMPTY_STRING;
 }

+std::map<std::string, std::string> Utility::parseCookies(const RequestHeaderMap& headers) {


The other places using parseCookieValue would be benefit from this logic if you see how oauth2 filter use it.

https://github.com/envoyproxy/envoy/blob/main/source/extensions/filters/http/oauth2/filter.cc#L141-L143

I would suggest you to land the refactoring before this PR.

lizan · 2021-08-20T18:19:57Z

source/extensions/filters/http/jwt_authn/extractor.cc

+      : JwtLocationBase(token, issuer_checker) {}
+
+  void removeJwt(Http::HeaderMap&) const override {
+    // TODO(theshubhamp): remove JWT from cookies.


Use NOT_IMPLEMENTED_GCOVR_EXCL_LINE

Added below the comment

Reordered `from_cookies` to be after `from_params` in proto. Added NOT_IMPLEMENTED_GCOVR_EXCL_LINE in JwtCookieLocation::removeJwt(..) Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp · 2021-08-22T16:59:37Z

/retest

repokitteh-read-only · 2021-08-22T16:59:41Z

Retrying Azure Pipelines:
Retried failed jobs in: envoy-presubmit

🐱

Caused by: a #17721 (comment) was created by @theshubhamp.

see: more, trace.

adisuissa

/lgtm api

lizan · 2021-08-30T22:24:39Z

can you resolve conflicts?

…-cookie Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp · 2021-08-31T12:55:37Z

Merged in master and resolved conflicts. PTAL!

rojkov

Thank you! Looks good to me.

Support extraction of JWT from Cookies in JWT Extension

65f7e61

Added "from_cookies" config directive to jwt_authn that enables JWT extraction from request cookies. Testing: unit tests Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp requested a review from lizan as a code owner August 15, 2021 18:47

repokitteh-read-only bot added the api label Aug 15, 2021

repokitteh-read-only bot assigned adisuissa Aug 15, 2021

theshubhamp commented Aug 15, 2021

View reviewed changes

End release note entry with a "."

3d37b65

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

rojkov reviewed Aug 16, 2021

View reviewed changes

Modify TestCookieToken ExtractorTest to cover quoted cookie values

09eaa9b

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

qiwzhang previously approved these changes Aug 16, 2021

View reviewed changes

theshubhamp dismissed qiwzhang’s stale review via 09eaa9b August 16, 2021 17:54

qiwzhang previously approved these changes Aug 16, 2021

View reviewed changes

Add TODO for cookie iterator rework in parseCookies

c9f88ef

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp dismissed qiwzhang’s stale review via c9f88ef August 17, 2021 17:59

theshubhamp requested review from qiwzhang and rojkov August 17, 2021 18:00

Fix spellings to make pre-check happy

72ee06c

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

rojkov previously approved these changes Aug 18, 2021

View reviewed changes

lizan reviewed Aug 20, 2021

View reviewed changes

repokitteh-read-only bot added the waiting label Aug 20, 2021

jwt_authn proto reordering & NOT_IMPLEMENTED_GCOVR_EXCL_LINE usage

1a958af

Reordered `from_cookies` to be after `from_params` in proto. Added NOT_IMPLEMENTED_GCOVR_EXCL_LINE in JwtCookieLocation::removeJwt(..) Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

theshubhamp dismissed rojkov’s stale review via 1a958af August 21, 2021 17:11

repokitteh-read-only bot removed the waiting label Aug 21, 2021

adisuissa reviewed Aug 23, 2021

View reviewed changes

repokitteh-read-only bot removed the api label Aug 23, 2021

theshubhamp mentioned this pull request Aug 23, 2021

common: support parsing cookies into a map #17811

Merged

junr03 assigned rojkov and lizan Aug 30, 2021

lizan added the waiting label Aug 30, 2021

theshubhamp added 3 commits August 31, 2021 16:27

Merge remote-tracking branch 'origin/main' into theshubhamp/envoy-jwt…

4c78c4a

…-cookie Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

Remove older parseCookies implementation

e61bab0

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

Use parseCookies with filter predicate in jwt_auth cookie extraction

883eb21

Signed-off-by: Shubham Patil <theshubhamp@gmail.com>

repokitteh-read-only bot removed the waiting label Aug 31, 2021

rojkov approved these changes Sep 1, 2021

View reviewed changes

lizan approved these changes Sep 3, 2021

View reviewed changes

lizan merged commit 143083c into envoyproxy:main Sep 3, 2021

theshubhamp deleted the theshubhamp/envoy-jwt-cookie branch September 3, 2021 08:32

Conversation

theshubhamp commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

repokitteh-read-only bot commented Aug 15, 2021

Uh oh!

repokitteh-read-only bot commented Aug 15, 2021

Uh oh!

theshubhamp commented Aug 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

qiwzhang left a comment

Choose a reason for hiding this comment

Uh oh!

theshubhamp commented Aug 16, 2021

Uh oh!

rojkov commented Aug 17, 2021

Uh oh!

repokitteh-read-only bot commented Aug 17, 2021

Uh oh!

theshubhamp commented Aug 17, 2021

Uh oh!

theshubhamp commented Aug 18, 2021

Uh oh!

repokitteh-read-only bot commented Aug 18, 2021

Uh oh!

rojkov commented Aug 18, 2021

Uh oh!

repokitteh-read-only bot commented Aug 18, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

Uh oh!

theshubhamp commented Aug 18, 2021 • 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!

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!

theshubhamp commented Aug 22, 2021

Uh oh!

repokitteh-read-only bot commented Aug 22, 2021

Uh oh!

adisuissa left a comment

Choose a reason for hiding this comment

Uh oh!

lizan commented Aug 30, 2021

Uh oh!

theshubhamp commented Aug 31, 2021

Uh oh!

rojkov left a comment

Choose a reason for hiding this comment

theshubhamp commented Aug 15, 2021 •

edited

Loading

theshubhamp commented Aug 15, 2021 •

edited

Loading

theshubhamp commented Aug 18, 2021 •

edited

Loading