Skip to content

Commit c26d149

Browse files
jaymodejaymode
committed
Security: propagate auth result to listeners
After elastic#30794, our caching realms limit each principal to a single auth attempt at a time. This prevents hammering of external servers but can cause a significant performance hit when requests need to go through a realm that takes a long time to attempt to authenticate in order to get to the realm that actually authenticates. In order to address this, this change will propagate failed results to listeners if they use the same set of credentials that the authentication attempt used. This does prevent these stalled requests from retrying the authentication attempt but the implementation does allow for new requests to retry the attempt.
1 parent b42074c commit c26d149

File tree

2 files changed

+136
-84
lines changed

2 files changed

+136
-84
lines changed

‎x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java‎

Lines changed: 61 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
package org.elasticsearch.xpack.security.authc.support;
77

88
import org.elasticsearch.action.ActionListener;
9+
import org.elasticsearch.common.Nullable;
910
import org.elasticsearch.common.cache.Cache;
1011
import org.elasticsearch.common.cache.CacheBuilder;
1112
import org.elasticsearch.common.settings.SecureString;
@@ -29,7 +30,7 @@
2930

3031
public abstract class CachingUsernamePasswordRealm extends UsernamePasswordRealm implements CachingRealm {
3132

32-
private final Cache<String, ListenableFuture<UserWithHash>> cache;
33+
private final Cache<String, ListenableFuture<CachedResult>> cache;
3334
private final ThreadPool threadPool;
3435
private final boolean authenticationEnabled;
3536
final Hasher cacheHasher;
@@ -40,7 +41,7 @@ protected CachingUsernamePasswordRealm(RealmConfig config, ThreadPool threadPool
4041
this.threadPool = threadPool;
4142
final TimeValue ttl = this.config.getSetting(CachingUsernamePasswordRealmSettings.CACHE_TTL_SETTING);
4243
if (ttl.getNanos() > 0) {
43-
cache = CacheBuilder.<String, ListenableFuture<UserWithHash>>builder()
44+
cache = CacheBuilder.<String, ListenableFuture<CachedResult>>builder()
4445
.setExpireAfterWrite(ttl)
4546
.setMaximumWeight(this.config.getSetting(CachingUsernamePasswordRealmSettings.CACHE_MAX_USERS_SETTING))
4647
.build();
@@ -121,58 +122,61 @@ public final void authenticate(AuthenticationToken authToken, ActionListener<Aut
121122
private void authenticateWithCache(UsernamePasswordToken token, ActionListener<AuthenticationResult> listener) {
122123
try {
123124
final AtomicBoolean authenticationInCache = new AtomicBoolean(true);
124-
final ListenableFuture<UserWithHash> listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> {
125+
final ListenableFuture<CachedResult> listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> {
125126
authenticationInCache.set(false);
126127
return new ListenableFuture<>();
127128
});
128129
if (authenticationInCache.get()) {
129130
// there is a cached or an inflight authenticate request
130-
listenableCacheEntry.addListener(ActionListener.wrap(authenticatedUserWithHash -> {
131-
if (authenticatedUserWithHash != null && authenticatedUserWithHash.verify(token.credentials())) {
132-
// cached credential hash matches the credential hash for this forestalled request
133-
handleCachedAuthentication(authenticatedUserWithHash.user, ActionListener.wrap(cacheResult -> {
134-
if (cacheResult.isAuthenticated()) {
135-
logger.debug("realm [{}] authenticated user [{}], with roles [{}]",
136-
name(), token.principal(), cacheResult.getUser().roles());
137-
} else {
138-
logger.debug("realm [{}] authenticated user [{}] from cache, but then failed [{}]",
139-
name(), token.principal(), cacheResult.getMessage());
140-
}
141-
listener.onResponse(cacheResult);
142-
}, listener::onFailure));
131+
listenableCacheEntry.addListener(ActionListener.wrap(cachedResult -> {
132+
final boolean credsMatch = cachedResult.verify(token.credentials());
133+
if (cachedResult.authenticationResult.isAuthenticated()) {
134+
if (credsMatch) {
135+
// cached credential hash matches the credential hash for this forestalled request
136+
handleCachedAuthentication(cachedResult.user, ActionListener.wrap(cacheResult -> {
137+
if (cacheResult.isAuthenticated()) {
138+
logger.debug("realm [{}] authenticated user [{}], with roles [{}]",
139+
name(), token.principal(), cacheResult.getUser().roles());
140+
} else {
141+
logger.debug("realm [{}] authenticated user [{}] from cache, but then failed [{}]",
142+
name(), token.principal(), cacheResult.getMessage());
143+
}
144+
listener.onResponse(cacheResult);
145+
}, listener::onFailure));
146+
} else {
147+
// its credential hash does not match the
148+
// hash of the credential for this forestalled request.
149+
// clear cache and try to reach the authentication source again because password
150+
// might have changed there and the local cached hash got stale
151+
cache.invalidate(token.principal(), listenableCacheEntry);
152+
authenticateWithCache(token, listener);
153+
}
154+
} else if (credsMatch) {
155+
// not authenticated but instead of hammering reuse the result. a new
156+
// request will trigger a retried auth
157+
listener.onResponse(cachedResult.authenticationResult);
143158
} else {
144-
// The inflight request has failed or its credential hash does not match the
145-
// hash of the credential for this forestalled request.
146-
// clear cache and try to reach the authentication source again because password
147-
// might have changed there and the local cached hash got stale
148159
cache.invalidate(token.principal(), listenableCacheEntry);
149160
authenticateWithCache(token, listener);
150161
}
151-
}, e -> {
152-
// the inflight request failed, so try again, but first (always) make sure cache
153-
// is cleared of the failed authentication
154-
cache.invalidate(token.principal(), listenableCacheEntry);
155-
authenticateWithCache(token, listener);
156-
}), threadPool.executor(ThreadPool.Names.GENERIC), threadPool.getThreadContext());
162+
}, listener::onFailure), threadPool.executor(ThreadPool.Names.GENERIC), threadPool.getThreadContext());
157163
} else {
158164
// attempt authentication against the authentication source
159165
doAuthenticate(token, ActionListener.wrap(authResult -> {
160-
if (authResult.isAuthenticated() && authResult.getUser().enabled()) {
161-
// compute the credential hash of this successful authentication request
162-
final UserWithHash userWithHash = new UserWithHash(authResult.getUser(), token.credentials(), cacheHasher);
163-
// notify any forestalled request listeners; they will not reach to the
164-
// authentication request and instead will use this hash for comparison
165-
listenableCacheEntry.onResponse(userWithHash);
166-
} else {
167-
// notify any forestalled request listeners; they will retry the request
168-
listenableCacheEntry.onResponse(null);
166+
if (authResult.isAuthenticated() == false || authResult.getUser().enabled() == false) {
167+
// a new request should trigger a new authentication
168+
cache.invalidate(token.principal(), listenableCacheEntry);
169169
}
170-
// notify the listener of the inflight authentication request; this request is not retried
170+
// notify any forestalled request listeners; they will not reach to the
171+
// authentication request and instead will use this result if they contain
172+
// the same credentials
173+
listenableCacheEntry.onResponse(new CachedResult(authResult, cacheHasher, authResult.getUser(), token.credentials()));
171174
listener.onResponse(authResult);
172175
}, e -> {
173-
// notify any staved off listeners; they will retry the request
176+
cache.invalidate(token.principal(), listenableCacheEntry);
177+
// notify any staved off listeners; they will propagate this error
174178
listenableCacheEntry.onFailure(e);
175-
// notify the listener of the inflight authentication request; this request is not retried
179+
// notify the listener of the inflight authentication request
176180
listener.onFailure(e);
177181
}));
178182
}
@@ -223,35 +227,31 @@ public final void lookupUser(String username, ActionListener<User> listener) {
223227
private void lookupWithCache(String username, ActionListener<User> listener) {
224228
try {
225229
final AtomicBoolean lookupInCache = new AtomicBoolean(true);
226-
final ListenableFuture<UserWithHash> listenableCacheEntry = cache.computeIfAbsent(username, key -> {
230+
final ListenableFuture<CachedResult> listenableCacheEntry = cache.computeIfAbsent(username, key -> {
227231
lookupInCache.set(false);
228232
return new ListenableFuture<>();
229233
});
230234
if (false == lookupInCache.get()) {
231235
// attempt lookup against the user directory
232236
doLookupUser(username, ActionListener.wrap(user -> {
233-
if (user != null) {
234-
// user found
235-
final UserWithHash userWithHash = new UserWithHash(user, null, null);
236-
// notify forestalled request listeners
237-
listenableCacheEntry.onResponse(userWithHash);
238-
} else {
237+
final CachedResult result = new CachedResult(AuthenticationResult.notHandled(), cacheHasher, user, null);
238+
if (user == null) {
239239
// user not found, invalidate cache so that subsequent requests are forwarded to
240240
// the user directory
241241
cache.invalidate(username, listenableCacheEntry);
242-
// notify forestalled request listeners
243-
listenableCacheEntry.onResponse(null);
244242
}
243+
// notify forestalled request listeners
244+
listenableCacheEntry.onResponse(result);
245245
}, e -> {
246246
// the next request should be forwarded, not halted by a failed lookup attempt
247247
cache.invalidate(username, listenableCacheEntry);
248248
// notify forestalled listeners
249249
listenableCacheEntry.onFailure(e);
250250
}));
251251
}
252-
listenableCacheEntry.addListener(ActionListener.wrap(userWithHash -> {
253-
if (userWithHash != null) {
254-
listener.onResponse(userWithHash.user);
252+
listenableCacheEntry.addListener(ActionListener.wrap(cachedResult -> {
253+
if (cachedResult.user != null) {
254+
listener.onResponse(cachedResult.user);
255255
} else {
256256
listener.onResponse(null);
257257
}
@@ -263,16 +263,21 @@ private void lookupWithCache(String username, ActionListener<User> listener) {
263263

264264
protected abstract void doLookupUser(String username, ActionListener<User> listener);
265265

266-
private static class UserWithHash {
267-
final User user;
268-
final char[] hash;
266+
private static class CachedResult {
267+
private final AuthenticationResult authenticationResult;
268+
private final User user;
269+
private final char[] hash;
269270

270-
UserWithHash(User user, SecureString password, Hasher hasher) {
271-
this.user = Objects.requireNonNull(user);
271+
private CachedResult(AuthenticationResult result, Hasher hasher, @Nullable User user, @Nullable SecureString password) {
272+
this.authenticationResult = Objects.requireNonNull(result);
273+
if (authenticationResult.isAuthenticated() && user == null) {
274+
throw new IllegalArgumentException("authentication cannot be successful with a null user");
275+
}
276+
this.user = user;
272277
this.hash = password == null ? null : hasher.hash(password);
273278
}
274279

275-
boolean verify(SecureString password) {
280+
private boolean verify(SecureString password) {
276281
return hash != null && Hasher.verifyHash(password, hash);
277282
}
278283
}

‎x-pack/plugin/security/src/test/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealmTests.java‎

Lines changed: 75 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -58,13 +58,13 @@ public void setup() {
5858
}
5959

6060
@After
61-
public void stop() throws InterruptedException {
61+
public void stop() {
6262
if (threadPool != null) {
6363
terminate(threadPool);
6464
}
6565
}
6666

67-
public void testCacheSettings() throws Exception {
67+
public void testCacheSettings() {
6868
String cachingHashAlgo = Hasher.values()[randomIntBetween(0, Hasher.values().length - 1)].name().toLowerCase(Locale.ROOT);
6969
int maxUsers = randomIntBetween(10, 100);
7070
TimeValue ttl = TimeValue.timeValueMinutes(randomIntBetween(10, 20));
@@ -323,7 +323,7 @@ private void sleepUntil(long until) throws InterruptedException {
323323
}
324324
}
325325

326-
public void testAuthenticateContract() throws Exception {
326+
public void testAuthenticateContract() {
327327
Realm realm = new FailingAuthenticationRealm(globalSettings, threadPool);
328328
PlainActionFuture<AuthenticationResult> future = new PlainActionFuture<>();
329329
realm.authenticate(new UsernamePasswordToken("user", new SecureString("pass")), future);
@@ -337,7 +337,7 @@ public void testAuthenticateContract() throws Exception {
337337
assertThat(e.getMessage(), containsString("whatever exception"));
338338
}
339339

340-
public void testLookupContract() throws Exception {
340+
public void testLookupContract() {
341341
Realm realm = new FailingAuthenticationRealm(globalSettings, threadPool);
342342
PlainActionFuture<User> future = new PlainActionFuture<>();
343343
realm.lookupUser("user", future);
@@ -351,7 +351,7 @@ public void testLookupContract() throws Exception {
351351
assertThat(e.getMessage(), containsString("lookup exception"));
352352
}
353353

354-
public void testReturnDifferentObjectFromCache() throws Exception {
354+
public void testReturnDifferentObjectFromCache() {
355355
final AtomicReference<User> userArg = new AtomicReference<>();
356356
final AtomicReference<AuthenticationResult> result = new AtomicReference<>();
357357
Realm realm = new AlwaysAuthenticateCachingRealm(globalSettings, threadPool) {
@@ -444,6 +444,76 @@ protected void doLookupUser(String username, ActionListener<User> listener) {
444444
assertEquals(1, authCounter.get());
445445
}
446446

447+
public void testUnauthenticatedResultPropagatesWithSameCreds() throws Exception {
448+
final String username = "username";
449+
final SecureString password = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING;
450+
final AtomicInteger authCounter = new AtomicInteger(0);
451+
final Hasher pwdHasher = Hasher.resolve(randomFrom("pbkdf2", "pbkdf2_1000", "bcrypt", "bcrypt9"));
452+
final String passwordHash = new String(pwdHasher.hash(password));
453+
RealmConfig config = new RealmConfig(new RealmConfig.RealmIdentifier("caching", "test_realm"), globalSettings,
454+
TestEnvironment.newEnvironment(globalSettings), new ThreadContext(Settings.EMPTY));
455+
456+
457+
458+
final int numberOfProcessors = Runtime.getRuntime().availableProcessors();
459+
final int numberOfThreads = scaledRandomIntBetween((numberOfProcessors + 1) / 2, numberOfProcessors * 3);
460+
final int numberOfIterations = scaledRandomIntBetween(20, 100);
461+
final CountDownLatch latch = new CountDownLatch(1 + numberOfThreads);
462+
List<Thread> threads = new ArrayList<>(numberOfThreads);
463+
final SecureString credsToUse = new SecureString(randomAlphaOfLength(12).toCharArray());
464+
final CachingUsernamePasswordRealm realm = new CachingUsernamePasswordRealm(config, threadPool) {
465+
@Override
466+
protected void doAuthenticate(UsernamePasswordToken token, ActionListener<AuthenticationResult> listener) {
467+
authCounter.incrementAndGet();
468+
// do something slow
469+
if (pwdHasher.verify(token.credentials(), passwordHash.toCharArray())) {
470+
listener.onFailure(new IllegalStateException("password auth should never succeed"));
471+
} else {
472+
listener.onResponse(AuthenticationResult.unsuccessful("password verification failed", null));
473+
}
474+
}
475+
476+
@Override
477+
protected void doLookupUser(String username, ActionListener<User> listener) {
478+
listener.onFailure(new UnsupportedOperationException("this method should not be called"));
479+
}
480+
};
481+
for (int i = 0; i < numberOfThreads; i++) {
482+
threads.add(new Thread(() -> {
483+
try {
484+
latch.countDown();
485+
latch.await();
486+
for (int i1 = 0; i1 < numberOfIterations; i1++) {
487+
final UsernamePasswordToken token = new UsernamePasswordToken(username, credsToUse);
488+
489+
realm.authenticate(token, ActionListener.wrap((result) -> {
490+
if (result.isAuthenticated()) {
491+
throw new IllegalStateException("invalid password led to an authenticated result: " + result);
492+
}
493+
assertThat(result.getMessage(), containsString("password verification failed"));
494+
}, (e) -> {
495+
logger.error("caught exception", e);
496+
fail("unexpected exception - " + e);
497+
}));
498+
}
499+
500+
} catch (InterruptedException e) {
501+
logger.error("thread was interrupted", e);
502+
Thread.currentThread().interrupt();
503+
}
504+
}));
505+
}
506+
507+
for (Thread thread : threads) {
508+
thread.start();
509+
}
510+
latch.countDown();
511+
for (Thread thread : threads) {
512+
thread.join();
513+
}
514+
assertEquals(numberOfIterations, authCounter.get());
515+
}
516+
447517
public void testCacheConcurrency() throws Exception {
448518
final String username = "username";
449519
final SecureString password = SecuritySettingsSourceField.TEST_PASSWORD_SECURE_STRING;
@@ -675,27 +745,4 @@ protected void doLookupUser(String username, ActionListener<User> listener) {
675745
listener.onResponse(new User(username, new String[]{"lookupRole1", "lookupRole2"}));
676746
}
677747
}
678-
679-
static class LookupNotSupportedRealm extends CachingUsernamePasswordRealm {
680-
681-
public final AtomicInteger authInvocationCounter = new AtomicInteger(0);
682-
public final AtomicInteger lookupInvocationCounter = new AtomicInteger(0);
683-
684-
LookupNotSupportedRealm(Settings globalSettings, ThreadPool threadPool) {
685-
super(new RealmConfig(new RealmConfig.RealmIdentifier("caching", "lookup-notsupported-test"), globalSettings,
686-
TestEnvironment.newEnvironment(globalSettings), threadPool.getThreadContext()), threadPool);
687-
}
688-
689-
@Override
690-
protected void doAuthenticate(UsernamePasswordToken token, ActionListener<AuthenticationResult> listener) {
691-
authInvocationCounter.incrementAndGet();
692-
listener.onResponse(AuthenticationResult.success(new User(token.principal(), new String[]{"testRole1", "testRole2"})));
693-
}
694-
695-
@Override
696-
protected void doLookupUser(String username, ActionListener<User> listener) {
697-
lookupInvocationCounter.incrementAndGet();
698-
listener.onFailure(new UnsupportedOperationException("don't call lookup if lookup isn't supported!!!"));
699-
}
700-
}
701748
}

0 commit comments

Comments
 (0)