[SSSD] [PATCH] LDAP: Detect the presence of POSIX attributes

Pavel Březina pbrezina at redhat.com
Wed Feb 12 10:30:24 UTC 2014


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.




More information about the sssd-devel mailing list