Skip to content

Commit 554bf23

Browse files
authored
Enable caching of roles from the api keys (#36387)
This commit enables api keys to share the same cache for roles that is already in use for roles from other sources. In order to avoid the possibility of a key collision with roles that do not belong to api keys, the key for the roles cache now includes a source field that prevents these collisions.
1 parent 602feeb commit 554bf23

File tree

8 files changed

+145
-158
lines changed

8 files changed

+145
-158
lines changed

x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/Security.java

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -465,6 +465,7 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
465465
final NativePrivilegeStore privilegeStore = new NativePrivilegeStore(settings, client, securityIndex.get());
466466
components.add(privilegeStore);
467467

468+
final FieldPermissionsCache fieldPermissionsCache = new FieldPermissionsCache(settings);
468469
final FileRolesStore fileRolesStore = new FileRolesStore(settings, env, resourceWatcherService, getLicenseState());
469470
final NativeRolesStore nativeRolesStore = new NativeRolesStore(settings, client, getLicenseState(), securityIndex.get());
470471
final ReservedRolesStore reservedRolesStore = new ReservedRolesStore();
@@ -473,13 +474,13 @@ Collection<Object> createComponents(Client client, ThreadPool threadPool, Cluste
473474
rolesProviders.addAll(extension.getRolesProviders(settings, resourceWatcherService));
474475
}
475476
final CompositeRolesStore allRolesStore = new CompositeRolesStore(settings, fileRolesStore, nativeRolesStore,
476-
reservedRolesStore, privilegeStore, rolesProviders, threadPool.getThreadContext(), getLicenseState());
477+
reservedRolesStore, privilegeStore, rolesProviders, threadPool.getThreadContext(), getLicenseState(), fieldPermissionsCache);
477478
securityIndex.get().addIndexStateListener(allRolesStore::onSecurityIndexStateChange);
478479
// to keep things simple, just invalidate all cached entries on license change. this happens so rarely that the impact should be
479480
// minimal
480481
getLicenseState().addListener(allRolesStore::invalidateAll);
481482
final AuthorizationService authzService = new AuthorizationService(settings, allRolesStore, clusterService,
482-
auditTrailService, failureHandler, threadPool, anonymousUser, apiKeyService);
483+
auditTrailService, failureHandler, threadPool, anonymousUser, apiKeyService, fieldPermissionsCache);
483484
components.add(nativeRolesStore); // used by roles actions
484485
components.add(reservedRolesStore); // used by roles actions
485486
components.add(allRolesStore); // for SecurityFeatureSet and clear roles cache

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

