Conversation
There was a problem hiding this comment.
Pull request overview
Adds a new Docker-based test image to run OkHttp’s SSL/TLS test suite against wolfJSSE in FIPS mode, including scripts to build the image and patch OkHttp’s TLS keystore passwords for FIPS constraints.
Changes:
- Added an
okhttptest-image Dockerfile that clones/builds OkHttp, injects wolfJSSE FIPS JVM configuration via Gradle init script, and runs OkHttp TLS-related tests. - Added a build helper script to build the Docker image locally.
- Added a patch script to update OkHttp TLS keystore/PKCS12 passwords to a FIPS-compliant length.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| java/wolfssl-openjdk-fips-root/test-images/okhttp/Dockerfile | Multi-stage image to build OkHttp, configure Gradle test JVMs for wolfJSSE FIPS, and run OkHttp TLS tests. |
| java/wolfssl-openjdk-fips-root/test-images/okhttp/apply_okhttp_fips_fixes.sh | Patches cloned OkHttp sources to use a longer TLS keystore password for FIPS PBKDF2-HMAC constraints. |
| java/wolfssl-openjdk-fips-root/test-images/okhttp/build.sh | Convenience script to build the new OkHttp test image. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ========================================== | ||
| // FIPS-specific exclusions | ||
| // ========================================== | ||
| excludeTestsMatching 'okhttp3.SessionReuseTest.testSessionReuse_TLSv1_3' |
There was a problem hiding this comment.
testSessionReuse_TLSv1_3 might similar to our Netty one. Let's try to figure out why that fails with FIPS.
Also, please add a specific comment by each of the below tests as to what the specific FIPS incompatibility is that causes the test to fail. Based on their names along (ex: okhttp3.internal.ws.WebSocketHttpTest.httpsScheme), it doesn't seem like they should be non-FIPS specific.
There was a problem hiding this comment.
TLS 1.3 resumption here needs session tickets (--enable-session-ticket in the base image, plus the JDK client ticket property
There was a problem hiding this comment.
Do you recommend we add --enable-session-ticket to the base image? If so, does that cause any issues with the SunJSSE tests, or SunJCE base image tests? And, does that allow this test to pass then?
Or, maybe the bigger question is if we should auto-enable --enable-session-tickkets inside the --enable-jni build option.
There was a problem hiding this comment.
This was a weird test, --enable-session-ticket did cause it to pass, but it was failing due to wolfJSSE session handling bugs and currently passes without the flag. So we should leave the flag out of the base image.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
| // ClientAuthTest: Uses cross-signed cert chains for client | ||
| // auth. The cert chains are properly constructed with | ||
| // CA:true on intermediates, but wolfSSL may handle | ||
| // cross-signed chain validation differently than SunJSSE. |
There was a problem hiding this comment.
We need a more precise answer than "wolfSSL may handle cross-signed chain validation differently than SunJSSE". If a real user runs into this same problem, it will become a support ticket (which is what we are trying to reduce with this testing).
What does the above mean exactly? Do we need to adjust wolfJSSE behavior to work more closely to how SunJSSE does in cross-cert chain validation? I feel like cross-signed cert verification should work, since --enable-jni defines WOLFSSL_ALT_CERT_CHAINS.
There was a problem hiding this comment.
Fixed. The ClientAuthTest exclusion was removed as those tests now pass. I added findIssuerBySignature() to WolfSSLTrustX509 which resolves cross-signed chain validation by verifying signatures when multiple CAs share the same subject DN.
| // | ||
| // HandshakeCertificatesTest.clientAndServer: May fail due | ||
| // to missing SAN on leaf certificates or wolfSSL trust | ||
| // anchor handling differences. |
There was a problem hiding this comment.
Also here, we need a more precise answer than "wolfSSL trust anchor handling differences" so we can determine if this will be a real-world error people will encounter.
There was a problem hiding this comment.
Removed, the tests now pass with wolfssljni changes.
| // TrustManager whose checkServerTrusted() throws. When | ||
| // wolfJSSE is the default provider, SSLContext.init() | ||
| // with a foreign TrustManager behaves differently than | ||
| // SunJSSE. Legitimate provider incompatibility. |
There was a problem hiding this comment.
Same here, what does this mean exactly? What is the difference in wolfJSSE's behavior?
// wolfJSSE is the default provider, SSLContext.init()
// with a foreign TrustManager behaves differently than
// SunJSSE. Legitimate provider incompatibility.
There was a problem hiding this comment.
Fixed. Test passes now, exclusion removed. The wolfJSSE fixes for WolfSSLInternalVerifyCb resolved this.
| // FIPS certificate and are disabled at the native layer. | ||
| // ========================================== | ||
|
|
||
| // TLSv1 and TLSv1.1 - not FIPS approved |
There was a problem hiding this comment.
TLS protocols are above the wolfCrypt FIPS level. A better way to say this is that they are "compiled out by default in the base image".
| excludeTestsMatching '*Des*' | ||
| excludeTestsMatching '*DES*' | ||
|
|
||
| // Anonymous ciphers - not FIPS approved |
There was a problem hiding this comment.
Same as above - cipher suites themselves are not inside the crypto-level FIPS boundary. We should say something like "disabled in the FIPS base image".
| @@ -0,0 +1,203 @@ | |||
| PR #8 Review Responses - Chris Conlon's Comments | |||
There was a problem hiding this comment.
This file should not be part of this pull request.
|
@JeremiahM37 One more comment on this - please remove Claude as a co-author to that one commit, and squash commits into meaningful messaged ones rather than "Add PR #8 review response notes for Chris Conlon's comments" etc. We like to keep commit history meaningful, rather than "address review comments" type messages. Thanks! |
|
Those commit messages were never on this branch. That message was another branch I used for testing, and I accidentally let claude push to it where it referenced this pr which left the commit history on this pr. That was never intended to be referenced on this pr at all. Im not sure how I can remove it, as that branch has been deleted on my fork. The only that can remove the pr timeline is closing this branch and opening a new one. |
Adds comprehensive test infrastructure for validating OkHttp's SSL/TLS functionality against wolfJSSE in FIPS mode.