-
Notifications
You must be signed in to change notification settings - Fork 24.6k
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 the capability for the LDAP and AD realms to bind using Kerberos credentials #41126
Conversation
@elasticmachine run elasticsearch-ci/1 |
…ocally it is running fine
Pinging @elastic/es-security |
@elasticmachine run elasticsearch-ci/packaging-sample |
unrelated failure |
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.
This looks solid in the test department! Nice job 👍
I have left a few comments around simplifying realm configuration, that will have repercussions on BindRequestBuilder
as well.
I still have to grok the tests, but I will do that in a follow-up.
...ain/java/org/elasticsearch/xpack/core/security/authc/ldap/PoolingSessionFactorySettings.java
Show resolved
Hide resolved
default: | ||
throw new IllegalArgumentException("only [simple] and [sasl_gssapi] bind mode are allowed, [" + v + "] is invalid"); | ||
} | ||
}, Setting.Property.NodeScope)); |
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.
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.
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.
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.
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.
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.
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.
This is just an opinion, do as you think, or ask for other's opinions.
...ain/java/org/elasticsearch/xpack/core/security/authc/ldap/PoolingSessionFactorySettings.java
Outdated
Show resolved
Hide resolved
...ain/java/org/elasticsearch/xpack/core/security/authc/ldap/PoolingSessionFactorySettings.java
Outdated
Show resolved
Hide resolved
permission java.util.PropertyPermission "java.security.debug","write"; | ||
|
||
permission javax.security.auth.AuthPermission "createLoginContext.GSSAPIBindRequest"; | ||
permission javax.security.auth.AuthPermission "setLoginConfiguration"; |
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.
can't these be moved to x-pack/plugin/security/src/main/plugin-metadata/plugin-security.policy
?
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.
No, as unboundid dependencies are in the core
we need to have these here. Thank you.
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.
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?
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.
++ 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.
I believe enhancements shouldn't go in patch releases. |
And another suggestion: I would change the PR title. |
return bindRequest; | ||
} | ||
|
||
private Path validateGSSAPISettings(final byte[] bindPassword) { |
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.
Call me hypercritical, but this is woefully complex. And it all stems from that conditional setting I'm gripping about...
What I don't lilke:
This class can deduce the SASL principal, if it needs to, but it cannot deduce the bind DN. So, in case it needs the bind DN, it will use the provided function for that. But the caller needs to know when the bind DN is required, so that it can pass the function. This requires for the caller to know of the internals of the BindRequestBuilder
which is a violation of the abstraction.
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.
I suggest we create a separate BindRequestBuilder
for the SASL case only, and have the caller (or another wrapper) handle the decision of which BindRequest
type to build, given the realm settings.
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.
I had another pass and I'll admit I am opinionated on the setting topic and I would like to have someone else opinion on it. Besides that, I would also like to move the JVM permissions (and unboundid dependency) inside the security
plugin and away from the plugin-core
.
Thank you for the iterations Yogesh! |
@elasticmachine run elasticsearch-ci/1 |
This PR is stale and we are not planning to proceed with it at this time. |
This commit adds support for LDAP bind using SASL-GSSAPI as an
alternative to simple bind.
bind_mode
with valuesasl_gssapi
willenable LDAP bind using GSSAPI. Defaults to
simple
bind.Testing is done by enabling SASL on OpenLDAP and mapping a user
from Kerberos(mapped to bind DN) in a dockerized environment.
Verified the GSSAPI bind happens either using keytab or credentials.
Closes #39878