[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