jwe: Support overriding the algorithm when supplying a JWK

[bojidar-bg]

Otherwise, the current code makes it impossible to supply a JWK-encoded ECDSA public key in the encryption config. (ParsePublicKey returns parseJWKPublicKey which returns the JWK itself; hence the switch-case fails to notice the ECDSA key inside the JWK)

The code in keywrapper_jwe.go finding the right value for alg could probably be structured better; something such as the following pseudocode might capture intent better:

if key is JWK:
  if JWK has algorithm set:
    use the algorithm from the JWK 
  else:
    continue with JWK.key
if key is RSA:
  use RSA_OAEP
else if key is ECDA:
  use ECDH_ES_A256KW
else:
  report error

Or well, for the purposes of the issue I was facing, just having the initial if key is JWK: continue with JWK.key, without allowing for customizing the algorithm, would also be sufficient.

[stefanberger]

Can you update your patch description with the problem the patch solves?

[bojidar-bg]

Sure; apologies for the delay.

[stefanberger]

Sure; apologies for the delay.

Can you format it to 75 characters-per-line, please.

[bojidar-bg]

Can you format it to 75 characters-per-line, please.

Could you perhaps link to a document outlining the commit message format guidelines/requirements? I feel I'd be much simpler than this back-and-forth.

[stefanberger]

This now opens up usage also for symmetric keys that the rest of the code cannot handle: https://github.com/go-jose/go-jose/blob/v3/shared.go#L90

I think we need another switch statement here that only accepts algorithms that we know we can handle which probably (subject to testing) are: ED25519, RSA_OAEP*, and ECDH_ES_*

[stefanberger]

ECDH_ES is not accepted in multi-recipient modes it seem, so this cannot be used, and it has to be ECDH_ES_A*
It's better to only use OAEP with RSA, so no RSA1_5.

[bojidar-bg]

Actually, parseJWKPublicKey will never return a non-asymmetric key, because it checks for IsPublic on the JWK.

ocicrypt/utils/utils.go

Lines 53 to 55 in a24b477

	if !jwk.IsPublic() {
	return nil, fmt.Errorf("%s: JWK is not a public key", prefix)
	}

Fair point about multi-recipient mode, I had not considered that..

[stefanberger]

I think this here would resolve the issue:

diff --git a/keywrap/jwe/keywrapper_jwe.go b/keywrap/jwe/keywrapper_jwe.go
index 62e03b5..f281b08 100644
--- a/keywrap/jwe/keywrapper_jwe.go
+++ b/keywrap/jwe/keywrapper_jwe.go
@@ -129,6 +129,18 @@ func addPubKeys(joseRecipients *[]jose.Recipient, pubKeys [][]byte) error {
                case *jose.JSONWebKey:
                        if key.Algorithm != "" {
                                alg = jose.KeyAlgorithm(key.Algorithm)
+                               switch alg {
+                               /* accepted algorithms */
+                               case jose.RSA_OAEP:
+                               case jose.RSA_OAEP_256:
+                               case jose.ECDH_ES_A128KW:
+                               case jose.ECDH_ES_A192KW:
+                               case jose.ECDH_ES_A256KW:
+                               /* all others are rejected */
+                               default:
+                                       return fmt.Errorf("%s is an unsupported JWE key algorithm", alg);
+                               }
                        }
                }

[stefanberger]

Actually, parseJWKPublicKey will never return a non-asymmetric key, because it checks for IsPublic on the JWK.

ocicrypt/utils/utils.go

Lines 53 to 55 in a24b477

	if !jwk.IsPublic() {
	return nil, fmt.Errorf("%s: JWK is not a public key", prefix)
	}

Fair point about multi-recipient mode, I had not considered that..

Yes, I have seen that but it will then go to try to parse it as a pkcs11 key and one gets odd error messages about a pkcs11 key that cannot be parsed:

ocicrypt/utils/utils.go

Lines 150 to 153 in a24b477

	key, err = parseJWKPublicKey(pubKey, prefix)
	if err != nil {
	key, err = parsePkcs11PublicKeyYaml(pubKey, prefix)
	}

[stefanberger]

I wasn't successful with ED25519 key - it seems to get rejected by the MultiEcnrypter, so I had to take this out from the supported algorithms.

[stefanberger]

Can you format it to 75 characters-per-line, please.

Could you perhaps link to a document outlining the commit message format guidelines/requirements? I feel I'd be much simpler than this back-and-forth.

Sorry, but we don't have such a document here.

[bojidar-bg]

Thanks for the review! Do you think it might be better to just change parseJWKPublicKey to return jwk.Key instead of &jwk? That way, one wouldn't need to worry about JWK-s spreading all throughout the code, and it would actually make JWK-s compatible with the other keyproviders (through utils.ParsePublicKey).

Sorry, but we don't have such a document here.

Fair. Bad UX, but fair 😅 Will fix the commit message format along with the code changes.

[stefanberger]

Thanks for the review! Do you think it might be better to just change parseJWKPublicKey to return jwk.Key instead of &jwk? That way, one wouldn't need to worry about JWK-s spreading all throughout the code, and it would actually make JWK-s compatible with the other keyproviders (through utils.ParsePublicKey).

The problem with this is that ParsePublicKey is part of the public interface and existing callers may expect certain types of objects to be returned by this function when and I don't think we can change this so easily. Besides that a JWK would then end up looking like a plain rsa.PublicKey or ecsdsa.PublicKey etc. I'd rather keep the JSONWebKey wrapper around it.

[bojidar-bg]

Apologies for the huge delay; changes have been implemented as requested. Let me know if there is anything else I should do (:

[stefanberger]

You need to reformat:

keywrap/jwe/keywrapper_jwe.go:141: File is not `gofmt`-ed with `-s` (gofmt)
				  return fmt.Errorf("%s is an unsupported JWE key algorithm", alg);

[bojidar-bg]

Ah, whoops, that shouldn't have slipped through. 😅 Good catch!