keyfunc.JWKs: Keys field undefined in v0.8.0

[tillkuhn]

After upgrading from v0.7.0 to v0.8.0 I get the following compile time error:

jwks.Keys undefined (type *keyfunc.JWKs has no field or method Keys)

It seems the previous JWKs struct exported a field Keys of type map[string]*JSONKey, which is now gone. What is the alternative to access the keys for a Set?

Thanks,
Till

[MicahParks]

In v0.8.0 I made the breaking change to unexport the JWKs.Keys field.

The reason for this is because it's unsafe to access this field when there's a background refresh goroutine. Accessing the previously exported JWKs.Keys field could produce a Go panic.

fatal error: concurrent map read and map write

When I noticed this was possible, I unexported the variable to prevent a package user from doing this. I failed to find a use case for a package user to read or write to the JWKs.Keys map.

I'm assuming this issue was raised because there is a use case to read or write to the JWKs.Keys map. If it was a write, I would encourage you too look at the keyfunc.NewGiven function or the new field in keyfunc.Options, the Options.GivenKeys field.

If the use case involves a write and the new implementation involving "given keys" does not satisfy the use case, would you mind sharing it? It would help me better implement a solution to access the JWKs.Keys field.

If the use case is a read-only call to JWKs.Keys, I can add a method to lock the map and return a copy of the map. (This is what I'll do if the issue becomes inactive.)

Some people are familiar with the map read/write race condition. For those who are not, take a look at this example. It should cause a Go panic.

package main

import (
	"sync"
)

func main() {
	m := make(map[string]int)
	wait := sync.WaitGroup{}
	iterations := 1000
	const key = ""
	wait.Add(1)
	go func() {
		for i := 0; i < iterations; i++ {
			m[key] = i
		}
		wait.Done()
	}()
	for i := 0; i < iterations; i++ {
		println(m[key])
	}
	wait.Wait()
}

[tillkuhn]

Thanks for the thorough explanation. You're right, at some point the above code fails with a fatal error: concurrent map read and map write. I had to dig a bit to find out how the Keys were actually used in the application. We recently activated Renovate with automerge for minor and patch versions, and that's where I noticed the sudden compile error with v0.8.0!

It turns out the Keys was only used to validate that the JWKs contains at least one key, and check on the algorithm. So it's read-only, and also not super important for the use case, since Keyfunc does the actual work under the hood. I keep the issue open if you still want to perform some actions, but feel free to close it. Maybe in future you can add a comment on the Tag, or add a CHANGELOG.md (or prominent README section) for breaking changes with new versions. This could avoid issues and, give some background on the motivation for the change. Also noticed KeyFunc was renamed to Keyfunc. But hey, it's open source and I'm happy you share your work. Have a nice weekend // Till

[MicahParks]

As a suggestion, I'd turn off "Renovate with automerge" for packages in pre-release, like this one. Breaking changes can occur at any minor version bump until there is a full release. Here's a reference for Go packages in the first-unstable release like this one:
https://golang.org/doc/modules/release-workflow#first-unstable
Also see the v0: the initial, unstable version section of this guide:
https://go.dev/blog/publishing-go-modules

It's certainly useful to check that keys exist in the JWKs. I'll add that read-only method called KIDs that returns a slice of the kids found in the JWKs. Let me know if that does not cover your use case. After that, I'll close this issue. Should be done after work today.

As a note, I'm currently in the process of drafting a PR for the upstream github.com/golang-jwt/jwt/v4: golang-jwt/jwt#105. It's my goal to do a v1.0.0 release for this package if that PR is merged, then not introduce any more breaking changes. If this upstream PR goes through and you're using github.com/golang-jwt/jwt/v4, I'd recommend removing github.com/MicahParks/keyfunc as a dependency and using the github.com/golang-jwt/jwt/v4/keyfunc package. The github.com/MicahParks/keyfunc will still be maintained and kept up to date with upstream, for compatibility purposes.

[tillkuhn]

Sounds great! We don't use automerge for mission critical apps, and it's linked with the CI/CD Pipelines. So if build or tests fail, nothing gets merged or even deployed to staging, and the developer gets notified. That's a good compromise for most of my standard use cases. And yes, I'm using golang-jwt/jwt/v4 and would appreciate if your utils become part of it. As for my use case, a KIDs slice would be sufficient, thanks you very much. If you want me to test an unreleased version, just let me know!

[MicahParks]

@tillkuhn Please see the recent addition in v0.8.2

[tillkuhn]

It works, thanks for the quick fix!