Fix key compatibility problem in XDHKeyAgreement #723

dev-koan · 2025-07-16T17:48:29Z

The KeyAgreement test was failing because the key passed in the parameter was an instance of Sun. Resolved by using XDHKeyFactory for private and public keys passed as parameters in engineInit() and engineDoPhase(). Added a check to throw InvalidKeyException when the key is not an instance of XECPrivateKey or XECPublicKey as required.

Consistency is also ensured between the two function with respect to the order that the checks are taking place.

johnpeck-us-ibm · 2025-07-16T19:16:16Z

Is there an issue to look at for this? This seems like a very normal issue for ANY key that comes from a different provider than the one that key is being used with. I do no think this issue is only for these keys.

dev-koan · 2025-07-16T19:22:13Z

Is there an issue to look at for this? This seems like a very normal issue for ANY key that comes from a different provider than the one that key is being used with. I do no think this issue is only for these keys.

Yes, there is an issue that the PR addresses (https://github.ibm.com/runtimes/jit-crypto/issues/839).
It is a normal problem that exists when the key comes from a different provider.

johnpeck-us-ibm · 2025-07-17T13:13:35Z

Okay so the issue is that the key that was passed into the OpenJCEPlus provider was created by the SUN provider.

I doubt the test was meant to cross test providers. Meaning that a key was created by one provider and than passed to another provider to verify compatibility and function. This sounds like a test case issue or a test set up issue. Please check the test case and determine why the key was created by the SUN provider and not the OpenJCEPlus provider. Since, this test was for XDHKeyAgreement I would have expected ALL keys to be created by the same provider or at least all operations including key creation to be done separately on each side.

dev-koan · 2025-07-17T18:18:16Z

Okay so the issue is that the key that was passed into the OpenJCEPlus provider was created by the SUN provider.

I doubt the test was meant to cross test providers. Meaning that a key was created by one provider and than passed to another provider to verify compatibility and function. This sounds like a test case issue or a test set up issue. Please check the test case and determine why the key was created by the SUN provider and not the OpenJCEPlus provider. Since, this test was for XDHKeyAgreement I would have expected ALL keys to be created by the same provider or at least all operations including key creation to be done separately on each side.

I looked into the test case and you are correct, the test was not meant to cross test providers. When it gets key pair (link), the function checkKPGSupportAndGetKeyPair doesn't have a parameter for the provider, so it must be using Sun as default. But when an instance of KeyAgreement is created, the provider is specified causing a mismatch.
This can either be resolved by changing the test case (but it is a JCK test) or the provider code.

johnpeck-us-ibm · 2025-07-17T19:01:06Z

Are you sure this is a JCK test? If it is try adding OpenJCEPlus first in the provider list and rerun the test.

dev-koan · 2025-07-25T15:11:34Z

I tried adding OpenJCEPlus at the top of the provider list and running it but it failed with or without my code changes. This PR fixes the test when OpenJCEPlus is added at the last of the provider list.
To fix the test when it is added first in the list another update will be required in Sun or SunEC.

johnpeck-us-ibm · 2025-07-25T17:49:29Z

Interesting but I guess not surprising since the issues you show indicate that the failure is in the Native code version of SunEC. The actual code for this version of the SunEC provider does not seem to have been open sourced. So we can not determine what the SunEC code was looking for in order for this to work.

Now, if you put OpenJCEPlus last in the list and add code to do the translate that will only fix that scenario.

The real thing we want to do is to put OpenJCEPlus first in the list and have the JCK work. However, doing that will cause the at least the Native SunEC provider to have issues with the keys from OpenJCEPlus.

Therefore, I think the best thing to do for this issue is to go back to Oracle to have them remove the test for now and to correct it in the future.

@jasonkatonica Lets discuss this when you return from vacation.

johnpeck-us-ibm

Code is fine.
However, we need an interop test case to be created to verify that this code is working as expected.

Also, I want to voice comcern, Doing this can set an expectation that all operations in OpenJCEPlus have a similar feature. So, if we do this here. We should probably do this for ALL operations: Key Agreement, signnatures, Ciphers, etc.

jasonkatonica · 2025-08-14T11:46:14Z

src/main/java/com/ibm/crypto/plus/provider/XDHKeyAgreement.java

+            } catch (ClassCastException cce) {
+                throw new InvalidKeyException("Key is not an instance of XDHPublicKeyImpl", cce);
+            } catch (Exception exception) {
+                throw new InvalidKeyException("KeyFactory is not working as expected", exception);


Perhaps we could come up with an exception message here that is more generalized such as Unable to translate key. Only reason I am thinking that is that the message is a bit deceiving since at this point we dont know if the key was an unknown / incorrect format or class OR the KeyFactory simply could not translate the key. This could be working as expected in those cases.

jasonkatonica · 2025-08-14T11:46:24Z

src/main/java/com/ibm/crypto/plus/provider/XDHKeyAgreement.java

+            } catch (ClassCastException cce) {
+                throw new InvalidKeyException("Key is not an instance of XDHPrivateKeyImpl", cce);
+            } catch (Exception exception) {
+                throw new InvalidKeyException("KeyFactory is not working as expected", exception);


Similar comment as above.

jasonkatonica · 2025-08-14T11:48:28Z

The tests also seem to be failing in the associated CI pipeline :

[2025-08-08T20:15:03.390Z] [ERROR] Errors: 

[2025-08-08T20:15:03.390Z] [ERROR]   TestXDHKeyAgreementInterop>BaseTestXDHKeyAgreementInterop.testKey:39 » InvalidKey Unsupported key type

[2025-08-08T20:15:03.390Z] [INFO] 

[2025-08-08T20:15:03.390Z] [ERROR] Tests run: 2677, Failures: 0, Errors: 1, Skipped: 0

jasonkatonica · 2025-08-14T11:49:43Z

It is also strange that all platforms are failing EXCEPT for AIX which is not expected.

KostasTsiounis · 2025-08-14T14:21:56Z

It is also strange that all platforms are failing EXCEPT for AIX which is not expected.

This might be failing due to the interop problem on the Sun side. The NativeXDHKeyPairGenerator, which lacks the ability to handle non-Sun keys at the moment, is used by all platforms except AIX.

KostasTsiounis · 2025-08-14T14:38:36Z

src/main/java/com/ibm/crypto/plus/provider/XDHKeyAgreement.java

+            try {
+                key = (XDHPublicKeyImpl) XDHKeyFactory.toXECKey(this.provider, this.alg, key);
+            } catch (ClassCastException cce) {
+                throw new InvalidKeyException("Key is not an instance of XDHPublicKeyImpl", cce);


It is not clear in this exception message which key is not an instance of XDHPublicKeyImpl. It sounds as if the user provided key has an issue, when it's the one produced by the factory.

There is no KeyFactory involved here. In this code it has not idea what has or has not been done to the key. It is just making sure that the passed in key is what it expects.

KostasTsiounis · 2025-08-14T14:38:52Z

src/main/java/com/ibm/crypto/plus/provider/XDHKeyAgreement.java

+            try {
+                key = (XDHPrivateKeyImpl) XDHKeyFactory.toXECKey(this.provider, this.alg, key);
+            } catch (ClassCastException cce) {
+                throw new InvalidKeyException("Key is not an instance of XDHPrivateKeyImpl", cce);


Same comment as above.

KostasTsiounis · 2025-08-14T14:41:10Z

src/test/java/ibm/jceplus/junit/base/BaseTestXDHKeyAgreementInterop.java

+        KeyPair kp1 = kpg1.generateKeyPair();
+        KeyPair kp2 = kpg2.generateKeyPair();
+
+        ka1.init(kp1.getPrivate());
+        ka1.doPhase(kp2.getPublic(), true);
+
+        ka2.init(kp2.getPrivate());
+        ka2.doPhase(kp1.getPublic(), true);
+
+        byte[] ss1 = ka1.generateSecret();
+        byte[] ss2 = ka2.generateSecret();


I would add some more combinations. In the ones you have now the KeyAgreement instance and private key are always coming from the same provider. I think we should try the inverse too where the private key comes from the other provider.

Update XDHKeyAgreement to handle different providers using XDHKeyFactory. Signed-off-by: Dev Agarwal <[email protected]>

dev-koan requested review from JinhangZhang, KostasTsiounis, jasonkatonica, johnpeck-us-ibm and taoliult July 16, 2025 19:18

johnpeck-us-ibm requested changes Aug 4, 2025

View reviewed changes

dev-koan force-pushed the fix_KeyAgreement branch from 968f950 to 683feb6 Compare August 8, 2025 19:15

jasonkatonica reviewed Aug 14, 2025

View reviewed changes

KostasTsiounis requested changes Aug 14, 2025

View reviewed changes

Fix key compatibility problem in XDHKeyAgreement

57cd8d1

Update XDHKeyAgreement to handle different providers using XDHKeyFactory. Signed-off-by: Dev Agarwal <[email protected]>

dev-koan force-pushed the fix_KeyAgreement branch from 683feb6 to 57cd8d1 Compare August 20, 2025 15:18

Fix key compatibility problem in XDHKeyAgreement #723

Are you sure you want to change the base?

Fix key compatibility problem in XDHKeyAgreement #723

Conversation

dev-koan commented Jul 16, 2025

Uh oh!

johnpeck-us-ibm commented Jul 16, 2025

Uh oh!

dev-koan commented Jul 16, 2025

Uh oh!

johnpeck-us-ibm commented Jul 17, 2025

Uh oh!

dev-koan commented Jul 17, 2025

Uh oh!

johnpeck-us-ibm commented Jul 17, 2025

Uh oh!

dev-koan commented Jul 25, 2025

Uh oh!

johnpeck-us-ibm commented Jul 25, 2025

Uh oh!

johnpeck-us-ibm left a comment

Choose a reason for hiding this comment

Uh oh!

jasonkatonica Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

jasonkatonica Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

jasonkatonica commented Aug 14, 2025

Uh oh!

jasonkatonica commented Aug 14, 2025

Uh oh!

KostasTsiounis commented Aug 14, 2025

Uh oh!

KostasTsiounis Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

johnpeck-us-ibm Aug 15, 2025

Choose a reason for hiding this comment

Uh oh!

KostasTsiounis Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

KostasTsiounis Aug 14, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants