-
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
Generation and parsing of jwt supports string type keys #157
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package jwt | ||
|
||
import ( | ||
"reflect" | ||
"unsafe" | ||
) | ||
|
||
func StringToBytes(s string) (b []byte) { | ||
bh := (*reflect.SliceHeader)(unsafe.Pointer(&b)) | ||
sh := (*reflect.StringHeader)(unsafe.Pointer(&s)) | ||
bh.Data = sh.Data | ||
bh.Len = sh.Len | ||
bh.Cap = sh.Len | ||
return b | ||
} |
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.
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.
Thank you for opening a PR.
There has to be an extremely good rationale for introducing the
unsafe
package backed with benchmark results.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.
Thanks for your answer.
I'm in China and it's night time. I can add benchmark examples tomorrow.
First of all, the memory layout of string and []byte is similar.
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.
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.
I added benchmark data @mfridman
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 might be a bit biased, but I do not seem a good reason here to include
unsafe
code, despite the performance gain. If you want faster performance, why not just use[]byte
?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.
Using StringToBytes or [] byte to realize the type conversion from string to [] byte is not the key point. This PR is to make the sign and verify interface support string type
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.
Putting aside performance, if the caller has a string it is trivial to pass
[]byte("string")
. I general I wish this wasn't aninterface{}
but we can't break the contract in the current version.Do we gain anything by passing that type conversion to this library? Maybe? I'm trying to understand which problem we're solving.
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.
I ran into this same issue of expecting that Sign would accept a
string
type and ended up opening #245 without noticing this PR..It is friendlier to callers of this code.
Sure it's simple for the caller to wrap it but the same can be said for adding this directly into this package.
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.
If the conversion were to happen in jwt, I agree that it should not rely on unsafe and would recommend following the approach here #245
It looks like gin and fasthttp do this conversion:
https://github.com/gin-gonic/gin/blob/ee4de846a894e9049321e809d69f4343f62d2862/internal/bytesconv/bytesconv.go#L11-L24
https://github.com/valyala/fasthttp/blob/404c8a896896943715f8fb3906a8d054fae17d3e/bytesconv.go#L320-L343
heres an thread on golang-nuts about this and go team members directly recommending against it:
https://groups.google.com/g/Golang-Nuts/c/ENgbUzYvCuU/m/90yGx7GUAgAJ
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.
Why is your key a string in the first place? It shouldn’t be. The hmac package expects a byte slice, so that is why we only accept a byte slice. Having the key as a string for me is not a good design choice since this is not a „password“ and does not necessarily be human readible but a random byte slice.