jwt_authn: Add functionality to remove query parameter containing JWT

[arulthileeban]

Commit Message: jwt_authn: Add functionality to remove query parameter containing JWT

Setting forward as false in JWT Authn filter config removes the JWT from headers, but doesn't remove JWT from query params or cookies. This change adds functionality to remove query parameters based on forward config

Risk Level: Low Testing: Unit Testing Docs Changes: Added Release Notes: Not yet Platform Specific Features: N/A

[repokitteh-read-only[bot]]

Hi @arulthileeban, 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.

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/34418 was opened by arulthileeban.

see: more, trace.

[repokitteh-read-only[bot]]

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/). envoyproxy/api-shepherds assignee is @lizan CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/34418 was opened by arulthileeban.

see: more, trace.

[arulthileeban]

Modified HeaderMap object to RequestHeaderMap in authenticator/extractor to utilize some functions associated with the latter class. Also, RequestHeaderMap is what is used in the rest of the filter flow. I'm not sure if there was a design decision behind keeping it as HeaderMap for these two files

[adisuissa]

@TAOXUY can you PTAL as a code-owner?

[htuch]

This seems a breaking change for the data plane? It should be at least runtime guarded, and I think it would be preferable in this case to express as an opt-in config option, unless this is considered a security concern.

[arulthileeban]

This seems a breaking change for the data plane? It should be at least runtime guarded, and I think it would be preferable in this case to express as an opt-in config option, unless this is considered a security concern.

Thanks. I'll add the runtime guard. There is no security concern, but just putting down my thoughts on how to make this an opt-in config option. Currently, we have the forward config , whose doc comments seem to suggest that in an ideal state when you set it to false(default value), it should strip all JWT related header/params/cookies away.

If we were to create a config like "forward_jwt_param", this is how it would look like:

forward: [false] 
forward_jwt_param: [true]

[]->default value

The config variations seem a little counterintuitive personally, but I understand why this would be preferred to avoid any production issues. Please let me know if this is okay or if there is an alternate way.

[htuch]

@TAOXUY @lizan could you folks weight in as code owners on this extension? I think the runtime guard is the minimum if this changes behaviors, the real question is whether this is likely to break folks generally? I'm not sure. In general, we prioritize not breaking end users over having ideal APIs, but I don't have an intuition on whether this is likely to cause widespread issues or not.

[tyxia]

/wait-any

Friendly ping @TAOXUY @lizan , please also check this comment

[TAOXUY]

Yeah, it can potentially break users. In terms of feature flag/config field, I am leaning to add a runtime flag to protect that as this is a tech debt and we don't need to add another config to make forward that granular.

[htuch]

OK, let's go with runtime guard only.

[repokitteh-read-only[bot]]

CC @envoyproxy/runtime-guard-changes: FYI only for changes made to (source/common/runtime/runtime_features.cc).

:cat:

Caused by: https://github.com/envoyproxy/envoy/pull/34418 was synchronize by arulthileeban.

see: more, trace.

[arulthileeban]

Thanks. Updated it with runtime guard

[arulthileeban]

Looks like CI is failing due to an unrelated flaky test