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.

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

Report anonymous roles in authenticate response #61355

Merged
merged 3 commits into from
Aug 24, 2020
Merged

Report anonymous roles in authenticate response #61355

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4d2fd93
Report anonymous roles in authenticate response
ywangd Aug 20, 2020
1559790
Address feedback
ywangd Aug 24, 2020
b36efcb
Merge remote-tracking branch 'origin/master' into es-47195-anonymous-…
ywangd Aug 24, 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
2 changes: 1 addition & 1 deletion ...curity/src/test/java/org/elasticsearch/xpack/security/EnableSecurityOnBasicLicenseIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -132,7 +132,7 @@ private void checkAuthentication() throws IOException {
final Map<String, Object> auth = getAsMap("/_security/_authenticate");
// From file realm, configured in build.gradle
Copy link
Member

Choose a reason for hiding this comment

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

nit: add a hint that anonymous user is enabled maybe so the test makes more sense when you look at it ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Add a comment about anonymous access is 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"), contains("security_test_role", "anonymous"));
}

private void checkAllowedWrite(String indexName) throws IOException {
Expand Down
1 change: 1 addition & 0 deletions x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
final NativeRoleMappingStore nativeRoleMappingStore = new NativeRoleMappingStore(settings, client, securityIndex.get(),
scriptService);
final AnonymousUser anonymousUser = new AnonymousUser(settings);
components.add(anonymousUser);
final ReservedRealm reservedRealm = new ReservedRealm(environment, settings, nativeUsersStore,
anonymousUser, securityIndex.get(), threadPool);
final SecurityExtension.SecurityComponents extensionComponents = new ExtensionComponents(environment, client, clusterService,
Expand Down
35 changes: 33 additions & 2 deletions ...c/main/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateAction.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,24 @@
import org.elasticsearch.xpack.core.security.action.user.AuthenticateRequest;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.user.AnonymousUser;
import org.elasticsearch.xpack.core.security.user.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.XPackUser;

import java.util.stream.Stream;

public class TransportAuthenticateAction extends HandledTransportAction<AuthenticateRequest, AuthenticateResponse> {

private final SecurityContext securityContext;
private final AnonymousUser anonymousUser;

@Inject
public TransportAuthenticateAction(TransportService transportService, ActionFilters actionFilters, SecurityContext securityContext) {
public TransportAuthenticateAction(TransportService transportService, ActionFilters actionFilters, SecurityContext securityContext,
AnonymousUser anonymousUser) {
super(AuthenticateAction.NAME, transportService, actionFilters, AuthenticateRequest::new);
this.securityContext = securityContext;
this.anonymousUser = anonymousUser;
}

@Override
Expand All @@ -43,7 +49,32 @@ protected void doExecute(Task task, AuthenticateRequest request, ActionListener<
} else if (SystemUser.is(runAsUser) || XPackUser.is(runAsUser)) {
listener.onFailure(new IllegalArgumentException("user [" + runAsUser.principal() + "] is internal"));
} else {
listener.onResponse(new AuthenticateResponse(authentication));
final User user = authentication.getUser();
final boolean shouldAddAnonymousRoleNames = anonymousUser.enabled() && false == anonymousUser.equals(user)
&& authentication.getAuthenticationType() != Authentication.AuthenticationType.API_KEY;
if (shouldAddAnonymousRoleNames) {
final String[] allRoleNames = Stream.concat(
Stream.of(user.roles()), Stream.of(anonymousUser.roles())).toArray(String[]::new);
listener.onResponse(new AuthenticateResponse(
new Authentication(
Copy link
Member

Choose a reason for hiding this comment

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

This is rather verbose but I don't have any good suggestions to do otherwise and we don't seem to have a generic need for a cloneWithChanges method !

Copy link
Member Author

Choose a reason for hiding this comment

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

I initially had this logic inside the User class, i.e. User#withRoles with the intention to simply things here. But as you said, there is no generic need for this type of method, which made it seem out of place. So I decided to keep everything in this one class. It is verbose but at least self-contained and hopefully easy to understand.

new User(new User(
user.principal(),
allRoleNames,
user.fullName(),
user.email(),
user.metadata(),
user.enabled()
), user.authenticatedUser()),
authentication.getAuthenticatedBy(),
authentication.getLookedUpBy(),
authentication.getVersion(),
authentication.getAuthenticationType(),
authentication.getMetadata()
)
));
} else {
listener.onResponse(new AuthenticateResponse(authentication));
}
}
}
}
38 changes: 34 additions & 4 deletions ...t/java/org/elasticsearch/xpack/security/action/user/TransportAuthenticateActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,13 +17,15 @@
import org.elasticsearch.xpack.core.security.action.user.AuthenticateRequest;
import org.elasticsearch.xpack.core.security.action.user.AuthenticateResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
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.SystemUser;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.core.security.user.XPackUser;

import java.util.Collections;
import java.util.List;
import java.util.concurrent.atomic.AtomicReference;

import static org.hamcrest.Matchers.containsString;
Expand All @@ -44,7 +46,7 @@ public void testInternalUser() {
TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null,
TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet());
TransportAuthenticateAction action = new TransportAuthenticateAction(transportService,
mock(ActionFilters.class), securityContext);
mock(ActionFilters.class), securityContext, prepareAnonymousUser());

final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
final AtomicReference<AuthenticateResponse> responseRef = new AtomicReference<>();
Expand All @@ -70,7 +72,7 @@ public void testNullUser() {
TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null,
TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet());
TransportAuthenticateAction action = new TransportAuthenticateAction(transportService,
mock(ActionFilters.class), securityContext);
mock(ActionFilters.class), securityContext, prepareAnonymousUser());

