-
Notifications
You must be signed in to change notification settings - Fork 352
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
Supporting alternative base64/json encoder/decoder #168
Comments
In theory, that could be an interesting option to explore. However, because this library is used by many users, we basically have two major design principles maybe should write them down somewhere :):
So, if we find a way that is non-breaking (and defaults for the stdlib), has no dependencies to the mentioned project (which should be doable, if we introduce interfaces here), I would not strongly object it. Personally, I do not see a major use case though. Not having any hard numbers here, but my guess is that the most expensive operation is the crypto part, so not sure how large the performance gain really is. I do get the idea of having the other libraries in your project and having this consistent. Testing it would be a little tricky though, without importing the other libraries and we cannot really make any guarantees that everything works with external libraries. Would you mind drafting up a PR? |
I can look through it. The nice part about working through interfaces here is we wouldn't need to add any external dependencies. We can just define a I agree that crypto bits would likely outweigh this, but if you're operating on the order of tens of thousands of rps, these wins with encoding/decoding are likely a measurable %age gain. I'll probably take a pass and see what a theoretical implementation would look like and throw together a benchmark to prove/disprove that this would be worthwhile. |
I had a quick look, starting with the base64 part and I fear that this is not done easily. The main encoding is done in diff --git a/token.go b/token.go
index 09b4cde..d6a576e 100644
--- a/token.go
+++ b/token.go
@@ -7,7 +7,6 @@ import (
"time"
)
-
// DecodePaddingAllowed will switch the codec used for decoding JWTs respectively. Note that the JWS RFC7515
// states that the tokens will utilize a Base64url encoding with no padding. Unfortunately, some implementations
// of JWT are producing non-standard tokens, and thus require support for decoding. Note that this is a global
@@ -26,6 +25,9 @@ var TimeFunc = time.Now
// Header of the token (such as `kid`) to identify which key to use.
type Keyfunc func(*Token) (interface{}, error)
+type EncodeSegmentFunc func([]byte) string
+type DecodeSegmentFunc func(string) ([]byte, error)
+
// Token represents a JWT Token. Different fields will be used depending on whether you're
// creating or parsing/verifying a token.
type Token struct {
@@ -35,6 +37,8 @@ type Token struct {
Claims Claims // The second segment of the token
Signature string // The third segment of the token. Populated when you Parse a token
Valid bool // Is the token valid? Populated when you Parse/Verify a token
+
+ b64enc EncodeSegmentFunc
}
// New creates a new Token with the specified signing method and an empty map of claims.
@@ -76,15 +80,19 @@ func (t *Token) SigningString() (string, error) {
var err error
var jsonValue []byte
+ if t.b64enc == nil {
+ t.b64enc = EncodeSegment
+ }
+
if jsonValue, err = json.Marshal(t.Header); err != nil {
return "", err
}
- header := EncodeSegment(jsonValue)
+ header := t.b64enc(jsonValue)
if jsonValue, err = json.Marshal(t.Claims); err != nil {
return "", err
}
- claim := EncodeSegment(jsonValue)
+ claim := t.b64enc(jsonValue)
return strings.Join([]string{header, claim}, "."), nil
} This could then be configured by Problematic is the decode part. Decoding is done in 7 places (1x for the header, 1x for the claim and 5x for the individual signatures). Header and claim are probably easy because they are part of the |
After fine-tuning the structure of Token, I tested the And promising no breaking changes, I'll raise an PR to implement this idea. func BenchmarkSigningString(t *testing.B) {
token := &jwt.Token{
Raw: data.fields.Raw,
Method: data.fields.Method,
Header: data.fields.Header,
Claims: data.fields.Claims,
Signature: data.fields.Signature,
Valid: data.fields.Valid,
}
t.Run("std", func(b *testing.B) {
for i := 0; i < b.N; i++ {
_, _ = token.SigningString()
}
})
t.Run("sonic", func(b *testing.B) {
token.JsonEncFunc = sonic.Marshal
for i := 0; i < b.N; i++ {
_, _ = token.SigningString()
}
})
} Benchmark:
|
@mattrobenolt @dcalsky Now that this should be possible with v5 (at least for b64), would any of you be interested in drafting a PR? |
I'd like to. I'll submit the PR after ending my vacation. |
Hi everyone. Does this issue need a new PR to keep it moving for custom Json encoder/decoder? |
It is actually already part of #301. Progress on this is slow for two reasons:
|
Would you be open to supporting alternative base64 implementations? The one we've been using is https://github.com/segmentio/asm/tree/main/base64
It's significantly faster than the stdlib encoding, and takes shortcuts that I don't see why wouldn't be acceptable here.
Ideally, if the token parsers were abstracted to accept a
Encoding
interface, we could pass in our own, with default behaviors to maintainencoding/base64
.In similar vein, similar can be said for JSON codecs. The stdlib is known for having sub-optimal performance here. Swapping
encoding/json
for something like,fastjson
(https://github.com/valyala/fastjson) would be nice.I'm new to this project and haven't dug in much, so I don't know all the consequences of this or how to gauge it yet, but I know for us internally, we've adopted these codec alternatives for packages we control.
The text was updated successfully, but these errors were encountered: