fix: relaxes the admin root redirect check #39095
Conversation
|
|
||
| if (!isAdminConsoleEnabled() || (!requestUrl.startsWith(adminUrl) && !requestUrl.startsWith(localAdminUrl))) { | ||
| return Response.status(Response.Status.NOT_FOUND).build(); | ||
| if (adminUrl.equals(frontEndUrl)) { |
There was a problem hiding this comment.
This makes sense to me - the issue I reported would enter here, my frontend and admin are the same.
There was a problem hiding this comment.
I built a new keycloak-999.0.0-SNAPSHOT.zip and tested my edge case.
$ ./bin/kc.sh start-dev \
--hostname=https://banana.russdev
In another window I curled the /admin/ endpoint and saw the results I was expecting
$ curl -v http://banana.russdev:8080/admin/
< HTTP/1.1 302 Found
< Location: https://banana.russdev/admin/master/console/
LGTM!
|
Discussed offline that the local admin concept is a bit odd, but we may want to stick with it in this pr for now and just note that the original pr introduced a small breaking change that any localhost host or address other than localhost will no longer work for admin redirects. @vmuzikar @mabartos Looking into things more - LOCAL_ADMIN was introduced because that seemed to be the right way to resolve #23259 - but bringing the hostname provider into this didn't really help the situation. The current code is still not good in the EDGE case where no proxy headers are set. Using the incoming request from the proxy in this situation will effectively leak the actual scheme, port, and path the proxy is using to access Keycloak - which we probably should not be doing. Alternatively if we cannot detect that the connection is local, the best we should do is make a general statement about using localhost, rather than providing a link, and get rid of LOCAL_ADMIN completely (after deprecating). |
There was a problem hiding this comment.
I was thinking about it more too. It indeed seems like an unnecessary weakness to use LOCAL_ADMIN at the welcome page. +1 for just removing the link and having there just a statement that local address needs to be used.
LOCAL_ADMIN was supposed to be useful for crafting localhost address but as you say it's very problematic if edge (or reencrypt) proxy without proxy headers is involved.
For the admin redirect, let's use the approach in this PR and let's deprecate LOCAL_ADMIN.
services/src/main/java/org/keycloak/services/resources/admin/AdminRoot.java
Outdated
Show resolved
Hide resolved
Sounds good, I'll make thoses changes here. |
also deprecates the usage of local_admin closes: keycloak#39085 Signed-off-by: Steve Hawkins <shawkins@redhat.com>
|
@vmuzikar changed the pr to use the same method for the local check in the AdminRoot and the WelcomeResource, deprecated admin local, and removed the link from the welcome page. An upgrade note was added for 26.2.2 presuming this will be quickly backported, or if you want I can open that pr first. |
services/src/main/java/org/keycloak/services/resources/WelcomeResource.java
Show resolved
Hide resolved
| === local admin deprecation | ||
|
|
||
| `UrlType.LOCAL_ADMIN` and the corresponding welcome theme variable `localAdminUrl` have been deprecated for eventual removal. The default welcome resource will now simply mention localhost rather than providing a URL when an admin user has yet to be created. |
There was a problem hiding this comment.
Despite the deprecation not being a breaking change, I believe we can't do it in a patch release. I'd suggest handling the deprecation in a separate PR for 26.3.
There was a problem hiding this comment.
Despite the deprecation not being a breaking change, I believe we can't do it in a patch release.
It seems like we can't avoid having some kind of notice in 26.2.2. If you are worried about the case of having a custom hostname provider customize LOCAL_ADMIN, they would need to know that it is no longer used by the default welcome resource.
There was a problem hiding this comment.
I'm not really worried about the LOCAL_ADMIN not being used on the Welcome page. That's basically a security hardening, IMHO doesn't even need to be mentioned in the upgrading guide – it's just a removed link.
What I believe we should not do in a patch release is deprecating an API, which LOCAL_ADMIN is.
There was a problem hiding this comment.
My belief is that it would still be a good idea to let users know that LOCAL_ADMIN is not used by default anymore, and that if they are providing their own LOCAL_ADMIN that it should be hardened. If this can't be done via a deprecation because it's a patch release, then I'd still want some change note.
There was a problem hiding this comment.
There might be one argument towards deprecating it in a patch release: it's basically a security hardening. As far as we know. there's no good use of LOCAL_ADMIN.
However as you can see, this makes the non-redirect case harder to test (in an integration test) - we'll need to access the server via a non-localhost connection. @vmuzikar are you okay with changing the test to make it seem like the request is coming from a proxy so that it fails the isLocal check? |
services/src/main/java/org/keycloak/services/resources/WelcomeResource.java
Show resolved
Hide resolved
That's IMHO fine. |
also changing the adminroot test to seem like it's coming from a proxy Signed-off-by: Steve Hawkins <shawkins@redhat.com>
| @@ -2,6 +2,10 @@ | |||
|
|
|||
| Breaking changes are identified as requiring changes from existing users to their configurations. | |||
|
|
|||
| === local admin deprecated for removal | |||
There was a problem hiding this comment.
The heading should provide more context, for example:
| === local admin deprecated for removal | |
| === Java enum `UrlType.LOCAL_ADMIN` deprecated for removal |
There was a problem hiding this comment.
It's both the enum and the template variable that are being deprecated.
* fix: relaxes the admin root redirect check also deprecates the usage of local_admin closes: keycloak#39085 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * moving deprecation to 26.3 also changing the adminroot test to seem like it's coming from a proxy Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com> (cherry picked from commit 08b5183)
closes: keycloak#39085 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * moving deprecation to 26.3 also changing the adminroot test to seem like it's coming from a proxy Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com> (cherry picked from commit 08b5183) Signed-off-by: Steven Hawkins <shawkins@redhat.com>
closes: keycloak#39085 Signed-off-by: Steve Hawkins <shawkins@redhat.com> also changing the adminroot test to seem like it's coming from a proxy Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com> (cherry picked from commit 08b5183) Signed-off-by: Steven Hawkins <shawkins@redhat.com>
* fix: relaxes the admin root redirect check also deprecates the usage of local_admin closes: keycloak#39085 Signed-off-by: Steve Hawkins <shawkins@redhat.com> * moving deprecation to 26.3 also changing the adminroot test to seem like it's coming from a proxy Signed-off-by: Steve Hawkins <shawkins@redhat.com> --------- Signed-off-by: Steve Hawkins <shawkins@redhat.com>
closes: #39085
@vmuzikar @russau this relaxes the check in two ways