Skip to content

Commit bf268c5

Browse files
ywangdywangd
authored
Revert "Resolve anonymous roles and deduplicate roles during authentication (#53453)" (#57853)
This reverts commit 3fa765a.
1 parent 2d21c21 commit bf268c5

File tree

14 files changed

+75
-125
lines changed

14 files changed

+75
-125
lines changed

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/action/user/AuthenticateResponse.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -34,4 +34,4 @@ public void writeTo(StreamOutput out) throws IOException {
3434
authentication.writeTo(out);
3535
}
3636

37-
}
37+
}

‎x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/security/user/User.java‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -118,10 +118,6 @@ public boolean isRunAs() {
118118
return authenticatedUser != null;
119119
}
120120

121-
public User withRoles(String[] newRoles) {
122-
return new User(username, newRoles, fullName, email, metadata, enabled);
123-
}
124-
125121
@Override
126122
public String toString() {
127123
StringBuilder sb = new StringBuilder();

‎x-pack/plugin/security/qa/basic-enable-security/src/test/java/org/elasticsearch/xpack/security/EnableSecurityOnBasicLicenseIT.java‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,7 @@
2424
import java.util.Map;
2525

2626
import static org.elasticsearch.xpack.core.security.authc.support.UsernamePasswordToken.basicAuthHeaderValue;
27-
import static org.hamcrest.Matchers.containsInAnyOrder;
27+
import static org.hamcrest.Matchers.contains;
2828
import static org.hamcrest.Matchers.containsString;
2929
import static org.hamcrest.Matchers.equalTo;
3030
import static org.hamcrest.Matchers.notNullValue;
@@ -126,7 +126,7 @@ private void checkAuthentication() throws IOException {
126126
final Map<String, Object> auth = getAsMap("/_security/_authenticate");
127127
// From file realm, configured in build.gradle
128128
assertThat(ObjectPath.evaluate(auth, "username"), equalTo("security_test_user"));
129-
assertThat(ObjectPath.evaluate(auth, "roles"), containsInAnyOrder("security_test_role", "anonymous"));
129+
assertThat(ObjectPath.evaluate(auth, "roles"), contains("security_test_role"));
130130
}
131131

132132
private void checkAllowedWrite(String indexName) throws IOException {

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/AuthenticationService.java‎

Lines changed: 1 addition & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -49,14 +49,11 @@
4949
import org.elasticsearch.xpack.security.support.SecurityIndexManager;
5050

5151
import java.util.ArrayList;
52-
import java.util.Arrays;
5352
import java.util.Collections;
5453
import java.util.LinkedHashMap;
55-
import java.util.LinkedHashSet;
5654
import java.util.List;
5755
import java.util.Map;
5856
import java.util.Objects;
59-
import java.util.Set;
6057
import java.util.concurrent.atomic.AtomicLong;
6158
import java.util.function.BiConsumer;
6259
import java.util.function.Consumer;
@@ -669,8 +666,7 @@ void finishAuthentication(User finalUser) {
669666
logger.debug("user [{}] is disabled. failing authentication", finalUser);
670667
listener.onFailure(request.authenticationFailed(authenticationToken));
671668
} else {
672-
final Authentication finalAuth = new Authentication(
673-
maybeConsolidateRolesForUser(finalUser), authenticatedBy, lookedupBy);
669+
final Authentication finalAuth = new Authentication(finalUser, authenticatedBy, lookedupBy);
674670
writeAuthToContext(finalAuth);
675671
}
676672
}
@@ -703,33 +699,6 @@ void writeAuthToContext(Authentication authentication) {
703699
private void authenticateToken(AuthenticationToken token) {
704700
this.consumeToken(token);
705701
}
706-
707-
private User maybeConsolidateRolesForUser(User user) {
708-
if (User.isInternal(user)) {
709-
return user;
710-
} else if (isAnonymousUserEnabled && anonymousUser.equals(user) == false) {
711-
if (anonymousUser.roles().length == 0) {
712-
throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles");
713-
}
714-
User userWithMergedRoles = user.withRoles(mergeRoles(user.roles(), anonymousUser.roles()));
715-
if (user.isRunAs()) {
716-
final User authUserWithMergedRoles = user.authenticatedUser().withRoles(
717-
mergeRoles(user.authenticatedUser().roles(), anonymousUser.roles()));
718-
userWithMergedRoles = new User(userWithMergedRoles, authUserWithMergedRoles);
719-
}
720-
return userWithMergedRoles;
721-
} else {
722-
return user;
723-
}
724-
}
725-
726-
private String[] mergeRoles(String[] existingRoles, String[] otherRoles) {
727-
Set<String> roles = new LinkedHashSet<>(Arrays.asList(existingRoles));
728-
if (otherRoles != null) {
729-
Collections.addAll(roles, otherRoles);
730-
}
731-
return roles.toArray(new String[0]);
732-
}
733702
}
734703

735704
abstract static class AuditableRequest {

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeRealm.java‎

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -24,21 +24,21 @@
2424
*/
2525
public class NativeRealm extends CachingUsernamePasswordRealm {
2626

27-
private final NativeUsersStore usersStore;
27+
private final NativeUsersStore userStore;
2828

2929
public NativeRealm(RealmConfig config, NativeUsersStore usersStore, ThreadPool threadPool) {
3030
super(config, threadPool);
31-
this.usersStore = usersStore;
31+
this.userStore = usersStore;
3232
}
3333

3434
@Override
3535
protected void doLookupUser(String username, ActionListener<User> listener) {
36-
usersStore.getUser(username, listener);
36+
userStore.getUser(username, listener);
3737
}
3838

3939
@Override
4040
protected void doAuthenticate(UsernamePasswordToken token, ActionListener<AuthenticationResult> listener) {
41-
usersStore.verifyPassword(token.principal(), token.credentials(), listener);
41+
userStore.verifyPassword(token.principal(), token.credentials(), listener);
4242
}
4343

4444
public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) {
@@ -50,7 +50,7 @@ public void onSecurityIndexStateChange(SecurityIndexManager.State previousState,
5050
@Override
5151
public void usageStats(ActionListener<Map<String, Object>> listener) {
5252
super.usageStats(ActionListener.wrap(stats ->
53-
usersStore.getUserCount(ActionListener.wrap(size -> {
53+
userStore.getUserCount(ActionListener.wrap(size -> {
5454
stats.put("size", size);
5555
listener.onResponse(stats);
5656
}, listener::onFailure))

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/esnative/NativeUsersStore.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,6 @@
5757
import java.util.Collection;
5858
import java.util.Collections;
5959
import java.util.HashMap;
60-
import java.util.LinkedHashSet;
6160
import java.util.List;
6261
import java.util.Map;
6362
import java.util.function.Consumer;
@@ -644,7 +643,7 @@ private UserAndPassword transformUser(final String id, final Map<String, Object>
644643
final String username = id.substring(USER_DOC_TYPE.length() + 1);
645644
try {
646645
String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName());
647-
String[] roles = new LinkedHashSet<>((List<String>)sourceMap.get(Fields.ROLES.getPreferredName())).toArray(Strings.EMPTY_ARRAY);
646+
String[] roles = ((List<String>) sourceMap.get(Fields.ROLES.getPreferredName())).toArray(Strings.EMPTY_ARRAY);
648647
String fullName = (String) sourceMap.get(Fields.FULL_NAME.getPreferredName());
649648
String email = (String) sourceMap.get(Fields.EMAIL.getPreferredName());
650649
Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName());

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/file/FileUserRolesStore.java‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import java.util.ArrayList;
3131
import java.util.Collections;
3232
import java.util.HashMap;
33-
import java.util.LinkedHashSet;
3433
import java.util.List;
3534
import java.util.Locale;
3635
import java.util.Map;
@@ -165,7 +164,7 @@ public static Map<String, String[]> parseFile(Path path, @Nullable Logger logger
165164

166165
Map<String, String[]> usersRoles = new HashMap<>();
167166
for (Map.Entry<String, List<String>> entry : userToRoles.entrySet()) {
168-
usersRoles.put(entry.getKey(), new LinkedHashSet<>(entry.getValue()).toArray(new String[0]));
167+
usersRoles.put(entry.getKey(), entry.getValue().toArray(new String[entry.getValue().size()]));
169168
}
170169

171170
logger.debug("parsed [{}] user to role mappings from file [{}]", usersRoles.size(), path.toAbsolutePath());

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java‎

Lines changed: 9 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -60,8 +60,11 @@
6060
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
6161
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
6262
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
63+
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
6364
import org.elasticsearch.xpack.core.security.user.SystemUser;
6465
import org.elasticsearch.xpack.core.security.user.User;
66+
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
67+
import org.elasticsearch.xpack.core.security.user.XPackUser;
6568
import org.elasticsearch.xpack.security.audit.AuditLevel;
6669
import org.elasticsearch.xpack.security.audit.AuditTrail;
6770
import org.elasticsearch.xpack.security.audit.AuditTrailService;
@@ -170,7 +173,7 @@ public void authorize(final Authentication authentication, final String action,
170173
if (auditId == null) {
171174
// We would like to assert that there is an existing request-id, but if this is a system action, then that might not be
172175
// true because the request-id is generated during authentication
173-
if (User.isInternal(authentication.getUser()) != false) {
176+
if (isInternalUser(authentication.getUser()) != false) {
174177
auditId = AuditUtil.getOrGenerateRequestId(threadContext);
175178
} else {
176179
auditTrailService.get().tamperedRequest(null, authentication.getUser(), action, originalRequest);
@@ -365,7 +368,7 @@ AuthorizationEngine getAuthorizationEngine(final Authentication authentication)
365368
private AuthorizationEngine getAuthorizationEngineForUser(final User user) {
366369
if (rbacEngine != authorizationEngine && licenseState.isSecurityEnabled() &&
367370
licenseState.isAllowed(Feature.SECURITY_AUTHORIZATION_ENGINE)) {
368-
if (ClientReservedRealm.isReserved(user.principal(), settings) || User.isInternal(user)) {
371+
if (ClientReservedRealm.isReserved(user.principal(), settings) || isInternalUser(user)) {
369372
return rbacEngine;
370373
} else {
371374
return authorizationEngine;
@@ -416,6 +419,10 @@ private TransportRequest maybeUnwrapRequest(Authentication authentication, Trans
416419
return request;
417420
}
418421

422+
private boolean isInternalUser(User user) {
423+
return SystemUser.is(user) || XPackUser.is(user) || XPackSecurityUser.is(user) || AsyncSearchUser.is(user);
424+
}
425+
419426
private void authorizeRunAs(final RequestInfo requestInfo, final AuthorizationInfo authzInfo,
420427
final ActionListener<AuthorizationResult> listener) {
421428
final Authentication authentication = requestInfo.getAuthentication();

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,6 +44,7 @@
4444
import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult;
4545
import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper;
4646
import org.elasticsearch.xpack.core.security.support.MetadataUtils;
47+
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
4748
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
4849
import org.elasticsearch.xpack.core.security.user.SystemUser;
4950
import org.elasticsearch.xpack.core.security.user.User;
@@ -100,7 +101,9 @@ public class CompositeRolesStore {
100101
private final DocumentSubsetBitsetCache dlsBitsetCache;
101102
private final ThreadContext threadContext;
102103
private final AtomicLong numInvalidation = new AtomicLong();
104+
private final AnonymousUser anonymousUser;
103105
private final ApiKeyService apiKeyService;
106+
private final boolean isAnonymousEnabled;
104107
private final List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> builtInRoleProviders;
105108
private final List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> allRoleProviders;
106109

@@ -143,6 +146,8 @@ public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, Nat
143146
allList.addAll(rolesProviders);
144147
this.allRoleProviders = Collections.unmodifiableList(allList);
145148
}
149+
this.anonymousUser = new AnonymousUser(settings);
150+
this.isAnonymousEnabled = AnonymousUser.isAnonymousEnabled(settings);
146151
}
147152

148153
public void roles(Set<String> roleNames, ActionListener<Role> roleActionListener) {
@@ -232,6 +237,13 @@ public void getRoles(User user, Authentication authentication, ActionListener<Ro
232237
}, roleActionListener::onFailure));
233238
} else {
234239
Set<String> roleNames = new HashSet<>(Arrays.asList(user.roles()));
240+
if (isAnonymousEnabled && anonymousUser.equals(user) == false) {
241+
if (anonymousUser.roles().length == 0) {
242+
throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles");
243+
}
244+
Collections.addAll(roleNames, anonymousUser.roles());
245+
}
246+
235247
if (roleNames.isEmpty()) {
236248
roleActionListener.onResponse(Role.EMPTY);
237249
} else if (roleNames.contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName())) {

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/AuthenticationServiceTests.java‎

Lines changed: 0 additions & 46 deletions
Original file line numberDiff line numberDiff line change
@@ -74,11 +74,8 @@
7474
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.EmptyAuthorizationInfo;
7575
import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames;
7676
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
77-
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
7877
import org.elasticsearch.xpack.core.security.user.SystemUser;
7978
import org.elasticsearch.xpack.core.security.user.User;
80-
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
81-
import org.elasticsearch.xpack.core.security.user.XPackUser;
8279
import org.elasticsearch.xpack.security.audit.AuditTrail;
8380
import org.elasticsearch.xpack.security.audit.AuditTrailService;
8481
import org.elasticsearch.xpack.security.audit.AuditUtil;
@@ -111,7 +108,6 @@
111108
import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError;
112109
import static org.elasticsearch.xpack.security.authc.TokenServiceTests.mockGetTokenFromId;
113110
import static org.hamcrest.Matchers.arrayContaining;
114-
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
115111
import static org.hamcrest.Matchers.contains;
116112
import static org.hamcrest.Matchers.containsString;
117113
import static org.hamcrest.Matchers.emptyOrNullString;
@@ -906,48 +902,6 @@ public void testAnonymousUserTransportWithDefaultUser() throws Exception {
906902
assertThreadContextContainsAuthentication(result);
907903
}
908904

909-
public void testInheritAnonymousUserRoles() {
910-
Settings settings = Settings.builder()
911-
.putList(AnonymousUser.ROLES_SETTING.getKey(), "r3", "r4", "r5")
912-
.build();
913-
final AnonymousUser anonymousUser = new AnonymousUser(settings);
914-
service = new AuthenticationService(settings, realms, auditTrailService,
915-
new DefaultAuthenticationFailureHandler(Collections.emptyMap()),
916-
threadPool, anonymousUser, tokenService, apiKeyService);
917-
User user1 = new User("username", "r1", "r2", "r3");
918-
when(firstRealm.token(threadContext)).thenReturn(token);
919-
when(firstRealm.supports(token)).thenReturn(true);
920-
mockAuthenticate(firstRealm, token, user1);
921-
// this call does not actually go async
922-
final AtomicBoolean completed = new AtomicBoolean(false);
923-
service.authenticate(restRequest, true, ActionListener.wrap(authentication -> {
924-
assertThat(authentication.getUser().roles(), arrayContainingInAnyOrder("r1", "r2", "r3", "r4", "r5"));
925-
setCompletedToTrue(completed);
926-
}, this::logAndFail));
927-
assertTrue(completed.get());
928-
}
929-
930-
public void testSystemUsersDoNotInheritAnonymousRoles() {
931-
Settings settings = Settings.builder()
932-
.putList(AnonymousUser.ROLES_SETTING.getKey(), "r3", "r4", "r5")
933-
.build();
934-
final AnonymousUser anonymousUser = new AnonymousUser(settings);
935-
service = new AuthenticationService(settings, realms, auditTrailService,
936-
new DefaultAuthenticationFailureHandler(Collections.emptyMap()),
937-
threadPool, anonymousUser, tokenService, apiKeyService);
938-
when(firstRealm.token(threadContext)).thenReturn(token);
939-
when(firstRealm.supports(token)).thenReturn(true);
940-
final User sysUser = randomFrom(SystemUser.INSTANCE, XPackUser.INSTANCE, XPackSecurityUser.INSTANCE, AsyncSearchUser.INSTANCE);
941-
mockAuthenticate(firstRealm, token, sysUser);
942-
// this call does not actually go async
943-
final AtomicBoolean completed = new AtomicBoolean(false);
944-
service.authenticate(restRequest, true, ActionListener.wrap(authentication -> {
945-
assertThat(authentication.getUser().roles(), equalTo(sysUser.roles()));
946-
setCompletedToTrue(completed);
947-
}, this::logAndFail));
948-
assertTrue(completed.get());
949-
}
950-
951905
public void testRealmTokenThrowingException() throws Exception {
952906
final String reqId = AuditUtil.getOrGenerateRequestId(threadContext);
953907
when(firstRealm.token(threadContext)).thenThrow(authenticationError("realm doesn't like tokens"));

0 commit comments

Comments
 (0)