From 5127f4cf74e6b2fd3c37ca7d86626368b6f81e60 Mon Sep 17 00:00:00 2001 From: translatenix <119817707+translatenix@users.noreply.github.com> Date: Wed, 21 Feb 2024 12:04:58 -0800 Subject: [PATCH] Improve SSL related error messages --- .../kotlin/org/pkl/cli/CliEvaluatorTest.kt | 54 +++++++++++++++---- .../org/pkl/commons/test/FileTestUtils.kt | 12 ++++- .../java/org/pkl/core/http/JdkHttpClient.java | 14 +++-- .../org/pkl/core/util/ExceptionUtils.java | 36 +++++++++++++ .../org/pkl/core/errorMessages.properties | 4 ++ .../org/pkl/core/http/HttpClientTest.kt | 17 +++--- .../org/pkl/core/http/LazyHttpClientTest.kt | 2 - .../org/pkl/core/util/ExceptionUtilsTest.kt | 48 +++++++++++++++++ 8 files changed, 160 insertions(+), 27 deletions(-) create mode 100644 pkl-core/src/main/java/org/pkl/core/util/ExceptionUtils.java create mode 100644 pkl-core/src/test/kotlin/org/pkl/core/util/ExceptionUtilsTest.kt diff --git a/pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt b/pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt index 92e922a18..45640dba6 100644 --- a/pkl-cli/src/test/kotlin/org/pkl/cli/CliEvaluatorTest.kt +++ b/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 { 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 { 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 { 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 { 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 { 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 { diff --git a/pkl-commons-test/src/main/kotlin/org/pkl/commons/test/FileTestUtils.kt b/pkl-commons-test/src/main/kotlin/org/pkl/commons/test/FileTestUtils.kt index ae6a32836..cfbb1ee3e 100644 --- a/pkl-commons-test/src/main/kotlin/org/pkl/commons/test/FileTestUtils.kt +++ b/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 = diff --git a/pkl-core/src/main/java/org/pkl/core/http/JdkHttpClient.java b/pkl-core/src/main/java/org/pkl/core/http/JdkHttpClient.java index e6e62fd9c..62edc1d06 100644 --- a/pkl-core/src/main/java/org/pkl/core/http/JdkHttpClient.java +++ b/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 HttpResponse send(HttpRequest request, BodyHandler 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 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 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) 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)); diff --git a/pkl-core/src/main/java/org/pkl/core/util/ExceptionUtils.java b/pkl-core/src/main/java/org/pkl/core/util/ExceptionUtils.java new file mode 100644 index 000000000..f56943bb6 --- /dev/null +++ b/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; + } +} diff --git a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties b/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties index 2b04f1405..32f1b660e 100644 --- a/pkl-core/src/main/resources/org/pkl/core/errorMessages.properties +++ b/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} diff --git a/pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt index 8ad85438b..a08c38fb1 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt +++ b/pkl-core/src/test/kotlin/org/pkl/core/http/HttpClientTest.kt @@ -11,7 +11,6 @@ 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 @@ -19,7 +18,7 @@ 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() diff --git a/pkl-core/src/test/kotlin/org/pkl/core/http/LazyHttpClientTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/http/LazyHttpClientTest.kt index d90370442..a0ea8fb9c 100644 --- a/pkl-core/src/test/kotlin/org/pkl/core/http/LazyHttpClientTest.kt +++ b/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 diff --git a/pkl-core/src/test/kotlin/org/pkl/core/util/ExceptionUtilsTest.kt b/pkl-core/src/test/kotlin/org/pkl/core/util/ExceptionUtilsTest.kt new file mode 100644 index 000000000..45ee769d5 --- /dev/null +++ b/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)") + } +}