Switch to using a single JWT library and unify api package #119
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
sethvargo
requested review from
a team,
raserva,
sqin2019,
capri-xiyue and
yolocs
sethvargo
commented
Sep 21, 2022
sethvargo
force-pushed
the
sethvargo/jwt
branch
from
7608a21 to
22df428
Compare
The current implementation uses two JWT packages: github.com/golang-jwt/jwt and github.com/lestrrat-go/jwx/v2/jwt. These packages have very different APIs and developer surfaces. This PR standardizes everything on github.com/lestrrat-go/jwx/v2/jwt since we need to the jwx package anyway for jwk parsing and jws signing. Because github.com/lestrrat-go/jwx/v2/jwt uses an interface to store claims, this PR introduces new top-level functions for getting/setting justifications on a token. This is significantly less error-prone than relying on "justs" to be populated in the JSON and enforces a structure. The downside is that it requires an extra option during parsing, but that is abstracted away into helpers. These helpers are important because the previous implementation had some extremely rough edges because slices are passed by reference. This caused data races and extremely-difficult-to-debug side-effects. The helpers make a copy of slices to ensure this doesn't happen. One of the major downsides of the github.com/lestrrat-go/jwx/v2/jwt package is that there is no way to create "unsigned" tokens. github.com/golang-jwt/jwt has a "SigningString()" function for this purpose, but that does not exist in the new package. In retrospect, I'm not convinced that using the ".NOT_SIGNED" suffix is the right solution for breakglass, and I think we should explore using HMAC-signed keys instead. That being said, I was able to preserve backwards-compatibility with the existing implementation by signing the key, stripping off the signature, and appending ".NOT_SIGNED". This is less than ideal, but I think any refactors to the breakglass semantics should be done in a separate PR and discussion. This PR also has some general code cleanup, such as being consistent with the name of the jvsapis/v0 package import.
sethvargo
force-pushed
the
sethvargo/jwt
branch
from
22df428 to
d07c8fb
Compare
yolocs
reviewed
Sep 21, 2022
sethvargo
force-pushed
the
sethvargo/jwt
branch
from
5c58380 to
13806b4
Compare
yolocs
approved these changes
Sep 21, 2022
sqin2019
pushed a commit
that referenced
this pull request
Apr 6, 2023
* Switch to using a single JWT library and unify api package The current implementation uses two JWT packages: github.com/golang-jwt/jwt and github.com/lestrrat-go/jwx/v2/jwt. These packages have very different APIs and developer surfaces. This PR standardizes everything on github.com/lestrrat-go/jwx/v2/jwt since we need to the jwx package anyway for jwk parsing and jws signing. Because github.com/lestrrat-go/jwx/v2/jwt uses an interface to store claims, this PR introduces new top-level functions for getting/setting justifications on a token. This is significantly less error-prone than relying on "justs" to be populated in the JSON and enforces a structure. The downside is that it requires an extra option during parsing, but that is abstracted away into helpers. These helpers are important because the previous implementation had some extremely rough edges because slices are passed by reference. This caused data races and extremely-difficult-to-debug side-effects. The helpers make a copy of slices to ensure this doesn't happen. One of the major downsides of the github.com/lestrrat-go/jwx/v2/jwt package is that there is no way to create "unsigned" tokens. github.com/golang-jwt/jwt has a "SigningString()" function for this purpose, but that does not exist in the new package. In retrospect, I'm not convinced that using the ".NOT_SIGNED" suffix is the right solution for breakglass, and I think we should explore using HMAC-signed keys instead. That being said, I was able to preserve backwards-compatibility with the existing implementation by signing the key, stripping off the signature, and appending ".NOT_SIGNED". This is less than ideal, but I think any refactors to the breakglass semantics should be done in a separate PR and discussion. This PR also has some general code cleanup, such as being consistent with the name of the jvsapis/v0 package import. * Process review feedback * Add more defense about unmarshalling bad types
verbanicm
pushed a commit
that referenced
this pull request
Jun 14, 2023
* Switch to using a single JWT library and unify api package The current implementation uses two JWT packages: github.com/golang-jwt/jwt and github.com/lestrrat-go/jwx/v2/jwt. These packages have very different APIs and developer surfaces. This PR standardizes everything on github.com/lestrrat-go/jwx/v2/jwt since we need to the jwx package anyway for jwk parsing and jws signing. Because github.com/lestrrat-go/jwx/v2/jwt uses an interface to store claims, this PR introduces new top-level functions for getting/setting justifications on a token. This is significantly less error-prone than relying on "justs" to be populated in the JSON and enforces a structure. The downside is that it requires an extra option during parsing, but that is abstracted away into helpers. These helpers are important because the previous implementation had some extremely rough edges because slices are passed by reference. This caused data races and extremely-difficult-to-debug side-effects. The helpers make a copy of slices to ensure this doesn't happen. One of the major downsides of the github.com/lestrrat-go/jwx/v2/jwt package is that there is no way to create "unsigned" tokens. github.com/golang-jwt/jwt has a "SigningString()" function for this purpose, but that does not exist in the new package. In retrospect, I'm not convinced that using the ".NOT_SIGNED" suffix is the right solution for breakglass, and I think we should explore using HMAC-signed keys instead. That being said, I was able to preserve backwards-compatibility with the existing implementation by signing the key, stripping off the signature, and appending ".NOT_SIGNED". This is less than ideal, but I think any refactors to the breakglass semantics should be done in a separate PR and discussion. This PR also has some general code cleanup, such as being consistent with the name of the jvsapis/v0 package import. * Process review feedback * Add more defense about unmarshalling bad types
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
The current implementation uses two JWT packages: github.com/golang-jwt/jwt and github.com/lestrrat-go/jwx/v2/jwt. These packages have very different APIs and developer surfaces. This PR standardizes everything on github.com/lestrrat-go/jwx/v2/jwt since we need to the jwx package anyway for jwk parsing and jws signing.
Because github.com/lestrrat-go/jwx/v2/jwt uses an interface to store claims, this PR introduces new top-level functions for getting/setting justifications on a token. This is significantly less error-prone than relying on "justs" to be populated in the JSON and enforces a structure. The downside is that it requires an extra option during parsing, but that is abstracted away into helpers. These helpers are important because the previous implementation had some extremely rough edges because slices are passed by reference. This caused data races and extremely-difficult-to-debug side-effects. The helpers make a copy of slices to ensure this doesn't happen.
One of the major downsides of the github.com/lestrrat-go/jwx/v2/jwt package is that there is no way to create "unsigned" tokens. github.com/golang-jwt/jwt has a "SigningString()" function for this purpose, but that does not exist in the new package. In retrospect, I'm not convinced that using the ".NOT_SIGNED" suffix is the right solution for breakglass, and I think we should explore using HMAC-signed keys instead. That being said, I was able to preserve backwards-compatibility with the existing implementation by signing the key, stripping off the signature, and appending ".NOT_SIGNED". This is less than ideal, but I think any refactors to the breakglass semantics should be done in a separate PR and discussion.
This PR also has some general code cleanup, such as being consistent with the name of the jvsapis/v0 package import.
Closes #116