Unmarshal jwk keys with unsupported key type or algorithm into empty … #26
Conversation
…JSONWebKeys. Ignore invalid jwk keys when unmarshalling jwks key sets.
|
Fixes #25 |
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Thanks for working on this!
By my read of the RFC, the SHOULD only applies to keys within a JWK Set. If there's a solo key and the implementation doesn't understand it, there's no SHOULD to accept it.
I think the right approach here is to return a sentinel error for "unknown key type" from JSONWebKey.UnmarshalJSON(). Then in JSONWebKeySet.UnmarshalJSON() we can look for that sentinel error and not add the key to the list when it happens (but also not error out the whole parse).
Yes, that approach sounds like doing the same job. Maybe that is a better and clearer solution too. |
…own jwk key type or parameters. Check for this error when unmarshalling jwks key sets.
|
Is there any blocker for this PR? I've a similar case to #59 (comment) where the OIDC conformance suite is failing due to the inability to handle unknown key types Is there any way that I can help to move forward this PR? |
Is anyone actively working on this fix? Sounds like a good solution has been proposed but it is unclear the status. Thanks! |
I hope this is not blocked because of me (the author). If there's still something I need to do, then please advice. Thanks! |
jsha
left a comment
jsha
left a comment
There was a problem hiding this comment.
Looks great. Thanks for the fix, and apologies for letting it linger so long. I have one proposed improvement below.
| return err | ||
| } | ||
| } else { | ||
| s.Keys = append(s.Keys, k) |
There was a problem hiding this comment.
To unmarshal a JSON array into a slice, Unmarshal resets the slice length to zero and then appends each element to the slice.
We should do the same here, resetting s.Keys to zero length just in case we wind up unmarshaling into a pre-existing JSONWebKeySet.
|
I'm going to merge this and send a followup PR with the proposed improvement. |
#26) Unmarshal JWKs with unsupported key type or algorithm into empty JSONWebKeys. Ignore invalid JWKs when unmarshalling JWK Sets.
go-jose#26) Unmarshal JWKs with unsupported key type or algorithm into empty JSONWebKeys. Ignore invalid JWKs when unmarshalling JWK Sets.
Ultimately we want to ignore unsupported key types in JSONWebKeySets (#26 / #130), but we are blocked on doing a breaking change release. In the meantime, we can have JSONWebKey (the singular version) return ErrUnsupportedKeyType. This allows users to write their own JWKS umarshalling method to ignore unsupported key types, eg. https://github.com/zitadel/oidc/blob/045b59e5a55be97ba180fdaae96fb66302a03353/pkg/client/rp/jwks.go#L221-L235.
4 participants
Unmarshal jwk keys with unsupported key type or algorithm into empty JSONWebKeys. Ignore invalid jwk keys when unmarshalling jwks key sets.