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.

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

filter_users and filter_groups stop working properly in v 1.15 #246

Closed
wants to merge 5 commits into from

Conversation

Projects
None yet
5 participants
@fidencio
Copy link
Contributor

commented Apr 24, 2017

This patchset fix the issue reported on https://pagure.io/SSSD/sssd/issue/3362.

@pbrezina suggested to do the changes in a new cache_req module, but I'm really not sure whether we want to have NSS specific code (like nss_get_pwent() and nss_get_grent() calls) there.

For now I'm leaving this as it was before the nss/cache_req refactoring.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

Hi, we should solve this on cache_req level so we get the same resultt in nss and ifp (and others) responders. What I had in mind was to create another plugin function that will be called in the and of cache_req process. I.e. something like this:

/**
 * Filter the result through negative cache. 
 *
 * This is useful for plugins that don't use name as an input token but can be affected by filter_users and filter_groups options. 
 *
 * @return EOK    If the object is not found.
 * @return EEXIST If the object is found in negative cache.
 * @return Other errno code in case of an error.
 */
typedef errno_t
(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
                             struct sss_domain_info *domain,
                             struct ldb_message *msg);

In the end, we should iterate over the result and pass each ldb_message to the function (if defined) and remove it from the result if it is present in the negative cache.

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 25, 2017

This is really from top of my mind, there may be another, easier way to do it. But we should do it on cache_req level.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

@lslebodn

This comment has been minimized.

Copy link
Contributor

commented Apr 25, 2017

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from db59ef4 to 90f2f21 Apr 25, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 25, 2017

New patch set pushed and in the end the changes were not intrusive at all.

@pbrezina, I've partially taken your idea (with some changes).
Please, let me know if you're okay of the way the new plugin function has been introduced.

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from 90f2f21 to 79e2f83 Apr 26, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 26, 2017

  • NSS: Use fqnames when performing a ncache check
    This should be done in the ncache module, not in the callers.

  • RESPONDER: Make nss_get_name_from_msg() part of responder_utils
    Nikolai already did the same for his tlog integration and he need this function also in providers. Can you cherry-pick this commit instead so Nikolai doesn't have to solve conflicts?
    7465d48

  • CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function

 /**
+ * Filter the result through the negative cache.
+ *
+ * This is useful for plugins which don't use name as an input
+ * takes but can be affected by filter_users and filter_groups
     ^ token
+ * options.
+ */
+typedef errno_t
+(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
+                              struct sss_domain_info *domain,
+                              char *name);

* **CACHE_REQ: Make use of cache_req_ncache_filter_fn()**
```c
static errno_t
cache_req_search_get_name_from_msg(TALLOC_CTX *mem_ctx,
                                   struct ldb_message *msg,
                                   struct sss_domain_info *domain,
                                   bool override_space,
                                   char **_name)
{
    TALLOC_CTX *tmp_ctx;
    const char *name;
    char *output_name;
    char *fqname;
    errno_t ret;

    tmp_ctx = talloc_new(NULL);
    if (tmp_ctx == NULL) {
        return ENOMEM;
    }

    name = sss_get_name_from_msg(domain, msg);

^^^ name can be NULL and we don't want to fail with EINVAL in this case

    output_name = sss_output_name(tmp_ctx, name, domain->case_preserve,
                                  override_space);
    if (output_name == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_output_name() failed\n");
        ret = ENOMEM;
        goto done;
    }

    fqname = sss_create_internal_fqname(tmp_ctx, output_name, domain->name);
    if (fqname == NULL) {
        DEBUG(SSSDBG_CRIT_FAILURE, "sss_create_internal_fqname() failed\n");
        ret = ENOMEM;
        goto done;
    }

    *_name = talloc_steal(mem_ctx, fqname);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}

Besides these small things, the approach you've taken is good. I would just like you to do two more things -- move code that deals with creating the new result into separate function(s) into cache_req_result.c and enable this also for enumeration so you can remove the check (and the first patch) from nss responder.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 26, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

What do we want to do in this case? It would fail with EINVAL later on when filling the grent/pwent anyways, no?

That comment should said ENOMEM. If name is NULL, sss_output_name will return NULL and we fail with ENOMEM.

And last comment ... the introduced functions are not dealing with cache_req_result at all. What they do is changing the ldb_result and moving this part of the code to the cache_req_result.c does seem a little bit weird to me.

But it still deals in some sort of result that is used but cache_reqso I think it is ok. It deserves to be placed in separate function at least, doesn't matter if static or incache_req_result.c`.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

