URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_s…

fidencio commented: """ As this patch should also come with a test, I'm adding the "Changes Requested" label and I will revisit it soon. """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-303070562

URL: https://github.com/SSSD/sssd/pull/277 Author: fidencio Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_s… Action: edited

Changed field: body Original value: """ cache_req_send() may take some shortcuts in case the object is found in the cache and it's still valid.

This behaviour may lead to exposing filtered users and groups when they're searched by their uid/gid.

A solution for this issue was proposed on 4ef0b19a but, unfortunately, didn't take into consideration that this shortcut could be taken.

There are basically two really easy ways to test this issue: 1) Using enumeration: - Set "enumerate = True" in the domain section - restart SSSD cleaning up the cache; - Wait a little bit till the enumerated users are cached - id <uid of a user who is part of the filter_users>

2) Not using enumeration: - getent passwd <uid of a user who is part of the filter_users> - Wait a little bit till the user is cached - id <same uid used above>

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

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

URL: https://github.com/SSSD/sssd/pull/277 Author: fidencio Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send() Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/277/head:pr277 git checkout pr277

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

pbrezina commented: """ I do not like having a temporary memory context inside `cache_req_search_send` and since we are using `cache_req_search_ncache_filter` on two places now, I would like you to change it a little bit so we have clear and unified usage.

The purpose of `cache_req_search_ncache_filter` is to sieve result from cache trough the negative cache and return a new result. The reason why a temporary memory context is needed here is that we return the original result as a "performance improvement" and we do not know in the caller whether we created a new result or reused the original one. We should not use the original result in the caller anymore regardless of what was returned from the filter.

I propose the following change:

```c static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx, struct cache_req *cr, struct ldb_result *result, struct ldb_result **_result) =>

static errno_t cache_req_search_ncache_filter(TALLOC_CTX *mem_ctx, struct cache_req *cr, struct ldb_result **_result) ```

The function will take an input/output argument `_result` and it will free the original result if needed. Then a caller doesn't have to care about it at all.

"""

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-303375053

URL: https://github.com/SSSD/sssd/pull/277 Author: fidencio Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send() Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/277/head:pr277 git checkout pr277

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

pbrezina commented: """ Hmm, also case for midpoint refresh needs to be handled. If `cache_req_expiration_status` returns `CACHE_OBJECT_VALID` or `CACHE_OBJECT_MIDPOINT`, you want to pass the result to the filter and return it immediately (for midpoint case an oob dp request needs to be fired). I think the best solution is to move the filter inside `ret == EOK` section in `done`. """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-305728937

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

fidencio commented: """ @pbrezina: I've updated the last patch according to your suggestion.

Thanks for the review. """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-305734608

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

lslebodn commented: """ FYI: much simpler reproducer is getent passwd 12785; sleep 2; getent passwd 12785 It works also with "id $num" because it calls getpwuid as well. Do we want to change commit message? """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-306174192

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

fidencio commented: """ @lslebodn: I've just wrote a simple test for it.

Let me make sure the test actually tests the issue and I'll update the PR in a few. """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-306183882

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

fidencio commented: """ I've updated the commit message and added the test for the change.

I'm removing the "Changes Requested" label for now (as the tests were the missing part). """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-306188498

URL: https://github.com/SSSD/sssd/pull/277 Author: fidencio Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send() Action: synchronized

To pull the PR as Git branch: git remote add ghsssd https://github.com/SSSD/sssd git fetch ghsssd pull/277/head:pr277 git checkout pr277

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

lslebodn commented: """ master: * 13205258cc17d3833558244251f5adbc98cf34e5 * 4c09cd008967c5c0ec358dc658ffc6fc1cef2697 * c8193b1602cf44740b59f5dfcdc5330508c0c365 """

See the full comment at https://github.com/SSSD/sssd/pull/277#issuecomment-306218109

URL: https://github.com/SSSD/sssd/pull/277 Title: #277: CACHE_REQ_SEARCH: Check for filtered users/groups also on cache_req_send()

Label: +Pushed