Skip to content
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.

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

Resolve anonymous roles and deduplicate roles during authentication #53453

Merged
merged 25 commits into from
Apr 30, 2020
Merged
Show file tree
Hide file tree
Changes from 24 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
8dac757
Move anonymous role merging from authz to authc
ywangd Mar 20, 2020
09df4fb
Update rest tests
ywangd Mar 20, 2020
e24072a
Add unit tests for authenticationService
ywangd Mar 20, 2020
2cf8516
Remove unnecessary authorizationService test
ywangd Mar 20, 2020
8c5531b
Fix test
ywangd Mar 20, 2020
903b805
style
ywangd Mar 20, 2020
c171757
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Mar 24, 2020
c43f8ba
Address feedback
ywangd Mar 24, 2020
857c9e4
Checkstyle
ywangd Mar 24, 2020
1deaa58
Remove extra empty line
ywangd Mar 24, 2020
fe6eae8
Address feedback
ywangd Mar 24, 2020
2fcbfad
Address feedback
ywangd Mar 24, 2020
72d2667
Fix tests
ywangd Mar 24, 2020
65faa2c
Fix typo
ywangd Mar 24, 2020
c686985
Refactor for clarity
ywangd Mar 24, 2020
faa0471
Refactor for more predictable behaviour
ywangd Mar 24, 2020
384aa17
Address feedback
ywangd Mar 26, 2020
6fa37e4
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Mar 26, 2020
6fd8249
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Mar 26, 2020
cc7f259
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Apr 8, 2020
9a8bd63
Deduplicate roles for file and native realms earlier in the chain
ywangd Apr 8, 2020
6af416c
Remove unnecessary tests
ywangd Apr 8, 2020
2892218
Update tests
ywangd Apr 8, 2020
c6a8aa1
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Apr 22, 2020
c63c49f
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Apr 30, 2020
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
Original file line number Diff line number Diff line change
Expand Up @@ -34,4 +34,4 @@ public void writeTo(StreamOutput out) throws IOException {
authentication.writeTo(out);
}

}
}
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,10 @@ public boolean isRunAs() {
return authenticatedUser != null;
}

public User withRoles(String[] newRoles) {
return new User(username, newRoles, fullName, email, metadata, enabled);
}

