Restrict access to environment variables when at the server runtime

Signed-off-by: Pedro Igor <pigor.craveiro@gmail.com>

Author: pedroigor

adapters/saml/core/src/main/java/org/keycloak/adapters/saml/config/parsers/KeyParser.java

@@ -19,6 +19,7 @@
 
 import org.keycloak.adapters.saml.config.Key;
 import org.keycloak.common.util.StringPropertyReplacer;
+import org.keycloak.common.util.SystemEnvProperties;
 import org.keycloak.saml.common.exceptions.ParsingException;
 import org.keycloak.saml.common.util.StaxParserUtil;
 
@@ -60,20 +61,24 @@ protected void processSubElement(XMLEventReader xmlEventReader, Key target, Keyc
             case CERTIFICATE_PEM:
                 StaxParserUtil.advance(xmlEventReader);
                 value = StaxParserUtil.getElementText(xmlEventReader);
-                target.setCertificatePem(StringPropertyReplacer.replaceProperties(value));
+                target.setCertificatePem(replaceProperties(value));
                 break;
 
             case PUBLIC_KEY_PEM:
                 StaxParserUtil.advance(xmlEventReader);
                 value = StaxParserUtil.getElementText(xmlEventReader);
-                target.setPublicKeyPem(StringPropertyReplacer.replaceProperties(value));
+                target.setPublicKeyPem(replaceProperties(value));
                 break;
 
             case PRIVATE_KEY_PEM:
                 StaxParserUtil.advance(xmlEventReader);
                 value = StaxParserUtil.getElementText(xmlEventReader);
-                target.setPrivateKeyPem(StringPropertyReplacer.replaceProperties(value));
+                target.setPrivateKeyPem(replaceProperties(value));
                 break;
         }
     }
+
+    private String replaceProperties(String value) {
+        return StringPropertyReplacer.replaceProperties(value, SystemEnvProperties.UNFILTERED::getProperty);
+    }
 }

authz/client/src/main/java/org/keycloak/authorization/client/AuthzClient.java

@@ -21,7 +21,6 @@
 
 import java.io.IOException;
 import java.io.InputStream;
-import java.util.Objects;
 
 import com.fasterxml.jackson.annotation.JsonInclude;
 import com.fasterxml.jackson.databind.ObjectMapper;
@@ -33,7 +32,6 @@
 import org.keycloak.common.crypto.CryptoIntegration;
 import org.keycloak.common.util.KeycloakUriBuilder;
 import org.keycloak.representations.AccessTokenResponse;
-import org.keycloak.util.SystemPropertiesJsonParserFactory;
 
 /**
  * <p>This is class serves as an entry point for clients looking for access to Keycloak Authorization Services.

core/src/main/java/org/keycloak/util/SystemPropertiesJsonParserFactory.java authz/client/src/main/java/org/keycloak/authorization/client/SystemPropertiesJsonParserFactory.java

@@ -15,7 +15,11 @@
  * limitations under the License.
  */
 
-package org.keycloak.util;
+package org.keycloak.authorization.client;
+
+import java.io.IOException;
+import java.io.InputStream;
+import java.io.Reader;
 
 import com.fasterxml.jackson.core.JsonParser;
 import com.fasterxml.jackson.core.io.IOContext;
@@ -24,19 +28,12 @@
 import org.keycloak.common.util.StringPropertyReplacer;
 import org.keycloak.common.util.SystemEnvProperties;
 
-import java.io.IOException;
-import java.io.InputStream;
-import java.io.Reader;
-import java.util.Properties;
-
 /**
  * Provides replacing of system properties for parsed values
  *
  * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
  */
