gnodet commented on PR #1303: URL: https://github.com/apache/cxf/pull/1303#issuecomment-4039385885
## Review findings and fixes I've performed an in-depth review of this PR and pushed a fix commit (771d279e86) addressing the following issues: ### Fixed issues 1. **Bug: cert/sha headers in `useReqSigCert` path would fail at runtime** — In `JweUtils`, when the alias is `useReqSigCert`, the code for `includeCert`/`includeCertSha1`/`includeCertSha256` called `KeyManagementUtils.loadAndEncodeX509CertificateOrChain(m, props)` which reads the alias from props, getting `"useReqSigCert"`, and tries to look it up in the keystore. Since no certificate with that alias exists, this would throw at runtime. Currently dead code in the tests (no include flags set in the properties files), but was a latent bug. Removed the broken cert header blocks from the `useReqSigCert` branch. 2. **`USE_REQ_SIG_CERT` Javadoc was misleading** — The Javadoc read "Whether to use request signing certificate to create encryption provider" which sounds like a boolean flag. Updated it to clarify this is a magic value for `rs.security.keystore.alias`, mirroring the WS-Security convention. 3. **Missing `throws Exception`** on `testJweJwsJwkRsaUseReqSigCert()` and `testJweJwsRsaUseReqSigCert()` test methods, unlike all other test methods in the class. 4. **Test boilerplate deduplication** — Extracted common setup into `createUseReqSigCertBookStore()` helper, reducing ~90 lines of duplicated code to ~30. ### Review notes (no action needed) 5. **Security model is sound** — The `useReqSigCert` feature requires `rs.security.accept.public.key=true` as an explicit opt-in on the server, and the test endpoints run over mutual TLS. This correctly mirrors the WS-Security `useReqSigCert` pattern. 6. **PublicKey stored on Exchange unconditionally** — In `JwsContainerRequestFilter.configureSecurityContext()`, the public key is stored on the Exchange whenever a `PublicKeyJwsSignatureVerifier` is used, even if the server doesn't use `useReqSigCert`. This is harmless since the Exchange is request-scoped, and it keeps the code simple. -- 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]
