On 03/07/2016 01:11 PM, Pavel Březina wrote:
> On 03/02/2016 05:04 PM, Petr Cech wrote:
>> Hi all,
>>
>> attached two patches resolve [1]. This ticket has design page [2].
>>
>> In my opinion it could be fine to have tests on sysdb_sudo. I have
>> started write some, but there were troubles with memory leak. Maybe I
>> haven't understood necessary logic properly.
>>
>> However I could continue with tests or create new ticket for it.
>>
>> And little question. Is the name of sudo rule case sensitive? If yes, I
>> have to do one little change.
>
> I think we are good if we use domain settings so no change is needed.
>
>>
>> [1]
https://fedorahosted.org/sssd/ticket/2081
>> [2]
>>
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRulesInvalidate
>>
>> Regards
>
> Hi,
> see comments inline.
>
>> errno_t sysdb_search_sudo_rules(TALLOC_CTX *mem_ctx,
>> struct sss_domain_info *domain,
>> const char *sub_filter,
>> const char **attrs,
>> size_t *msgs_count,
>> struct ldb_message ***msgs)
>> {
>> TALLOC_CTX *tmp_ctx;
>> struct ldb_dn *dn;
>> char *filter;
>> int ret;
>>
>> tmp_ctx = talloc_new(NULL);
>> if (!tmp_ctx) {
>> return ENOMEM;
>> }
>>
>> dn = ldb_dn_new_fmt(tmp_ctx, domain->sysdb->ldb,
>> SYSDB_TMPL_CUSTOM_SUBTREE,
>> SUDORULE_SUBDIR, domain->name);
>> if (!dn) {
>> DEBUG(SSSDBG_OP_FAILURE, "Failed to build base dn\n");
>> ret = ENOMEM;
>> goto fail;
>> }
>>
>> filter = talloc_asprintf(tmp_ctx, "(&%s%s)", SUDO_ALL_FILTER,
>> sub_filter);
>
> You also need to add case where sub_filter is NULL.
>
>> if (!filter) {
>> DEBUG(SSSDBG_OP_FAILURE, "Failed to build filter\n");
>> ret = ENOMEM;
>> goto fail;
>> }
>>
>> DEBUG(SSSDBG_TRACE_INTERNAL,
>> "Search services with filter: %s\n", filter);
>
> sudo rule, not "services"
>
>>
>> ret = sysdb_search_entry(mem_ctx, domain->sysdb, dn,
>> LDB_SCOPE_SUBTREE, filter, attrs,
>> msgs_count, msgs);
>> if (ret) {
>> goto fail;
>> }
>>
>> talloc_zfree(tmp_ctx);
>> return EOK;
>>
>> fail:
>> if (ret == ENOENT) {
>> DEBUG(SSSDBG_TRACE_INTERNAL, "No such entry\n");
>> }
>> else if (ret) {
>> DEBUG(SSSDBG_MINOR_FAILURE, "Error: %d (%s)\n", ret,
>> strerror(ret));
>> }
>> talloc_zfree(tmp_ctx);
>
> There is already good debug message per dn and filter case. Move this to
> sysdb_search_entry and use done scheme instead of fail, please.
>
>> return ret;
>> }
Also atm -E won't trigger invalidation of sudo rules which I think is
also desired. Instead of modifying the current #ifdefs thingy, I'd
suggest using |= operator in init_context. I.e.:
case 'e':
idb = INVALIDATE_EVERYTHING;
idb |= SUDO...
break;
There are new fixed patch set. We discussed offline and all comments are
addressed.
--
Petr^4 Čech