[SSSD] [PATCH] LDAP: Detect the presence of POSIX attributes
Jakub Hrozek
jhrozek at redhat.com
Wed Feb 12 14:31:42 UTC 2014
On Wed, Feb 12, 2014 at 11:30:24AM +0100, Pavel Březina wrote:
> On 02/11/2014 09:33 PM, Jakub Hrozek wrote:
> >On Thu, Jan 30, 2014 at 11:17:06AM +0100, Sumit Bose wrote:
> >>On Wed, Jan 29, 2014 at 05:11:59PM +0100, Jakub Hrozek wrote:
> >>>On Wed, Jan 29, 2014 at 03:39:41PM +0100, Pavel Březina wrote:
> >>>>On 01/27/2014 11:33 PM, Jakub Hrozek wrote:
> >>>>>Hi,
> >>>>>
> >>>>>When the schema is set to AD and ID mapping is used, there is a one-time
> >>>>
> >>>>you wanted to say "when ID mapping is *not* used"
> >>>>
> >>>>>check ran when searching for users to detect the presence of POSIX attributes
> >>>>>in LDAP. If this check fails, the search fails as if no entry was found
> >>>>>and returns a special error code.
> >>>>>
> >>>>>If the AD identity lookup finds this error code, the GC is disabled for
> >>>>>the next search.
> >>>>>
> >>>>>The sdap_server_opts structure is filled every time a client connects to
> >>>>>a server so the posix check boolean is reset to false again on connecting
> >>>>>to the server.
> >>>>
> >>>>GC lookups should not be disabled permanently. Administrator can
> >>>>setup POSIX attribute propagation to GC any time and SSSD should
> >>>>notice it
> >>>>without restarting.
> >>>>
> >>>>So we should rather perform the check periodically and not only
> >>>>disable GC lookups, but also re-enable it if it succeeds.
> >>>
> >>>See the attached patch, I added a timeout of 3600 seconds for the next
> >>>detection. I don't think we want to have the timeout configurable but
> >>>maybe we want the timeout to be reset on changing the online status as
> >>>well?
> >>
> >>I think we do not have to reset it when changing the online status since
> >>having the POSIX IDs in the GC or not is a global feature of the forest
> >>and changes should be quite rare.
> >
> >After some discussion last week with Simo I decided to remove the timer
> >altogether. The decision to change AD schema is so rare it makes sense
> >to let admins restart a service on clients.
> >
> >>
> >>>
> >>>>
> >>>>>It might be better to move the check to where the rootDSE is retrieved,
> >>>>>but the check depends on several features that are not known to the code
> >>>>>that retrieves the rootDSE (or the connection code for example) such as what
> >>>>>the attribute mappings are or the authentication method that should be used.
> >>>>>
> >>>>>Currently this patch only runs the check when users are requested. It
> >>>>>would be trivial to add the same code (about 70 lines) to the group
> >>>>>request as well.
> >>
> >>I think it is not called when enumeration is the first task running. Can
> >>you add it here as well? And while you are on it it might be worth to
> >>add it to group lookups as well, just for completeness.
> >
> >Done. During the testing of the AD enumeration I found some other issues
> >that are on the list right now as separate patches.
> >
> >>
> >>Please find more comments inline.
> >>
> >>bye,
> >>Sumit
> >>
> >>>>>
> >>>>>Additionally, I wonder if the absence of POSIX attributes in GC should
> >>>>>be reported louder. Currently there is just MINOR_FAILURE.
> >>>>
> >>>>We have SSSDBG_IMPORTANT_INFO for such things.
> >>>
> >>>Done.
> >>>
> >>>>
> >>>>>We could go as far as report to syslog when a user or a group from
> >>>>>subdomains is requested and the GC was already disabled, but I wanted to
> >>>>>check with the other developers before implementing this.
> >>>>
> >>>>I haven't made up my mind on this question yet, but I'm more
> >>>>inclined to avoid using syslog for this situation. :-)
> >>>
> >>>Yes, me too.
> >>>
> >>>>
> >>>>>One additional question -- currently the AD ID provider always retries
> >>>>>in LDAP if search in GC didn't find the result. With the possibility of
> >>>>>detecting the POSIX attributes, do we want to remove this fallback if we
> >>>>>were able to run the detection and know that GC is available but doesn't
> >>>>>contain the POSIX attributes?
> >>>>>
> >>>>>I would say the fallback should stay, because there is still a chance
> >>>>>other required attribute will be missing, after all the user can
> >>>>>configure additional filter and this fallback only affects negative
> >>>>>searches.
> >>>>
> >>>>I'm not sure if I understand you. Isn't the whole point of this
> >>>>patch to actually avoid the fallback? I mean if we detect that GC
> >>>>doesn't have POSIX attributes we want to go directly to LDAP on next
> >>>>queries, don't we?
> >>>
> >>>I meant the other way. Currently even if we know that GC *has* POSIX
> >>>attributes and we don't find the user in GC, we still retry LDAP. I was
> >>>wondering whether we can remove this fallback if we know that POSIX
> >>>attributes are present. But I think we shouldn't remove the fallback for
> >>>the general case because admins can still configure a custom filter in
> >>>the search base and this custom filter can contain any attributes.
> >>
> >>> From c76a1058f1aea30b83f3dd133230a093b1de5b44 Mon Sep 17 00:00:00 2001
> >>>From: Jakub Hrozek <jhrozek at redhat.com>
> >>>Date: Mon, 16 Dec 2013 18:36:12 +0100
> >>>Subject: [PATCH] LDAP: Detect the presence of POSIX attributes
> >>>
> >>>When the schema is set to AD and ID mapping is used, there is a one-time
> >>
> >> ^ 'not' missing ?
> >
> >Right, fixed
> >
> >>
> >>>check ran when searching for users to detect the presence of POSIX
> >>>attributes in LDAP. If this check fails, the search fails as if no entry
> >>>was found and returns a special error code.
> >>>
> >>>The sdap_server_opts structure is filled every time a client connects to
> >>>a server so the posix check boolean is reset to false again on connecting
> >>>to the server.
> >>>
> >>>It might be better to move the check to where the rootDSE is retrieved,
> >>>but the check depends on several features that are not known to the code
> >>>that retrieves the rootDSE (or the connection code for example) such as what
> >>>the attribute mappings are or the authentication method that should be used.
> >>>---
> >>> src/providers/ad/ad_common.h | 3 +
> >>> src/providers/ad/ad_id.c | 45 ++++++++-
> >>> src/providers/ad/ad_id.h | 1 +
> >>> src/providers/ipa/ipa_subdomains_id.c | 2 +-
> >>> src/providers/ldap/ldap_id.c | 79 ++++++++++++++-
> >>> src/providers/ldap/sdap.h | 1 +
> >>> src/providers/ldap/sdap_async.c | 185 ++++++++++++++++++++++++++++++++++
> >>> src/providers/ldap/sdap_async.h | 9 ++
> >>> src/util/util_errors.c | 1 +
> >>> src/util/util_errors.h | 1 +
> >>> 10 files changed, 320 insertions(+), 7 deletions(-)
> >>>
> >>>diff --git a/src/providers/ad/ad_common.h b/src/providers/ad/ad_common.h
> >>>index d370cef69124c127f41d7c4cbaa25713363e7752..d70d4656c1d1878a882f261e481a2507f8e58c55 100644
> >>>--- a/src/providers/ad/ad_common.h
> >>>+++ b/src/providers/ad/ad_common.h
> >>>@@ -69,6 +69,9 @@ struct ad_options {
> >>> struct sdap_options *id;
> >>> struct ad_id_ctx *id_ctx;
> >>>
> >>>+ /* POSIX detection */
> >>>+ time_t gc_check_timer;
> >>>+
> >>> /* Auth and chpass Provider */
> >>> struct krb5_ctx *auth_ctx;
> >>>
> >>>diff --git a/src/providers/ad/ad_id.c b/src/providers/ad/ad_id.c
> >>>index db921b11850c16cc7e818cad481705acbb4b9127..164855fb13a9ae88fcf244529b357824762178a8 100644
> >>>--- a/src/providers/ad/ad_id.c
> >>>+++ b/src/providers/ad/ad_id.c
> >>>@@ -27,6 +27,8 @@
> >>> #include "providers/ldap/sdap_async_enum.h"
> >>> #include "providers/ldap/sdap_idmap.h"
> >>>
> >>>+#define GC_POSIX_CHECK_TIMEOUT 3600
> >>>+
> >>> struct ad_handle_acct_info_state {
> >>> struct be_req *breq;
> >>> struct be_acct_req *ar;
> >>>@@ -34,6 +36,7 @@ struct ad_handle_acct_info_state {
> >>> struct sdap_id_conn_ctx **conn;
> >>> struct sdap_domain *sdom;
> >>> size_t cindex;
> >>>+ struct ad_options *ad_options;
> >>>
> >>> int dp_error;
> >>> const char *err;
> >>>@@ -47,6 +50,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>> struct be_req *breq,
> >>> struct be_acct_req *ar,
> >>> struct sdap_id_ctx *ctx,
> >>>+ struct ad_options *ad_options,
> >>> struct sdap_domain *sdom,
> >>> struct sdap_id_conn_ctx **conn)
> >>> {
> >>>@@ -64,6 +68,7 @@ ad_handle_acct_info_send(TALLOC_CTX *mem_ctx,
> >>> state->ctx = ctx;
> >>> state->sdom = sdom;
> >>> state->conn = conn;
> >>>+ state->ad_options = ad_options;
> >>> state->cindex = 0;
> >>>
> >>> ret = ad_handle_acct_info_step(req);
> >>>@@ -137,12 +142,29 @@ ad_handle_acct_info_done(struct tevent_req *subreq)
> >>> if (sdap_err == EOK) {
> >>> tevent_req_done(req);
> >>> return;
> >>>+ } else if (sdap_err == ERR_NO_POSIX) {
> >>>+ if (dp_opt_get_bool(state->ad_options->basic, AD_ENABLE_GC)) {
> >>>+ DEBUG(SSSDBG_IMPORTANT_INFO, ("POSIX attributes were requested "
> >>>+ "but are not present on the server side. Global Catalog "
> >>>+ "lookups will be disabled for %d seconds\n",
> >>>+ GC_POSIX_CHECK_TIMEOUT));
> >>>+
> >>>+ state->ad_options->gc_check_timer = time(NULL);
> >>
> >>I would like to suggest rename gc_check_timer to gc_next_check and set
> >>it to time(NULL)+GC_POSIX_CHECK_TIMEOUT here. This will save you an add
> >>operation later which might be called more often (ok, compared to
> >>calling time() the cycles spend for an add are negligible, but still the
> >>check might be easier to read).
> >
> >The timer is gone now :-)
> >
> >>
> >>>+
> >>>+ ret = dp_opt_set_bool(state->ad_options->basic,
> >>>+ AD_ENABLE_GC, false);
> >>>+ if (ret != EOK) {
> >>>+ DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+ ("Could not turn off GC support\n"));
> >>>+ /* Not fatal */
> >>>+ }
> >>>+ }
> >>> } else if (sdap_err != ENOENT) {
> >>> tevent_req_error(req, EIO);
> >>> return;
> >>> }
> >>>
> >>>- /* Ret is only ENOENT now. Try the next connection */
> >>>+ /* Ret is only ENOENT or ERR_NO_POSIX now. Try the next connection */
> >>> state->cindex++;
> >>> ret = ad_handle_acct_info_step(req);
> >>> if (ret != EAGAIN) {
> >>>@@ -188,6 +210,25 @@ get_conn_list(struct be_req *breq, struct ad_id_ctx *ad_ctx,
> >>> struct sss_domain_info *dom, struct be_acct_req *ar)
> >>> {
> >>> struct sdap_id_conn_ctx **clist;
> >>>+ time_t now;
> >>>+ errno_t ret;
> >>>+
> >>>+ /* If the last POSIX check was too far in the past, we might need to
> >>>+ * re-check again
> >>>+ */
> >>>+ now = time(NULL);
> >>>+ if (ad_ctx->ad_options->gc_check_timer &&
> >>>+ ad_ctx->ad_options->gc_check_timer + GC_POSIX_CHECK_TIMEOUT < now) {
> >>
> >>see above
> >>
> >>>+ ad_ctx->sdap_id_ctx->srv_opts->posix_checked = false;
> >>>+
> >>>+ ret = dp_opt_set_bool(ad_ctx->ad_options->basic,
> >>>+ AD_ENABLE_GC, true);
> >>>+ if (ret != EOK) {
> >>>+ DEBUG(SSSDBG_MINOR_FAILURE,
> >>>+ ("Could not turn on GC support\n"));
> >>>+ /* Not fatal */
> >>>+ }
> >>>+ }
> >>>
> >>> switch (ar->entry_type & BE_REQ_TYPE_MASK) {
> >>> case BE_REQ_USER: /* user */
> >>
> >>...
> >>
> >>>+static errno_t sdap_posix_check_parse(struct sdap_handle *sh,
> >>>+ struct sdap_msg *msg,
> >>>+ void *pvt)
> >>>+{
> >>>+ struct berval **vals;
> >>>+ struct sdap_posix_check_state *state =
> >>>+ talloc_get_type(pvt, struct sdap_posix_check_state);
> >>>+ char *dn;
> >>>+
> >>>+ dn = ldap_get_dn(sh->ldap, msg->msg);
> >>>+ if (dn == NULL) {
> >>>+ DEBUG(SSSDBG_TRACE_LIBS,
> >>>+ ("Search did not find any entry with POSIX attributes\n"));
> >>>+ goto done;
> >>>+ }
> >>>+ DEBUG(SSSDBG_TRACE_LIBS, ("Found [%s] with POSIX attributes\n", dn));
> >>>+ ldap_memfree(dn);
> >>>+
> >>>+ vals = ldap_get_values_len(sh->ldap, msg->msg,
> >>>+ state->opts->user_map[SDAP_AT_USER_UID].name);
> >>>+ if (vals == NULL) {
> >>>+ vals = ldap_get_values_len(sh->ldap, msg->msg,
> >>>+ state->opts->group_map[SDAP_AT_GROUP_GID].name);
> >>>+ if (vals == NULL) {
> >>>+ DEBUG(SSSDBG_TRACE_LIBS, ("Entry does not have POSIX attrs?\n"));
> >>>+ goto done;
> >>>+ }
> >>>+ }
> >>
> >>If you really want to check if one of the attributes is present you
> >>might also want to check if the value is really an integer?
> >
> >I'm not sure, this code specifically checks the presence of the
> >attributes, the semantics is checked later when the attributes are used.
> >But the performance hit is very small and the additional checks might
> >bring some nicer error messages, so I've added them.
> >
> >Thank you for the review a new patch is attached.
>
> Ack.
Thank you both for the review, pushed to master and sssd-1-11
More information about the sssd-devel
mailing list