golang-jwt / jwt

Go implementation of JSON Web Tokens (JWT).
https://golang-jwt.github.io/jwt/
MIT License
6.98k stars 335 forks source link

bug(token): Interface includes constraint elements, can only be used in type parameters #401

Closed godcong closed 1 week ago

godcong commented 1 month ago

the crypto.PublicKey is defined as any, although I don't know why it compiles. But the type checking does throw an exception

image

// VerificationKey represents a public or secret key for verifying a token's signature.
type VerificationKey interface {
    crypto.PublicKey | []uint8
}

// VerificationKeySet is a set of public or secret keys. It is used by the parser to verify a token.
type VerificationKeySet struct {
    Keys []VerificationKey
}

VerificationKeySet should also be changed to

type VerificationKeySet[T VerificationKey] struct {
    Keys []T
}

However, since any exists, this doesn't seem to make any sense.

Finally, I changed it to this.

// VerificationKey represents a public or secret key for verifying a token's signature.
type VerificationKey = any

// VerificationKeySet is a set of public or secret keys. It is used by the parser to verify a token.
type VerificationKeySet struct {
    Keys []VerificationKey
}
oxisto commented 1 month ago

Can you give us some more context where this error should occur? The type of VerificationKey is intentionally set to either crypto.PublicKey or []uint8'. Semanticallycrypto.PublicKey` is a distinct type and not "just anything". The issue is that for backwards compatibility the underlying type is "any", but that is an implementation detail of the Go standard library and should not concern this library.

godcong commented 1 month ago

It's not clear if this is a go or goland problem. But apparently this bypasses go's generic checking mechanism.

From the test results of the current definition, VerificationKey is always any. You can do some debugging in parser.go:96 Although the []uint8 type is defined, it does not work, it is always the type any([]uint8).

Or Alternatively, you can use the specified type at parser.go:96

switch have := got.(type) {
    case VerificationKeySet[crypto.PublicKey]:
        if len(have.Keys) == 0 {
            return token, newError("keyfunc returned empty verification key set", ErrTokenUnverifiable)
        }
        // Iterate through keys and verify signature, skipping the rest when a match is found.
        // Return the last error if no match is found.
        for _, key := range have.Keys {
            if err = token.Method.Verify(text, token.Signature, key); err == nil {
                break
            }
        }
    case VerificationKeySet[[]uint8]:
        // Do something with []uint8 here
    default:
        err = token.Method.Verify(text, token.Signature, have)
    }

But VerificationKeySet needs change to this.

type VerificationKeySet[T VerificationKey] struct {
    Keys []T
}

The associated tests parser_test.go should also be changed. But obviously, that's a big change.

oxisto commented 1 month ago

Ok I am beginning to understand your issue... I can finally reproduce this in my IntelliJ ultimate. This is actually a bug in the IDE it seems.

The following example works perfectly in VScode or directly using go run:

package main

import (
    "fmt"

    "github.com/golang-jwt/jwt/v5"
)

func main() {
    var key1 = []uint8{1, 2, 3, 4}
    var key2 = []uint8{2, 3, 4, 5}

    // create a token
    t := jwt.New(jwt.SigningMethodHS256)
    token, _ := t.SignedString(key1)

    var set jwt.VerificationKeySet = jwt.VerificationKeySet{
        Keys: []jwt.VerificationKey{
            key1, key2,
        },
    }

    t, err := jwt.Parse(token, func(t *jwt.Token) (interface{}, error) {
        return set, nil
    })
    if err == nil {
        fmt.Printf("Yay %s!\n", t.Method.Alg())
    }
}

This will print "Yay HS256!" on your console after go run main.go

But I am also seeing this error if I open this in IntelliJ...

oxisto commented 1 month ago

This seems to be a very annoying and old bug in Goland... https://youtrack.jetbrains.com/issue/GO-15406/False-positive-Interface-includes-constraint-elements-can-only-be-used-in-type-parameters

godcong commented 1 month ago

This seems to be a very annoying and old bug in Goland... https://youtrack.jetbrains.com/issue/GO-15406/False-positive-Interface-includes-constraint-elements-can-only-be-used-in-type-parameters I saw this one too... But now I'm not sure if it's Go's intention to do this...

Again, the following code:

package main

import (
    "fmt"

    "github.com/golang-jwt/jwt/v5"
)

func main() {
    var key1 = []uint8{1, 2, 3, 4}
    var key2 = []uint8{2, 3, 4, 5}

    // create a token
    t := jwt.New(jwt.SigningMethodHS256)
    token, _ := t.SignedString(key1)

    var set jwt.VerificationKeySet = jwt.VerificationKeySet{
        Keys: []jwt.VerificationKey{
            key1, key2,
        },
    }

    t, err := jwt.Parse(token, func(t *jwt.Token) (interface{}, error) {
        return set, nil
    })
    if err == nil {
        fmt.Printf("Yay %s!\n", t.Method.Alg())
    }
}

The above code, if you remove any from the VerificationKey, will fail to compile, even though []uint8 is defined. It feels like any allows the code to do some amazing things...

if remove any when use go vet ./... will result.

.\token.go:24:9: cannot use type VerificationKey outside a type constraint: interface contains type constraints

It looks like any lets the compiler bypass this restriction.

