New __hash__() method doesn't consider kid

[rabbittsoup]

Version 0.9 introduced a new JWK.hash() method that does not consider kid.

0edf66d

When using JWKSet.import_keyset(), server keys can be unexpectedly filtered out if some keys contain the same underlying key values, but only differ by other metadata, such as kid. If client tokens generated using the filtered kid key, authorization failure occurs.

[simo5]

Uhmmm I had no condiered the fact that you may have the same key values with different key IDs ...
Are there other parameters we should take into account in order to consider keys "different" ?

[rabbittsoup]

An edge case, for sure. But... I encountered that edge case :(

kid is the only parameter I am aware of, but I am not intimately familiar with all the different ways to lookup a key in a JMKSet. I think it would really tie back to whichever parameters could be used to lookup a key from the client token.

[simo5]

@rabbitsoup can you put together a three line reproducer in a comment ?
I am trying to trigger the issue by importing two keys the differ exclusively by 'kid', and I am not experiencing issues.
I think this code is buggy due to the eq() stuff as well and the fact that the objects can, technically, change, but reproducing what you see would be helpful in coming to a resolution that is as good as possible.

[rabbittsoup]

Yeah, I'll try to script up something.

[rabbittsoup]

This script passes with version 0.8, and fails with 0.9

from jwcrypto.tests import PrivateKeys
from jwcrypto.common import json_encode
from jwcrypto.jwk import JWKSet

# make a copy to modify
keys = {'keys': [{**key} for key in PrivateKeys['keys']]}

# duplicate a key, change kid
keys['keys'].append({**keys['keys'][-1]})
keys['keys'][-1]['kid'] = 'duplicate'

ksm = JWKSet.from_json(json_encode(keys))
l1 = len(keys['keys'])
l2 = len(list(ksm))
assert l1 == l2, f'Sourced {l1} keys != JWKSet {l2} keys'

This test could be improved by checking all the individual kid values as well.

[simo5]

Thanks I ended up writing a similar test in parallel in #211
Also discovered this issue is intertwined with what I discovered while fixing #209
So resolution may take a little more time