On 01/12/2016 01:53 PM, Pavel Březina wrote:
On 01/12/2016 01:19 PM, Sumit Bose wrote:On Mon, Jan 11, 2016 at 12:16:29PM +0100, Pavel Březina wrote:On 01/08/2016 04:37 PM, Sumit Bose wrote:On Fri, Jan 08, 2016 at 11:27:41AM +0100, Pavel Březina wrote:On 01/07/2016 02:01 PM, Sumit Bose wrote:On Tue, Jan 05, 2016 at 11:41:45AM +0100, Pavel Březina wrote:On 12/18/2015 02:36 PM, Pavel Březina wrote:Hello list, the support of IPA sudo schema is almost complete. The only thing that remains is to finish smart refresh implementation and one patch to reduce code duplication between LDAP and IPA implementation. Then I need to run some tests but I don't expect much troubles here since I tested it a lot during development. I'll finish all of this after my Christmas vacation.The patches are probably too big to be sent as an attachment, so until I complete the last two patches, you can check it on my git repo, branch sudo [1]. I don't really expect anyone to review them during Christmas break, but I thought it's a good thing to present if in case someone will get really bored from all the candies and family visits :-) Happy reviewing. [1] https://fedorapeople.org/cgit/pbrezina/public_git/sssd.git/log/?h=sudoFinal patches are attached. Happy reviewing.Here are my first comments: 0001: timeout == 0 is checked twiceFixed.+ + for (state->map_num_attrs = 0; + state->map[state->map_num_attrs].opt_name != NULL; wouldn't is be safer to check if map is not NULL before deferencinf it?Check added.Is there a reason for incrementing base_iter at the end of sdap_search_bases_next_base(), I think it would be more clear to increment it in sdap_search_bases_done() immediately before calling sdap_search_bases_next_base() again?It depends on the point of view, I guess. This way the iteration logic is all in one place which is better in my opinion.hm, I see your point. What about initializing base_iter to some magic value in sdap_search_bases_send and doing something like state->base_iter = (state->base_iter == MAGIC) ? 0 : ++state->base_iter; then state->base_iter always points to the current base and you do not need this odd base = state->bases[state->base_iter - 1] in sdap_search_bases_done(). But that's only cosmetic and personal preference, feel free to ignore it.0005: +int sysdb_compare_usn(const char *a, const char *b) +{ + if (a == NULL) { + return -1; + } + + if (b == NULL) { + return 1; + } + + /* less digits means lower number */ this is only true as long as a and b do not start with 0. I would recommend to add a if (*a != '0' && *b != '0') {I added a cycle to remove leading zeros.+ if (strlen(a) < strlen(b)) { + return -1; + } + + /* more digits means bigger number */ + if (strlen(a) > strlen(b)) { + return 1; + } + + /* now we can compare digits since alphabetical order is the same + * as numeric order */ + return strcmp(a, b); +} +errno_t sysdb_get_highest_usn(TALLOC_CTX *mem_ctx, + struct sysdb_attrs **attrs, + size_t num_attrs, + char **_usn) +{ + const char *highest = NULL; + const char *current = NULL; + char *usn; + errno_t ret; + size_t i; + + if (num_attrs == 0 || attrs == NULL) { you can save a few lines since highest == NULL with: goto done;Done.+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC ")" + I would prefer a more specific name like e.g. SUDO_ALL_FILTER.Done.0008:@@ -516,6 +516,8 @@ static void sdap_sudo_refresh_done(struct tevent_req *subreq) tevent_req_error(req, ret); } return; + } else if (ret != EOK) { + tevent_req_error(req, ret);Isn't a return missing here?Yes, fixed.0009: +static bool check_dn(struct ldb_dn *dn, + const char *rdn_attr, + va_list in_ap) +{ + const struct ldb_val *ldbval; + const char *strval; + const char *ldbattr; + const char *attr; + const char *val; + va_list ap; + int num_comp; + int comp; + + /* check RDN attribute */ + ldbattr = ldb_dn_get_rdn_name(dn); + if (ldbattr == NULL || strcasecmp(ldbattr, rdn_attr) != 0) { + return false; + } + + /* Check DN components. First we check if all attr=value pairs match input. + * Then we check that the next attribute is a domain component. + */ + + comp = 1; + num_comp = ldb_dn_get_comp_num(dn); + + va_copy(ap, in_ap); + while ((attr = va_arg(ap, const char *)) != NULL) { + val = va_arg(ap, const char *); + if (val == NULL) { + return false; + } + + if (comp > num_comp) { + return false; + } + + ldbattr = ldb_dn_get_component_name(dn, comp); + if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) { + return false; + } + + ldbval = ldb_dn_get_component_val(dn, comp); + if (ldbval == NULL) { + return false; + } + + strval = (const char *)ldbval->data; + if (strval == NULL || strncasecmp(strval, val, ldbval->length) != 0) { + return false; + } + + comp++; + } + va_end(ap); + + ldbattr = ldb_dn_get_component_name(dn, comp); + if (ldbattr == NULL || strcmp(ldbattr, "dc") != 0) { If I see it correctly the "dc" is the only thing specific to IPA (and it would work for AD as well). I would recommend to make the name of the domain component a parameter of check_dn() and not put it in ipa/ipa_dn.c but ldap/sdap_dn.c. This would avoid in the next patch to include a file from the IPA provider in libsss_ldap_common_la_SOURCES.Even though I don't like having it included in ldap_common.la, I think it is a lesser evil. You can't use it in LDAP and AD, since you can't expect any specific dn format there, or can you? Also having "dc" value a parameter and making the function more general would make the check and function call much more complicated since we would have to check for a correct ending (i.e. dc=example,dc=com or whatever is set) and not just for a presence of dc. But we can work on that if you want, I just think it is unnecessary for the given usage.ok, it makes it at least obvious that some IPA details are leaking in the general LDAP/SDAP code because of sdap_nested_group_get_ipa_user() and stays there as a reminder to fix it.+ return false; + } + + return true; +} That's all for now, more will come later.Thank you, patches are attached.I started testing and found an issues when using the compat version from ou=sudoers,dc=ipa,dc=devel and some issue if there are no command groups on the server. Please find the fixes I used in attached patch. bye, SumitHi, thank you for the fixes. I squashed them into the patch set and hope you don't mind.of course notI have changed search_bases a little, added a state->cur_base that points to the currently selected search base. This solves the -1 problem you mentioned and looks better in my opinion then using some magic value.good idea, I think with this you can even say + state->cur_base = state->bases[state->base_iter++]; and drop the state->base_iter++; line.I also moved the first change in your patch (if (num_attrs != 0)) into ipa_sudo_filter_rules_bycmdgroups as: if (num_cmdgroups == 0) { *_filter = NULL; return EOK; }ok. I'm done with my testing. I basically added different kinds of sudo related IPA objects and compared what is written to the cache with the old LDAP scheme and the new IPA scheme. Except for some obvious differences like no entryUSN with the IPA scheme and different original* attributes I found only one issue. If a rule object on the server-side has no sudoHost attribute is is still read and written to the cache by the LDAP scheme: dn: name=sudorule008,cn=sudorules,cn=custom,cn=ipa.devel,cn=sysdb cn: sudorule008 dataExpireTimestamp: 1452605137 entryUSN: 358647 name: sudorule008 objectClass: sudoRule originalDN: cn=sudorule008,ou=sudoers,dc=ipa,dc=devel sudoUser: ALL distinguishedName: name=sudorule008,cn=sudorules,cn=custom,cn=ipa.devel,cn=sys db but it is not read by the IPA scheme. Does this make a difference in the later processing by the sudo responder, i.e. will sudorule008 be available on the client with the LDAP scheme? bye, SumitHi, thank you for review. Yes, we should download even rules without host specified, nice catch. I'm attaching new patch set, the diff is: static char * ipa_sudo_host_filter(TALLOC_CTX *mem_ctx, struct ipa_hostinfo *host, struct sdap_attr_map *map) { TALLOC_CTX *tmp_ctx; char *filter; size_t i; /* If realloc fails we will free all data through tmp_ctx. */ tmp_ctx = talloc_new(NULL); if (tmp_ctx == NULL) { return NULL; } + filter = talloc_asprintf(tmp_ctx, "(!(%s=*))", + map[IPA_AT_SUDORULE_HOST].name); + if (filter == NULL) { + goto fail; + } This patch set also contains four new patches, but they are quite trivial. It actually allows setting full_refresh=0 and smart_refresh != 0 which should be supported but actually never was.
The previous tar ball got malformed. New one is attached.
patches.tgz
Description: GNU Zip compressed data
_______________________________________________ sssd-devel mailing list sssd-devel@lists.fedorahosted.org https://lists.fedorahosted.org/admin/lists/sssd-devel@lists.fedorahosted.org
