8000 fix(admin): block TOCTOU rename bypass of protected admin roles by ThreeMangoTrees · Pull Request #49516 · keycloak/keycloak · GitHub
[go: up one dir, main page]
Skip to content

fix(admin): block TOCTOU rename bypass of protected admin roles #49516

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.

Sign up for GitHub

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

Closed
ThreeMangoTrees wants to merge 1 commit into from
Closed

fix(admin): block TOCTOU rename bypass of protected admin roles #49516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions services/src/main/java/org/keycloak/services/resources/admin/RoleByIdResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@

import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.POST;
Expand Down Expand Up @@ -170,6 +171,9 @@ public void deleteRole(final @Parameter(description = "id of role") @PathParam("
public void updateRole(final @Parameter(description = "id of role") @PathParam("role-id") String id, final RoleRepresentation rep) {
RoleModel role = getRoleModel(id);
auth.roles().requireManage(role);
if (isProtectedAdminRoleRename(role, rep.getName()) && !auth.realm().canManageRealm()) {
Comment thread
pedroigor marked this conversation as resolved.
Resolved
throw new ForbiddenException("Cannot rename a protected admin role");
}
updateRole(rep, role, realm, session);

if (role.isClientRole()) {
Expand Down
4 changes: 4 additions & 0 deletions services/src/main/java/org/keycloak/services/resources/admin/RoleContainerResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import jakarta.ws.rs.Consumes;
import jakarta.ws.rs.DELETE;
import jakarta.ws.rs.DefaultValue;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.GET;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.POST;
Expand Down Expand Up @@ -333,6 +334,9 @@ public Response updateRole(final @Parameter(description = "role's name (not id!)
if (role == null) {
throw new NotFoundException("Could not find role");
}
if (isProtectedAdminRoleRename(role, rep.getName()) && !auth.realm().canManageRealm()) {
@sguilhen sguilhen Jun 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also related to this: Looking at the resources (both RoleByIdResource and RoleContainerResource, the admin roles are still vulnerable to deletes - shouldn't we prevent deletion of protected roles as well?

@vramik vramik Jun 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1, IMO we should not allow deletion as well.

throw new ForbiddenException("Cannot rename a protected admin role");
}
try {
updateRole(rep, role, realm, session);

Expand Down
35 changes: 35 additions & 0 deletions services/src/main/java/org/keycloak/services/resources/admin/RoleResource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,12 @@
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.core.UriInfo;

import org.keycloak.Config;
import org.keycloak.events.admin.OperationType;
import org.keycloak.events.admin.ResourceType;
import org.keycloak.models.AdminRoles;
import org.keycloak.models.ClientModel;
import org.keycloak.models.Constants;
import org.keycloak.models.KeycloakSession;
import org.keycloak.models.RealmModel;
import org.keycloak.models.RoleModel;
Expand Down Expand Up @@ -120,6 +123,38 @@ public KeycloakSession getKeycloakSession() {
}
}

/**
* Returns true when the rename involves a protected admin role name and requires
* full realm-management permission. Renaming TO a protected name is blocked for any
* container; renaming FROM one is only blocked for admin containers (realm or realm-management client).
*/
protected boolean isProtectedAdminRoleRename(RoleModel role, String newName) {
@sguilhen sguilhen Jun 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If I'm getting this right, it would block other updates for protected roles even if we're not renaming the role. For example, if one changes the role description this method would block the update even if no rename is happening.

@pedroigor pedroigor Jun 3, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, that is the goal.

boolean currentNameProtected = AdminRoles.ALL_ROLES.contains(role.getName());
boolean newNameProtected = newName != null && AdminRoles.ALL_ROLES.contains(newName);

if (!currentNameProtected && !newNameProtected) {
return false;
}

// Renaming TO a protected name is blocked for any container.
if (newNameProtected) {
return true;
}

// Renaming FROM a protected name: only guard admin containers.
if (role.getContainer() instanceof RealmModel) {
return true;
}
if (role.getContainer() instanceof ClientModel client) {
@pedroigor pedroigor Jun 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also need to check the client roles from master realm clients such as foo-realm and master-realm. See AdminRoles.APP_SUFFIX.

This is the "realm-management" client, but in the master realm, which allows cross-realm admin delegation.

String adminClientId = realm.getName().equals(Config.getAdminRealm())
? Config.getAdminRealm() + "-realm"
: Constants.REALM_MANAGEMENT_CLIENT_ID;
ClientModel adminClient = realm.getClientByClientId(adminClientId);
return adminClient != null && adminClient.equals(client);
}
return false;
}

protected void addComposites(AdminPermissionEvaluator auth, AdminEventBuilder adminEvent, UriInfo uriInfo, List<RoleRepresentation> roles, RoleModel role) {
for (RoleRepresentation rep : roles) {
if (rep.getId() == null) throw new NotFoundException("Could not find composite role");
Expand Down
192 changes: 192 additions & 0 deletions tests/base/src/test/java/org/keycloak/tests/admin/RoleByIdResourceTest.java
View file
Open in desktop
10BC8
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
package org.keycloak.tests.admin;

import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedList;
import java.util.List;
Expand All @@ -28,13 +29,22 @@
import java.util.stream.Collectors;

import jakarta.ws.rs.BadRequestException;
import jakarta.ws.rs.ForbiddenException;
import jakarta.ws.rs.NotFoundException;
import jakarta.ws.rs.core.Response;

import org.keycloak.admin.client.Keycloak;
import org.keycloak.admin.client.resource.RoleByIdResource;
import org.keycloak.events.admin.OperationType;
import org.keycloak.events.admin.ResourceType;
import org.keycloak.models.AdminRoles;
import org.keycloak.models.Constants;
import org.keycloak.representations.idm.ClientRepresentation;
import org.keycloak.representations.idm.CredentialRepresentation;
import org.keycloak.representations.idm.RoleRepresentation;
import org.keycloak.representations.idm.UserRepresentation;
import org.keycloak.testframework.admin.AdminClientFactory;
import org.keycloak.testframework.annotations.InjectAdminClientFactory;
import org.keycloak.testframework.annotations.InjectAdminEvents;
import org.keycloak.testframework.annotations.InjectClient;
import org.keycloak.testframework.annotations.InjectRealm;
Expand Down Expand Up @@ -78,6 +88,9 @@ public class RoleByIdResourceTest {
@InjectAdminEvents
AdminEvents adminEvents;

@InjectAdminClientFactory
AdminClientFactory adminClientFactory;

private RoleByIdResource resource;

private final String roleNameA = "role-a";
Expand Down Expand Up @@ -367,4 +380,183 @@ public void createRolePreviouslyCached() {
private void cacheMissingRoleName(String missingRoleName) {
assertThrows(NotFoundException.class, () -> managedRealm.admin().roles().get(missingRoleName).toRepresentation());
}

/**
* see #49427
*/
@Test
@DatabaseTest
@vramik vramik Jun 2, 2026
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems unnecessary to test this in database testsuite. Regular integration test should suffice,

public void testRenameProtectedRoleBlockedForDelegatedAdmin() {
createDelegatedAdminUser("delegated-admin", "password", AdminRoles.MANAGE_CLIENTS);

ClientRepresentation rmClient = managedRealm.admin().clients()
.findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0);
RoleRepresentation realmAdminRole = managedRealm.admin().clients()
.get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation();

try (Keycloak delegatedClient = adminClientFactory.create()
.realm(managedRealm.getName())
.username("delegated-admin")
.password("password")
.clientId(Constants.ADMIN_CLI_CLIENT_ID)
.build()) {

RoleRepresentation renamed = new RoleRepresentation();
renamed.setId(realmAdminRole.getId());
renamed.setName("temp-role");

Assertions.assertThrows(ForbiddenException.class, () ->
delegatedClient.realm(managedRealm.getName()).rolesById()
.updateRole(realmAdminRole.getId(), renamed));
}
}

/**
* see #49427
*/
@Test
@DatabaseTest
@vramik vramik Jun 2, 2026 •
edited
Loading
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, and other tests. No need to test it for DBs.

public void testRenamingToProtectedRoleNameBlockedForDelegatedAdmin() {
createDelegatedAdminUser("delegated-admin2", "password", AdminRoles.MANAGE_CLIENTS);

String attackerClientId = "attacker-client-" + new Random().nextInt(10000);
try (Keycloak delegatedClient = adminClientFactory.create()
.realm(managedRealm.getName())
.username("delegated-admin2")
.password("password")
.clientId(Constants.ADMIN_CLI_CLIENT_ID)
.build()) {

// Create a client and a role the delegated admin owns
Response resp = delegatedClient.realm(managedRealm.getName()).clients()
.create(clientRepresentation(attackerClientId));
String attackerClientUuid = resp.getLocation().getPath().replaceAll(".*/", "");
resp.close();

delegatedClient.realm(managedRealm.getName()).clients()
.get(attackerClientUuid).roles().create(roleRepresentation("innocent-role"));

RoleRepresentation innocentRole = delegatedClient.realm(managedRealm.getName()).clients()
.get(attackerClientUuid).roles().get("innocent-role").toRepresentation();

// Attempt to rename innocent-role → realm-admin
RoleRepresentation renamed = new RoleRepresentation();
renamed.setId(innocentRole.getId());
renamed.setName(AdminRoles.REALM_ADMIN);

Assertions.assertThrows(ForbiddenException.class, () ->
delegatedClient.realm(managedRealm.getName()).rolesById()
.updateRole(innocentRole.getId(), renamed));
}
}

/**
* see #49427 — named-role endpoint (PUT /roles/{role-name}) must also be guarded.
*/
@Test
@DatabaseTest
public void testRenameProtectedRoleBlockedViaNamedRoleEndpoint() {
createDelegatedAdminUser("delegated-admin3", "password", AdminRoles.MANAGE_CLIENTS);

ClientRepresentation rmClient = managedRealm.admin().clients()
.findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0);

try (Keycloak delegatedClient = adminClientFactory.create()
.realm(managedRealm.getName())
.username("delegated-admin3")
.password("password")
.clientId(Constants.ADMIN_CLI_CLIENT_ID)
.build()) {

RoleRepresentation renamed = new RoleRepresentation();
renamed.setName("temp-role");

Assertions.assertThrows(ForbiddenException.class, () ->
delegatedClient.realm(managedRealm.getName()).clients()
.get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN)
.update(renamed));
}
}

/**
* see #49427
*/
@Test
@DatabaseTest
public void testTOCTOURenameCompositeAttackPrevented() {
createDelegatedAdminUser("toctou-attacker", "password", AdminRoles.MANAGE_CLIENTS);

ClientRepresentation rmClient = managedRealm.admin().clients()
.findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0);
RoleRepresentation realmAdminRole = managedRealm.admin().clients()
.get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation();

try (Keycloak delegatedClient = adminClientFactory.create()
.realm(managedRealm.getName())
.username("toctou-attacker")
.password("password")
.clientId(Constants.ADMIN_CLI_CLIENT_ID)
.build()) {

// Step 1: rename realm-admin → temp-role must be blocked
RoleRepresentation renamed = new RoleRepresentation();
renamed.setId(realmAdminRole.getId());
renamed.setName("temp-role");

Assertions.assertThrows(ForbiddenException.class, () ->
delegatedClient.realm(managedRealm.getName()).rolesById()
.updateRole(realmAdminRole.getId(), renamed));

// Confirm realm-admin name is unchanged
RoleRepresentation unchanged = managedRealm.admin().clients()
.get(rmClient.getId()).roles().get(AdminRoles.REALM_ADMIN).toRepresentation();
assertEquals(AdminRoles.REALM_ADMIN, unchanged.getName());
}
}

private void createDelegatedAdminUser(String username, String password, String clientRoleName) {
UserRepresentation user = new UserRepresentation();
user.setUsername(username);
user.setEnabled(true);
user.setEmailVerified(true);
user.setFirstName("Delegated");
user.setLastName("Admin");
user.setEmail(username + "@test.local");

Response resp = managedRealm.admin().users().create(user);
String userId = resp.getLocation().getPath().replaceAll(".*/", "");
resp.close();

CredentialRepresentation cred = new CredentialRepresentation();
cred.setType(CredentialRepresentation.PASSWORD);
cred.setValue(password);
cred.setTemporary(false);
managedRealm.admin().users().get(userId).resetPassword(cred);

ClientRepresentation rmClient = managedRealm.admin().clients()
.findByClientId(Constants.REALM_MANAGEMENT_CLIENT_ID).get(0);
RoleRepresentation role = managedRealm.admin().clients()
.get(rmClient.getId()).roles().get(clientRoleName).toRepresentation();
managedRealm.admin().users().get(userId).roles()
.clientLevel(rmClient.getId()).add(Collections.singletonList(role));

// query-clients is required so the delegated admin can look up clients
RoleRepresentation queryClients = managedRealm.admin().clients()
.get(rmClient.getId()).roles().get(AdminRoles.QUERY_CLIENTS).toRepresentation();
managedRealm.admin().users().get(userId).roles()
.clientLevel(rmClient.getId()).add(Collections.singletonList(queryClients));
}

private ClientRepresentation clientRepresentation(String clientId) {
ClientRepresentation client = new ClientRepresentation();
client.setClientId(clientId);
client.setEnabled(true);
return client;
}

private RoleRepresentation roleRepr 55CD esentation(String name) {
RoleRepresentation role = new RoleRepresentation();
role.setName(name);
return role;
}
}
Loading
0