@Override
public String toString() {
StringBuilder sb = new StringBuilder();
Expand Down Expand Up @@ -209,6 +213,10 @@ public static void writeTo(User user, StreamOutput output) throws IOException {
output.writeBoolean(false); // last user written, regardless of bwc, does not have an inner user
}

public static boolean isInternal(User user) {
return SystemUser.is(user) || XPackUser.is(user) || XPackSecurityUser.is(user) || AsyncSearchUser.is(user);
}

/** Write just the given {@link User}, but not the inner {@link #authenticatedUser}. */
private static void writeUser(User user, StreamOutput output) throws IOException {
output.writeBoolean(false); // not a system user
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@
import java.util.Map;

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

private void checkAllowedWrite(String indexName) throws IOException {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -49,11 +49,14 @@
import org.elasticsearch.xpack.security.support.SecurityIndexManager;

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.LinkedHashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;
import java.util.concurrent.atomic.AtomicLong;
import java.util.function.BiConsumer;
import java.util.function.Consumer;
Expand Down Expand Up @@ -656,7 +659,8 @@ void finishAuthentication(User finalUser) {
logger.debug("user [{}] is disabled. failing authentication", finalUser);
listener.onFailure(request.authenticationFailed(authenticationToken));
} else {
final Authentication finalAuth = new Authentication(finalUser, authenticatedBy, lookedupBy);
final Authentication finalAuth = new Authentication(
maybeConsolidateRolesForUser(finalUser), authenticatedBy, lookedupBy);
writeAuthToContext(finalAuth);
}
}
Expand Down Expand Up @@ -689,6 +693,33 @@ void writeAuthToContext(Authentication authentication) {
private void authenticateToken(AuthenticationToken token) {
this.consumeToken(token);
}

private User maybeConsolidateRolesForUser(User user) {
if (User.isInternal(user)) {
return user;
} else if (isAnonymousUserEnabled && anonymousUser.equals(user) == false) {
if (anonymousUser.roles().length == 0) {
throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles");
}
User userWithMergedRoles = user.withRoles(mergeRoles(user.roles(), anonymousUser.roles()));
if (user.isRunAs()) {
final User authUserWithMergedRoles = user.authenticatedUser().withRoles(
mergeRoles(user.authenticatedUser().roles(), anonymousUser.roles()));
userWithMergedRoles = new User(userWithMergedRoles, authUserWithMergedRoles);
}
return userWithMergedRoles;
} else {
return user;
}
}

private String[] mergeRoles(String[] existingRoles, String[] otherRoles) {
Set<String> roles = new LinkedHashSet<>(Arrays.asList(existingRoles));
if (otherRoles != null) {
Collections.addAll(roles, otherRoles);
}
return roles.toArray(new String[0]);
}
}

abstract static class AuditableRequest {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -24,21 +24,21 @@
*/
public class NativeRealm extends CachingUsernamePasswordRealm {

private final NativeUsersStore userStore;
private final NativeUsersStore usersStore;

public NativeRealm(RealmConfig config, NativeUsersStore usersStore, ThreadPool threadPool) {
super(config, threadPool);
this.userStore = usersStore;
this.usersStore = usersStore;
}

@Override
protected void doLookupUser(String username, ActionListener<User> listener) {
userStore.getUser(username, listener);
usersStore.getUser(username, listener);
}

@Override
protected void doAuthenticate(UsernamePasswordToken token, ActionListener<AuthenticationResult> listener) {
userStore.verifyPassword(token.principal(), token.credentials(), listener);
usersStore.verifyPassword(token.principal(), token.credentials(), listener);
}

public void onSecurityIndexStateChange(SecurityIndexManager.State previousState, SecurityIndexManager.State currentState) {
Expand All @@ -50,7 +50,7 @@ public void onSecurityIndexStateChange(SecurityIndexManager.State previousState,
@Override
public void usageStats(ActionListener<Map<String, Object>> listener) {
super.usageStats(ActionListener.wrap(stats ->
userStore.getUserCount(ActionListener.wrap(size -> {
usersStore.getUserCount(ActionListener.wrap(size -> {
stats.put("size", size);
listener.onResponse(stats);
}, listener::onFailure))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;
Expand Down Expand Up @@ -643,7 +644,7 @@ private UserAndPassword transformUser(final String id, final Map<String, Object>
final String username = id.substring(USER_DOC_TYPE.length() + 1);
try {
String password = (String) sourceMap.get(Fields.PASSWORD.getPreferredName());
String[] roles = ((List<String>) sourceMap.get(Fields.ROLES.getPreferredName())).toArray(Strings.EMPTY_ARRAY);
String[] roles = new LinkedHashSet<>((List<String>)sourceMap.get(Fields.ROLES.getPreferredName())).toArray(Strings.EMPTY_ARRAY);
String fullName = (String) sourceMap.get(Fields.FULL_NAME.getPreferredName());
String email = (String) sourceMap.get(Fields.EMAIL.getPreferredName());
Boolean enabled = (Boolean) sourceMap.get(Fields.ENABLED.getPreferredName());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
Expand Down Expand Up @@ -164,7 +165,7 @@ public static Map<String, String[]> parseFile(Path path, @Nullable Logger logger

Map<String, String[]> usersRoles = new HashMap<>();
for (Map.Entry<String, List<String>> entry : userToRoles.entrySet()) {
usersRoles.put(entry.getKey(), entry.getValue().toArray(new String[entry.getValue().size()]));
usersRoles.put(entry.getKey(), new LinkedHashSet<>(entry.getValue()).toArray(new String[0]));
}

logger.debug("parsed [{}] user to role mappings from file [{}]", usersRoles.size(), path.toAbsolutePath());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,11 +59,8 @@
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
import org.elasticsearch.xpack.core.security.user.XPackUser;
import org.elasticsearch.xpack.security.audit.AuditLevel;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;
Expand Down Expand Up @@ -172,7 +169,7 @@ public void authorize(final Authentication authentication, final String action,
if (auditId == null) {
// We would like to assert that there is an existing request-id, but if this is a system action, then that might not be
// true because the request-id is generated during authentication
if (isInternalUser(authentication.getUser()) != false) {
if (User.isInternal(authentication.getUser()) != false) {
auditId = AuditUtil.getOrGenerateRequestId(threadContext);
} else {
auditTrailService.get().tamperedRequest(null, authentication.getUser(), action, originalRequest);
Expand Down Expand Up @@ -366,7 +363,7 @@ AuthorizationEngine getAuthorizationEngine(final Authentication authentication)

private AuthorizationEngine getAuthorizationEngineForUser(final User user) {
if (rbacEngine != authorizationEngine && licenseState.isSecurityEnabled() && licenseState.isAuthorizationEngineAllowed()) {
if (ClientReservedRealm.isReserved(user.principal(), settings) || isInternalUser(user)) {
if (ClientReservedRealm.isReserved(user.principal(), settings) || User.isInternal(user)) {
return rbacEngine;
} else {
return authorizationEngine;
Expand Down Expand Up @@ -417,10 +414,6 @@ private TransportRequest maybeUnwrapRequest(Authentication authentication, Trans
return request;
}

private boolean isInternalUser(User user) {
return SystemUser.is(user) || XPackUser.is(user) || XPackSecurityUser.is(user) || AsyncSearchUser.is(user);
}

private void authorizeRunAs(final RequestInfo requestInfo, final AuthorizationInfo authzInfo,
final ActionListener<AuthorizationResult> listener) {
final Authentication authentication = requestInfo.getAuthentication();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,6 @@
import org.elasticsearch.xpack.core.security.authz.store.RoleRetrievalResult;
import org.elasticsearch.xpack.core.security.support.CacheIteratorHelper;
import org.elasticsearch.xpack.core.security.support.MetadataUtils;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
Expand Down Expand Up @@ -100,9 +99,7 @@ public class CompositeRolesStore {
private final DocumentSubsetBitsetCache dlsBitsetCache;
private final ThreadContext threadContext;
private final AtomicLong numInvalidation = new AtomicLong();
private final AnonymousUser anonymousUser;
private final ApiKeyService apiKeyService;
private final boolean isAnonymousEnabled;
private final List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> builtInRoleProviders;
private final List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> allRoleProviders;

Expand Down Expand Up @@ -145,8 +142,6 @@ public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, Nat
allList.addAll(rolesProviders);
this.allRoleProviders = Collections.unmodifiableList(allList);
}
this.anonymousUser = new AnonymousUser(settings);
this.isAnonymousEnabled = AnonymousUser.isAnonymousEnabled(settings);
}

public void roles(Set<String> roleNames, ActionListener<Role> roleActionListener) {
Expand Down Expand Up @@ -236,13 +231,6 @@ public void getRoles(User user, Authentication authentication, ActionListener<Ro
}, roleActionListener::onFailure));
} else {
Set<String> roleNames = new HashSet<>(Arrays.asList(user.roles()));
if (isAnonymousEnabled && anonymousUser.equals(user) == false) {
if (anonymousUser.roles().length == 0) {
throw new IllegalStateException("anonymous is only enabled when the anonymous user has roles");
}
Collections.addAll(roleNames, anonymousUser.roles());
}

if (roleNames.isEmpty()) {
roleActionListener.onResponse(Role.EMPTY);
} else if (roleNames.contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName())) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -73,8 +73,11 @@
import org.elasticsearch.xpack.core.security.authz.AuthorizationEngine.EmptyAuthorizationInfo;
import org.elasticsearch.xpack.core.security.index.RestrictedIndicesNames;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
import org.elasticsearch.xpack.core.security.user.AsyncSearchUser;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.XPackSecurityUser;
import org.elasticsearch.xpack.core.security.user.XPackUser;
import org.elasticsearch.xpack.security.audit.AuditTrail;
import org.elasticsearch.xpack.security.audit.AuditTrailService;
import org.elasticsearch.xpack.security.audit.AuditUtil;
Expand Down Expand Up @@ -107,6 +110,7 @@
import static org.elasticsearch.xpack.core.security.support.Exceptions.authenticationError;
import static org.elasticsearch.xpack.security.authc.TokenServiceTests.mockGetTokenFromId;
import static org.hamcrest.Matchers.arrayContaining;
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.hamcrest.Matchers.contains;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.emptyOrNullString;
Expand Down Expand Up @@ -901,6 +905,48 @@ public void testAnonymousUserTransportWithDefaultUser() throws Exception {
assertThreadContextContainsAuthentication(result);
}

public void testInheritAnonymousUserRoles() {
Settings settings = Settings.builder()
.putList(AnonymousUser.ROLES_SETTING.getKey(), "r3", "r4", "r5")
.build();
final AnonymousUser anonymousUser = new AnonymousUser(settings);
service = new AuthenticationService(settings, realms, auditTrailService,
new DefaultAuthenticationFailureHandler(Collections.emptyMap()),
threadPool, anonymousUser, tokenService, apiKeyService);
User user1 = new User("username", "r1", "r2", "r3");
when(firstRealm.token(threadContext)).thenReturn(token);
when(firstRealm.supports(token)).thenReturn(true);
mockAuthenticate(firstRealm, token, user1);
// this call does not actually go async
final AtomicBoolean completed = new AtomicBoolean(false);
service.authenticate(restRequest, true, ActionListener.wrap(authentication -> {
assertThat(authentication.getUser().roles(), arrayContainingInAnyOrder("r1", "r2", "r3", "r4", "r5"));
setCompletedToTrue(completed);
}, this::logAndFail));
assertTrue(completed.get());
}

public void testSystemUsersDoNotInheritAnonymousRoles() {
Settings settings = Settings.builder()
.putList(AnonymousUser.ROLES_SETTING.getKey(), "r3", "r4", "r5")
.build();
final AnonymousUser anonymousUser = new AnonymousUser(settings);
service = new AuthenticationService(settings, realms, auditTrailService,
new DefaultAuthenticationFailureHandler(Collections.emptyMap()),
threadPool, anonymousUser, tokenService, apiKeyService);
when(firstRealm.token(threadContext)).thenReturn(token);
when(firstRealm.supports(token)).thenReturn(true);
final User sysUser = randomFrom(SystemUser.INSTANCE, XPackUser.INSTANCE, XPackSecurityUser.INSTANCE, AsyncSearchUser.INSTANCE);
mockAuthenticate(firstRealm, token, sysUser);
// this call does not actually go async
final AtomicBoolean completed = new AtomicBoolean(false);
service.authenticate(restRequest, true, ActionListener.wrap(authentication -> {
assertThat(authentication.getUser().roles(), equalTo(sysUser.roles()));
setCompletedToTrue(completed);
}, this::logAndFail));
assertTrue(completed.get());
}

public void testRealmTokenThrowingException() throws Exception {
final String reqId = AuditUtil.getOrGenerateRequestId(threadContext);
when(firstRealm.token(threadContext)).thenThrow(authenticationError("realm doesn't like tokens"));
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.CoreMatchers.nullValue;
import static org.hamcrest.Matchers.arrayContainingInAnyOrder;
import static org.mockito.Matchers.any;
import static org.mockito.Mockito.doAnswer;
import static org.mockito.Mockito.mock;
Expand Down Expand Up @@ -153,6 +154,21 @@ public void testVerifyUserWithCorrectPassword() throws Exception {
}

public void testVerifyUserWithIncorrectPassword() throws Exception {
final NativeUsersStore nativeUsersStore = startNativeUsersStore();
final String username = randomAlphaOfLengthBetween(4, 12);
final SecureString password = new SecureString(randomAlphaOfLengthBetween(12, 16).toCharArray());
final List<String> roles = randomList(1, 4, () -> randomAlphaOfLength(12));
roles.add(randomIntBetween(0, roles.size()), roles.get(0));

final PlainActionFuture<AuthenticationResult> future = new PlainActionFuture<>();
nativeUsersStore.verifyPassword(username, password, future);
respondToGetUserRequest(username, password, roles.toArray(new String[0]));

final AuthenticationResult result = future.get();
assertThat(result.getUser().roles(), arrayContainingInAnyOrder(roles.stream().distinct().toArray()));
}

public void testDeduplicateUserRoles() throws Exception {
final NativeUsersStore nativeUsersStore = startNativeUsersStore();
final String username = randomAlphaOfLengthBetween(4, 12);
final SecureString correctPassword = new SecureString(randomAlphaOfLengthBetween(12, 16).toCharArray());
Expand Down
Loading