[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