address review comments

richard-dennehy · richard-dennehy · commit f66291dd7c11 · 2025-04-29T11:22:04.000+01:00
diff --git a/.idea/inspectionProfiles/Project_Default.xml b/.idea/inspectionProfiles/Project_Default.xml
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/jwt/JwtRealmSettings.java
@@ -16,6 +16,7 @@
 import org.elasticsearch.xpack.core.security.authc.RealmSettings;
 import org.elasticsearch.xpack.core.security.authc.support.ClaimSetting;
 import org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings;
+import org.elasticsearch.xpack.core.security.authc.support.SecuritySettingsUtil;
 import org.elasticsearch.xpack.core.ssl.SSLConfigurationSettings;
 
 import java.util.Collection;
@@ -30,6 +31,7 @@
 import java.util.stream.Stream;
 
 import static org.elasticsearch.xpack.core.security.authc.support.SecuritySettingsUtil.verifyNonNullNotEmpty;
+import static org.elasticsearch.xpack.core.security.authc.support.SecuritySettingsUtil.verifyProxySettings;
 
 /**
  * Settings unique to each JWT realm.
@@ -496,32 +498,7 @@ public void validate(String value) {
 
             @Override
             public void validate(String value, Map<Setting<?>, Object> settings) {
-                final String namespace = HTTP_PROXY_HOST.getNamespace(HTTP_PROXY_HOST.getConcreteSetting(key));
-                final Setting<Integer> portSetting = HTTP_PROXY_PORT.getConcreteSettingForNamespace(namespace);
-                final Integer port = (Integer) settings.get(portSetting);
-                final Setting<String> schemeSetting = HTTP_PROXY_SCHEME.getConcreteSettingForNamespace(namespace);
-                final String scheme = (String) settings.get(schemeSetting);
-                try {
-                    new HttpHost(value, port, scheme);
-                } catch (Exception e) {
-                    throw new IllegalArgumentException(
-                        "HTTP host for hostname ["
-                            + value
-                            + "] (from ["
-                            + key
-                            + "]),"
-                            + " port ["
-                            + port
-                            + "] (from ["
-                            + portSetting.getKey()
-                            + "]) and "
-                            + "scheme ["
-                            + scheme
-                            + "] (from (["
-                            + schemeSetting.getKey()
-                            + "]) is invalid"
-                    );
-                }
+                verifyProxySettings(key, value, settings, HTTP_PROXY_HOST, HTTP_PROXY_SCHEME, HTTP_PROXY_PORT);
             }
 
             @Override
@@ -544,11 +521,13 @@ public Iterator<Setting<?>> settings() {
     public static final Setting.AffixSetting<String> HTTP_PROXY_SCHEME = Setting.affixKeySetting(
         RealmSettings.realmSettingPrefix(TYPE),
         "http.proxy.scheme",
-        key -> Setting.simpleString(key, "http", value -> {
-            if (value.equals("http") == false && value.equals("https") == false) {
-                throw new IllegalArgumentException("Invalid value [" + value + "] for [" + key + "]. Only `http` or `https` are allowed.");
-            }
-        }, Setting.Property.NodeScope)
+        key -> Setting.simpleString(
+            key,
+            "http",
+            // TODO allow HTTPS once https://github.com/elastic/elasticsearch/issues/100264 is fixed
+            value -> verifyNonNullNotEmpty(key, value, List.of("http")),
+            Setting.Property.NodeScope
+        )
     );
 
     // SSL Configuration settings
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/oidc/OpenIdConnectRealmSettings.java
@@ -14,6 +14,7 @@
 import org.elasticsearch.xpack.core.security.authc.RealmSettings;
 import org.elasticsearch.xpack.core.security.authc.support.ClaimSetting;
 import org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings;
+import org.elasticsearch.xpack.core.security.authc.support.SecuritySettingsUtil;
 import org.elasticsearch.xpack.core.ssl.SSLConfigurationSettings;
 
 import java.net.URI;
@@ -234,32 +235,7 @@ public void validate(String value) {
 
             @Override
             public void validate(String value, Map<Setting<?>, Object> settings) {
-                final String namespace = HTTP_PROXY_HOST.getNamespace(HTTP_PROXY_HOST.getConcreteSetting(key));
-                final Setting<Integer> portSetting = HTTP_PROXY_PORT.getConcreteSettingForNamespace(namespace);
-                final Integer port = (Integer) settings.get(portSetting);
-                final Setting<String> schemeSetting = HTTP_PROXY_SCHEME.getConcreteSettingForNamespace(namespace);
-                final String scheme = (String) settings.get(schemeSetting);
-                try {
-                    new HttpHost(value, port, scheme);
-                } catch (Exception e) {
-                    throw new IllegalArgumentException(
-                        "HTTP host for hostname ["
-                            + value
-                            + "] (from ["
-                            + key
-                            + "]),"
-                            + " port ["
-                            + port
-                            + "] (from ["
-                            + portSetting.getKey()
-                            + "]) and "
-                            + "scheme ["
-                            + scheme
-                            + "] (from (["
-                            + schemeSetting.getKey()
-                            + "]) is invalid"
-                    );
-                }
+                SecuritySettingsUtil.verifyProxySettings(key, value, settings, HTTP_PROXY_HOST, HTTP_PROXY_SCHEME, HTTP_PROXY_PORT);
             }
 
             @Override
diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/SecuritySettingsUtil.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/support/SecuritySettingsUtil.java
@@ -7,8 +7,12 @@
 
 package org.elasticsearch.xpack.core.security.authc.support;
 
+import org.apache.http.HttpHost;
+import org.elasticsearch.common.settings.Setting;
+
 import java.util.Collection;
 import java.util.List;
+import java.util.Map;
 
 /**
  * Utilities for validating security settings.
@@ -85,6 +89,45 @@ public static void verifyNonNullNotEmpty(
         }
     }
 
+    public static void verifyProxySettings(
+        String key,
+        String hostValue,
+        Map<Setting<?>, Object> settings,
+        Setting.AffixSetting<String> hostKey,
+        Setting.AffixSetting<String> schemeKey,
+        Setting.AffixSetting<Integer> portKey
+    ) {
+        final String namespace = hostKey.getNamespace(hostKey.getConcreteSetting(key));
+
+        final Setting<Integer> portSetting = portKey.getConcreteSettingForNamespace(namespace);
+        final Integer port = (Integer) settings.get(portSetting);
+
+        final Setting<String> schemeSetting = schemeKey.getConcreteSettingForNamespace(namespace);
+        final String scheme = (String) settings.get(schemeSetting);
+
+        try {
+            new HttpHost(hostValue, port, scheme);
+        } catch (Exception e) {
+            throw new IllegalArgumentException(
+                "HTTP host for hostname ["
+                    + hostValue
+                    + "] (from ["
+                    + key
+                    + "]),"
+                    + " port ["
+                    + port
+                    + "] (from ["
+                    + portSetting.getKey()
+                    + "]) and "
+                    + "scheme ["
+                    + scheme
+                    + "] (from (["
+                    + schemeSetting.getKey()
+                    + "]) is invalid"
+            );
+        }
+    }
+
     private SecuritySettingsUtil() {
         throw new IllegalAccessError("not allowed!");
     }
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealmSettingsTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtRealmSettingsTests.java
@@ -23,11 +23,14 @@
 import java.util.Locale;
 
 import static org.elasticsearch.common.Strings.capitalize;
+import static org.hamcrest.Matchers.allOf;
 import static org.hamcrest.Matchers.containsInAnyOrder;
 import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.emptyIterable;
+import static org.hamcrest.Matchers.endsWith;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
+import static org.hamcrest.Matchers.startsWith;
 
 /**
  * JWT realm settings unit tests. These are low-level tests against ES settings parsers.
@@ -588,4 +591,59 @@ public void testRequiredClaimsCannotBeEmpty() {
 
         assertThat(e.getMessage(), containsString("required claim [" + fullSettingKey + "] cannot be empty"));
     }
+
+    public void testInvalidProxySchemeThrowsError() {
+        final String scheme = randomBoolean() ? "https" : randomAlphaOfLengthBetween(3, 8);
+        final String realmName = randomAlphaOfLengthBetween(3, 8);
+        final String proxySchemeSettingKey = RealmSettings.getFullSettingKey(realmName, JwtRealmSettings.HTTP_PROXY_SCHEME);
+        final Settings settings = Settings.builder().put(proxySchemeSettingKey, scheme).build();
+
+        final RealmConfig realmConfig = buildRealmConfig(JwtRealmSettings.TYPE, realmName, settings, randomInt());
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> realmConfig.getSetting(JwtRealmSettings.HTTP_PROXY_SCHEME)
+        );
+
+        assertThat(
+            e.getMessage(),
+            equalTo(Strings.format("Invalid value [https] for [%s]. Allowed values are [http].", proxySchemeSettingKey))
+        );
+    }
+
+    public void testInvalidProxyHostThrowsError() {
+        final int proxyPort = randomIntBetween(1, 65535);
+        final String realmName = randomAlphaOfLengthBetween(3, 8);
+        final String proxyPortSettingKey = RealmSettings.getFullSettingKey(realmName, JwtRealmSettings.HTTP_PROXY_PORT);
+        final String proxyHostSettingKey = RealmSettings.getFullSettingKey(realmName, JwtRealmSettings.HTTP_PROXY_HOST);
+        final Settings settings = Settings.builder().put(proxyHostSettingKey, "not a url").put(proxyPortSettingKey, proxyPort).build();
+
+        final RealmConfig realmConfig = buildRealmConfig(JwtRealmSettings.TYPE, realmName, settings, randomInt());
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> realmConfig.getSetting(JwtRealmSettings.HTTP_PROXY_HOST)
+        );
+
+        assertThat(
+            e.getMessage(),
+            allOf(startsWith(Strings.format("HTTP host for hostname [not a url] (from [%s])", proxyHostSettingKey)), endsWith("is invalid"))
+        );
+    }
+
+    public void testInvalidProxyPortThrowsError() {
+        final int proxyPort = randomFrom(randomIntBetween(Integer.MIN_VALUE, -1), randomIntBetween(65536, Integer.MAX_VALUE));
+        final String realmName = randomAlphaOfLengthBetween(3, 8);
+        final String proxyPortSettingKey = RealmSettings.getFullSettingKey(realmName, JwtRealmSettings.HTTP_PROXY_PORT);
+        final Settings settings = Settings.builder().put(proxyPortSettingKey, proxyPort).build();
+
+        final RealmConfig realmConfig = buildRealmConfig(JwtRealmSettings.TYPE, realmName, settings, randomInt());
+        final IllegalArgumentException e = expectThrows(
+            IllegalArgumentException.class,
+            () -> realmConfig.getSetting(JwtRealmSettings.HTTP_PROXY_PORT)
+        );
+
+        assertThat(
+            e.getMessage(),
+            startsWith(Strings.format("Failed to parse value [%d] for setting [%s]", proxyPort, proxyPortSettingKey))
+        );
+    }
 }
diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtTestCase.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/jwt/JwtTestCase.java
@@ -98,12 +98,14 @@ protected Settings.Builder generateRandomRealmSettings(final String name) throws
         final boolean includePublicKey = includeRsa || includeEc;
         final boolean includeHmac = randomBoolean() || (includePublicKey == false); // one of HMAC/RSA/EC must be true
         final boolean populateUserMetadata = randomBoolean();
+        final boolean useJwksEndpoint = randomBoolean();
+        final boolean useProxy = useJwksEndpoint && randomBoolean();
         final Path jwtSetPathObj = PathUtils.get(pathHome);
-        final String jwkSetPath = randomBoolean()
+        final String jwkSetPath = useJwksEndpoint
             ? "https://op.example.com/jwkset.json"
             : Files.createTempFile(jwtSetPathObj, "jwkset.", ".json").toString();
 
-        if (jwkSetPath.equals("https://op.example.com/jwkset.json") == false) {
+        if (useJwksEndpoint == false) {
             Files.writeString(PathUtils.get(jwkSetPath), "Non-empty JWK Set Path contents");
         }
         final ClientAuthenticationType clientAuthenticationType = randomFrom(ClientAuthenticationType.values());
@@ -195,6 +197,17 @@ protected Settings.Builder generateRandomRealmSettings(final String name) throws
             .put(RealmSettings.getFullSettingKey(name, SSLConfigurationSettings.TRUSTSTORE_ALGORITHM.realm(JwtRealmSettings.TYPE)), "PKIX")
             .put(RealmSettings.getFullSettingKey(name, SSLConfigurationSettings.CERT_AUTH_PATH.realm(JwtRealmSettings.TYPE)), "ca2.pem");
 
+        if (useProxy) {
+            if (randomBoolean()) {
+                // Scheme is optional, and defaults to HTTP
+                settingsBuilder.put(RealmSettings.getFullSettingKey(name, JwtRealmSettings.HTTP_PROXY_SCHEME), "http");
+            }
+
+            settingsBuilder
+                .put(RealmSettings.getFullSettingKey(name, JwtRealmSettings.HTTP_PROXY_HOST), "localhost/proxy")
+                .put(RealmSettings.getFullSettingKey(name, JwtRealmSettings.HTTP_PROXY_PORT), randomIntBetween(1, 65535));
+        }
+
         final MockSecureSettings secureSettings = new MockSecureSettings();
         if (includeHmac) {
             if (randomBoolean()) {
diff --git a/x-pack/test/idp-fixture/src/main/resources/nginx/Dockerfile b/x-pack/test/idp-fixture/src/main/resources/nginx/Dockerfile
@@ -1,15 +1,15 @@
 # We need to do SSL tunnelling, but NGINX doesn't support this out of the box - there's a module for this, but the only
 #  way to install it is to compile NGINX from source, so here we go
 
-FROM nginx:latest
+FROM nginx:1.27.1
 
 RUN set -x \
     && apt-get update \
     # libs and tools that we'll need to build NGINX
     && apt-get install --no-install-recommends --no-install-suggests -y  \
       zlib1g-dev libpcre2-dev libssl-dev wget git gcc patch make  \
     && wget http://nginx.org/download/nginx-1.27.1.tar.gz \
-    && git clone https://github.com/chobits/ngx_http_proxy_connect_module.git \
+    && git clone --depth 1 --branch v0.0.7 --single-branch https://github.com/chobits/ngx_http_proxy_connect_module.git \
     && tar -xzvf nginx-1.27.1.tar.gz \
     && cd nginx-1.27.1 \
     # patch the CONNECT module in
