From 47cacc02073f87ac6204451b43815499111c89c3 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 3 Aug 2018 02:36:09 +0300 Subject: [PATCH 1/5] Removed dead if branch --- .../authc/support/CachingUsernamePasswordRealm.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java index bcdbc1e1dd305..76fd7baecfd7b 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java @@ -131,11 +131,7 @@ private void handleResult(ListenableFuture result, ActionListener listener) { final AuthenticationResult authResult = result.v1(); - if (authResult == null) { - // this was from a lookup; clear and redo - cache.invalidate(token.principal(), future); - authenticateWithCache(token, listener); - } else if (authResult.isAuthenticated()) { + if (authResult.isAuthenticated()) { if (performedAuthentication) { listener.onResponse(authResult); } else { From 3a66abfeffa683c894b8c2503b920a8d2cd354d3 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Mon, 6 Aug 2018 20:30:13 +0300 Subject: [PATCH 2/5] Lookup done too! --- .../support/CachingUsernamePasswordRealm.java | 211 +++++++++--------- 1 file changed, 100 insertions(+), 111 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java index 76fd7baecfd7b..997faf8653e3c 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java @@ -5,11 +5,9 @@ */ package org.elasticsearch.xpack.security.authc.support; -import org.apache.lucene.util.SetOnce; import org.elasticsearch.action.ActionListener; import org.elasticsearch.common.cache.Cache; import org.elasticsearch.common.cache.CacheBuilder; -import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.settings.SecureString; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.util.concurrent.ListenableFuture; @@ -30,7 +28,7 @@ public abstract class CachingUsernamePasswordRealm extends UsernamePasswordRealm implements CachingRealm { - private final Cache>> cache; + private final Cache> cache; private final ThreadPool threadPool; final Hasher cacheHasher; @@ -38,9 +36,9 @@ protected CachingUsernamePasswordRealm(String type, RealmConfig config, ThreadPo super(type, config); cacheHasher = Hasher.resolve(CachingUsernamePasswordRealmSettings.CACHE_HASH_ALGO_SETTING.get(config.settings())); this.threadPool = threadPool; - TimeValue ttl = CachingUsernamePasswordRealmSettings.CACHE_TTL_SETTING.get(config.settings()); + final TimeValue ttl = CachingUsernamePasswordRealmSettings.CACHE_TTL_SETTING.get(config.settings()); if (ttl.getNanos() > 0) { - cache = CacheBuilder.>>builder() + cache = CacheBuilder.>builder() .setExpireAfterWrite(ttl) .setMaximumWeight(CachingUsernamePasswordRealmSettings.CACHE_MAX_USERS_SETTING.get(config.settings())) .build(); @@ -49,6 +47,7 @@ protected CachingUsernamePasswordRealm(String type, RealmConfig config, ThreadPo } } + @Override public final void expire(String username) { if (cache != null) { logger.trace("invalidating cache for user [{}] in realm [{}]", username, name()); @@ -56,6 +55,7 @@ public final void expire(String username) { } } + @Override public final void expireAll() { if (cache != null) { logger.trace("invalidating cache for all users in realm [{}]", name()); @@ -72,14 +72,14 @@ public final void expireAll() { */ @Override public final void authenticate(AuthenticationToken authToken, ActionListener listener) { - UsernamePasswordToken token = (UsernamePasswordToken) authToken; + final UsernamePasswordToken token = (UsernamePasswordToken) authToken; try { if (cache == null) { doAuthenticate(token, listener); } else { authenticateWithCache(token, listener); } - } catch (Exception e) { + } catch (final Exception e) { // each realm should handle exceptions, if we get one here it should be considered fatal listener.onFailure(e); } @@ -87,89 +87,62 @@ public final void authenticate(AuthenticationToken authToken, ActionListener listener) { try { - final SetOnce authenticatedUser = new SetOnce<>(); - final AtomicBoolean createdAndStartedFuture = new AtomicBoolean(false); - final ListenableFuture> future = cache.computeIfAbsent(token.principal(), k -> { - final ListenableFuture> created = new ListenableFuture<>(); - if (createdAndStartedFuture.compareAndSet(false, true) == false) { - throw new IllegalStateException("something else already started this. how?"); - } - return created; - }); - - if (createdAndStartedFuture.get()) { - doAuthenticate(token, ActionListener.wrap(result -> { - if (result.isAuthenticated()) { - final User user = result.getUser(); - authenticatedUser.set(user); - final UserWithHash userWithHash = new UserWithHash(user, token.credentials(), cacheHasher); - future.onResponse(new Tuple<>(result, userWithHash)); + final AtomicBoolean cachedAuthentication = new AtomicBoolean(true); + final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> { + final ListenableFuture created = new ListenableFuture<>(); + // forward a new authenticate request to the external system + doAuthenticate(token, ActionListener.wrap(authResult -> { + if (authResult.isAuthenticated() && authResult.getUser().enabled()) { + // compute the hash of the successful authn request + final UserWithHash userWithHash = new UserWithHash(authResult.getUser(), token.credentials(), cacheHasher); + // notify forestalled request listeners + created.onResponse(userWithHash); } else { - future.onResponse(new Tuple<>(result, null)); + // the inflight request has failed to authenticate + // clear cache, the next request should be forwarded, not halted by a failed + // authn attempt + cache.invalidate(token.principal(), created); + // notify forestalled request listeners + created.onResponse(null); } - }, future::onFailure)); - } - - future.addListener(ActionListener.wrap(tuple -> { - if (tuple != null) { - final UserWithHash userWithHash = tuple.v2(); - final boolean performedAuthentication = createdAndStartedFuture.get() && userWithHash != null && - tuple.v2().user == authenticatedUser.get(); - handleResult(future, createdAndStartedFuture.get(), performedAuthentication, token, tuple, listener); - } else { - handleFailure(future, createdAndStartedFuture.get(), token, new IllegalStateException("unknown error authenticating"), - listener); - } - }, e -> handleFailure(future, createdAndStartedFuture.get(), token, e, listener)), - threadPool.executor(ThreadPool.Names.GENERIC)); - } catch (ExecutionException e) { - listener.onResponse(AuthenticationResult.unsuccessful("", e)); - } - } - - private void handleResult(ListenableFuture> future, boolean createdAndStartedFuture, - boolean performedAuthentication, UsernamePasswordToken token, - Tuple result, ActionListener listener) { - final AuthenticationResult authResult = result.v1(); - if (authResult.isAuthenticated()) { - if (performedAuthentication) { - listener.onResponse(authResult); - } else { - UserWithHash userWithHash = result.v2(); - if (userWithHash.verify(token.credentials())) { - if (userWithHash.user.enabled()) { - User user = userWithHash.user; - logger.debug("realm [{}] authenticated user [{}], with roles [{}]", - name(), token.principal(), user.roles()); + // notify the listener of the inflight authentication request + listener.onResponse(authResult); + }, e -> { + // the next request should be forwarded, not halted by a failed authn attempt + cache.invalidate(token.principal(), created); + // notify staved off listeners + created.onFailure(e); + // notify the listener of the inflight authentication request + listener.onFailure(e); + })); + cachedAuthentication.set(false); + return created; + }); + if (cachedAuthentication.get()) { + // there is a cached or an inflight authenticate request + listenableCacheEntry.addListener(ActionListener.wrap(authenticatedUserWithHash -> { + if (null == authenticatedUserWithHash) { + // inflight request has failed, try again to reach for the external system + authenticateWithCache(token, listener); + } else if (authenticatedUserWithHash.verify(token.credentials())) { + // cached hash matches the credentials for this forestalled request + final User user = authenticatedUserWithHash.user; + logger.debug("realm [{}] authenticated user [{}], with roles [{}]", name(), token.principal(), user.roles()); listener.onResponse(AuthenticationResult.success(user)); } else { - // re-auth to see if user has been enabled - cache.invalidate(token.principal(), future); + // cached hash does not match the credentials for this forestalled request. + // however, we should clear cache and try to reach the external system again + // because password might have changed and the cached hash got stale + cache.invalidate(token.principal(), listenableCacheEntry); authenticateWithCache(token, listener); } - } else { - // could be a password change? - cache.invalidate(token.principal(), future); + }, e -> { + // try again, the inflight request failed authenticateWithCache(token, listener); - } - } - } else { - cache.invalidate(token.principal(), future); - if (createdAndStartedFuture) { - listener.onResponse(authResult); - } else { - authenticateWithCache(token, listener); + }), threadPool.executor(ThreadPool.Names.GENERIC)); } - } - } - - private void handleFailure(ListenableFuture> future, boolean createdAndStarted, - UsernamePasswordToken token, Exception e, ActionListener listener) { - cache.invalidate(token.principal(), future); - if (createdAndStarted) { + } catch (final ExecutionException e) { listener.onFailure(e); - } else { - authenticateWithCache(token, listener); } } @@ -189,38 +162,54 @@ protected int getCacheSize() { @Override public final void lookupUser(String username, ActionListener listener) { - if (cache != null) { - try { - ListenableFuture> future = cache.computeIfAbsent(username, key -> { - ListenableFuture> created = new ListenableFuture<>(); - doLookupUser(username, ActionListener.wrap(user -> { - if (user != null) { - UserWithHash userWithHash = new UserWithHash(user, null, null); - created.onResponse(new Tuple<>(null, userWithHash)); - } else { - created.onResponse(new Tuple<>(null, null)); - } - }, created::onFailure)); - return created; - }); - - future.addListener(ActionListener.wrap(tuple -> { - if (tuple != null) { - if (tuple.v2() == null) { - cache.invalidate(username, future); - listener.onResponse(null); - } else { - listener.onResponse(tuple.v2().user); - } + try { + if (cache == null) { + doLookupUser(username, listener); + } else { + lookupWithCache(username, listener); + } + } catch (final Exception e) { + // each realm should handle exceptions, if we get one here it should be + // considered fatal + listener.onFailure(e); + } + } + + private void lookupWithCache(String username, ActionListener listener) { + try { + final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(username, key -> { + final ListenableFuture created = new ListenableFuture<>(); + // forward a new lookup request to the external system + doLookupUser(username, ActionListener.wrap(user -> { + if (user != null) { + // user found + final UserWithHash userWithHash = new UserWithHash(user, null, null); + // notify forestalled request listeners + created.onResponse(userWithHash); } else { - listener.onResponse(null); + // user not found, invalidate cache so that subsequent requests are forwarded to + // the external system + cache.invalidate(username, created); + // notify forestalled request listeners + created.onResponse(null); } - }, listener::onFailure), threadPool.executor(ThreadPool.Names.GENERIC)); - } catch (ExecutionException e) { - listener.onFailure(e); - } - } else { - doLookupUser(username, listener); + }, e -> { + // the next request should be forwarded, not halted by a failed lookup attempt + cache.invalidate(username, created); + // notify forestalled listeners + created.onFailure(e); + })); + return created; + }); + listenableCacheEntry.addListener(ActionListener.wrap(userWithHash -> { + if (userWithHash != null) { + listener.onResponse(userWithHash.user); + } else { + listener.onResponse(null); + } + }, listener::onFailure), threadPool.executor(ThreadPool.Names.GENERIC)); + } catch (final ExecutionException e) { + listener.onFailure(e); } } From 78c132d52801e4e1fcc10b927fbba312fbcba5e3 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Thu, 23 Aug 2018 17:24:02 +0300 Subject: [PATCH 3/5] Address feedback --- .../support/CachingUsernamePasswordRealm.java | 61 +++++++++++-------- 1 file changed, 34 insertions(+), 27 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java index 997faf8653e3c..bd68bc84053c3 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java @@ -85,59 +85,66 @@ public final void authenticate(AuthenticationToken authToken, ActionListenersame password, will succeed + * without reaching to the authentication source. A different password in a + * subsequent request, however, will clear the cache and try to reach to + * the authentication source. + * + * @param token The authentication token + * @param listener to be called at completion + */ private void authenticateWithCache(UsernamePasswordToken token, ActionListener listener) { try { - final AtomicBoolean cachedAuthentication = new AtomicBoolean(true); + final AtomicBoolean authenticationInCache = new AtomicBoolean(true); final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> { + authenticationInCache.set(false); final ListenableFuture created = new ListenableFuture<>(); - // forward a new authenticate request to the external system + // attempt authentication against the authentication source doAuthenticate(token, ActionListener.wrap(authResult -> { if (authResult.isAuthenticated() && authResult.getUser().enabled()) { - // compute the hash of the successful authn request + // compute the credential hash of this successful authentication request final UserWithHash userWithHash = new UserWithHash(authResult.getUser(), token.credentials(), cacheHasher); - // notify forestalled request listeners + // notify any forestalled request listeners; they will not reach to the + // authentication request and instead will use this hash for comparison created.onResponse(userWithHash); } else { - // the inflight request has failed to authenticate - // clear cache, the next request should be forwarded, not halted by a failed - // authn attempt - cache.invalidate(token.principal(), created); - // notify forestalled request listeners + // notify any forestalled request listeners; they will retry the request created.onResponse(null); } - // notify the listener of the inflight authentication request + // notify the listener of the inflight authentication request; this request is not retried listener.onResponse(authResult); }, e -> { - // the next request should be forwarded, not halted by a failed authn attempt - cache.invalidate(token.principal(), created); - // notify staved off listeners + // notify any staved off listeners; they will retry the request created.onFailure(e); - // notify the listener of the inflight authentication request + // notify the listener of the inflight authentication request; this request is not retried listener.onFailure(e); })); - cachedAuthentication.set(false); return created; }); - if (cachedAuthentication.get()) { + if (authenticationInCache.get()) { // there is a cached or an inflight authenticate request listenableCacheEntry.addListener(ActionListener.wrap(authenticatedUserWithHash -> { - if (null == authenticatedUserWithHash) { - // inflight request has failed, try again to reach for the external system - authenticateWithCache(token, listener); - } else if (authenticatedUserWithHash.verify(token.credentials())) { - // cached hash matches the credentials for this forestalled request + if (authenticatedUserWithHash != null && authenticatedUserWithHash.verify(token.credentials())) { + // cached credential hash matches the credential hash for this forestalled request final User user = authenticatedUserWithHash.user; - logger.debug("realm [{}] authenticated user [{}], with roles [{}]", name(), token.principal(), user.roles()); + logger.debug("realm [{}] authenticated user [{}], with roles [{}], from cache", name(), token.principal(), + user.roles()); listener.onResponse(AuthenticationResult.success(user)); } else { - // cached hash does not match the credentials for this forestalled request. - // however, we should clear cache and try to reach the external system again - // because password might have changed and the cached hash got stale + // The inflight request has failed or its credential hash does not match the + // hash of the credential for this forestalled request. + // clear cache and try to reach the authentication source again because password + // might have changed there and the local cached hash got stale cache.invalidate(token.principal(), listenableCacheEntry); authenticateWithCache(token, listener); } }, e -> { - // try again, the inflight request failed + // the inflight request failed, so try again, but first (always) make sure cache + // is cleared of the failed authentication + cache.invalidate(token.principal(), listenableCacheEntry); authenticateWithCache(token, listener); }), threadPool.executor(ThreadPool.Names.GENERIC)); } @@ -179,7 +186,7 @@ private void lookupWithCache(String username, ActionListener listener) { try { final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(username, key -> { final ListenableFuture created = new ListenableFuture<>(); - // forward a new lookup request to the external system + // attempt authentication against authentication source doLookupUser(username, ActionListener.wrap(user -> { if (user != null) { // user found From ec56c35fa91b2f5758fa20ad70ce0e62e7628dd9 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 24 Aug 2018 12:55:17 +0300 Subject: [PATCH 4/5] Address sloppy comment --- .../security/authc/support/CachingUsernamePasswordRealm.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java index bd68bc84053c3..2276be8ed8619 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java @@ -186,7 +186,7 @@ private void lookupWithCache(String username, ActionListener listener) { try { final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(username, key -> { final ListenableFuture created = new ListenableFuture<>(); - // attempt authentication against authentication source + // attempt lookup against the user directory doLookupUser(username, ActionListener.wrap(user -> { if (user != null) { // user found @@ -195,7 +195,7 @@ private void lookupWithCache(String username, ActionListener listener) { created.onResponse(userWithHash); } else { // user not found, invalidate cache so that subsequent requests are forwarded to - // the external system + // the user directory cache.invalidate(username, created); // notify forestalled request listeners created.onResponse(null); From 6a8b45245eff535d2456d9c1e6f97853ebcb5ca3 Mon Sep 17 00:00:00 2001 From: Albert Zaharovits Date: Fri, 24 Aug 2018 20:41:04 +0300 Subject: [PATCH 5/5] move doAuthenticate and doLookup outside of computeIfAbsent --- .../support/CachingUsernamePasswordRealm.java | 63 ++++++++++--------- 1 file changed, 33 insertions(+), 30 deletions(-) diff --git a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java index 2276be8ed8619..ab559cebe54e5 100644 --- a/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java +++ b/x-pack/plugin/security/src/main/java/org/elasticsearch/xpack/security/authc/support/CachingUsernamePasswordRealm.java @@ -101,28 +101,7 @@ private void authenticateWithCache(UsernamePasswordToken token, ActionListener listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> { authenticationInCache.set(false); - final ListenableFuture created = new ListenableFuture<>(); - // attempt authentication against the authentication source - doAuthenticate(token, ActionListener.wrap(authResult -> { - if (authResult.isAuthenticated() && authResult.getUser().enabled()) { - // compute the credential hash of this successful authentication request - final UserWithHash userWithHash = new UserWithHash(authResult.getUser(), token.credentials(), cacheHasher); - // notify any forestalled request listeners; they will not reach to the - // authentication request and instead will use this hash for comparison - created.onResponse(userWithHash); - } else { - // notify any forestalled request listeners; they will retry the request - created.onResponse(null); - } - // notify the listener of the inflight authentication request; this request is not retried - listener.onResponse(authResult); - }, e -> { - // notify any staved off listeners; they will retry the request - created.onFailure(e); - // notify the listener of the inflight authentication request; this request is not retried - listener.onFailure(e); - })); - return created; + return new ListenableFuture<>(); }); if (authenticationInCache.get()) { // there is a cached or an inflight authenticate request @@ -147,6 +126,27 @@ private void authenticateWithCache(UsernamePasswordToken token, ActionListener { + if (authResult.isAuthenticated() && authResult.getUser().enabled()) { + // compute the credential hash of this successful authentication request + final UserWithHash userWithHash = new UserWithHash(authResult.getUser(), token.credentials(), cacheHasher); + // notify any forestalled request listeners; they will not reach to the + // authentication request and instead will use this hash for comparison + listenableCacheEntry.onResponse(userWithHash); + } else { + // notify any forestalled request listeners; they will retry the request + listenableCacheEntry.onResponse(null); + } + // notify the listener of the inflight authentication request; this request is not retried + listener.onResponse(authResult); + }, e -> { + // notify any staved off listeners; they will retry the request + listenableCacheEntry.onFailure(e); + // notify the listener of the inflight authentication request; this request is not retried + listener.onFailure(e); + })); } } catch (final ExecutionException e) { listener.onFailure(e); @@ -184,30 +184,33 @@ public final void lookupUser(String username, ActionListener listener) { private void lookupWithCache(String username, ActionListener listener) { try { + final AtomicBoolean lookupInCache = new AtomicBoolean(true); final ListenableFuture listenableCacheEntry = cache.computeIfAbsent(username, key -> { - final ListenableFuture created = new ListenableFuture<>(); + lookupInCache.set(false); + return new ListenableFuture<>(); + }); + if (false == lookupInCache.get()) { // attempt lookup against the user directory doLookupUser(username, ActionListener.wrap(user -> { if (user != null) { // user found final UserWithHash userWithHash = new UserWithHash(user, null, null); // notify forestalled request listeners - created.onResponse(userWithHash); + listenableCacheEntry.onResponse(userWithHash); } else { // user not found, invalidate cache so that subsequent requests are forwarded to // the user directory - cache.invalidate(username, created); + cache.invalidate(username, listenableCacheEntry); // notify forestalled request listeners - created.onResponse(null); + listenableCacheEntry.onResponse(null); } }, e -> { // the next request should be forwarded, not halted by a failed lookup attempt - cache.invalidate(username, created); + cache.invalidate(username, listenableCacheEntry); // notify forestalled listeners - created.onFailure(e); + listenableCacheEntry.onFailure(e); })); - return created; - }); + } listenableCacheEntry.addListener(ActionListener.wrap(userWithHash -> { if (userWithHash != null) { listener.onResponse(userWithHash.user);