auth
auth copied to clipboard
go-pkgz
JWT: Upgrade from v3 to v5
The current jwt code has been deprecated in the upstream This PR aims to upgrade the library to v5.
This will make upgrading for security vulnerabilities / bugs easier in the future.
Big Changes
-
StandardClaimshas been deprecated -- RegisteredClaims is the new struct to use. - The library prefers audience as an array (but allows us to turn it off with the global variable
MarshalSingleStringAsArray)
I resolved a go mod conflict, but then forgot to run go mod tidy -- should work now.
Sorry about that. I got it to pass the build and test portion in my forked repo. Now I'm stuck at a linting error that says I can't use init() functions. As a general practice, I think that makes sense, but I think this is a reasonable usecase. The new jwt package has a global variable that needs to be set to true before any of the auth package code runs. How would you like me to approach that?
How would you like me to approach that?
can this be set in NewService (auth.go)? Or this be too late?
The unit tests in the token package would stop working. I think the init function is the right approach in this case, because we are configuring the jwt package before we run any code from the token package.
Well, the token's Service has token.NewService constructor, which to me seems to be the good place to put this jwt init thing. From what i can see all the tests for token start with NewService
Moved the variable. The linter didn't seem to run right on my fork, but at least the tests did.
there we go! all passed :sweat_smile: https://github.com/freddierice/go-pkgz-auth/actions/runs/7563891254/job/20597114793
Pull Request Test Coverage Report for Build 7563891591
- 0 of 0 changed or added relevant lines in 0 files are covered.
- No unchanged relevant lines lost coverage.
- Overall coverage decreased (-0.07%) to 82.676%
| Totals | |
|---|---|
| Change from base Build 7552291388: | -0.07% |
| Covered Lines: | 2577 |
| Relevant Lines: | 3117 |
💛 - Coveralls
Just so you know these are the changes which have to be implemented in Remark42 after updating to the go-pkgz/auth version from that PR. The changes are significant enough to release v2.
I propose to mark the current master as the last release of v1 and then bump the major version of this library after the merge. I thought of updating to jwt v4 many times before, but the necessity to break backwards compatibility stopped me.
The part that worries me is that you had to change JWT for tests. I thought the reason was aud related, but it seems to have a string aud, not an array. So, I'm puzzled why it was needed
Before:
after:
I don't see any reason why this would be the case, and this is something I want to understand. As long as both versions produce the same jwt the change would be back compatible, but if for some reason it is not - this is kind of big deal as we may invalidate all the "sessions" and should carefully test the migration path, i.e. what will happen if the new version will get "old" token
But as I said above - I don't really get why token is different
The order of "aud" and "iss" are different because the new version of the library changed the types of each of the members. "aud" is now a union type of string and []string. While the jwt string itself is different, it will get decoded the same.
For reference: https://pkg.go.dev/github.com/golang-jwt/jwt/[email protected]#RegisteredClaims vs https://pkg.go.dev/github.com/golang-jwt/jwt#StandardClaims
The order of "aud" and "iss" are different because the new version of the library changed the types of each of the members. "aud" is now a union type of
stringand[]string. While the jwt string itself is different, it will get decoded the same.
Will the new version properly verify the old message? It should be because the signature matches the content. But what will happen next? I mean, will the old message be correctly decoded/unmarshaled, or will the order of fields prevent it from happening?
Having a test consuming the old token should help to clarify this scenario
Field ordering doesn't matter for golang's json.Unmarshal functions. Unfortunately I can't put any more time into this, but feel free to take/leave whichever parts you want.
@freddierice Thank you very much, we will deal with this.
@paskal assuming old tokens will continue to work, what is the part breaking back compatibility? I don't see anything on the exposed level that will require any change on the consumer side. I have not checked it in detail, but on the surface, it seems to be back-compatible.
Here are the changes due to migration to v5. I prefer to cut the current master as v1 of this library and release version v2 with JWTv5 + a minor breaking change of setting the type explicitly here in RefreshCache as it has interface{} in its signature purely by mistake.
As a user of this library JWTv5 changes seem inconvenient enough to cut a new release for me.
I've prepared https://github.com/go-pkgz/auth/pull/200, and once it's merged, I'll ask you to change the PR to target the v2 directory.
I want to clarify one interface used for cache to specify types explicitly, which is a breaking change strictly speaking. I consider changes in this PR breaking as well, as they require some users to change their code after the update.
FYI, I think you can copy all changed files to v2, fix imports, re-run go get to get v5 of JWT, and it will be enough to get PR ready for v2.
The changes in branch https://github.com/go-pkgz/auth/tree/paskal/v2_jwt5 have been moved to the v2 directory. Feel free to copy them to this PR.
@freddierice could you port your work to v2 please? We would be able to release it then with your changes.
@freddierice, could you please add your changes to v2 please? That is the only thing preventing us from releasing it.