From 51d7265ec602aac25d1974c13502fc35b8d3c8fd Mon Sep 17 00:00:00 2001 From: Daniel Chao Date: Fri, 28 Jun 2024 08:57:46 -0700 Subject: [PATCH] Do not enable TLS certificate revocation checks by default (#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 --- pkl-cli/pkl-cli.gradle.kts | 38 +------------ .../kotlin/org/pkl/commons/cli/CliCommand.kt | 14 ++++- .../kotlin/org/pkl/commons/cli/CliMain.kt | 4 -- .../org/pkl/commons/cli}/PklCARoots.pem | 0 .../java/org/pkl/core/http/JdkHttpClient.java | 54 +++++++------------ .../org/pkl/core/runtime/ModuleCache.java | 3 +- 6 files changed, 36 insertions(+), 77 deletions(-) rename {pkl-cli/src/certs/resources => pkl-commons-cli/src/main/resources/org/pkl/commons/cli}/PklCARoots.pem (100%) diff --git a/pkl-cli/pkl-cli.gradle.kts b/pkl-cli/pkl-cli.gradle.kts index d148e9c7..d9d49511 100644 --- 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, extraArgs: List = 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 index d7fd72c7..77b7d55c 100644 --- 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 index 200b7414..eb83d17c 100644 --- 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 b/pkl-commons-cli/src/main/resources/org/pkl/commons/cli/PklCARoots.pem similarity index 100% rename from pkl-cli/src/certs/resources/PklCARoots.pem rename to pkl-commons-cli/src/main/resources/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 index 87bcda4f..caccd164 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 @@ -31,22 +31,17 @@ import java.nio.file.Files; 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 @@ final class JdkHttpClient implements HttpClient { List certificateFiles, List 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 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 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 createTrustAnchors( + private static List gatherCertificates( CertificateFactory factory, List certificateFiles, List certificateBytes) { - var anchors = new HashSet(); + var certificates = new ArrayList(); 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 @@ final class JdkHttpClient implements HttpClient { } for (var byteBuffer : certificateBytes) { var stream = new ByteArrayInputStream(byteBuffer.array()); - collectTrustAnchors(anchors, factory, stream, ""); + collectCertificates(certificates, factory, stream, ""); } - return anchors; + return certificates; } - private static void collectTrustAnchors( - Collection anchors, + private static void collectCertificates( + ArrayList anchors, CertificateFactory factory, InputStream stream, Object source) { @@ -197,8 +185,6 @@ final class JdkHttpClient implements HttpClient { 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 index f0add3f9..9c201a2d 100644 --- 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 java.util.stream.Collectors; 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 @@ public final class ModuleCache { 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 =