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

Sign JSON Web Token (JWT) #772

Merged
merged 4 commits into from
May 26, 2021
Merged

Sign JSON Web Token (JWT) #772

merged 4 commits into from
May 26, 2021

Conversation

manute
Copy link
Contributor

@manute manute commented May 23, 2021

@manute
Copy link
Contributor Author

manute commented May 23, 2021

WIP

First Implementation to see if the approach I took is the correct one.
Also I would like to test it but I could not see other tests for similar cases like OAuth/2, so I was not sure how to test it.
Any feedback is really appreciated 👍

Copy link
Collaborator

@Jeffail Jeffail left a comment

Choose a reason for hiding this comment

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

Hey @manute, a couple of comments but this looks fine to me.

lib/util/http/auth/jwt.go Outdated Show resolved Hide resolved
lib/util/http/auth/jwt.go Outdated Show resolved Hide resolved
lib/util/http/auth/jwt.go Outdated Show resolved Hide resolved
@manute
Copy link
Contributor Author

manute commented May 24, 2021

@Jeffail I think I've addressed all the feedback, thanks

@Jeffail
Copy link
Collaborator

Jeffail commented May 26, 2021

Thanks @manute, this looks great. The pipeline will fail as the docs need generating with make docs, but as long as the unit tests pass I'll merge and do that myself.

@Jeffail Jeffail merged commit 504f0d6 into redpanda-data:master May 26, 2021
@mihaitodor
Copy link
Collaborator

mihaitodor commented May 27, 2021

@manute @Jeffail I know this was merged, but I had a look at dgrijalva/jwt-go library and it looks like they have a preview release for a CVE as described here. Not sure if it's an issue for this use case. From the comments, it looks like form3 adopted a fork of it and made a bunch of security fixes. Then again, it seems this will be the new home for this library based on this conversation, but they just started moving it there.

@manute
Copy link
Contributor Author

manute commented May 27, 2021

@mihaitodor thanks for the heads up. I believe this should be addressed in other PR, and when the official new home for this lib has added that sec issue. Because base on its README :

NEW VERSION COMING: There have been a lot of improvements suggested since the version 3.0.0 released in 2016. I'm working now on cutting two different releases: 3.2.0 will contain any non-breaking changes or enhancements. 4.0.0 will follow shortly which will include breaking changes. See the 4.0.0 milestone to get an idea of what's coming. If you have other ideas, or would like to participate in 4.0.0, now's the time. If you depend on this library and don't want to be interrupted, I recommend you use your dependency mangement tool to pin to version 3.

@mihaitodor
Copy link
Collaborator

I agree. I opened an issue to keep track of this situation: #779.

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.

3 participants