[v2][v3] LookupKeyID / KeyID behavior changed

[datosh]

Contribution Guidelines

Before filing an issue, please read the contents of CONTRIBUTING.md, and follow its instructions.

Describe the bug

I am migrating openpubkey from jwx/v2 to jwx/v3, and ran into the following situtation:

The behavior of jwk.set.LookupKeyID / jwk.rsaPublicKey.KeyID has changed between version 2 and 3, but the Changes-v3.md does not point out the change.

The relevant code is

• v3 KeyID, v3 LookupKeyID
• v2 KeyID, v2 LookupKeyID

The behavior changes when we add a key with an empty KeyID, because now KeyID will return "", false and the new implementation of LookupKeyID requires the bool to be true, whereas in v2 an empty KeyID return value had no notion of failure.

I would like to understand whether this is

• A changed and intended behavior in v3
• A bug in v3 and we can restore the behavior of v2 in LookupKeyID, maybe even KeyID? Not sure if an empty KeyID should be "not ok".

Happy to contribute the required changes, once I understand what the intended behavior is.

Please attach the output of go version

› go version
go version go1.25.3 linux/amd64

To Reproduce / Expected behavior
Please attach a standalone Go test code that shows the problem, and what you expected to happen.

package main

import (
	"crypto/rand"
	"crypto/rsa"
	"fmt"

	"github.com/lestrrat-go/jwx/v2/jwk"
//      compare to v3
//	"github.com/lestrrat-go/jwx/v3/jwk"

)

func main() {
	set := jwk.NewSet()

	signer, err := rsa.GenerateKey(rand.Reader, 2048)
	if err != nil {
		panic(err)
	}

	key, err := jwk.PublicKeyOf(signer)
	if err != nil {
		panic(err)
	}

	set.AddKey(key)

	gotKey, ok := set.LookupKeyID("")
	if !ok {
		panic("key not found")
	}

	fmt.Println(gotKey)
}

The output changes:

datosh at bingo in ~/code/jwx-lookupkeyid
› go run ./v2/main.go
&{<nil> [1 0 1] <nil> <nil> <nil> [242 159 129 225 147 80 142 229 177 26 247 52 123 243 183 155 244 70 132 86 42 240 113 102 224 125 202 240 35 171 51 230 81 212 61 119 222 92 7 49 82 162 79 81 69 43 26 156 58 46 22 73 148 141 241 244 247 223 20 23 180 54 1 219 106 237 224 103 30 226 101 233 172 251 166 88 179 106 0 75 226 252 61 102 53 66 139 40 13 233 238 226 249 146 196 54 115 20 32 207 81 6 36 226 233 98 130 235 6 5 139 40 245 137 18 182 185 144 159 138 170 152 147 219 137 143 152 75 152 228 211 249 36 73 19 61 3 204 234 224 139 195 12 106 144 89 240 159 118 93 158 224 137 45 146 112 221 80 185 43 29 254 180 169 145 228 128 235 12 90 74 179 11 40 204 30 189 65 148 235 254 77 186 243 59 124 237 212 91 221 141 21 72 114 145 117 194 89 156 88 47 141 122 137 218 70 27 138 15 204 196 219 169 130 75 115 196 25 73 52 45 63 146 154 57 234 182 252 17 120 110 74 110 184 52 158 44 251 131 48 155 237 161 90 130 6 0 76 39 220 90 232 193 98 172 131] <nil> <nil> <nil> <nil> map[] 0xc00001c3f0 <nil>}
datosh at bingo in ~/code/jwx-lookupkeyid
› go run ./v3/main.go
panic: key not found

goroutine 1 [running]:
main.main()
	/home/datosh/code/jwx-lookupkeyid/v3/main.go:28 +0xfe
exit status 2

If you are asking for an API change or some such which inhibits you from providing a working code, please do your best to come up with a near-valid code.

[lestrrat]

I would like to understand whether this is

A changed and intended behavior in v3
A bug in v3 and we can restore the behavior of v2 in LookupKeyID, maybe even KeyID? Not sure if an empty KeyID should be "not ok".

