Skip to content

Enable caching of roles from the api keys #36387

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

Merged
merged 3 commits into from
Dec 10, 2018
Merged

Enable caching of roles from the api keys #36387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
95e167b
Enable caching of roles from the api keys
jaymode Dec 7, 2018
90197af
fix bad mock in test
jaymode Dec 7, 2018
ada3e19
add missing newline
jaymode Dec 10, 2018
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
5 changes: 3 additions & 2 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 @@ -465,6 +465,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
final NativePrivilegeStore privilegeStore = new NativePrivilegeStore(settings, client, securityIndex.get());
components.add(privilegeStore);

final FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(settings);
final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService, getLicenseState());
final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, getLicenseState(), securityIndex.get());
final ReservedRolesStore reservedRolesStore = new ReservedRolesStore();
Expand All @@ -473,13 +474,13 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
rolesProviders.addAll(extension.getRolesProviders(settings, resourceWatcherService));
}
final CompositeRolesStore allRolesStore = new CompositeRolesStore(settings, fileRolesStore, nativeRolesStore,
reservedRolesStore, privilegeStore, rolesProviders, threadPool.getThreadContext(), getLicenseState());
reservedRolesStore, privilegeStore, rolesProviders, threadPool.getThreadContext(), getLicenseState(), fieldPermissionsCache);
securityIndex.get().addIndexStateListener(allRolesStore::onSecurityIndexStateChange);
// to keep things simple, just invalidate all cached entries on license change. this happens so rarely that the impact should be
// minimal
getLicenseState().addListener(allRolesStore::invalidateAll);
final AuthorizationService authzService = new AuthorizationService(settings, allRolesStore, clusterService,
auditTrailService, failureHandler, threadPool, anonymousUser, apiKeyService);
auditTrailService, failureHandler, threadPool, anonymousUser, apiKeyService, fieldPermissionsCache);
components.add(nativeRolesStore); // used by roles actions
components.add(reservedRolesStore); // used by roles actions
components.add(allRolesStore); // for SecurityFeatureSet and clear roles cache
Expand Down
24 changes: 3 additions & 21 deletions ...k/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/ApiKeyService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
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;
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsCache;
import org.elasticsearch.xpack.core.security.authz.permission.Role;
import org.elasticsearch.xpack.core.security.user.User;
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
Expand Down Expand Up @@ -96,8 +95,7 @@ public class ApiKeyService {
private final Hasher hasher;
private final boolean enabled;

public ApiKeyService(Settings settings, Clock clock, Client client,
SecurityIndexManager securityIndex, ClusterService clusterService) {
public ApiKeyService(Settings settings, Clock clock, Client client, SecurityIndexManager securityIndex, ClusterService clusterService) {
this.clock = clock;
this.client = client;
this.securityIndex = securityIndex;
Expand Down Expand Up @@ -221,25 +219,13 @@ void authenticateWithApiKeyIfPresent(ThreadContext ctx, ActionListener<Authentic
* retrieval of role descriptors that are associated with the api key and triggers the building
* of the {@link Role} to authorize the request.
*/
public void getRoleForApiKey(Authentication authentication, ThreadContext threadContext, CompositeRolesStore rolesStore,
FieldPermissionsCache fieldPermissionsCache, ActionListener<Role> listener) {
public void getRoleForApiKey(Authentication authentication, CompositeRolesStore rolesStore, ActionListener<Role> listener) {
if (authentication.getAuthenticationType() != Authentication.AuthenticationType.API_KEY) {
throw new IllegalStateException("authentication type must be api key but is " + authentication.getAuthenticationType());
}

final Map<String, Object> metadata = authentication.getMetadata();
final String apiKeyId = (String) metadata.get(API_KEY_ID_KEY);
final String contextKeyId = threadContext.getTransient(API_KEY_ID_KEY);
if (apiKeyId.equals(contextKeyId)) {
final Role preBuiltRole = threadContext.getTransient(API_KEY_ROLE_KEY);
if (preBuiltRole != null) {
listener.onResponse(preBuiltRole);
return;
}
} else if (contextKeyId != null) {
throw new IllegalStateException("authentication api key id [" + apiKeyId + "] does not match context value [" +
contextKeyId + "]");
}

final Map<String, Object> roleDescriptors = (Map<String, Object>) metadata.get(API_KEY_ROLE_DESCRIPTORS_KEY);
final List<RoleDescriptor> roleDescriptorList = roleDescriptors.entrySet().stream()
Expand All @@ -258,11 +244,7 @@ public void getRoleForApiKey(Authentication authentication, ThreadContext thread
}
}).collect(Collectors.toList());

rolesStore.buildRoleFromDescriptors(roleDescriptorList, fieldPermissionsCache, ActionListener.wrap(role -> {
threadContext.putTransient(API_KEY_ID_KEY, apiKeyId);
threadContext.putTransient(API_KEY_ROLE_KEY, role);
listener.onResponse(role);
}, listener::onFailure));
rolesStore.buildAndCacheRoleFromDescriptors(roleDescriptorList, apiKeyId, listener);

}

Expand Down
9 changes: 5 additions & 4 deletions ...n/security/src/main/java/org/elasticsearch/xpack/security/authz/AuthorizationService.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,8 @@ public class AuthorizationService {

public AuthorizationService(Settings settings, CompositeRolesStore rolesStore, ClusterService clusterService,
AuditTrailService auditTrail, AuthenticationFailureHandler authcFailureHandler,
ThreadPool threadPool, AnonymousUser anonymousUser, ApiKeyService apiKeyService) {
ThreadPool threadPool, AnonymousUser anonymousUser, ApiKeyService apiKeyService,
FieldPermissionsCache fieldPermissionsCache) {
this.rolesStore = rolesStore;
this.clusterService = clusterService;
this.auditTrail = auditTrail;
Expand All @@ -122,7 +123,7 @@ public AuthorizationService(Settings settings, CompositeRolesStore rolesStore, C
this.anonymousUser = anonymousUser;
this.isAnonymousEnabled = AnonymousUser.isAnonymousEnabled(settings);
this.anonymousAuthzExceptionEnabled = ANONYMOUS_AUTHORIZATION_EXCEPTION_SETTING.get(settings);
this.fieldPermissionsCache = new FieldPermissionsCache(settings);
this.fieldPermissionsCache = fieldPermissionsCache;
this.apiKeyService = apiKeyService;
}

Expand Down Expand Up @@ -480,7 +481,7 @@ public void roles(User user, Authentication authentication, ActionListener<Role>

final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
if (authType == Authentication.AuthenticationType.API_KEY) {
apiKeyService.getRoleForApiKey(authentication, threadContext, rolesStore, fieldPermissionsCache, roleActionListener);
apiKeyService.getRoleForApiKey(authentication, rolesStore, roleActionListener);
} else {
Set<String> roleNames = new HashSet<>();
Collections.addAll(roleNames, user.roles());
Expand All @@ -496,7 +497,7 @@ public void roles(User user, Authentication authentication, ActionListener<Role>
} else if (roleNames.contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName())) {
roleActionListener.onResponse(ReservedRolesStore.SUPERUSER_ROLE);
} else {
rolesStore.roles(roleNames, fieldPermissionsCache, roleActionListener);
rolesStore.roles(roleNames, roleActionListener);
}
}
}
Expand Down
122 changes: 83 additions & 39 deletions ...urity/src/main/java/org/elasticsearch/xpack/security/authz/store/CompositeRolesStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@
*/
public class CompositeRolesStore {


private static final String ROLES_STORE_SOURCE = "roles_stores";
private static final Setting<Integer> CACHE_SIZE_SETTING =
Setting.intSetting("xpack.security.authz.store.roles.cache.max_size", 10000, Property.NodeScope);
private static final Setting<Integer> NEGATIVE_LOOKUP_CACHE_SIZE_SETTING =
Expand All @@ -92,7 +92,8 @@ public class CompositeRolesStore {
private final NativeRolesStore nativeRolesStore;
private final NativePrivilegeStore privilegeStore;
private final XPackLicenseState licenseState;
private final Cache<Set<String>, Role> roleCache;
private final FieldPermissionsCache fieldPermissionsCache;
private final Cache<RoleKey, Role> roleCache;
private final Cache<String, Boolean> negativeLookupCache;
private final ThreadContext threadContext;
private final AtomicLong numInvalidation = new AtomicLong();
Expand All @@ -102,13 +103,14 @@ public class CompositeRolesStore {
public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, NativeRolesStore nativeRolesStore,
ReservedRolesStore reservedRolesStore, NativePrivilegeStore privilegeStore,
List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> rolesProviders,
ThreadContext threadContext, XPackLicenseState licenseState) {
ThreadContext threadContext, XPackLicenseState licenseState, FieldPermissionsCache fieldPermissionsCache) {
this.fileRolesStore = fileRolesStore;
fileRolesStore.addListener(this::invalidate);
this.nativeRolesStore = nativeRolesStore;
this.privilegeStore = privilegeStore;
this.licenseState = licenseState;
CacheBuilder<Set<String>, Role> builder = CacheBuilder.builder();
this.fieldPermissionsCache = fieldPermissionsCache;
CacheBuilder<RoleKey, Role> builder = CacheBuilder.builder();
final int cacheSize = CACHE_SIZE_SETTING.get(settings);
if (cacheSize >= 0) {
builder.setMaximumWeight(cacheSize);
Expand All @@ -133,8 +135,9 @@ public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, Nat
}
}

public void roles(Set<String> roleNames, FieldPermissionsCache fieldPermissionsCache, ActionListener<Role> roleActionListener) {
Role existing = roleCache.get(roleNames);
public void roles(Set<String> roleNames, ActionListener<Role> roleActionListener) {
final RoleKey roleKey = new RoleKey(roleNames, ROLES_STORE_SOURCE);
Role existing = roleCache.get(roleKey);
if (existing != null) {
roleActionListener.onResponse(existing);
} else {
Expand All @@ -154,33 +157,54 @@ public void roles(Set<String> roleNames, FieldPermissionsCache fieldPermissionsC
.filter((rd) -> rd.isUsingDocumentOrFieldLevelSecurity() == false)
.collect(Collectors.toSet());
}
logger.trace("Building role from descriptors [{}] for names [{}]", effectiveDescriptors, roleNames);
buildRoleFromDescriptors(effectiveDescriptors, fieldPermissionsCache, privilegeStore, ActionListener.wrap(role -> {
if (role != null && rolesRetrievalResult.isSuccess()) {
try (ReleasableLock ignored = readLock.acquire()) {
/* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold
* the write lock (fetching stats for instance - which is kinda overkill?) but since we fetching
* stuff in an async fashion we need to make sure that if the cache got invalidated since we
* started the request we don't put a potential stale result in the cache, hence the
* numInvalidation.get() comparison to the number of invalidation when we started. we just try to
* be on the safe side and don't cache potentially stale results
*/
if (invalidationCounter == numInvalidation.get()) {
roleCache.computeIfAbsent(roleNames, (s) -> role);
}
}

for (String missingRole : rolesRetrievalResult.getMissingRoles()) {
negativeLookupCache.computeIfAbsent(missingRole, s -> Boolean.TRUE);
}
}
roleActionListener.onResponse(role);
}, roleActionListener::onFailure));
buildThenMaybeCacheRole(roleKey, effectiveDescriptors, rolesRetrievalResult.getMissingRoles(),
rolesRetrievalResult.isSuccess(), invalidationCounter, roleActionListener);
},
roleActionListener::onFailure));
}
}

public void buildAndCacheRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, String source,
ActionListener<Role> listener) {
if (ROLES_STORE_SOURCE.equals(source)) {
throw new IllegalArgumentException("source [" + ROLES_STORE_SOURCE + "] is reserved for internal use");
}
RoleKey roleKey = new RoleKey(roleDescriptors.stream().map(RoleDescriptor::getName).collect(Collectors.toSet()), source);
Role existing = roleCache.get(roleKey);
if (existing != null) {
listener.onResponse(existing);
} else {
final long invalidationCounter = numInvalidation.get();
buildThenMaybeCacheRole(roleKey, roleDescriptors, Collections.emptySet(), true, invalidationCounter, listener);
}
}

private void buildThenMaybeCacheRole(RoleKey roleKey, Collection<RoleDescriptor> roleDescriptors, Set<String> missing,
boolean tryCache, long invalidationCounter, ActionListener<Role> listener) {
logger.trace("Building role from descriptors [{}] for names [{}] from source [{}]", roleDescriptors, roleKey.names, roleKey.source);
buildRoleFromDescriptors(roleDescriptors, fieldPermissionsCache, privilegeStore, ActionListener.wrap(role -> {
if (role != null && tryCache) {
try (ReleasableLock ignored = readLock.acquire()) {
/* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold
* the write lock (fetching stats for instance - which is kinda overkill?) but since we fetching
* stuff in an async fashion we need to make sure that if the cache got invalidated since we
* started the request we don't put a potential stale result in the cache, hence the
* numInvalidation.get() comparison to the number of invalidation when we started. we just try to
* be on the safe side and don't cache potentially stale results
*/
if (invalidationCounter == numInvalidation.get()) {
roleCache.computeIfAbsent(roleKey, (s) -> role);
}
}

for (String missingRole : missing) {
negativeLookupCache.computeIfAbsent(missingRole, s -> Boolean.TRUE);
}
}
listener.onResponse(role);
}, listener::onFailure));
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: Needs a blank line.

Copy link
Member Author

Choose a reason for hiding this comment

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

👍


public void getRoleDescriptors(Set<String> roleNames, ActionListener<Set<RoleDescriptor>> listener) {
roleDescriptors(roleNames, ActionListener.wrap(rolesRetrievalResult -> {
if (rolesRetrievalResult.isSuccess()) {
Expand Down Expand Up @@ -241,11 +265,6 @@ private String names(Collection<RoleDescriptor> descriptors) {
return descriptors.stream().map(RoleDescriptor::getName).collect(Collectors.joining(","));
}

public void buildRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, FieldPermissionsCache fieldPermissionsCache,
ActionListener<Role> listener) {
buildRoleFromDescriptors(roleDescriptors, fieldPermissionsCache, privilegeStore, listener);
}

public static void buildRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, FieldPermissionsCache fieldPermissionsCache,
NativePrivilegeStore privilegeStore, ActionListener<Role> listener) {
if (roleDescriptors.isEmpty()) {
Expand Down Expand Up @@ -346,10 +365,10 @@ public void invalidate(String role) {

// the cache cannot be modified while doing this operation per the terms of the cache iterator
try (ReleasableLock ignored = writeLock.acquire()) {
Iterator<Set<String>> keyIter = roleCache.keys().iterator();
Iterator<RoleKey> keyIter = roleCache.keys().iterator();
while (keyIter.hasNext()) {
Set<String> key = keyIter.next();
if (key.contains(role)) {
RoleKey key = keyIter.next();
if (key.names.contains(role)) {
keyIter.remove();
}
}
Expand All @@ -362,10 +381,10 @@ public void invalidate(Set<String> roles) {

// the cache cannot be modified while doing this operation per the terms of the cache iterator
try (ReleasableLock ignored = writeLock.acquire()) {
Iterator<Set<String>> keyIter = roleCache.keys().iterator();
Iterator<RoleKey> keyIter = roleCache.keys().iterator();
while (keyIter.hasNext()) {
Set<String> key = keyIter.next();
if (Sets.haveEmptyIntersection(key, roles) == false) {
RoleKey key = keyIter.next();
if (Sets.haveEmptyIntersection(key.names, roles) == false) {
keyIter.remove();
}
}
Expand Down Expand Up @@ -461,6 +480,31 @@ private Set<String> getMissingRoles() {
}
}

private static final class RoleKey {

private final Set<String> names;
private final String source;

private RoleKey(Set<String> names, String source) {
this.names = Objects.requireNonNull(names);
this.source = Objects.requireNonNull(source);
}

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (o == null || getClass() != o.getClass()) return false;
RoleKey roleKey = (RoleKey) o;
return names.equals(roleKey.names) &&
source.equals(roleKey.source);
}

@Override
public int hashCode() {
return Objects.hash(names, source);
}
}

public static List<Setting<?>> getSettings() {
return Arrays.asList(CACHE_SIZE_SETTING, NEGATIVE_LOOKUP_CACHE_SIZE_SETTING);
}
Expand Down
Loading