sssd-devel February 2012

sssd-devel@lists.fedorahosted.org

19 participants
122 discussions

Re: [SSSD] [PATCH] autofs integration
by Jakub Hrozek 06 Feb '12

06 Feb '12

On Sun, Feb 05, 2012 at 11:42:31PM +0100, Jakub Hrozek wrote: > On Sun, Feb 05, 2012 at 12:04:39PM -0500, Stephen Gallagher wrote: > > I discovered a few other issues with the earlier patches, so don't just > > jump to 0009 and 0010 :) > > > > Comments inline > > > > > > On Sat, 2012-02-04 at 21:42 -0500, Stephen Gallagher wrote: > > > On Fri, 2012-02-03 at 16:46 +0100, Jakub Hrozek wrote: > > > > Hi, > > > > > > > > attached is a series of patches that implements autofs support. In order > > > > to test it, you can either use a provided command-line client or patch > > > > autofs and test directly using automounter. > > > > > > > > I have patched the rawhide version of automounter and put it into my > > > > fedorapeople account until it lands in Fedora: > > > > http://jhrozek.fedorapeople.org/autofs-sssd/ > > > > > > > > Alternatively, you can apply the patches directly onto autofs-5.0.6: > > > > http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-sss-lookup-module.… > > > > http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-teach-automount-ab… > > > > > > > > The patches are functionally complete with the single exception of the > > > > IPA provider. I will finish it during today. To test with an IPA server, > > > > you need to set "autofs_provider = ldap" manually. > > > > > > > > There is a bug in the responder where we don't properly clean up after > > > > setent, but I don't think I should hold off sending the patches for review. > > > > The responder also uses the deprecated cache checking logic (see patch > > > > 0008). I will either finish that before 1.7.9 or during 1.8 development. > > > > The responder also does not perform an implicit setautomntent() yet. A > > > > fix for none of these issues would need a string freeze break. > > > > > > > > The patches are based on the sudo patches on-list and Stephen's pending > > > > cache timeout patch and must be applied on top of them. > > > > > > > > [PATCH 01/10] BUILD: Introduce a --with-autofs config option > > > > This would allow to select the autofs feature during build without > > > > having to select the other features. > > > > > > > > > > Ack > > > > > I fixed the issue that was causing the RHEL5 builds to fail. > > > > > [PATCH 02/10] SYSDB: Remove code duplication between member_add and member_del > > > > Refactoring patch that will be used in the next one. > > > > > > > > > > Ack > > > > > > > [PATCH 03/10] AUTOFS: sysdb interface > > > > The cache interface. > > > > > > > > > > Nack (minor). > > > > > > sysdb_autofsmap_strdn() should be static. It's not used outside of > > > sysdb_autofs.c > > > > > > > sysdb_get_map_byname() needs to sss_filter_sanitize() the map_name. > > Doing this will also have repercussions in the LDAP provider, which I'll > > cover in 0010. > > > > Both are done. > > > > > > > > > > > [PATCH 04/10] AUTOFS: a client library > > > > This is the library the autofs client is using. automounter dlopen()s > > > > the library so there is no header file, no pkgconfig file and the > > > > library is in the libsss_autofs package, not in -devel. > > > > > > > > The library provides the following interface that translates into > > > > responder interface in the next patch: > > > > * _sss_setautomntent() - select the map for processing > > > > * _sss_getautomntent_r() - iterates through key/value pairs in the > > > > selected map. The key is usually the mount > > > > point, the value is mount information > > > > (server:/export) > > > > * _sss_getautomntbyname_r() - returns value for a specific key. > > > > * _sss_endautomntent() deselect a map, clean up > > > > > > > > As you can see, the interface closely resembles netgroup interface glibc > > > > uses, so I was able to reuse many concepts from the netgroup code. > > > > > > > > > > Nack. > > > > > > The autofs plugin library should not be versioned. Autofs isn't linking > > > against it, so there's no benefit to including version information. In > > > the case of plugins, the interface is defined by the application calling > > > dlopen(), not the library. This is how we handle our libsss_*.so > > > libraries too, as well as the krb5 locator plugin (if you need an > > > example of a library we're exposing to an external application) > > > > > OK, the current version uses --avoid-version > > > > The .exports file has indentation issues and also includes some tabs > > > instead of spaces. > > > > > Fixed > > > > While it's probably too late now to change the public interface (since > > > the autofs folks are working against this), I do wish we'd stuck with > > > our policy of function arguments being inputs followed by outputs. I > > > don't like how the "context" argument is last (except in > > > _sss_setautomntent(), of course). > > > > > > > > In general I agree with the sentiment, but this is an autofs oddity, > they just pass the context as the last parameter in all the lookups > modules. > > We decided to keep it that way so that the interface is consistent in > all autofs lookup modules. > > > > Potential memory leak in _sss_setautomntent(): if the strdup(name) > > > fails, you're not freeing "name" before exiting. > > > > > fixed > > > > Use the safealign_memcpy() from util.h for copying the mapname into the > > > request buffer in _sss_getautomntent_r() (mostly for consistency. It's > > > identical code to what you wrote). > > > > > fixed > > > > General architecture question: Why don't we just store the result data > > > in the "struct automtent" when we complete the _sss_setautomntent() > > > routine? Granted, we'd be carrying around more memory in autofs, but it > > > would be accessible quite a bit faster than going back and forth to the > > > responder for multiple _sss_getautomntbyname_r() calls. At the very > > > minimum, it would be better to replicate what we're doing in the NSS > > > passwd client, where we get SSS_NSS_MAX_ENTRIES at a time. I can see > > > this being a bottleneck for a system looking up user home directories > > > for thousands of users. > > > > > > That said, this would be a performance enhancement and I'd be fine with > > > modifying this after feature freeze. (The public interface will remain > > > the same). > > > > > > > Additional comment about this. It's not sufficient to have the > > endautomntent() value not go to the responder. If the responder is > > allocating memory and maintaining a view of the data for this request > > series, we need to be able to tell it to release it when we're done. > > > > (The responder will also need to implement a timer event to free memory > > after a certain amount of time, so we don't have a memory leak problem > > if the calling function doesn't invoke endautomntent()) > > > > I filed https://fedorahosted.org/sssd/ticket/1166 > > I'll try to make the beta deadline, but if I won't make it, we can fix > this post-beta. > > > > > > > > > > > > > > > > > [PATCH 05/10] AUTOFS: a command-line test client > > > > A very simply binary that can be used to test getting data from the > > > > library via SSSD in pretty much the same way SSSD would. A required > > > > positional parameter specifies the map name and the tool would print out > > > > all the key/value pairs using _sss_getautomntent_r(). You can also > > > > specify -n to query a specific key using _sss_getautomntbyname_r(). > > > > > > > > There is one gotcha in the code - the library location is hardcoded to > > > > ".libs". Is there a way to get this from libtool? > > > > > > Is there a really good reason not to just create a private header for > > > the exposed functions and just link directly against it? I think that's > > > perfectly acceptable for a test tool. Sure, you're not testing that it > > > dlopen()s properly, but that's pretty safe to assume that if it links it > > > can be dlopen()ed. > > > > > I was just trying to test that dlopen()ing the library works so there > would be no surprises for Ian. The current version has a private header > (that would also warn if the interface changed by a chance) and links to > this header. > > > > Otherwise, Ack. > > > > > > (And no, I don't know off-hand any way to get libtool to report the > > > default output location of libraries) > > > > > > > > > > > [PATCH 06/10] AUTOFS: Data Provider request > > > > Implements the request that the responder might use to communicate with > > > > the back end. > > > > > > > > > > Nack. > > > > > > Please use the new DEBUG level macros. > > > > > > Functionally, the DP request looks good. > > > > > Done. I converted some macros that were not introduced in these patches > (mainly in be_client_destructor and be_process_init) as well so that the > debug levels are consistent inside a single function. > > > > > [PATCH 07/10] Refactor setent_req_list > > > > Makes the setent_add_ref() and setent_notify_*() functions more generic > > > > to be reusable by the autofs responder. > > > > > > > > > > Ack > > > > > > > [PATCH 08/10] Split the logic to check cache expiration into separate function > > > > The autofs responder can't reuse the check_cache function, so I split > > > > the cache expiration check logic into a separate function instead. > > > > As mentioned earlier, this patch might not be needed if I can finish the > > > > rewrite to the new cache check logic. > > > > > > > > > > Ack > > > > > > > [PATCH 09/10] AUTOFS: responder > > > > The responder process. The patch seems big, but many parts are just > > > > generic responder plumbing. > > > > > > > > > > I will review this tomorrow. > > > > > > > Nack > > Typo in the function name: sss_autofs_cmd_getautomnbyname (Should be > > sss_autofs_cmd_getautomntbyname) > > > > Fixed. > > > As mentioned above, you need to have an endautomntent() entry point into > > the responder, to do memory cleanup. Look at how endnetgrent() works in > > nsssrv_netgroup.c. > > > > Done although the endautomntent is just a stub until #1166 is > implemented. > > > I'm okay with deferring the fix for the missing implicit setautomntent() > > call to post-beta, since we really only have one consumer for this > > (autofs), so our exposure is low. Just open a ticket. > > > > https://fedorahosted.org/sssd/ticket/1167 > > > When you register against a result object that's already being > > processed, you have a comment that says: > > /* Will return control below */ > > But following it through, it falls into fail: and calls talloc_free(req) > > and returns NULL. > > > > That was supposed to say "return req" before the fail label, thanks. > > > > > This doesn't need to be done for this version of the patch, but please > > open a ticket to convert lookup_automntmap_step() and friends to the new > > cache lookup approach I developed for the service patches. Among other > > things, it will eliminate the need for the ugly EAGAIN return (and it > > will behave better for multiple domains). > > > > There already is https://fedorahosted.org/sssd/ticket/843 which is > tracking conversion to the new logic so I just added a comment there. > > > Maybe I'm not seeing it, but if an unexpected error occurs anywhere > > during the setautomntent_send() processing, we never release the > > in-construction map entry in the cache. I think that's going to break > > subsequent lookups for that same map (it will never reply). > > I'm sorry, I don't quite understand. I only saw a couple of cases where > we wouldn't remove the map (state->map of struct autofs_map_ctx *) if an > operation failed after we set the map into the hash. Is that what you > meant? > > > > > As previously-stated, I'd like to see us return larger chunks to the > > client at a time, so we're not going across the pipe for every entry. > > But this can be done post-beta. > > > > Yes, that's being tracked. > > > In sss_autofs_cmd_getautomntent() you probably only want to be checking > > the UTF-8 status of the 'name', not the complete body. The namelen > > portion of the buffer could break this. Same for > > sss_autofs_cmd_getautomnbyname() for both name and key. > > > > Fixed. > > > > > > > > > [PATCH 10/10] AUTOFS: LDAP provder > > > > The whole LDAP provider in a single patch. One place that might need > > > > improving is removing a map from an entry. The entry might become > > > > orphaned when no maps link to it and the cache might grow. I will > > > > improve the patch by searching for entries with no memberof: links and > > > > deleting them after the save. > > > > > > > > There is neither enumerate task nor any periodic download. I'm not quite > > > > sure it is needed. I'll ask Ian about that. > > > > > > > > > > I will review this tomorrow. > > > > Nack. > > > > Nitpick: please remove the tabs you added in > > +if BUILD_AUTOFS > > +libsss_ldap_la_SOURCES += src/providers/ldap/sdap_autofs.c \ > > + src/providers/ldap/sdap_async_autofs.c > > +endif > > > > and > > > > +if BUILD_AUTOFS > > +libsss_ipa_la_SOURCES += src/providers/ldap/sdap_autofs.c \ > > + src/providers/ldap/sdap_async_autofs.c > > +endif > > > > > > Fixed > > > Spot the copy-paste error ;-) > > +#ifdef BUILD_AUTOFS > > +/* sudo */ > > +void sdap_autofs_handler(struct be_req *be_req); > > +#endif > > > > It's actually not necessary to have the autofs handler in a header file, > I completely removed the declaration. > > > sdap_autofs_setautomntent_send() doesn't handle a NULL mapname properly > > (which you expressly allow in sdap_autofs_handler()). > > > > Done. > > > Don't store the filter-sanitized version of the mapname in struct > > sdap_autofs_setautomntent_state(). Keep the original version around and > > sanitize it for filters where appropriate. Otherwise, you may end up > > double-filtering if you pass it into functions that do this for you, > > like sysdb_get_map_byname() should be doing (see above). > > > > Done. > > > In sdap_get_automntmap_process() you throw an error if a search returns > > more than one map. I assume this is because we're not supporting > > enumeration, but it's inconsistent with allowing NULL/<ALL> in > > sdap_autofs_setautomntent_send(). Please pick one and make it > > consistent. > > > > Fixed by disallowing NULL names in sdap_autofs_setautomntent_send(). We > can extend the whole logic to handle enumerations should it turn out > that autofs client needs it. > > > automntmaps_process_members_send() should test whether the requested > > entry is present in ANY of the search bases (not just the current one). > > This can be fixed post-beta, please open a ticket. > > > > https://fedorahosted.org/sssd/ticket/1168 > > > I think the comment in sdap_autofs_setautomntent_save() when receiving > > an error from sysdb_get_map_byname() is wrong and misleading. > > > > Done. > > > Otherwise, I think the logic looks good. Great job! Attached is a tarball that contains one more change in the configAPI.

