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

Jakub Hrozek jhrozek at redhat.com
Tue Sep 17 13:30:57 UTC 2013


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.



More information about the sssd-devel mailing list