Refactor CachingUsernamePassword realm #32646
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
albertzaharovits
added
>non-issue
review
v7.0.0
:Security/Authentication
|
Pinging @elastic/es-security |
@elasticmachine test this please |
jaymode
requested changes
Aug 10, 2018
| final AtomicBoolean cachedAuthentication = new AtomicBoolean(true); | ||
| final ListenableFuture<UserWithHash> listenableCacheEntry = cache.computeIfAbsent(token.principal(), k -> { | ||
| final ListenableFuture<UserWithHash> created = new ListenableFuture<>(); | ||
| // forward a new authenticate request to the external system |
There was a problem hiding this comment.
this is a nit but the authentication system may be internal too. So I'd update the comment to say something like attempt authentication against authentication source
| // notify the listener of the inflight authentication request | ||
| listener.onFailure(e); | ||
| })); | ||
| cachedAuthentication.set(false); |
There was a problem hiding this comment.
we should move this to before the call to doAuthenticate. Even though doAuthenticate is asynchronous there is no requirement that another thread is forked.
There was a problem hiding this comment.
I don't think it makes any difference if setting this flag is before or after the doAuthenticate.
It is not used in the doAuthenticate listeners.
I have moved it nonetheless because it is closer to where the flag is declared and it is probably easier to reason about it.
| authenticatedUser.set(user); | ||
| final UserWithHash userWithHash = new UserWithHash(user, token.credentials(), cacheHasher); | ||
| future.onResponse(new Tuple<>(result, userWithHash)); | ||
| final AtomicBoolean cachedAuthentication = new AtomicBoolean(true); |
There was a problem hiding this comment.
can we rename this to authenticationInCache? cachedAuthentication could mean a few different things to me such as creating the future and putting it in the cache
| logger.debug("realm [{}] authenticated user [{}], with roles [{}]", | ||
| name(), token.principal(), user.roles()); | ||
| // notify the listener of the inflight authentication request | ||
| listener.onResponse(authResult); |
There was a problem hiding this comment.
If I understand correctly, the listener is not added to the future because we don't want to create a loop in the failure cases? If so, can you please document this aspect.
There was a problem hiding this comment.
If I understand correctly, the listener is not added to the future because we don't want to create a loop in the failure cases?
Yes, the if (authenticationInCache) only adds a listener if this is not the inflight request. The listener retries the request if passwords don't match or the authn failed (when the inflight request returns). The inflight request's result, however, is definitive, it will not be retried, it has reached to the "source of truth" and if it has failed, there is no point in retrying. This strategy, of not retrying requests if they have reached to the source of truth, has the consequence of avoiding the loop in the failure case; given a set of requests that have to be retried, at least one will be handled in the next loop (and not retried anymore) - the one that had reached to the source of authentication.
This has not changed in this refactoring.
I have added comments about when retries happen (and don't happen). I hope it is clearer now.
| try { | ||
| final ListenableFuture<UserWithHash> listenableCacheEntry = cache.computeIfAbsent(username, key -> { | ||
| final ListenableFuture<UserWithHash> created = new ListenableFuture<>(); | ||
| // forward a new lookup request to the external system |
There was a problem hiding this comment.
same comment about external system changing to authentication source.
|
@jaymode I have addressed your review. Please take it to another inspection when you get some cycles :) |
tvernum
approved these changes
Aug 24, 2018
| try { | ||
| final ListenableFuture<UserWithHash> listenableCacheEntry = cache.computeIfAbsent(username, key -> { | ||
| final ListenableFuture<UserWithHash> created = new ListenableFuture<>(); | ||
| // attempt authentication against authentication source |
There was a problem hiding this comment.
This comment is incorrect - there's no "authentication" taking place here.
albertzaharovits
force-pushed
the
caching_realm_refactor
branch
from
867be46
to
5d55ab5
Compare
|
Soooo, I can already hear the providential "I told you so". It turns out I would still merge it, without straining your eyes again, since the change is minor - moving the function call outside of the Here is the stacktrace: |
jaymode
requested changes
Aug 24, 2018
| } | ||
| } else { | ||
| doLookupUser(username, listener); | ||
| listenableCacheEntry.addListener(ActionListener.wrap(userWithHash -> { |
There was a problem hiding this comment.
this should be in an else block IMO
There was a problem hiding this comment.
no, for lookup there is, and there was, no retrying; the listener for the the reaching-out-request is notified just like all the other listeners.
When a lookup returns negatively, it clears the cache, but deferred requests will not be retried, they return negatively as well.
| lookupInCache.set(false); | ||
| return new ListenableFuture<>(); | ||
| }); | ||
| if (false == lookupInCache.get()) { |
There was a problem hiding this comment.
can we invert this condition to make it more like the authenticate version? so the actual lookup happens in the else of this if statement
There was a problem hiding this comment.
lookupInCache is tailored to be similar to authenticationInCache, just that, in the if branch, there is no action (cf. the next comment https://github.com/elastic/elasticsearch/pull/32646/files#r212724950).
Should I still invert it?
jaymode
approved these changes
Aug 24, 2018
Build was successful, but GUI looks stuck in pending, I will retry out of diligence. @elasticmachine test this please |
albertzaharovits
added a commit
that referenced
this pull request
Aug 26, 2018
Refactors the logic of authentication and lookup caching in `CachingUsernamePasswordRealm`. Nothing changed about the single-inflight-request or positive caching.
jasontedor
added a commit
to jasontedor/elasticsearch
that referenced
this pull request
Aug 26, 2018
* master: Fix a mappings update test (elastic#33146) Reload Secure Settings REST specs & docs (elastic#32990) Refactor CachingUsernamePassword realm (elastic#32646) Add proxy support to RemoteClusterConnection (elastic#33062)
jasontedor
added a commit
that referenced
this pull request
Aug 27, 2018
* master: Adjust BWC version on mapping version Token API supports the client_credentials grant (#33106) Build: forked compiler max memory matches jvmArgs (#33138) Introduce mapping version to index metadata (#33147) SQL: Enable aggregations to create a separate bucket for missing values (#32832) Fix grammar in contributing docs SECURITY: Fix Compile Error in ReservedRealmTests (#33166) APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Fix forbiddenapis on java 11 (#33116) Apply publishing to genreate pom (#33094) Have circuit breaker succeed on unknown mem usage Do not lose default mapper on metadata updates (#33153) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)
jasontedor
added a commit
that referenced
this pull request
Aug 27, 2018
* 6.x: Introduce mapping version to index metadata (#33147) Move non duplicated actions back into xpack core (#32952) HLRC: Create server agnostic request and response (#32912) Build: forked compiler max memory matches jvmArgs (#33138) * Added breaking change section for GROUP BY behavior: now it considers null or empty values as a separate group/bucket. Previously, they were ignored. * This is part of backporting of #32832 SQL: Enable aggregations to create a separate bucket for missing values (#32832) [TEST] version guard for reload rest-api-spec Fix grammar in contributing docs APM server monitoring (#32515) Support only string `format` in date, root object & date range (#28117) [Rollup] Move toBuilders() methods out of rollup config objects (#32585) Accept Gradle build scan agreement (#30645) Fix forbiddenapis on java 11 (#33116) Run forbidden api checks with runtimeJavaVersion (#32947) Apply publishing to genreate pom (#33094) Fix a mappings update test (#33146) Reload Secure Settings REST specs & docs (#32990) Refactor CachingUsernamePassword realm (#32646)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This is a refactoring of the gnarly caching logic in
CachingUsernamePasswordRealm. I've been mulling over #30794 (review).The refactoring concerns the
authenticateWithCachemethod;lookupWithCachehas only been revamped so that it follows the same pattern.There is now a distinction between the listener of the in-flight authn request and the listeners of requests returned from cache. This greatly simplifies branching, removing redundancy and dead branches.