I'll try asking the go developers tomorrow.

oxisto commented 1 month ago

This seems to be a very annoying and old bug in Goland... https://youtrack.jetbrains.com/issue/GO-15406/False-positive-Interface-includes-constraint-elements-can-only-be-used-in-type-parameters I saw this one too... But now I'm not sure if it's Go's intention to do this...

Again, the following code:

package main

import (
  "fmt"

  "github.com/golang-jwt/jwt/v5"
)

func main() {
  var key1 = []uint8{1, 2, 3, 4}
  var key2 = []uint8{2, 3, 4, 5}

  // create a token
  t := jwt.New(jwt.SigningMethodHS256)
  token, _ := t.SignedString(key1)

  var set jwt.VerificationKeySet = jwt.VerificationKeySet{
      Keys: []jwt.VerificationKey{
          key1, key2,
      },
  }

  t, err := jwt.Parse(token, func(t *jwt.Token) (interface{}, error) {
      return set, nil
  })
  if err == nil {
      fmt.Printf("Yay %s!\n", t.Method.Alg())
  }
}

The above code, if you remove any from the VerificationKey, will fail to compile, even though []uint8 is defined. It feels like any allows the code to do some amazing things...

if remove any when use go vet ./... will result.

.\token.go:24:9: cannot use type VerificationKey outside a type constraint: interface contains type constraints

It looks like any lets the compiler bypass this restriction.

I'll try asking the go developers tomorrow.

Which version of Go are you using? If I run go vet ./... on the code above, I get no errors, I also get no go vet errors on the repository itself. I am using Go go1.22.5

godcong commented 1 month ago

This seems to be a very annoying and old bug in Goland... https://youtrack.jetbrains.com/issue/GO-15406/False-positive-Interface-includes-constraint-elements-can-only-be-used-in-type-parameters I saw this one too... But now I'm not sure if it's Go's intention to do this...

Again, the following code:

package main

import (
    "fmt"

    "github.com/golang-jwt/jwt/v5"
)

func main() {
    var key1 = []uint8{1, 2, 3, 4}
    var key2 = []uint8{2, 3, 4, 5}

    // create a token
    t := jwt.New(jwt.SigningMethodHS256)
    token, _ := t.SignedString(key1)

    var set jwt.VerificationKeySet = jwt.VerificationKeySet{
        Keys: []jwt.VerificationKey{
            key1, key2,
        },
    }

    t, err := jwt.Parse(token, func(t *jwt.Token) (interface{}, error) {
        return set, nil
    })
    if err == nil {
        fmt.Printf("Yay %s!\n", t.Method.Alg())
    }
}

The above code, if you remove any from the VerificationKey, will fail to compile, even though []uint8 is defined. It feels like any allows the code to do some amazing things... if remove any when use go vet ./... will result.

.\token.go:24:9: cannot use type VerificationKey outside a type constraint: interface contains type constraints

It looks like any lets the compiler bypass this restriction. I'll try asking the go developers tomorrow.

Which version of Go are you using? If I run go vet ./... on the code above, I get no errors, I also get no go vet errors on the repository itself. I am using Go go1.22.5

same as go1.22.5 I meant to move any out of VerificationKey


// before change:

// VerificationKey represents a public or secret key for verifying a token's signature.
type VerificationKey interface {
    crypto.PublicKey | []uint8
}

// change for check test:

// VerificationKey represents a public or secret key for verifying a token's signature.
type VerificationKey interface {
    []uint8
}

If you do that, it won't pass type checking with []int8. Although VerificationKey defines []uint8

oxisto commented 1 month ago

@mfridman can you help here? I am getting confused and I cannot see any issue with the code that we have…

mfridman commented 1 month ago

If you do that, it won't pass type checking with []int8. Although VerificationKey defines []uint8

Correct, because it would no longer would be a constraint-type.

Iirc GoLand doesn't use the standard gopls (Go language server) and uses their hand-rolled one. So the linter is probably getting tripped up.

But to your point there's two way to represent this:

version 1

type VerificationKeySet[T VerificationKey] struct {
    Keys []T
}

or

version 2

type VerificationKeySet struct {
    Keys []VerificationKey
}

The first version uses a generic type parameter T that must satisfy the VerificationKey constraint. The second version directly uses the VerificationKey interface type. I believe I picked the second version because it was slightly more ergonomic and didn't require explicit instantiation.

I think what you're proposing is the first version, but it wouldn't be using any though. Here's a draft PR that shows the first version.

https://github.com/golang-jwt/jwt/pull/403

I'm not proposing we change this, and I don't think there's anything to do since it would be a breaking change. But afaik both versions are valid, albeit version one is probably the more "idiomatic" way of doing it.

mfridman commented 1 week ago

Going to close this issue for now, I don't think there's anything to change here. But if you have further thoughts feel free to continue the discussion.

godcong commented 1 week ago

@mfridman sure, https://github.com/golang/go/issues/68710 Doesn't look like it's going to come back anytime soon.

mfridman commented 6 days ago

@godcong but I'm genuinely curious if one option is favored over the other (https://github.com/golang-jwt/jwt/issues/401#issuecomment-2265362588). Both produce compilable Go code so it's a bit puzzling.

GoLand reporting an error is a wholly different problem I think.