Fix "Empty input" when loading SSL_CERT_FILE/SSL_CERT_DIR cert bundles. - #1706
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 4 included reviews per hour; 3 remain after this review. 📝 WalkthroughWalkthroughThis PR updates certificate bundle loading to parse all certificates in one pass. It adds parameterized tests for PEM files with four trailing newline and whitespace variants. ChangesCertificate Bundle Loading
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The stated change addresses whole-stream certificate parsing and adds trailing-whitespace coverage; no merge-blocking failure is established. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Git: CodeRabbit could not clone the repository, so clone-backed analysis was skipped and this review may be incomplete. Verify repository clone access, such as SSH credentials, before requesting another full review. If clone access is intentionally unavailable, use Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. A rabbit loads each certificate bright Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
api/src/test/java/io/minio/HttpExternalCertificatesTest.java (2)
55-63: ⚡ Quick winAdd a
SSL_CERT_DIR-path test for parity with the production fix scope.Current coverage exercises only the file-path flow. Add a case that writes the PEM into a temp directory and calls
Http.enableExternalCertificates(baseClient, null, dirPath)to validate the directory code path too.Also applies to: 73-77
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/io/minio/HttpExternalCertificatesTest.java` around lines 55 - 63, Update HttpExternalCertificatesTest.bundles() to include a new parameterized case that exercises the SSL_CERT_DIR directory path: create a temporary directory, write the PEM content into a file inside it, and add a test case name like "ssl_cert_dir" that will be used by the test harness; then in the corresponding test execution branch call Http.enableExternalCertificates(baseClient, null, dirPath) (instead of the file path) so the directory code path in Http.enableExternalCertificates is validated; ensure you add the same directory-based case for the other parameterized list at lines 73-77 as well.
73-77: ⚡ Quick winStrengthen the assertion to prove certificates were actually applied.
Assert.assertNotNull(client)can pass even when no external certs are loaded (the method may return the original client). Assert that a new client instance is returned.Suggested test tightening
`@Test` public void loadsExternalCertificateBundle() throws Exception { String path = writeBundle(bundle); - OkHttpClient client = Http.enableExternalCertificates(new OkHttpClient(), path, null); + OkHttpClient baseClient = new OkHttpClient(); + OkHttpClient client = Http.enableExternalCertificates(baseClient, path, null); Assert.assertNotNull(client); + Assert.assertNotSame(baseClient, client); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@api/src/test/java/io/minio/HttpExternalCertificatesTest.java` around lines 73 - 77, The test currently only asserts the returned client is non-null; instead create a named original OkHttpClient (e.g., OkHttpClient original = new OkHttpClient()), call Http.enableExternalCertificates(original, path, null) into client, then assert the method returned a new instance (Assert.assertNotSame(original, client)) and additionally assert that the returned client's SSL configuration changed (for example verify client.sslSocketFactory() differs from original.sslSocketFactory() or that the client's SSL/TrustManager references the loaded certificate) to prove the bundle was actually applied. Use the existing test method loadsExternalCertificateBundle and the Http.enableExternalCertificates call to locate where to update assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@api/src/test/java/io/minio/HttpExternalCertificatesTest.java`:
- Around line 55-63: Update HttpExternalCertificatesTest.bundles() to include a
new parameterized case that exercises the SSL_CERT_DIR directory path: create a
temporary directory, write the PEM content into a file inside it, and add a test
case name like "ssl_cert_dir" that will be used by the test harness; then in the
corresponding test execution branch call
Http.enableExternalCertificates(baseClient, null, dirPath) (instead of the file
path) so the directory code path in Http.enableExternalCertificates is
validated; ensure you add the same directory-based case for the other
parameterized list at lines 73-77 as well.
- Around line 73-77: The test currently only asserts the returned client is
non-null; instead create a named original OkHttpClient (e.g., OkHttpClient
original = new OkHttpClient()), call Http.enableExternalCertificates(original,
path, null) into client, then assert the method returned a new instance
(Assert.assertNotSame(original, client)) and additionally assert that the
returned client's SSL configuration changed (for example verify
client.sslSocketFactory() differs from original.sslSocketFactory() or that the
client's SSL/TrustManager references the loaded certificate) to prove the bundle
was actually applied. Use the existing test method
loadsExternalCertificateBundle and the Http.enableExternalCertificates call to
locate where to update assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 39cf8c2f-fb3a-46ab-b275-692568504bfc
📒 Files selected for processing (2)
api/src/main/java/io/minio/Http.javaapi/src/test/java/io/minio/HttpExternalCertificatesTest.java
balamurugana
left a comment
balamurugana
left a comment
There was a problem hiding this comment.
Why are those env var set to empty?
|
Hi @balamurugana , sorry, not sure, what you mean :/ I encountered the issue, when using the chainguard image, which exports sets The crt file was not empty, but the MinIO still failed to start ( exception in the description ). |
|
Hi @balamurugana, may I ask for a review again? 🙏 |
|
Hi @balamurugana , can you please rerun the pipeline again? I want to rule-out the possibility of the functional test flakiness. |
|
@PavolMisudikAta Please rebase this PR |
2 participants
Problem description
Constructing a
MinioClientfails at startup whenSSL_CERT_FILE(orSSL_CERT_DIR) points at a normal CA bundle:This reproduces on 9.0.0 and 9.0.1. It bites any base image that exports
SSL_CERT_FILE/SSL_CERT_DIR(Chainguard/Wolfi, Alpine, Debian, …), which isincreasingly common.
Root cause
Http#setCertificateEntryreads the bundle with:After the final -----END CERTIFICATE-----, any trailing bytes (a trailing
blank line, or trailing whitespace — both present in real CA bundles) leave
available() > 0, so the loop calls generateCertificate once more on the
remaining whitespace and X509Factory throws IOException: Empty input.
InputStream.available() is also not a reliable end-of-stream test, so the
exact failure is environment-dependent.
Fix
Read the whole bundle with CertificateFactory#generateCertificates(InputStream),
which parses all certificates in one pass, stops at EOF, and ignores trailing
whitespace. One-line change in setCertificateEntry; covers both the
SSL_CERT_FILE and SSL_CERT_DIR paths (both call it).
Summary by CodeRabbit
Bug Fixes
Tests