Config order issue - kubeconfig insecure-skip-tls-verify overrides higher-priority CA material

[GrosQuildu]

Description

The README says Fabric8 client auto-configuration should prefer sources in this order: system properties, environment variables, kube config file, then service account token and mounted CA certificate.

A kubeconfig cluster entry with insecure-skip-tls-verify: true sets Fabric8's trust-all TLS mode. If a caller supplies higher-priority CA material through -Dkubernetes.certs.ca.file=/trusted/ca.pem, Config.configFromSysPropsOrEnvVars sets the CA file but leaves the lower-priority trust-all flag enabled. Later, SSLUtils.trustManagers checks isTrustCerts before it checks CA data or CA files. The result is a trust-all TrustManager, and the higher-priority CA file is ignored.

Exploit Scenario

A Fabric8-based automation tool receives a kubeconfig from a project workspace or support bundle, while the operator pins a CA with a system property. The kubeconfig includes insecure-skip-tls-verify: true. Fabric8 keeps trust-all enabled after applying the system CA file, so a network attacker can present any server certificate and receive Kubernetes credentials or tamper with cluster traffic.

Add the following test as kubernetes-client-api/src/test/java/io/fabric8/kubernetes/client/ConfigOrderInvariantPoCTest.java:

package io.fabric8.kubernetes.client;

import io.fabric8.kubernetes.client.internal.SSLUtils;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;
import java.security.cert.X509Certificate;

import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatCode;

@RestoreSystemProperties({
    "kubeconfig",
    "kubernetes.certs.ca.file"
})
class ConfigOrderInvariantPoCTest {

  @TempDir
  private Path temporaryFolder;

  @Test
  void lowerPriorityInsecureSkipTlsVerifySuppressesHigherPriorityCaFile() throws Exception {
    Path caFile = temporaryFolder.resolve("operator-ca.crt");
    Files.writeString(caFile, "not used by the trust-all manager", StandardCharsets.UTF_8);
    Path kubeconfig = temporaryFolder.resolve("lower-trust-all.yaml");
    Files.writeString(kubeconfig, String.join("\n",
        "apiVersion: v1",
        "kind: Config",
        "clusters:",
        "- name: c",
        "  cluster:",
        "    server: https://api.example",
        "    insecure-skip-tls-verify: true",
        "contexts:",
        "- name: ctx",
        "  context:",
        "    cluster: c",
        "current-context: ctx",
        "users: []",
        ""),
        StandardCharsets.UTF_8);
    System.setProperty("kubeconfig", kubeconfig.toString());
    System.setProperty("kubernetes.certs.ca.file", caFile.toString());

    Config config = new ConfigBuilder().build();

    assertThat(config.getCaCertFile()).isEqualTo(caFile.toString());
    assertThat(config.isTrustCerts()).isTrue();
    TrustManager[] trustManagers = SSLUtils.trustManagers(config);
    assertThatCode(() -> ((X509TrustManager) trustManagers[0])
        .checkServerTrusted(new X509Certificate[0], "RSA"))
        .doesNotThrowAnyException();
  }
}

The PoC passed on an unpatched disposable clone:

./mvnw -pl kubernetes-client-api -Dtest=ConfigOrderInvariantPoCTest test
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0
BUILD SUCCESS

After applying the patch below, the unchanged PoC fails because config.isTrustCerts() is false and the trust manager is no longer the trust-all implementation. Updating the regression expectation to assert isTrustCerts() == false and a non-trust-all manager passes.

Threat Model

This fix is discussable - maybe lower priority config that explicitly disable TLS verification should be applied even in presence of higher-priority TLS configurations. If so, a threat model should mention that.

Fix

Track the source rank of the trust-all flags and CA material. If CA material comes from a higher-priority source than trustCerts or hostname-verification disablement, clear the lower-priority insecure flags before TLS setup.

diff --git a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
index 1f71105aa0..7fe980d12a 100644
--- a/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
+++ b/kubernetes-client-api/src/main/java/io/fabric8/kubernetes/client/Config.java
@@ -48,6 +48,7 @@ import java.util.List;
 import java.util.Locale;
 import java.util.Map;
 import java.util.Optional;
+import java.util.function.Consumer;
 import java.util.function.Predicate;
 import java.util.function.UnaryOperator;
 import java.util.stream.Collectors;
