containers icon indicating copy to clipboard operation
containers copied to clipboard

[bitnami/keycloak] Add support for proxy-headers

Open Kajot-dev opened this issue 1 year ago • 3 comments

Description of the change

Added support for proxy-headers via KEYCLOAK_PROXY_HEADERS. This also removes support for KEYCLOAK_PROXY Implementing both this and KEYCLOAK_PROXY when the current default for KEYCLOAK_PROXY is passthorugh (and not empty) would be a mess since we won't really know if user meant to set "passthrough" or just did not specify this at all (unless we would change the default, but this would be a breaking change anyway) and creating to many possible combinations.

Benefits

Not using deprecated options

Possible drawbacks

Lack of KEYCLOAK_PROXY must be accounted for in helm charts etc.

Applicable issues

  • fixes #65190

Kajot-dev avatar Jun 18 '24 18:06 Kajot-dev

Hi @Kajot-dev

This isn't simply adding support for proxy-headers but dropping support for customizing the proxy mode (which is going to be deprecated). Based on the Upgrading Guide below:

  • https://www.keycloak.org/docs/latest/upgrading/index.html#deprecated-proxy-option

I'm assuming you're not setting any proxy-header by default given it's the equivalent to the "passthrough" proxy mode, am I right?

juan131 avatar Jun 26 '24 06:06 juan131

@juan131 Yes, that's correct!

If you wish, I may try to implement backwards compatibility by introducing KEYCLOAK_LEGACY_PROXY which defaulta to the value of KEYCLOAK_PROXY (and is only a helper variable) before it's assigned the default "passthrough" value. That way the intentions of the user would be clear. Then if KEYCLOAK_LEGACY_PROXY is empty, we can use proxy-headers, else use proxy setting like before. Let me know what you think?

Kajot-dev avatar Jun 26 '24 06:06 Kajot-dev

I don't think that's necessary, it's overcomplicated.

We should add some mechanism though in the associated chart. For instance, we could mark the proxy parameter as deprecated and introduce a backwards compatible mechanism:

  • values.yaml:
+## @param proxyHeaders Set Keycloak proxy headers
+##
+proxyHeaders: ""

 ## @param proxy reverse Proxy mode edge, reencrypt, passthrough or none
+## DEPRECATED: use proxyHeaders instead
 ## ref: https://www.keycloak.org/server/reverseproxy
 ##
-proxy: passthrough
+proxy: ""
  • templates/configmap-env-vars.yaml
+ {{- if and .Values.proxy (empty .Values.proxyHeaders }}
- KEYCLOAK_PROXY: {{ .Values.proxy | quote }}
+ KEYCLOAK_PROXY_HEADERS: {{ ternary "" "forwarded|xforwarded" (eq .Values.proxy "passthrough") }} 
+ {{- else }}
+ KEYCLOAK_PROXY_HEADERS: {{ .Values.proxyHeaders | quote }} 
+ {{- end }}

juan131 avatar Jun 26 '24 08:06 juan131

I will make counterpart PR for helm chart and link it here

Kajot-dev avatar Jul 02 '24 16:07 Kajot-dev

According to the docs, the edge mode (kc.sh --proxy edge) is equivalent to kc.sh --proxy-headers forwarded|xforwarded --http-enabled true. Don't we need to add another variable to also support the http-enabled option (KC_HTTP_ENABLED)?.. HTTP is disabled by default... or just rely on the Keycloak provided variable. Perhaps we keep the proxy mode still an option in the chart, but conditionally update the KEYCLOAK_PROXY_HEADERS and KC_HTTP_ENABLED based on the mode value, per that table.

singhbaljit avatar Jul 10 '24 01:07 singhbaljit

According to the docs, the edge mode (kc.sh --proxy edge) is equivalent to kc.sh --proxy-headers forwarded|xforwarded --http-enabled true. Don't we need to add another variable to also support the http-enabled option (KC_HTTP_ENABLED)?.. HTTP is disabled by default... or just rely on the Keycloak provided variable. Perhaps we keep the proxy mode still an option in the chart, but conditionally update the KEYCLOAK_PROXY_HEADERS and KC_HTTP_ENABLED based on the mode value, per that table.

I don't think so, because of: https://github.com/bitnami/containers/blob/main/bitnami/keycloak/24/debian-12/rootfs/opt/bitnami/scripts/libkeycloak.sh#L222

In bitnami containers http-enabled is always set to true unconditionally

In other words helm chart can just disable/disallow enabling https together with deprecated proxy in edge mode

Kajot-dev avatar Jul 10 '24 20:07 Kajot-dev

@juan131 Please see: https://github.com/bitnami/charts/pull/27890

Kajot-dev avatar Jul 10 '24 21:07 Kajot-dev

Hi @Kajot-dev

We released a new container image version (see 24.0.5-debian-12-r3) including your changes, see https://github.com/bitnami/containers/commit/536990b719e19f1b9e2967985c242db2ae01180a

Thanks for your contribution

juan131 avatar Jul 15 '24 11:07 juan131