But there's no memory involved at all on sss_get_name_from_msg(). I'll follow you suggestion and update the PR anyways.

Yes, that means if SYSDB_NAME attribute exists, it is returned. What if the attribute does not exist? It should, but we should handle case were an error happened.

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from 79e2f83 to 7fc15b2 Apr 27, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

patchset updated.

CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
"Filtering out results from negative cache\n");

filtered_result = talloc_zero(mem_ctx, struct ldb_result);

This comment has been minimized.

Copy link
@pbrezina

pbrezina Apr 27, 2017

Member

Unused at this moment.

goto done;
}

msgs = talloc_zero(tmp_ctx, struct ldb_message *);

This comment has been minimized.

Copy link
@pbrezina

pbrezina Apr 27, 2017

Member

Because this function will filter out only users and groups from the configuration file, there won't be many objects filtered out. But there will be many allocations with enumeration. It would be better to allocate enough space to hold all the messages at once to safe some allocations. I.e.

msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count)

And remove the reallocs.

DEBUG(SSSDBG_CRIT_FAILURE,
"sss_get_name_from_msg() returned NULL, which should never "
"happen in this scenario!\n");
ret = ENOMEM;

This comment has been minimized.

Copy link
@pbrezina

pbrezina Apr 27, 2017

Member

ERR_INTERNAL since it is corrupted cache.

goto done;
}

cased_name = sss_get_cased_name(tmp_ctx, name, domain->case_preserve);

This comment has been minimized.

Copy link
@pbrezina

pbrezina Apr 27, 2017

Member

You used sss_output_name in previous patch set. Why did you change it?

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch 2 times, most recently from c9e4bf9 to f504651 Apr 27, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@pbrezina

This comment has been minimized.

Copy link
Member

commented Apr 27, 2017

One thing that is not exactly clear to me (and I may need your guidance) is ... just getting the name from the DB is not enough to the the fully-qualified name that we added to the negative cache? Do I really have to get the cased name and replace the space?

I'm not sure from the top of my head. Please, see what's stored in the negative cache, when you use overriden names, names with replaced space and qualified names in filter_users and filter_groups.

From a quick look into the code, I can see that we parse qualified names into shortname and domain. We also store the entries in internal fqname format. We don't use reverse_replace_space here. An overriden name is stored into ncache. I think you are fine with this code. But please test it. See: sss_ncache_prepopulate.

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 27, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented Apr 27, 2017

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from f504651 to c3993c3 Apr 28, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2017

Okay, patchset update and using directly the name we get from the message (which is the internal fqname).

It does work for the bug report and it doesn't break any tests that we already have.

IMO, if @pbrezina agrees, the patches are good to go.

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from c3993c3 to cbe92ef Apr 28, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented Apr 28, 2017

