Bugfix issue 27877 jwt token with space is valid #28678

danieldradware · 2023-07-28T05:29:55Z

…non base64 characters

Commit Message:
This PR should fix issue #27877
Additional Description:
New behavior isn't cutting characters after non base64 character
Now expected to get all token and then mark it as invalid jwt
All details in issue #27877
Risk Level:
Medium
Testing:
Added unit test
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
Fixes #27877
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional API Considerations:]

yanavlasov · 2023-08-01T00:21:36Z

The spell checker is not happy: https://dev.azure.com/cncf/envoy/_build/results?buildId=144417&view=logs&j=c7de00aa-b0fb-52ba-707b-da6afd918eda&t=2ec0901d-43fb-521a-88ec-473706b56eab&l=45

yanavlasov · 2023-08-01T00:21:42Z

/wait

nareddyt · 2023-08-01T02:03:53Z

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

-    return value_str.substr(starting);
-  }
-  return value_str.substr(starting, ending - starting);
+  return value_str.substr(starting);


Is a runtime guard warranted for this change? https://github.com/envoyproxy/envoy/blob/main/CONTRIBUTING.md#runtime-guarding

Generally as a community we try to guard [...] most user-visible non-config-guarded changes to protocol processing (for example additions or changes to HTTP headers or how HTTP is serialized out) for non-alpha features. Feel free to tag @envoyproxy/maintainers if you aren't sure if a given change merits runtime guarding.

I'm slightly worried it will break runtime traffic for clients using it (https://www.hyrumslaw.com/). Though this is a very unique corner case and I doubt clients rely on this behavior. Either way, worth asking @envoyproxy/maintainers

RFC 7519 implies that adding non-Base64URL characters after a valid JWT token makes it invalid. Thus, if a the JWT is followed by a space and some Base64URL characters or just followed by non Base64URL characters like ### (without any space) it should be considered invalid.

@danieldradware we agree with you that the current behavior is a bug. However there could be Envoy users who relied on this buggy behavior and if we just change it it will affect their system. To prevent this we add runtime override that would bring back old behavior. In 6 months this override is removed.

It is not very hard to add a runtime override. Please see PR #27974 for example.

danieldradware · 2023-08-09T17:31:19Z

Hi @yanavlasov
CI is pass, someone can review it?
Thanks in advance
Daniel

yanavlasov

/wait

yanavlasov · 2023-08-16T00:33:22Z

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

-    return value_str.substr(starting);
-  }
-  return value_str.substr(starting, ending - starting);
+  return value_str.substr(starting);


@danieldradware we agree with you that the current behavior is a bug. However there could be Envoy users who relied on this buggy behavior and if we just change it it will affect their system. To prevent this we add runtime override that would bring back old behavior. In 6 months this override is removed.

It is not very hard to add a runtime override. Please see PR #27974 for example.

repokitteh-read-only · 2023-08-16T20:19:02Z

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

🐱

Caused by: #28678 was synchronize by danieldradware.

see: more, trace.

yanavlasov · 2023-08-22T00:20:24Z

@danieldradware the runtime features require alphanumeric order

ERROR: From ./source/common/runtime/runtime_features.cc
ERROR: RUNTIME_GUARD(envoy_reloadable_features_http_reject_path_with_fragment); and RUNTIME_GUARD(envoy_reloadable_features_token_passed_entirely); are out of order

yanavlasov · 2023-08-22T00:22:31Z

Can you also add a test that check the behavior with the envoy.reloadable_features.token_passed_entirely == false, please? In this was it is certain that old behavior is still working with override. Look good otherwise.

/wait

…non base64 characters Signed-off-by: danield <danield@radware.com>

Signed-off-by: danield <danield@radware.com>

…(dis) Signed-off-by: danield <danield@radware.com>

phlax · 2023-08-29T09:57:00Z

@yanavlasov i think this is waiting on further review

/wait-any

Signed-off-by: danield <danield@radware.com>

…ass-invalid-authorization-bearer

Signed-off-by: danield <danield@radware.com>

danieldradware · 2023-08-31T04:59:02Z

Hi @yanavlasov @phlax
I saw there is a failure in pipeline cause network issue,
Can you please rerun only specific jobs that failures?
Thanks!

danieldradware · 2023-08-31T10:04:01Z

Hi @yanavlasov @phlax I saw there is a failure in pipeline cause network issue, Can you please rerun only specific jobs that failures? Thanks!

Thanks for rerun it.
Can you please approve the PR and merge it with main?

yanavlasov

/wait

yanavlasov · 2023-09-01T19:03:04Z

