Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not provide rootUrl #195

Closed
wants to merge 1 commit into from
Closed

Conversation

semik
Copy link
Contributor

@semik semik commented Aug 5, 2024

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
Copy link
Contributor

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.

@3keyroman 3keyroman closed this Aug 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

hostName change is not correctly propagated into Keycloak
2 participants