Skip to content
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.

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

Security: reorder realms based on last success #36878

Merged
merged 7 commits into from Jan 10, 2019

Conversation

Projects
None yet
5 participants
@jaymode
Copy link
Member

commented Dec 20, 2018

This commit reorders the realm list for iteration based on the last
successful authentication for the given principal. This is an
optimization to prevent unnecessary iteration over realms if we can
make a smart guess on which realm to try first.

Security: reorder realms based on last success
This commit reorders the realm list for iteration based on the last
succesful authentication for the given principal. This is an
optimization to prevent unnecessary iteration over realms if we can
make a smart guess on which realm to try first.
@elasticmachine

This comment has been minimized.

Copy link

commented Dec 20, 2018

@tvernum

This comment has been minimized.

Copy link
Contributor

commented Dec 20, 2018

I'm concerned about changing this behaviour in a minor.

The problem comes when there are multiple realms that can handle the same principal, and the higher priority one is temporarily unavailable.
In that case we will cache the second realm as the authenticating realm, and the only solution is to clear the cache (which customers don't tend to do).

I think we should default the setting to false when it is backported to 6.7

@jaymode

This comment has been minimized.

Copy link
Member Author

commented Dec 20, 2018

I think we should default the setting to false when it is backported to 6.7

I am fine with doing that

@albertzaharovits
Copy link
Contributor

left a comment

LGTM

@tvernum
Copy link
Contributor

left a comment

I wonder if we want to do something about the cache on security index status change.
One of:

  1. don't populate the cache if the security index is red (probably because it has not yet been recovered)
  2. don't consult the cache if the security index is red
  3. clear the cache on security index state change (like NativeRealm.onSecurityIndexStateChange)

I think (3) is the more consistent approach with our other security caches.

int smartListIndex = 1;
for (int i = 1; i < defaultOrderedRealms.size(); i++) {
if (i != index) {
smartOrder.add(smartListIndex++, defaultOrderedRealms.get(i));

This comment has been minimized.

Copy link
@tvernum

tvernum Dec 28, 2018

Contributor

Why do we need the smartListIndex ? Can we just add to the end of the list?

jaymode added some commits Jan 7, 2019

@jaymode jaymode requested a review from tvernum Jan 9, 2019

@tvernum

tvernum approved these changes Jan 9, 2019

Copy link
Contributor

left a comment

👍

@jaymode jaymode merged commit 7163377 into elastic:master Jan 10, 2019

7 checks passed

CLA Commit author has signed the CLA
Details
elasticsearch-ci-1 Build finished.
Details
elasticsearch-ci-2 Build finished.
Details
elasticsearch-ci/default-distro Build finished.
Details
elasticsearch-ci/docbldesx Build finished.
Details
elasticsearch-ci/oss-distro-docs Build finished.
Details
elasticsearch-ci/packaging-sample Build finished.
Details

@jaymode jaymode deleted the jaymode:reorder_realm_cache branch Jan 10, 2019

jaymode added a commit that referenced this pull request Jan 10, 2019

Security: reorder realms based on last success (#36878)
This commit reorders the realm list for iteration based on the last
successful authentication for the given principal. This is an
optimization to prevent unnecessary iteration over realms if we can
make a smart guess on which realm to try first.

@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.