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

fix(jwt): deny requests that have different tokens in the jwt token search locations. #9946

Merged
merged 5 commits into from
Dec 29, 2022

Conversation

vm-001
Copy link
Contributor

@vm-001 vm-001 commented Dec 13, 2022

Summary

Deny requests that have different tokens in the jwt token search locations.

Checklist

Issue reference

FTI-4580

@vm-001 vm-001 force-pushed the fix/jwt-deny-multiple-token branch 2 times, most recently from 09184cb to bcc9528 Compare December 13, 2022 02:34
@vm-001 vm-001 marked this pull request as ready for review December 13, 2022 02:34
Copy link
Member

@dndx dndx left a comment

Choose a reason for hiding this comment

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

The general approach of this PR is fine. However, I see no good reason of returning a Lua table from retrieve_tokens just for duplicate detection. Maybe the retrieve_tokens function can simply return nil, "Multiple tokens provided" in case more than one unique token is present in the same request. This way we avoid two table allocations and use of pairs which could be expensive.

CHANGELOG.md Outdated Show resolved Hide resolved
@vm-001
Copy link
Contributor Author

vm-001 commented Dec 13, 2022

The general approach of this PR is fine. However, I see no good reason of returning a Lua table from retrieve_tokens just for duplicate detection. Maybe the retrieve_tokens function can simply return nil, "Multiple tokens provided" in case more than one unique token is present in the same request. This way we avoid two table allocations and use of pairs which could be expensive.

I've thought about this approach(And yes, this is a better way for performance). If retrieve_tokens returns nil, "Multiple tokens provided", it will run into. https://github.com/Kong/kong/blob/master/kong/plugins/jwt/handler.lua#L122. Instead of leveraging this predefined branch(https://github.com/Kong/kong/blob/master/kong/plugins/jwt/handler.lua#L129), adding an extra branch to return a detailed error seems a little bit ugly.

kong/plugins/jwt/handler.lua Outdated Show resolved Hide resolved
@vm-001 vm-001 requested a review from dndx December 21, 2022 09:11
CHANGELOG.md Show resolved Hide resolved

#### Plugins

- **JWT**: JWT plugin now denies a request that has different tokens in the jwt token search locations.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- **JWT**: JWT plugin now denies a request that has different tokens in the jwt token search locations.
- **JWT**: JWT plugin now denies a request that has different JWTs in the HTTP request header and Query parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are three search locations, which are querystring(query parameter), header, and cookie.

@vm-001 vm-001 added this to the 3.2 milestone Dec 28, 2022
@dndx dndx merged commit 81be86d into master Dec 29, 2022
@dndx dndx deleted the fix/jwt-deny-multiple-token branch December 29, 2022 08:43
oowl pushed a commit that referenced this pull request Aug 15, 2024
…ective_header` is readonly (#9946)

Panic on writing the EMPTY table

* Fix #13458
* https://konghq.atlassian.net/browse/KAG-5139?

(cherry-picked from commit #13491)

Co-authored-by: Qi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants