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

Consider dropping go-jwt in favor of go-jose #514

Closed
mitar opened this issue Oct 19, 2020 · 5 comments
Closed

Consider dropping go-jwt in favor of go-jose #514

mitar opened this issue Oct 19, 2020 · 5 comments
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.

Comments

@mitar
Copy link
Contributor

mitar commented Oct 19, 2020

go-jwt seems to not be maintained really. It has open/known security issue (which have multiple PRs open to address, earliest in 2018: dgrijalva/jwt-go#286). And it requires numbers to be floats because it cannot validate ints. PR to fix this has been made in 2016. This is why fosite has to issue float values as expiration timestamps which are not necessary the most compatible. See my comment here.

Fosite is already using go-jose which seems to be more maintained. So not sure why not fully switch to it?

Or we could at least use a fork of go-jwt?

@aeneasr aeneasr added feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one. labels Oct 20, 2020
@aeneasr
Copy link
Member

aeneasr commented Oct 20, 2020

Yes, let's do that!

@mitar
Copy link
Contributor Author

mitar commented Jan 21, 2021

Another reason to move to go-jose is that go-jose supports clock skew leeway in validation: https://github.com/square/go-jose/blob/v2/jwt/validation.go#L68-L75

go-jwt does not: https://github.com/dgrijalva/jwt-go/blob/master/claims.go#L29

@aeneasr
Copy link
Member

aeneasr commented Jan 26, 2021

Absolutely, if anyone is up for the challenge I'm happy to help!

@narg95
Copy link
Contributor

narg95 commented May 5, 2021

Hi, I would propose to first migrate adapting go-Jose to jwt-go types. I took a look and it is feasible without much friction/adaptation. The following jwt-go types needs to be moved to Fosite’s jwt package , which are simple and small: jwt.Claims Interface, jwt.Token Struct (only Claims and Header fields are used in fosite), claims validation logic with its validation errors. I anticipate also that concrete JWT Strategies (RSA, ECDA) will get short or even just type aliases because go-Jose provides a common interface independent of the signing algorithm, it does type checking on the private key to determine which logic and signing algorithm to use, that means less code to maintain in fosite. The biggest advantage of this approach is having a low impact on fosite external interfaces, only is required changing one import from jwt-go to fosite’s jwt package, something easy to document in a change log, also unit tests do not need to be changed providing a good level of confidence in the correctness of the adaptation.

in a second phase we can take advantage of go-Jose extra capabilities.

what do you think? I think, given it’s expected size I would be able to contribute to this change.

@aeneasr
Copy link
Member

aeneasr commented May 5, 2021

That sounds like a great plan!

narg95 added a commit to narg95/fosite that referenced this issue May 22, 2021
Closes ory#514

Co-authored-by: hackerman <3372410+aeneasr@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request. good first issue A good issue to tackle when being a novice to the project. help wanted We are looking for help on this one.
Projects
None yet
Development

No branches or pull requests

3 participants