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.

Sign up for GitHub

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

Jump to bottom

Add the capability for the LDAP and AD realms to bind using Kerberos credentials #41126

Closed
wants to merge 23 commits into from
Closed

Add the capability for the LDAP and AD realms to bind using Kerberos credentials #41126

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
23 commits
Select commit Hold shift + click to select a range
cd80858
WIP: Add support for LDAP bind using SASL GSS-API
Mar 29, 2019
42ff127
remove image tag during build, not sure if this will fix the build, l…
Apr 12, 2019
230bc87
add healthcheck for kdc-kadmin
Apr 12, 2019
33dbb09
create temp file so it can be mounted
Apr 12, 2019
d3be59f
correct the file name
Apr 12, 2019
20cbecd
another file
Apr 12, 2019
e29cf26
things not working on CI, debug
Apr 12, 2019
7332792
remove unrelated changes
Apr 12, 2019
26ae7da
explicit volumes docker, see if it works
Apr 12, 2019
955469e
we are running as root inside container, give permissions
Apr 12, 2019
b2b4963
remove unused code
Apr 12, 2019
841f394
Merge branch 'master' into fix/39878
Apr 12, 2019
eaa5965
remove empty keytab
Apr 12, 2019
e6eda93
create file, check if non-file empty krb5.keytab exists as healthcheck
Apr 13, 2019
d9e5293
correct the method name
Apr 15, 2019
23bfb73
Address review comments
Apr 18, 2019
0ee24b1
removed use keytab setting
Apr 18, 2019
1119857
Increase interval time for kdc kadmin health check
Apr 18, 2019
986ff5e
Merge branch 'master' into fix/39878
Apr 18, 2019
7a5dc4c
add autoheal for openldap container
Apr 18, 2019
b6dba5d
add docs, also make kerberos principal name filtered
Apr 18, 2019
6fbf757
use filtered for setting instead of dynamic
Apr 30, 2019
460b8aa
Merge branch 'master' into fix/39878
May 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 23 additions & 1 deletion .../java/org/elasticsearch/xpack/core/security/authc/ldap/PoolingSessionFactorySettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@

import org.elasticsearch.common.settings.SecureString;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Setting.Property;
import org.elasticsearch.common.unit.TimeValue;
import org.elasticsearch.xpack.core.security.authc.RealmSettings;

Expand Down Expand Up @@ -35,6 +36,26 @@ public final class PoolingSessionFactorySettings {
key -> secureString(key, null)
);

public static final Function<String, Setting.AffixSetting<String>> BIND_MODE = RealmSettings.affixSetting("bind.mode",
key -> new Setting<>(key, "simple", Function.identity(), v -> {
switch (v) {
case "simple":
case "sasl_gssapi":
break;
default:
throw new IllegalArgumentException("only [simple] and [sasl_gssapi] bind mode are allowed, [" + v + "] is invalid");
}
}, Setting.Property.NodeScope));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the switch validation !
But, can't we infer the mode from if SASL_GSSAPI_PRINCIPAL is present. SASL_GSSAPI_PRINCIPAL cannot be null in the way we use the library, right? Also, using SASL_GSSAPI_PRINCIPAL together with a BIND_DN should trigger a settings validation error.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right now other than simple and sasl_gssapi we do not wish to support other bind types but there are other bind types via SASL that the LDAP has support for. So relying on the presence of SASL_GSSAPI_PRINCIPAL or SASL_GSSAPI_KEYTAB_PATH is something I would like to avoid.

Right now it gets ignored but I agree to throw settings validation error in presence of both BIND_DN and SASL_GSSAPI_PRINCIPAL settings. I will make these changes. Thank you.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't other bind methods not also have their setting keys namespaced similarly to sasl_gssapi ? So there would be no ambiguity? To me this is a case of namespaced settings versus conditional settings, as in #30241 . The conditional setting is harder to validate in the code, compared to the namespaced ones, because the validation of the namespaced ones is just to get the whole namespace and validate them as a group. It's also simpler from a user perspective, because he has fewer settings to worry about.

Copy link
Contributor

@albertzaharovits albertzaharovits Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just an opinion, do as you think, or ask for other's opinions.


