-
Notifications
You must be signed in to change notification settings - Fork 24.9k
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
Add authorizing_realms support to PKI realm #31643
Changes from 7 commits
aecdc22
9d4032f
a0e7222
6a80c4d
aefab91
3be69e4
32f5e8e
4fd642e
f7e22d9
efbda3d
e4c9d57
94500a6
6eec3f3
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,24 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.core.security.authc.support; | ||
|
||
import org.elasticsearch.common.settings.Setting; | ||
|
||
import java.util.Collection; | ||
import java.util.Collections; | ||
import java.util.List; | ||
import java.util.function.Function; | ||
|
||
public class DelegatedAuthorizationSettings { | ||
|
||
public static final Setting<List<String>> AUTHZ_REALMS = Setting.listSetting("authorizing_realms", | ||
Collections.emptyList(), Function.identity(), Setting.Property.NodeScope); | ||
|
||
public static Collection<Setting<?>> getSettings() { | ||
return Collections.singleton(AUTHZ_REALMS); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -31,12 +31,12 @@ | |
import org.elasticsearch.xpack.core.ssl.SSLConfigurationSettings; | ||
import org.elasticsearch.xpack.security.authc.BytesKey; | ||
import org.elasticsearch.xpack.security.authc.support.CachingRealm; | ||
import org.elasticsearch.xpack.security.authc.support.DelegatedAuthorizationSupport; | ||
import org.elasticsearch.xpack.security.authc.support.UserRoleMapper; | ||
import org.elasticsearch.xpack.security.authc.support.mapper.CompositeRoleMapper; | ||
import org.elasticsearch.xpack.security.authc.support.mapper.NativeRoleMappingStore; | ||
|
||
import javax.net.ssl.X509TrustManager; | ||
|
||
import java.security.MessageDigest; | ||
import java.security.cert.Certificate; | ||
import java.security.cert.CertificateEncodingException; | ||
|
@@ -75,6 +75,7 @@ public class PkiRealm extends Realm implements CachingRealm { | |
private final Pattern principalPattern; | ||
private final UserRoleMapper roleMapper; | ||
private final Cache<BytesKey, User> cache; | ||
private DelegatedAuthorizationSupport delegatedRealms; | ||
|
||
public PkiRealm(RealmConfig config, ResourceWatcherService watcherService, NativeRoleMappingStore nativeRoleMappingStore) { | ||
this(config, new CompositeRoleMapper(PkiRealmSettings.TYPE, config, watcherService, nativeRoleMappingStore)); | ||
|
@@ -93,6 +94,14 @@ public PkiRealm(RealmConfig config, ResourceWatcherService watcherService, Nativ | |
.build(); | ||
} | ||
|
||
@Override | ||
public void initialize(Iterable<Realm> realms) { | ||
if(delegatedRealms != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: space between There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Grr, I changed my pre-commit hook and broke this check. |
||
throw new IllegalStateException("Realm has already been initialized"); | ||
} | ||
delegatedRealms = new DelegatedAuthorizationSupport(realms, config); | ||
} | ||
|
||
@Override | ||
public boolean supports(AuthenticationToken token) { | ||
return token instanceof X509AuthenticationToken; | ||
|
@@ -105,32 +114,50 @@ public X509AuthenticationToken token(ThreadContext context) { | |
|
||
@Override | ||
public void authenticate(AuthenticationToken authToken, ActionListener<AuthenticationResult> listener) { | ||
assert delegatedRealms != null : "Realm has not been initialized correctly"; | ||
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()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if the user is in the cache, why are we resolving anything? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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:
The PKI realm doesn't clear its own cache for those events, but the LDAP realm does. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is the question that @bizybot raised about whether we even care about the PKI cache in this case. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. |
||
delegatedRealms.resolve(token.principal(), listener); | ||
} else { | ||
listener.onResponse(AuthenticationResult.success(user)); | ||
} | ||
} else if (isCertificateChainTrusted(trustManager, token, logger) == false) { | ||
listener.onResponse(AuthenticationResult.unsuccessful("Certificate for " + token.dn() + " is not trusted", null)); | ||
} else { | ||
final Map<String, Object> metadata = Collections.singletonMap("pki_dn", token.dn()); | ||
final UserRoleMapper.UserData userData = new UserRoleMapper.UserData(token.principal(), | ||
token.dn(), Collections.emptySet(), metadata, this.config); | ||
roleMapper.resolveRoles(userData, ActionListener.wrap(roles -> { | ||
final User computedUser = | ||
new User(token.principal(), roles.toArray(new String[roles.size()]), null, null, metadata, true); | ||
try (ReleasableLock ignored = readLock.acquire()) { | ||
cache.put(fingerprint, computedUser); | ||
final ActionListener<AuthenticationResult> cachingListener = ActionListener.wrap(result -> { | ||
if (result.isAuthenticated()) { | ||
try (ReleasableLock ignored = readLock.acquire()) { | ||
cache.put(fingerprint, result.getUser()); | ||
} | ||
} | ||
listener.onResponse(AuthenticationResult.success(computedUser)); | ||
}, listener::onFailure)); | ||
listener.onResponse(result); | ||
}, listener::onFailure); | ||
if (delegatedRealms.hasDelegation()) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ++ 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. |
||
delegatedRealms.resolve(token.principal(), cachingListener); | ||
} else { | ||
this.buildUser(token, cachingListener); | ||
} | ||
} | ||
} catch (CertificateEncodingException e) { | ||
listener.onResponse(AuthenticationResult.unsuccessful("Certificate for " + token.dn() + " has encoding issues", e)); | ||
} | ||
} | ||
|
||
private void buildUser(X509AuthenticationToken token, ActionListener<AuthenticationResult> listener) { | ||
final Map<String, Object> metadata = Collections.singletonMap("pki_dn", token.dn()); | ||
final UserRoleMapper.UserData userData = new UserRoleMapper.UserData(token.principal(), | ||
token.dn(), Collections.emptySet(), metadata, this.config); | ||
roleMapper.resolveRoles(userData, ActionListener.wrap(roles -> { | ||
final User computedUser = | ||
new User(token.principal(), roles.toArray(new String[roles.size()]), null, null, metadata, true); | ||
listener.onResponse(AuthenticationResult.success(computedUser)); | ||
}, listener::onFailure)); | ||
} | ||
|
||
@Override | ||
public void lookupUser(String username, ActionListener<User> listener) { | ||
listener.onResponse(null); | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,80 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.security.authc.support; | ||
|
||
import org.apache.logging.log4j.Logger; | ||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.common.collect.Tuple; | ||
import org.elasticsearch.common.logging.Loggers; | ||
import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
import org.elasticsearch.xpack.core.security.authc.AuthenticationResult; | ||
import org.elasticsearch.xpack.core.security.authc.Realm; | ||
import org.elasticsearch.xpack.core.security.authc.RealmConfig; | ||
import org.elasticsearch.xpack.core.security.authc.support.DelegatedAuthorizationSettings; | ||
import org.elasticsearch.xpack.core.security.user.User; | ||
|
||
import java.util.ArrayList; | ||
import java.util.List; | ||
|
||
import static org.elasticsearch.common.Strings.collectionToDelimitedString; | ||
|
||
public class DelegatedAuthorizationSupport { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. please add javadocs to the class and methods There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Another precommit fail 😞 |
||
|
||
private final RealmUserLookup lookup; | ||
private final Logger logger; | ||
|
||
public DelegatedAuthorizationSupport(Iterable<? extends Realm> allRealms, RealmConfig config) { | ||
this(allRealms, DelegatedAuthorizationSettings.AUTHZ_REALMS.get(config.settings()), config.threadContext()); | ||
} | ||
|
||
protected DelegatedAuthorizationSupport(Iterable<? extends Realm> allRealms, List<String> lookupRealms, ThreadContext threadContext) { | ||
this.lookup = new RealmUserLookup(resolveRealms(allRealms, lookupRealms), threadContext); | ||
this.logger = Loggers.getLogger(getClass()); | ||
} | ||
|
||
public boolean hasDelegation() { | ||
return this.lookup.hasRealms(); | ||
} | ||
|
||
public void resolve(String username, ActionListener<AuthenticationResult> resultListener) { | ||
if (hasDelegation() == false) { | ||
resultListener.onResponse(AuthenticationResult.unsuccessful( | ||
"No [" + DelegatedAuthorizationSettings.AUTHZ_REALMS.getKey() + "] have been configured", null)); | ||
return; | ||
} | ||
ActionListener<Tuple<User, Realm>> userListener = ActionListener.wrap(tuple -> { | ||
if (tuple != null) { | ||
logger.trace("Found user " + tuple.v1() + " in realm " + tuple.v2()); | ||
resultListener.onResponse(AuthenticationResult.success(tuple.v1())); | ||
} else { | ||
resultListener.onResponse(AuthenticationResult.unsuccessful("the principal [" + username | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. But then every caller would need to reproduce the same error handling. What gain do you see? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed in above comment. |
||
+ "] was authenticated, but no user could be found in realms [" + collectionToDelimitedString(lookup.getRealms(), ",") | ||
+ "]", null)); | ||
} | ||
}, resultListener::onFailure); | ||
lookup.lookup(username, userListener); | ||
} | ||
|
||
private List<Realm> resolveRealms(Iterable<? extends Realm> allRealms, List<String> lookupRealms) { | ||
final List<Realm> result = new ArrayList<>(lookupRealms.size()); | ||
for (String name : lookupRealms) { | ||
result.add(findRealm(name, allRealms)); | ||
} | ||
assert result.size() == lookupRealms.size(); | ||
return result; | ||
} | ||
|
||
private Realm findRealm(String name, Iterable<? extends Realm> allRealms) { | ||
for (Realm realm : allRealms) { | ||
if (name.equals(realm.name())) { | ||
return realm; | ||
} | ||
} | ||
throw new IllegalStateException("configured authorizing realm [" + name + "] does not exist (or is not enabled)"); | ||
} | ||
|
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,63 @@ | ||
/* | ||
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one | ||
* or more contributor license agreements. Licensed under the Elastic License; | ||
* you may not use this file except in compliance with the Elastic License. | ||
*/ | ||
|
||
package org.elasticsearch.xpack.security.authc.support; | ||
|
||
import org.elasticsearch.action.ActionListener; | ||
import org.elasticsearch.common.collect.Tuple; | ||
import org.elasticsearch.common.util.concurrent.ThreadContext; | ||
import org.elasticsearch.xpack.core.common.IteratingActionListener; | ||
import org.elasticsearch.xpack.core.security.authc.Realm; | ||
import org.elasticsearch.xpack.core.security.user.User; | ||
|
||
import java.util.Collections; | ||
import java.util.List; | ||
|
||
public class RealmUserLookup { | ||
|
||
private final List<? extends Realm> realms; | ||
private final ThreadContext threadContext; | ||
|
||
public RealmUserLookup(List<? extends Realm> realms, ThreadContext threadContext) { | ||
this.realms = realms; | ||
this.threadContext = threadContext; | ||
} | ||
|
||
public List<Realm> getRealms() { | ||
return Collections.unmodifiableList(realms); | ||
} | ||
|
||
public boolean hasRealms() { | ||
return realms.isEmpty() == false; | ||
} | ||
|
||
/** | ||
* Lookup the {@code principal} in the list of {@link #realms}. | ||
* The realms are consulted in order. When one realm responds with a non-null {@link User}, this | ||
* is returned with the matching realm, through the {@code listener}. | ||
* If no user if found (including the case where the {@link #realms} list is empty), then | ||
* {@link ActionListener#onResponse(Object)} is called with a {@code null} {@link Tuple}. | ||
*/ | ||
public void lookup(String principal, ActionListener<Tuple<User, Realm>> listener) { | ||
final IteratingActionListener<Tuple<User, Realm>, ? extends Realm> userLookupListener = | ||
new IteratingActionListener<>(listener, | ||
(realm, lookupUserListener) -> realm.lookupUser(principal, | ||
ActionListener.wrap(foundUser -> { | ||
if (foundUser != null) { | ||
lookupUserListener.onResponse(new Tuple<>(foundUser, realm)); | ||
} else { | ||
lookupUserListener.onResponse(null); | ||
} | ||
}, | ||
lookupUserListener::onFailure)), | ||
realms, threadContext); | ||
try { | ||
userLookupListener.run(); | ||
} catch (Exception e) { | ||
listener.onFailure(e); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe we should just use a
SetOnce
which would enforce the only initialized onceThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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