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=sudo
>>>
>>> Final patches are attached. Happy reviewing.
>>>
>>
>> Here are my first comments:
>>
>> 0001:
>> timeout == 0 is checked twice
>
> Fixed.
>
>> +
>> + 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,
Sumit
Hi,
thank you for the fixes. I squashed them into the patch set and hope you
don't mind.
I 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.
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;
}