Fix "Empty input" when loading SSL_CERT_FILE/SSL_CERT_DIR cert bundles.#1706
Fix "Empty input" when loading SSL_CERT_FILE/SSL_CERT_DIR cert bundles.#1706PavolMisudikAta wants to merge 2 commits into
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR changes Http.setCertificateEntry() to parse all PEM certificates at once via CertificateFactory.generateCertificates(), adds the missing Certificate import, and adds a parameterized JUnit test that validates loading bundles with different trailing newline/whitespace variants. ChangesCertificate Bundle Loading
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 ). |
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