(Hey, just a quick reply, please note that I haven't really spent time debugging this)

That's tricky, but I would say it's kind of the intended behavior. So there was no intention to change how LookupKeyID() works, but I did want to differentiate between utility methods like KeyID() returning "" silently without any notion that there could be something happening.

[datosh]

I have created a fork to see what this could look like: 071c5ae

[lestrrat]

So you want to find a key with an empty kid? Is this the ultimate goal?
i.e. you want to detect a key like { kid: "" .... }? Should a key without a kid also a match?

[datosh]

Should a key without a kid also a match?

I think this is exactly the changed behavior between v2 and v3 that was confusing to me. In v2 a nonexisting keyID matched the empty keyID, which is no longer the case in v3.

I am happy to close this issue.

Do you think it makes sense to record a line in the changes md file, or should this be obvious from the changed function signature? I might have just been slow picking up on that detail 😉

[lestrrat]

nah, if it wasn't in the Changes file (which I haven't checked, but I'll take your word for it), it's my oversight.

What I'm trying to figure out is if this is something that needs to be fixed or something that just needs a doc for a workaround. In general I'm not very keen on changing behavior back unless it's a real bug, but at this point I'm just trying to figure out what it is you're trying to do and if it's a legitimate usecase that I forgot to consider

[datosh]

Got it! As I mentioned in my initial post I am migrating the openpubkey code base. We relied on the fact that when you look up a key by ID "" a key is returned, even though there was no key ID set for this key.

I think the new behavior of jwx/v3 is more correct as you can distinguish between no kid and empty kid. I didn't pick up on this, so a line in the docs might help with that.

[lestrrat]

@datosh got it. So would it be okay if we add documentation in Changes+LookupKeyID() and a completed version of a workaround like below?

func LookupEmptyKeyID(set jwk.Set) (jwk.Key, error) {
   // iterate through set, find missing kid or kid == ""
   for .... {
      if kid, _ := key.KeyID(); kid == "" {
         return key, nil
      }
   }
   return nil, errors.New("not found")
}

[datosh]

This works for my specific use case, but you would never be able to retrieve multiple keys without a kid.

I stumbled across this limitation in the LookupKeyID v3 as well, although the interface states that Iterate() should be used, this functionality is no longer available in v3. Does this doc string need fixing or is there an iterate functionality I overlooked?

In the end I used Len and Key(int) to iterate through all elements in the set. I think this should be enough for anyone who needs to find keys without a kid or empty kid to do so.

[lestrrat]

I stumbled across this limitation in the LookupKeyID v3 as well, although the interface states that Iterate() should be used, this functionality is no longer available in v3. Does this doc string need fixing or is there an iterate functionality I overlooked?

Oh I see. That's the sort of thing I wanted to know when I said I wanted to know your usecase :)

Without seeing the code, I think what you described is probably the correct way to do it in v3. Perhaps we can come up with something for the future, but I seriously never thought that people would need to filter out multiple keys without a key ID, so I haven't really invested the time to come up with a solution.

[datosh]

Oh I see. That's the sort of thing I wanted to know when I said I wanted to know your usecase :)

Ah my bad - I was on the road and answering from the GH app, which is not as nice to provide code examples.

From the PR linked above, these are the two most relevant sections:

• https://github.com/openpubkey/openpubkey/pull/349/files#diff-951da8610ceeb5bfcea78b1586861f19c0365bdc6328636efeea6268f5993dc3R200-R208
• https://github.com/openpubkey/openpubkey/pull/349/files#diff-951da8610ceeb5bfcea78b1586861f19c0365bdc6328636efeea6268f5993dc3R220-R224

Just to summarize - I would propose to:

1. Disregard the changes I suggested in my fork: 071c5ae - I see these are contrary to the new design
2. Len() and Key(int) should be enough when you need to do your own filtering
3. FiX up the Set.LookupKeyID doc string to remove the reference to Iterate

[lestrrat]

I merged a doc fix. Please feel free to reopen if you think this is not enough / not correct.