source/common/runtime/runtime_features.cc

 RUNTIME_GUARD(envoy_reloadable_features_uhv_allow_malformed_url_encoding);
+RUNTIME_GUARD(envoy_reloadable_features_uhv_preserve_url_encoded_case);


These runtime keys looks like result of a bad merge. Please remove them, since they are not part of this change.

yanavlasov · 2023-09-01T19:08:33Z

I was also looking for a test with the following in it:

      TestScopedRuntime scoped_runtime;
      scoped_runtime.mergeValues({{"envoy.reloadable_features.token_passed_entirely", "false"}});

that tests old behavior. Can you add a test like this please?

danieldradware · 2023-09-03T04:40:22Z

I was also looking for a test with the following in it:
      TestScopedRuntime scoped_runtime;
      scoped_runtime.mergeValues({{"envoy.reloadable_features.token_passed_entirely", "false"}});
that tests old behavior. Can you add a test like this please?

Hi @yanavlasov
Sure I already added unit test to test the old behavior, you can see it at:test/extensions/filters/http/jwt_authn/extractor_runtime_guard_test.cc and BUILD file that contain: args = ["--runtime-feature-disable-for-tests=envoy.reloadable_features.token_passed_entirely"],

removed RUNTIME_GUARD(envoy_reloadable_features_uhv_preserve_url_encoded_case); Signed-off-by: danieldradware <117576776+danieldradware@users.noreply.github.com>

danieldradware · 2023-09-03T07:01:02Z

Can you please rerun Linux arm64 section?
Thanks!

zirain · 2023-09-03T09:40:49Z

/retest

danieldradware · 2023-09-04T19:07:04Z

Hi @yanavlasov
Can you please remove "change requested" action? (I already pushed all changed you asked from me).

BTW who can give to this PR an approval?

Thanks a lot
Daniel

danieldradware requested a review from lizan as a code owner July 28, 2023 05:29

danieldradware force-pushed the issue-25239-jwt-authn-filter-is-pass-invalid-authorization-bearer branch from 912903a to 70731bd Compare July 28, 2023 05:38

weissne mentioned this pull request Jul 31, 2023

Jwt Authentication filter falsely authenticates invalid JWT made up of a valid JWT followed by a space and additional characters #27877

Closed

repokitteh-read-only bot added the waiting label Aug 1, 2023

nareddyt reviewed Aug 1, 2023

View reviewed changes

repokitteh-read-only bot removed the waiting label Aug 1, 2023

danieldradware force-pushed the issue-25239-jwt-authn-filter-is-pass-invalid-authorization-bearer branch 2 times, most recently from a86086f to 0c37cbf Compare August 1, 2023 10:22

htuch assigned lizan Aug 1, 2023

danieldradware force-pushed the issue-25239-jwt-authn-filter-is-pass-invalid-authorization-bearer branch 2 times, most recently from feb3a6f to 0ed9834 Compare August 9, 2023 16:29

yanavlasov self-assigned this Aug 15, 2023

yanavlasov requested changes Aug 16, 2023

View reviewed changes

repokitteh-read-only bot added waiting and removed waiting labels Aug 16, 2023

danieldradware force-pushed the issue-25239-jwt-authn-filter-is-pass-invalid-authorization-bearer branch from 5e0be3c to 3ac8ab9 Compare August 17, 2023 05:31

repokitteh-read-only bot added the waiting label Aug 22, 2023

danieldradware force-pushed the issue-25239-jwt-authn-filter-is-pass-invalid-authorization-bearer branch from ced687c to ba5972b Compare August 22, 2023 14:37

repokitteh-read-only bot removed the waiting label Aug 22, 2023

danieldradware added 5 commits August 22, 2023 18:17

signed-off-by: danieldradware danield@radware.com fix for token with …

cd3877d

…non base64 characters Signed-off-by: danield <danield@radware.com>

signed-off-by danieldradware danield@radware.com fix spelling

b69baef

Signed-off-by: danield <danield@radware.com>

signed-off-by danieldradware danield@radware.com fix spelling errors

0d90d8c

Signed-off-by: danield <danield@radware.com>

Signed-off-by danieldradware danield@radware.com const char*

d5985e6

Signed-off-by: danield <danield@radware.com>

-sdanieldradware danield@radware fix unit test with wrong type

925107c

Signed-off-by: danield <danield@radware.com>

danieldradware added 3 commits August 23, 2023 06:35

danieldradware danield@radware.com format errors

7c59a61

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radware.com format errors

652deff

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radware.com added unit test for runtime_guard …

b1e29e5

…(dis) Signed-off-by: danield <danield@radware.com>

repokitteh-read-only bot added the waiting:any label Aug 29, 2023

danieldradware danield@radware.com add unit test

eca1a08

Signed-off-by: danield <danield@radware.com>

repokitteh-read-only bot removed the waiting:any label Aug 29, 2023

danieldradware and others added 7 commits August 29, 2023 17:54

Merge branch 'envoyproxy:main' into issue-25239-jwt-authn-filter-is-p…

0680009

…ass-invalid-authorization-bearer

danieldradware danield@radware.com add unit test

e6337cc

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radwae.com fix format

730cf25

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radwae.com fix format

ddcc1c0

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radwae.com fix format

94ad520

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radwae.com fix format

b762b8a

Signed-off-by: danield <danield@radware.com>

danieldradware danield@radwae.com fix unit test

afce8f4

Signed-off-by: danield <danield@radware.com>

yanavlasov requested changes Sep 1, 2023

View reviewed changes

repokitteh-read-only bot added the waiting label Sep 1, 2023

Update runtime_features.cc

5364858

removed RUNTIME_GUARD(envoy_reloadable_features_uhv_preserve_url_encoded_case); Signed-off-by: danieldradware <117576776+danieldradware@users.noreply.github.com>

repokitteh-read-only bot removed the waiting label Sep 3, 2023

yanavlasov approved these changes Sep 5, 2023

View reviewed changes

yanavlasov merged commit aba7518 into envoyproxy:main Sep 5, 2023
116 checks passed

samuelvl mentioned this pull request Dec 20, 2023

Can't use JWT and basic authorization in 1.28 #31468

Closed

adisuissa mentioned this pull request Apr 17, 2024

envoy_reloadable_features_token_passed_entirely deprecation #33627

Closed

adisuissa mentioned this pull request May 20, 2024

Removed all parts related to runtime guard - https://github.com/envoy… #34199

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Bugfix issue 27877 jwt token with space is valid #28678

Bugfix issue 27877 jwt token with space is valid #28678

danieldradware commented Jul 28, 2023 •

edited by yanavlasov

Loading

yanavlasov commented Aug 1, 2023

yanavlasov commented Aug 1, 2023

nareddyt Aug 1, 2023

danieldradware Aug 1, 2023

yanavlasov Aug 16, 2023

danieldradware commented Aug 9, 2023

yanavlasov left a comment

yanavlasov Aug 16, 2023

repokitteh-read-only bot commented Aug 16, 2023

yanavlasov commented Aug 22, 2023

yanavlasov commented Aug 22, 2023

phlax commented Aug 29, 2023

danieldradware commented Aug 31, 2023

danieldradware commented Aug 31, 2023

yanavlasov left a comment

yanavlasov Sep 1, 2023

yanavlasov commented Sep 1, 2023

danieldradware commented Sep 3, 2023

danieldradware commented Sep 3, 2023

zirain commented Sep 3, 2023

danieldradware commented Sep 4, 2023

		RUNTIME_GUARD(envoy_reloadable_features_uhv_allow_malformed_url_encoding);
		RUNTIME_GUARD(envoy_reloadable_features_uhv_preserve_url_encoded_case);

Bugfix issue 27877 jwt token with space is valid #28678

Bugfix issue 27877 jwt token with space is valid #28678

Conversation

danieldradware commented Jul 28, 2023 • edited by yanavlasov Loading

yanavlasov commented Aug 1, 2023

yanavlasov commented Aug 1, 2023

nareddyt Aug 1, 2023

Choose a reason for hiding this comment

danieldradware Aug 1, 2023

Choose a reason for hiding this comment

yanavlasov Aug 16, 2023

Choose a reason for hiding this comment

danieldradware commented Aug 9, 2023

yanavlasov left a comment

Choose a reason for hiding this comment

yanavlasov Aug 16, 2023

Choose a reason for hiding this comment

repokitteh-read-only bot commented Aug 16, 2023

yanavlasov commented Aug 22, 2023

yanavlasov commented Aug 22, 2023

phlax commented Aug 29, 2023

danieldradware commented Aug 31, 2023

danieldradware commented Aug 31, 2023

yanavlasov left a comment

Choose a reason for hiding this comment

yanavlasov Sep 1, 2023

Choose a reason for hiding this comment

yanavlasov commented Sep 1, 2023

danieldradware commented Sep 3, 2023

danieldradware commented Sep 3, 2023

zirain commented Sep 3, 2023

danieldradware commented Sep 4, 2023

danieldradware commented Jul 28, 2023 •

edited by yanavlasov

Loading