diff --git a/docs/changelog/127621.yaml b/docs/changelog/127621.yaml new file mode 100644 index 0000000000000..8fc0c7068be5a --- /dev/null +++ b/docs/changelog/127621.yaml @@ -0,0 +1,5 @@ +pr: 127621 +summary: Fix error message when changing the password for a user in the file realm +area: Security +type: bug +issues: [] diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/esnative/ClientReservedRealm.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/esnative/ClientReservedRealm.java index e78e3c9335817..69dc8d169e2bc 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/esnative/ClientReservedRealm.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/authc/esnative/ClientReservedRealm.java @@ -11,21 +11,32 @@ import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.UsernamesField; +import java.util.Set; + public class ClientReservedRealm { + private static final Set RESERVED_USERNAMES = Set.of( + UsernamesField.ELASTIC_NAME, + UsernamesField.DEPRECATED_KIBANA_NAME, + UsernamesField.KIBANA_NAME, + UsernamesField.LOGSTASH_NAME, + UsernamesField.BEATS_NAME, + UsernamesField.APM_NAME, + UsernamesField.REMOTE_MONITORING_NAME + ); + public static boolean isReserved(String username, Settings settings) { assert username != null; - switch (username) { - case UsernamesField.ELASTIC_NAME: - case UsernamesField.DEPRECATED_KIBANA_NAME: - case UsernamesField.KIBANA_NAME: - case UsernamesField.LOGSTASH_NAME: - case UsernamesField.BEATS_NAME: - case UsernamesField.APM_NAME: - case UsernamesField.REMOTE_MONITORING_NAME: - return XPackSettings.RESERVED_REALM_ENABLED_SETTING.get(settings); - default: - return AnonymousUser.isAnonymousUsername(username, settings); + if (isReservedUsername(username)) { + return XPackSettings.RESERVED_REALM_ENABLED_SETTING.get(settings); } + return AnonymousUser.isAnonymousUsername(username, settings); + } + + /** + * checks membership in a set, doesn't care if the reserved realm is enabled + */ + public static boolean isReservedUsername(String username) { + return RESERVED_USERNAMES.contains(username); } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordAction.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordAction.java index 541bbdddd657e..cb64d5f193cc2 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordAction.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordAction.java @@ -8,9 +8,12 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionResponse; +import org.elasticsearch.action.ActionRunnable; import org.elasticsearch.action.ActionType; import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.action.support.GroupedActionListener; import org.elasticsearch.action.support.HandledTransportAction; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.util.concurrent.EsExecutors; import org.elasticsearch.injection.guice.Inject; @@ -18,28 +21,44 @@ import org.elasticsearch.transport.TransportService; import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.security.action.user.ChangePasswordRequest; +import org.elasticsearch.xpack.core.security.authc.Realm; +import org.elasticsearch.xpack.core.security.authc.esnative.ClientReservedRealm; +import org.elasticsearch.xpack.core.security.authc.esnative.NativeRealmSettings; import org.elasticsearch.xpack.core.security.authc.support.Hasher; import org.elasticsearch.xpack.core.security.user.AnonymousUser; +import org.elasticsearch.xpack.core.security.user.User; +import org.elasticsearch.xpack.security.authc.Realms; import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore; +import org.elasticsearch.xpack.security.authc.esnative.ReservedRealm; +import java.util.List; import java.util.Locale; +import java.util.Objects; +import java.util.Optional; +import java.util.Set; + +import static org.elasticsearch.xpack.core.security.user.UsernamesField.ELASTIC_NAME; +import static org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore.USER_NOT_FOUND_MESSAGE; public class TransportChangePasswordAction extends HandledTransportAction { public static final ActionType TYPE = new ActionType<>("cluster:admin/xpack/security/user/change_password"); private final Settings settings; private final NativeUsersStore nativeUsersStore; + private final Realms realms; @Inject public TransportChangePasswordAction( Settings settings, TransportService transportService, ActionFilters actionFilters, - NativeUsersStore nativeUsersStore + NativeUsersStore nativeUsersStore, + Realms realms ) { super(TYPE.name(), transportService, actionFilters, ChangePasswordRequest::new, EsExecutors.DIRECT_EXECUTOR_SERVICE); this.settings = settings; this.nativeUsersStore = nativeUsersStore; + this.realms = realms; } @Override @@ -62,6 +81,76 @@ protected void doExecute(Task task, ChangePasswordRequest request, ActionListene ); return; } - nativeUsersStore.changePassword(request, listener.safeMap(v -> ActionResponse.Empty.INSTANCE)); + + if (ClientReservedRealm.isReservedUsername(username) && XPackSettings.RESERVED_REALM_ENABLED_SETTING.get(settings) == false) { + // when on cloud and resetting the elastic operator user by mistake + ValidationException validationException = new ValidationException(); + validationException.addValidationError( + "user [" + + username + + "] belongs to the " + + ReservedRealm.NAME + + " realm which is disabled." + + (ELASTIC_NAME.equalsIgnoreCase(username) + ? " In a cloud deployment, the password can be changed through the cloud console." + : "") + ); + listener.onFailure(validationException); + return; + } + + // check if user exists in the native realm + nativeUsersStore.getUser(username, new ActionListener<>() { + @Override + public void onResponse(User user) { + // nativeUsersStore.changePassword can create a missing reserved user, so enter only if not reserved + if (ClientReservedRealm.isReserved(username, settings) == false && user == null) { + List nonNativeRealms = realms.getActiveRealms() + .stream() + .filter(t -> Set.of(NativeRealmSettings.TYPE, ReservedRealm.TYPE).contains(t.type()) == false) // Reserved realm is + // implemented in the + // native store + .toList(); + if (nonNativeRealms.isEmpty()) { + listener.onFailure(createUserNotFoundException()); + return; + } + + GroupedActionListener gal = new GroupedActionListener<>(nonNativeRealms.size(), ActionListener.wrap(users -> { + final Optional nonNativeUser = users.stream().filter(Objects::nonNull).findAny(); + if (nonNativeUser.isPresent()) { + listener.onFailure( + new ValidationException().addValidationError( + "user [" + username + "] does not belong to the native realm and cannot be managed via this API." + ) + ); + } else { + // user wasn't found in any other realm, display standard not-found message + listener.onFailure(createUserNotFoundException()); + } + }, listener::onFailure)); + for (Realm realm : nonNativeRealms) { + EsExecutors.DIRECT_EXECUTOR_SERVICE.execute( + ActionRunnable.wrap(gal, userActionListener -> realm.lookupUser(username, userActionListener)) + ); + } + } else { + // safe to proceed + nativeUsersStore.changePassword(request, listener.safeMap(v -> ActionResponse.Empty.INSTANCE)); + } + } + + @Override + public void onFailure(Exception e) { + listener.onFailure(e); + } + }); + + } + + private static ValidationException createUserNotFoundException() { + ValidationException validationException = new ValidationException(); + validationException.addValidationError(USER_NOT_FOUND_MESSAGE); + return validationException; } } diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java index d866bd2a9d229..d073c0fb8fd85 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java @@ -81,6 +81,7 @@ public class NativeUsersStore { public static final String USER_DOC_TYPE = "user"; public static final String RESERVED_USER_TYPE = "reserved-user"; + public static final String USER_NOT_FOUND_MESSAGE = "user must exist in order to change password"; private static final Logger logger = LogManager.getLogger(NativeUsersStore.class); private final Settings settings; @@ -315,7 +316,7 @@ public void onFailure(Exception e) { } else { logger.debug(() -> format("failed to change password for user [%s]", request.username()), e); ValidationException validationException = new ValidationException(); - validationException.addValidationError("user must exist in order to change password"); + validationException.addValidationError(USER_NOT_FOUND_MESSAGE); listener.onFailure(validationException); } } else { diff --git a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java index 8151034390cf5..44b107944b9ad 100644 --- a/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java +++ b/x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/action/user/TransportChangePasswordActionTests.java @@ -10,6 +10,7 @@ import org.elasticsearch.action.ActionListener; import org.elasticsearch.action.ActionResponse; import org.elasticsearch.action.support.ActionFilters; +import org.elasticsearch.common.ValidationException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.tasks.Task; import org.elasticsearch.test.ESTestCase; @@ -20,16 +21,24 @@ import org.elasticsearch.xpack.core.XPackSettings; import org.elasticsearch.xpack.core.security.action.user.ChangePasswordRequest; import org.elasticsearch.xpack.core.security.authc.AuthenticationTestHelper; +import org.elasticsearch.xpack.core.security.authc.Realm; +import org.elasticsearch.xpack.core.security.authc.ldap.LdapRealmSettings; import org.elasticsearch.xpack.core.security.authc.support.Hasher; import org.elasticsearch.xpack.core.security.user.AnonymousUser; import org.elasticsearch.xpack.core.security.user.ElasticUser; import org.elasticsearch.xpack.core.security.user.KibanaUser; import org.elasticsearch.xpack.core.security.user.User; +import org.elasticsearch.xpack.security.authc.Realms; import org.elasticsearch.xpack.security.authc.esnative.NativeUsersStore; +import org.elasticsearch.xpack.security.authc.file.FileRealm; +import org.elasticsearch.xpack.security.authc.pki.PkiRealm; +import org.mockito.Mockito; import java.util.Collections; +import java.util.List; import java.util.Locale; import java.util.concurrent.atomic.AtomicReference; +import java.util.stream.Stream; import static org.elasticsearch.test.ActionListenerUtils.anyActionListener; import static org.elasticsearch.test.SecurityIntegTestCase.getFastStoredHashAlgoForTests; @@ -39,6 +48,7 @@ import static org.hamcrest.Matchers.notNullValue; import static org.hamcrest.Matchers.nullValue; import static org.hamcrest.Matchers.sameInstance; +import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.mock; @@ -69,7 +79,8 @@ public void testAnonymousUser() { settings, transportService, mock(ActionFilters.class), - usersStore + usersStore, + mockRealms() ); // Request will fail before the request hashing algorithm is checked, but we use the same algorithm as in settings for consistency ChangePasswordRequest request = new ChangePasswordRequest(); @@ -96,6 +107,131 @@ public void onFailure(Exception e) { verifyNoMoreInteractions(usersStore); } + public void testFileRealmUser() { + final Hasher hasher = getFastStoredHashAlgoForTests(); + Settings settings = Settings.builder() + .put(AnonymousUser.ROLES_SETTING.getKey(), "superuser") + .put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name()) + .put(XPackSettings.RESERVED_REALM_ENABLED_SETTING.getKey(), false) // simulate cloud set up + .build(); + ElasticUser elasticUser = new ElasticUser(true); + NativeUsersStore usersStore = mock(NativeUsersStore.class); + + TransportService transportService = new TransportService( + Settings.EMPTY, + mock(Transport.class), + mock(ThreadPool.class), + TransportService.NOOP_TRANSPORT_INTERCEPTOR, + x -> null, + null, + Collections.emptySet() + ); + TransportChangePasswordAction action = new TransportChangePasswordAction( + settings, + transportService, + mock(ActionFilters.class), + usersStore, + mockRealms(mockFileRealm(elasticUser)) + ); + ChangePasswordRequest request = new ChangePasswordRequest(); + request.username(elasticUser.principal()); + request.passwordHash(hasher.hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); + + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(null); + return null; + }).when(usersStore).getUser(eq(request.username()), anyActionListener()); + + final AtomicReference throwableRef = new AtomicReference<>(); + final AtomicReference responseRef = new AtomicReference<>(); + action.doExecute(mock(Task.class), request, new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty changePasswordResponse) { + responseRef.set(changePasswordResponse); + } + + @Override + public void onFailure(Exception e) { + throwableRef.set(e); + } + }); + + assertThat(responseRef.get(), is(nullValue())); + assertThat(throwableRef.get(), instanceOf(ValidationException.class)); + assertThat( + throwableRef.get().getMessage(), + containsString("In a cloud deployment, the password can be changed through the " + "cloud console.") + ); + verify(usersStore, times(0)).changePassword(any(ChangePasswordRequest.class), anyActionListener()); + } + + public void testUserPresentInNonNativeRealms() { + // place a user 'foo' in both file and pki realms. it will not be found in the native realm. + final Hasher hasher = getFastStoredHashAlgoForTests(); + Settings settings = Settings.builder() + .put(AnonymousUser.ROLES_SETTING.getKey(), "superuser") + .put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name()) + .put(XPackSettings.RESERVED_REALM_ENABLED_SETTING.getKey(), true) + .build(); + User foo = new User("foo", "admin"); + NativeUsersStore usersStore = mock(NativeUsersStore.class); + + TransportService transportService = new TransportService( + Settings.EMPTY, + mock(Transport.class), + mock(ThreadPool.class), + TransportService.NOOP_TRANSPORT_INTERCEPTOR, + x -> null, + null, + Collections.emptySet() + ); + TransportChangePasswordAction action = new TransportChangePasswordAction( + settings, + transportService, + mock(ActionFilters.class), + usersStore, + mockRealms(mockFileRealm(foo), mockPkiRealm(foo)) + ); + ChangePasswordRequest request = new ChangePasswordRequest(); + request.username(foo.principal()); + request.passwordHash(hasher.hash(SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING)); + + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(null); + return null; + }).when(usersStore).getUser(eq(request.username()), anyActionListener()); + + final AtomicReference throwableRef = new AtomicReference<>(); + final AtomicReference responseRef = new AtomicReference<>(); + action.doExecute(mock(Task.class), request, new ActionListener<>() { + @Override + public void onResponse(ActionResponse.Empty changePasswordResponse) { + responseRef.set(changePasswordResponse); + } + + @Override + public void onFailure(Exception e) { + throwableRef.set(e); + } + }); + + assertThat(responseRef.get(), is(nullValue())); + assertThat(throwableRef.get(), instanceOf(ValidationException.class)); + assertThat( + throwableRef.get().getMessage(), + containsString("user [" + foo.principal() + "] does not belong to the native realm and cannot be managed via this API.") + ); + verify(usersStore, times(0)).changePassword(any(ChangePasswordRequest.class), anyActionListener()); + } + public void testValidUser() { testValidUser(randomFrom(new ElasticUser(true), new KibanaUser(true), new User("joe"))); } @@ -127,12 +263,21 @@ private void testValidUser(User user) { null, Collections.emptySet() ); + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(user); + return null; + }).when(usersStore).getUser(eq(request.username()), anyActionListener()); Settings passwordHashingSettings = Settings.builder().put(XPackSettings.PASSWORD_HASHING_ALGORITHM.getKey(), hasher.name()).build(); TransportChangePasswordAction action = new TransportChangePasswordAction( passwordHashingSettings, transportService, mock(ActionFilters.class), - usersStore + usersStore, + mockRealms() ); final AtomicReference throwableRef = new AtomicReference<>(); final AtomicReference responseRef = new AtomicReference<>(); @@ -177,7 +322,8 @@ public void testWithPasswordThatsNotAHash() { passwordHashingSettings, transportService, mock(ActionFilters.class), - usersStore + usersStore, + mockRealms() ); action.doExecute(mock(Task.class), request, new ActionListener<>() { @Override @@ -215,6 +361,14 @@ public void testWithDifferentPasswordHashingAlgorithm() { listener.onResponse(null); return null; }).when(usersStore).changePassword(eq(request), anyActionListener()); + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(user); + return null; + }).when(usersStore).getUser(eq(request.username()), anyActionListener()); final AtomicReference throwableRef = new AtomicReference<>(); final AtomicReference responseRef = new AtomicReference<>(); TransportService transportService = new TransportService( @@ -235,7 +389,8 @@ public void testWithDifferentPasswordHashingAlgorithm() { passwordHashingSettings, transportService, mock(ActionFilters.class), - usersStore + usersStore, + mockRealms() ); action.doExecute(mock(Task.class), request, new ActionListener<>() { @Override @@ -271,6 +426,14 @@ public void testException() { listener.onFailure(e); return null; }).when(usersStore).changePassword(eq(request), anyActionListener()); + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(user); + return null; + }).when(usersStore).getUser(eq(request.username()), anyActionListener()); TransportService transportService = new TransportService( Settings.EMPTY, mock(Transport.class), @@ -285,7 +448,8 @@ public void testException() { passwordHashingSettings, transportService, mock(ActionFilters.class), - usersStore + usersStore, + mockRealms() ); final AtomicReference throwableRef = new AtomicReference<>(); final AtomicReference responseRef = new AtomicReference<>(); @@ -306,4 +470,46 @@ public void onFailure(Exception e) { assertThat(throwableRef.get(), sameInstance(e)); verify(usersStore, times(1)).changePassword(eq(request), anyActionListener()); } + + private static FileRealm mockFileRealm(User fileRealmUser) { + + FileRealm fileRealm = Mockito.mock(FileRealm.class); + Mockito.when(fileRealm.type()).thenReturn("file"); + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(fileRealmUser); + return null; + }).when(fileRealm).lookupUser(Mockito.any(String.class), anyActionListener()); + + return fileRealm; + } + + private Realm mockPkiRealm(User foo) { + PkiRealm ldapRealm = Mockito.mock(PkiRealm.class); + Mockito.when(ldapRealm.type()).thenReturn(LdapRealmSettings.LDAP_TYPE); + doAnswer(invocation -> { + Object[] args = invocation.getArguments(); + assert args.length == 2; + @SuppressWarnings("unchecked") + ActionListener listener = (ActionListener) args[1]; + listener.onResponse(foo); + return null; + }).when(ldapRealm).lookupUser(Mockito.any(String.class), anyActionListener()); + + return ldapRealm; + } + + private static Realms mockRealms() { + return mockRealms(mockFileRealm(null)); + } + + private static Realms mockRealms(Realm... realm) { + Realms realms = mock(Realms.class); + Mockito.when(realms.stream()).thenReturn(realm == null ? Stream.of() : Stream.of(realm)); + Mockito.when(realms.getActiveRealms()).thenReturn(realm == null ? List.of() : List.of(realm)); + return realms; + } }