WithInferAlgorithmFromKey should cache inferences alas

[wingsofovnia]

Abstract

jws.WithInferAlgorithmFromKey(true) is handful option for JWKs that do not present alg field. When this option is set to true, a list of algorithm(s) that is compatible with the key type will be enumerated, and ALL of them will be tried against the key/message pair. If any of them succeeds, the verification will be considered successful.

This however is much slower than having an alg field.

Describe the proposed change

To speed up the look up, I suggest caching the inferred alg. According to the RFC 7517 #4.5, keys SHOULD use distinct kid values. Keys might use the same kid value is if they have different kty, so cache

algCache := make(map[string /* kid || kty  */]jwa.SignatureAlgorithm)

should be sufficient (1).

Second option (2) would be to cache the key that actually worked after the verification via WithKeyUsed.

In addition, I would suggest setting WithInferAlgorithmFromKey to true by default, because as per RFC 7517 #4.4, alg is OPTIONAL and some major service providers (like Azure) do not present it, so missing alg should be expected.

[lestrrat]

I don't have an opinion on if caches are good/bad for this purpose yet, but I immediately had a few questions pop up to my head:

• What's the lifetime/scope of your proposed cache? E.g. suppose there are multiple distinct consumers of JWS messages in a same Go process, should they see different caches? Or are they universal?
• Are the cache entries controlled by some TTL? If so, how does one specify it?
• Can they be manually purged?
• Keys should use distinct kids, but what happens when they don't -- how do you expect the users to recover from those?

None of these are necessarily showstoppers, but I want to know if you have thought this through, and if so, I want to know your proposed changes.

In addition, I would suggest setting WithInferAlgorithmFromKey to true by default, because as per RFC 7517 #4.4, alg is OPTIONAL and some major service providers (like Azure) do not present it, so missing alg should be expected.

I don't agree. If you want to discuss further, please open a separate issue. I do not wish to discuss multiple items in the same entry.

[github-actions]

This issue is stale because it has been open 14 days with no activity. Remove stale label or comment or this will be closed in 7 days.

[github-actions]

This issue was closed because it has been stalled for 7 days with no activity. This does not mean your issue is rejected, but rather it is done to hide it from the view of the maintains for the time being. Feel free to reopen if you have new comments