Lines changed: 3 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -38,7 +38,6 @@
3838
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult;
3939
import org.elasticsearch.xpack.core.security.authc.support.Hasher;
4040
import org.elasticsearch.xpack.core.security.authz.RoleDescriptor;
41-
import org.elasticsearch.xpack.core.security.authz.permission.FieldPermissionsCache;
4241
import org.elasticsearch.xpack.core.security.authz.permission.Role;
4342
import org.elasticsearch.xpack.core.security.user.User;
4443
import org.elasticsearch.xpack.security.authz.store.CompositeRolesStore;
@@ -96,8 +95,7 @@ public class ApiKeyService {
9695
private final Hasher hasher;
9796
private final boolean enabled;
9897

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

230227
final Map<String, Object> metadata = authentication.getMetadata();
231228
final String apiKeyId = (String) metadata.get(API_KEY_ID_KEY);
232-
final String contextKeyId = threadContext.getTransient(API_KEY_ID_KEY);
233-
if (apiKeyId.equals(contextKeyId)) {
234-
final Role preBuiltRole = threadContext.getTransient(API_KEY_ROLE_KEY);
235-
if (preBuiltRole != null) {
236-
listener.onResponse(preBuiltRole);
237-
return;
238-
}
239-
} else if (contextKeyId != null) {
240-
throw new IllegalStateException("authentication api key id [" + apiKeyId + "] does not match context value [" +
241-
contextKeyId + "]");
242-
}
243229

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

261-
rolesStore.buildRoleFromDescriptors(roleDescriptorList, fieldPermissionsCache, ActionListener.wrap(role -> {
262-
threadContext.putTransient(API_KEY_ID_KEY, apiKeyId);
263-
threadContext.putTransient(API_KEY_ROLE_KEY, role);
264-
listener.onResponse(role);
265-
}, listener::onFailure));
247+
rolesStore.buildAndCacheRoleFromDescriptors(roleDescriptorList, apiKeyId, listener);
266248

267249
}
268250

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

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,8 @@ public class AuthorizationService {
112112

113113
public AuthorizationService(Settings settings, CompositeRolesStore rolesStore, ClusterService clusterService,
114114
AuditTrailService auditTrail, AuthenticationFailureHandler authcFailureHandler,
115-
ThreadPool threadPool, AnonymousUser anonymousUser, ApiKeyService apiKeyService) {
115+
ThreadPool threadPool, AnonymousUser anonymousUser, ApiKeyService apiKeyService,
116+
FieldPermissionsCache fieldPermissionsCache) {
116117
this.rolesStore = rolesStore;
117118
this.clusterService = clusterService;
118119
this.auditTrail = auditTrail;
@@ -122,7 +123,7 @@ public AuthorizationService(Settings settings, CompositeRolesStore rolesStore, C
122123
this.anonymousUser = anonymousUser;
123124
this.isAnonymousEnabled = AnonymousUser.isAnonymousEnabled(settings);
124125
this.anonymousAuthzExceptionEnabled = ANONYMOUS_AUTHORIZATION_EXCEPTION_SETTING.get(settings);
125-
this.fieldPermissionsCache = new FieldPermissionsCache(settings);
126+
this.fieldPermissionsCache = fieldPermissionsCache;
126127
this.apiKeyService = apiKeyService;
127128
}
128129

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

481482
final Authentication.AuthenticationType authType = authentication.getAuthenticationType();
482483
if (authType == Authentication.AuthenticationType.API_KEY) {
483-
apiKeyService.getRoleForApiKey(authentication, threadContext, rolesStore, fieldPermissionsCache, roleActionListener);
484+
apiKeyService.getRoleForApiKey(authentication, rolesStore, roleActionListener);
484485
} else {
485486
Set<String> roleNames = new HashSet<>();
486487
Collections.addAll(roleNames, user.roles());
@@ -496,7 +497,7 @@ public void roles(User user, Authentication authentication, ActionListener<Role>
496497
} else if (roleNames.contains(ReservedRolesStore.SUPERUSER_ROLE_DESCRIPTOR.getName())) {
497498
roleActionListener.onResponse(ReservedRolesStore.SUPERUSER_ROLE);
498499
} else {
499-
rolesStore.roles(roleNames, fieldPermissionsCache, roleActionListener);
500+
rolesStore.roles(roleNames, roleActionListener);
500501
}
501502
}
502503
}

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

Lines changed: 83 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@
6868
*/
6969
public class CompositeRolesStore {
7070

71-
71+
private static final String ROLES_STORE_SOURCE = "roles_stores";
7272
private static final Setting<Integer> CACHE_SIZE_SETTING =
7373
Setting.intSetting("xpack.security.authz.store.roles.cache.max_size", 10000, Property.NodeScope);
7474
private static final Setting<Integer> NEGATIVE_LOOKUP_CACHE_SIZE_SETTING =
@@ -92,7 +92,8 @@ public class CompositeRolesStore {
9292
private final NativeRolesStore nativeRolesStore;
9393
private final NativePrivilegeStore privilegeStore;
9494
private final XPackLicenseState licenseState;
95-
private final Cache<Set<String>, Role> roleCache;
95+
private final FieldPermissionsCache fieldPermissionsCache;
96+
private final Cache<RoleKey, Role> roleCache;
9697
private final Cache<String, Boolean> negativeLookupCache;
9798
private final ThreadContext threadContext;
9899
private final AtomicLong numInvalidation = new AtomicLong();
@@ -102,13 +103,14 @@ public class CompositeRolesStore {
102103
public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, NativeRolesStore nativeRolesStore,
103104
ReservedRolesStore reservedRolesStore, NativePrivilegeStore privilegeStore,
104105
List<BiConsumer<Set<String>, ActionListener<RoleRetrievalResult>>> rolesProviders,
105-
ThreadContext threadContext, XPackLicenseState licenseState) {
106+
ThreadContext threadContext, XPackLicenseState licenseState, FieldPermissionsCache fieldPermissionsCache) {
106107
this.fileRolesStore = fileRolesStore;
107108
fileRolesStore.addListener(this::invalidate);
108109
this.nativeRolesStore = nativeRolesStore;
109110
this.privilegeStore = privilegeStore;
110111
this.licenseState = licenseState;
111-
CacheBuilder<Set<String>, Role> builder = CacheBuilder.builder();
112+
this.fieldPermissionsCache = fieldPermissionsCache;
113+
CacheBuilder<RoleKey, Role> builder = CacheBuilder.builder();
112114
final int cacheSize = CACHE_SIZE_SETTING.get(settings);
113115
if (cacheSize >= 0) {
114116
builder.setMaximumWeight(cacheSize);
@@ -133,8 +135,9 @@ public CompositeRolesStore(Settings settings, FileRolesStore fileRolesStore, Nat
133135
}
134136
}
135137

136-
public void roles(Set<String> roleNames, FieldPermissionsCache fieldPermissionsCache, ActionListener<Role> roleActionListener) {
137-
Role existing = roleCache.get(roleNames);
138+
public void roles(Set<String> roleNames, ActionListener<Role> roleActionListener) {
139+
final RoleKey roleKey = new RoleKey(roleNames, ROLES_STORE_SOURCE);
140+
Role existing = roleCache.get(roleKey);
138141
if (existing != null) {
139142
roleActionListener.onResponse(existing);
140143
} else {
@@ -154,33 +157,54 @@ public void roles(Set<String> roleNames, FieldPermissionsCache fieldPermissionsC
154157
.filter((rd) -> rd.isUsingDocumentOrFieldLevelSecurity() == false)
155158
.collect(Collectors.toSet());
156159
}
157-
logger.trace("Building role from descriptors [{}] for names [{}]", effectiveDescriptors, roleNames);
158-
buildRoleFromDescriptors(effectiveDescriptors, fieldPermissionsCache, privilegeStore, ActionListener.wrap(role -> {
159-
if (role != null && rolesRetrievalResult.isSuccess()) {
160-
try (ReleasableLock ignored = readLock.acquire()) {
161-
/* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold
162-
* the write lock (fetching stats for instance - which is kinda overkill?) but since we fetching
163-
* stuff in an async fashion we need to make sure that if the cache got invalidated since we
164-
* started the request we don't put a potential stale result in the cache, hence the
165-
* numInvalidation.get() comparison to the number of invalidation when we started. we just try to
166-
* be on the safe side and don't cache potentially stale results
167-
*/
168-
if (invalidationCounter == numInvalidation.get()) {
169-
roleCache.computeIfAbsent(roleNames, (s) -> role);
170-
}
171-
}
172-
173-
for (String missingRole : rolesRetrievalResult.getMissingRoles()) {
174-
negativeLookupCache.computeIfAbsent(missingRole, s -> Boolean.TRUE);
175-
}
176-
}
177-
roleActionListener.onResponse(role);
178-
}, roleActionListener::onFailure));
160+
buildThenMaybeCacheRole(roleKey, effectiveDescriptors, rolesRetrievalResult.getMissingRoles(),
161+
rolesRetrievalResult.isSuccess(), invalidationCounter, roleActionListener);
179162
},
180163
roleActionListener::onFailure));
181164
}
182165
}
183166

167+
public void buildAndCacheRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, String source,
168+
ActionListener<Role> listener) {
169+
if (ROLES_STORE_SOURCE.equals(source)) {
170+
throw new IllegalArgumentException("source [" + ROLES_STORE_SOURCE + "] is reserved for internal use");
171+
}
172+
RoleKey roleKey = new RoleKey(roleDescriptors.stream().map(RoleDescriptor::getName).collect(Collectors.toSet()), source);
173+
Role existing = roleCache.get(roleKey);
174+
if (existing != null) {
175+
listener.onResponse(existing);
176+
} else {
177+
final long invalidationCounter = numInvalidation.get();
178+
buildThenMaybeCacheRole(roleKey, roleDescriptors, Collections.emptySet(), true, invalidationCounter, listener);
179+
}
180+
}
181+
182+
private void buildThenMaybeCacheRole(RoleKey roleKey, Collection<RoleDescriptor> roleDescriptors, Set<String> missing,
183+
boolean tryCache, long invalidationCounter, ActionListener<Role> listener) {
184+
logger.trace("Building role from descriptors [{}] for names [{}] from source [{}]", roleDescriptors, roleKey.names, roleKey.source);
185+
buildRoleFromDescriptors(roleDescriptors, fieldPermissionsCache, privilegeStore, ActionListener.wrap(role -> {
186+
if (role != null && tryCache) {
187+
try (ReleasableLock ignored = readLock.acquire()) {
188+
/* this is kinda spooky. We use a read/write lock to ensure we don't modify the cache if we hold
189+
* the write lock (fetching stats for instance - which is kinda overkill?) but since we fetching
190+
* stuff in an async fashion we need to make sure that if the cache got invalidated since we
191+
* started the request we don't put a potential stale result in the cache, hence the
192+
* numInvalidation.get() comparison to the number of invalidation when we started. we just try to
193+
* be on the safe side and don't cache potentially stale results
194+
*/
195+
if (invalidationCounter == numInvalidation.get()) {
196+
roleCache.computeIfAbsent(roleKey, (s) -> role);
197+
}
198+
}
199+
200+
for (String missingRole : missing) {
201+
negativeLookupCache.computeIfAbsent(missingRole, s -> Boolean.TRUE);
202+
}
203+
}
204+
listener.onResponse(role);
205+
}, listener::onFailure));
206+
}
207+
184208
public void getRoleDescriptors(Set<String> roleNames, ActionListener<Set<RoleDescriptor>> listener) {
185209
roleDescriptors(roleNames, ActionListener.wrap(rolesRetrievalResult -> {
186210
if (rolesRetrievalResult.isSuccess()) {
@@ -241,11 +265,6 @@ private String names(Collection<RoleDescriptor> descriptors) {
241265
return descriptors.stream().map(RoleDescriptor::getName).collect(Collectors.joining(","));
242266
}
243267

244-
public void buildRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, FieldPermissionsCache fieldPermissionsCache,
245-
ActionListener<Role> listener) {
246-
buildRoleFromDescriptors(roleDescriptors, fieldPermissionsCache, privilegeStore, listener);
247-
}
248-
249268
public static void buildRoleFromDescriptors(Collection<RoleDescriptor> roleDescriptors, FieldPermissionsCache fieldPermissionsCache,
250269
NativePrivilegeStore privilegeStore, ActionListener<Role> listener) {
251270
if (roleDescriptors.isEmpty()) {
@@ -346,10 +365,10 @@ public void invalidate(String role) {
346365

347366
// the cache cannot be modified while doing this operation per the terms of the cache iterator
348367
try (ReleasableLock ignored = writeLock.acquire()) {
349-
Iterator<Set<String>> keyIter = roleCache.keys().iterator();
368+
Iterator<RoleKey> keyIter = roleCache.keys().iterator();
350369
while (keyIter.hasNext()) {
351-
Set<String> key = keyIter.next();
352-
if (key.contains(role)) {
370+
RoleKey key = keyIter.next();
371+
if (key.names.contains(role)) {
353372
keyIter.remove();
354373
}
355374
}
@@ -362,10 +381,10 @@ public void invalidate(Set<String> roles) {
362381

363382
// the cache cannot be modified while doing this operation per the terms of the cache iterator
364383
try (ReleasableLock ignored = writeLock.acquire()) {
365-
Iterator<Set<String>> keyIter = roleCache.keys().iterator();
384+
Iterator<RoleKey> keyIter = roleCache.keys().iterator();
366385
while (keyIter.hasNext()) {
367-
Set<String> key = keyIter.next();
368-
if (Sets.haveEmptyIntersection(key, roles) == false) {
386+
RoleKey key = keyIter.next();
387+
if (Sets.haveEmptyIntersection(key.names, roles) == false) {
369388
keyIter.remove();
370389
}
371390
}
@@ -461,6 +480,31 @@ private Set<String> getMissingRoles() {
461480
}
462481
}
463482

483+
private static final class RoleKey {
484+
485+
private final Set<String> names;
486+
private final String source;
487+
488+
private RoleKey(Set<String> names, String source) {
489+
this.names = Objects.requireNonNull(names);
490+
this.source = Objects.requireNonNull(source);
491+
}
492+
493+
@Override
494+
public boolean equals(Object o) {
495+
if (this == o) return true;
496+
if (o == null || getClass() != o.getClass()) return false;
497+
RoleKey roleKey = (RoleKey) o;
498+
return names.equals(roleKey.names) &&
499+
source.equals(roleKey.source);
500+
}
501+
502+
@Override
503+
public int hashCode() {
504+
return Objects.hash(names, source);
505+
}
506+
}
507+
464508
public static List<Setting<?>> getSettings() {
465509
return Arrays.asList(CACHE_SIZE_SETTING, NEGATIVE_LOOKUP_CACHE_SIZE_SETTING);
466510
}

0 commit comments

Comments
 (0)