KeyBundle's do_keys ignores invalid keys

[angelakis]

KeyBundle's do_keys method tries to load keys from a JSON jwks file and if it finds a key that is considered invalid, it overwrites it in the existing file.

Not sure if this is considered a bug, but it seems counter-intuitive to me. Maybe we should instead raise an exception and let the user/app know that there's an error with their key?

[jschlyter]

Can you provide an example? I tried to replicate, but failed.

[angelakis]

You can replicate by loading a malformed RSA key from a json file given in the private_path of init_key_jar. Cryptojwt will issue a warning (e.g. While loading keys: private_exponent must be < modulus.) and then proceed in generating a new RSA key and overwriting the existing (malformed) RSA key in the file.

This happens only with read_only set to False. If it's True, a new key is generated but is not written to the file and a warning is written: Not allowed to write to disc! (sic). This also strikes me as non-intuitive, as the key will be lost when the app is restarted.

I would prefer that an exception is raised in cases of corrupted/malformed keys, instead of handling it and generating new keys, but I may well be wrong.

[jschlyter]

A challenge might be that sometimes you want to read all the keys you can and ignore the other, and sometimes you want to fail upon error. Perhaps a flag to KeyBundle (default False) to indicate whether to abort on invalid keys?

[rohe]

Yeah, I've never been able to decide on the correct behaviour (fail or best effort).
A flag deciding on what to do sounds good to me.

[jschlyter]

I'm thinking we could add a accept_invalid_keys(default True, current behavior) flag to KeyBundle. If False, raise exception when reading an invalid/unknown/unsupported key.

[jschlyter]

Code review needed by @angelakis

[rohe]

What I think is more of an ignore_invalid_keys.