-public class SystemPropertiesJsonParserFactory extends MappingJsonFactory {
-
-    private static final Properties properties = new SystemEnvProperties();
+class SystemPropertiesJsonParserFactory extends MappingJsonFactory {
 
     @Override
     protected JsonParser _createParser(InputStream in, IOContext ctxt) throws IOException {
@@ -71,7 +68,7 @@ public SystemPropertiesAwareJsonParser(JsonParser d) {
         @Override
         public String getText() throws IOException {
             String orig = super.getText();
-            return StringPropertyReplacer.replaceProperties(orig, properties);
+            return StringPropertyReplacer.replaceProperties(orig, SystemEnvProperties.UNFILTERED::getProperty);
         }
     }
 }

authz/client/src/test/java/org/keycloak/authorization/client/JsonParserTest.java

@@ -0,0 +1,57 @@
+/*
+ * Copyright 2024 Red Hat, Inc. and/or its affiliates
+ * and other contributors as indicated by the @author tags.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.keycloak.authorization.client;
+
+import java.io.IOException;
+import java.io.InputStream;
+
+import com.fasterxml.jackson.databind.ObjectMapper;
+import org.junit.Assert;
+import org.junit.Test;
+import org.keycloak.representations.adapters.config.AdapterConfig;
+
+/**
+ * @author <a href="mailto:mposolda@redhat.com">Marek Posolda</a>
+ */
+public class JsonParserTest {
+
+    @Test
+    public void testParsingSystemProps() throws IOException {
+        System.setProperty("my.host", "foo");
+        System.setProperty("con.pool.size", "200");
+        System.setProperty("allow.any.hostname", "true");
+        System.setProperty("socket.timeout.millis", "6000");
+        System.setProperty("connection.timeout.millis", "7000");
+        System.setProperty("connection.ttl.millis", "500");
+
+        InputStream is = getClass().getClassLoader().getResourceAsStream("keycloak.json");
+
+        ObjectMapper mapper = new ObjectMapper(new SystemPropertiesJsonParserFactory());
+        AdapterConfig config = mapper.readValue(is, AdapterConfig.class);
+        Assert.assertEquals("http://foo:8080/auth", config.getAuthServerUrl());
+        Assert.assertEquals("external", config.getSslRequired());
+        Assert.assertEquals("angular-product${non.existing}", config.getResource());
+        Assert.assertTrue(config.isPublicClient());
+        Assert.assertTrue(config.isAllowAnyHostname());
+        Assert.assertEquals(100, config.getCorsMaxAge());
+        Assert.assertEquals(200, config.getConnectionPoolSize());
+        Assert.assertEquals(6000L, config.getSocketTimeout());
+        Assert.assertEquals(7000L, config.getConnectionTimeout());
+        Assert.assertEquals(500L, config.getConnectionTTL());
+    }
+}

authz/client/src/test/resources/keycloak.json

@@ -0,0 +1,12 @@
+{
+  "auth-server-url" : "http://${my.host}:8080/auth",
+  "ssl-required" : "external",
+  "resource" : "angular-product${non.existing}",
+  "public-client" : true,
+  "allow-any-hostname": "${allow.any.hostname}",
+  "cors-max-age": 100,
+  "connection-pool-size": "${con.pool.size}",
+  "socket-timeout-millis": "${socket.timeout.millis}",
+  "connection-timeout-millis": "${connection.timeout.millis}",
+  "connection-ttl-millis": "${connection.ttl.millis}"
+}

common/src/main/java/org/keycloak/common/util/StringPropertyReplacer.java

@@ -17,22 +17,20 @@
 package org.keycloak.common.util;
 
 import java.io.File;
-import java.util.Properties;
+import java.util.Optional;
 
 /**
- * A utility class for replacing properties in strings. 
+ * A utility class for replacing properties in strings.
  *
  * @author <a href="mailto:jason@planet57.com">Jason Dillon</a>
  * @author <a href="Scott.Stark@jboss.org">Scott Stark</a>
  * @author <a href="claudio.vesco@previnet.it">Claudio Vesco</a>
  * @author <a href="mailto:adrian@jboss.com">Adrian Brock</a>
  * @author <a href="mailto:dimitris@jboss.org">Dimitris Andreadis</a>
- * @version <tt>$Revision: 2898 $</tt> 
+ * @version <tt>$Revision: 2898 $</tt>
  */
 public final class StringPropertyReplacer
 {
-    /** New line string constant */
-    public static final String NEWLINE = System.getProperty("line.separator", "\n");
 
     /** File separator value */
     private static final String FILE_SEPARATOR = File.separator;
@@ -51,7 +49,12 @@ public final class StringPropertyReplacer
     private static final int SEEN_DOLLAR = 1;
     private static final int IN_BRACKET = 2;
 
-    private static final Properties systemEnvProperties = new SystemEnvProperties();
+    private static final PropertyResolver NULL_RESOLVER = property -> null;
+    private static PropertyResolver DEFAULT_PROPERTY_RESOLVER;
+
+    public static void setDefaultPropertyResolver(PropertyResolver systemVariables) {
+        DEFAULT_PROPERTY_RESOLVER = systemVariables;
+    }
 
     /**
      * Go through the input string and replace any occurrence of ${p} with
@@ -72,14 +75,13 @@ public final class StringPropertyReplacer
      * @return the input string with all property references replaced if any.
      *    If there are no valid references the input string will be returned.
      */
-    public static String replaceProperties(final String string)
-    {
-        return replaceProperties(string, (Properties) null);
+    public static String replaceProperties(final String string) {
+        return replaceProperties(string, getDefaultPropertyResolver());
     }
 
     /**
      * Go through the input string and replace any occurrence of ${p} with
-     * the props.getProperty(p) value. If there is no such property p defined,
+     * the value resolves from {@code resolver}. If there is no such property p defined,
      * then the ${p} reference will remain unchanged.
      *
      * If the property reference is of the form ${p:v} and there is no such property p,
@@ -93,17 +95,10 @@ public static String replaceProperties(final String string)
      * value and the property ${:} is replaced with System.getProperty("path.separator").
      *
      * @param string - the string with possible ${} references
-     * @param props - the source for ${x} property ref values, null means use System.getProperty()
+     * @param resolver - the property resolver
      * @return the input string with all property references replaced if any.
      *    If there are no valid references the input string will be returned.
      */
-    public static String replaceProperties(final String string, final Properties props) {
-        if (props == null) {
-            return replaceProperties(string, (PropertyResolver) null);
-        }
-        return replaceProperties(string, props::getProperty);
-    }
-
     public static String replaceProperties(final String string, PropertyResolver resolver)
     {
         if(string == null) {
@@ -171,10 +166,7 @@ else if (PATH_SEPARATOR_ALIAS.equals(key))
                     else
                     {
                         // check from the properties
-                        if (resolver != null)
-                            value = resolver.resolve(key);
-                        else
-                            value = systemEnvProperties.getProperty(key);
+                        value = resolveValue(resolver, key);
 
                         if (value == null)
                         {
@@ -183,10 +175,7 @@ else if (PATH_SEPARATOR_ALIAS.equals(key))
                             if (colon > 0)
                             {
                                 String realKey = key.substring(0, colon);
-                                if (resolver != null)
-                                    value = resolver.resolve(realKey);
-                                else
-                                    value = systemEnvProperties.getProperty(realKey);
+                                value = resolveValue(resolver, realKey);
 
                                 if (value == null)
                                 {
@@ -239,7 +228,7 @@ else if (PATH_SEPARATOR_ALIAS.equals(key))
                 throw new IllegalStateException("Infinite recursion happening when replacing properties on '" + buffer + "'");
             }
         }
-        
+
         // Done
         return buffer.toString();
     }
@@ -257,26 +246,32 @@ private static String resolveCompositeKey(String key, PropertyResolver resolver)
             {
                 // Check the first part
                 String key1 = key.substring(0, comma);
-                if (resolver != null)
-                    value = resolver.resolve(key1);
-                else
-                    value = systemEnvProperties.getProperty(key1);
+                value = resolveValue(resolver, key1);
             }
             // Check the second part, if there is one and first lookup failed
             if (value == null && comma < key.length() - 1)
             {
                 String key2 = key.substring(comma + 1);
-                if (resolver != null)
-                    value = resolver.resolve(key2);
-                else
-                    value = systemEnvProperties.getProperty(key2);
+                value = resolveValue(resolver, key2);
             }
         }
         // Return whatever we've found or null
         return value;
     }
-    
+
     public interface PropertyResolver {
         String resolve(String property);
     }
+
+    private static String resolveValue(PropertyResolver resolver, String key) {
+        if (resolver == null) {
+            return getDefaultPropertyResolver().resolve(key);
+        }
+
+        return resolver.resolve(key);
+    }
+
+    private static PropertyResolver getDefaultPropertyResolver() {
+        return Optional.ofNullable(DEFAULT_PROPERTY_RESOLVER).orElse(NULL_RESOLVER);
+    }
 }

common/src/main/java/org/keycloak/common/util/SystemEnvProperties.java

@@ -17,19 +17,53 @@
 
 package org.keycloak.common.util;
 
+import java.util.Collections;
+import java.util.Optional;
 import java.util.Properties;
+import java.util.Set;
 
 /**
+ * <p>An utility class to resolve the value of a key based on the environment variables
+ * and system properties available at runtime. In most cases, you do not want to resolve whatever system variable is available at runtime but specify which ones
+ * can be used when resolving placeholders.
+ *
+ * <p>To resolve to an environment variable, the key must have a format like {@code env.<key>} where {@code key} is the name of an environment variable.
+ * For system properties, there is no specific format and the value is resolved from a system property that matches the key.
+ *
  * @author <a href="mailto:sthorger@redhat.com">Stian Thorgersen</a>
  */
 public class SystemEnvProperties extends Properties {
 
+    /**
+     * <p>An variation of {@link SystemEnvProperties} that gives unrestricted access to any system variable available at runtime.
+     * Most of the time you don't want to use this class but favor creating a {@link SystemEnvProperties} instance that
+     * filters which system variables should be available at runtime.
+     */
+    public static final SystemEnvProperties UNFILTERED = new SystemEnvProperties(Collections.emptySet()) {
+        @Override
+        protected boolean isAllowed(String key) {
+            return true;
+        }
+    };
+
+    private final Set<String> allowedSystemVariables;
+
+    /**
+     * Creates a new instance where system variables where only specific keys can be resolved from system variables.
+     *
+     * @param allowedSystemVariables the keys of system variables that should be available at runtime
+     */
+    public SystemEnvProperties(Set<String> allowedSystemVariables) {
+        this.allowedSystemVariables = Optional.ofNullable(allowedSystemVariables).orElse(Collections.emptySet());
+    }
+
     @Override
     public String getProperty(String key) {
         if (key.startsWith("env.")) {
-            return System.getenv().get(key.substring(4));
+            String envKey = key.substring(4);
+            return isAllowed(envKey) ? System.getenv().get(envKey) : null;
         } else {
-            return System.getProperty(key);
+            return isAllowed(key) ? System.getProperty(key) : null;
         }
     }
 
@@ -39,4 +73,7 @@ public String getProperty(String key, String defaultValue) {
         return value != null ? value : defaultValue;
     }
 
+    protected boolean isAllowed(String key) {
+        return allowedSystemVariables.contains(key);
+    }
 }