Add authorizing_realms support to PKI realm #31643

tvernum · 2018-06-28T04:09:27Z

Authorizing Realms allow an authenticating realm to delegate the task
of constructing a User object (with name, roles, etc) to one or more
other realms.
This commit allows the PKI realm to delegate authorization to any
other configured realm

I've selected the name "authorizing realms" for what we've been calling lookup realms, but that can be changed if there are strong opinions.

This feature-branch PR adds support to the PKI realm only.
To come:

License checking
Support in other realms
Docs

Authorizing Realms allow an authenticating realm to delegate the task of constructing a User object (with name, roles, etc) to one or more other realms. This commit allows the PKI realm to delegate authorization to any other configured realm

elasticmachine · 2018-06-28T04:09:58Z

Pinging @elastic/es-security

bizybot · 2018-06-28T09:40:11Z

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

+                    listener.onResponse(result);
+                }, listener::onFailure);
+                if (delegatedRealms.hasDelegation()) {
+                    delegatedRealms.resolveUser(token.principal(), cachingListener);


do we want to cache when we are using delegated realms for resolving user? because we are doing lookups in any case

I asked myself the same question while I was implementing it.
I decided that consistency was preferable so we use the cache in all cases.

Did you have a specific concern? I'm not tied to the current approach it just seemed the more consistenct one.

Not a specific concern, but just more configuration options for the end user when it is not being that effective. The code is trivial and not of maintenance concern so I am fine with we being consistent in all cases.

bizybot · 2018-06-28T09:40:44Z

.../main/java/org/elasticsearch/xpack/security/authc/support/DelegatedAuthorizationSupport.java

+        return this.lookupRealms.isEmpty() == false;
+    }
+
+    public void resolveUser(String username, ActionListener<AuthenticationResult> resultListener) {


As this is resolving user, the action listener should return Optional<User> instead of AuthenticationResult, we are not authenticating here but just trying to resolve a user.

I understand the concern.
The reason I chose to respond with the AuthenticationResult is that it primarily exists as a wrapper for passing back failure messages during authentication and that's what we want here.
I could call onFailure instead, but then every realm would need to translate that to an Authentication failure.

Would you be happier if I renamed the method to refer to Authentication? Or do you fundamentally think that this class should only deal with users?

I am replying here but the discussion is applicable to other places below:

With current implementation I feel DelegatedAuthorizationSupport is deciding on
the AuthenticationResult. I still think it should just be concerned with user lookup and its result.

Yes, I meant to call onResponse with an Optional<User> as result as one option instead of AuthenticationResult.

Let's take a following example to drive through the discussion:
Delegating Realm - PkiRealm
Authorizing Realms - dapRealm, NativeRealm
Users: UserA (LdapRealm) UserB (NativeRealm) both authenticated by PkiRealm but depend on authorizing realms for user details.

With this scenario, we expect for UserA, LdapRealm provides user details whereas for UserB it is NativeRealm.
This works fine till LdapRealm say is unable to connect to remote server. This now fails UserB authentication.
I think the expectation here may be if LdapRealm is not available continue further down the line to find a user.
But as we are terminating the lookup this does not happen for UserB and it is also denied access.

So I think it's better to let the delegating realm know of the result of user lookup as Optional<User> denoting if User was found or is absent. The handling of the result is the responsibility of Delegating Realm about what to do next. What do you think?

Using Optional has an alternative that you discuss of calling onFailure with an exception at the end of the loop.
I am fine with it as long as the delegating realm has control over what to do in case of success and failure scenarios.

The other condition was on entry to resolveUser without checking hasDelegation. If we go above way it simplifies that we just say the user not found/is absent and the delegating realm takes a decision on what to to do next.

Yes, there might be similar error handling for all realms but mostly the behavior would be what those delegating realms want.

This works fine till LdapRealm say is unable to connect to remote server. This now fails UserB authentication.

I don't follow why this would be the case. LdapRealm.lookupUser will respond with null, and so the next realm will be tried. Am I missing something?

Maybe I am, but I looked at this -> For LdapRealm here I see if LdapException is thrown while connecting it will call onFailure with LdapException. I think IteratingActionListener continues only when the onResponse is called with null value to consume next one if available.

The LdapRealm translates those failures to a null result.

Thanks, I missed that. But this would be applicable for any other realm which does not handle it the way LDAP handles? Is that expected that lookupUser always returns either the user or null and no exception onFailure?

bizybot · 2018-06-28T09:40:49Z

.../main/java/org/elasticsearch/xpack/security/authc/support/DelegatedAuthorizationSupport.java

+
+    public void resolveUser(String username, ActionListener<AuthenticationResult> resultListener) {
+        if (lookupRealms.isEmpty()) {
+            throw new IllegalStateException("No realms have been configured for delegation");


instead, IMO we should return Optional here.

Really? Because the method doesn't actually return anything, so it would need to call onResponse with an Optional, and you wouldn't necessarily know what the absent value represented.
Does it mean no realms configured, or does it mean the user couldn't be found?

Maybe you could show me how it might work, because I'm not seeing it.

Contractually, I think it is an error to call this if there are no lookup realms. I could see an argument for passing the exception through onFailure instead, but if this method is called without any lookup realms then the calling code (the originating realm) has a bug, because it should check hasDelegation() first.

I did originally have an interface where the "if no lookup" body was passed in as a Function so this method never threw an exception, but it made the calling code messier, with no benefit.

Discussed in above comment.

bizybot · 2018-06-28T09:40:52Z

.../main/java/org/elasticsearch/xpack/security/authc/support/DelegatedAuthorizationSupport.java

+            if (user != null) {
+                resultListener.onResponse(AuthenticationResult.success(user));
+            } else {
+                resultListener.onResponse(AuthenticationResult.unsuccessful("the principal [" + username


But then every caller would need to reproduce the same error handling. What gain do you see?

Discussed in above comment.

bizybot · 2018-06-28T09:41:03Z

.../main/java/org/elasticsearch/xpack/security/authc/support/DelegatedAuthorizationSupport.java

+                return realm;
+            }
+        }
+        throw new IllegalStateException("configured authorizing realm [" + name + "] does not exist");


or it can be a disabled realm

Thanks. I'll handle that.

bizybot

Thank you. I left some comments.

…-pki

tvernum · 2018-07-06T07:20:09Z

@bizybot I've updated per our discussion. There's now a class that handles User lookup only, and is also used by AuthenticationService, and DelegatedAuthorizationSupport just wraps that add special handling for use in delegated authz.

bizybot

LGTM, Thank you.

…-pki

jaymode · 2018-07-09T15:57:14Z

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

@@ -93,6 +94,14 @@ public PkiRealm(RealmConfig config, ResourceWatcherService watcherService, Nativ
                .build();
    }

+    @Override
+    public void initialize(Iterable<Realm> realms) {
+        if(delegatedRealms != null) {


nit: space between if and (

Grr, I changed my pre-commit hook and broke this check.

jaymode · 2018-07-09T15:59:06Z

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

@@ -75,6 +75,7 @@
    private final Pattern principalPattern;
    private final UserRoleMapper roleMapper;
    private final Cache<BytesKey, User> cache;
+    private DelegatedAuthorizationSupport delegatedRealms;


maybe we should just use a SetOnce which would enforce the only initialized once

I've switched this to SetOnce.

I had considered it when writing the code, but I find it hard to weigh up the readability & utility benefits of the setter vs the cost of having .get() calls everywhere it's used.

Do you have a particular reason for liking SetOnce in these cases?

For this case I was thinking about initializing the value and multi-threading (does the value need to be volatile), but now I realize that's not really an issue so the old way is fine. Please feel free to go back to that

jaymode · 2018-07-09T16:01:27Z

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

        X509AuthenticationToken token = (X509AuthenticationToken)authToken;
        try {
            final BytesKey fingerprint = computeFingerprint(token.credentials()[0]);
            User user = cache.get(fingerprint);
            if (user != null) {
-                listener.onResponse(AuthenticationResult.success(user));
+                if (delegatedRealms.hasDelegation()) {


if the user is in the cache, why are we resolving anything?

Assume that the lookup is being done on LDAP (which is likely) then we might expect any of the following to be automatically reflected in the results of an authentication:

role mapping file change

role mapping index/API change

explicit LDAP clear cache (possibly)

The PKI realm doesn't clear its own cache for those events, but the LDAP realm does.
We could change the PKI realm to detect those events and clear the cache, but it's more straight-forward to never rely on the local Realm's cache, and always lookup the other (LDAP) realm which will have its own cache.

There is the question that @bizybot raised about whether we even care about the PKI cache in this case.
And for PKI we could take the path of not consulting the local cache. When we extend this to UsernamePassword realms, we do need to look at the local cache for Authc purposes (to get the faster hash & avoid hitting the external directory), so I elected to be consistent here and also use the local cache.
I could easily be swayed to a different approach if there was an argument to do so.

The PKI realm should clear its own cache for both of the role mapping changes as of #31510. For the explicit LDAP clear cache, it is the cache clearing of a different realm and this user is technically coming from the PKI realm and the cache should be cleared for that user in the PKI realm; now if the PKI realm has authorizing realms that are caching realms, then it should delegate the call to clear the cache for the user to those other realms as well.

The PKI realm should clear its own cache for both of the role mapping changes

You're correct - this is a bad example, although there is no guarantee that the PKI realm and LDAP realm are using & monitoring the same role mapping file.

But it was just an example - in the general case, the authenticating realm doesn't know what conditions ought to trigger a cache-invalidation for the subordinate (delegated) realm.

In the current implementation of lookup user, where the user from the delegated realm is returned as-is, I think this caching approach is sound. It defers the caching of roles/metadata entirely to the delegated realm which is where those decisions are made.
However, if we take the approach of merging data from both realms, then that will force us to revisit the cache question, so I think we can hold off on a decision about caching until we conclude that conversation.

jaymode · 2018-07-09T16:07:09Z

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

-                }, listener::onFailure));
+                    listener.onResponse(result);
+                }, listener::onFailure);
+                if (delegatedRealms.hasDelegation()) {


Hmm I don't like that we do not get the metadata from the pki realm when we use a delegating realm and we do not even attempt to map roles. There may be cases where a PKI cert doesn't map to an AD/LDAP user but role mapping is desired, so we now need two realms.

I think this is worth discussing in person. I intentionally opted for this approach because I think its what some customers will want. Maybe less so for PKI, but for LDAP, I believe there will be a desire to authc against LDAP, but then lookup in the native realm, and fail auth if the native user doesn't exist.

I think it's worth talking this through.

++ to talking this through but to put it out there, what I am thinking is that we re-build the user after the lookup.

For this case we have PkiUser and LookedUpUser. The final user will be the combination of the PkiUser's metadata, the LookedUpUser's metadata, and the LookedUpUser's roles. The looked up user's metadata would trump the PkiUser's metadata in case of a conflict.

This does get trickier when you do this in an AD/LDAP realm since some of the metadata comes from the group resolution. In that case, I would only include the metadata that does not involve group resolution from the authenticating realm.

jaymode · 2018-07-09T16:07:41Z

.../main/java/org/elasticsearch/xpack/security/authc/support/DelegatedAuthorizationSupport.java

+
+import static org.elasticsearch.common.Strings.collectionToDelimitedString;
+
+public class DelegatedAuthorizationSupport {


please add javadocs to the class and methods

Another precommit fail 😞

jaymode · 2018-07-09T16:08:51Z

...n/security/src/test/java/org/elasticsearch/xpack/security/authc/support/MockLookupRealm.java

+import java.util.HashMap;
+import java.util.Map;
+
+public class MockLookupRealm extends Realm {


make it package private?

It's also used in PkiRealmTests

…-pki

jaymode

LGTM

Add authorizing_realms support to PKI realm

aecdc22

Authorizing Realms allow an authenticating realm to delegate the task of constructing a User object (with name, roles, etc) to one or more other realms. This commit allows the PKI realm to delegate authorization to any other configured realm

tvernum requested review from bizybot and jaymode June 28, 2018 04:09

tvernum added >feature review :Security/Authentication Logging in, Usernames/passwords, Realms (Native/LDAP/AD/SAML/PKI/etc) :Security/Authorization Roles, Privileges, DLS/FLS, RBAC/ABAC labels Jun 28, 2018

bizybot reviewed Jun 28, 2018

View reviewed changes

tvernum added 5 commits July 4, 2018 17:51

Merge branch 'security-lookup-realms' into feature/31267-lookup-realm…

9d4032f

…-pki

Fix error message

a0e7222

Fix test

6a80c4d

Merge branch 'master' into feature/31267-lookup-realm-pki

aefab91

Extract common user lookup code from Authc Service

3be69e4

Cleanup error message

32f5e8e

bizybot approved these changes Jul 9, 2018

View reviewed changes

tvernum added 2 commits July 9, 2018 12:58

Merge branch 'security-lookup-realms' into feature/31267-lookup-realm…

4fd642e

…-pki

Fix error check in test

f7e22d9

jaymode requested changes Jul 9, 2018

View reviewed changes

tvernum added 4 commits July 10, 2018 14:53

Merge branch 'security-lookup-realms' into feature/31267-lookup-realm…

efbda3d

…-pki

Address feedback & add test

e4c9d57

Merge branch 'security-lookup-realms' into feature/31267-lookup-realm…

94500a6

…-pki

Revert use of SetOnce

6eec3f3

jaymode approved these changes Jul 16, 2018

View reviewed changes

tvernum merged commit c363a84 into elastic:security-lookup-realms Jul 17, 2018


		import static org.elasticsearch.common.Strings.collectionToDelimitedString;

		public class DelegatedAuthorizationSupport {

Add authorizing_realms support to PKI realm #31643

Add authorizing_realms support to PKI realm #31643

Conversation

tvernum commented Jun 28, 2018

elasticmachine commented Jun 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot Jul 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bizybot left a comment

Choose a reason for hiding this comment

tvernum commented Jul 6, 2018

bizybot left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tvernum Jul 10, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaymode left a comment

Choose a reason for hiding this comment

bizybot Jul 5, 2018 •

edited

Loading

tvernum Jul 10, 2018 •

edited

Loading

tvernum Jul 10, 2018 •

edited

Loading