sssd-devel August 2011

sssd-devel@lists.fedorahosted.org

16 participants
68 discussions

[PATCHES] New DEBUG facility
by Pavel Březina 25 Aug '11

25 Aug '11

https://fedorahosted.org/sssd/ticket/925 [PATCH 1/2] There are several new macros in util/util.h: - SSSDBG_* to reflect a debug level (same names as in the ticket) - please, don't use magic numbers anymore - DEBUG_MSG(level, function, message) which will format the debug message like "(time) [prg_name] [function] (level): message\n" - DEBUG_IS_SET(level) that you should use to check if the level is allowed to be logged You can use it like: if (DEBUG_IS_SET(SSSDBG_TRACE_LIBS)) {...} [PATCH 2/2] Changes unresolved debug level value (SSSDBG_UNRESOLVED) from -1 to 0 so DEBUG macro could be reduced by one condition. Anyway, it has a minor effect, every time you want to load debug_level from command line parameters, you have to use following pattern: /* Set debug level to invalid value so we can deside if -d 0 was used. */ debug_level = SSSDBG_INVALID; pc = poptGetContext(argv[0], argc, argv, long_options, 0); while((opt = poptGetNextOpt(pc)) != -1) { ... } debug_level = debug_level != SSSDBG_INVALID ? debug_convert_old_level(debug_level) : SSSDBG_UNRESOLVED; /* Debug level should be loaded from config file. */

5 16

[PATCH] Return the first value of name if the multivalued name attribute does not match RDN
by Jakub Hrozek 25 Aug '11

25 Aug '11

https://fedorahosted.org/sssd/ticket/926 There's one more branch where the RDN attribute name matches the naming attribute name but value is not found in the naming attribute values. I didn't touch that one because I wanted to loosen the rules as little as possible.

3 4

[PATCH] IPA dyndns: do not segfault if the server cannot be resolved
by Jakub Hrozek 25 Aug '11

25 Aug '11

https://fedorahosted.org/sssd/ticket/963

3 3

[PATCH] Improve password policy error code and message
by Sumit Bose 25 Aug '11

25 Aug '11

Hi, if we request e.g. the shadow password policy but a user does not have the corresponding attributes the following log message is shown: (Thu Aug 25 06:22:59 2011) [sssd[be[LDAP]]] [find_password_expiration_attributes] (1): No shadow password attributes found, but shadow password policy was requested. (Thu Aug 25 06:22:59 2011) [sssd[be[LDAP]]] [get_user_dn] (1): find_password_expiration_attributes failed. (Thu Aug 25 06:22:59 2011) [sssd[be[LDAP]]] [sdap_handle_release] (8): Trace: sh[0x241b610], connected[1], ops[(nil)], ldap[0x241e270], destructor_lock[0], release_memory[0] (Thu Aug 25 06:22:59 2011) [sssd[be[LDAP]]] [remove_connection_callback] (9): Successfully removed connection callback. (Thu Aug 25 06:22:59 2011) [sssd[be[LDAP]]] [be_pam_handler_callback] (4): Backend returned: (3, 4, <NULL>) [Internal Error (System error)] Kaushik mentioned that "Backend returned: (3, 4, <NULL>) [Internal Error (System error)]" might irritate the admin who tries to find out why the access was denied. The attached patch changes this to: (Thu Aug 25 13:06:15 2011) [sssd[be[ldap]]] [find_password_expiration_attributes] (1): No shadow password attributes found, but shadow password policy was requested. Access will be denied. (Thu Aug 25 13:06:15 2011) [sssd[be[ldap]]] [get_user_dn] (1): find_password_expiration_attributes failed. (Thu Aug 25 13:06:15 2011) [sssd[be[ldap]]] [sdap_handle_release] (8): Trace: sh[0x87d3b28], connected[1], ops[(nil)], ldap[0x87d4a58], destructor_lock[0], release_memory[0] (Thu Aug 25 13:06:15 2011) [sssd[be[ldap]]] [remove_connection_callback] (9): Successfully removed connection callback. (Thu Aug 25 13:06:15 2011) [sssd[be[ldap]]] [be_pam_handler_callback] (4): Backend returned: (0, 6, <NULL>) [Success] bye, Sumit

2 2

sssd & autofs
by Ondrej Valousek 19 Aug '11

19 Aug '11

Hi list, Quick question - is anyone working on the automounter support yet? I am just curious how it would be done. I see only one ticket (#900) which suggests support via executable maps, but this solution seems to be quite nasty. I believe that ideal solution would be via plugin library (something like /usr/lib/autofs/lookup_sssd.so) to the automounter - I am sure Ian Kent (the autofs maintainer) would help as well. What do you think? Thanks, Ondrej The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s). Please direct any additional queries to: communications(a)s3group.com. Thank You. Silicon and Software Systems Limited (S3 Group). Registered in Ireland no. 378073. Registered Office: South County Business Park, Leopardstown, Dublin 18

3 3

better handling of DNS timeouts
by Jakub Hrozek 19 Aug '11

19 Aug '11

Hi, In light of https://bugzilla.redhat.com/show_bug.cgi?id=726467 I have been thinking about improving how we handle DNS timeouts in general. Currently there is one timeout option we pass to c-ares. However, that option is per-nameserver. I guess that makes sense from a resolver library POV - as a resolver library you want to control how long you talk to each name server. We have been mostly OK with this because in most situations the resolver can't connect the socket to the name server at all at times out immediatelly. Problems arise when the server is very slow to respond, drops packets. To solve this in SSSD we need to have control over how long a name resolution takes regardless of the number of name servers and also regardless of the number of servers in failover. The failover in SSSD has the concept of "services". LDAP is a service, Kerberos is a service etc. From back end you don't care and don't know how many servers there are in a service. From fail over we don't know and don't care how many name servers there are. My proposal is to: 1) change the current "dns_resolver_timeout" to be per-service, so the semantics would be "How long to wait until we get an LDAP server IP address" for example 2) introduce a new option, something like "dns_resolver_server_timeout", that would control a per-server timeout. This option could maybe be undocumented, it seems quite low-level. 3) hide the per-nameserver resolver timeout (only #define it). It is too low level. Thoughts, comments and ideas are welcome.

5 14

sssd issues
by Ondrej Valousek 18 Aug '11

18 Aug '11

Hi list, Ok here is the list of issues I discovered while configuring sssd against Win2008 AD & rfc2307bis schema: 1. If I specify both dns_discovery_domain and ldap_uri parameters then what happens is that dns srv discovery returns a list of ldap servers. Now if the first one found is not working, others are not tried. I have to comment out the 'ldap_uri' parameter to make it working correctly. 2. SSSD is unable to detect default Kerberos realm as per /etc/krb5.conf - I have to configure it manually 3. Why do we actually need to specify Kerberos realm and KDC? Isn't /etc/krb5.conf supposed to record these kind of parameters? 4. authconfig is unable to configure sssd to use IPA backend provider Thanks, Ondrej The information contained in this e-mail and in any attachments is confidential and is designated solely for the attention of the intended recipient(s). If you are not an intended recipient, you must not use, disclose, copy, distribute or retain this e-mail or any part thereof. If you have received this e-mail in error, please notify the sender by return e-mail and delete all copies of this e-mail from your computer system(s). Please direct any additional queries to: communications(a)s3group.com. Thank You. Silicon and Software Systems Limited (S3 Group). Registered in Ireland no. 378073. Registered Office: South County Business Park, Leopardstown, Dublin 18

4 5

Re: [SSSD] THE SUDO rule elimination is over.
by Stephen Gallagher 16 Aug '11

16 Aug '11

For those following along, this is a review of the patches in Arun's public git repository located at: https://github.com/arunscaria91/sssd.git On Tue, 2011-08-16 at 22:05 +0530, arun scaria wrote: > > Dear sgallagh, > I'm happy to inform you that the sudo rule elimination is over, Now I > need to evaluate left out rules as we planned. Check my repo and > comment through mail. If you can, inform the next meeting time. :) I have a few review comments to make. Sorry that some of these probably should have happened earlier, but this is the first chance I've had to go through this in-depth. sudo_query_validation(): 1) This function is getting far too large. Please separate the D-BUS message parsing into a separate function that allocates and returns 'struct sss_sudo_msg_contents'. It should look like: sudo_query_parse(TALLOC_CTX *mem_ctx, struct DBusMessage *message, struct sss_sudo_msg_contents **contents); and would be invoked like: sudo_query_parse(sudo_cli, message, &msg); Please also separate the reply message creation into its own function, in such a way that it can be called for success or failure. Another point: do not explicitly cast to (TALLOC_CTX *). Internally, it's just an alias for 'void *' and is safe to implicitly cast everywhere. Adding the explicit cast just makes the code harder to read. Please don't use GOTO except to an exit condition. Your use of 'free_ctx' is completely unacceptable. We only ever use GOTO if we're short-circuiting to exit on failure. As mentioned above, please create a function to send the reply and call it explicitly for either success or user not found. Please always test the returned values from ldb_msg_find_attr_*(), because if the message did not contain the value, we need to consider it an error and return a failure reply. You still have "tom" passed into search_sudo_rules(), where it should be user_name. Please don't use malloc() or strdup() directly (as with "PASS"). Use talloc() and talloc_strdup() instead. search_sudo_rules(): Do not allocate tmp_ctx on sudocli. In a synchronous function, tmp_ctx contexts should be allocated on NULL and guaranteed to be maintained internally. This means that you must always talloc_free(tmp_ctx) before any return from the function. This provides an easy way to clean up any internal-only memory, but it should NOT be attached to an external context. If it's attached to an external context and we forget to free it before returning in some situations, not only is it a leak, but it's a leak we cannot detect with tools like valgrind. Furthermore, functions should not attach to an external context (such as sudocli) directly. If they are going to allocate memory that is to be returned, the function MUST take a mem_ctx argument (then the caller of the function will decide whether sudocli is the appropriate parent for it). Don't allocate part of 'filter' first and then call prepare_filter(). You should just pass in tmpctx and user_name to prepare_filter and create it all in that function. Don't allocate listctx on NULL. You have a tmpctx already; this is exactly what it's supposed to be for (so you only have to worry about calling talloc_free(tmpctx) before returning). Please don't write your own linked list method. We have our own implementation already in the SSSD sources. See src/util/dlinklist.h. You probably want to be using DLIST_FOREACH() in place of your while() loop. Your functions initList(), appendNode() etc violate our naming standards. There are a bunch of things wrong with list_sss. 1) Do not use typedefs. 2) Don't try to create generic lists. Just create a 'struct sss_sudo_list' and use the real 'struct ldb_message *" instead of casting back and forth to (void *). 3) sss_sudo_list should have the prev and next pointers so it can be used with the DLIST macros. The comment /* see if this is a user */ is in the wrong place. You're comparing the command here. Please make each of the individual comparisons (command, user, host) into their own functions. search_sudo_rules() is too large and hard to follow. (Also, add comments) We need to figure out an alternative to the _SSS_LOOPS issue here (but this isn't your problem, Arun). We have no choice but to call into glibc if we want innetgr() to resolve properly. This comment is added so that if anyone besides Arun has read this far, they should feel free to comment :) For users, commands and hosts you need to check for negation "!" rules. As you're walking through each, if you hit an explicit denial at any time, you should short-circuit to returning PERMISSION DENIED. On a related note, this function needs to return the results necessary to reply to the client. You probably need to construct this as a struct that you'll pass back to sudo_query_validation() which will in turn pass it into the reply creation routine I mentioned above. get_host_name(): Please use "mem_ctx" to make things clear. 'ctx' is too ambiguous and we're trying to phase it out elsewhere in the code. Do not call exit(). EVER. It is much better to have the function return an errno_t and instead return the hostname as a "char **hostname" parameter to the function. This function needs comments *desperately*. I really don't have any idea what you're trying to do, but I'm sure you aren't doing it. Right now, you're returning a copy of a member of the "p" variable that has never been initialized. Also, this should just be talloc_strdup(mem_ctx, p->ai_canonname); I don't really know why you're calling getaddrinfo() for the "http" service either. prepare_filter(): See comment above about passing in user_name. You also need to handle looking up the explicit negations. So your search filter needs to include "sudoUser=!username", "sudoUser=!#gid" and "sudoUser=!+*" (same for hosts). We need to make sure that if any rule specifically denies this user or host, it is honored. Change the name of the variable "res" once it's passed in. It would be more clear if it was named "groups_res". As I said above, you can't assume that ldb_msg_find_* will return non-NULL. You need to check this before you can pass it into talloc_asprintf_append(). If it's NULL, log an error with DEBUG() and skip it. You use a lot of repeated code when checking the filter pieces. Might be a good idea to just create a macro like FILTER_APPEND_CHECK(). (This is just a suggestion; there's nothing wrong with the repetition other than it takes up more space). However, you're checking whether filter is NULL and then attempting to print it in the DEBUG statements. That's not going to work :) sysdb_search_sudo_rules(): As described above, tmpctx should be allocated on NULL, not mem_ctx. Only the values being returned should ever be allocated or stolen with talloc_steal() onto the passed-in mem_ctx. compare_sudo_order(): Correct me if I'm wrong, but won't this be sorting from lowest to highest? Doesn't sudo require us to process the higher values first?

1 0

[PATCH] Handle timeout during sss_ldap_init_send
by Jakub Hrozek 15 Aug '11

15 Aug '11

In some cases, where there would be no response from the LDAP server, there would be no R/W events on the LDAP fd, so sdap_async_sys_connect_done would never be called. This patch adds a tevent timer that cancels the connection after SDAP_NETWORK_TIMEOUT seconds. A simple testcase is enabling DROP iptables rules on the LDAP server.

2 2

[PATCH] Do not delete requests inside hash_iterate loop
by Jakub Hrozek 15 Aug '11

15 Aug '11

This patch should solve #932/rhbz #725472, although I have not been able to reproduce the original crash, so I'm not sure. But it's definitely something we want to fix as deleting a hash table item from a hash_iterate visitor callback yields potentially undefined results.

2 1

2024

2023

2022

2021

2020

2019

2018

2017

2016

2015

2014

2013

2012

2011

2010

2009

sssd-devel August 2011