public static final Function<String, Setting.AffixSetting<String>> SASL_GSSAPI_PRINCIPAL = RealmSettings
.affixSetting("sasl_gssapi.bind.principal", key -> Setting.simpleString(key, Setting.Property.NodeScope, Property.Filtered));
bizybot marked this conversation as resolved.
Show resolved Hide resolved
public static final Function<String, Setting.AffixSetting<Boolean>> SASL_GSSAPI_USE_KEYTAB = RealmSettings
albertzaharovits marked this conversation as resolved.
Show resolved Hide resolved
.affixSetting("sasl_gssapi.bind.use_keytab", key -> Setting.boolSetting(key, false, Setting.Property.NodeScope));
public static final Function<String, Setting.AffixSetting<String>> SASL_GSSAPI_KEYTAB_PATH = RealmSettings
.affixSetting("sasl_gssapi.bind.keytab.path", key -> Setting.simpleString(key, Setting.Property.NodeScope, Property.Filtered));
public static final Function<String, Setting.AffixSetting<Boolean>> SASL_GSSAPI_DEBUG = RealmSettings
bizybot marked this conversation as resolved.
Show resolved Hide resolved
.affixSetting("sasl_gssapi.bind.debug", key -> Setting.boolSetting(key, false, Setting.Property.NodeScope));

