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

[Pavel Reichl]

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

[Pavel Reichl]

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(a)lists.fedorahosted.org
>  https://lists.fedorahosted.org/mailman/listinfo/sssd-devel 
Sorry, there was forgotten //todo comment. Updated patches attached.

Thanks,
PR

[Jakub Hrozek]

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(a)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(a)redhat.com&gt;
>  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(a)redhat.com&gt;
>  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");
>

[Pavel Reichl]

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(a)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(a)redhat.com&gt;
> > 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(a)redhat.com&gt;
> > 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(a)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!