And I started a CI build ...

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 2, 2017

  • CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function
 /**
+ * Filter the result through the negative cache.
+ *
+ * This is useful for plugins which don't use name as an input
+ * takes but can be affected by filter_users and filter_groups
    ^ token
+ * options.
+ */
+typedef errno_t
+(*cache_req_ncache_filter_fn)(struct sss_nc_ctx *ncache,
+                              struct sss_domain_info *domain,
+                              const char *name);
+
+/**
  • CACHE_REQ: Make use of cache_req_ncache_filter_fn()
    Since the original result may be theoretically quite big, we should free it in cache_req_search_done after we obtain the filtered result and use talloc_steal in cache_req_search_ncache_filter to steal the messages on new context. i.e.
       msgs[msg_count] = talloc_steal(msgs, result->msgs[i]);
       msg_count++

And also use tmp_ctx for filtered result and then steal it (for clarity):

+    filtered_result = cache_req_create_ldb_result_from_msg_list(mem_ctx, msgs,
+                                                                msg_count);

^ tmp_ctx here

+    if (filtered_result == NULL) {
+        ret = ENOMEM;
+        goto done;
+    }
+
+    *_result = filtered_result;

^ *_result = talloc_steal(mem_ctx, filtered_result)

Besides those nitpicks, the patches are good to go.

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from cbe92ef to 3b9a888 May 2, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented May 2, 2017

@pbrezina: patch set updated.

I'm firing our internal CI with the latest changes.

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 5, 2017

static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx,
                                              struct cache_req *cr,
                                              struct ldb_result *result,
                                              struct ldb_result **_result)
{
    ...

    if (cr->plugin->ncache_filter_fn == NULL) {
        CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "This request type does not support filtering negative cache\n");
        ^^^ This request type does not support filtering result by negative cache

        filtered_result = cache_req_create_ldb_result_from_msg_list(tmp_ctx,
                                                                    result->msgs,
                                                                    result->count);
        if (filtered_result == NULL) {
            ret = ENOMEM;
            goto done;
        }

        goto immediately;

        ^^^ shorter version:
        *_result = talloc_steal(mem_ctx, result);

        ret = EOK;
        goto done;

    }

    CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                    "Filtering out results from negative cache\n");
    ^^^ Filtering out results by negative cache

    msgs = talloc_zero_array(tmp_ctx, struct ldb_message *, result->count);
    msg_count = 0;

    for (size_t i = 0; i < result->count; i++) {
        ...

        CACHE_REQ_DEBUG(SSSDBG_TRACE_FUNC, cr,
                        "[%s] is not present in negative cache\n",
                        name);

        ^^^ Please, remove this debug message.
            We will know which users are removed. We know which
            remain. This would be insanely long for enumeration.

        msgs[msg_count] = talloc_steal(msgs, result->msgs[i]);
        msg_count++;
    }

    ...

---   immediately:
    *_result = talloc_steal(mem_ctx, filtered_result);
    ret = EOK;

done:
    talloc_free(tmp_ctx);
    return ret;
}

spbnick and others added some commits Mar 22, 2017

NSS: Move output name formatting to utils
Move NSS nss_get_name_from_msg and the core of sized_output_name to the
utils to make them available to provider and other responders.
CACHE_REQ: Add a new cache_req_ncache_filter_fn() plugin function
This function will be responsible for filtering out all the results that
we have that are also present in the negative cache.

This is useful mainly for plugins which don't use name as an input token
but can still be affected by filter_{users,groups} options.

For now this new function is not being used anywhere.

Related:
https://pagure.io/SSSD/sssd/issue/3362

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
CACHE_REQ_RESULT: Introduce cache_req_create_ldb_result_from_msg_list()
Similarly to what cache_req_create_ldb_result_from_msg() does this new
function creates a new ldb_result from a list of ldb_message.

It's going to be used in the follow-up patch where some messages from
ldb_result may be filtered and then a new ldb_result has to be created.

Related:
https://pagure.io/SSSD/sssd/issue/3362

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>
test_ldap.py: Add test for filter_{users,groups}
Related:
https://pagure.io/SSSD/sssd/issue/3362

Reviewed-by: Fabiano Fidêncio <fidencio@redhat.com>
CACHE_REQ: Make use of cache_req_ncache_filter_fn()
This patch makes use of cache_req_ncache_filter_fn() in order to process
the result of a cache_req search and then filter out all the results
that are present in the negative cache.

The "post cache_req search" result processing is done basically in two
different cases:
- plugins which don't use name as an input token (group_by_id, user_by_id
  and object_by_id), but still can be affected by filter_{users,groups}
  options;
- plugins responsible for groups and users enumeration (enum_groups and
  enum_users);

Resolves:
https://pagure.io/SSSD/sssd/issue/3362

Signed-off-by: Fabiano Fidêncio <fidencio@redhat.com>

@fidencio fidencio force-pushed the fidencio:wip/filter_users_groups branch from 3b9a888 to a5c3fc1 May 5, 2017

@fidencio

This comment has been minimized.

Copy link
Contributor Author

commented May 5, 2017

Patchset updated!

@pbrezina

This comment has been minimized.

Copy link
Member

commented May 10, 2017

Ack. Thank you for your patience.

@jhrozek jhrozek added the Accepted label May 10, 2017

@jhrozek

This comment has been minimized.

Copy link
Contributor

commented May 10, 2017

@jhrozek jhrozek closed this May 10, 2017

@jhrozek jhrozek added the Pushed label May 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.