LFT-1285 - Ignore use=enc JWKs in JWKS parsing #364
Conversation
| JsonWebKey key = JsonWebKey.FromJson( keyJson ); | ||
|
|
||
| if( !JsonWebKey.TryParseJsonWebKey( keyJson, out var key, out var error, out var exception, out var useEncKey ) ) { | ||
| if( useEncKey ) { |
There was a problem hiding this comment.
This filtering could apply more generally but for now I just want to do the minimal thing to get the integrations working. I think the API needs a redesign overall.
| @@ -49,91 +51,169 @@ public abstract class JsonWebKey { | |||
| /// <returns>A JWK DTO</returns> | |||
| public abstract object ToJwkDto(); | |||
|
|
|||
|
|
|||
| /// <summary> | |||
| /// Deserialize a JWK | |||
| /// </summary> | |||
| /// <param name="json">The json JWK</param> | |||
| /// <returns>A <see cref="JsonWebKey"/></returns> | |||
| public static JsonWebKey FromJson( string json ) { | |||
There was a problem hiding this comment.
Maintaining this API + behaviour for people that call this directly (> 0 in our code)
| [NullWhen( true )] | ||
| out Exception? exception, | ||
| out bool useEncKey | ||
| ) { |
There was a problem hiding this comment.
Mostly changed to TryX from throwing, but also added this separate useEncKey signal.
| @@ -0,0 +1,10 @@ | |||
| namespace System.Diagnostics.CodeAnalysis; | |||
There was a problem hiding this comment.
(Note filepath: .net4x.cs is treated specially via our build.)
| if( data.ContainsKey( "use" ) && data[ "use" ] != null && data[ "use" ].ToString() != "sig" ) { | ||
| string msg = String.Format( "invalid 'use' value in JSON web key: {0}", data[ "use" ] ); | ||
| throw new JsonWebKeyParseException( msg ); | ||
| result = null; | ||
| error = "invalid 'use' value in JSON web key: " + data[ "use" ]; | ||
| exception = null; | ||
| useEncKey = data[ "use" ].ToString() == "enc"; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
I would maybe make this ignored in general (and not just "enc")? Perhaps a ParseResult { Success, Error, Ignore } vs bool?
It would ultimately be nice to be able to say "the key {kid} is not intended for singnatures" rather than "the key {kid} was not found" though.
There was a problem hiding this comment.
Yeah, so I mentioned in the PR description basically I think our parser should just parse and deciding if a key was valid or not ought to be up to the user.
All of our users currently only want signing keys so I think it's OK practically to just filter them (vs. exploding) but I only exposed that on JWKS because the API for getting one key implies it will never return null (it throws currently) and it gets used e.g. parsing keys from the database, and I didn't want to add sig=enc logic for right now.
There was a problem hiding this comment.
In other words rather than returning ignore from here I'd prefer if we just successfully parsed all valid JWKs and then moved the policy decisions elsewhere
We never have use=enc keys and so our parsing throws when it encounters them.
We don't distinguish between unparsable keys vs. keys that we don't want to use for signing (e.g. use=enc, but also other constraints we impose like having a suitable kty, kid etc.)
In an ideal world I think we would push more of that logic out a bit and parse more keys, ignoring ones that aren't relevant to us anyway and emitting errors in a more sensible location.
This PR makes the minimal change possible to allow us to parse a JWKS response that includes a use=enc key that we will otherwise not need. This is needed for interoperability with LTI vendors that have a JWKS URL serving multiple purposes.