CZERTAINLY / CZERTAINLY-Helm-Charts

CZERTAINLY - Helm Charts
https://www.czertainly.com
MIT License
4 stars 2 forks source link

do not provide rootUrl #195

Closed semik closed 3 months ago

semik commented 3 months ago

This PR is solution for issue #194, please see there for initial description of the problem.

The idea with setting rootUrl as ${authBaseUrl} for kong client was wrong. baseURL of our Keycloak inside of CZERTAILY is https://hostname/kc but we need to somehow generate redirect_uri https://hostname/login`. This is not possible with our baseURL.

Experments with .. also failed:

2024-08-05 09:30:40,417 DEBUG [org.keycloak.protocol.oidc.utils.RedirectUtils] (executor-thread-15) replacing relative valid redirect with: https://czertainly.local/kc/../login
2024-08-05 09:30:40,417 DEBUG [org.keycloak.protocol.oidc.utils.RedirectUtils] (executor-thread-15) replacing relative valid redirect with: https://czertainly.local/kc/login/
2024-08-05 09:30:40,417 DEBUG [org.keycloak.protocol.oidc.utils.RedirectUtils] (executor-thread-15) replacing relative valid redirect with: https://czertainly.local/kc/../../login
2024-08-05 09:30:40,417 DEBUG [org.keycloak.transaction.JtaTransactionWrapper] (executor-thread-15) new JtaTransactionWrapper
2024-08-05 09:30:40,417 DEBUG [org.keycloak.transaction.JtaTransactionWrapper] (executor-thread-15) was existing? true
2024-08-05 09:30:40,418 DEBUG [org.keycloak.transaction.JtaTransactionWrapper] (executor-thread-15) JtaTransactionWrapper  commit
2024-08-05 09:30:40,418 DEBUG [org.keycloak.transaction.JtaTransactionWrapper] (executor-thread-15) JtaTransactionWrapper end
2024-08-05 09:30:40,418 DEBUG [org.keycloak.transaction.JtaTransactionWrapper] (executor-thread-15) JtaTransactionWrapper resuming suspended
2024-08-05 09:30:40,418 WARN  [org.keycloak.events] (executor-thread-15) type="LOGIN_ERROR", realmId="1595e715-e7d0-417a-8df5-77bbdde4e8d8", clientId="kong", userId="null", ipAddress="192.168.1.12", error="invalid_redirect_uri", redirect_uri="https://czertainly.local/login/"

The right solution is to not provide rootUrl at all. Tooltip associated with Valid redirect URLS says: ... Valid URI pattern a browser can redirect to after a successful login. Simple wildcards are allowed such as 'http://example.com/'. Relative path can be specified too such as /my/relative/path/. Relative paths are relative to the client root URL, or if none is specified the auth server root URL is used. For SAML, you must set valid URI patterns if you are relying on the consumer service URL embedded with the login request. The behavior when no rootUrl is provided is exactly what we need.

This patch resolves problem when renamed instance of CERTAINLY have broken Keycloak. However there still remains other values of czertainly_realm.json which are templated by Helm, but applied only during the first import of this file. Maybe we need to search other way of initial Keycloak configuration.

This PR is just about possibility rename CZERTAINLY instance.

closes #194

3keyroman commented 3 months ago

An empty root URL might lead to more relaxed validation of redirect URIs. This can open up vulnerabilities, such as allowing malicious redirections. Although we have configure the redirect URIs, it can open possible way to compromise the system.

I would suggest to keep the root URL configured. The issue here is not with the values not being properly changed in the configuration files and yaml manifests using Helm, but rather with existing CZERTAINLY realm in the Keycloak database that would not be overwritten with updated realm. Import of existing realm does not have any effect, even if some of the configuration values has changed.

We need to implement a proper way how to make it happen, and the only reasonable way seems to update data of existing realm using Keycloak REST API. It should be feasible since the credentials to authenticate for the API are available in the container. The script should be implemented that will update client data after successful Keycloak startup.

It should be applicable not only for the hostname value, but also for other dynamic configuration handled by the Helm chart.

I am closing this PR without merging it.