From 33425dacd94236affd484933b3a539c5bcf9500e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=A1clav=20Muzik=C3=A1=C5=99?= Date: Wed, 21 Feb 2024 12:19:37 +0100 Subject: [PATCH] Add `proxy-headers` option to the Keycloak CR (#27092) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closes #25179 Signed-off-by: Václav Muzikář --- .../topics/keycloak/changes-24_0_0.adoc | 19 ++++++ docs/guides/operator/basic-deployment.adoc | 30 +++++++++ .../KeycloakDeploymentDependentResource.java | 10 ++- .../controllers/KeycloakDistConfigurator.java | 7 +++ .../v2alpha1/deployment/KeycloakSpec.java | 12 ++++ .../v2alpha1/deployment/spec/ProxySpec.java | 40 ++++++++++++ .../integration/KeycloakDeploymentTest.java | 19 ------ .../integration/KeycloakIngressTest.java | 62 +++++++++++++------ .../unit/KeycloakDistConfiguratorTest.java | 9 +++ .../src/test/resources/example-keycloak.yaml | 4 +- .../test-serialization-keycloak-cr.yml | 2 + .../resources/ConstantsDebugHostname.java | 1 + 12 files changed, 175 insertions(+), 40 deletions(-) create mode 100644 operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/spec/ProxySpec.java diff --git a/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc b/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc index 0a665ac105..1b3bda81c1 100644 --- a/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc +++ b/docs/documentation/upgrading/topics/keycloak/changes-24_0_0.adoc @@ -174,6 +174,25 @@ The `--proxy` option has been deprecated and will be removed in a future release NOTE: For hardened security, the `--proxy-headers` option does not allow selecting both `forwarded` and `xforwarded` values at the same time (as it was the case before for `--proxy edge` and `--proxy reencrypt`). +WARNING: When using the proxy headers option, make sure your reverse proxy properly sets and overwrites the `Forwarded` or `X-Forwarded-*` headers respectively. +To set these headers, consult the documentation for your reverse proxy. Misconfiguration will leave {project_name} exposed to security vulnerabilities. + +You can also set the proxy headers when using the Operator: +[source,yaml] +---- +apiVersion: k8s.keycloak.org/v2alpha1 +kind: Keycloak +metadata: + name: example-kc +spec: + ... + proxy: + headers: forwarded|xforwarded +---- +NOTE: If the `proxy.headers` field is not specified, the Operator falls back to the previous behaviour by implicitly setting +`proxy=passthrough` by default. This results in deprecation warnings in the server log. This fallback will be removed +in a future release. + = Changes to the user representation in both Admin API and Account contexts Both `org.keycloak.representations.idm.UserRepresentation` and `org.keycloak.representations.account.UserRepresentation` representation classes have changed diff --git a/docs/guides/operator/basic-deployment.adoc b/docs/guides/operator/basic-deployment.adoc index 3f62c61f10..7a846826b7 100644 --- a/docs/guides/operator/basic-deployment.adoc +++ b/docs/guides/operator/basic-deployment.adoc @@ -152,6 +152,8 @@ spec: tlsSecret: example-tls-secret hostname: hostname: test.keycloak.org + proxy: + headers: xforwarded # double check your reverse proxy sets and overwrites the X-Forwarded-* headers ---- Apply the changes: @@ -233,6 +235,34 @@ For debugging and development purposes, consider directly connecting to the {pro kubectl port-forward service/example-kc-service 8443:8443 ---- +==== Configuring the reverse proxy settings matching your Ingress Controller + +The Operator supports configuring which of the reverse proxy headers should be accepted by server, which includes +`Forwarded` and `X-Forwarded-*` headers. + +If you Ingress implementation sets and overwrites either `Forwarded` or `X-Forwarded-*` headers, you can reflect that +in the Keycloak CR as follows: +[source,yaml] +---- +apiVersion: k8s.keycloak.org/v2alpha1 +kind: Keycloak +metadata: + name: example-kc +spec: + ... + proxy: + headers: forwarded|xforwarded +---- +NOTE: If the `proxy.headers` field is not specified, the Operator falls back to legacy behaviour by implicitly setting +`proxy=passthrough` by default. This results in deprecation warnings in the server log. This fallback will be removed +in a future release. + +WARNING: When using the `proxy.headers` field, make sure your Ingress properly sets and overwrites the `Forwarded` or `X-Forwarded-*` headers respectively. To set these headers, consult the documentation for your Ingress Controller. Consider configuring it for +either reencrypt or edge TLS termination as passthrough TLS doesn't allow the Ingress to modify the requests headers. +Misconfiguration will leave {project_name} exposed to security vulnerabilities. + +For more details refer to the <@links.server id="reverseproxy"/> guide. + === Accessing the Admin Console When deploying {project_name}, the operator generates an arbitrary initial admin `username` and `password` and stores those credentials as a basic-auth Secret object in the same namespace as the CR. diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentDependentResource.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentDependentResource.java index e96d76272f..f9d376732f 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentDependentResource.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDeploymentDependentResource.java @@ -68,6 +68,7 @@ import java.util.stream.Stream; import jakarta.inject.Inject; import static org.keycloak.operator.Utils.addResources; +import static org.keycloak.operator.controllers.KeycloakDistConfigurator.getKeycloakOptionEnvVarName; import static org.keycloak.operator.crds.v2alpha1.CRDUtils.isTlsConfigured; @KubernetesDependent(labelSelector = Constants.DEFAULT_LABELS_AS_STRING) @@ -368,6 +369,13 @@ public class KeycloakDeploymentDependentResource extends CRUDKubernetesDependent // include the kube CA if the user is not controlling KC_TRUSTSTORE_PATHS via the unsupported or the additional varMap.putIfAbsent(KC_TRUSTSTORE_PATHS, new EnvVarBuilder().withName(KC_TRUSTSTORE_PATHS).withValue(truststores).build()); + // TODO remove this once the --proxy option is finally removed from Keycloak + // not strictly necessary as --proxy-headers take precedence over --proxy but at least removes the warning + // about deprecated --proxy option in use + if (varMap.containsKey(getKeycloakOptionEnvVarName("proxy-headers"))) { + varMap.remove(getKeycloakOptionEnvVarName("proxy")); + } + var envVars = new ArrayList<>(varMap.values()); baseDeployment.getSpec().getTemplate().getSpec().getContainers().get(0).setEnv(envVars); @@ -395,7 +403,7 @@ public class KeycloakDeploymentDependentResource extends CRUDKubernetesDependent // set env vars List envVars = serverConfigsList.stream() .map(v -> { - var envBuilder = new EnvVarBuilder().withName(KeycloakDistConfigurator.getKeycloakOptionEnvVarName(v.getName())); + var envBuilder = new EnvVarBuilder().withName(getKeycloakOptionEnvVarName(v.getName())); var secret = v.getSecret(); if (secret != null) { envBuilder.withValueFrom( diff --git a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java index a9e45d5a87..a742d47e8e 100644 --- a/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java +++ b/operator/src/main/java/org/keycloak/operator/controllers/KeycloakDistConfigurator.java @@ -32,6 +32,7 @@ import org.keycloak.operator.crds.v2alpha1.deployment.spec.DatabaseSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.FeatureSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.HostnameSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.HttpSpec; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.ProxySpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.TransactionsSpec; import java.util.ArrayList; @@ -68,6 +69,7 @@ public class KeycloakDistConfigurator { configureHttp(); configureDatabase(); configureCache(); + configureProxy(); } /** @@ -130,6 +132,11 @@ public class KeycloakDistConfigurator { .mapOption("db-pool-max-size", DatabaseSpec::getPoolMaxSize); } + void configureProxy() { + optionMapper(keycloakCR -> keycloakCR.getSpec().getProxySpec()) + .mapOption("proxy-headers", ProxySpec::getHeaders); + } + /* ---------- END of configuration of first-class citizen fields ---------- */ /** diff --git a/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/KeycloakSpec.java b/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/KeycloakSpec.java index f839e92624..bd0b67612b 100644 --- a/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/KeycloakSpec.java +++ b/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/KeycloakSpec.java @@ -26,6 +26,7 @@ import org.keycloak.operator.crds.v2alpha1.deployment.spec.FeatureSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.HostnameSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.HttpSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpec; +import org.keycloak.operator.crds.v2alpha1.deployment.spec.ProxySpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.TransactionsSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.Truststore; import org.keycloak.operator.crds.v2alpha1.deployment.spec.UnsupportedSpec; @@ -100,6 +101,10 @@ public class KeycloakSpec { @JsonPropertyDescription("Compute Resources required by Keycloak container") private ResourceRequirements resourceRequirements; + @JsonProperty("proxy") + @JsonPropertyDescription("In this section you can configure Keycloak's reverse proxy setting") + private ProxySpec proxySpec; + public HttpSpec getHttpSpec() { return httpSpec; } @@ -226,4 +231,11 @@ public class KeycloakSpec { this.resourceRequirements = resourceRequirements; } + public ProxySpec getProxySpec() { + return proxySpec; + } + + public void setProxySpec(ProxySpec proxySpec) { + this.proxySpec = proxySpec; + } } \ No newline at end of file diff --git a/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/spec/ProxySpec.java b/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/spec/ProxySpec.java new file mode 100644 index 0000000000..396f96f334 --- /dev/null +++ b/operator/src/main/java/org/keycloak/operator/crds/v2alpha1/deployment/spec/ProxySpec.java @@ -0,0 +1,40 @@ +/* + * 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.operator.crds.v2alpha1.deployment.spec; + +import com.fasterxml.jackson.annotation.JsonInclude; +import com.fasterxml.jackson.annotation.JsonPropertyDescription; +import io.sundr.builder.annotations.Buildable; + +/** + * @author Vaclav Muzikar + */ +@JsonInclude(JsonInclude.Include.NON_NULL) +@Buildable(editableEnabled = false, builderPackage = "io.fabric8.kubernetes.api.builder") +public class ProxySpec { + @JsonPropertyDescription("The proxy headers that should be accepted by the server. Misconfiguration might leave the server exposed to security vulnerabilities.") + private String headers; + + public String getHeaders() { + return headers; + } + + public void setHeaders(String headers) { + this.headers = headers; + } +} diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java index cf1888c87b..4ba466aebb 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakDeploymentTest.java @@ -158,20 +158,13 @@ public class KeycloakDeploymentTest extends BaseOperatorTest { var defaultKCDeploy = getTestKeycloakDeployment(true); var valueSecretHealthProp = new ValueOrSecret("health-enabled", "false"); - var valueSecretProxyProp = new ValueOrSecret("proxy", "reencrypt"); var healthEnvVar = new EnvVarBuilder() .withName(KeycloakDistConfigurator.getKeycloakOptionEnvVarName(valueSecretHealthProp.getName())) .withValue(valueSecretHealthProp.getValue()) .build(); - var proxyEnvVar = new EnvVarBuilder() - .withName(KeycloakDistConfigurator.getKeycloakOptionEnvVarName(valueSecretProxyProp.getName())) - .withValue(valueSecretProxyProp.getValue()) - .build(); - defaultKCDeploy.getSpec().getAdditionalOptions().add(valueSecretHealthProp); - defaultKCDeploy.getSpec().getAdditionalOptions().add(valueSecretProxyProp); deployKeycloak(k8sclient, defaultKCDeploy, false); @@ -183,14 +176,6 @@ public class KeycloakDeploymentTest extends BaseOperatorTest { .getValue() ).isEqualTo("true"); // just a sanity check default values did not change - assertThat( - Constants.DEFAULT_DIST_CONFIG_LIST.stream() - .filter(oneValueOrSecret -> oneValueOrSecret.getName().equalsIgnoreCase(valueSecretProxyProp.getName())) - .findFirst() - .get() - .getValue() - ).isEqualTo("passthrough"); // just a sanity check default values did not change - Awaitility.await() .ignoreExceptions() .untilAsserted(() -> { @@ -213,10 +198,6 @@ public class KeycloakDeploymentTest extends BaseOperatorTest { .filter(oneEnvVar -> oneEnvVar.getName().equalsIgnoreCase(healthEnvVar.getName()))) .containsExactly(healthEnvVar); - assertThat(firstKCContainer.getEnv().stream() - .filter(oneEnvVar -> oneEnvVar.getName().equalsIgnoreCase(proxyEnvVar.getName()))) - .containsExactly(proxyEnvVar); - }); } diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java index 94d35904e0..6ee4056ed9 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/integration/KeycloakIngressTest.java @@ -34,7 +34,6 @@ import org.keycloak.operator.crds.v2alpha1.deployment.Keycloak; import org.keycloak.operator.crds.v2alpha1.deployment.spec.HostnameSpecBuilder; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpec; import org.keycloak.operator.crds.v2alpha1.deployment.spec.IngressSpecBuilder; -import org.keycloak.operator.crds.v2alpha1.deployment.spec.UnsupportedSpecBuilder; import org.keycloak.operator.testsuite.utils.K8sUtils; import java.util.Map; @@ -57,22 +56,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { .withStrictBackchannel(false); if (isOpenShift) { kc.getSpec().setIngressSpec(new IngressSpecBuilder().withIngressClassName(KeycloakController.OPENSHIFT_DEFAULT).build()); - - // see https://github.com/keycloak/keycloak/issues/14400#issuecomment-1659900081 - kc.getSpec().setUnsupported(new UnsupportedSpecBuilder() - .withNewPodTemplate() - .withNewSpec() - .addNewContainer() - .addNewEnv() - .withName("KC_PROXY") - .withValue("edge") - .endEnv() - .endContainer() - .endSpec() - .endPodTemplate() - .build()); } - kc.getSpec().setHostnameSpec(hostnameSpecBuilder.build()); K8sUtils.deployKeycloak(k8sclient, kc, true); @@ -91,7 +75,7 @@ public class KeycloakIngressTest extends BaseOperatorTest { } @Test - public void testIngressOnHTTPS() { + public void testIngressOnHTTPSAndProxySettings() { var kc = getTestKeycloakDeployment(false); var hostnameSpecBuilder = new HostnameSpecBuilder() .withStrict(false) @@ -111,6 +95,45 @@ public class KeycloakIngressTest extends BaseOperatorTest { } testIngressURLs("https://" + testHostname + ":443"); + + // just check we really have proxy set correctly + var envVars = k8sclient.apps().statefulSets().withName(kc.getMetadata().getName()).get().getSpec() + .getTemplate().getSpec().getContainers().get(0).getEnv(); + assertThat(envVars) + .noneMatch(e -> "KC_PROXY".equals(e.getName())) + .anyMatch(e -> "KC_PROXY_HEADERS".equals(e.getName()) && "xforwarded".equals(e.getValue())); + } + + // TODO remove this test once the --proxy option is finally removed from Keycloak + @Test + public void testFallbackToDefaultProxySettings() { + var kc = getTestKeycloakDeployment(false); + var hostnameSpecBuilder = new HostnameSpecBuilder() + .withStrict(false) + .withStrictBackchannel(false); + if (isOpenShift) { + kc.getSpec().setIngressSpec(new IngressSpecBuilder().withIngressClassName(KeycloakController.OPENSHIFT_DEFAULT).build()); + } + kc.getSpec().setHostnameSpec(hostnameSpecBuilder.build()); + kc.getSpec().setProxySpec(null); + + K8sUtils.deployKeycloak(k8sclient, kc, true); + + String testHostname; + if (isOpenShift) { + testHostname = k8sclient.resource(kc).get().getSpec().getHostnameSpec().getHostname(); + } else { + testHostname = kubernetesIp; + } + + testIngressURLs("https://" + testHostname + ":443"); + + // just check we really have proxy set correctly + var envVars = k8sclient.apps().statefulSets().withName(kc.getMetadata().getName()).get().getSpec() + .getTemplate().getSpec().getContainers().get(0).getEnv(); + assertThat(envVars) + .anyMatch(e -> "KC_PROXY".equals(e.getName()) && "passthrough".equals(e.getValue())) + .noneMatch(e -> "KC_PROXY_HEADERS".equals(e.getName())); } private void testIngressURLs(String baseUrl) { @@ -125,9 +148,10 @@ public class KeycloakIngressTest extends BaseOperatorTest { .get(url) .body() .jsonPath() - .getString("realm"); + .getString("token-service"); - assertEquals("master", output); + // the Keycloak URL must be without port, otherwise proxy resolution doesn't work correctly + assertEquals(url.replaceAll(":\\d+", "") + "/protocol/openid-connect", output); }); Awaitility.await() diff --git a/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java b/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java index d3d02d6983..895253ed2b 100644 --- a/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java +++ b/operator/src/test/java/org/keycloak/operator/testsuite/unit/KeycloakDistConfiguratorTest.java @@ -135,6 +135,15 @@ public class KeycloakDistConfiguratorTest { assertEnvVarNotPresent(envVars, "KC_HOSTNAME_STRICT_BACKCHANNEL"); } + @Test + public void proxy() { + final Map expectedValues = Map.of( + "proxy-headers", "forwarded" + ); + + testFirstClassCitizen(expectedValues); + } + /* UTILS */ private void testFirstClassCitizen(Map expectedValues) { diff --git a/operator/src/test/resources/example-keycloak.yaml b/operator/src/test/resources/example-keycloak.yaml index c79b821a16..d20ecc07b3 100644 --- a/operator/src/test/resources/example-keycloak.yaml +++ b/operator/src/test/resources/example-keycloak.yaml @@ -16,4 +16,6 @@ spec: http: tlsSecret: example-tls-secret hostname: - hostname: example.com \ No newline at end of file + hostname: example.com + proxy: + headers: xforwarded # default nginx ingress sets x-forwarded \ No newline at end of file diff --git a/operator/src/test/resources/test-serialization-keycloak-cr.yml b/operator/src/test/resources/test-serialization-keycloak-cr.yml index 8d3fa0226b..cad8830982 100644 --- a/operator/src/test/resources/test-serialization-keycloak-cr.yml +++ b/operator/src/test/resources/test-serialization-keycloak-cr.yml @@ -63,6 +63,8 @@ spec: limits: cpu: "2" memory: "1500M" + proxy: + headers: forwarded unsupported: podTemplate: metadata: diff --git a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/services/resources/ConstantsDebugHostname.java b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/services/resources/ConstantsDebugHostname.java index 2ff612ecb9..d9c7a88a39 100644 --- a/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/services/resources/ConstantsDebugHostname.java +++ b/quarkus/runtime/src/main/java/org/keycloak/quarkus/runtime/services/resources/ConstantsDebugHostname.java @@ -37,6 +37,7 @@ public class ConstantsDebugHostname { "hostname-path", "hostname-port", "proxy", + "proxy-headers", "http-enabled", "http-relative-path", "http-port",