[SSSD] [PATCHES] LDAP: SID-Mapping - Store non-POSIX users in cache if they have a SID

Pavel Reichl preichl at redhat.com
Wed Oct 15 09:11:30 UTC 2014


On 06/27/2014 11:06 AM, Jakub Hrozek wrote:
> On Tue, Jun 24, 2014 at 02:35:30PM +0200, Pavel Reichl wrote:
>> On Thu, 2014-05-22 at 11:30 +0200, Pavel Reichl wrote:
>>> Hello,
>>>
>>> please see attached patches.
>>>
>>> I have briefly discussed with Jakub how to handle saving users with uid
>>> 0 whether to resurrect sysdb_add_fake_user or modify existing fuctions
>>> for storing users. I decided to add wrapper function around existing
>>> ones to minimize changes in code which calls them.
>>>
>>> Thanks,
>>>
>>> Pavel Reichl
>>> _______________________________________________
>>> sssd-devel mailing list
>>> sssd-devel at lists.fedorahosted.org
>>> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>> Sorry, there was forgotten //todo comment. Updated patches attached.
>>
>> Thanks,
>> PR
>
> I'm sorry this review took so long. Don't worry about pushing this patch
> out of 1.12.0 and into 1.12.1.
>
>>  From c0e570a8381f9abda157ba9dc25554f51773325e Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Thu, 22 May 2014 10:37:57 +0100
>> Subject: [PATCH 1/2] SYSDB: enable storing urers with uid 0
>>
>> Store users with uid 0 if explicitly asked to.
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2117
>> ---
>>   src/db/sysdb.h     |  13 +++++++
>>   src/db/sysdb_ops.c | 109 ++++++++++++++++++++++++++++++++++++++++-------------
>>   2 files changed, 95 insertions(+), 27 deletions(-)
>>
>> diff --git a/src/db/sysdb.h b/src/db/sysdb.h
>> index 911430eefcbf520ca04f123a3b6f1882dc25876b..79f8b6b04a8e48af8154ae9a5437ec4fba56790a 100644
>> --- a/src/db/sysdb.h
>> +++ b/src/db/sysdb.h
>> @@ -658,6 +658,19 @@ int sysdb_store_user(struct sss_domain_info *domain,
>>                        uint64_t cache_timeout,
>>                        time_t now);
>>   
>> +int sysdb_store_user_tolerate_uid_zero(struct sss_domain_info *domain,
>> +                                       const char *name,
>> +                                       const char *pwd,
>> +                                       uid_t uid, gid_t gid,
>> +                                       const char *gecos,
>> +                                       const char *homedir,
>> +                                       const char *shell,
>> +                                       const char *orig_dn,
>> +                                       struct sysdb_attrs *attrs,
>> +                                       char **remove_attrs,
>> +                                       uint64_t cache_timeout,
>> +                                       time_t now);
> I'm not really thrilled about the naming conventions. What if the
> function was named sysdb_store_nonposix_user() and explicitly added or
> set the SYSDB_UIDNUM attribute to 0?
>
> There should also be a unit test along with the new function.
>
>> +
>>   int sysdb_store_group(struct sss_domain_info *domain,
>>                         const char *name,
>>                         gid_t gid,
>> diff --git a/src/db/sysdb_ops.c b/src/db/sysdb_ops.c
>> index 4d31cb6a0e83370a2c49a666f9474d93baf430b0..8a3d5094d7f29248b7a64093bee95534e655db1d 100644
>> --- a/src/db/sysdb_ops.c
>> +++ b/src/db/sysdb_ops.c
>> @@ -1246,16 +1246,17 @@ done:
>>   
>>   /* =Add-User-Function===================================================== */
>>   
>> -int sysdb_add_user(struct sss_domain_info *domain,
>> -                   const char *name,
>> -                   uid_t uid, gid_t gid,
>> -                   const char *gecos,
>> -                   const char *homedir,
>> -                   const char *shell,
>> -                   const char *orig_dn,
>> -                   struct sysdb_attrs *attrs,
>> -                   int cache_timeout,
>> -                   time_t now)
>> +static int sysdb_add_user_impl(struct sss_domain_info *domain,
> What does the _impl suffix mean?
>
>> +                               const char *name,
>> +                               uid_t uid, gid_t gid,
>> +                               const char *gecos,
>> +                               const char *homedir,
>> +                               const char *shell,
>> +                               const char *orig_dn,
>> +                               struct sysdb_attrs *attrs,
>> +                               int cache_timeout,
>> +                               time_t now,
>> +                               bool tolererate_uid_zero)
> Again, I think we should have a common naming, in both patches and I'd
> like to propose POSIX/non-POSIX.
>
>
>>  From 21d66ed6b303837c050fb5f185686abe74265af9 Mon Sep 17 00:00:00 2001
>> From: Pavel Reichl <preichl at redhat.com>
>> Date: Thu, 22 May 2014 10:53:15 +0100
>> Subject: [PATCH 2/2] LDAP: save non-posix users with ID-mapping off
>>
>> Save such users into sysdb with UID and GID set to 0.
>> Also set isPosix attribute to FALSE.
>>
>> Resolves:
>> https://fedorahosted.org/sssd/ticket/2117
>> ---
>>   src/providers/ldap/ldap_id.c          | 10 ++++-----
>>   src/providers/ldap/sdap_async_users.c | 41 +++++++++++++++++++++++++----------
>>   2 files changed, 35 insertions(+), 16 deletions(-)
>>
>> diff --git a/src/providers/ldap/ldap_id.c b/src/providers/ldap/ldap_id.c
>> index c788b6bdd6235f5b940d99382b115a2534dbb1d9..10b7a491472aa56540dba34d0ef6303f5cab910b 100644
>> --- a/src/providers/ldap/ldap_id.c
>> +++ b/src/providers/ldap/ldap_id.c
>> @@ -197,14 +197,14 @@ struct tevent_req *users_get_send(TALLOC_CTX *memctx,
>>                                           ctx->opts->user_map[SDAP_AT_USER_NAME].name,
>>                                           ctx->opts->user_map[SDAP_AT_USER_OBJECTSID].name);
>>       } else {
>> -        /* When not ID-mapping, make sure there is a non-NULL UID */
>> +        /* When not ID-mapping, do not require UID attribute to be present and
>> +         * if present do not constrain allowed value
>> +         */
>>           state->filter = talloc_asprintf(state,
>> -                                        "(&(%s=%s)(objectclass=%s)(%s=*)(&(%s=*)(!(%s=0))))",
>> +                                        "(&(%s=%s)(objectclass=%s)(%s=*))",
>>                                           attr_name, clean_name,
>>                                           ctx->opts->user_map[SDAP_OC_USER].name,
>> -                                        ctx->opts->user_map[SDAP_AT_USER_NAME].name,
>> -                                        ctx->opts->user_map[SDAP_AT_USER_UID].name,
>> -                                        ctx->opts->user_map[SDAP_AT_USER_UID].name);
>> +                                        ctx->opts->user_map[SDAP_AT_USER_NAME].name);
> I think this change made the filter a bit too relaxed. I think the
> proper change would be to allow only entries that don't have a UID but
> have a SID. Something like (untested, but I hope it illustrates the idea):
>      (|(&(uidNumber=*)(!(uidNumber=0)))(objectSID=*))
>
> Ideally there would be a unit test that would save a basic POSIX user, a
> basic non-POSIX user and a POSIX user with uidNumber=0. But I'm not sure
> how easy this would be, our dependencies in the LDAP provider did not
> have the best granularity. If it was easy to write this unit test, I
> would prefer it.
>
>>       }
>>   
>>       talloc_zfree(clean_name);
>> diff --git a/src/providers/ldap/sdap_async_users.c b/src/providers/ldap/sdap_async_users.c
>> index be0536ef3c3ffaf398c4f2d3e85094d4deb7bd81..ccc4fca06e1bf3cc12fb579935022c3fa7a65b10 100644
>> --- a/src/providers/ldap/sdap_async_users.c
>> +++ b/src/providers/ldap/sdap_async_users.c
>> @@ -142,6 +142,7 @@ int sdap_save_user(TALLOC_CTX *memctx,
>>       char *sid_str;
>>       char *dom_sid_str = NULL;
>>       struct sss_domain_info *subdomain;
>> +    bool is_fake_user = false;
> I would prefer to use is_posix here. I realize I told you the term 'fake
> user' myself, but we had used it with a special meaning in the past --
> the 'fake' user not only had no UID, but was also marked as expired and
> only present in the cache until the next request. Kindof like the ghost
> users we have now.
>
>>   
>>       DEBUG(SSSDBG_TRACE_FUNC, "Save user\n");
>>   
> _______________________________________________
> sssd-devel mailing list
> sssd-devel at lists.fedorahosted.org
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
Hello,

please see attached patch set hopefully addressing all comments above.

Please be aware that following setting may be necessary when using LDAP 
provider

For IPA schema:
ldap_user_objectsid = ipaNTSecurityIdentifier

For AD schema:
ldap_user_objectsid = objectSID

AD case should be covered when Michal's patches gets pushed (LDAP: 
Change defaults for ldap_user/group_objectsid).

Thanks!
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-SYSDB-enable-storing-users-with-uid-0.patch
Type: text/x-patch
Size: 31878 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141015/6f6c48b3/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-LDAP-save-non-posix-users-with-ID-mapping-off.patch
Type: text/x-patch
Size: 6645 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141015/6f6c48b3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-TESTS-unit-tests-for-storing-non-posix-users.patch
Type: text/x-patch
Size: 3115 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20141015/6f6c48b3/attachment-0002.bin>


More information about the sssd-devel mailing list