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

Ensure authz role for API key is named after owner role #59041

Original file line number Diff line number Diff line change
Expand Up @@ -29,17 +29,12 @@
public final class LimitedRole extends Role {
private final Role limitedBy;

LimitedRole(String[] names, ClusterPermission cluster, IndicesPermission indices, ApplicationPermission application,
RunAsPermission runAs, Role limitedBy) {
super(names, cluster, indices, application, runAs);
assert limitedBy != null : "limiting role is required";
LimitedRole(ClusterPermission cluster, IndicesPermission indices, ApplicationPermission application, RunAsPermission runAs,
Role limitedBy) {
super(Objects.requireNonNull(limitedBy, "limiting role is required").names(), cluster, indices, application, runAs);
this.limitedBy = limitedBy;
}

public Role limitedBy() {
return limitedBy;
}

@Override
public ClusterPermission cluster() {
throw new UnsupportedOperationException("cannot retrieve cluster permission on limited role");
Expand Down Expand Up @@ -86,7 +81,7 @@ public Predicate<String> allowedIndicesMatcher(String action) {
@Override
public Automaton allowedActionsMatcher(String index) {
final Automaton allowedMatcher = super.allowedActionsMatcher(index);
final Automaton limitedByMatcher = super.allowedActionsMatcher(index);
final Automaton limitedByMatcher = limitedBy.allowedActionsMatcher(index);
Copy link
Member

Choose a reason for hiding this comment

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

Gosh! Is this a bug!?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes. At first sight testing for it was not trivial. The bug doesn't seem important though. I'll see what I can do.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've pushed a simple unit test 9738963, it is trivial after all.

The bug is not consequential. It could manifest when resizing and when adding aliases, when using keys with role descriptors, where it fails to allow the operation, although it should.

return Automatons.intersectAndMinimize(allowedMatcher, limitedByMatcher);
}

Expand Down Expand Up @@ -187,7 +182,6 @@ public boolean checkRunAs(String runAs) {
*/
public static LimitedRole createLimitedRole(Role fromRole, Role limitedByRole) {
Objects.requireNonNull(limitedByRole, "limited by role is required to create limited role");
return new LimitedRole(fromRole.names(), fromRole.cluster(), fromRole.indices(), fromRole.application(), fromRole.runAs(),
limitedByRole);
return new LimitedRole(fromRole.cluster(), fromRole.indices(), fromRole.application(), fromRole.runAs(), limitedByRole);
albertzaharovits marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,11 @@

package org.elasticsearch.xpack.core.security.authz.permission;

import org.apache.lucene.util.automaton.Automaton;
import org.elasticsearch.Version;
import org.elasticsearch.action.admin.indices.create.CreateIndexAction;
import org.elasticsearch.action.admin.indices.delete.DeleteIndexAction;
import org.elasticsearch.action.bulk.BulkAction;
import org.elasticsearch.action.search.SearchAction;
import org.elasticsearch.cluster.metadata.AliasMetadata;
import org.elasticsearch.cluster.metadata.IndexMetadata;
Expand All @@ -24,12 +26,14 @@
import org.elasticsearch.xpack.core.security.authz.privilege.ApplicationPrivilegeDescriptor;
import org.elasticsearch.xpack.core.security.authz.privilege.ClusterPrivilegeResolver;
import org.elasticsearch.xpack.core.security.authz.privilege.IndexPrivilege;
import org.elasticsearch.xpack.core.security.support.Automatons;
import org.junit.Before;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Set;
import java.util.function.Predicate;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
Expand All @@ -50,6 +54,7 @@ public void testRoleConstructorWithLimitedRole() {
Role limitedByRole = Role.builder("limited-role").build();
Role role = LimitedRole.createLimitedRole(fromRole, limitedByRole);
assertNotNull(role);
assertThat(role.names(), is(limitedByRole.names()));

NullPointerException npe = expectThrows(NullPointerException.class, () -> LimitedRole.createLimitedRole(fromRole, null));
assertThat(npe.getMessage(), containsString("limited by role is required to create limited role"));
Expand Down Expand Up @@ -200,6 +205,42 @@ public void testAllowedIndicesMatcher() {
}
}

public void testAllowedActionsMatcher() {
Role fromRole = Role.builder("fromRole")
.add(IndexPrivilege.WRITE, "ind*")
.add(IndexPrivilege.READ, "ind*")
.add(IndexPrivilege.READ, "other*")
.build();
Automaton fromRoleAutomaton = fromRole.allowedActionsMatcher("index1");
Predicate<String> fromRolePredicate = Automatons.predicate(fromRoleAutomaton);
assertThat(fromRolePredicate.test(SearchAction.NAME), is(true));
assertThat(fromRolePredicate.test(BulkAction.NAME), is(true));

Role limitedByRole = Role.builder("limitedRole")
.add(IndexPrivilege.READ, "index1", "index2")
.build();
Automaton limitedByRoleAutomaton = limitedByRole.allowedActionsMatcher("index1");
Predicate<String> limitedByRolePredicated = Automatons.predicate(limitedByRoleAutomaton);
assertThat(limitedByRolePredicated.test(SearchAction.NAME), is(true));
assertThat(limitedByRolePredicated.test(BulkAction.NAME), is(false));
Role role = LimitedRole.createLimitedRole(fromRole, limitedByRole);

Automaton roleAutomaton = role.allowedActionsMatcher("index1");
Predicate<String> rolePredicate = Automatons.predicate(roleAutomaton);
assertThat(rolePredicate.test(SearchAction.NAME), is(true));
assertThat(rolePredicate.test(BulkAction.NAME), is(false));

roleAutomaton = role.allowedActionsMatcher("index2");
rolePredicate = Automatons.predicate(roleAutomaton);
assertThat(rolePredicate.test(SearchAction.NAME), is(true));
assertThat(rolePredicate.test(BulkAction.NAME), is(false));

roleAutomaton = role.allowedActionsMatcher("other");
rolePredicate = Automatons.predicate(roleAutomaton);
assertThat(rolePredicate.test(SearchAction.NAME), is(false));
assertThat(rolePredicate.test(BulkAction.NAME), is(false));
}

public void testCheckClusterPrivilege() {
Role fromRole = Role.builder("a-role").cluster(Collections.singleton("manage_security"), Collections.emptyList())
.build();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@
import org.elasticsearch.xpack.core.security.action.GetApiKeyResponse;
import org.elasticsearch.xpack.core.security.action.InvalidateApiKeyResponse;
import org.elasticsearch.xpack.core.security.authc.Authentication;
import org.elasticsearch.xpack.core.security.authc.Authentication.RealmRef;
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
Expand Down Expand Up @@ -240,7 +241,7 @@ private void createApiKeyAndIndexIt(Authentication authentication, CreateApiKeyR
}

/**
* package protected for testing
* package-private for testing
*/
XContentBuilder newDocument(SecureString apiKey, String name, Authentication authentication, Set<RoleDescriptor> userRoles,
Instant created, Instant expiration, List<RoleDescriptor> keyRoles,
Expand Down Expand Up @@ -329,6 +330,16 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListener<Authentic
}
}

public Authentication createApiKeyAuthentication(AuthenticationResult authResult, String nodeName) {
if (false == authResult.isAuthenticated()) {
throw new IllegalArgumentException("API Key authn result must be successful");
}
final User user = authResult.getUser();
final RealmRef authenticatedBy = new RealmRef(ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, nodeName);
return new Authentication(user, authenticatedBy, null, Version.CURRENT, Authentication.AuthenticationType.API_KEY,
authResult.getMetadata());
}

private void loadApiKeyAndValidateCredentials(ThreadContext ctx, ApiKeyCredentials credentials,
ActionListener<AuthenticationResult> listener) {
final String docId = credentials.getId();
Expand Down Expand Up @@ -525,7 +536,8 @@ CachedApiKeyHashResult getFromCache(String id) {
return apiKeyAuthCache == null ? null : FutureUtils.get(apiKeyAuthCache.get(id), 0L, TimeUnit.MILLISECONDS);
}

private void validateApiKeyExpiration(Map<String, Object> source, ApiKeyCredentials credentials, Clock clock,
// package-private for testing
void validateApiKeyExpiration(Map<String, Object> source, ApiKeyCredentials credentials, Clock clock,
ActionListener<AuthenticationResult> listener) {
final Long expirationEpochMilli = (Long) source.get("expiration_time");
if (expirationEpochMilli == null || Instant.ofEpochMilli(expirationEpochMilli).isAfter(clock.instant())) {
Expand Down Expand Up @@ -618,12 +630,12 @@ public void ensureEnabled() {
}
}

// package private class for testing
static final class ApiKeyCredentials implements Closeable {
// public class for testing
public static final class ApiKeyCredentials implements Closeable {
private final String id;
private final SecureString key;

ApiKeyCredentials(String id, SecureString key) {
public ApiKeyCredentials(String id, SecureString key) {
this.id = id;
this.key = key;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -346,10 +346,9 @@ private void authenticateAsync() {
private void checkForApiKey() {
apiKeyService.authenticateWithApiKeyIfPresent(threadContext, ActionListener.wrap(authResult -> {
if (authResult.isAuthenticated()) {
final User user = authResult.getUser();
authenticatedBy = new RealmRef(ApiKeyService.API_KEY_REALM_NAME, ApiKeyService.API_KEY_REALM_TYPE, nodeName);
writeAuthToContext(new Authentication(user, authenticatedBy, null, Version.CURRENT,
Authentication.AuthenticationType.API_KEY, authResult.getMetadata()));
final Authentication authentication = apiKeyService.createApiKeyAuthentication(authResult, nodeName);
Copy link
Contributor

Choose a reason for hiding this comment

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

++

this.authenticatedBy = authentication.getAuthenticatedBy();
writeAuthToContext(authentication);
} else if (authResult.getStatus() == AuthenticationResult.Status.TERMINATE) {
Exception e = (authResult.getException() != null) ? authResult.getException()
: Exceptions.authenticationError(authResult.getMessage());
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -66,6 +66,7 @@
import java.util.Collection;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ExecutionException;
Expand Down Expand Up @@ -720,6 +721,23 @@ public void testCachedApiKeyValidationWillNotBeBlockedByUnCachedApiKey() throws
assertEquals(AuthenticationResult.Status.SUCCESS, authenticationResult3.getStatus());
}

public static class Utils {

public static Authentication createApiKeyAuthentication(ApiKeyService apiKeyService,
Authentication authentication,
Set<RoleDescriptor> userRoles,
List<RoleDescriptor> keyRoles) throws Exception {
XContentBuilder keyDocSource = apiKeyService.newDocument(new SecureString("secret".toCharArray()), "test", authentication,
userRoles, Instant.now(), Instant.now().plus(Duration.ofSeconds(3600)), keyRoles, Version.CURRENT);
Map<String, Object> keyDocMap = XContentHelper.convertToMap(BytesReference.bytes(keyDocSource), true, XContentType.JSON).v2();
PlainActionFuture<AuthenticationResult> authenticationResultFuture = PlainActionFuture.newFuture();
apiKeyService.validateApiKeyExpiration(keyDocMap, new ApiKeyService.ApiKeyCredentials("id",
new SecureString("pass".toCharArray())),
Clock.systemUTC(), authenticationResultFuture);
return apiKeyService.createApiKeyAuthentication(authenticationResultFuture.get(), "node01");
}
}

private ApiKeyService createApiKeyService(Settings baseSettings) {
final Settings settings = Settings.builder()
.put(XPackSettings.API_KEY_SERVICE_ENABLED_SETTING.getKey(), true)
Expand Down
Loading