@@ -435,16 +436,23 @@ public class Config extends SundrioConfig {
   }

   public static void configFromSysPropsOrEnvVars(Config config) {
-    config.setTrustCerts(Utils.getSystemPropertyOrEnvVar(KUBERNETES_TRUST_CERT_SYSTEM_PROPERTY, config.isTrustCerts()));
-    config.setDisableHostnameVerification(Utils.getSystemPropertyOrEnvVar(
-        KUBERNETES_DISABLE_HOSTNAME_VERIFICATION_SYSTEM_PROPERTY, config.isDisableHostnameVerification()));
+    ConfigValue trustCerts = booleanConfigValue(KUBERNETES_TRUST_CERT_SYSTEM_PROPERTY, config.isTrustCerts());
+    ConfigValue disableHostnameVerification = booleanConfigValue(KUBERNETES_DISABLE_HOSTNAME_VERIFICATION_SYSTEM_PROPERTY,
+        config.isDisableHostnameVerification());
+    config.setTrustCerts(Boolean.parseBoolean(trustCerts.value));
+    config.setDisableHostnameVerification(Boolean.parseBoolean(disableHostnameVerification.value));
     config.setMasterUrl(Utils.getSystemPropertyOrEnvVar(KUBERNETES_MASTER_SYSTEM_PROPERTY, config.getMasterUrl()));
     config.setApiVersion(Utils.getSystemPropertyOrEnvVar(KUBERNETES_API_VERSION_SYSTEM_PROPERTY, config.getApiVersion()));
     config.setNamespace(Utils.getSystemPropertyOrEnvVar(KUBERNETES_NAMESPACE_SYSTEM_PROPERTY, config.getNamespace()));
-    config
-        .setCaCertFile(Utils.getSystemPropertyOrEnvVar(KUBERNETES_CA_CERTIFICATE_FILE_SYSTEM_PROPERTY, config.getCaCertFile()));
-    config
-        .setCaCertData(Utils.getSystemPropertyOrEnvVar(KUBERNETES_CA_CERTIFICATE_DATA_SYSTEM_PROPERTY, config.getCaCertData()));
+    ConfigValue caCert = setRankedFileData(config::setCaCertFile, config::setCaCertData,
+        KUBERNETES_CA_CERTIFICATE_FILE_SYSTEM_PROPERTY, KUBERNETES_CA_CERTIFICATE_DATA_SYSTEM_PROPERTY,
+        config.getCaCertFile(), config.getCaCertData());
+    if (caCert.rank > trustCerts.rank) {
+      config.setTrustCerts(false);
+    }
+    if (caCert.rank > disableHostnameVerification.rank) {
+      config.setDisableHostnameVerification(false);
+    }
     config.setClientCertFile(
         Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_CERTIFICATE_FILE_SYSTEM_PROPERTY, config.getClientCertFile()));
     config.setClientCertData(
@@ -564,6 +572,59 @@ public class Config extends SundrioConfig {
     }
   }

+  private static ConfigValue setRankedFileData(
+      Consumer<String> setFile, Consumer<String> setData,
+      String fileKey, String dataKey,
+      String existingFile, String existingData) {
+    ConfigValue file = systemEnvOrExisting(fileKey, existingFile);
+    ConfigValue data = systemEnvOrExisting(dataKey, existingData);
+    if (file.rank > data.rank) {
+      setFile.accept(file.value);
+      setData.accept(null);
+      return file;
+    }
+    if (data.rank > file.rank) {
+      setData.accept(data.value);
+      setFile.accept(null);
+      return data;
+    }
+    setFile.accept(file.value);
+    setData.accept(data.value);
+    return file.rank == 0 ? data : file;
+  }
+
+  private static ConfigValue systemEnvOrExisting(String key, String existing) {
+    String value = System.getProperty(key);
+    if (Utils.isNotNullOrEmpty(value)) {
+      return new ConfigValue(value, 3);
+    }
+    value = System.getenv(Utils.convertSystemPropertyNameToEnvVar(key));
+    if (Utils.isNotNullOrEmpty(value)) {
+      return new ConfigValue(value, 2);
+    }
+    return new ConfigValue(existing, Utils.isNotNullOrEmpty(existing) ? 1 : 0);
+  }
+
+  private static ConfigValue booleanConfigValue(String key, boolean existing) {
+    String value = System.getProperty(key);
+    if (Utils.isNotNullOrEmpty(value)) {
+      return new ConfigValue(value, 3);
+    }
+    value = System.getenv(Utils.convertSystemPropertyNameToEnvVar(key));
+    if (Utils.isNotNullOrEmpty(value)) {
+      return new ConfigValue(value, 2);
+    }
+    return new ConfigValue(String.valueOf(existing), existing ? 1 : 0);
+  }
+
+  private static final class ConfigValue {
+    private final String value;
+    private final int rank;
+
+    private ConfigValue(String value, int rank) {
+      this.value = value;
+      this.rank = rank;
+    }
+  }
+

---

Paweł Płatek from Trail of Bits in collaboration with OpenAI.