Config order issue - lower-priority certificate data overrides a higher-priority certificate file

[GrosQuildu]

Lower-priority certificate data overrides a higher-priority certificate file

Severity: Low

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.

For certificate material, Config.configFromSysPropsOrEnvVars overlays the file and data properties independently. If a lower-priority kubeconfig supplies certificate-authority-data and a higher-priority system property supplies kubernetes.certs.ca.file, the final Config contains both values. The TLS path then calls CertUtils.getInputStreamFromDataOrFile, which always chooses the data value before the file value. The lower-priority kubeconfig CA data therefore overrides the higher-priority system-property CA file.

The same file/data priority shape exists for client certificate and client key material, so the fix should resolve source priority between paired *.file and *.data fields before those values reach certificate-loading helpers.

Exploit Scenario

A wrapper launches a Fabric8-based tool with -Dkubernetes.certs.ca.file=/trusted/ca.pem to pin a trusted API-server CA, but accepts a kubeconfig from a less-trusted source. The kubeconfig includes certificate-authority-data for an attacker-chosen CA. Fabric8 records both the trusted CA file and attacker CA data, then loads the data value first. A network attacker with a certificate from the attacker CA can impersonate the API server despite the higher-priority CA file.

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.CertUtils;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.io.TempDir;

import java.io.InputStream;
import java.nio.charset.StandardCharsets;
import java.nio.file.Files;
import java.nio.file.Path;

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

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

  @TempDir
  private Path temporaryFolder;

  @Test
  void higherPriorityCaFileLosesToLowerPriorityCaData() throws Exception {
    Path highFile = temporaryFolder.resolve("high-ca.txt");
    Files.writeString(highFile, "HIGH_CA", StandardCharsets.UTF_8);
    Path kubeconfig = temporaryFolder.resolve("lower-ca-data.yaml");
    Files.writeString(kubeconfig, String.join("\n",
        "apiVersion: v1",
        "kind: Config",
        "clusters:",
        "- name: c",
        "  cluster:",
        "    server: https://api.example",
        "    certificate-authority-data: TE9XX0NB",
        "contexts:",
        "- name: ctx",
        "  context:",
        "    cluster: c",
        "current-context: ctx",
        "users: []",
        ""),
        StandardCharsets.UTF_8);
    System.setProperty("kubeconfig", kubeconfig.toString());
    System.setProperty("kubernetes.certs.ca.file", highFile.toString());

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

    assertThat(config.getCaCertFile()).isEqualTo(highFile.toString());
    assertThat(config.getCaCertData()).isEqualTo("TE9XX0NB");
    try (InputStream selected = CertUtils.getInputStreamFromDataOrFile(
        config.getCaCertData(), config.getCaCertFile())) {
      assertThat(new String(selected.readAllBytes(), StandardCharsets.UTF_8))
          .isEqualTo("LOW_CA");
    }
  }
}

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.getCaCertData() is cleared and the selected input stream reads HIGH_CA. Updating the regression expectation to HIGH_CA passes.

Threat Model

This ordering is tricky, because it is about technically different fields. Maybe the CertUtils.getInputStreamFromDataOrFile resolving the entries as it does currently is the correct/expected behavior. If so, then patch below is not needed, but this is a gotcha worth documenting in the threat model.

Also requires careful consideration to not break users.

Fix

Resolve each file/data pair by source rank before storing it on Config. When one side comes from a higher-priority source, clear the lower-priority paired field so later certificate utilities cannot re-prioritize by representation.

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..37c2e79c51 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;
@@ -441,18 +442,15 @@ public class Config extends SundrioConfig {
     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()));
-    config.setClientCertFile(
-        Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_CERTIFICATE_FILE_SYSTEM_PROPERTY, config.getClientCertFile()));
-    config.setClientCertData(
-        Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_CERTIFICATE_DATA_SYSTEM_PROPERTY, config.getClientCertData()));
-    config.setClientKeyFile(
-        Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_KEY_FILE_SYSTEM_PROPERTY, config.getClientKeyFile()));
-    config.setClientKeyData(
-        Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_KEY_DATA_SYSTEM_PROPERTY, config.getClientKeyData()));
+    setRankedFileData(config::setCaCertFile, config::setCaCertData,
+        KUBERNETES_CA_CERTIFICATE_FILE_SYSTEM_PROPERTY, KUBERNETES_CA_CERTIFICATE_DATA_SYSTEM_PROPERTY,
+        config.getCaCertFile(), config.getCaCertData());
+    setRankedFileData(config::setClientCertFile, config::setClientCertData,
+        KUBERNETES_CLIENT_CERTIFICATE_FILE_SYSTEM_PROPERTY, KUBERNETES_CLIENT_CERTIFICATE_DATA_SYSTEM_PROPERTY,
+        config.getClientCertFile(), config.getClientCertData());
+    setRankedFileData(config::setClientKeyFile, config::setClientKeyData,
+        KUBERNETES_CLIENT_KEY_FILE_SYSTEM_PROPERTY, KUBERNETES_CLIENT_KEY_DATA_SYSTEM_PROPERTY,
+        config.getClientKeyFile(), config.getClientKeyData());
     config.setClientKeyAlgo(getKeyAlgorithm(config.getClientKeyFile(), config.getClientKeyData()));
     config.setClientKeyPassphrase(Utils.getSystemPropertyOrEnvVar(KUBERNETES_CLIENT_KEY_PASSPHRASE_SYSTEM_PROPERTY,
         config.getClientKeyPassphrase()));
@@ -564,6 +562,48 @@ 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 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.