Skip to content

Commit

Permalink
Revert "Fix BWC for file-settings based role mappings (elastic#113900)…
Browse files Browse the repository at this point in the history 
…" and related (elastic#114326)

Revert "Fix BWC for file-settings based role mappings (elastic#113900)" and related changes. Reverted commits:

- 763764c
- bc8f9dc
- ce07060

This is due to a bug in the above fix. We will reintroduce a paired down version of the fix in a subsequent PR.
  • Loading branch information
@n1v0lg @matthewabbott
n1v0lg authored and matthewabbott committed Oct 10, 2024
1 parent 050cbef commit 8bfdd72
Show file tree
Hide file tree
Showing 14 changed files with 69 additions and 311 deletions.
5 changes: 0 additions & 5 deletions docs/changelog/113900.yaml
View file

This file was deleted.

5 changes: 0 additions & 5 deletions ...ava/org/elasticsearch/xpack/core/security/action/rolemapping/GetRoleMappingsResponse.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;

import java.io.IOException;
import java.util.Collection;

/**
* Response to {@link GetRoleMappingsAction get role-mappings API}.
Expand All @@ -22,10 +21,6 @@ public class GetRoleMappingsResponse extends ActionResponse {

private final ExpressionRoleMapping[] mappings;

public GetRoleMappingsResponse(Collection<ExpressionRoleMapping> mappings) {
this(mappings.toArray(new ExpressionRoleMapping[0]));
}

public GetRoleMappingsResponse(ExpressionRoleMapping... mappings) {
this.mappings = mappings;
}
Expand Down
166 changes: 45 additions & 121 deletions ...src/internalClusterTest/java/org/elasticsearch/integration/RoleMappingFileSettingsIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,6 @@
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingAction;
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequest;
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingRequestBuilder;
import org.elasticsearch.xpack.core.security.action.rolemapping.PutRoleMappingResponse;
import org.elasticsearch.xpack.core.security.authc.RealmConfig;
import org.elasticsearch.xpack.core.security.authc.support.UserRoleMapper;
import org.elasticsearch.xpack.core.security.authc.support.mapper.ExpressionRoleMapping;
Expand All @@ -59,11 +58,12 @@
import static org.elasticsearch.indices.recovery.RecoverySettings.INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING;
import static org.elasticsearch.xcontent.XContentType.JSON;
import static org.elasticsearch.xpack.core.security.test.TestRestrictedIndices.INTERNAL_SECURITY_MAIN_INDEX_7;
import static org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper.RESERVED_ROLE_MAPPING_SUFFIX;
import static org.hamcrest.Matchers.allOf;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsInAnyOrder;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.empty;
import static org.hamcrest.Matchers.emptyArray;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.hasSize;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -270,28 +270,21 @@ private void assertRoleMappingsSaveOK(CountDownLatch savedClusterState, AtomicLo
assertThat(resolveRolesFuture.get(), containsInAnyOrder("kibana_user", "fleet_user"));
}

// the role mappings are retrievable by the role mapping action for BWC
assertGetResponseHasMappings(true, "everyone_kibana", "everyone_fleet");

// role mappings (with the same names) can be stored in the "native" store
{
PutRoleMappingResponse response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana"))
.actionGet();
assertTrue(response.isCreated());
response = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
assertTrue(response.isCreated());
}
{
// deleting role mappings that exist in the native store and in cluster-state should result in success
var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet();
assertTrue(response.isFound());
response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_fleet")).actionGet();
assertTrue(response.isFound());
}

// the role mappings are not retrievable by the role mapping action (which only accesses "native" i.e. index-based role mappings)
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertFalse(response.hasMappings());
assertThat(response.mappings(), emptyArray());

// role mappings (with the same names) can also be stored in the "native" store
var putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_kibana")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
putRoleMappingResponse = client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest("everyone_fleet")).actionGet();
assertTrue(putRoleMappingResponse.isCreated());
}

public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
public void testRoleMappingsApplied() throws Exception {
ensureGreen();

var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
Expand All @@ -300,12 +293,6 @@ public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
assertRoleMappingsSaveOK(savedClusterState.v1(), savedClusterState.v2());
logger.info("---> cleanup cluster settings...");

{
// Deleting non-existent native role mappings returns not found even if they exist in config file
var response = client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).get();
assertFalse(response.isFound());
}

savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());

writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
Expand All @@ -320,95 +307,48 @@ public void testClusterStateRoleMappingsAddedThenDeleted() throws Exception {
clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
);

// cluster-state role mapping was removed and is not returned in the API anymore
// native role mappings are not affected by the removal of the cluster-state based ones
{
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertFalse(response.hasMappings());
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder("everyone_kibana", "everyone_fleet")
);
}

// no role mappings means no roles are resolved
// and roles are resolved based on the native role mappings
for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
userRoleMapper.resolveRoles(
new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
resolveRolesFuture
);
assertThat(resolveRolesFuture.get(), empty());
assertThat(resolveRolesFuture.get(), contains("kibana_user_native"));
}
}

public void testGetRoleMappings() throws Exception {
ensureGreen();

final List<String> nativeMappings = List.of("everyone_kibana", "_everyone_kibana", "zzz_mapping", "123_mapping");
for (var mapping : nativeMappings) {
client().execute(PutRoleMappingAction.INSTANCE, sampleRestRequest(mapping)).actionGet();
{
var request = new DeleteRoleMappingRequest();
request.setName("everyone_kibana");
var response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
assertTrue(response.isFound());
request = new DeleteRoleMappingRequest();
request.setName("everyone_fleet");
response = client().execute(DeleteRoleMappingAction.INSTANCE, request).get();
assertTrue(response.isFound());
}

var savedClusterState = setupClusterStateListener(internalCluster().getMasterName(), "everyone_kibana");
writeJSONFile(internalCluster().getMasterName(), testJSON, logger, versionCounter);
boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

var request = new GetRoleMappingsRequest();
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder(
"everyone_kibana",
"everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX,
"_everyone_kibana",
"everyone_fleet" + RESERVED_ROLE_MAPPING_SUFFIX,
"zzz_mapping",
"123_mapping"
)
);

int readOnlyCount = 0;
// assert that cluster-state role mappings come last
for (ExpressionRoleMapping mapping : response.mappings()) {
readOnlyCount = mapping.getName().endsWith(RESERVED_ROLE_MAPPING_SUFFIX) ? readOnlyCount + 1 : readOnlyCount;
// no roles are resolved now, because both native and cluster-state based stores have been cleared
for (UserRoleMapper userRoleMapper : internalCluster().getInstances(UserRoleMapper.class)) {
PlainActionFuture<Set<String>> resolveRolesFuture = new PlainActionFuture<>();
userRoleMapper.resolveRoles(
new UserRoleMapper.UserData("anyUsername", null, List.of(), Map.of(), mock(RealmConfig.class)),
resolveRolesFuture
);
assertThat(resolveRolesFuture.get(), empty());
}
// Two sourced from cluster-state
assertEquals(readOnlyCount, 2);

// it's possible to delete overlapping native role mapping
assertTrue(client().execute(DeleteRoleMappingAction.INSTANCE, deleteRequest("everyone_kibana")).actionGet().isFound());

// Fetch a specific file based role
request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX);
response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder("everyone_kibana" + RESERVED_ROLE_MAPPING_SUFFIX)
);

savedClusterState = setupClusterStateListenerForCleanup(internalCluster().getMasterName());
writeJSONFile(internalCluster().getMasterName(), emptyJSON, logger, versionCounter);
awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

final ClusterStateResponse clusterStateResponse = clusterAdmin().state(
new ClusterStateRequest(TEST_REQUEST_TIMEOUT).waitForMetadataVersion(savedClusterState.v2().get())
).get();

assertNull(
clusterStateResponse.getState().metadata().persistentSettings().get(INDICES_RECOVERY_MAX_BYTES_PER_SEC_SETTING.getKey())
);

// Make sure remaining native mappings can still be fetched
request = new GetRoleMappingsRequest();
response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder("_everyone_kibana", "zzz_mapping", "123_mapping")
);
}

public static Tuple<CountDownLatch, AtomicLong> setupClusterStateListenerForError(
Expand Down Expand Up @@ -493,8 +433,11 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
boolean awaitSuccessful = savedClusterState.v1().await(20, TimeUnit.SECONDS);
assertTrue(awaitSuccessful);

// even if index is closed, cluster-state role mappings are still returned
assertGetResponseHasMappings(true, "everyone_kibana", "everyone_fleet");
// no native role mappings exist
var request = new GetRoleMappingsRequest();
request.setNames("everyone_kibana", "everyone_fleet");
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertFalse(response.hasMappings());

// cluster state settings are also applied
var clusterStateResponse = clusterAdmin().state(
Expand Down Expand Up @@ -533,12 +476,6 @@ public void testRoleMappingApplyWithSecurityIndexClosed() throws Exception {
}
}

private DeleteRoleMappingRequest deleteRequest(String name) {
var request = new DeleteRoleMappingRequest();
request.setName(name);
return request;
}

private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
var json = """
{
Expand All @@ -557,17 +494,4 @@ private PutRoleMappingRequest sampleRestRequest(String name) throws Exception {
return new PutRoleMappingRequestBuilder(null).source(name, parser).request();
}
}

private static void assertGetResponseHasMappings(boolean readOnly, String... mappings) throws InterruptedException, ExecutionException {
var request = new GetRoleMappingsRequest();
request.setNames(mappings);
var response = client().execute(GetRoleMappingsAction.INSTANCE, request).get();
assertTrue(response.hasMappings());
assertThat(
Arrays.stream(response.mappings()).map(ExpressionRoleMapping::getName).toList(),
containsInAnyOrder(
Arrays.stream(mappings).map(mapping -> mapping + (readOnly ? RESERVED_ROLE_MAPPING_SUFFIX : "")).toArray(String[]::new)
)
);
}
}
3 changes: 1 addition & 2 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -897,8 +897,7 @@ Collection<Object> createComponents(
reservedRealm
);
components.add(nativeUsersStore);
components.add(clusterStateRoleMapper);
components.add(nativeRoleMappingStore);
components.add(new PluginComponentBinding<>(NativeRoleMappingStore.class, nativeRoleMappingStore));
components.add(new PluginComponentBinding<>(UserRoleMapper.class, userRoleMapper));
components.add(reservedRealm);
components.add(realms);
Expand Down
22 changes: 2 additions & 20 deletions ...src/main/java/org/elasticsearch/xpack/security/action/role/TransportDeleteRoleAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,13 @@
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.support.ActionFilters;
import org.elasticsearch.action.support.TransportAction;
import org.elasticsearch.common.logging.HeaderWarning;
import org.elasticsearch.common.util.concurrent.EsExecutors;
import org.elasticsearch.injection.guice.Inject;
import org.elasticsearch.tasks.Task;
import org.elasticsearch.transport.TransportService;
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleAction;
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleRequest;
import org.elasticsearch.xpack.core.security.action.role.DeleteRoleResponse;
import org.elasticsearch.xpack.security.authc.support.mapper.ClusterStateRoleMapper;
import org.elasticsearch.xpack.security.authz.ReservedRoleNameChecker;
import org.elasticsearch.xpack.security.authz.store.NativeRolesStore;

Expand All @@ -27,20 +25,16 @@ public class TransportDeleteRoleAction extends TransportAction<DeleteRoleRequest
private final NativeRolesStore rolesStore;
private final ReservedRoleNameChecker reservedRoleNameChecker;

private final ClusterStateRoleMapper clusterStateRoleMapper;

@Inject
public TransportDeleteRoleAction(
ActionFilters actionFilters,
NativeRolesStore rolesStore,
TransportService transportService,
ReservedRoleNameChecker reservedRoleNameChecker,
ClusterStateRoleMapper clusterStateRoleMapper
ReservedRoleNameChecker reservedRoleNameChecker
) {
super(DeleteRoleAction.NAME, actionFilters, transportService.getTaskManager(), EsExecutors.DIRECT_EXECUTOR_SERVICE);
this.rolesStore = rolesStore;
this.reservedRoleNameChecker = reservedRoleNameChecker;
this.clusterStateRoleMapper = clusterStateRoleMapper;
}

@Override
Expand All @@ -51,19 +45,7 @@ protected void doExecute(Task task, DeleteRoleRequest request, ActionListener<De
}

try {
rolesStore.deleteRole(request, listener.safeMap((found) -> {
if (clusterStateRoleMapper.hasMapping(request.name())) {
// Allow to delete a mapping with the same name in the native role mapping store as the file_settings namespace, but
// add a warning header to signal to the caller that this could be a problem.
HeaderWarning.addWarning(
"A read only role mapping with the same name ["
+ request.name()
+ "] has been previously been defined in a configuration file. "
+ "The read only role mapping will still be active."
);
}
return new DeleteRoleResponse(found);
}));
rolesStore.deleteRole(request, listener.safeMap(DeleteRoleResponse::new));
} catch (Exception e) {
logger.error((Supplier<?>) () -> "failed to delete role [" + request.name() + "]", e);
listener.onFailure(e);
Expand Down
Loading

0 comments on commit 8bfdd72

Please sign in to comment.