fix: prevent inclusion of characters that could lead to FileVault path traversal (#219)

Closes: #211

Signed-off-by: Peter Zaoral <pzaoral@redhat.com>
Co-authored-by: Václav Muzikář <vmuzikar@redhat.com>

docs/documentation/server_admin/topics/vault.adoc

@@ -26,6 +26,7 @@ In the <<_ldap,LDAP settings>> of LDAP-based user federation.
 OIDC identity provider secret::
 In the _Client Secret_ inside identity provider <<_identity_broker_oidc,OpenID Connect Config>>
 
+[[_vault-key-resolvers]]
 === Key resolvers
 
 All built-in providers support the configuration of key resolvers. A key resolver implements the algorithm or strategy for combining the realm name with the key, obtained from the `${vault.key}` expression, into the final entry name used to retrieve the secret from the vault. {project_name} uses the `keyResolvers` property to configure the resolvers that the provider uses. The value is a comma-separated list of resolver names. An example of the configuration for the `files-plaintext` provider follows:
@@ -45,13 +46,13 @@ A list of the currently available resolvers follows:
 |Name |Description
 
 | KEY_ONLY
-| {project_name} ignores the realm name and uses the key from the vault expression.
+| {project_name} ignores the realm name and uses the key from the vault expression. {project_name} escapes occurrences of underscores in the key with another underscore character. For example, if the key is called `my_secret`, {project_name} searches for an entry in the vault named `my++__++secret`. This is to prevent conflicts with the default `REALM_UNDERSCORE_KEY` resolver.
 
 | REALM_UNDERSCORE_KEY
 | {project_name} combines the realm and key by using an underscore character. {project_name} escapes occurrences of underscores in the realm or key with another underscore character. For example, if the realm is called `master_realm` and the key is `smtp_key`, the combined key is `master+++__+++realm_smtp+++__+++key`.
 
 | REALM_FILESEPARATOR_KEY
-| {project_name} combines the realm and key by using the platform file separator character.
+| {project_name} combines the realm and key by using the platform file separator character. The vault expression prohibits the use of characters that could cause path traversal, thus preventing access to secrets outside the corresponding realm.
 
 ifeval::[{project_community}==true]
 | FACTORY_PROVIDED
@@ -60,4 +61,4 @@ endif::[]
 
 |===
 
-If you have not configured a resolver for the built-in providers, {project_name} selects the `REALM_UNDERSCORE_KEY`.
+If you have not configured a resolver for the built-in providers, {project_name} selects the `REALM_UNDERSCORE_KEY`.

docs/documentation/upgrading/topics/changes/changes-24_0_9.adoc

@@ -4,3 +4,9 @@ Potential vulnerable configurations have been identified in the X.509 client cer
 Additional configuration steps might be required depending on your current configuration. Make sure to review the updated
 link:{client_certificate_lookup_link}[reverse proxy guide] if you have configured
 the client certificate lookup via a proxy header.
+
+= Security improvements for the key resolvers
+
+While using the `REALM_FILESEPARATOR_KEY` key resolver, {project_name} now restricts access to FileVault secrets outside of its realm. Characters that could cause path traversal when specifying the expression placeholder in the Administration Console are now prohibited.
+
+Additionally, the `KEY_ONLY` key resolver now escapes the `+_+` character to prevent reading secrets that would otherwise be linked to another realm when the `REALM_UNDERSCORE_KEY` resolver is used. The escaping simply replaces `+_+` with `+__+`, so, for example, `${vault.my_secret}` now looks for a file named `my++__++secret`. We recognize that this is a breaking change; therefore, a warning is logged to ease the transition.

docs/guides/server/vault.adoc

@@ -43,19 +43,6 @@ Kubernetes/OpenShift Secrets are used on a per-realm basis in {project_name}, wh
 ${r"${vault.<realmname>_<secretname>}"}
 ----
 
-=== Using underscores in the Name
-To process the secret correctly, you double all underscores in the <realmname> or the <secretname>, separated by a single underscore.
-
-.Example
-* Realm Name: `sso_realm`
-* Desired Name: `ldap_credential`
-* Resulting file Name:
-[source, bash]
-----
-sso__realm_ldap__credential
-----
-Note the doubled underscores between __sso__ and __realm__ and also between __ldap__ and __credential__.
-
 == Configuring the Java KeyStore-based vault
 
 In order to use the Java KeyStore-based vault, you need to create a KeyStore file first. You can use the following command for doing so:
@@ -75,6 +62,21 @@ Note that the `--vault-type` parameter is optional and defaults to `PKCS12`.
 
 Secrets stored in the vault can then be accessed in a realm via the following placeholder (assuming using the `REALM_UNDERSCORE_KEY` key resolver): `${r"${vault.realm-name_alias}"}`.
 
+== Using underscores in the secret names
+To process the secret correctly, you double all underscores in the <secretname>. When `REALM_UNDERSCORE_KEY` key resolver is used, underscores in <realmname> are also doubled and <secretname> and <realmname> is separated by a single underscore.
+
+.Example
+* Realm Name: `sso_realm`
+* Desired Name: `ldap_credential`
+* Resulting file name:
+[source, bash]
+----
+sso__realm_ldap__credential
+----
+Note the doubled underscores between __sso__ and __realm__ and also between __ldap__ and __credential__.
+
+To learn more about key resolvers, see link:{adminguide_link}#_vault-key-resolvers[Key resolvers section in the Server Administration guide].
+
 == Example: Use an LDAP bind credential secret in the Admin Console
 
 .Example setup

services/src/main/java/org/keycloak/vault/AbstractVaultProvider.java

@@ -17,6 +17,10 @@
 
 package org.keycloak.vault;
 
+import org.jboss.logging.Logger;
+
+import java.io.File;
+import java.lang.invoke.MethodHandles;
 import java.util.List;
 import java.util.Optional;
 
@@ -38,6 +42,8 @@ import java.util.Optional;
  */
 public abstract class AbstractVaultProvider implements VaultProvider {
 
+    private static final Logger logger = Logger.getLogger(MethodHandles.lookup().lookupClass());
+
     protected final String realm;
     protected final List<VaultKeyResolver> resolvers;
 
@@ -56,14 +62,50 @@ public abstract class AbstractVaultProvider implements VaultProvider {
     @Override
     public VaultRawSecret obtainSecret(String vaultSecretId) {
         for (VaultKeyResolver resolver : this.resolvers) {
-            VaultRawSecret secret = this.obtainSecretInternal(resolver.apply(this.realm, vaultSecretId));
+            String resolvedKey = resolver.apply(this.realm, vaultSecretId);
+            if (!validate(resolver, vaultSecretId, resolvedKey)) {
+                logger.warnf("Validation failed for secret %s with resolved key %s", vaultSecretId, resolvedKey);
+                return DefaultVaultRawSecret.forBuffer(Optional.empty());
+            }
+        }
+
+        for (VaultKeyResolver resolver : this.resolvers) {
+            String resolvedKey = resolver.apply(this.realm, vaultSecretId);
+            VaultRawSecret secret = this.obtainSecretInternal(resolvedKey);
             if (secret != null && secret.get().isPresent()) {
                 return secret;
             }
+            checkForLegacyKey(resolver, vaultSecretId, resolvedKey);
         }
         return DefaultVaultRawSecret.forBuffer(Optional.empty());
     }
 
+    private void checkForLegacyKey(VaultKeyResolver resolver, String vaultSecretId, String resolvedKey) {
+        if (resolver == AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver() && vaultSecretId.contains("_")) {
+            String legacyKey = vaultSecretId.replaceAll("__", "_");
+            VaultRawSecret legacySecret = this.obtainSecretInternal(legacyKey);
+            if (legacySecret != null && legacySecret.get().isPresent()) {
+                logger.warnf("Secret was found using legacy key '%s'. Please rename the key to '%s' and repeat the action.", legacyKey, resolvedKey);
+            }
+        }
+    }
+
+    /**
+     * Validates the resolved key to ensure it meets the necessary criteria.
+     *
+     * @param resolver the {@link VaultKeyResolver} used to resolve the key.
+     * @param key the original key provided.
+     * @param resolvedKey the key after being resolved by the resolver.
+     * @return a boolean indicating whether the validation passed.
+     */
+    protected boolean validate(VaultKeyResolver resolver, String key, String resolvedKey) {
+        if (key.contains(File.separator)) {
+            logger.warnf("Key %s contains invalid file separator character", key);
+            return false;
+        }
+        return true;
+    }
+
     /**
      * Subclasses of {@code AbstractVaultProvider} must implement this method. It is meant to be implemented in the same
      * way as the {@link #obtainSecret(String)} method from the {@link VaultProvider} interface, but the specified vault

services/src/main/java/org/keycloak/vault/AbstractVaultProviderFactory.java

@@ -140,7 +140,7 @@ public abstract class AbstractVaultProviderFactory implements VaultProviderFacto
          * all realms to share the secrets, so instead of replicating entries for all existing realms in the vault one can
          * simply use key directly and all realms will obtain the same secret.
          */
-        KEY_ONLY((realm, key) -> key),
+        KEY_ONLY((realm, key) -> key.replaceAll("_", "__")),
 
         /**
          * The realm is prepended to the vault key and they are separated by an underscore ({@code '_'}) character. If either

services/src/main/java/org/keycloak/vault/FilesPlainTextVaultProvider.java

@@ -63,6 +63,24 @@ public class FilesPlainTextVaultProvider extends AbstractVaultProvider {
         }
     }
 
+    @Override
+    protected boolean validate(VaultKeyResolver resolver, String key, String resolvedKey) {
+        if (!super.validate(resolver, key, resolvedKey)) {
+            return false;
+        }
+        Path secretPath = vaultPath.resolve(resolvedKey);
+
+        Path expectedPath = vaultPath;
+        if (resolver == AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver()) {
+            expectedPath = expectedPath.resolve(realm);
+        }
+        if (!secretPath.getParent().equals(expectedPath)) {
+            logger.warnf("Path traversal attempt detected in secret %s.", key);
+            return false;
+        }
+        return true;
+    }
+
     @Override
     public void close() {
 

services/src/test/java/org/keycloak/vault/PlainTextVaultProviderTest.java

@@ -1,17 +1,29 @@
 package org.keycloak.vault;
 
 import org.junit.Test;
+import org.junit.Before;
+import org.junit.After;
 
 import java.nio.charset.StandardCharsets;
 import java.nio.file.Files;
 import java.nio.file.Path;
+import java.nio.file.Paths;
 import java.util.Arrays;
+import java.util.concurrent.BlockingQueue;
+import java.util.concurrent.LinkedBlockingQueue;
+import java.util.logging.Handler;
+import java.util.logging.Level;
+import java.util.logging.LogRecord;
+import java.util.logging.Logger;
+import java.io.ByteArrayOutputStream;
+import java.io.PrintStream;
 
 import static org.hamcrest.CoreMatchers.not;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNotNull;
 import static org.hamcrest.MatcherAssert.assertThat;
+import static org.junit.Assert.assertTrue;
 import static org.keycloak.vault.SecretContains.secretContains;
 
 /**
@@ -21,6 +33,38 @@ import static org.keycloak.vault.SecretContains.secretContains;
  */
 public class PlainTextVaultProviderTest {
 
+    private static final Logger logger = Logger.getLogger("org.keycloak.vault");
+    private BlockingQueue<String> logMessages;
+    private final ByteArrayOutputStream errContent = new ByteArrayOutputStream();
+    private final PrintStream originalErr = System.err;
+    private Handler logHandler;
+
+    @Before
+    public void setUp() {
+        logMessages = new LinkedBlockingQueue<>();
+        logger.setLevel(Level.WARNING);
+        logHandler = new Handler() {
+            @Override
+            public void publish(LogRecord record) {
+                logMessages.add(record.getMessage());
+            }
+
+            @Override
+            public void flush() { }
+
+            @Override
+            public void close() throws SecurityException { }
+        };
+        logger.addHandler(logHandler);
+        System.setErr(new PrintStream(errContent));
+    }
+
+    @After
+    public void tearDown() {
+        logger.removeHandler(logHandler);
+        System.setErr(originalErr);
+    }
+
     @Test
     public void shouldObtainSecret() throws Exception {
         //given
@@ -69,7 +113,7 @@ public class PlainTextVaultProviderTest {
     public void shouldOperateOnNonExistingVaultDirectory() throws Exception {
         //given
         FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(Scenario.NON_EXISTING.getPath(), "test",
-            Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_UNDERSCORE_KEY.getVaultKeyResolver()));
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_UNDERSCORE_KEY.getVaultKeyResolver()));
 
         //when
         VaultRawSecret secret = provider.obtainSecret("non-existing-key");
@@ -161,4 +205,96 @@ public class PlainTextVaultProviderTest {
         assertThat(secretAfterFirstRead, not(secretContains("secret")));
         assertThat(secretAfterSecondRead, secretContains("secret"));
     }
-}
+
+    @Test
+    public void shouldPreventPathFileSeparatorInVaultSecretId() {
+        // given
+        FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(
+                Scenario.EXISTING.getPath(),
+                "test",
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver())
+        );
+
+        // when
+        VaultRawSecret secret = provider.obtainSecret(".../key1");
+
+        // then
+        assertNotNull(secret);
+        assertFalse(secret.get().isPresent());
+        assertTrue(
+                logMessages.stream()
+                        .anyMatch(msg -> msg.contains("Key .../key1 contains invalid file separator character"))
+        );
+    }
+
+    @Test
+    public void shouldNotValidateWithInvalidPath() {
+        // given
+        Path vaultPath = Paths.get("/vault");
+        FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(vaultPath, "test_realm",
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver()));
+        VaultKeyResolver resolver = AbstractVaultProviderFactory.AvailableResolvers.REALM_FILESEPARATOR_KEY.getVaultKeyResolver();
+        String key = "key1";
+        String resolvedKey = "../key1";
+
+        // when
+        boolean isValid = provider.validate(resolver, key, resolvedKey);
+
+        // then
+        assertFalse(isValid);
+    }
+
+    @Test
+    public void shouldValidateWithDifferentResolver() {
+        // given
+        Path vaultPath = Paths.get("/vault");
+        FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(vaultPath, "test_realm",
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver()));
+        VaultKeyResolver resolver = AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver();
+        String key = "key1";
+        String resolvedKey = "key1";
+
+        // when
+        boolean isValid = provider.validate(resolver, key, resolvedKey);
+
+        // then
+        assertTrue(isValid);
+    }
+
+    @Test
+    public void shouldSearchForEscapedKeyOnlySecret() throws Exception {
+        // given
+        FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(Scenario.EXISTING.getPath(), "test",
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver()));
+
+        // when
+        VaultRawSecret secret = provider.obtainSecret("keyonly_escaped");
+
+        // then
+        assertNotNull(secret);
+        assertNotNull(secret.get().get());
+        assertThat(secret, secretContains("expected_secret_value"));
+    }
+
+    @Test
+    public void shouldSearchForKeyOnlyLegacy() throws Exception {
+        // given
+        FilesPlainTextVaultProvider provider = new FilesPlainTextVaultProvider(
+                Scenario.EXISTING.getPath(),
+                "test",
+                Arrays.asList(AbstractVaultProviderFactory.AvailableResolvers.KEY_ONLY.getVaultKeyResolver())
+        );
+
+        // when
+        VaultRawSecret secret = provider.obtainSecret("keyonly_legacy");
+
+        // then
+        assertNotNull(secret);
+        assertFalse(secret.get().isPresent());
+        assertTrue(
+                logMessages.stream()
+                        .anyMatch(msg -> msg.contains("Secret was found using legacy key 'keyonly_legacy'. Please rename the key to 'keyonly__legacy' and repeat the action."))
+        );
+    }
+
+}

services/src/test/resources/org/keycloak/vault/keyonly__escaped

@@ -0,0 +1 @@
+expected_secret_value

services/src/test/resources/org/keycloak/vault/keyonly_legacy

@@ -0,0 +1 @@
+should_not_be_retrieved