fix flaky test PemTrustConfigTests#testTrustConfigReloadsFileContents

Signed-off-by: Igonin <iigonin@sternad.de>
Co-authored-by: Benny Goerzig <benny.goerzig@sap.com>
Co-authored-by: Karsten Schnitter <k.schnitter@sap.com>
Co-authored-by: Kai Sternad <k.sternad@sternad.de>

Author: 4 people

libs/ssl-config/src/test/java/org/opensearch/common/ssl/PemTrustConfigTests.java

@@ -37,6 +37,7 @@
 
 import javax.net.ssl.X509ExtendedTrustManager;
 
+import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
@@ -69,11 +70,29 @@ public void testBuildTrustConfigFromMultiplePemFiles() throws Exception {
     }
 
     public void testBadFileFormatFails() throws Exception {
-        final Path ca = createTempFile("ca", ".crt");
-        Files.write(ca, generateRandomByteArrayOfLength(128), StandardOpenOption.APPEND);
-        final PemTrustConfig trustConfig = new PemTrustConfig(Collections.singletonList(ca));
-        assertThat(trustConfig.getDependentFiles(), Matchers.containsInAnyOrder(ca));
-        assertFailedToParse(trustConfig, ca);
+        {
+            final Path ca = createTempFile("ca", ".crt");
+            Files.write(ca, "This is definitely not a PEM certificate".getBytes(StandardCharsets.UTF_8), StandardOpenOption.CREATE);
+            final PemTrustConfig trustConfig = new PemTrustConfig(Collections.singletonList(ca));
+            assertThat(trustConfig.getDependentFiles(), Matchers.containsInAnyOrder(ca));
+            assertFailedToParse(trustConfig, ca);
+        }
+
+        {
+            final Path ca = createTempFile("ca", ".crt");
+            Files.write(ca, generateInvalidPemBytes(), StandardOpenOption.CREATE);
+            final PemTrustConfig trustConfig = new PemTrustConfig(Collections.singletonList(ca));
+            assertThat(trustConfig.getDependentFiles(), Matchers.containsInAnyOrder(ca));
+            assertCannotCreateTrust(trustConfig, ca);
+        }
+
+        { // test DER-encoded sequence
+            final Path ca = createTempFile("ca", ".crt");
+            Files.write(ca, generateInvalidDerEncodedPemBytes(), StandardOpenOption.CREATE);
+            final PemTrustConfig trustConfig = new PemTrustConfig(Collections.singletonList(ca));
+            assertThat(trustConfig.getDependentFiles(), Matchers.containsInAnyOrder(ca));
+            assertCannotCreateTrust(trustConfig, ca);
+        }
     }
 
     public void testEmptyFileFails() throws Exception {
@@ -119,8 +138,8 @@ public void testTrustConfigReloadsFileContents() throws Exception {
         Files.delete(ca1);
         assertFileNotFound(trustConfig, ca1);
 
-        Files.write(ca1, generateRandomByteArrayOfLength(128), StandardOpenOption.CREATE);
-        assertFailedToParse(trustConfig, ca1);
+        Files.write(ca1, generateInvalidPemBytes(), StandardOpenOption.CREATE);
+        assertCannotCreateTrust(trustConfig, ca1);
     }
 
     private void assertCertificateChain(PemTrustConfig trustConfig, String... caNames) {
@@ -134,13 +153,23 @@ private void assertCertificateChain(PemTrustConfig trustConfig, String... caName
         assertThat(issuerNames, Matchers.containsInAnyOrder(caNames));
     }
 
+    // The parser returns an empty collection when no valid sequence is found,
+    // but our implementation requires an exception to be thrown in this case
     private void assertFailedToParse(PemTrustConfig trustConfig, Path file) {
         final SslConfigException exception = expectThrows(SslConfigException.class, trustConfig::createTrustManager);
         logger.info("failure", exception);
         assertThat(exception.getMessage(), Matchers.containsString(file.toAbsolutePath().toString()));
         assertThat(exception.getMessage(), Matchers.containsString("Failed to parse any certificate from"));
     }
 
+    // The parser encounters malformed PEM data
+    private void assertCannotCreateTrust(PemTrustConfig trustConfig, Path file) {
+        final SslConfigException exception = expectThrows(SslConfigException.class, trustConfig::createTrustManager);
+        logger.info("failure", exception);
+        assertThat(exception.getMessage(), Matchers.containsString(file.toAbsolutePath().toString()));
+        assertThat(exception.getMessage(), Matchers.containsString("cannot create trust using PEM certificates"));
+    }
+
     private void assertFileNotFound(PemTrustConfig trustConfig, Path file) {
         final SslConfigException exception = expectThrows(SslConfigException.class, trustConfig::createTrustManager);
         assertThat(exception.getMessage(), Matchers.containsString("files do not exist"));
@@ -149,23 +178,15 @@ private void assertFileNotFound(PemTrustConfig trustConfig, Path file) {
         assertThat(exception.getCause(), Matchers.instanceOf(NoSuchFileException.class));
     }
 
-    private byte[] generateRandomByteArrayOfLength(int length) {
-        byte[] bytes = randomByteArrayOfLength(length);
-        /*
-         * If the bytes represent DER encoded value indicating ASN.1 SEQUENCE followed by length byte if it is zero then while trying to
-         * parse PKCS7 block from the encoded stream, it failed parsing the content type. The DerInputStream.getSequence() method in this
-         * case returns an empty DerValue array but ContentType does not check the length of array before accessing the array resulting in a
-         * ArrayIndexOutOfBoundsException. This check ensures that when we create random stream of bytes we do not create ASN.1 SEQUENCE
-         * followed by zero length which fails the test intermittently.
-         */
-        while (checkRandomGeneratedBytesRepresentZeroLengthDerSequenceCausingArrayIndexOutOfBound(bytes)) {
-            bytes = randomByteArrayOfLength(length);
-        }
-        return bytes;
+    private byte[] generateInvalidPemBytes() {
+        String invalidPem = "-----BEGIN CERTIFICATE-----\nINVALID_CONTENT\n-----END CERTIFICATE-----";
+        return invalidPem.getBytes(StandardCharsets.UTF_8);
     }
 
-    private static boolean checkRandomGeneratedBytesRepresentZeroLengthDerSequenceCausingArrayIndexOutOfBound(byte[] bytes) {
-        // Tag value indicating an ASN.1 "SEQUENCE". Reference: sun.security.util.DerValue.tag_Sequence = 0x30
-        return bytes[0] == 0x30 && bytes[1] == 0x00;
+    private byte[] generateInvalidDerEncodedPemBytes() {
+        byte[] shortFormZeroLength = { 0x30, 0x00 };
+        byte[] longFormZeroLength = { 0x30, (byte) 0x81, 0x00 };
+        byte[] indefiniteForm = { 0x30, (byte) 0x80, 0x01, 0x02, 0x00, 0x00 };
+        return randomFrom(shortFormZeroLength, longFormZeroLength, indefiniteForm);
     }
 }