Use generateCertificates() of CertificateFactory to process certificates #6579
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #6579 +/- ##
============================================
- Coverage 90.62% 90.60% -0.03%
- Complexity 6261 6262 +1
============================================
Files 689 690 +1
Lines 18704 18706 +2
Branches 1844 1846 +2
============================================
- Hits 16951 16948 -3
- Misses 1198 1199 +1
- Partials 555 559 +4 ☔ View full report in Codecov by Sentry. |
| // pass the input stream to generateCertificates to get a list of certificates | ||
| // generateCertificates can handle multiple certificates in a single input stream | ||
| // including PEM files with explanatory text | ||
| List<? extends Certificate> chain = (List<? extends Certificate>) cf.generateCertificates(is); |
There was a problem hiding this comment.
I wrote a test case for this, but was surprised that it passes on the main branch. Not sure why it would pass if this was required for handle explanatory text:
@TempDir private Path tempDir;
private static final String EXPLANATORY_TEXT =
"Subject: CN=Foo\n"
+ "Issuer: CN=Foo\n"
+ "Validity: from 7/9/2012 3:10:38 AM UTC to 7/9/2013 3:10:37 AM UTC\n";
/**
* Append <a href="https://datatracker.ietf.org/doc/html/rfc7468#section-5.2">explanatory text</a>
* prefix and verify {@link TlsUtil#keyManager(byte[], byte[])} succeeds.
*/
@ParameterizedTest
@MethodSource("keyManagerArgs")
void keyManager_CertWithExplanatoryText(SelfSignedCertificate selfSignedCertificate)
throws IOException {
Path certificate = tempDir.resolve("certificate");
Files.write(certificate, EXPLANATORY_TEXT.getBytes(StandardCharsets.UTF_8));
Files.write(
certificate,
com.google.common.io.Files.toByteArray(selfSignedCertificate.certificate()),
StandardOpenOption.APPEND);
assertThatCode(
() ->
TlsUtil.keyManager(
com.google.common.io.Files.toByteArray(selfSignedCertificate.privateKey()),
com.google.common.io.Files.toByteArray(new File(certificate.toString()))))
.doesNotThrowAnyException();
}
private static Stream<Arguments> keyManagerArgs() throws CertificateException {
Instant now = Instant.now();
return Stream.of(
Arguments.of(
new SelfSignedCertificate(Date.from(now), Date.from(now), "RSA", 2048),
new SelfSignedCertificate(Date.from(now), Date.from(now), "EC", 256)));
}
There was a problem hiding this comment.
I definitely faced this problem with our internal certificate bundle, but after your comment, I debugged a little more and I am wondering if the bundle was not created correctly.
When I iterated using our internal bundle with the main branch code, it added 12 certificates correctly ( even with the explanatory text ) but at the end there was one byte which ended up throwing "empty input" error.
So main branch code is able to handle explanatory text but cf.generateCertificates(is) is a little more forgiving.
I completely understand if you prefer to not merge this PR.
There was a problem hiding this comment.
I'm happy enough with switching to cf.generateCertificates(is) if its a more robust parsing solution, but just need to demonstrate that with some test case. I.e. some certificate that fails with the logic on main but succeeds when this logic is applied.
| Files.write( | ||
| certificate, | ||
| "\n".getBytes(StandardCharsets.UTF_8), | ||
| StandardOpenOption.APPEND); |
There was a problem hiding this comment.
If I understand correct, the approach in this PR of using factory.generateCertificates(is) is able to handle a trailing line break where as the approach on main can not?
There was a problem hiding this comment.
yes, that is correct.
Signed-off-by: Abhijeet V <[email protected]>
a1972c1 to
71a7702
Compare
|
I reordered the commits and removed my "fix" commit. This should fail the test with main branch code. |
|
As expected, the main branch code is failing with current test. |
Signed-off-by: Abhijeet V <[email protected]>
|
Thank you @jack-berg for the original test and @evantorrie for helping to reproduce the issue. |
…tes (open-telemetry#6579) Signed-off-by: Abhijeet V <[email protected]> Co-authored-by: ET <[email protected]>
fixes #6572
Instead of parsing the inputStream ourselves, we should let the underlying CertificateFactory implementation process it and return list of certificates.