Skip to content
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

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

Closed
godcong opened this issue Jul 31, 2024 · 12 comments

Comments

@godcong
Copy link

godcong commented Jul 31, 2024

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
Copy link
Collaborator

oxisto commented Aug 1, 2024

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'. Semantically crypto.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
Copy link
Author

godcong commented Aug 1, 2024

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
Copy link
Collaborator

oxisto commented Aug 1, 2024

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
Copy link
Collaborator

oxisto commented Aug 1, 2024

@godcong
Copy link
Author

godcong commented Aug 1, 2024

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
Copy link
Collaborator

oxisto commented Aug 2, 2024

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
Copy link
Author

godcong commented Aug 2, 2024

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
Copy link
Collaborator

oxisto commented Aug 2, 2024

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

@mfridman
Copy link
Member

mfridman commented Aug 2, 2024

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.

#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
Copy link
Member

mfridman commented Sep 6, 2024

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.

@mfridman mfridman closed this as completed Sep 6, 2024
@godcong
Copy link
Author

godcong commented Sep 6, 2024

@mfridman sure, golang/go#68710 Doesn't look like it's going to come back anytime soon.

@mfridman
Copy link
Member

mfridman commented Sep 6, 2024

@godcong but I'm genuinely curious if one option is favored over the other (#401 (comment)). Both produce compilable Go code so it's a bit puzzling.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants