Use java.net.http.HttpClient instead of java.net.Http(s)URLConnection (…

…#217)

Moving to java.net.http.HttpClient brings many benefits, including
HTTP/2 support and the ability to make asynchronous requests.

Major additions and changes:
- Introduce a lightweight org.pkl.core.http.HttpClient API.
  This keeps some flexibility and allows to enforce behavior
  such as setting the User-Agent header.
- Provide an implementation that delegates to java.net.http.HttpClient.
- Use HttpClient for all HTTP(s) requests across the codebase.
  This required adding an HttpClient parameter to constructors and
  factory methods of multiple classes, some of which are public APIs.
- Manage CA certificates per HTTP client instead of per JVM.
  This makes it unnecessary to set JVM-wide system/security properties
  and default SSLSocketFactory's.
- Add executor v2 options to the executor SPI
- Add pkl-certs as a new artifact, and remove certs from pkl-commons-cli artifact

Each HTTP client maintains its own connection pool and SSLContext.
For efficiency reasons, It's best to reuse clients whenever feasible.
To avoid memory leaks, clients are not stored in static fields.

HTTP clients are expensive to create. For this reason,
EvaluatorBuilder defaults to a "lazy" client that creates the underlying
java.net.http.HttpClient on the first send (which may never happen).

bench/src/jmh/java/org/pkl/core/ListSort.java

@@ -23,6 +23,7 @@
 import org.openjdk.jmh.annotations.*;
 import org.openjdk.jmh.util.TempFile;
 import org.openjdk.jmh.util.TempFileManager;
+import org.pkl.core.http.HttpClient;
 import org.pkl.core.module.ModuleKeyFactories;
 import org.pkl.core.repl.ReplRequest;
 import org.pkl.core.repl.ReplResponse;
@@ -39,6 +40,7 @@ public class ListSort {
   private static final ReplServer repl =
       new ReplServer(
           SecurityManagers.defaultManager,
+          HttpClient.dummyClient(),
           Loggers.stdErr(),
           List.of(ModuleKeyFactories.standardLibrary),
           List.of(ResourceReaders.file()),

docs/src/test/kotlin/DocSnippetTests.kt

@@ -25,6 +25,7 @@ import org.pkl.core.repl.ReplServer
 import org.pkl.core.resource.ResourceReaders
 import org.pkl.core.util.IoUtils
 import org.antlr.v4.runtime.ParserRuleContext
+import org.pkl.core.http.HttpClient
 import java.nio.file.Files
 import kotlin.io.path.isDirectory
 import kotlin.io.path.isRegularFile
@@ -78,6 +79,7 @@ class DocSnippetTestsEngine : HierarchicalTestEngine<DocSnippetTestsEngine.Execu
   override fun createExecutionContext(request: ExecutionRequest): ExecutionContext {
     val replServer = ReplServer(
       SecurityManagers.defaultManager,
+      HttpClient.dummyClient(),
       Loggers.stdErr(),
       listOf(
         ModuleKeyFactories.standardLibrary,

gradle/libs.versions.toml

@@ -81,6 +81,7 @@ msgpack = { group = "org.msgpack", name = "msgpack-core", version.ref = "msgpack
 nuValidator = { group = "nu.validator", name = "validator", version.ref = "nuValidator" }
 # to be replaced with https://github.com/usethesource/capsule or https://github.com/lacuna/bifurcan
 paguro = { group = "org.organicdesign", name = "Paguro", version.ref = "paguro" }
+pklConfigJavaAll025 = { group = "org.pkl-lang", name = "pkl-config-java-all", version = "0.25.0" }
 shadowPlugin = { group = "gradle.plugin.com.github.johnrengelman", name = "shadow", version.ref = "shadowPlugin" }
 slf4jApi = { group = "org.slf4j", name = "slf4j-api", version.ref = "slf4j" }
 slf4jSimple = { group = "org.slf4j", name = "slf4j-simple", version.ref = "slf4j" }

pkl-certs/pkl-certs.gradle.kts

@@ -0,0 +1,19 @@
+plugins {
+  pklAllProjects
+  pklJavaLibrary
+  pklPublishLibrary
+}
+
+publishing {
+  publications {
+    named<MavenPublication>("library") {
+      pom {
+        url.set("https://github.com/apple/pkl/tree/main/pkl-certs")
+        description.set("""
+          Pkl's built-in CA certificates. 
+          Used by Pkl CLIs and optionally supported by pkl-core.")
+        """.trimIndent())
+      }
+    }
+  }
+}

pkl-cli/pkl-cli.gradle.kts

@@ -156,7 +156,7 @@ fun Exec.configureExecutable(isEnabled: Boolean, outputFile: File, extraArgs: Li
         ,"--no-fallback"
         ,"-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl"
         ,"-H:IncludeResources=org/jline/utils/.*"
-        ,"-H:IncludeResources=org/pkl/commons/cli/commands/IncludedCARoots.pem"
+        ,"-H:IncludeResources=org/pkl/certs/PklCARoots.pem"
         //,"-H:IncludeResources=org/pkl/core/Release.properties"
         ,"-H:IncludeResourceBundles=org.pkl.core.errorMessages"
         ,"--macro:truffle"

pkl-cli/src/main/kotlin/org/pkl/cli/CliPackageDownloader.kt

@@ -31,7 +31,7 @@ class CliPackageDownloader(
     if (moduleCacheDir == null) {
       throw CliException("Cannot download packages because no cache directory is specified.")
     }
-    val packageResolver = PackageResolver.getInstance(securityManager, moduleCacheDir)
+    val packageResolver = PackageResolver.getInstance(securityManager, httpClient, moduleCacheDir)
     val errors = mutableMapOf<PackageUri, Throwable>()
     for (pkg in packageUris) {
       try {

pkl-cli/src/main/kotlin/org/pkl/cli/CliProjectPackager.kt

@@ -82,6 +82,7 @@ class CliProjectPackager(
         outputPath,
         stackFrameTransformer,
         securityManager,
+        httpClient,
         skipPublishCheck,
         consoleWriter
       )

pkl-cli/src/main/kotlin/org/pkl/cli/CliProjectResolver.kt

@@ -40,6 +40,7 @@ class CliProjectResolver(
             SecurityManagers.defaultTrustLevels,
             rootDir
           ),
+          httpClient,
           moduleCacheDir
         )
       val dependencies = ProjectDependenciesResolver(project, packageResolver, errWriter).resolve()

pkl-cli/src/main/kotlin/org/pkl/cli/CliRepl.kt

@@ -36,6 +36,7 @@ internal class CliRepl(private val options: CliEvaluatorOptions) : CliCommand(op
             SecurityManagers.defaultTrustLevels,
             rootDir
           ),
+          httpClient,
           Loggers.stdErr(),
           listOf(
             ModuleKeyFactories.standardLibrary,

pkl-cli/src/main/kotlin/org/pkl/cli/CliServer.kt

@@ -25,7 +25,7 @@ import org.pkl.server.Server
 class CliServer(options: CliBaseOptions) : CliCommand(options) {
   override fun doRun() =
     try {
-      val server = Server(MessageTransports.stream(System.`in`, System.out))
+      val server = Server(MessageTransports.stream(System.`in`, System.out), httpClient)
       server.use { it.start() }
     } catch (e: ProtocolException) {
       throw CliException(e.message!!)

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

@@ -21,19 +21,18 @@ 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.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.*
 import org.pkl.commons.cli.CliBaseOptions
 import org.pkl.commons.cli.CliException
-import org.pkl.commons.cli.commands.BaseOptions
 import org.pkl.commons.test.FileTestUtils
 import org.pkl.commons.test.PackageServer
 import org.pkl.core.OutputFormat
@@ -1158,8 +1157,46 @@ result = someLib.x
   }
 
   @Test
-  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)
+      // no assert for detail message because it differs between JDK implementations
+      .hasMessageContaining("Error parsing CA certificate file `${invalidCerts.pathString}`:")
+      .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)
+      // on some JDK11's this doesn't cause SSLHandshakeException but some other SSLException
+      // .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",
@@ -1168,22 +1205,17 @@ result = someLib.x
       
       res = Swallow
     """
-          .trimIndent()
       )
-    val buffer = StringWriter()
+
     val options =
       CliEvaluatorOptions(
         CliBaseOptions(
           sourceModules = listOf(moduleUri),
-          workingDir = tempDir,
-          moduleCacheDir = tempDir,
-          noCache = true,
-          // ensure we override any previously set root cert to the default buundle.
-          caCertificates = listOf(BaseOptions.Companion.includedCARootCerts())
+          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-cli/src/test/kotlin/org/pkl/cli/CliPackageDownloaderTest.kt

@@ -202,7 +202,7 @@ class CliPackageDownloaderTest {
 
         Failed to download package://bogus.domain/[email protected] because:
         Exception when making request `GET https://bogus.domain/[email protected]`:
-        bogus.domain
+        Error connecting to host `bogus.domain`.
 
       """
           .trimIndent()

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

@@ -34,7 +34,6 @@ import org.pkl.commons.readString
 import org.pkl.commons.test.FileTestUtils
 import org.pkl.commons.test.PackageServer
 import org.pkl.commons.writeString
-import org.pkl.core.runtime.CertificateUtils
 
 class CliProjectPackagerTest {
   @Test
@@ -868,7 +867,6 @@ class CliProjectPackagerTest {
   @Test
   fun `publish checks`(@TempDir tempDir: Path) {
     PackageServer.ensureStarted()
-    CertificateUtils.setupAllX509CertificatesGlobally(listOf(FileTestUtils.selfSignedCertificate))
     tempDir.writeFile("project/main.pkl", "res = 1")
     tempDir.writeFile(
       "project/PklProject",
@@ -888,7 +886,10 @@ class CliProjectPackagerTest {
     val e =
       assertThrows<CliException> {
         CliProjectPackager(
-            CliBaseOptions(workingDir = tempDir),
+            CliBaseOptions(
+              workingDir = tempDir,
+              caCertificates = listOf(FileTestUtils.selfSignedCertificate)
+            ),
             listOf(tempDir.resolve("project")),
             CliTestOptions(),
             ".out/%{name}@%{version}",
@@ -912,7 +913,6 @@ class CliProjectPackagerTest {
   @Test
   fun `publish check when package is not yet published`(@TempDir tempDir: Path) {
     PackageServer.ensureStarted()
-    CertificateUtils.setupAllX509CertificatesGlobally(listOf(FileTestUtils.selfSignedCertificate))
     tempDir.writeFile("project/main.pkl", "res = 1")
     tempDir.writeFile(
       "project/PklProject",
@@ -930,7 +930,10 @@ class CliProjectPackagerTest {
     )
     val out = StringWriter()
     CliProjectPackager(
-        CliBaseOptions(workingDir = tempDir),
+        CliBaseOptions(
+          workingDir = tempDir,
+          caCertificates = listOf(FileTestUtils.selfSignedCertificate)
+        ),
         listOf(tempDir.resolve("project")),
         CliTestOptions(),
         ".out/%{name}@%{version}",

pkl-cli/src/test/kotlin/org/pkl/cli/repl/ReplMessagesTest.kt

@@ -21,6 +21,7 @@ import org.pkl.commons.toPath
 import org.pkl.core.Loggers
 import org.pkl.core.SecurityManagers
 import org.pkl.core.StackFrameTransformers
+import org.pkl.core.http.HttpClient
 import org.pkl.core.module.ModuleKeyFactories
 import org.pkl.core.repl.ReplRequest
 import org.pkl.core.repl.ReplResponse
@@ -30,6 +31,7 @@ class ReplMessagesTest {
   private val server =
     ReplServer(
       SecurityManagers.defaultManager,
+      HttpClient.dummyClient(),
       Loggers.stdErr(),
       listOf(ModuleKeyFactories.standardLibrary),
       listOf(),

pkl-commons-cli/pkl-commons-cli.gradle.kts

@@ -14,6 +14,7 @@ dependencies {
 
   implementation(projects.pklCommons)
   testImplementation(projects.pklCommonsTest)
+  runtimeOnly(projects.pklCerts)
 }
 
 publishing {

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliBaseOptions.kt

@@ -20,6 +20,7 @@ import java.nio.file.Files
 import java.nio.file.Path
 import java.time.Duration
 import java.util.regex.Pattern
+import org.pkl.core.http.HttpClient
 import org.pkl.core.module.ProjectDependenciesManager
 import org.pkl.core.util.IoUtils
 
@@ -113,15 +114,15 @@ data class CliBaseOptions(
   val testMode: Boolean = false,
 
   /**
-   * [X.509 certificates](https://en.wikipedia.org/wiki/X.509) in PEM format.
+   * The CA certificates to trust.
    *
-   * Elements can either be a [Path] or a [java.io.InputStream]. Input streams are closed when
-   * [CliCommand] is initialized.
+   * The given files must contain [X.509](https://en.wikipedia.org/wiki/X.509) certificates in PEM
+   * format.
    *
-   * If not empty, this determines the CA root certs used for all HTTPS requests. Warning: this
-   * affects the whole Java runtime, not just the Pkl API!
+   * If [caCertificates] is the empty list, the certificate files in `~/.pkl/cacerts/` are used. If
+   * `~/.pkl/cacerts/` does not exist or is empty, Pkl's built-in CA certificates are used.
    */
-  val caCertificates: List<Any> = emptyList(),
+  val caCertificates: List<Path> = listOf(),
 ) {
 
   companion object {
@@ -167,4 +168,26 @@ data class CliBaseOptions(
     projectDir?.resolve(ProjectDependenciesManager.PKL_PROJECT_FILENAME)
       ?: normalizedWorkingDir.getProjectFile(rootDir)
   }
+
+  /** [caCertificates] after normalization. */
+  val normalizedCaCertificates: List<Path> = caCertificates.map(normalizedWorkingDir::resolve)
+
+  /**
+   * The HTTP client shared between CLI commands created with this [CliBaseOptions] instance.
+   *
+   * To release the resources held by the HTTP client in a timely manner, call its `close()` method.
+   */
+  val httpClient: HttpClient by lazy {
+    with(HttpClient.builder()) {
+      if (normalizedCaCertificates.isEmpty()) {
+        addDefaultCliCertificates()
+      } else {
+        for (file in normalizedCaCertificates) addCertificates(file)
+      }
+      // Lazy building significantly reduces execution time of commands that do minimal work.
+      // However, it means that HTTP client initialization errors won't surface until an HTTP
+      // request is made.
+      buildLazily()
+    }
+  }
 }

pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt

@@ -18,24 +18,18 @@ package org.pkl.commons.cli
 import java.nio.file.Path
 import java.util.regex.Pattern
 import org.pkl.core.*
+import org.pkl.core.http.HttpClient
 import org.pkl.core.module.ModuleKeyFactories
 import org.pkl.core.module.ModuleKeyFactory
 import org.pkl.core.module.ModulePathResolver
 import org.pkl.core.project.Project
 import org.pkl.core.resource.ResourceReader
 import org.pkl.core.resource.ResourceReaders
-import org.pkl.core.runtime.CertificateUtils
 import org.pkl.core.settings.PklSettings
 import org.pkl.core.util.IoUtils
 
 /** Building block for CLI commands. Configured programmatically to allow for embedding. */
 abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
-  init {
-    if (cliOptions.caCertificates.isNotEmpty()) {
-      CertificateUtils.setupAllX509CertificatesGlobally(cliOptions.caCertificates)
-    }
-  }
-
   /** Runs this command. */
   fun run() {
     if (cliOptions.testMode) {
@@ -158,6 +152,10 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
     )
   }
 
+  // share HTTP client with other commands with the same cliOptions
+  protected val httpClient: HttpClient
+    get() = cliOptions.httpClient
+
   protected fun moduleKeyFactories(modulePathResolver: ModulePathResolver): List<ModuleKeyFactory> {
     return buildList {
       add(ModuleKeyFactories.standardLibrary)
@@ -195,6 +193,7 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
       .setStackFrameTransformer(stackFrameTransformer)
       .apply { project?.let { setProjectDependencies(it.dependencies) } }
       .setSecurityManager(securityManager)
+      .setHttpClient(httpClient)
       .setExternalProperties(externalProperties)
       .setEnvironmentVariables(environmentVariables)
       .addModuleKeyFactories(moduleKeyFactories(modulePathResolver))