[SSSD] [PATCHES] simple access provider: support subdomain users and groups

Jakub Hrozek jhrozek at redhat.com
Tue Sep 17 14:27:00 UTC 2013


On Tue, Sep 17, 2013 at 03:30:57PM +0200, Jakub Hrozek wrote:
> On Tue, Sep 17, 2013 at 01:49:30PM +0200, Pavel Březina wrote:
> > On 09/16/2013 05:35 PM, Jakub Hrozek wrote:
> > >On Mon, Sep 16, 2013 at 05:30:38PM +0200, Jakub Hrozek wrote:
> > >>On Thu, Sep 12, 2013 at 02:01:43PM +0200, Pavel Březina wrote:
> > >>>On 09/10/2013 03:46 PM, Jakub Hrozek wrote:
> > >>>>On Tue, Sep 03, 2013 at 12:42:21PM +0200, Pavel Březina wrote:
> > >>>>>Patch 0001: I haven't used this one after all, but I still think it
> > >>>>>is nice to have. It reduces amount of code duplication.
> > >>>>>
> > >>>>>Patch 0005:
> > >>>>>>+    /* Try to get group SID and assign it a domain */ +
> > >>>>>>group_sid = ldb_msg_find_attr_as_string(group, SYSDB_SID_STR,
> > >>>>>>NULL); +    if (group_sid == NULL) {
> > >>>>>
> > >>>>>I'm not sure if this branch isn't a dead code, but I kept it there
> > >>>>>because it mimics current behaviour.
> > >>>>>
> > >>>>>>+        /* We will look it up in main domain. */ +        domain
> > >>>>>>= state->ctx->domain; +    } else { +        domain =
> > >>>>>>find_subdomain_by_sid(state->ctx->domain, group_sid); +        if
> > >>>>>>(domain == NULL) { +            DEBUG(SSSDBG_CRIT_FAILURE,
> > >>>>>>("There is no domain information for " +
> > >>>>>>"SID %s\n", group_sid)); +            return ENOENT; +        } +
> > >>>>>>}
> > >>>>
> > >>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com> Date:
> > >>>>>Mon, 2 Sep 2013 13:36:25 +0200 Subject: [PATCH 1/6] util: add
> > >>>>>sss_idmap_talloc[_free]
> > >>>>>
> > >>>>>Remove code duplication.
> > >>>>
> > >>>>Removing code duplication added code :-) 76 insertions(+), 62
> > >>>>deletions(-)
> > >>>>
> > >>>>But it's mostly the comment blobs, so ack.
> > >>>>
> > >>>>>From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com> Date:
> > >>>>>Tue, 27 Aug 2013 14:53:03 +0200 Subject: [PATCH 2/6] simple access
> > >>>>>tests: fix typos
> > >>>>
> > >>>>ACK
> > >>>>
> > >>>>>From 5648c71b1a5a6bad5ce8e3361c7b4baf7d972c0b Mon Sep 17 00:00:00
> > >>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>>>Date: Tue, 27 Aug 2013 14:02:42 +0200 Subject: [PATCH 3/6] simple
> > >>>>>provider: support subdomain users
> > >>>>>
> > >>>>>Resolves: https://fedorahosted.org/sssd/ticket/2034
> > >>>>
> > >>>>There is one thing that still needs fixing -- the case sensitivity
> > >>>>flag is still read from the master domain through be_ctx->domain. In
> > >>>>general, the providers should not access be_ctx->domain directly, but
> > >>>>rather the handler should assign a sss_domain_info structure to some
> > >>>>per-provider or per-request state and the code should operate on
> > >>>>that.
> > >>>>
> > >>>>>From c1b31c3c2eefa71f5c17539fd615a785d7e1ccd0 Mon Sep 17 00:00:00
> > >>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>>>Date: Mon, 2 Sep 2013 15:15:59 +0200 Subject: [PATCH 4/6] util: add
> > >>>>>find_subdomain_by_sid()
> > >>>>
> > >>>>ACK, but please file a ticket to mass-namespace the functions in
> > >>>>this module.
> > >>>
> > >>>https://fedorahosted.org/sssd/ticket/2086
> > >>>
> > >>>>>From 78fff868aa9f1b5a8392328973e4ce3ba000a9e6 Mon Sep 17 00:00:00
> > >>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>>>Date: Wed, 28 Aug 2013 11:35:27 +0200 Subject: [PATCH 5/6] simple
> > >>>>>provider: support subdomain groups
> > >>>>>
> > >>>>
> > >>>>Same comment about case sensitivity.
> > >>>>
> > >>>>>From 844b9cc30ad694116772f519791b3725a72d178a Mon Sep 17 00:00:00
> > >>>>>2001 From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > >>>>>Date: Tue, 3 Sep 2013 12:34:07 +0200 Subject: [PATCH 6/6] simple
> > >>>>>access test: initialize be_ctx for all tests
> > >>>>>
> > >>>>
> > >>>>ACK
> > >>>
> > >>>Thanks. New patches are attached. I'm sending the case sensitivity
> > >>>change in a separate patch, since I think it can stand on its own as
> > >>>improvement of subdomains support in simple access provider.
> > >>>
> > >>>I also included patch that contains find_subdomain_by_object_name().
> > >>>
> > >>
> > >>Hi,
> > >>
> > >>as part of review, I resolved conflict between your and Lukas' patches.
> > >>Can you check if it's OK?
> > >
> > >No it's not :-)
> > >
> > >I'll prepare a local version for my review, but it would be nice if you
> > >resent the whole set.
> > 
> > Rebased patches are attached.
> > 
> 
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Mon, 2 Sep 2013 13:36:25 +0200
> > Subject: [PATCH 1/8] util: add sss_idmap_talloc[_free]
> 
> ID mapping still works in the AD provider. I haven't tested the other
> providers, but make check also passes and we'd know if there were
> unresolved symbol.
> 
> ACK
> 
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Tue, 27 Aug 2013 14:53:03 +0200
> > Subject: [PATCH 2/8] simple access tests: fix typos
> ACK
> 
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Tue, 27 Aug 2013 14:02:42 +0200
> > Subject: [PATCH 3/8] simple provider: support subdomain users
> 
> ACK. I tested both positive and negative tests with trusted AD and
> trusted native AD provider users. The patch looks good as well.
> 
> > Date: Mon, 2 Sep 2013 15:15:59 +0200
> > Subject: [PATCH 4/8] util: add find_subdomain_by_sid()
> > 
> > This function takes domain SID (doesn't have the last component)
> > or object SID (have all components) and returns subdomain.
> > 
> > The subdomain is found by comparing domain->domainid with the SID.
> > 
> > E.g.
> > domain SID: S-1-5-21-3940105347-3434501867-2690409756
> > object SID: S-1-5-21-3940105347-3434501867-2690409756-513
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2034
> 
> ACK. Tested as part of the other patches. The code looks good to me.
> 
> > From 4ed75ba1e6ab17968f56c919111389a77bde8ed0 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Thu, 12 Sep 2013 12:55:44 +0200
> > Subject: [PATCH 5/8] util: add find_subdomain_by_object_name()
> > 
> > This function will parse object name into name and domain
> > name part and return appropriate sss domain.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2034
> 
> ACK. If we ever support non-FQDN names for subdomain object, as some
> users request, we would have to hunt for cases like the one in this
> patch, where no domain gets translated into the parent domain. But I
> think that's OK.
> 
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Wed, 28 Aug 2013 11:35:27 +0200
> > Subject: [PATCH 6/8] simple provider: support subdomain groups
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2034
> > ---
> 
> The domain structure from ctx is now only used as anchor for looping
> over domains starting from the primary domain. Both positive and
> negative tests with group names pass.
> 
> ACK
> 
> > From 1178e290b45a889e3609ff085891e525eceadd2d Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Tue, 3 Sep 2013 12:34:07 +0200
> > Subject: [PATCH 7/8] simple access test: initialize be_ctx for all tests
> > 
> > Recent simple access provider patches started using
> > be_ctx during access check. This caused segfault in
> > unit tests, since be_ctx wasn't initialized.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2034
> > ---
> 
> ACK. The tests passes and the code looks OK to me.
> 
> > From 11cf0a7dc49c3074861babfd0c825a7d56710ff6 Mon Sep 17 00:00:00 2001
> > From: =?UTF-8?q?Pavel=20B=C5=99ezina?= <pbrezina at redhat.com>
> > Date: Thu, 12 Sep 2013 13:15:47 +0200
> > Subject: [PATCH 8/8] simple provider: obey case sensitivity for subdomain
> >  users and groups
> > 
> > When comparing username and his groups to access list, we will
> > obey case sensitivity of object from access list.
> > 
> > Resolves:
> > https://fedorahosted.org/sssd/ticket/2034
> 
> ACK. I tested with IPA native users and AD trusted users. With Admin in
> the allow ACL, admin can't login, but with Tuser in the allow ACL, tuser
> from AD could.

Pushed all patches to master and sssd-1-11



More information about the sssd-devel mailing list