[SSSD] [PATCHES] Add services map support to the LDAP Provider

Jakub Hrozek jhrozek at redhat.com
Tue Jan 31 09:58:21 UTC 2012


On Mon, Jan 30, 2012 at 09:26:21AM -0500, Stephen Gallagher wrote:
> On Mon, 2012-01-30 at 15:04 +0100, Jakub Hrozek wrote:
> > On Sat, Jan 28, 2012 at 02:10:19PM -0500, Stephen Gallagher wrote:
> > > On Sat, 2012-01-28 at 13:52 -0500, Stephen Gallagher wrote:
> > > > On Sat, 2012-01-28 at 12:08 -0500, Stephen Gallagher wrote:
> > > > > I'm still working on the enumeration support, but I wanted to get the
> > > > > direct lookup on the list early.
> > > > > 
> > > > > Patch 0001: extend sysdb_store_service() to accept additional attributes
> > > > > This is necessary to be able to store provider-specific data in the
> > > > > entry (such as the entryUSN).
> > 
> > Can you move the NULL check of msg->dn outside the switch so that there
> > is only one and all cases are checked? Currently the service case does
> > not have a NULL check.
> > 
> 
> Fixed. Thanks, I meant to do that in the first place and then... didn't.
> 
> > You should also pass primary_name, not name into the call of
> > sysdb_remove_attrs() because name points to ldb_message structure that
> > was already freed.
> > 
> 
> Good catch, thanks!
> 
> > > > > 
> > > > > Patch 0002: Add a helper function to get an integer value from a
> > > > > sysdb_attrs object directly as a uint16_t.
> > > > > 
> > 
> > Ack
> > 
> > > > > Patch 0003: The big one. This patch adds support for looking up services
> > > > > in LDAP. There shouldn't be any big "gotchas" here. It's very
> > > > > straightforward (if long).
> > > > > 
> > 
> > Setting errno to 0 before strtouint16() is not needed, strtouint16()
> > does it internally.
> > 
> 
> Removed.
> 
> > Otherwise Ack.
> > 
> > Patch 0004: LDAP: Add enumeration support for services
> > Ack
> > 
> > > > > Patch 0004: Extend this support to the IPA Provider (even though IPA
> > > > > itself does not yet support the services map natively).
> > > > 
> > 
> > (This is actually patch 0005 in the most recent set)
> > Ack
> > 
> > > > I over-estimated the additional effort necessary to add the enumeration
> > > > support. New patch set includes the enumeration work (before the IPA
> > > > provider) as well as fixing a minor typo in patch 0003 (bad copy-paste
> > > > in a comment).
> > > 
> > > One more set. I forgot to include updates to the SSSDConfig API and the
> > > manpages in the previous set. I added it as a new, sixth patch. This
> > > should now be complete.
> > 
> > Patch 0006: I don't understand this line:
> > #replaced by ldap_entry_usn# 'ldap_service_entry_usn' : _('Service entryUSN attribute'),
> > 
> > It looks like SDAP_AT_SERVICE_USN is used in the sdap services code?
> 
> If you look at the rest of the SSSDConfig.py list, we make the same
> comment elsewhere for users, groups, etc. The reason is that there's
> never actually a reason to use the specific ones, because no LDAP server
> is going to have different entryUSN attributes for different maps.
> 
> Furthermore, we generally prefer that these options are unset, since we
> have very good auto-detection for this attribute.
> 
> So I'm leaving that as-is.
> 
> > 
> > Also "ldap_service_search_base" is missing from the manual page.
> 
> Fixed.
> 
> 
> New patches attached.

Ack to all the patches.

I noticed one more bug in the responder - for a service that has both
"tcp" and "udp" values for the "ipServiceProtocol" attribute, the first
one is always returned unconditionally even if the user specifically
asked for a protocol.

So getservbyname("genericservice", "udp"); would always retun "tcp" if
it's the first service in the cache even if "udp" exists.

We can obviously fix this post-beta, though.



More information about the sssd-devel mailing list