atiaomar1978-hub commented on PR #25510:
URL: https://github.com/apache/camel/pull/25510#issuecomment-5304842603
## Review
_AI-generated review on behalf of Omar Atie_
Thanks for the follow-up to CAMEL-22114 — the root-cause analysis is clear
and matches what we see on Java 25: JKS deserialization of ML-DSA keys yields
JDK-native `PrivateKey`/`PublicKey` instances that BC's `Signature` SPI rejects
at `initSign`/`initVerify`.
### What looks good
- **Fix location** — Calling `ensureBcKeyPair()` in `doStart()` immediately
after KeyStore / user-supplied `KeyPair` loading is the right hook. It covers
sign, verify, KEM, hybrid PQC, and key-export paths that all reuse the same
`keyPair` field.
- **Conversion approach** — Re-encoding through PKCS#8 / X.509 specs and BC
`KeyFactory` is the standard interoperability pattern and is a no-op when keys
are already BC instances.
- **Provider fallback** — `getBcKeyFactory()` trying `BouncyCastleProvider`
first, then `BouncyCastlePQCProvider`, mirrors how the rest of the component
resolves algorithms.
- **Failure handling** — Returning the original pair on conversion failure
(with a debug log) is reasonable; the crypto operation will still fail with a
meaningful exception rather than masking a misconfiguration silently at
INFO/WARN level.
- **Scope** — Single-file, focused change with no API surface impact. CI is
green on both Java 17 and Java 25.
### Minor nits (non-blocking)
1. **BC detection** — The
`getClass().getName().startsWith("org.bouncycastle.")` check is pragmatic and
fine here. If we ever hit a BC key loaded through a JDK proxy/wrapper, we'd
fall through to re-encoding (still safe). Not worth blocking on.
2. **Test plan checkboxes** — The PR description still has the Java 25
KeyStore test items unchecked, but CI (`build (25, false)`) is green — worth
ticking those off for completeness.
3. **Dedicated unit test** — No isolated test for `ensureBcKeyPair()`
itself; coverage comes from the existing KeyStore ITs. Acceptable given the
JDK-version-specific nature, though a small `@EnabledForJreRange(min =
JRE.JAVA_25)` test could make future regressions easier to spot locally.
### Verdict
Looks good to merge once a committer approves. This closes the gap left by
the earlier provider-less `Signature.getInstance()` fix for the KeyStore
deserialization path exposed by the recent PQC KeyStore test additions.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