2 1

Info on [services/dp] section
by Marco Pizzoli 05 Feb '12

05 Feb '12

Hi, I'm curious to know about the section [services/dp]. I found it documented here: http://www.digipedia.pl/man/doc/view/sssd.conf.5/ but not in the man page installed on my system. I'm using sssd-1.7.0-5.fc16.x86_64 . Is that directive a rest from the past? Thanks Marco -- _________________________________________ Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi. Jim Morrison

2 2

[PATCH] autofs integration
by Jakub Hrozek 05 Feb '12

05 Feb '12

Hi, attached is a series of patches that implements autofs support. In order to test it, you can either use a provided command-line client or patch autofs and test directly using automounter. I have patched the rawhide version of automounter and put it into my fedorapeople account until it lands in Fedora: http://jhrozek.fedorapeople.org/autofs-sssd/ Alternatively, you can apply the patches directly onto autofs-5.0.6: http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-sss-lookup-module.… http://jhrozek.fedorapeople.org/autofs-sssd/autofs-5.0.6-teach-automount-ab… The patches are functionally complete with the single exception of the IPA provider. I will finish it during today. To test with an IPA server, you need to set "autofs_provider = ldap" manually. There is a bug in the responder where we don't properly clean up after setent, but I don't think I should hold off sending the patches for review. The responder also uses the deprecated cache checking logic (see patch 0008). I will either finish that before 1.7.9 or during 1.8 development. The responder also does not perform an implicit setautomntent() yet. A fix for none of these issues would need a string freeze break. The patches are based on the sudo patches on-list and Stephen's pending cache timeout patch and must be applied on top of them. [PATCH 01/10] BUILD: Introduce a --with-autofs config option This would allow to select the autofs feature during build without having to select the other features. [PATCH 02/10] SYSDB: Remove code duplication between member_add and member_del Refactoring patch that will be used in the next one. [PATCH 03/10] AUTOFS: sysdb interface The cache interface. [PATCH 04/10] AUTOFS: a client library This is the library the autofs client is using. automounter dlopen()s the library so there is no header file, no pkgconfig file and the library is in the libsss_autofs package, not in -devel. The library provides the following interface that translates into responder interface in the next patch: * _sss_setautomntent() - select the map for processing * _sss_getautomntent_r() - iterates through key/value pairs in the selected map. The key is usually the mount point, the value is mount information (server:/export) * _sss_getautomntbyname_r() - returns value for a specific key. * _sss_endautomntent() deselect a map, clean up As you can see, the interface closely resembles netgroup interface glibc uses, so I was able to reuse many concepts from the netgroup code. [PATCH 05/10] AUTOFS: a command-line test client A very simply binary that can be used to test getting data from the library via SSSD in pretty much the same way SSSD would. A required positional parameter specifies the map name and the tool would print out all the key/value pairs using _sss_getautomntent_r(). You can also specify -n to query a specific key using _sss_getautomntbyname_r(). There is one gotcha in the code - the library location is hardcoded to ".libs". Is there a way to get this from libtool? [PATCH 06/10] AUTOFS: Data Provider request Implements the request that the responder might use to communicate with the back end. [PATCH 07/10] Refactor setent_req_list Makes the setent_add_ref() and setent_notify_*() functions more generic to be reusable by the autofs responder. [PATCH 08/10] Split the logic to check cache expiration into separate function The autofs responder can't reuse the check_cache function, so I split the cache expiration check logic into a separate function instead. As mentioned earlier, this patch might not be needed if I can finish the rewrite to the new cache check logic. [PATCH 09/10] AUTOFS: responder The responder process. The patch seems big, but many parts are just generic responder plumbing. [PATCH 10/10] AUTOFS: LDAP provder The whole LDAP provider in a single patch. One place that might need improving is removing a map from an entry. The entry might become orphaned when no maps link to it and the cache might grow. I will improve the patch by searching for entries with no memberof: links and deleting them after the save. There is neither enumerate task nor any periodic download. I'm not quite sure it is needed. I'll ask Ian about that. Happy reviewing.

