Do not enable TLS certificate revocation checks by default (apple#553)

This addresses an issue where network requests may fail if cert revocation checks error, which may occur due to availability issues, or due to lack of internet access. Revocation checking can still be enabled by setting JVM property com.sun.net.ssl.checkRevocation if on the JVM. Also: * Load built-in certs from resources, and move them to pkl-commons-cli * Fix an issue where HttpInitException is not caught when loading a module
luuvish · Jun 28, 2024 · 51d7265 · 51d7265
1 parent 420336d
commit 51d7265
Show file tree

Hide file tree

Showing 6 changed files with 36 additions and 77 deletions.
diff --git a/pkl-cli/pkl-cli.gradle.kts b/pkl-cli/pkl-cli.gradle.kts
@@ -1,6 +1,3 @@
-import java.security.KeyStore
-import java.security.cert.CertificateFactory
-
 plugins {
   pklAllProjects
   pklKotlinLibrary
@@ -38,8 +35,6 @@ val stagedLinuxAarch64Executable: Configuration by configurations.creating
 val stagedAlpineLinuxAmd64Executable: Configuration by configurations.creating
 val stagedWindowsAmd64Executable: Configuration by configurations.creating
 
-val certs: SourceSet by sourceSets.creating
-
 dependencies {
   compileOnly(libs.svm)
 
@@ -148,38 +143,11 @@ tasks.check {
   dependsOn(testStartJavaExecutable)
 }
 
-val trustStore = layout.buildDirectory.dir("generateTrustStore/PklCARoots.p12")
-val trustStorePassword = "password" // no sensitive data to protect
-
-// generate a trust store for Pkl's built-in CA certificates
-val generateTrustStore by tasks.registering {
-  inputs.file(certs.resources.singleFile)
-  outputs.file(trustStore)
-  doLast {
-    val certificates = certs.resources.singleFile.inputStream().use { stream ->
-      CertificateFactory.getInstance("X.509").generateCertificates(stream)
-    }
-    KeyStore.getInstance("PKCS12").apply {
-      load(null, trustStorePassword.toCharArray()) // initialize empty trust store
-      for ((index, certificate) in certificates.withIndex()) {
-        setCertificateEntry("cert-$index", certificate)
-      }
-      val trustStoreFile = trustStore.get().asFile
-      trustStoreFile.parentFile.mkdirs()
-      trustStoreFile.outputStream().use { stream ->
-        store(stream, trustStorePassword.toCharArray())
-      }
-    }
-  }
-}
-
 fun Exec.configureExecutable(
   graalVm: BuildInfo.GraalVm,
   outputFile: Provider<RegularFile>,
   extraArgs: List<String> = listOf()
 ) {
-  dependsOn(generateTrustStore)
-
   inputs.files(sourceSets.main.map { it.output })
     .withPropertyName("mainSourceSets")
     .withPathSensitivity(PathSensitivity.RELATIVE)
@@ -210,14 +178,10 @@ fun Exec.configureExecutable(
       // needed for messagepack-java (see https://github.com/msgpack/msgpack-java/issues/600)
       add("--initialize-at-run-time=org.msgpack.core.buffer.DirectBufferAccess")
       add("--no-fallback")
-      add("-Djavax.net.ssl.trustStore=${trustStore.get().asFile}")
-      add("-Djavax.net.ssl.trustStorePassword=$trustStorePassword")
-      add("-Djavax.net.ssl.trustStoreType=PKCS12")
-      // security property "ocsp.enable=true" is set in Main.kt
-      add("-Dcom.sun.net.ssl.checkRevocation=true")
       add("-H:IncludeResources=org/pkl/core/stdlib/.*\\.pkl")
       add("-H:IncludeResources=org/jline/utils/.*")
       add("-H:IncludeResourceBundles=org.pkl.core.errorMessages")
+      add("-H:IncludeResources=org/pkl/commons/cli/PklCARoots.pem")
       add("--macro:truffle")
       add("-H:Class=org.pkl.cli.Main")
       add("-H:Name=${outputFile.get().asFile.name}")

diff --git a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliCommand.kt
@@ -171,8 +171,20 @@ abstract class CliCommand(protected val cliOptions: CliBaseOptions) {
 
   private fun HttpClient.Builder.addDefaultCliCertificates() {
     val caCertsDir = IoUtils.getPklHomeDir().resolve("cacerts")
+    var certsAdded = false
     if (Files.isDirectory(caCertsDir)) {
-      Files.list(caCertsDir).filter { it.isRegularFile() }.forEach { addCertificates(it) }
+      Files.list(caCertsDir)
+        .filter { it.isRegularFile() }
+        .forEach { cert ->
+          certsAdded = true
+          addCertificates(cert)
+        }
+    }
+    if (!certsAdded) {
+      val defaultCerts =
+        javaClass.classLoader.getResourceAsStream("org/pkl/commons/cli/PklCARoots.pem")
+          ?: throw CliException("Could not find bundled certificates")
+      addCertificates(defaultCerts.readAllBytes())
     }
   }
 

diff --git a/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliMain.kt b/pkl-commons-cli/src/main/kotlin/org/pkl/commons/cli/CliMain.kt
@@ -16,7 +16,6 @@
 package org.pkl.commons.cli
 
 import java.io.PrintStream
-import java.security.Security
 import kotlin.system.exitProcess
 
 /** Building block for CLIs. Intended to be called from a `main` method. */
@@ -30,9 +29,6 @@ fun cliMain(block: () -> Unit) {
 
   // Force `native-image` to use system proxies (which does not happen with `-D`).
   System.setProperty("java.net.useSystemProxies", "true")
-  // enable OCSP for default SSL context
-  Security.setProperty("ocsp.enable", "true")
-
   try {
     block()
   } catch (e: CliTestException) {

diff --git a/pkl-cli/src/certs/resources/PklCARoots.pem → ...ources/org/pkl/commons/cli/PklCARoots.pem b/pkl-cli/src/certs/resources/PklCARoots.pem → ...ources/org/pkl/commons/cli/PklCARoots.pem
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
@@ -31,22 +31,17 @@
 import java.nio.file.NoSuchFileException;
 import java.nio.file.Path;
 import java.security.GeneralSecurityException;
+import java.security.KeyStore;
 import java.security.SecureRandom;
-import java.security.cert.CertPathBuilder;
+import java.security.cert.Certificate;
 import java.security.cert.CertificateException;
 import java.security.cert.CertificateFactory;
-import java.security.cert.PKIXBuilderParameters;
-import java.security.cert.PKIXRevocationChecker;
-import java.security.cert.TrustAnchor;
-import java.security.cert.X509CertSelector;
 import java.security.cert.X509Certificate;
 import java.time.Duration;
+import java.util.ArrayList;
 import java.util.Collection;
-import java.util.HashSet;
 import java.util.List;
-import java.util.Set;
 import javax.annotation.concurrent.ThreadSafe;
-import javax.net.ssl.CertPathTrustManagerParameters;
 import javax.net.ssl.SSLContext;
 import javax.net.ssl.SSLException;
 import javax.net.ssl.SSLHandshakeException;
@@ -130,43 +125,36 @@ private static SSLContext createSslContext(
       List<Path> certificateFiles, List<ByteBuffer> certificateBytes) {
     try {
       if (certificateFiles.isEmpty() && certificateBytes.isEmpty()) {
-        // use Pkl native executable's or JVM's built-in CA certificates
+        // use JVM's built-in CA certificates
         return SSLContext.getDefault();
       }
 
-      var certPathBuilder = CertPathBuilder.getInstance("PKIX");
-      // create a non-legacy revocation checker that is configured via setOptions() instead of
-      // security property "ocsp.enabled"
-      var revocationChecker = (PKIXRevocationChecker) certPathBuilder.getRevocationChecker();
-      revocationChecker.setOptions(Set.of()); // prefer OCSP, fall back to CRLs
-
       var certFactory = CertificateFactory.getInstance("X.509");
-      Set<TrustAnchor> trustAnchors =
-          createTrustAnchors(certFactory, certificateFiles, certificateBytes);
-      var pkixParameters = new PKIXBuilderParameters(trustAnchors, new X509CertSelector());
-      // equivalent of "com.sun.net.ssl.checkRevocation=true"
-      pkixParameters.setRevocationEnabled(true);
-      pkixParameters.addCertPathChecker(revocationChecker);
-
+      List<Certificate> certs = gatherCertificates(certFactory, certificateFiles, certificateBytes);
+      var keystore = KeyStore.getInstance(KeyStore.getDefaultType());
+      keystore.load(null);
+      for (var i = 0; i < certs.size(); i++) {
+        keystore.setCertificateEntry("Certificate" + i, certs.get(i));
+      }
       var trustManagerFactory = TrustManagerFactory.getInstance("PKIX");
-      trustManagerFactory.init(new CertPathTrustManagerParameters(pkixParameters));
+      trustManagerFactory.init(keystore);
 
       var sslContext = SSLContext.getInstance("TLS");
       sslContext.init(null, trustManagerFactory.getTrustManagers(), new SecureRandom());
 
       return sslContext;
-    } catch (GeneralSecurityException e) {
+    } catch (GeneralSecurityException | IOException e) {
       throw new HttpClientInitException(
           ErrorMessages.create("cannotInitHttpClient", Exceptions.getRootReason(e)), e);
     }
   }
 
-  private static Set<TrustAnchor> createTrustAnchors(
+  private static List<Certificate> gatherCertificates(
       CertificateFactory factory, List<Path> certificateFiles, List<ByteBuffer> certificateBytes) {
-    var anchors = new HashSet<TrustAnchor>();
+    var certificates = new ArrayList<Certificate>();
     for (var file : certificateFiles) {
       try (var stream = Files.newInputStream(file)) {
-        collectTrustAnchors(anchors, factory, stream, file);
+        collectCertificates(certificates, factory, stream, file);
       } catch (NoSuchFileException e) {
         throw new HttpClientInitException(ErrorMessages.create("cannotFindCertFile", file));
       } catch (IOException e) {
@@ -176,13 +164,13 @@ private static Set<TrustAnchor> createTrustAnchors(
     }
     for (var byteBuffer : certificateBytes) {
       var stream = new ByteArrayInputStream(byteBuffer.array());
-      collectTrustAnchors(anchors, factory, stream, "<unavailable>");
+      collectCertificates(certificates, factory, stream, "<unavailable>");
     }
-    return anchors;
+    return certificates;
   }
 
-  private static void collectTrustAnchors(
-      Collection<TrustAnchor> anchors,
+  private static void collectCertificates(
+      ArrayList<Certificate> anchors,
       CertificateFactory factory,
       InputStream stream,
       Object source) {
@@ -197,8 +185,6 @@ private static void collectTrustAnchors(
     if (certificates.isEmpty()) {
       throw new HttpClientInitException(ErrorMessages.create("emptyCertFile", source));
     }
-    for (var certificate : certificates) {
-      anchors.add(new TrustAnchor(certificate, null));
-    }
+    anchors.addAll(certificates);
   }
 }
diff --git a/pkl-core/src/main/java/org/pkl/core/runtime/ModuleCache.java b/pkl-core/src/main/java/org/pkl/core/runtime/ModuleCache.java
@@ -30,6 +30,7 @@
 import org.pkl.core.Release;
 import org.pkl.core.SecurityManager;
 import org.pkl.core.SecurityManagerException;
+import org.pkl.core.http.HttpClientInitException;
 import org.pkl.core.module.ModuleKey;
 import org.pkl.core.module.ModuleKeys;
 import org.pkl.core.module.ResolvedModuleKey;
@@ -191,7 +192,7 @@ private ResolvedModuleKey resolve(
       ModuleKey module, SecurityManager securityManager, @Nullable Node importNode) {
     try {
       return module.resolve(securityManager);
-    } catch (SecurityManagerException | PackageLoadError e) {
+    } catch (SecurityManagerException | PackageLoadError | HttpClientInitException e) {
       throw new VmExceptionBuilder().withOptionalLocation(importNode).withCause(e).build();
     } catch (FileNotFoundException | NoSuchFileException e) {
       var exceptionBuilder =