-
Notifications
You must be signed in to change notification settings - Fork 994
Add gzip compression option for the JWT Claims part #102
Conversation
👍 for compression support, but I'm reluctant to support a breaking change to token.New() Maybe a token.NewCompressed() or something? And let the old one internally default to jwt.CompressionNone |
Do any other major JWT libs do gzip compression of claims? (out of On Tue, Dec 15, 2015 at 5:43 PM Ben Brooks notifications@github.com wrote:
|
I'd just be cautious about actually gzipping claims—the spec doesn't say Importantly though, what browser-based gzip libs exist for decompressing On Tue, Dec 15, 2015 at 5:55 PM Ben Brooks notifications@github.com wrote:
|
To be fair, you could just compress any huge byte slices before you put it in the JWT. And decompress when you want to use the data. This has the advantage of sticking to the spec whilst giving you a huge compressed blob to consume. (using ridiculous never-in-the-real-world sized sample data) ![screen shot 2015-12-15 at 10 41 06](https://cloud.githubusercontent.com/assets/1525809/11808427/60f1c6da-a318-11e5-94d6-c6b967d03449.png) |
First, I agree, that adding a new method and not breaking the existing API could be better. I wanted to see what @dgrijalva would say about it, but it seems that he is taking a break from github right now :). Also, gzipping the claims is not enforced. We use JWT in a RESTful application, the claims won't be read by the client, only by the server which have created them in the first place. It is useful only in such restricted use case, but we are not trying to make everyone happy here anyway. Also, yes, I can gzip my data before passing it to JWT, but I try to contribute some useful changes to the upstream if I can. I rather have it done in JWT, makes the code cleaner and more reusable. |
Why not just compress the whole blob? |
It seems the right place to add a feature like this would be in the spec. Though, there are several other ways to implement this without violating the spec.
For this library, I'd be most comfortable with making compression possible with an external add on. I'd like to adhere to the spec as much as possible, without closing doors for users to do what they'd like. |
} | ||
|
||
if compression, err = getCompressionMethod(t.Header["cpr"]); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this may break older versions of the token
@dgrijalva how can you make this as an addon? by implementing a wrapper encryption method? |
Here's a function that will compress and re-base64url encode the content. Because of the double encoding, the content ends up being larger. This may not hold true for larger tokens. Give it a try and let me know what you think? func compressedToken(s string)string {
buf := new(bytes.Buffer)
enc := base64.NewEncoder(base64.URLEncoding, buf)
wr := gzip.NewWriter(enc)
wr.Write([]byte(s))
wr.Close()
enc.Close()
return string(buf.Bytes())
} |
Unless this gets added to the spec, I don't think its inclusion is wise. If you need new hooks allow you to accomplish this outside the library, let's have that discussion instead. |
Just poking back here: regardless of the spec, we came to the conclusion that most mediums for storage or transfer of JWT tokens are already supporting their lower-layer compression (before filesystem or network interaction). Ref: lcobucci/jwt#82 (comment) (just to provide arguments, should this come up again) |
Compression is part of JOSE specs on IANA, reference: https://www.iana.org/assignments/jose/jose.xhtml and also RFC 7516. @szank @dgrijalva : If the change mentioned here is restricted to zip header with Deflate algorithm, it will be compliant with specs and will achieve the objective (i.e. compression). |
No description provided.