2 2

Split sssd.conf file in N file?
by Marco Pizzoli 05 Feb '12

05 Feb '12

Hi guys, is there a way to split the sssd.conf file in N sub-files and "include" them in the original sssd.conf? In a nutshell, is there a sort of "include" directive I can use to include fragments of configuration from other files? Thanks Marco -- _________________________________________ Non è forte chi non cade, ma chi cadendo ha la forza di rialzarsi. Jim Morrison

2 2

[PATCH] SUDO: Introduce a new configure option --with-sudo
by Jakub Hrozek 04 Feb '12

04 Feb '12

We will want to set this option (but perhaps not --enable-all-experimental-features) for F17 to be able to build the sudo library. https://fedorahosted.org/sssd/ticket/1145 At the time being the option is also turned on when --enable-all-experimental-features is specified. The second patch does the refactoring mentioned in #1145 - just moves code around so that there are no #ifdefs in the main part of LDAP code.

2 10

[PATCH] SUDO Integration - in-memory cache in responder
by Jakub Hrozek 04 Feb '12

04 Feb '12

The attached patch implements a fast in-memory cache for serving sudo requests. Most of the code was written by Pavel, so I retained the attribution even though I made a couple of fixes and wrote the man page section. https://fedorahosted.org/sssd/ticket/1111

2 4

[PATCH] LDAP: Do not fail if RootDSE check cannot determine search bases
by Stephen Gallagher 04 Feb '12

04 Feb '12

Fixes https://fedorahosted.org/sssd/ticket/1152 I just made the get_naming_context() routine non-fatal and added NULL checks for the search bases in each of the routines that use them. So if SOME features have search bases available, they'll work. This will ease issues on upgrade. Note: some other patches in development (such as AutoFS and SELinux) will probably need to be updated to include these NULL checks as well.

3 5

[PATCH] NSS: Add individual timeouts for entry types
by Stephen Gallagher 04 Feb '12

04 Feb '12

Sorry for breaking the thread, but my email server seems to have lost track of the original emails (they're not showing up in my folders). Jakub, thanks for catching the ioa_common.c issue. Fixed. I didn't bother differentiating the messages in the API because they're A) unimportant and B) not sufficiently different to be worth adding new translatable strings for. I did fix the manpages though. Those SHOULD be documented. New patch attached. Also, this is designed to apply atop my patches for the RootDSE/search base fix (which still needs reviewing). See "[PATCH] LDAP: Do not fail if RootDSE check cannot determine search bases"

3 5

[PATCH] NSS: Use sss_hash_create instead of destructor
by Jakub Hrozek 04 Feb '12

04 Feb '12

The hash table that is used to cache netgroups is allocated with low-level hash_create() and the freed in a destructor. The entries themselves are talloc pointers with destructors which results in a funky behaviour during shutdown, often a segfault. I think the correct thing to do is just use sss_hash_create() so that the table is deallocated when the responder goes away.

3 2

[PATCH] Add SUDO provider to the list of available providers in SSSDConfig.py
by Jan Zelený 04 Feb '12

04 Feb '12

I checked and the sudo provider is on other places it should be, it was just left out from this place. Jan

3 2

Jump to page:

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

sssd-devel February 2012