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

server/auth: fix panic a malformed jwt generation and add test-cases #15639

Merged
merged 2 commits into from
Apr 11, 2023

Conversation

ArkaSaha30
Copy link
Contributor

@ArkaSaha30 ArkaSaha30 commented Apr 4, 2023

This PR is to fix panic on identical JWT token generation and authentication as well as add test cases for the same.

Extension of #15002

Fixes: #14931

Signed-off-by: ArkaSaha30 <arkasaha30@gmail.com>
@ArkaSaha30 ArkaSaha30 force-pushed the add-unit-test-malformed-jwt branch from 96d9c72 to 386aede Compare April 4, 2023 12:32
@ahrtr
Copy link
Member

ahrtr commented Apr 5, 2023

Thanks @ArkaSaha30 . Please take this as a high priority, because it's planned to be included in 3.5.8.

@ArkaSaha30
Copy link
Contributor Author

Hi @ahrtr , just wanted to confirm if I need to create public.pem private.pem in ./tests/fixtures/ as a part of adding the test case?

@ahrtr
Copy link
Member

ahrtr commented Apr 5, 2023

Hi @ahrtr , just wanted to confirm if I need to create public.pem private.pem in ./tests/fixtures/ as a part of adding the test case?

Can you reuse the existing certificate and key files? Please refer to server/auth/jwt_test.go.

@ArkaSaha30
Copy link
Contributor Author

In server/auth/jwt_test.go will it be an addition to the map object in TestJWTInfo(). I am not sure what exact unit test case should be written around the change

@ahrtr
Copy link
Member

ahrtr commented Apr 5, 2023

You need to intentionally generate a JWT token without username or revision field, then call (*tokenJWT) info() with the token, you should get nil, false

@ahrtr
Copy link
Member

ahrtr commented Apr 6, 2023

@ArkaSaha30 Please remove WIP from the title once you finish this PR, and then I will take a look at this PR soon.

@ArkaSaha30 ArkaSaha30 changed the title [WIP]server/auth: fix panic a malformed jwt generation and add test-cases server/auth: fix panic a malformed jwt generation and add test-cases Apr 6, 2023
server/auth/jwt_test.go Outdated Show resolved Hide resolved
@ArkaSaha30 ArkaSaha30 requested a review from ahrtr April 7, 2023 09:37
@ArkaSaha30 ArkaSaha30 force-pushed the add-unit-test-malformed-jwt branch from 46b6da0 to 1b903d7 Compare April 7, 2023 09:42
server/auth/jwt_test.go Outdated Show resolved Hide resolved
server/auth/jwt_test.go Outdated Show resolved Hide resolved
@serathius
Copy link
Member

serathius commented Apr 7, 2023

This PR is blocking v3.5.8 release, please provide fixes as fast you can. Plan is to release next week, so please provide fixes till Monday or we will need to finish the PR. Sorry for not making clear earlier.

@ahrtr
Copy link
Member

ahrtr commented Apr 7, 2023

This PR is blocking v3.5.8 release, please provide fixes as fast you can. Plan is to release next week, so please provide fixes till Monday or we will need to finish the PR. Sorry for not making the importance the PR earlier.

I will work with @ArkaSaha30 to get it done next Monday. See also #15585 (comment)

@ArkaSaha30
Copy link
Contributor Author

I'm already working on it, I will push the changes for review ASAP.

@ahrtr
Copy link
Member

ahrtr commented Apr 10, 2023

@ArkaSaha30 Please follow #15676

@ahrtr
Copy link
Member

ahrtr commented Apr 10, 2023

Usually we fix issue on main branch firstly, and then backport to 3.5 and 3.4. But we have no time to wait, because we need to release both 3.5.8 and 3.4.25 soon to resolve some CVEs. So I delivered two PRs myself for both 3.5 and 3.4 before this one is approved/merged. Sorry for the confusion.

Anyway, please follow the same way to update this PR.

@ArkaSaha30
Copy link
Contributor Author

Thank you @ahrtr, I am updating this PR accordingly.

@ArkaSaha30 ArkaSaha30 force-pushed the add-unit-test-malformed-jwt branch from 1b903d7 to 971f732 Compare April 10, 2023 03:41
Copy link
Member

@jmhbnz jmhbnz left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @ArkaSaha30.

@ahrtr
Copy link
Member

ahrtr commented Apr 10, 2023

minor comment: please consider to squash the second and third commits.

Signed-off-by: ArkaSaha30 <arkasaha30@gmail.com>
@ArkaSaha30 ArkaSaha30 force-pushed the add-unit-test-malformed-jwt branch from 971f732 to a1fa3bf Compare April 10, 2023 04:09
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

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

LGTM

@ahrtr
Copy link
Member

ahrtr commented Apr 10, 2023

cc @mitake @spzala PTAL, this should be safe change.

@serathius serathius merged commit 6519a15 into etcd-io:main Apr 11, 2023
@mitake
Copy link
Contributor

mitake commented Apr 11, 2023

LGTM, thanks @ArkaSaha30 !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Generating identitcal JWT token and authenticating results in server go panic
6 participants