Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth: Use URL-encoding when adding URL as query parameters. #25278

Merged
merged 7 commits into from
Feb 8, 2023

Conversation

yanavlasov
Copy link
Contributor

Commit Message:
Decode these query parameters using reverse algorithm but leaving intact character sequences that must be encoded in URLs.

Additional Description:
This change addresses issue raised in #23167, which prevents OAuth filter from working with URLs that contain %-encoded UTF-8 characters in URL path or query.

During development I have also noticed that OAuth uses an odd character set for encoding URLs, which does not correspond to any standard I know. I have replaced it as well with well defined URL-encoding.

The behavioral changes are reflected in the additional tests.

Risk Level: Medium, Behavioral change in OAuth filter
Testing: Unit Tests
Docs Changes: Yes
Release Notes: Yes
Platform Specific Features: N/A
Optional Runtime guard: envoy.reloadable_features.oauth_use_url_encoding
Optional Fixes #23167

Signed-off-by: Yan Avlasov yavlasov@google.com

Decode these query parameters using reverse algorithm but leaving
intact character sequences that must be encoded in URLs.

Signed-off-by: Yan Avlasov <yavlasov@google.com>
@repokitteh-read-only
Copy link

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

🐱

Caused by: #25278 was opened by yanavlasov.

see: more, trace.

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@wbpcode
Copy link
Member

wbpcode commented Feb 8, 2023

friendly ping @snowp

Copy link
Contributor

@snowp snowp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Makes sense to me

@@ -37,6 +37,20 @@ is set to true the filter will send over a
cookie named ``BearerToken`` to the upstream. Additionally, the ``Authorization`` header will be populated
with the same value.

The OAuth filer encodes URLs in query parameters using the
`URL encoding alogirthm. <https://www.w3.org/TR/html5/forms.html#application/x-www-form-urlencoded-encoding-algorithm>`_
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: algorithm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Comment on lines +1112 to +1115
bool testChar(const uint32_t table[8], char c) {
uint8_t uc = static_cast<uint8_t>(c);
return (table[uc >> 5] & (0x80000000 >> (uc & 0x1f))) != 0;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this an existing algorithm for checking whether a char is one of a well known set of chars? Not immediately obvious to me how it this works. First part takes the most significant few bits and use them to index into one of the bitmasks, and then the least significant bits are compared against the bitmask?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we use it in other places, like UHV. It is CPU cache friendly bit array lookup.

The character validity check could be implemented as a table of size 256 with 0 or 1 in each element indicating character validity. This algorithm packs all 1s and 0s into bits of 32 bit words. That expression with shifts is equivalent to division by 32 and getting the remainder of division by 32 to figure which 32 bit word and which bit in that word to check. It is more CPU cache friendly because table size it 64 bytes instead of 256.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks for explaining!

Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
Signed-off-by: Yan Avlasov <yavlasov@google.com>
@yanavlasov yanavlasov merged commit fd874d9 into envoyproxy:main Feb 8, 2023
@yanavlasov yanavlasov deleted the decode-url-query branch February 8, 2023 20:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OAuth2 filter fails a request with Non-ASCII characters
3 participants