Generation and parsing of jwt supports string type keys

[guonaihong]

Type conversion, key will not allocate memory

[mfridman]

Thank you for opening a PR.

There has to be an extremely good rationale for introducing the unsafe package backed with benchmark results.

[guonaihong]

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.

// reflect.StringHeader

type StringHeader struct {
        Data uintptr
        Len  int
}
/*
┌────────────────┐
│                │
│                │
│  Data(uintptr) │
│                │
│  8byte         │
│                │
├────────────────┤
│                │
│                │
│  Len(int)      │
│                │
│  8byte         │
│                │
└────────────────┘
 */

// reflect.SliceHeader
type SliceHeader struct {
        Data uintptr
        Len  int
        Cap  int
}

/*
┌───────────────────┐
│                   │
│   Data(uintptr)   │
│                   │
│   8byte           │
│                   │
│                   │
├───────────────────┤
│                   │
│   Len(int)        │
│                   │
│   8byte           │
│                   │
│                   │
├───────────────────┤
│                   │
│   Cap(int)        │
│                   │
│   8byte           │
│                   │
│                   │
└───────────────────┘
 */

[guonaihong]

package test

import (
	"reflect"
	"testing"
	"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
}

func BenchmarkStringToBytes_Std(t *testing.B) {
	s := "hello world"
	for i := 0; i < t.N; i++ {
		b := []byte(s)
		_ = b
	}
}

func BenchmarkStringToBytes(t *testing.B) {
	s := "hello world"
	for i := 0; i < t.N; i++ {
		b := StringToBytes(s)
		_ = b
	}
}

goos: darwin
goarch: amd64
pkg: test
cpu: Intel(R) Core(TM) i7-1068NG7 CPU @ 2.30GHz
BenchmarkStringToBytes_Std-8   	296108143	         3.970 ns/op	       0 B/op	       0 allocs/op
BenchmarkStringToBytes-8       	1000000000	         0.2813 ns/op	       0 B/op	       0 allocs/op
PASS
ok  	test	2.305s

[guonaihong]

I added benchmark data @mfridman

[oxisto]

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?

[guonaihong]

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?

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

[mfridman]

Putting aside performance, if the caller has a string it is trivial to pass []byte("string"). I general I wish this wasn't an interface{} 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.

[dillonstreator]

I ran into this same issue of expecting that Sign would accept a string type and ended up opening #245 without noticing this PR..

Do we gain anything by passing that type conversion to this library? Maybe? I'm trying to understand which problem we're solving.

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.

[dillonstreator]

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

[oxisto]

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.

[oxisto]

Closing this stale PR. Accepting string for an HMAC key is a bad idea. See the discussion in #249 for more details.