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

JWE Support #1

Merged
merged 5 commits into from
Aug 15, 2022
Merged

JWE Support #1

merged 5 commits into from
Aug 15, 2022

Conversation

ortyomka
Copy link
Contributor

@ortyomka ortyomka commented Jul 6, 2022

Issue golang-jwt/jwt#67

Add support of JWE with one key (Compact)

Add AES GCM cipher to encrypt content
Add RSA-OAEP to encrypt key

Add test from RFC7516 Section 3.3

cc @mfridman @oxisto

Signed-off-by: ortyomka <iurin.art@gmail.com>
Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka
Copy link
Contributor Author

ortyomka commented Jul 8, 2022

@oxisto if you have free time, review the PR, please

@ortyomka
Copy link
Contributor Author

I would like to finish this PR by Tuesday(12.07) inclusive.

If your schedule allows it, I'm willing to fix comments ASAP.

CC @mfridman @oxisto

@mfridman
Copy link
Member

mfridman commented Jul 11, 2022

Hey there, thanks for putting a PR .. additions/changes related to security take time to review. It requires brushing up on the JWE specification, ensuring the implementation matches and there are no bugs or security issues to the best of our abilities/knowledge.

Furthermore, a lot of folks submit PR's, but then it's the maintainers who are on the hook to support that code and address bugs, issues, improvements, review new PR's, etc.

I can't speak for others, but right now is the end of quarter crunch time .. so my availability for open-source projects is limited. Tbh, adding JWE support isn't very high on my list and I'd rather not rush something for the sake of getting it out there.

@ortyomka
Copy link
Contributor Author

I have understood your position. Thank you for your response.

Copy link
Collaborator

@oxisto oxisto left a comment

Choose a reason for hiding this comment

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

First set of review done. Thanks again for this contribution, it is very welcome. Sorry for the long delay in reviewing this.

There are still some kinks to work out in the (public) API, but since we are no way near releasing any 1.x version for this, it is ok and we can merge it once a few of the comments are addressed. Then afterwards, this probably needs another fine-tuning of the API.

LICENSE Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
jwe.go Outdated Show resolved Hide resolved
jwe.go Show resolved Hide resolved
jwe_decrypt.go Show resolved Hide resolved
jwe_decrypt.go Outdated Show resolved Hide resolved
jwe_parse.go Show resolved Hide resolved
staticcheck.conf Outdated Show resolved Hide resolved
oxisto and others added 2 commits August 14, 2022 17:18
Fix small issues

Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka
Copy link
Contributor Author

@oxisto Thank you for review. I fixed all comments

@ortyomka ortyomka requested a review from oxisto August 15, 2022 05:02
@oxisto
Copy link
Collaborator

oxisto commented Aug 15, 2022

@oxisto Thank you for review. I fixed all comments

Thank you! I have one small last comment for the errors and then I would suggest merging this initial version. Everything else can be done in increments.

Signed-off-by: ortyomka <iurin.art@gmail.com>
@ortyomka
Copy link
Contributor Author

@oxisto Added your suggestion

@oxisto oxisto merged commit 15b79f6 into golang-jwt:main Aug 15, 2022
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.

None yet

3 participants