Improve SSL related error messages

Author: odenix

pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt

@@ -21,14 +21,13 @@ import java.net.URI
 import java.nio.file.Files
 import java.nio.file.Path
 import java.time.Duration
-import kotlin.io.path.createDirectories
-import kotlin.io.path.listDirectoryEntries
+import kotlin.io.path.*
 import org.assertj.core.api.Assertions.assertThat
 import org.assertj.core.api.Assertions.assertThatCode
 import org.junit.jupiter.api.AfterEach
-import org.junit.jupiter.api.Disabled
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertThrows
+import org.junit.jupiter.api.io.TempDir
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.EnumSource
 import org.pkl.commons.*
@@ -1158,9 +1157,45 @@ result = someLib.x
   }
 
   @Test
-  @Disabled("flaky because CliEvaluator falls back to ~/.pkl/cacerts if no certs are given")
-  fun `not including the self signed certificate will result in a error`() {
+  fun `gives decent error message if certificate file contains random text`() {
+    val certsFile = tempDir.writeFile("random.pem", "RANDOM")
+    val err = assertThrows<CliException> { evalModuleThatImportsPackage(certsFile) }
+    assertThat(err)
+      .hasMessageContaining("Error parsing CA certificate file `${certsFile.pathString}`:")
+      .hasMessageContaining("No certificate data found")
+      .hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
+  }
+
+  @Test
+  fun `gives decent error message if certificate file is emtpy`(@TempDir tempDir: Path) {
+    val emptyCerts = tempDir.writeEmptyFile("empty.pem")
+    val err = assertThrows<CliException> { evalModuleThatImportsPackage(emptyCerts) }
+    assertThat(err).hasMessageContaining("CA certificate file `${emptyCerts.pathString}` is empty.")
+  }
+
+  @Test
+  fun `gives decent error message if certificate cannot be parsed`(@TempDir tempDir: Path) {
+    val invalidCerts = FileTestUtils.writeCertificateWithMissingLines(tempDir)
+    val err = assertThrows<CliException> { evalModuleThatImportsPackage(invalidCerts) }
+    assertThat(err)
+      .hasMessageContaining("Error parsing CA certificate file `${invalidCerts.pathString}`:")
+      .hasMessageContaining("not enough content")
+      .hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
+  }
+
+  @Test
+  fun `gives decent error message if CLI doesn't have the required CA certificate`() {
     PackageServer.ensureStarted()
+    // provide SOME certs to prevent CliEvaluator from falling back to ~/.pkl/cacerts
+    val builtInCerts = FileTestUtils.writePklBuiltInCertificates(tempDir)
+    val err = assertThrows<CliException> { evalModuleThatImportsPackage(builtInCerts) }
+    assertThat(err)
+      .hasMessageContaining("Error during SSL handshake with host `localhost`:")
+      .hasMessageContaining("unable to find valid certification path to requested target")
+      .hasMessageNotContainingAny("java.", "sun.") // class names have been filtered out
+  }
+
+  private fun evalModuleThatImportsPackage(certsFile: Path) {
     val moduleUri =
       writePklFile(
         "test.pkl",
@@ -1169,20 +1204,17 @@ result = someLib.x
       
       res = Swallow
     """
-          .trimIndent()
       )
-    val buffer = StringWriter()
+
     val options =
       CliEvaluatorOptions(
         CliBaseOptions(
           sourceModules = listOf(moduleUri),
-          workingDir = tempDir,
-          moduleCacheDir = tempDir,
+          caCertificates = listOf(certsFile),
           noCache = true
         ),
       )
-    val err = assertThrows<CliException> { CliEvaluator(options, consoleWriter = buffer).run() }
-    assertThat(err.message).contains("unable to find valid certification path to requested target")
+    CliEvaluator(options).run()
   }
 
   private fun writePklFile(fileName: String, contents: String = defaultContents): URI {

pkl-commons-test/src/main/kotlin/org/pkl/commons/test/FileTestUtils.kt

@@ -17,7 +17,6 @@ package org.pkl.commons.test
 
 import java.nio.file.Path
 import kotlin.io.path.*
-import kotlin.streams.toList
 import org.assertj.core.api.Assertions.fail
 import org.pkl.commons.*
 
@@ -32,6 +31,17 @@ object FileTestUtils {
   val selfSignedCertificate: Path by lazy {
     rootProjectDir.resolve("pkl-commons-test/build/keystore/localhost.pem")
   }
+
+  fun writeCertificateWithMissingLines(dir: Path): Path {
+    val lines = selfSignedCertificate.readLines()
+    // drop some lines in the middle
+    return dir.resolve("invalidCerts.pem").writeLines(lines.take(5) + lines.takeLast(5))
+  }
+
+  fun writePklBuiltInCertificates(dir: Path): Path {
+    val text = javaClass.getResource("/org/pkl/core/http/IncludedCARoots.pem")!!.readText()
+    return dir.resolve("IncludedCARoots.pem").apply { writeText(text) }
+  }
 }
 
 fun Path.listFilesRecursively(): List<Path> =

pkl-core/src/main/java/org/pkl/core/http/JdkHttpClient.java

@@ -46,8 +46,10 @@
 import javax.annotation.concurrent.ThreadSafe;
 import javax.net.ssl.CertPathTrustManagerParameters;
 import javax.net.ssl.SSLContext;
+import javax.net.ssl.SSLHandshakeException;
 import javax.net.ssl.TrustManagerFactory;
 import org.pkl.core.util.ErrorMessages;
+import org.pkl.core.util.ExceptionUtils;
 
 @ThreadSafe
 final class JdkHttpClient implements HttpClient {
@@ -89,6 +91,10 @@ public <T> HttpResponse<T> send(HttpRequest request, BodyHandler<T> responseBody
       // original exception has no message
       throw new ConnectException(
           ErrorMessages.create("errorConnectingToHost", request.uri().getHost()));
+    } catch (SSLHandshakeException e) {
+      throw new SSLHandshakeException(
+          ErrorMessages.create(
+              "errorSslHandshake", request.uri().getHost(), ExceptionUtils.getRootReason(e)));
     } catch (InterruptedException e) {
       // next best thing after letting (checked) InterruptedException bubble up
       Thread.currentThread().interrupt();
@@ -134,7 +140,7 @@ private static SSLContext createSslContext(
       return sslContext;
     } catch (GeneralSecurityException e) {
       throw new HttpClientInitException(
-          ErrorMessages.create("cannotInitHttpClient", e.getMessage()), e);
+          ErrorMessages.create("cannotInitHttpClient", ExceptionUtils.getRootReason(e)), e);
     }
   }
 
@@ -149,7 +155,7 @@ private static Set<TrustAnchor> createTrustAnchors(
         throw new HttpClientInitException(ErrorMessages.create("cannotFindCertFile", file));
       } catch (IOException e) {
         throw new HttpClientInitException(
-            ErrorMessages.create("cannotReadCertFile", e.getMessage()));
+            ErrorMessages.create("cannotReadCertFile", ExceptionUtils.getRootReason(e)));
       }
     }
 
@@ -158,7 +164,7 @@ private static Set<TrustAnchor> createTrustAnchors(
         collectTrustAnchors(anchors, factory, stream, url);
       } catch (IOException e) {
         throw new HttpClientInitException(
-            ErrorMessages.create("cannotReadCertFile", e.getMessage()));
+            ErrorMessages.create("cannotReadCertFile", ExceptionUtils.getRootReason(e)));
       }
     }
 
@@ -176,7 +182,7 @@ private static void collectTrustAnchors(
       certificates = (Collection<X509Certificate>) factory.generateCertificates(stream);
     } catch (CertificateException e) {
       throw new HttpClientInitException(
-          ErrorMessages.create("cannotParseCertFile", source, e.getMessage()));
+          ErrorMessages.create("cannotParseCertFile", source, ExceptionUtils.getRootReason(e)));
     }
     if (certificates.isEmpty()) {
       throw new HttpClientInitException(ErrorMessages.create("emptyCertFile", source));

pkl-core/src/main/java/org/pkl/core/util/ExceptionUtils.java

@@ -0,0 +1,36 @@
+/**
+ * Copyright © 2024 Apple Inc. and the Pkl project authors. All rights reserved.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+package org.pkl.core.util;
+
+public final class ExceptionUtils {
+  private ExceptionUtils() {}
+
+  public static Throwable getRootCause(Throwable t) {
+    var result = t;
+    var cause = result.getCause();
+    while (cause != null) {
+      result = cause;
+      cause = cause.getCause();
+    }
+    return result;
+  }
+
+  public static String getRootReason(Throwable t) {
+    var reason = getRootCause(t).getMessage();
+    if (reason == null || reason.isEmpty()) reason = "(unknown reason)";
+    return reason;
+  }
+}

pkl-core/src/main/resources/org/pkl/core/errorMessages.properties

@@ -943,6 +943,10 @@ Exception when making request `GET {0}`:\n\
 errorConnectingToHost=\
 Error connecting to host `{0}`.
 
+errorSslHandshake=\
+Error during SSL handshake with host `{0}`:\n\
+{1}
+
 cannotInitHttpClient=\
 Error initializing HTTP client:\n\
 {0}

pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt

@@ -11,15 +11,14 @@ import java.net.URI
 import java.net.http.HttpRequest
 import java.net.http.HttpResponse
 import java.nio.file.Path
-import java.security.cert.CertificateException
 import java.time.Duration
 import kotlin.io.path.copyTo
 import kotlin.io.path.createDirectories
 import kotlin.io.path.createFile
 
 class HttpClientTest {
   @Test
-  fun `build default client`() {
+  fun `can build default client`() {
     val client = assertDoesNotThrow {
       HttpClient.builder().build()
     } // means we loaded some (built-in) certificates
@@ -38,7 +37,7 @@ class HttpClientTest {
   }
 
   @Test
-  fun `build custom client`() {
+  fun `can build custom client`() {
     val client = HttpClient.builder()
       .setUserAgent("Agent 1")
       .setRequestTimeout(Duration.ofHours(86))
@@ -53,7 +52,7 @@ class HttpClientTest {
   }
 
   @Test
-  fun `load certificates from file system`() {
+  fun `can load certificates from file system`() {
     assertDoesNotThrow {
       HttpClient.builder().addCertificates(FileTestUtils.selfSignedCertificate).build()
     }
@@ -71,7 +70,7 @@ class HttpClientTest {
   }
 
   @Test
-  fun `load certificates from class path`() {
+  fun `can load certificates from class path`() {
     assertDoesNotThrow {
       HttpClient.builder().addCertificates(javaClass.getResource("IncludedCARoots.pem")).build()
     }
@@ -89,14 +88,14 @@ class HttpClientTest {
   }
 
   @Test
-  fun `load built-in certificates`() {
+  fun `can load built-in certificates`() {
     assertDoesNotThrow {  
       HttpClient.builder().addBuiltInCertificates().build()
     }
   }
 
   @Test
-  fun `load certificates from default location`(@TempDir userHome: Path) {
+  fun `can load certificates from default location`(@TempDir userHome: Path) {
     val certFile = userHome.resolve(".pkl")
       .resolve("cacerts")
       .createDirectories()
@@ -116,7 +115,7 @@ class HttpClientTest {
   }
 
   @Test
-  fun `client can be closed multiple times`() {
+  fun `can be closed multiple times`() {
     val client = HttpClient.builder().build()
 
     assertDoesNotThrow { 
@@ -126,7 +125,7 @@ class HttpClientTest {
   }
 
   @Test
-  fun `client refuses to send messages once closed`() {
+  fun `refuses to send messages once closed`() {
     val client = HttpClient.builder().build()
     val request = HttpRequest.newBuilder(URI("https://example.com")).build()
 

pkl-core/src/test/kotlin/org/pkl/core/http/LazyHttpClientTest.kt

@@ -1,13 +1,11 @@
 package org.pkl.core.http
 
-import org.assertj.core.api.Assertions.assertThat
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.api.assertDoesNotThrow
 import org.junit.jupiter.api.assertThrows
 import java.net.URI
 import java.net.http.HttpRequest
 import java.net.http.HttpResponse.BodyHandlers
-import java.security.cert.CertificateException
 
 class LazyHttpClientTest {
   @Test

pkl-core/src/test/kotlin/org/pkl/core/util/ExceptionUtilsTest.kt

@@ -0,0 +1,48 @@
+package org.pkl.core.util
+
+import org.assertj.core.api.Assertions.assertThat
+import org.junit.jupiter.api.Test
+import java.io.IOException
+import java.lang.Error
+
+class ExceptionUtilsTest {
+  @Test
+  fun `get root cause of simple exception`() {
+    val e = IOException("io")
+    assertThat(ExceptionUtils.getRootCause(e)).isSameAs(e)
+  }
+  
+  @Test
+  fun `get root cause of nested exception`() {
+    val e = IOException("io")
+    val e2 = RuntimeException("runtime")
+    val e3 = Error("error")
+    e.initCause(e2)
+    e2.initCause(e3)
+    assertThat(ExceptionUtils.getRootCause(e)).isSameAs(e3)
+  }
+
+  @Test
+  fun `get root reason`() {
+    val e = IOException("io")
+    val e2 = RuntimeException("the root reason")
+    e.initCause(e2)
+    assertThat(ExceptionUtils.getRootReason(e)).isEqualTo("the root reason")
+  }
+  
+  @Test
+  fun `get root reason if null`() {
+    val e = IOException("io")
+    val e2 = RuntimeException(null as String?)
+    e.initCause(e2)
+    assertThat(ExceptionUtils.getRootReason(e)).isEqualTo("(unknown reason)")
+  }
+
+  @Test
+  fun `get root reason if empty`() {
+    val e = IOException("io")
+    val e2 = RuntimeException("")
+    e.initCause(e2)
+    assertThat(ExceptionUtils.getRootReason(e)).isEqualTo("(unknown reason)")
+  }
+}