final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
final AtomicReference<AuthenticateResponse> responseRef = new AtomicReference<>();
Expand Down Expand Up @@ -98,10 +100,12 @@ public void testValidAuthentication(){
SecurityContext securityContext = mock(SecurityContext.class);
when(securityContext.getAuthentication()).thenReturn(authentication);
when(securityContext.getUser()).thenReturn(user);

final AnonymousUser anonymousUser = prepareAnonymousUser();
TransportService transportService = new TransportService(Settings.EMPTY, mock(Transport.class), null,
TransportService.NOOP_TRANSPORT_INTERCEPTOR, x -> null, null, Collections.emptySet());
TransportAuthenticateAction action = new TransportAuthenticateAction(transportService,
mock(ActionFilters.class), securityContext);
mock(ActionFilters.class), securityContext, anonymousUser);

final AtomicReference<Throwable> throwableRef = new AtomicReference<>();
final AtomicReference<AuthenticateResponse> responseRef = new AtomicReference<>();
Expand All @@ -118,7 +122,33 @@ public void onFailure(Exception e) {
});

assertThat(responseRef.get(), notNullValue());
assertThat(responseRef.get().authentication(), sameInstance(authentication));
if (anonymousUser.enabled()) {
final Authentication auth = responseRef.get().authentication();
final User authUser = auth.getUser();
List.of(authUser.roles()).containsAll(List.of(authentication.getUser().roles()));
List.of(authUser.roles()).containsAll(List.of(anonymousUser.roles()));
assertThat(authUser.authenticatedUser(), sameInstance(user.authenticatedUser()));
assertThat(auth.getAuthenticatedBy(), sameInstance(auth.getAuthenticatedBy()));
assertThat(auth.getLookedUpBy(), sameInstance(auth.getLookedUpBy()));
assertThat(auth.getVersion(), sameInstance(auth.getVersion()));
assertThat(auth.getAuthenticationType(), sameInstance(auth.getAuthenticationType()));
assertThat(auth.getMetadata(), sameInstance(auth.getMetadata()));
} else {
assertThat(responseRef.get().authentication(), sameInstance(authentication));
}
assertThat(throwableRef.get(), nullValue());
}

private AnonymousUser prepareAnonymousUser() {
final AnonymousUser anonymousUser = mock(AnonymousUser.class);
if (randomBoolean()) {
when(anonymousUser.enabled()).thenReturn(true);
} else {
when(anonymousUser.enabled()).thenReturn(false);
}
final String[] roleNames = randomList(1, 4, () -> randomAlphaOfLengthBetween(4, 12)).toArray(new String[0]);
Copy link
Member

Choose a reason for hiding this comment

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

nit: this only makes sense in the if branch right?

Copy link
Member Author

Choose a reason for hiding this comment

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

You right. Thanks

when(anonymousUser.roles()).thenReturn(roleNames);
return anonymousUser;
}

}
9 changes: 7 additions & 2 deletions ...c/test/java/org/elasticsearch/xpack/security/rest/action/RestAuthenticateActionTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -66,8 +66,13 @@ public void testAuthenticateApi() throws Exception {
assertThat(objectPath.evaluate("lookup_realm.name").toString(), equalTo("file"));
assertThat(objectPath.evaluate("lookup_realm.type").toString(), equalTo("file"));
List<String> roles = objectPath.evaluate("roles");
assertThat(roles.size(), is(1));
assertThat(roles, contains(SecuritySettingsSource.TEST_ROLE));
if (anonymousEnabled) {
assertThat(roles.size(), is(3));
assertThat(roles, contains(SecuritySettingsSource.TEST_ROLE, SecuritySettingsSource.TEST_ROLE, "foo"));
} else {
assertThat(roles.size(), is(1));
assertThat(roles, contains(SecuritySettingsSource.TEST_ROLE));
}
}

public void testAuthenticateApiWithoutAuthentication() throws Exception {
Expand Down