public static final int DEFAULT_CONNECTION_POOL_INITIAL_SIZE = 0;
public static final Function<String, Setting.AffixSetting<Integer>> POOL_INITIAL_SIZE = RealmSettings.affixSetting(
"user_search.pool.initial_size",
Expand Down Expand Up @@ -63,7 +84,8 @@ private PoolingSessionFactorySettings() {
public static Set<Setting.AffixSetting<?>> getSettings(String realmType) {
return Stream.of(
POOL_INITIAL_SIZE, POOL_SIZE, HEALTH_CHECK_ENABLED, HEALTH_CHECK_INTERVAL, HEALTH_CHECK_DN, BIND_DN,
LEGACY_BIND_PASSWORD, SECURE_BIND_PASSWORD
LEGACY_BIND_PASSWORD, SECURE_BIND_PASSWORD, BIND_MODE, SASL_GSSAPI_PRINCIPAL, SASL_GSSAPI_USE_KEYTAB, SASL_GSSAPI_KEYTAB_PATH,
SASL_GSSAPI_DEBUG
).map(f -> f.apply(realmType)).collect(Collectors.toSet());
}
}
19 changes: 18 additions & 1 deletion x-pack/plugin/core/src/main/plugin-metadata/plugin-security.policy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,24 @@ grant {
permission java.util.PropertyPermission "*", "read,write";

// needed for multiple server implementations used in tests
permission java.net.SocketPermission "*", "accept,connect";
permission java.net.SocketPermission "*", "accept,connect,resolve";

// needed for GSSAPI bind to LDAP
permission javax.security.auth.AuthPermission "modifyPrincipals";
permission javax.security.auth.AuthPermission "modifyPrivateCredentials";
permission javax.security.auth.PrivateCredentialPermission "javax.security.auth.kerberos.KerberosKey * \"*\"", "read";
permission javax.security.auth.PrivateCredentialPermission "javax.security.auth.kerberos.KeyTab * \"*\"", "read";
permission javax.security.auth.PrivateCredentialPermission "javax.security.auth.kerberos.KerberosTicket * \"*\"", "read";
permission javax.security.auth.AuthPermission "doAs";
permission javax.security.auth.kerberos.ServicePermission "*","initiate,accept";

permission java.util.PropertyPermission "javax.security.auth.useSubjectCredsOnly","write";
permission java.util.PropertyPermission "java.security.krb5.conf","write";
permission java.util.PropertyPermission "sun.security.krb5.debug","write";
permission java.util.PropertyPermission "java.security.debug","write";

permission javax.security.auth.AuthPermission "createLoginContext.GSSAPIBindRequest";
permission javax.security.auth.AuthPermission "setLoginConfiguration";
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can't these be moved to x-pack/plugin/security/src/main/plugin-metadata/plugin-security.policy ?

Copy link
Contributor Author

@bizybot bizybot Apr 18, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, as unboundid dependencies are in the core we need to have these here. Thank you.

Copy link
Contributor

@albertzaharovits albertzaharovits Apr 24, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these are a lot of obscure permissions we're granting for any plugin (that inherits x-pack-plugin-core). I worry especially about the maintainability. In this case I think we should move the unboundid dependency over to the security plugin. I think it should not have been a dependecy in plugin-core in the first place. The only imported class from that dependecy in plugin-core is an enum, LdapSearchScope, which I think we should emulate our own and use that in plugin-core and adapt the APIs.

What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

++ on moving the dependencies and getting the permissions in one place, though I see a TODO which I think is a non-trivial change and would require considerable thought as I am not much aware of what needs to be done here.
https://github.com/elastic/elasticsearch/blob/73bfdc4066be080dc4cad1f0521bf6ea14cded93/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/XPackPlugin.java#L95..L124

I think I will pick this up later as a cleanup task. Thank you.

};

grant codeBase "${codebase.netty-common}" {
Expand Down
69 changes: 38 additions & 31 deletions .../main/java/org/elasticsearch/xpack/security/authc/ldap/ActiveDirectorySessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
*/
package org.elasticsearch.xpack.security.authc.ldap;

import com.unboundid.ldap.sdk.BindRequest;
import com.unboundid.ldap.sdk.Filter;
import com.unboundid.ldap.sdk.LDAPConnection;
import com.unboundid.ldap.sdk.LDAPConnectionPool;
Expand All @@ -14,10 +15,12 @@
import com.unboundid.ldap.sdk.ServerSet;
import com.unboundid.ldap.sdk.SimpleBindRequest;
import com.unboundid.ldap.sdk.controls.AuthorizationIdentityRequestControl;

import org.apache.logging.log4j.Logger;
import org.elasticsearch.ElasticsearchSecurityException;
import org.elasticsearch.action.ActionListener;
import org.elasticsearch.action.ActionRunnable;
import org.elasticsearch.common.CharArrays;
import org.elasticsearch.common.cache.Cache;
import org.elasticsearch.common.cache.CacheBuilder;
import org.elasticsearch.common.logging.DeprecationLogger;
Expand All @@ -32,8 +35,8 @@
import org.elasticsearch.xpack.core.security.authc.ldap.ActiveDirectorySessionFactorySettings;
import org.elasticsearch.xpack.core.security.authc.ldap.PoolingSessionFactorySettings;
import org.elasticsearch.xpack.core.security.authc.ldap.support.LdapSearchScope;
import org.elasticsearch.common.CharArrays;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.security.authc.ldap.bind.BindRequestBuilder;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapMetaDataResolver;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapSession;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapSession.GroupsResolver;
Expand Down Expand Up @@ -66,7 +69,7 @@ class ActiveDirectorySessionFactory extends PoolingSessionFactory {
ActiveDirectorySessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException {
super(config, sslService, new ActiveDirectoryGroupsResolver(config),
ActiveDirectorySessionFactorySettings.POOL_ENABLED,
config.hasSetting(PoolingSessionFactorySettings.BIND_DN) ? getBindDN(config) : null,
new BindRequestBuilder(config, c -> c.hasSetting(PoolingSessionFactorySettings.BIND_DN) ? getBindDN(config) : null).build(),
() -> {
if (config.hasSetting(PoolingSessionFactorySettings.BIND_DN)) {
final String healthCheckDn = config.getSetting(PoolingSessionFactorySettings.BIND_DN);
Expand Down Expand Up @@ -149,7 +152,7 @@ void getUnauthenticatedSessionWithoutPool(String user, ActionListener<LdapSessio
}
try {
final LDAPConnection connection = LdapUtils.privilegedConnect(serverSet::getConnection);
LdapUtils.maybeForkThenBind(connection, bindCredentials, threadPool, new AbstractRunnable() {
LdapUtils.maybeForkThenBind(connection, bindRequestCredentials, threadPool, new AbstractRunnable() {

@Override
public void onFailure(Exception e) {
Expand Down Expand Up @@ -190,12 +193,17 @@ static String buildDnFromDomain(String domain) {

static String getBindDN(RealmConfig config) {
String bindDN = config.getSetting(PoolingSessionFactorySettings.BIND_DN);
if (bindDN.isEmpty() == false && bindDN.indexOf('\\') < 0 && bindDN.indexOf('@') < 0 && bindDN.indexOf('=') < 0) {
if (bindDN != null && bindDN.isEmpty() == false && bindDN.indexOf('\\') < 0 && bindDN.indexOf('@') < 0 && bindDN.indexOf('=') < 0) {
bindDN = bindDN + "@" + config.getSetting(ActiveDirectorySessionFactorySettings.AD_DOMAIN_NAME_SETTING);
}
return bindDN;
}

static boolean isSimpleBind(RealmConfig config) {
String mode = config.getSetting(PoolingSessionFactorySettings.BIND_MODE);
return "simple".equals(mode);
}

// Exposed for testing
ServerSet getServerSet() {
return super.serverSet;
Expand All @@ -221,8 +229,7 @@ abstract static class ADAuthenticator {
final String userSearchDN;
final LdapSearchScope userSearchScope;
final String userSearchFilter;
final String bindDN;
final SecureString bindPassword;
final BindRequestBuilder bindRequestBuilder;
final ThreadPool threadPool;

ADAuthenticator(RealmConfig realm, TimeValue timeout, boolean ignoreReferralErrors, Logger logger, GroupsResolver groupsResolver,
Expand All @@ -234,9 +241,7 @@ abstract static class ADAuthenticator {
this.logger = logger;
this.groupsResolver = groupsResolver;
this.metaDataResolver = metaDataResolver;
this.bindDN = getBindDN(realm);
this.bindPassword = realm.getSetting(PoolingSessionFactorySettings.SECURE_BIND_PASSWORD,
() -> realm.getSetting(PoolingSessionFactorySettings.LEGACY_BIND_PASSWORD));
this.bindRequestBuilder = new BindRequestBuilder(realm, ActiveDirectorySessionFactory::getBindDN);
this.threadPool = threadPool;
userSearchDN = realm.getSetting(ActiveDirectorySessionFactorySettings.AD_USER_SEARCH_BASEDN_SETTING, () -> domainDN);
userSearchScope = LdapSearchScope.resolve(realm.getSetting(ActiveDirectorySessionFactorySettings.AD_USER_SEARCH_SCOPE_SETTING),
Expand Down Expand Up @@ -268,11 +273,11 @@ protected void doRun() throws Exception {
}));
}
};
if (bindDN.isEmpty()) {
String bindDN = getBindDN(realm);
if (isSimpleBind(realm) && (bindDN == null || bindDN.isEmpty())) {
searchRunnable.run();
} else {
final SimpleBindRequest bind = new SimpleBindRequest(bindDN, CharArrays.toUtf8Bytes(bindPassword.getChars()));
LdapUtils.maybeForkThenBind(connection, bind, threadPool, searchRunnable);
LdapUtils.maybeForkThenBind(connection, bindRequestBuilder.build(), threadPool, searchRunnable);
}
}
});
Expand Down Expand Up @@ -434,31 +439,33 @@ void netBiosDomainNameToDn(LDAPInterface ldapInterface, String netBiosDomainName
finalLdapConnection.getConnectedAddress(),
finalLdapConnection.getSSLSession() != null ? ldapsPort : ldapPort));
final byte[] passwordBytes = CharArrays.toUtf8Bytes(password.getChars());
final SimpleBindRequest bind = bindDN.isEmpty()
? new SimpleBindRequest(username, passwordBytes)
: new SimpleBindRequest(bindDN, CharArrays.toUtf8Bytes(bindPassword.getChars()));
LdapUtils.maybeForkThenBind(searchConnection, bind, threadPool, new ActionRunnable<String>(listener) {
@Override
protected void doRun() throws Exception {
search(searchConnection, "CN=Configuration," + domainDN, LdapSearchScope.SUB_TREE.scope(), filter,
timeLimitSeconds, ignoreReferralErrors,
ActionListener.wrap(
results -> {
final BindRequest bind;
String bindDN = getBindDN(config);
if (isSimpleBind(config) && (bindDN == null || bindDN.isEmpty())) {
bind = new SimpleBindRequest(username, passwordBytes);
} else {
bind = bindRequestBuilder.build();
}
LdapUtils.maybeForkThenBind(searchConnection, bind, threadPool,
new ActionRunnable<String>(listener) {
@Override
protected void doRun() throws Exception {
search(searchConnection, "CN=Configuration," + domainDN, LdapSearchScope.SUB_TREE.scope(), filter,
timeLimitSeconds, ignoreReferralErrors, ActionListener.wrap(results -> {
IOUtils.close(searchConnection);
handleSearchResults(results, netBiosDomainName, domainNameCache, listener);
}, e -> {
IOUtils.closeWhileHandlingException(searchConnection);
listener.onFailure(e);
}),
"ncname");
}
}), "ncname");
}

@Override
public void onFailure(Exception e) {
IOUtils.closeWhileHandlingException(searchConnection);
listener.onFailure(e);
}
});
@Override
public void onFailure(Exception e) {
IOUtils.closeWhileHandlingException(searchConnection);
listener.onFailure(e);
}
});
}
} catch (LDAPException e) {
listener.onFailure(e);
Expand Down
9 changes: 5 additions & 4 deletions ...c/main/java/org/elasticsearch/xpack/security/authc/ldap/LdapUserSearchSessionFactory.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
import org.elasticsearch.xpack.core.security.authc.ldap.support.LdapSearchScope;
import org.elasticsearch.common.CharArrays;
import org.elasticsearch.xpack.core.ssl.SSLService;
import org.elasticsearch.xpack.security.authc.ldap.bind.BindRequestBuilder;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapSession;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapSession.GroupsResolver;
import org.elasticsearch.xpack.security.authc.ldap.support.LdapUtils;
Expand All @@ -46,7 +47,7 @@ class LdapUserSearchSessionFactory extends PoolingSessionFactory {

LdapUserSearchSessionFactory(RealmConfig config, SSLService sslService, ThreadPool threadPool) throws LDAPException {
super(config, sslService, groupResolver(config), LdapUserSearchSessionFactorySettings.POOL_ENABLED,
config.getSetting(BIND_DN, () -> null),
new BindRequestBuilder(config, c -> c.getSetting(BIND_DN, () -> null)).build(),
() -> config.getSetting(BIND_DN, () -> config.getSetting(LdapUserSearchSessionFactorySettings.SEARCH_BASE_DN)),
threadPool);
userSearchBaseDn = config.getSetting(LdapUserSearchSessionFactorySettings.SEARCH_BASE_DN,
Expand Down Expand Up @@ -110,7 +111,7 @@ protected void doRun() throws Exception {
void getSessionWithoutPool(String user, SecureString password, ActionListener<LdapSession> listener) {
try {
final LDAPConnection connection = LdapUtils.privilegedConnect(serverSet::getConnection);
LdapUtils.maybeForkThenBind(connection, bindCredentials, threadPool, new AbstractRunnable() {
LdapUtils.maybeForkThenBind(connection, bindRequestCredentials, threadPool, new AbstractRunnable() {
@Override
protected void doRun() throws Exception {
findUser(user, connection, ActionListener.wrap((entry) -> {
Expand All @@ -124,7 +125,7 @@ protected void doRun() throws Exception {
LdapUtils.maybeForkThenBind(connection, userBind, threadPool, new AbstractRunnable() {
@Override
protected void doRun() throws Exception {
LdapUtils.maybeForkThenBind(connection, bindCredentials, threadPool, new AbstractRunnable() {
LdapUtils.maybeForkThenBind(connection, bindRequestCredentials, threadPool, new AbstractRunnable() {

@Override
protected void doRun() throws Exception {
Expand Down Expand Up @@ -187,7 +188,7 @@ void getUnauthenticatedSessionWithPool(LDAPConnectionPool connectionPool, String
void getUnauthenticatedSessionWithoutPool(String user, ActionListener<LdapSession> listener) {
try {
final LDAPConnection connection = LdapUtils.privilegedConnect(serverSet::getConnection);
LdapUtils.maybeForkThenBind(connection, bindCredentials, threadPool, new AbstractRunnable() {
LdapUtils.maybeForkThenBind(connection, bindRequestCredentials, threadPool, new AbstractRunnable() {
@Override
protected void doRun() throws Exception {
findUser(user, connection, ActionListener.wrap((entry) -> {
Expand Down
Loading