[PATCH] Retain group members in setups where id_provider=ad and ignore_group_members=True
by Jakub Hrozek
Hi,
the attached patch fixes https://fedorahosted.org/sssd/ticket/2646. See
the commit message for the problem description. The bug was found by
Lukas (thanks!), I just wrote the patch.
I was not really happy with some parts of the patch myself, but I
couldn't find a better way to fix the ticket. Here are some parts of the
patch that I think need careful review:
- is_fake_tgs_group() uses heuristics based on the AD schema, RDN value
and and POSIX/non-POSIX flag. Is that OK or too much of a hack?
- sdap_get_tokengroup_members() plays a bit of a trick on the memory
hierarchy. I wanted the API of this function to only return a single
member (a ldb_message_element) but at the same time, the cache search
returns only the top-level ldb_message and the first element as talloc
pointers. So the functions returns the element, which points to the
message, but the element owns the message. I checked with valgrind that
there are no memory errors. I really want to avoid copying all the
members, because that number can go to thousands. The only alternative
that comes to mind is to run a second cache search only for the member
attribute, then the member would be a talloc pointer -- but this way
seems to rely a bit on ldb implementation which I don't like either.
- sdap_save_grpmem() and sdap_save_groups() now use a saved_ldap_group
structure. This means we now need to do an extra allocation per group,
but the number of groups is usually not that big (max. hundreds,
definitely not thousands like members).
Thanks for review!
8 years, 10 months
[PATCH] BUILD: Fix variable substitution in cwrap.m4
by Nikolai Kondrashov
Hi everyone,
Sumit found that our cwrap tests are not running. After quick research I made
the attached patch fixing that. The regression was introduced by my
integration test patch.
Nick
8 years, 10 months
[PATCHES] Support GPOs referred from other domains
by Stephen Gallagher
Patch 0001: LDAP: Support returning referral information
Some callers may be interested in the raw referral values returned from
a lookup. This patch allows interested consumers to get these referrals
back and process them if they wish. It does not implement a generic
automatic following of referrals.
Patch 0002: AD GPO: Support processing referrals
For GPOs assigned to a site, it's possible that their definition
actually exists in another domain. To retrieve this information,
we need to follow the referral and perform a base search on
another domain controller.
Resolves:
https://fedorahosted.org/sssd/ticket/2645
8 years, 10 months
Design document - Do not always override home directory with subdomain_homedir value in server mode
by Pavel Reichl
Hello,
please see design page for simple - RFE: Do not always override home
directory with subdomain_homedir value in server mode.
https://fedorahosted.org/sssd/wiki/DesignDocs/use_AD_homedir
**************************************************************************
**************************************************************************
**************************************************************************
= Do not always override home directory with subdomain_homedir value in
server mode =
Related ticket(s):
* https://fedorahosted.org/sssd/ticket/2583
=== Problem statement ===
Prior to sssd 1.12, we didn't have the ability to read home directory
values from AD in AD-IPA trust setups at all. Instead, we always used
the `subdomain_homedir` value. We can read custom LDAP values now, but
in order to stay backwards-compatible, we kept using the
`subdomain_homedir` value.
=== Use cases ===
Users from AD with POSIX attributes want to use individually set value
for home directory.
=== Overview of the solution ===
`subdomain_homedir` for SSSD in server mode should support '%o' template
expansion (The original home directory retrieved from the identity
provider). In case when `subdomain_homedir` would be expanded to an
empty string ('subdomain_homedir=%o' and AD user without POSIX
attributes) SSSD should not error out but `fallback_homedir` should be
utilized instead.
=== Implementation details ===
* Extend set of attributes returned by `get_object_from_cache()` by
SYSDB_HOMEDIR attribute.
* Update `apply_subdomain_homedir()`
* to parse AD home dirrctory from ldb_msg
* do not call `store_homedir_of_user()` if value of expanded home
directory is an empty string.
* Extend interface of `get_subdomain_homedir_of_user()` to accept AD
home directory as parameter.
=== Configuration changes ===
No configuration changes are proposed.
=== How To Test ===
1. On SSSD in server mode
* For AD user with posix attributes set home directory attribute
* in sssd.conf set `subdomain_homedir` option to '%o'
* delete cache and restart SSSD
* call `getent passwd user` and check that home directory reflects
value from AD
* For AD user `without` posix attributes
* in sssd.conf set `subdomain_homedir` option to %o and
`fallback_homedir` to /home/%u
* delete cache and restart SSSD
* call `getent passwd user` and check that home directory reflects
`fallback_homedir`
2. On SSSD acting as IPA client
* Check that results are the same as on SSSD in server mode and that
local `fallback_homedir` is ignored
=== Authors ===
preichl(a)redhat.com
8 years, 10 months
Re: [SSSD] [PATCH] Add integration tests
by Nikolai Kondrashov
On 04/28/2015 05:49 PM, Michal Židek wrote:
> On 04/27/2015 05:56 PM, Nikolai Kondrashov wrote:
>>> On 04/24/2015 01:44 PM, Nikolai Kondrashov wrote:
>> There is no actual libc interface to check for user existence, so I think
>> having an assertion for that will be misleading. If you don't care what
>> attributes a user has, and only want to check that it exists, just pass an
>> empty pattern:
>>
>> assert_passwd_by_name("user1", {})
>>
>> BTW, I wouldn't like to change "passwd" to "user". I think it will be more
>> straightforward to simply use NSS database names for all entry types.
>
> assert_passwd_by_name is a good name and it is probably better than
> the _has_attributes that I suggested. My strongest request was that
> all assert functions start with the prefix assert_ .
Alright.
>>> Do not worry too much if the identifiers get longer. Sometimes
>>> it is better to sacrifice the keyboard lifetime for the sake of
>>> better readability :) .
>>
>> More verbose interface might be easier to read the first few times, but
>> becomes a burden both to read (more text to scan) and to write (more
>> text to
>> type and make mistakes in), after you learn it.
>
> I tend to disagree here. It is IMO much greater burden if I need
> to check internals of functions just to get an idea
> of what the function is doing.
I don't like that either. I guess my function names and descriptions were just
not clear enough, or maybe we don't match on the acceptable clarity.
>>>> I attempted to describe the logic behind the generic matching function
>>>> "diff", but please ask if anything is not clear. There is one particular
>>>> flaw to it, though: there is no way to match a list partially, e.g. to
>>>> match for presence of just a few group members, instead of all. I
>>>> think we
>>>> can employ tuples for these. I.e. say that a tuple pattern matches a
>>>> list
>>>> if all tuple items match at least one list item, but not necessarily the
>>>> other way around.
>>>
>>> It will be better to remove this limitation, but not with the
>>> tuples.
>>> If think it is not good to have change in behaviour based on whether
>>> the argument is tuple or list. A better approach would be to have
>>> additional boolean argument in diff(), named for example match_all with
>>> default set to False (or True). If the parameter is True, then
>>> all returned entries must match some pattern, if not, it is enough
>>> if each given pattern matches some entries in the returned list of
>>> entries. Would that work for you?
>>
>> I would very much like to avoid this solution. It would be all-or-nothing
>> (i.e. no mixing different conditions in one call, no matter how high-level)
>> and would increase the code size by requiring passing that variable through
>> the stack of many functions.
>
> Well, the parameter match_all would have default value (I think False
> would be a good default) so the user will not have to pass it if
> he is OK with the default behaviour. And the stack of the functions
> is not very high (2) so I do not see this as a big problem.
The stacks are not deep but there are quite a few of them. These functions
would need to have that argument (using current, not projected names), along
with some limitations:
diff - matching all or some entries
passwd_list_assert - matching all or some entries
passwd_diff - matching all or some entries
passwd_assert - matching all or some entries
group_assert_by_name - matching all or some group members
group_assert_by_uid - matching all or some group members
group_list_assert - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
group_name_dict_assert - matching all or some group members
group_gid_dict_assert - matching all or some group members
passwd_diff - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
passwd_assert - matching all or some entries and members,
but cannot match all entries with some members, or
some entries with all members
We can, of course, remove or change some of these functions, but the general
problem will stay and it will reduce flexibility, forcing us to add more
functions eventually.
>> We already have different matching logic for dictionaries and lists. Adding
>> tuples doesn't make it much more complex, and keeps the patterns
>> succinct and
>> expressive.
>
> The difference between lists and dictionaries is visually big
> enough (if the dictionary is not empty) to make sure the user will
> not mistake the two and get the expected behaviour. In case
> of tuples and lists, the visual difference is not so big.
>
> I think it will be better if we get rid of this behaviour where
> different parameter types do these subtle diffrencies in
> behavior.
What if we think about these patterns not just as fixed format arguments, but
rather similar to, say, regexes? Sure, regexes are cryptic, and subtle
differences in them introduce significant behavior changes, like omitting or
inserting an extra backslash, using '*', instead of '+', etc. Yet, they're
also powerful.
I understand your motivation to keep the interface accessible and safe, and
I'm willing to implement it your way, if this message doesn't change your
mind.
May I suggest another solution, which might make it more palatable? What if we
replace lists/tuples with functions or class constructors, which would
annotate what's happening?
So, instead of writing this rather unusual, but illustrative example:
pattern = (
{name='group1', gid=1001, mem=['userA', 'userB', 'userC']},
{name='group2', gid=1002, mem=('userX', 'userY')},
)
ent.group_assert(pattern)
You would write something like this:
pattern = ent.some(
{name='group1', gid=1001, mem=ent.all('userA', 'userB', 'userC')},
{name='group2', gid=1002, mem=ent.some('userX', 'userY')}
)
ent.group_assert(pattern)
Theoretically, we can also make a distinction of matching some/all of
dictionary members, but I don't see that as useful ATM.
> We can add "type" parameter to the diff function and init
> the messages based on this parameter. This way we will not
> have to copy-paste the code and we could use more specific
> messages.
Hmm, I guess that might go some way. I'll try it, thanks.
> Also do not forget to rebase the patches on top of the
> current master. There were some changes in the build
> files recently.
Sure, of course.
Nick
8 years, 10 months
