[PATCHES] Support IPA sudo schema

[Pavel Březina]

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

[Pavel Březina]

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.

[Sumit Bose]

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:

sdap_search_bases_send():

+    if (timeout == 0) {
+        timeout = dp_opt_get_int(opts->basic, SDAP_SEARCH_TIMEOUT);
+    }

see below

+
+    if (bases == NULL) {
+        DEBUG(SSSDBG_CRIT_FAILURE, "No search base specified!\n");
+        ret = ERR_INTERNAL;
+        goto immediately;
+    }
+
+    state->ev = ev;
+    state->opts = opts;
+    state->sh = sh;
+    state->bases = bases;
+    state->map = map;
+    state->filter = filter;
+    state->attrs = attrs;
+    state->allow_paging = allow_paging;
+
+    state->timeout = timeout == 0
+                     ? dp_opt_get_int(opts->basic, SDAP_SEARCH_TIMEOUT)
+                     : timeout;

timeout == 0 is checked twice
+
+    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?

+            state->map_num_attrs++) {
+        /* no op */;
+    }
+
+    if (state->attrs == NULL) {
+        ret = build_attrs_from_map(state, state->map, state->map_num_attrs,
+                                   NULL, &state->attrs, NULL);
+        if (ret != EOK) {
+            DEBUG(SSSDBG_OP_FAILURE, "Unable to build attrs from map "
+                  "[%d]: %s\n", ret, sss_strerror(ret));
+            goto immediately;
+        }
+    }

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?

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') {

+    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;

+        usn = talloc_strdup(mem_ctx, "0");
+        if (usn == NULL) {
+            return ENOMEM;
+        }
+
+        *_usn = usn;
+        return EOK;
+    }
+
+    for (i = 0; i < num_attrs; i++) {
+        ret = sysdb_attrs_get_string(attrs[i], SYSDB_USN, &current);
+        if (ret == ENOENT) {
+            /* USN value is not present, assuming zero. */
+            current = "0";
+        } else if (ret != EOK) {
+            DEBUG(SSSDBG_MINOR_FAILURE, "Failed to retrieve USN value "
+                  "[%d]: %s\n", ret, sss_strerror(ret));
+
+            return ret;
+        }
+
+        if (current == NULL) {
+            continue;
+        }
+
+        if (highest == NULL) {
+            highest = current;
+            continue;
+        }
+
+        if (sysdb_compare_usn(current, highest) > 0 ) {
+            highest = current;
+        }
+    }
+

done:
+    if (highest == NULL) {
+        usn = talloc_strdup(mem_ctx, "0");
+    } else {
+        usn = talloc_strdup(mem_ctx, highest);
+    }
+
+    if (usn == NULL) {
+        return ENOMEM;
+    }
+
+    *_usn = usn;
+    return EOK;

...

+#define ALL_FILTER "(" SYSDB_OBJECTCLASS "=" SYSDB_SUDO_CACHE_OC ")"
+

I would prefer a more specific name like e.g. SUDO_ALL_FILTER.

0008:
> diff --git a/src/providers/ldap/sdap_async_sudo.c b/src/providers/ldap/sdap_async_sudo.c
> index a44edd72b43da47903c56f34434e810d6c407d94..f24e1d7ac7c0fc07dcde5c635408d3746f8b6c6a 100644
> --- a/src/providers/ldap/sdap_async_sudo.c
> +++ b/src/providers/ldap/sdap_async_sudo.c
> @@ -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?
>  }
>
>  DEBUG(SSSDBG_TRACE_FUNC, "Received %zu rules\n", rules_count);

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.

+        return false;
+    }
+
+    return true;
+}

That's all for now, more will come later.

bye,
Sumit

[Pavel Březina]

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.
> 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.
>    return false;
> }
> return true;
>
> +}
>
> That's all for now, more will come later.

Thank you, patches are attached.

[Sumit Bose]

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

[Pavel Březina]

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=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;
     }

[Sumit Bose]

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=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.

of course not
> 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.

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,
Sumit

[Pavel Březina]

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=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.
>
> of course not
> > 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.
>
> 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,
> Sumit

Hi, 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.

[Pavel Březina]

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=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.
> >
> > of course not
> > > 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.
> >
> > 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,
> > Sumit
>
> Hi, 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.

[Pavel Březina]

On 01/12/2016 02:20 PM, Pavel Březina wrote:
> 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=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.
> > >
> > > of course not
> > > > 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.
> > >
> > > 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,
> > > Sumit
> >
> > Hi, 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.

I'm sending patches with fixed few Coverity issues that Sumit sent me 
off-list. The diff is:

diff --git a/src/providers/ipa/ipa_dn.c b/src/providers/ipa/ipa_dn.c
index 9f2950b..c58e014 100644
--- a/src/providers/ipa/ipa_dn.c
+++ b/src/providers/ipa/ipa_dn.c
@@ -53,26 +53,26 @@ static bool check_dn(struct ldb_dn *dn,
      while ((attr = va_arg(ap, const char *)) != NULL) {
          val = va_arg(ap, const char *);
          if (val == NULL) {
-            return false;
+            goto vafail;
          }

if (comp > num_comp) {
-            return false;
+            goto vafail;
          }

ldbattr = ldb_dn_get_component_name(dn, comp);
          if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
-            return false;
+            goto vafail;
          }

ldbval = ldb_dn_get_component_val(dn, comp);
          if (ldbval == NULL) {
-            return false;
+            goto vafail;
          }

strval = (const char *)ldbval->data;
          if (strval == NULL || strncasecmp(strval, val, ldbval->length) 
!= 0) {
-            return false;
+            goto vafail;
          }

diff --git a/src/providers/ipa/ipa_sudo_async.c 
b/src/providers/ipa/ipa_sudo_async.c
index c2ee1b3..e2783d3 100644
--- a/src/providers/ipa/ipa_sudo_async.c
+++ b/src/providers/ipa/ipa_sudo_async.c
@@ -1046,7 +1046,7 @@ ipa_sudo_refresh_done(struct tevent_req *subreq)
  {
    struct ipa_sudo_refresh_state *state;
      struct tevent_req *req;
-    char *usn;
+    char *usn = NULL;
      bool in_transaction = false;
      errno_t sret;
      int ret;

diff --git a/src/providers/ipa/ipa_sudo_conversion.c 
b/src/providers/ipa/ipa_sudo_conversion.c
index 87f6462..5bba714 100644
--- a/src/providers/ipa/ipa_sudo_conversion.c
+++ b/src/providers/ipa/ipa_sudo_conversion.c
@@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,

filter = talloc_strdup(tmp_ctx, "");
      if (filter == NULL) {
+        ret = ENOMEM;
          goto done;
      }

@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,

if (ctx == NULL) {
          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
-        ctx->ret = ERR_INTERNAL;
          return false;
      }

@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,

if (ctx == NULL) {
          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
-        ctx->ret = ERR_INTERNAL;
          return false;
      }

[Sumit Bose]

> diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> b/src/providers/ipa/ipa_sudo_conversion.c
> index 87f6462..5bba714 100644
> --- a/src/providers/ipa/ipa_sudo_conversion.c
> +++ b/src/providers/ipa/ipa_sudo_conversion.c
> @@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,
>  filter = talloc_strdup(tmp_ctx, "");
>  if (filter == NULL) {
>    ret = ENOMEM;
>    goto done;
> }

There is still

diff --git a/src/providers/ipa/ipa_sudo_conversion.c
b/src/providers/ipa/ipa_sudo_conversion.c
index 87f6462..88ae36e 100644
--- a/src/providers/ipa/ipa_sudo_conversion.c
+++ b/src/providers/ipa/ipa_sudo_conversion.c
@@ -601,7 +601,7 @@ build_filter(TALLOC_CTX *mem_ctx,
         if (ret != EOK) {
             DEBUG(SSSDBG_CRIT_FAILURE, "Unable to sanitize DN "
                   "[%d]: %s\n", ret, sss_strerror(ret));
-            return NULL;
+            goto done;
         }

filter = talloc_asprintf_append(filter, "(%s=%s)", rdn_attr,
safe_rdn);

missing, otherwise memory allocated on tmp_ctx will leak.

bye,
Sumit

[Pavel Březina]

On 01/13/2016 11:35 AM, Sumit Bose wrote:
> > diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> > b/src/providers/ipa/ipa_sudo_conversion.c
> > index 87f6462..5bba714 100644
> > --- a/src/providers/ipa/ipa_sudo_conversion.c
> > +++ b/src/providers/ipa/ipa_sudo_conversion.c
> > @@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,
> >   filter = talloc_strdup(tmp_ctx, "");
> >   if (filter == NULL) {
> >    ret = ENOMEM;
> >     goto done;
> > }
>
> There is still
>
> diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> b/src/providers/ipa/ipa_sudo_conversion.c
> index 87f6462..88ae36e 100644
> --- a/src/providers/ipa/ipa_sudo_conversion.c
> +++ b/src/providers/ipa/ipa_sudo_conversion.c
> @@ -601,7 +601,7 @@ build_filter(TALLOC_CTX *mem_ctx,
>           if (ret != EOK) {
>               DEBUG(SSSDBG_CRIT_FAILURE, "Unable to sanitize DN "
>                     "[%d]: %s\n", ret, sss_strerror(ret));
>        return NULL;
>        goto done;
>     }
>
>     filter = talloc_asprintf_append(filter, "(%s=%s)", rdn_attr,
>
> safe_rdn);
>
> missing, otherwise memory allocated on tmp_ctx will leak.

Fixed, thanks.

[Pavel Březina]

On 01/13/2016 11:48 AM, Pavel Březina wrote:
> On 01/13/2016 11:35 AM, Sumit Bose wrote:
> > > diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> > > b/src/providers/ipa/ipa_sudo_conversion.c
> > > index 87f6462..5bba714 100644
> > > --- a/src/providers/ipa/ipa_sudo_conversion.c
> > > +++ b/src/providers/ipa/ipa_sudo_conversion.c
> > > @@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,
> > >   filter = talloc_strdup(tmp_ctx, "");
> > >   if (filter == NULL) {
> > >    ret = ENOMEM;
> > >     goto done;
> > > }
> >
> > There is still
> >
> > diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> > b/src/providers/ipa/ipa_sudo_conversion.c
> > index 87f6462..88ae36e 100644
> > --- a/src/providers/ipa/ipa_sudo_conversion.c
> > +++ b/src/providers/ipa/ipa_sudo_conversion.c
> > @@ -601,7 +601,7 @@ build_filter(TALLOC_CTX *mem_ctx,
> >           if (ret != EOK) {
> >               DEBUG(SSSDBG_CRIT_FAILURE, "Unable to sanitize DN "
> >                     "[%d]: %s\n", ret, sss_strerror(ret));
> >        return NULL;
> >        goto done;
> >     }
> >
> >     filter = talloc_asprintf_append(filter, "(%s=%s)", rdn_attr,
> >
> > safe_rdn);
> >
> > missing, otherwise memory allocated on tmp_ctx will leak.
>
> Fixed, thanks.

One more time.

[Sumit Bose]

On Wed, Jan 13, 2016 at 11:21:54AM +0100, Pavel Březina wrote:
> On 01/12/2016 02:20 PM, Pavel Březina wrote:
> > 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=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.
> > > >
> > > > of course not
> > > > > 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.
> > > >
> > > > 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,
> > > > Sumit
> > >
> > > Hi, 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.
>
> I'm sending patches with fixed few Coverity issues that Sumit sent me
> off-list. The diff is:
>
> diff --git a/src/providers/ipa/ipa_dn.c b/src/providers/ipa/ipa_dn.c
> index 9f2950b..c58e014 100644
> --- a/src/providers/ipa/ipa_dn.c
> +++ b/src/providers/ipa/ipa_dn.c
> @@ -53,26 +53,26 @@ static bool check_dn(struct ldb_dn *dn,
>      while ((attr = va_arg(ap, const char *)) != NULL) {
>          val = va_arg(ap, const char *);
>          if (val == NULL) {
>        return false;
>        goto vafail;
>    }
>
>    if (comp > num_comp) {
>        return false;
>        goto vafail;
>    }
>
>    ldbattr = ldb_dn_get_component_name(dn, comp);
>    if (ldbattr == NULL || strcasecmp(ldbattr, attr) != 0) {
>        return false;
>        goto vafail;
>    }
>
>    ldbval = ldb_dn_get_component_val(dn, comp);
>    if (ldbval == NULL) {
>        return false;
>        goto vafail;
>    }
>
>    strval = (const char *)ldbval->data;
>    if (strval == NULL || strncasecmp(strval, val, ldbval->length) !=
> {
>        return false;
>        goto vafail;
>    }
>
>
> diff --git a/src/providers/ipa/ipa_sudo_async.c
> b/src/providers/ipa/ipa_sudo_async.c
> index c2ee1b3..e2783d3 100644
> --- a/src/providers/ipa/ipa_sudo_async.c
> +++ b/src/providers/ipa/ipa_sudo_async.c
> @@ -1046,7 +1046,7 @@ ipa_sudo_refresh_done(struct tevent_req *subreq)
>  {
>    struct ipa_sudo_refresh_state *state;
>      struct tevent_req *req;
> char *usn;
> char *usn = NULL;
> bool in_transaction = false;
> errno_t sret;
> int ret;
>
> diff --git a/src/providers/ipa/ipa_sudo_conversion.c
> b/src/providers/ipa/ipa_sudo_conversion.c
> index 87f6462..5bba714 100644
> --- a/src/providers/ipa/ipa_sudo_conversion.c
> +++ b/src/providers/ipa/ipa_sudo_conversion.c
> @@ -585,6 +585,7 @@ build_filter(TALLOC_CTX *mem_ctx,
>  filter = talloc_strdup(tmp_ctx, "");
>  if (filter == NULL) {
>    ret = ENOMEM;
>    goto done;
> }
>
> @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
>  if (ctx == NULL) {
>      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
>    ctx->ret = ERR_INTERNAL;
>    return false;
> }
>
> @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
>  if (ctx == NULL) {
>      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
>    ctx->ret = ERR_INTERNAL;
>    return false;
> }

it looks like the last 2 hunks are not available in the latest tar ball.

and there are some trailing whitespaces in patch 9 at line 41.

bye,
Sumit

[Pavel Březina]

On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> >   if (ctx == NULL) {
> >       DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> >    ctx->ret = ERR_INTERNAL;
> >     return false;
> > }
> >
> > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> >   if (ctx == NULL) {
> >       DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> >    ctx->ret = ERR_INTERNAL;
> >     return false;
> > }
>
> it looks like the last 2 hunks are not available in the latest tar ball.

Sorry about that. Added.
> and there are some trailing whitespaces in patch 9 at line 41.

Fixed by git.

I also attached two more patches then converts usn into number and then 
use in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) && 
(entryUSN != cur) as per Thierry request.

It could probably be squashed and replace some previous patches but I 
think sending it as new patches is easier for you to review, given the 
previous patches are basically acked.

[Sumit Bose]

On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > >  if (ctx == NULL) {
> > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > >    ctx->ret = ERR_INTERNAL;
> > >    return false;
> > > }
> > >
> > > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > >  if (ctx == NULL) {
> > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > >    ctx->ret = ERR_INTERNAL;
> > >    return false;
> > > }
> >
> > it looks like the last 2 hunks are not available in the latest tar ball.
>
> Sorry about that. Added.
> > and there are some trailing whitespaces in patch 9 at line 41.
>
> Fixed by git.
>
> I also attached two more patches then converts usn into number and then use
> in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> (entryUSN != cur) as per Thierry request.
>
> It could probably be squashed and replace some previous patches but I think
> sending it as new patches is easier for you to review, given the previous
> patches are basically acked.
> errno = 0;
> usn_number = strtoul(usn, &endptr, 10);
> if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
>     && (usn_number > srv_opts->last_usn)) {
>     srv_opts->last_usn = usn_number;
> if (endptr != NULL && *endptr != '\0') {

Currently an empty string in usn would result in usn_number==0, I wonder
if this is valid or if it would be better to error out in this case?

Otherwise all looks good, CI and Coverity test are currently running.

bye,
Sumit
>    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
>    return;
> } else if (errno != 0) {
>    ret = errno;
>    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
>
> %s\n",
>          usn, ret, sss_strerror(ret));
>    return;
> }
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Pavel Březina]

On 01/14/2016 06:37 PM, Sumit Bose wrote:
> On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > >   if (ctx == NULL) {
> > > >       DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > >    ctx->ret = ERR_INTERNAL;
> > > >     return false;
> > > > }
> > > >
> > > > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > >   if (ctx == NULL) {
> > > >       DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > >    ctx->ret = ERR_INTERNAL;
> > > >     return false;
> > > > }
> > >
> > > it looks like the last 2 hunks are not available in the latest tar ball.
> >
> > Sorry about that. Added.
> > > and there are some trailing whitespaces in patch 9 at line 41.
> >
> > Fixed by git.
> >
> > I also attached two more patches then converts usn into number and then use
> > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > (entryUSN != cur) as per Thierry request.
> >
> > It could probably be squashed and replace some previous patches but I think
> > sending it as new patches is easier for you to review, given the previous
> > patches are basically acked.
> > errno = 0;
> >  usn_number = strtoul(usn, &endptr, 10);
> > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> >     && (usn_number > srv_opts->last_usn)) {
> >     srv_opts->last_usn = usn_number;
> > if (endptr != NULL && *endptr != '\0') {
>
> Currently an empty string in usn would result in usn_number==0, I wonder
> if this is valid or if it would be better to error out in this case?

Zero value will not change anything since we always compare if the 
current usn value is lower than the new one, we switch them only in such 
case. So I think it is not necessary to error out in this case since 
strtoul succeeds.
> Otherwise all looks good, CI and Coverity test are currently running.
>
> bye,
> Sumit
> >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> >    return;
> > } else if (errno != 0) {
> >    ret = errno;
> >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> >
> > %s\n",
> >          usn, ret, sss_strerror(ret));
> >    return;
> > }

[Sumit Bose]

On Fri, Jan 15, 2016 at 10:21:25AM +0100, Pavel Březina wrote:
> On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > >  if (ctx == NULL) {
> > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > >    ctx->ret = ERR_INTERNAL;
> > > > >    return false;
> > > > > }
> > > > >
> > > > > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > >  if (ctx == NULL) {
> > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > >    ctx->ret = ERR_INTERNAL;
> > > > >    return false;
> > > > > }
> > > >
> > > > it looks like the last 2 hunks are not available in the latest tar ball.
> > >
> > > Sorry about that. Added.
> > > > and there are some trailing whitespaces in patch 9 at line 41.
> > >
> > > Fixed by git.
> > >
> > > I also attached two more patches then converts usn into number and then use
> > > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > (entryUSN != cur) as per Thierry request.
> > >
> > > It could probably be squashed and replace some previous patches but I think
> > > sending it as new patches is easier for you to review, given the previous
> > > patches are basically acked.
> > > errno = 0;
> > > usn_number = strtoul(usn, &endptr, 10);
> > > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > >     && (usn_number > srv_opts->last_usn)) {
> > >     srv_opts->last_usn = usn_number;
> > > if (endptr != NULL && *endptr != '\0') {
> >
> > Currently an empty string in usn would result in usn_number==0, I wonder
> > if this is valid or if it would be better to error out in this case?
>
> Zero value will not change anything since we always compare if the current
> usn value is lower than the new one, we switch them only in such case. So I
> think it is not necessary to error out in this case since strtoul succeeds.

ok, that was my thinking, too.
> > Otherwise all looks good, CI and Coverity test are currently running.

Coverity didn't found any new issues and CI passed
(http://sssd-ci.duckdns.org/logs/job/35/58/summary.html) expect on Debian
where the test_add_remove_user integration tested failed. But since the
same test passed on other platform I expect that the failure it not
related to your tests.

ACK.

bye,
Sumit

[Lukas Slebodnik]

On (15/01/16 10:21), Pavel Březina wrote:
> On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > >  if (ctx == NULL) {
> > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > >    ctx->ret = ERR_INTERNAL;
> > > > >    return false;
> > > > > }
> > > > >
> > > > > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > >  if (ctx == NULL) {
> > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > >    ctx->ret = ERR_INTERNAL;
> > > > >    return false;
> > > > > }
> > > >
> > > > it looks like the last 2 hunks are not available in the latest tar ball.
> > >
> > > Sorry about that. Added.
> > > > and there are some trailing whitespaces in patch 9 at line 41.
> > >
> > > Fixed by git.
> > >
> > > I also attached two more patches then converts usn into number and then use
> > > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > (entryUSN != cur) as per Thierry request.
> > >
> > > It could probably be squashed and replace some previous patches but I think
> > > sending it as new patches is easier for you to review, given the previous
> > > patches are basically acked.
> > > errno = 0;
> > > usn_number = strtoul(usn, &endptr, 10);
> > > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > >     && (usn_number > srv_opts->last_usn)) {
> > >     srv_opts->last_usn = usn_number;
> > > if (endptr != NULL && *endptr != '\0') {
> >
> > Currently an empty string in usn would result in usn_number==0, I wonder
> > if this is valid or if it would be better to error out in this case?
>
> Zero value will not change anything since we always compare if the current
> usn value is lower than the new one, we switch them only in such case. So I
> think it is not necessary to error out in this case since strtoul succeeds.
> > Otherwise all looks good, CI and Coverity test are currently running.
> >
> > bye,
> > Sumit
> > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > >    return;
> > > } else if (errno != 0) {
> > >    ret = errno;
> > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > >
> > > %s\n",
> > >          usn, ret, sss_strerror(ret));
> > >    return;
> > > }

There are 4 failed test from freeipa sudo test suite.

test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group

=================================== FAILURES ===================================
______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>

def test_sudo_rule_restricted_to_one_hostmask(self):
        if self.__class__.skip_hostmask_based:
            raise pytest.skip("Hostmask could not be detected")

result1 = self.list_sudo_commands("testuser1")
>   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text

E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text

test_integration/test_sudo.py:295: AssertionError
______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>

def test_sudo_rule_restricted_to_running_as_single_local_user(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
>   assert "RunAsUsers: localuser" in result1.stdout_text

E       assert 'RunAsUsers: localuser' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7a81650>.stdout_text

test_integration/test_sudo.py:418: AssertionError
_____ TestSudo.test_sudo_rule_restricted_to_run_as_users_from_local_group ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d56d7d90>

def test_sudo_rule_restricted_to_run_as_users_from_local_group(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
>   assert "RunAsUsers: %localgroup" in result1.stdout_text

E       assert 'RunAsUsers: %localgroup' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7b08510>.stdout_text

test_integration/test_sudo.py:460: AssertionError
_____ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_group ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d544e790>

def test_sudo_rule_restricted_to_running_as_single_local_group(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
        assert "RunAsUsers: root" in result1.stdout_text
>   assert "RunAsGroups: localgroup" in result1.stdout_text

E       assert 'RunAsGroups: localgroup' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d79e4350>.stdout_text

test_integration/test_sudo.py:503: AssertionError
==================== 4 failed, 71 passed in 1384.43 seconds ====================

LS

[Sumit Bose]

On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> On (15/01/16 10:21), Pavel Březina wrote:
> > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > >  if (ctx == NULL) {
> > > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > >    ctx->ret = ERR_INTERNAL;
> > > > > >    return false;
> > > > > > }
> > > > > >
> > > > > > @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > >  if (ctx == NULL) {
> > > > > >      DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > >    ctx->ret = ERR_INTERNAL;
> > > > > >    return false;
> > > > > > }
> > > > >
> > > > > it looks like the last 2 hunks are not available in the latest tar ball.
> > > >
> > > > Sorry about that. Added.
> > > > > and there are some trailing whitespaces in patch 9 at line 41.
> > > >
> > > > Fixed by git.
> > > >
> > > > I also attached two more patches then converts usn into number and then use
> > > > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > (entryUSN != cur) as per Thierry request.
> > > >
> > > > It could probably be squashed and replace some previous patches but I think
> > > > sending it as new patches is easier for you to review, given the previous
> > > > patches are basically acked.
> > > > errno = 0;
> > > > usn_number = strtoul(usn, &endptr, 10);
> > > > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > >     && (usn_number > srv_opts->last_usn)) {
> > > >     srv_opts->last_usn = usn_number;
> > > > if (endptr != NULL && *endptr != '\0') {
> > >
> > > Currently an empty string in usn would result in usn_number==0, I wonder
> > > if this is valid or if it would be better to error out in this case?
> >
> > Zero value will not change anything since we always compare if the current
> > usn value is lower than the new one, we switch them only in such case. So I
> > think it is not necessary to error out in this case since strtoul succeeds.
> > > Otherwise all looks good, CI and Coverity test are currently running.
> > >
> > > bye,
> > > Sumit
> > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > >    return;
> > > > } else if (errno != 0) {
> > > >    ret = errno;
> > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > >
> > > > %s\n",
> > > >          usn, ret, sss_strerror(ret));
> > > >    return;
> > > > }
>
> There are 4 failed test from freeipa sudo test suite.
>
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
>
> =================================== FAILURES ===================================
> ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> def test_sudo_rule_restricted_to_one_hostmask(self):
>     if self.__class__.skip_hostmask_based:
>         raise pytest.skip("Hostmask could not be detected")
>
>     result1 = self.list_sudo_commands("testuser1")
> >   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
>
> E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
>
> test_integration/test_sudo.py:295: AssertionError
> ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> def test_sudo_rule_restricted_to_running_as_single_local_user(self):
>     result1 = self.list_sudo_commands("testuser1", verbose=True)
> >   assert "RunAsUsers: localuser" in result1.stdout_text

There are just too many options. I tested RunAsUsers and RunAsUsers
but only with IPA users and groups. I looks like the handling for the
ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> E       assert 'RunAsUsers: localuser' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7a81650>.stdout_text
>
> test_integration/test_sudo.py:418: AssertionError
> _____ TestSudo.test_sudo_rule_restricted_to_run_as_users_from_local_group ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d56d7d90>
> def test_sudo_rule_restricted_to_run_as_users_from_local_group(self):
>     result1 = self.list_sudo_commands("testuser1", verbose=True)
> >   assert "RunAsUsers: %localgroup" in result1.stdout_text
>
> E       assert 'RunAsUsers: %localgroup' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7b08510>.stdout_text
>
> test_integration/test_sudo.py:460: AssertionError
> _____ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_group ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d544e790>
> def test_sudo_rule_restricted_to_running_as_single_local_group(self):
>     result1 = self.list_sudo_commands("testuser1", verbose=True)
>     assert "RunAsUsers: root" in result1.stdout_text
> >   assert "RunAsGroups: localgroup" in result1.stdout_text
>
> E       assert 'RunAsGroups: localgroup' in 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em15:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em15:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d79e4350>.stdout_text
>
> test_integration/test_sudo.py:503: AssertionError
> ==================== 4 failed, 71 passed in 1384.43 seconds ====================
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Lukas Slebodnik]

On (15/01/16 15:06), Sumit Bose wrote:
> On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > On (15/01/16 10:21), Pavel Březina wrote:
> > > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >
> > > > > > >@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >
> > > > > > >      if (ctx == NULL) {
> > > > > > >          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >-        ctx->ret = ERR_INTERNAL;
> > > > > > >          return false;
> > > > > > >      }
> > > > > > >
> > > > > > >@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >
> > > > > > >      if (ctx == NULL) {
> > > > > > >          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >-        ctx->ret = ERR_INTERNAL;
> > > > > > >          return false;
> > > > > > >      }
> > > > > > >
> > > > > >
> > > > > > it looks like the last 2 hunks are not available in the latest tar ball.
> > > > >
> > > > > Sorry about that. Added.
> > > > > > and there are some trailing whitespaces in patch 9 at line 41.
> > > > >
> > > > > Fixed by git.
> > > > >
> > > > > I also attached two more patches then converts usn into number and then use
> > > > > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > > (entryUSN != cur) as per Thierry request.
> > > > >
> > > > > It could probably be squashed and replace some previous patches but I think
> > > > > sending it as new patches is easier for you to review, given the previous
> > > > > patches are basically acked.
> > > > > errno = 0;
> > > > > usn_number = strtoul(usn, &endptr, 10);
> > > > > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > >     && (usn_number > srv_opts->last_usn)) {
> > > > >     srv_opts->last_usn = usn_number;
> > > > > if (endptr != NULL && *endptr != '\0') {
> > > >
> > > > Currently an empty string in usn would result in usn_number==0, I wonder
> > > > if this is valid or if it would be better to error out in this case?
> > >
> > > Zero value will not change anything since we always compare if the current
> > > usn value is lower than the new one, we switch them only in such case. So I
> > > think it is not necessary to error out in this case since strtoul succeeds.
> > > > Otherwise all looks good, CI and Coverity test are currently running.
> > > >
> > > > bye,
> > > > Sumit
> > > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > > >    return;
> > > > > } else if (errno != 0) {
> > > > >    ret = errno;
> > > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > > >
> > > > > %s\n",
> > > > >          usn, ret, sss_strerror(ret));
> > > > >    return;
> > > > > }
> >
> > There are 4 failed test from freeipa sudo test suite.
> >
> > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> >
> > =================================== FAILURES ===================================
> > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
> >
> > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> > def test_sudo_rule_restricted_to_one_hostmask(self):
> >     if self.__class__.skip_hostmask_based:
> >         raise pytest.skip("Hostmask could not be detected")
> >
> >     result1 = self.list_sudo_commands("testuser1")
> > >   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> >
> > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
> >
> > test_integration/test_sudo.py:295: AssertionError
> > ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> >
> > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> > def test_sudo_rule_restricted_to_running_as_single_local_user(self):
> >     result1 = self.list_sudo_commands("testuser1", verbose=True)
> > >   assert "RunAsUsers: localuser" in result1.stdout_text
>
> There are just too many options. I tested RunAsUsers and RunAsUsers
> but only with IPA users and groups. I looks like the handling for the
> ipasudorunasextuser and ipasudorunasextgroup attributes are missing.

That's the purpose of automated tests :-)
To cover all use-case.

Furtunatelly, Tomas wrote quite good test.
But there still might be other use cases which are not covered.
Because we cannot see a code coverage :-(

LS

[Pavel Březina]

On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> On (15/01/16 15:06), Sumit Bose wrote:
> > On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > On (15/01/16 10:21), Pavel Březina wrote:
> > > > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>
> > > > > > >> @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>
> > > > > > >>       if (ctx == NULL) {
> > > > > > >>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>           return false;
> > > > > > >>       }
> > > > > > >>
> > > > > > >> @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>
> > > > > > >>       if (ctx == NULL) {
> > > > > > >>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>           return false;
> > > > > > >>       }
> > > > > > >>
> > > > > > >
> > > > > > > it looks like the last 2 hunks are not available in the latest tar ball.
> > > > > >
> > > > > > Sorry about that. Added.
> > > > > >
> > > > > > > and there are some trailing whitespaces in patch 9 at line 41.
> > > > > >
> > > > > > Fixed by git.
> > > > > >
> > > > > > I also attached two more patches then converts usn into number and then use
> > > > > > in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > > > (entryUSN != cur) as per Thierry request.
> > > > > >
> > > > > > It could probably be squashed and replace some previous patches but I think
> > > > > > sending it as new patches is easier for you to review, given the previous
> > > > > > patches are basically acked.
> > > > > > errno = 0;
> > > > > >  usn_number = strtoul(usn, &endptr, 10);
> > > > > > if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > >     && (usn_number > srv_opts->last_usn)) {
> > > > > >     srv_opts->last_usn = usn_number;
> > > > > > if (endptr != NULL && *endptr != '\0') {
> > > > >
> > > > > Currently an empty string in usn would result in usn_number==0, I wonder
> > > > > if this is valid or if it would be better to error out in this case?
> > > >
> > > > Zero value will not change anything since we always compare if the current
> > > > usn value is lower than the new one, we switch them only in such case. So I
> > > > think it is not necessary to error out in this case since strtoul succeeds.
> > > > > Otherwise all looks good, CI and Coverity test are currently running.
> > > > >
> > > > > bye,
> > > > > Sumit
> > > > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > > > >    return;
> > > > > > } else if (errno != 0) {
> > > > > >    ret = errno;
> > > > > >    DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > > > >
> > > > > > %s\n",
> > > > > >          usn, ret, sss_strerror(ret));
> > > > > >    return;
> > > > > > }
> > >
> > > There are 4 failed test from freeipa sudo test suite.
> > >
> > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > >
> > > =================================== FAILURES ===================================
> > > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
> > >
> > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> > >  def test_sudo_rule_restricted_to_one_hostmask(self):
> > >      if self.__class__.skip_hostmask_based:
> > >          raise pytest.skip("Hostmask could not be detected")
> > >
> > >      result1 = self.list_sudo_commands("testuser1")
> > > >    assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > >
> > > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
> > >
> > > test_integration/test_sudo.py:295: AssertionError
> > > ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > >
> > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> > >  def test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > >      result1 = self.list_sudo_commands("testuser1", verbose=True)
> > > >    assert "RunAsUsers: localuser" in result1.stdout_text
> >
> > There are just too many options. I tested RunAsUsers and RunAsUsers
> > but only with IPA users and groups. I looks like the handling for the
> > ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
>
> That's the purpose of automated tests :-)
> To cover all use-case.

These use cases are omitted by design.
> Furtunatelly, Tomas wrote quite good test.
> But there still might be other use cases which are not covered.
> Because we cannot see a code coverage :-(
>
> LS
> _______________________________________________
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/admin/lists/[e-mail]

[Jakub Hrozek]

On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > On (15/01/16 15:06), Sumit Bose wrote:
> > > On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > On (15/01/16 10:21), Pavel Březina wrote:
> > > > > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>
> > > > > > >>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>
> > > > > > >>>      if (ctx == NULL) {
> > > > > > >>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>          return false;
> > > > > > >>>      }
> > > > > > >>>
> > > > > > >>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>
> > > > > > >>>      if (ctx == NULL) {
> > > > > > >>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>          return false;
> > > > > > >>>      }
> > > > > > >>>
> > > > > > >>
> > > > > > >>it looks like the last 2 hunks are not available in the latest tar ball.
> > > > > > >
> > > > > > >Sorry about that. Added.
> > > > > > >
> > > > > > >>and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >
> > > > > > >Fixed by git.
> > > > > > >
> > > > > > >I also attached two more patches then converts usn into number and then use
> > > > > > >in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > > > >(entryUSN != cur) as per Thierry request.
> > > > > > >
> > > > > > >It could probably be squashed and replace some previous patches but I think
> > > > > > >sending it as new patches is easier for you to review, given the previous
> > > > > > >patches are basically acked.
> > > > > >
> > > > > > >+    errno = 0;
> > > > > > >      usn_number = strtoul(usn, &endptr, 10);
> > > > > > >-    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >-         && (usn_number > srv_opts->last_usn)) {
> > > > > > >-         srv_opts->last_usn = usn_number;
> > > > > > >+    if (endptr != NULL && *endptr != '\0') {
> > > > > >
> > > > > > Currently an empty string in usn would result in usn_number==0, I wonder
> > > > > > if this is valid or if it would be better to error out in this case?
> > > > >
> > > > > Zero value will not change anything since we always compare if the current
> > > > > usn value is lower than the new one, we switch them only in such case. So I
> > > > > think it is not necessary to error out in this case since strtoul succeeds.
> > > > > > Otherwise all looks good, CI and Coverity test are currently running.
> > > > > >
> > > > > > bye,
> > > > > > Sumit
> > > > > >
> > > > > > >+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > > > > >+        return;
> > > > > > >+    } else if (errno != 0) {
> > > > > > >+        ret = errno;
> > > > > > >+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > > > > >%s\n",
> > > > > > >+              usn, ret, sss_strerror(ret));
> > > > > > >+        return;
> > > > > > >      }
> > > >
> > > > There are 4 failed test from freeipa sudo test suite.
> > > >
> > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > >
> > > > =================================== FAILURES ===================================
> > > > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
> > > >
> > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> > > > def test_sudo_rule_restricted_to_one_hostmask(self):
> > > >     if self.__class__.skip_hostmask_based:
> > > >         raise pytest.skip("Hostmask could not be detected")
> > > >
> > > >     result1 = self.list_sudo_commands("testuser1")
> > > > >   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > >
> > > > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
> > > >
> > > > test_integration/test_sudo.py:295: AssertionError
> > > > ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > >
> > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> > > > def test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > >     result1 = self.list_sudo_commands("testuser1", verbose=True)
> > > > >   assert "RunAsUsers: localuser" in result1.stdout_text
> > >
> > > There are just too many options. I tested RunAsUsers and RunAsUsers
> > > but only with IPA users and groups. I looks like the handling for the
> > > ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> >
> > That's the purpose of automated tests :-)
> > To cover all use-case.
>
> These use cases are omitted by design.

We might still need to at least document this change. But with a recent IPA
server, I see:
    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --externaluser=jhrozek readfiles
    ipa: ERROR: invalid 'externaluser': this option has been deprecated.
    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternalgroup=jhrozek readfiles
    ipa: ERROR: invalid 'runasexternalgroup': this option has been deprecated.
    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternaluser=jhrozek readfiles
    ipa: ERROR: invalid 'runasexternaluser': this option has been deprecated.

So I guess we should document that if you use a new sssd version with an
old IPA server AND you use these deprecated options you should revert to
the old schema?

[Lukas Slebodnik]

On (18/01/16 10:18), Jakub Hrozek wrote:
> On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> > On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > > On (15/01/16 15:06), Sumit Bose wrote:
> > > > On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > > On (15/01/16 10:21), Pavel Březina wrote:
> > > > > > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > >On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >>On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>>
> > > > > > >>>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>>
> > > > > > >>>>      if (ctx == NULL) {
> > > > > > >>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>          return false;
> > > > > > >>>>      }
> > > > > > >>>>
> > > > > > >>>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>>
> > > > > > >>>>      if (ctx == NULL) {
> > > > > > >>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>          return false;
> > > > > > >>>>      }
> > > > > > >>>>
> > > > > > >>>
> > > > > > >>>it looks like the last 2 hunks are not available in the latest tar ball.
> > > > > > >>
> > > > > > >>Sorry about that. Added.
> > > > > > >>
> > > > > > >>>and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >>
> > > > > > >>Fixed by git.
> > > > > > >>
> > > > > > >>I also attached two more patches then converts usn into number and then use
> > > > > > >>in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > > > >>(entryUSN != cur) as per Thierry request.
> > > > > > >>
> > > > > > >>It could probably be squashed and replace some previous patches but I think
> > > > > > >>sending it as new patches is easier for you to review, given the previous
> > > > > > >>patches are basically acked.
> > > > > > >
> > > > > > >>+    errno = 0;
> > > > > > >>      usn_number = strtoul(usn, &endptr, 10);
> > > > > > >>-    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >>-         && (usn_number > srv_opts->last_usn)) {
> > > > > > >>-         srv_opts->last_usn = usn_number;
> > > > > > >>+    if (endptr != NULL && *endptr != '\0') {
> > > > > > >
> > > > > > >Currently an empty string in usn would result in usn_number==0, I wonder
> > > > > > >if this is valid or if it would be better to error out in this case?
> > > > > >
> > > > > > Zero value will not change anything since we always compare if the current
> > > > > > usn value is lower than the new one, we switch them only in such case. So I
> > > > > > think it is not necessary to error out in this case since strtoul succeeds.
> > > > > >
> > > > > > >
> > > > > > >Otherwise all looks good, CI and Coverity test are currently running.
> > > > > > >
> > > > > > >bye,
> > > > > > >Sumit
> > > > > > >
> > > > > > >>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > > > > >>+        return;
> > > > > > >>+    } else if (errno != 0) {
> > > > > > >>+        ret = errno;
> > > > > > >>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > > > > >>%s\n",
> > > > > > >>+              usn, ret, sss_strerror(ret));
> > > > > > >>+        return;
> > > > > > >>      }
> > > > >
> > > > > There are 4 failed test from freeipa sudo test suite.
> > > > >
> > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > > >
> > > > > =================================== FAILURES ===================================
> > > > > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
> > > > >
> > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> > > > > def test_sudo_rule_restricted_to_one_hostmask(self):
> > > > >     if self.__class__.skip_hostmask_based:
> > > > >         raise pytest.skip("Hostmask could not be detected")
> > > > >
> > > > >     result1 = self.list_sudo_commands("testuser1")
> > > > > >   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > > >
> > > > > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > > E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
> > > > >
> > > > > test_integration/test_sudo.py:295: AssertionError
> > > > > ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > > >
> > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> > > > > def test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > > >     result1 = self.list_sudo_commands("testuser1", verbose=True)
> > > > > >   assert "RunAsUsers: localuser" in result1.stdout_text
> > > >
> > > > There are just too many options. I tested RunAsUsers and RunAsUsers
> > > > but only with IPA users and groups. I looks like the handling for the
> > > > ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> > >
> > > That's the purpose of automated tests :-)
> > > To cover all use-case.
> >
> > These use cases are omitted by design.
>
> We might still need to at least document this change. But with a recent IPA
> server, I see:
>    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --externaluser=jhrozek readfiles
>    ipa: ERROR: invalid 'externaluser': this option has been deprecated.
>    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternalgroup=jhrozek readfiles
>    ipa: ERROR: invalid 'runasexternalgroup': this option has been deprecated.
>    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternaluser=jhrozek readfiles
>    ipa: ERROR: invalid 'runasexternaluser': this option has been deprecated.

We might ask whether Dreamworks need this feature.
Otherwise these patches will not help them.

LS

[Pavel Březina]

On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:
> On (18/01/16 10:18), Jakub Hrozek wrote:
> > On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> > > On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > > > On (15/01/16 15:06), Sumit Bose wrote:
> > > > > On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > > > On (15/01/16 10:21), Pavel Březina wrote:
> > > > > > > On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > >> On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >>> On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>>>
> > > > > > >>>>> @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>>>
> > > > > > >>>>>       if (ctx == NULL) {
> > > > > > >>>>>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>           return false;
> > > > > > >>>>>       }
> > > > > > >>>>>
> > > > > > >>>>> @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>>>
> > > > > > >>>>>       if (ctx == NULL) {
> > > > > > >>>>>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>           return false;
> > > > > > >>>>>       }
> > > > > > >>>>>
> > > > > > >>>>
> > > > > > >>>> it looks like the last 2 hunks are not available in the latest tar ball.
> > > > > > >>>
> > > > > > >>> Sorry about that. Added.
> > > > > > >>>
> > > > > > >>>> and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >>>
> > > > > > >>> Fixed by git.
> > > > > > >>>
> > > > > > >>> I also attached two more patches then converts usn into number and then use
> > > > > > >>> in filter entryUSN >= (current + 1) instead of (entryUSN >= cur) &&
> > > > > > >>> (entryUSN != cur) as per Thierry request.
> > > > > > >>>
> > > > > > >>> It could probably be squashed and replace some previous patches but I think
> > > > > > >>> sending it as new patches is easier for you to review, given the previous
> > > > > > >>> patches are basically acked.
> > > > > > >>
> > > > > > >>> +    errno = 0;
> > > > > > >>>       usn_number = strtoul(usn, &endptr, 10);
> > > > > > >>> -    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >>> -         && (usn_number > srv_opts->last_usn)) {
> > > > > > >>> -         srv_opts->last_usn = usn_number;
> > > > > > >>> +    if (endptr != NULL && *endptr != '\0') {
> > > > > > >>
> > > > > > >> Currently an empty string in usn would result in usn_number==0, I wonder
> > > > > > >> if this is valid or if it would be better to error out in this case?
> > > > > > >
> > > > > > > Zero value will not change anything since we always compare if the current
> > > > > > > usn value is lower than the new one, we switch them only in such case. So I
> > > > > > > think it is not necessary to error out in this case since strtoul succeeds.
> > > > > > >
> > > > > > >>
> > > > > > >> Otherwise all looks good, CI and Coverity test are currently running.
> > > > > > >>
> > > > > > >> bye,
> > > > > > >> Sumit
> > > > > > >>
> > > > > > >>> +        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s\n", usn);
> > > > > > >>> +        return;
> > > > > > >>> +    } else if (errno != 0) {
> > > > > > >>> +        ret = errno;
> > > > > > >>> +        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s [%d]:
> > > > > > >>> %s\n",
> > > > > > >>> +              usn, ret, sss_strerror(ret));
> > > > > > >>> +        return;
> > > > > > >>>       }
> > > > > >
> > > > > > There are 4 failed test from freeipa sudo test suite.
> > > > > >
> > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > > > >
> > > > > > =================================== FAILURES ===================================
> > > > > > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
> > > > > >
> > > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d7ace8d0>
> > > > > >  def test_sudo_rule_restricted_to_one_hostmask(self):
> > > > > >      if self.__class__.skip_hostmask_based:
> > > > > >          raise pytest.skip("Hostmask could not be detected")
> > > > > >
> > > > > >      result1 = self.list_sudo_commands("testuser1")
> > > > > >
> > > > > > >        assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > > > > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > > > E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7fe0d7acee10>.stdout_text
> > > > > >
> > > > > > test_integration/test_sudo.py:295: AssertionError
> > > > > > ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > > > >
> > > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7fe0d79de990>
> > > > > >  def test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > > > >      result1 = self.list_sudo_commands("testuser1", verbose=True)
> > > > > >
> > > > > > >        assert "RunAsUsers: localuser" in result1.stdout_text
> > > > >
> > > > > There are just too many options. I tested RunAsUsers and RunAsUsers
> > > > > but only with IPA users and groups. I looks like the handling for the
> > > > > ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> > > >
> > > > That's the purpose of automated tests :-)
> > > > To cover all use-case.
> > >
> > > These use cases are omitted by design.
> >
> > We might still need to at least document this change. But with a recent IPA
> > server, I see:
> >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --externaluser=jhrozek readfiles
> >     ipa: ERROR: invalid 'externaluser': this option has been deprecated.
> >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternalgroup=jhrozek readfiles
> >     ipa: ERROR: invalid 'runasexternalgroup': this option has been deprecated.
> >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod --runasexternaluser=jhrozek readfiles
> >     ipa: ERROR: invalid 'runasexternaluser': this option has been deprecated.
>
> We might ask whether Dreamworks need this feature.
> Otherwise these patches will not help them.

We talked with Jakub about it and decided that we will add support of 
ipaSudoRunAsExt* attributes. So with this patch set only hostmask test 
should fail, we will not add support for hostmask unless requested.

[Pavel Březina]

On 01/18/2016 12:23 PM, Pavel Březina wrote:
> On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:
> > On (18/01/16 10:18), Jakub Hrozek wrote:
> > > On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> > > > On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > > > > On (15/01/16 15:06), Sumit Bose wrote:
> > > > > > On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > > > > On (15/01/16 10:21), Pavel Březina wrote:
> > > > > > >> On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > >>> On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >>>> On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>>>>
> > > > > > >>>>>> @@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>>>>
> > > > > > >>>>>>       if (ctx == NULL) {
> > > > > > >>>>>>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>           return false;
> > > > > > >>>>>>       }
> > > > > > >>>>>>
> > > > > > >>>>>> @@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>>>>
> > > > > > >>>>>>       if (ctx == NULL) {
> > > > > > >>>>>>           DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>> -        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>           return false;
> > > > > > >>>>>>       }
> > > > > > >>>>>>
> > > > > > >>>>>
> > > > > > >>>>> it looks like the last 2 hunks are not available in the
> > > > > > >>>>> latest tar ball.
> > > > > > >>>>
> > > > > > >>>> Sorry about that. Added.
> > > > > > >>>>
> > > > > > >>>>> and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >>>>
> > > > > > >>>> Fixed by git.
> > > > > > >>>>
> > > > > > >>>> I also attached two more patches then converts usn into number
> > > > > > >>>> and then use
> > > > > > >>>> in filter entryUSN >= (current + 1) instead of (entryUSN >=
> > > > > > >>>> cur) &&
> > > > > > >>>> (entryUSN != cur) as per Thierry request.
> > > > > > >>>>
> > > > > > >>>> It could probably be squashed and replace some previous
> > > > > > >>>> patches but I think
> > > > > > >>>> sending it as new patches is easier for you to review, given
> > > > > > >>>> the previous
> > > > > > >>>> patches are basically acked.
> > > > > > >>>
> > > > > > >>>> +    errno = 0;
> > > > > > >>>>       usn_number = strtoul(usn, &endptr, 10);
> > > > > > >>>> -    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >>>> -         && (usn_number > srv_opts->last_usn)) {
> > > > > > >>>> -         srv_opts->last_usn = usn_number;
> > > > > > >>>> +    if (endptr != NULL && *endptr != '\0') {
> > > > > > >>>
> > > > > > >>> Currently an empty string in usn would result in usn_number==0,
> > > > > > >>> I wonder
> > > > > > >>> if this is valid or if it would be better to error out in this
> > > > > > >>> case?
> > > > > > >>
> > > > > > >> Zero value will not change anything since we always compare if
> > > > > > >> the current
> > > > > > >> usn value is lower than the new one, we switch them only in such
> > > > > > >> case. So I
> > > > > > >> think it is not necessary to error out in this case since
> > > > > > >> strtoul succeeds.
> > > > > > >>
> > > > > > >>>
> > > > > > >>> Otherwise all looks good, CI and Coverity test are currently
> > > > > > >>> running.
> > > > > > >>>
> > > > > > >>> bye,
> > > > > > >>> Sumit
> > > > > > >>>
> > > > > > >>>> +        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN
> > > > > > >>>> %s\n", usn);
> > > > > > >>>> +        return;
> > > > > > >>>> +    } else if (errno != 0) {
> > > > > > >>>> +        ret = errno;
> > > > > > >>>> +        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s
> > > > > > >>>> [%d]:
> > > > > > >>>> %s\n",
> > > > > > >>>> +              usn, ret, sss_strerror(ret));
> > > > > > >>>> +        return;
> > > > > > >>>>       }
> > > > > > >
> > > > > > > There are 4 failed test from freeipa sudo test suite.
> > > > > > >
> > > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > > > >
> > > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > > > >
> > > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > > > >
> > > > > > > test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > > > > >
> > > > > > >
> > > > > > >
> > > > > > > =================================== FAILURES
> > > > > > > ===================================
> > > > > > > ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask
> > > > > > > ______________
> > > > > > >
> > > > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > > 0x7fe0d7ace8d0>
> > > > > > >
> > > > > > >      def test_sudo_rule_restricted_to_one_hostmask(self):
> > > > > > >          if self.__class__.skip_hostmask_based:
> > > > > > >              raise pytest.skip("Hostmask could not be detected")
> > > > > > >
> > > > > > >          result1 = self.list_sudo_commands("testuser1")
> > > > > > >>        assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > > > > > E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > > > > E        +  where '' = <pytest_multihost.transport.SSHCommand
> > > > > > > object at 0x7fe0d7acee10>.stdout_text
> > > > > > >
> > > > > > > test_integration/test_sudo.py:295: AssertionError
> > > > > > > ______
> > > > > > > TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > > > > >
> > > > > > >
> > > > > > > self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > > 0x7fe0d79de990>
> > > > > > >
> > > > > > >      def
> > > > > > > test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > > > > >          result1 = self.list_sudo_commands("testuser1",
> > > > > > > verbose=True)
> > > > > > >>        assert "RunAsUsers: localuser" in result1.stdout_text
> > > > > >
> > > > > > There are just too many options. I tested RunAsUsers and RunAsUsers
> > > > > > but only with IPA users and groups. I looks like the handling for the
> > > > > > ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> > > > >
> > > > > That's the purpose of automated tests :-)
> > > > > To cover all use-case.
> > > >
> > > > These use cases are omitted by design.
> > >
> > > We might still need to at least document this change. But with a
> > > recent IPA
> > > server, I see:
> > >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > --externaluser=jhrozek readfiles
> > >     ipa: ERROR: invalid 'externaluser': this option has been deprecated.
> > >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > --runasexternalgroup=jhrozek readfiles
> > >     ipa: ERROR: invalid 'runasexternalgroup': this option has been
> > > deprecated.
> > >     [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > --runasexternaluser=jhrozek readfiles
> > >     ipa: ERROR: invalid 'runasexternaluser': this option has been
> > > deprecated.
> >
> > We might ask whether Dreamworks need this feature.
> > Otherwise these patches will not help them.
>
> We talked with Jakub about it and decided that we will add support of
> ipaSudoRunAsExt* attributes. So with this patch set only hostmask test
> should fail, we will not add support for hostmask unless requested.

The previous tarball contained one patch that was already pushed...

[Lukas Slebodnik]

On (18/01/16 13:05), Pavel Březina wrote:
> On 01/18/2016 12:23 PM, Pavel Březina wrote:
> > On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:
> > > On (18/01/16 10:18), Jakub Hrozek wrote:
> > > > On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> > > > > On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > > > > > On (15/01/16 15:06), Sumit Bose wrote:
> > > > > > >On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > > > >>On (15/01/16 10:21), Pavel Březina wrote:
> > > > > > >>>On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > >>>>On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >>>>>On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>>>>>
> > > > > > >>>>>>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>>>>>
> > > > > > >>>>>>>      if (ctx == NULL) {
> > > > > > >>>>>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>>          return false;
> > > > > > >>>>>>>      }
> > > > > > >>>>>>>
> > > > > > >>>>>>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>>>>>
> > > > > > >>>>>>>      if (ctx == NULL) {
> > > > > > >>>>>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>>          return false;
> > > > > > >>>>>>>      }
> > > > > > >>>>>>>
> > > > > > >>>>>>
> > > > > > >>>>>>it looks like the last 2 hunks are not available in the
> > > > > > >>>>>>latest tar ball.
> > > > > > >>>>>
> > > > > > >>>>>Sorry about that. Added.
> > > > > > >>>>>
> > > > > > >>>>>>and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >>>>>
> > > > > > >>>>>Fixed by git.
> > > > > > >>>>>
> > > > > > >>>>>I also attached two more patches then converts usn into number
> > > > > > >>>>>and then use
> > > > > > >>>>>in filter entryUSN >= (current + 1) instead of (entryUSN >=
> > > > > > >>>>>cur) &&
> > > > > > >>>>>(entryUSN != cur) as per Thierry request.
> > > > > > >>>>>
> > > > > > >>>>>It could probably be squashed and replace some previous
> > > > > > >>>>>patches but I think
> > > > > > >>>>>sending it as new patches is easier for you to review, given
> > > > > > >>>>>the previous
> > > > > > >>>>>patches are basically acked.
> > > > > > >>>>
> > > > > > >>>>>+    errno = 0;
> > > > > > >>>>>      usn_number = strtoul(usn, &endptr, 10);
> > > > > > >>>>>-    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >>>>>-         && (usn_number > srv_opts->last_usn)) {
> > > > > > >>>>>-         srv_opts->last_usn = usn_number;
> > > > > > >>>>>+    if (endptr != NULL && *endptr != '\0') {
> > > > > > >>>>
> > > > > > >>>>Currently an empty string in usn would result in usn_number==0,
> > > > > > >>>>I wonder
> > > > > > >>>>if this is valid or if it would be better to error out in this
> > > > > > >>>>case?
> > > > > > >>>
> > > > > > >>>Zero value will not change anything since we always compare if
> > > > > > >>>the current
> > > > > > >>>usn value is lower than the new one, we switch them only in such
> > > > > > >>>case. So I
> > > > > > >>>think it is not necessary to error out in this case since
> > > > > > >>>strtoul succeeds.
> > > > > > >>>
> > > > > > >>>>
> > > > > > >>>>Otherwise all looks good, CI and Coverity test are currently
> > > > > > >>>>running.
> > > > > > >>>>
> > > > > > >>>>bye,
> > > > > > >>>>Sumit
> > > > > > >>>>
> > > > > > >>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN
> > > > > > >>>>>%s\n", usn);
> > > > > > >>>>>+        return;
> > > > > > >>>>>+    } else if (errno != 0) {
> > > > > > >>>>>+        ret = errno;
> > > > > > >>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s
> > > > > > >>>>>[%d]:
> > > > > > >>>>>%s\n",
> > > > > > >>>>>+              usn, ret, sss_strerror(ret));
> > > > > > >>>>>+        return;
> > > > > > >>>>>      }
> > > > > > >>
> > > > > > >>There are 4 failed test from freeipa sudo test suite.
> > > > > > >>
> > > > > > >>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > > > >>
> > > > > > >>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > > > >>
> > > > > > >>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > > > >>
> > > > > > >>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > > > > >>
> > > > > > >>
> > > > > > >>
> > > > > > >>=================================== FAILURES
> > > > > > >>===================================
> > > > > > >>______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask
> > > > > > >>______________
> > > > > > >>
> > > > > > >>self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > >>0x7fe0d7ace8d0>
> > > > > > >>
> > > > > > >>     def test_sudo_rule_restricted_to_one_hostmask(self):
> > > > > > >>         if self.__class__.skip_hostmask_based:
> > > > > > >>             raise pytest.skip("Hostmask could not be detected")
> > > > > > >>
> > > > > > >>         result1 = self.list_sudo_commands("testuser1")
> > > > > > >>>       assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > > > > >>E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > > > >>E        +  where '' = <pytest_multihost.transport.SSHCommand
> > > > > > >>object at 0x7fe0d7acee10>.stdout_text
> > > > > > >>
> > > > > > >>test_integration/test_sudo.py:295: AssertionError
> > > > > > >>______
> > > > > > >>TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > > > > >>
> > > > > > >>
> > > > > > >>self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > >>0x7fe0d79de990>
> > > > > > >>
> > > > > > >>     def
> > > > > > >>test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > > > > >>         result1 = self.list_sudo_commands("testuser1",
> > > > > > >>verbose=True)
> > > > > > >>>       assert "RunAsUsers: localuser" in result1.stdout_text
> > > > > > >
> > > > > > >There are just too many options. I tested RunAsUsers and RunAsUsers
> > > > > > >but only with IPA users and groups. I looks like the handling for the
> > > > > > >ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> > > > > > >
> > > > > > That's the purpose of automated tests :-)
> > > > > > To cover all use-case.
> > > > >
> > > > > These use cases are omitted by design.
> > > >
> > > > We might still need to at least document this change. But with a
> > > > recent IPA
> > > > server, I see:
> > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > --externaluser=jhrozek readfiles
> > > >    ipa: ERROR: invalid 'externaluser': this option has been deprecated.
> > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > --runasexternalgroup=jhrozek readfiles
> > > >    ipa: ERROR: invalid 'runasexternalgroup': this option has been
> > > > deprecated.
> > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > --runasexternaluser=jhrozek readfiles
> > > >    ipa: ERROR: invalid 'runasexternaluser': this option has been
> > > > deprecated.
> > >
> > > We might ask whether Dreamworks need this feature.
> > > Otherwise these patches will not help them.
> >
> > We talked with Jakub about it and decided that we will add support of
> > ipaSudoRunAsExt* attributes. So with this patch set only hostmask test
> > should fail, we will not add support for hostmask unless requested.

Even if we ignore hostmask issue there is stil issue with local user/group

test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group

Full error:
=================================== FAILURES ===================================
______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d8b399c10>

def test_sudo_rule_restricted_to_one_hostmask(self):
        if self.__class__.skip_hostmask_based:
            raise pytest.skip("Hostmask could not be detected")

result1 = self.list_sudo_commands("testuser1")
>   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text

E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7f9d8b37c910>.stdout_text

test_integration/test_sudo.py:295: AssertionError
______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d88f0a990>

def test_sudo_rule_restricted_to_running_as_single_local_user(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
>   assert "RunAsUsers: localuser" in result1.stdout_text

E       assert 'RunAsUsers: localuser' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d88f0a490>.stdout_text

test_integration/test_sudo.py:418: AssertionError
_____ TestSudo.test_sudo_rule_restricted_to_run_as_users_from_local_group ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d8b37c690>

def test_sudo_rule_restricted_to_run_as_users_from_local_group(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
>   assert "RunAsUsers: %localgroup" in result1.stdout_text

E       assert 'RunAsUsers: %localgroup' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d8b487910>.stdout_text

test_integration/test_sudo.py:460: AssertionError
_____ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_group ______

self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d89109d10>

def test_sudo_rule_restricted_to_running_as_single_local_group(self):
        result1 = self.list_sudo_commands("testuser1", verbose=True)
        assert "RunAsUsers: root" in result1.stdout_text
>   assert "RunAsGroups: localgroup" in result1.stdout_text

E       assert 'RunAsGroups: localgroup' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d89109ed0>.stdout_text

test_integration/test_sudo.py:503: AssertionError
==================== 4 failed, 71 passed in 736.12 seconds =====================

LS

[Lukas Slebodnik]

On (18/01/16 15:25), Lukas Slebodnik wrote:
> On (18/01/16 13:05), Pavel Březina wrote:
> > On 01/18/2016 12:23 PM, Pavel Březina wrote:
> > > On 01/18/2016 10:54 AM, Lukas Slebodnik wrote:
> > > > On (18/01/16 10:18), Jakub Hrozek wrote:
> > > > > On Mon, Jan 18, 2016 at 10:03:30AM +0100, Pavel Březina wrote:
> > > > > > On 01/15/2016 04:31 PM, Lukas Slebodnik wrote:
> > > > > > >On (15/01/16 15:06), Sumit Bose wrote:
> > > > > > >>On Fri, Jan 15, 2016 at 02:26:48PM +0100, Lukas Slebodnik wrote:
> > > > > > >>>On (15/01/16 10:21), Pavel Březina wrote:
> > > > > > >>>>On 01/14/2016 06:37 PM, Sumit Bose wrote:
> > > > > > >>>>>On Thu, Jan 14, 2016 at 01:55:39PM +0100, Pavel Březina wrote:
> > > > > > >>>>>>On 01/13/2016 05:12 PM, Sumit Bose wrote:
> > > > > > >>>>>>>>
> > > > > > >>>>>>>>@@ -1018,7 +1019,6 @@ rules_iterator(hash_entry_t *item,
> > > > > > >>>>>>>>
> > > > > > >>>>>>>>      if (ctx == NULL) {
> > > > > > >>>>>>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>>>          return false;
> > > > > > >>>>>>>>      }
> > > > > > >>>>>>>>
> > > > > > >>>>>>>>@@ -1066,7 +1066,6 @@ cmdgroups_iterator(hash_entry_t *item,
> > > > > > >>>>>>>>
> > > > > > >>>>>>>>      if (ctx == NULL) {
> > > > > > >>>>>>>>          DEBUG(SSSDBG_CRIT_FAILURE, "Bug: ctx is NULL\n");
> > > > > > >>>>>>>>-        ctx->ret = ERR_INTERNAL;
> > > > > > >>>>>>>>          return false;
> > > > > > >>>>>>>>      }
> > > > > > >>>>>>>>
> > > > > > >>>>>>>
> > > > > > >>>>>>>it looks like the last 2 hunks are not available in the
> > > > > > >>>>>>>latest tar ball.
> > > > > > >>>>>>
> > > > > > >>>>>>Sorry about that. Added.
> > > > > > >>>>>>
> > > > > > >>>>>>>and there are some trailing whitespaces in patch 9 at line 41.
> > > > > > >>>>>>
> > > > > > >>>>>>Fixed by git.
> > > > > > >>>>>>
> > > > > > >>>>>>I also attached two more patches then converts usn into number
> > > > > > >>>>>>and then use
> > > > > > >>>>>>in filter entryUSN >= (current + 1) instead of (entryUSN >=
> > > > > > >>>>>>cur) &&
> > > > > > >>>>>>(entryUSN != cur) as per Thierry request.
> > > > > > >>>>>>
> > > > > > >>>>>>It could probably be squashed and replace some previous
> > > > > > >>>>>>patches but I think
> > > > > > >>>>>>sending it as new patches is easier for you to review, given
> > > > > > >>>>>>the previous
> > > > > > >>>>>>patches are basically acked.
> > > > > > >>>>>
> > > > > > >>>>>>+    errno = 0;
> > > > > > >>>>>>      usn_number = strtoul(usn, &endptr, 10);
> > > > > > >>>>>>-    if ((endptr == NULL || (*endptr == '\0' && endptr != usn))
> > > > > > >>>>>>-         && (usn_number > srv_opts->last_usn)) {
> > > > > > >>>>>>-         srv_opts->last_usn = usn_number;
> > > > > > >>>>>>+    if (endptr != NULL && *endptr != '\0') {
> > > > > > >>>>>
> > > > > > >>>>>Currently an empty string in usn would result in usn_number==0,
> > > > > > >>>>>I wonder
> > > > > > >>>>>if this is valid or if it would be better to error out in this
> > > > > > >>>>>case?
> > > > > > >>>>
> > > > > > >>>>Zero value will not change anything since we always compare if
> > > > > > >>>>the current
> > > > > > >>>>usn value is lower than the new one, we switch them only in such
> > > > > > >>>>case. So I
> > > > > > >>>>think it is not necessary to error out in this case since
> > > > > > >>>>strtoul succeeds.
> > > > > > >>>>
> > > > > > >>>>>
> > > > > > >>>>>Otherwise all looks good, CI and Coverity test are currently
> > > > > > >>>>>running.
> > > > > > >>>>>
> > > > > > >>>>>bye,
> > > > > > >>>>>Sumit
> > > > > > >>>>>
> > > > > > >>>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN
> > > > > > >>>>>>%s\n", usn);
> > > > > > >>>>>>+        return;
> > > > > > >>>>>>+    } else if (errno != 0) {
> > > > > > >>>>>>+        ret = errno;
> > > > > > >>>>>>+        DEBUG(SSSDBG_MINOR_FAILURE, "Unable to convert USN %s
> > > > > > >>>>>>[%d]:
> > > > > > >>>>>>%s\n",
> > > > > > >>>>>>+              usn, ret, sss_strerror(ret));
> > > > > > >>>>>>+        return;
> > > > > > >>>>>>      }
> > > > > > >>>
> > > > > > >>>There are 4 failed test from freeipa sudo test suite.
> > > > > > >>>
> > > > > > >>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> > > > > > >>>
> > > > > > >>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> > > > > > >>>
> > > > > > >>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
> > > > > > >>>
> > > > > > >>>test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-group
> > > > > > >>>
> > > > > > >>>
> > > > > > >>>
> > > > > > >>>=================================== FAILURES
> > > > > > >>>===================================
> > > > > > >>>______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask
> > > > > > >>>______________
> > > > > > >>>
> > > > > > >>>self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > >>>0x7fe0d7ace8d0>
> > > > > > >>>
> > > > > > >>>     def test_sudo_rule_restricted_to_one_hostmask(self):
> > > > > > >>>         if self.__class__.skip_hostmask_based:
> > > > > > >>>             raise pytest.skip("Hostmask could not be detected")
> > > > > > >>>
> > > > > > >>>         result1 = self.list_sudo_commands("testuser1")
> > > > > > >>>>       assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
> > > > > > >>>E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> > > > > > >>>E        +  where '' = <pytest_multihost.transport.SSHCommand
> > > > > > >>>object at 0x7fe0d7acee10>.stdout_text
> > > > > > >>>
> > > > > > >>>test_integration/test_sudo.py:295: AssertionError
> > > > > > >>>______
> > > > > > >>>TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
> > > > > > >>>
> > > > > > >>>
> > > > > > >>>self = <ipatests.test_integration.test_sudo.TestSudo object at
> > > > > > >>>0x7fe0d79de990>
> > > > > > >>>
> > > > > > >>>     def
> > > > > > >>>test_sudo_rule_restricted_to_running_as_single_local_user(self):
> > > > > > >>>         result1 = self.list_sudo_commands("testuser1",
> > > > > > >>>verbose=True)
> > > > > > >>>>       assert "RunAsUsers: localuser" in result1.stdout_text
> > > > > > >>
> > > > > > >>There are just too many options. I tested RunAsUsers and RunAsUsers
> > > > > > >>but only with IPA users and groups. I looks like the handling for the
> > > > > > >>ipasudorunasextuser and ipasudorunasextgroup attributes are missing.
> > > > > > >>
> > > > > > >That's the purpose of automated tests :-)
> > > > > > >To cover all use-case.
> > > > > >
> > > > > > These use cases are omitted by design.
> > > > >
> > > > > We might still need to at least document this change. But with a
> > > > > recent IPA
> > > > > server, I see:
> > > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > > --externaluser=jhrozek readfiles
> > > > >    ipa: ERROR: invalid 'externaluser': this option has been deprecated.
> > > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > > --runasexternalgroup=jhrozek readfiles
> > > > >    ipa: ERROR: invalid 'runasexternalgroup': this option has been
> > > > > deprecated.
> > > > >    [jhrozek@unidirect] sssd $ [(review)] ipa sudorule-mod
> > > > > --runasexternaluser=jhrozek readfiles
> > > > >    ipa: ERROR: invalid 'runasexternaluser': this option has been
> > > > > deprecated.
> > > >
> > > > We might ask whether Dreamworks need this feature.
> > > > Otherwise these patches will not help them.
> > >
> > > We talked with Jakub about it and decided that we will add support of
> > > ipaSudoRunAsExt* attributes. So with this patch set only hostmask test
> > > should fail, we will not add support for hostmask unless requested.
>
> Even if we ignore hostmask issue there is stil issue with local user/group
>
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-one-hostmask
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-running-as-single-local-user
> test-integration-test-sudo-TestSudo-test-sudo-rule-restricted-to-run-as-users-from-local-group
>
> Full error:
> =================================== FAILURES ===================================
> ______________ TestSudo.test_sudo_rule_restricted_to_one_hostmask ______________
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d8b399c10>
>
> def test_sudo_rule_restricted_to_one_hostmask(self):
>        if self.__class__.skip_hostmask_based:
>            raise pytest.skip("Hostmask could not be detected")
>    result1 = self.list_sudo_commands("testuser1")
> >   assert "(ALL : ALL) NOPASSWD: ALL" in result1.stdout_text
>
> E       assert '(ALL : ALL) NOPASSWD: ALL' in ''
> E        +  where '' = <pytest_multihost.transport.SSHCommand object at 0x7f9d8b37c910>.stdout_text
>
> test_integration/test_sudo.py:295: AssertionError
> ______ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_user ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d88f0a990>
>
> def test_sudo_rule_restricted_to_running_as_single_local_user(self):
>        result1 = self.list_sudo_commands("testuser1", verbose=True)
> >   assert "RunAsUsers: localuser" in result1.stdout_text
>
> E       assert 'RunAsUsers: localuser' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d88f0a490>.stdout_text
>
> test_integration/test_sudo.py:418: AssertionError
> _____ TestSudo.test_sudo_rule_restricted_to_run_as_users_from_local_group ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d8b37c690>
>
> def test_sudo_rule_restricted_to_run_as_users_from_local_group(self):
>        result1 = self.list_sudo_commands("testuser1", verbose=True)
> >   assert "RunAsUsers: %localgroup" in result1.stdout_text
>
> E       assert 'RunAsUsers: %localgroup' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d8b487910>.stdout_text
>
> test_integration/test_sudo.py:460: AssertionError
> _____ TestSudo.test_sudo_rule_restricted_to_running_as_single_local_group ______
>
> self = <ipatests.test_integration.test_sudo.TestSudo object at 0x7f9d89109d10>
>
> def test_sudo_rule_restricted_to_running_as_single_local_group(self):
>        result1 = self.list_sudo_commands("testuser1", verbose=True)
>        assert "RunAsUsers: root" in result1.stdout_text
> >   assert "RunAsGroups: localgroup" in result1.stdout_text
>
> E       assert 'RunAsGroups: localgroup' in 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n'
> E        +  where 'Matching Defaults entries for testuser1 on nec-em17:\n    !visiblepw, env_reset, env_keep="COLORS DISPLAY HOSTNAME HI... may run the following commands on nec-em17:\n    RunAsUsers: root\n    Options: !authenticate\n    Commands:\n\tALL\n' = <pytest_multihost.transport.SSHCommand object at 0x7f9d89109ed0>.stdout_text
>
> test_integration/test_sudo.py:503: AssertionError
> ==================== 4 failed, 71 passed in 736.12 seconds =====================

I would like to appologize for confusion the latest patches
works fine for local{user,group}

There is only issue with hostmask but we do not pland to add support now.

+1
tested by: LS (I didn't read patches)

LS

[Jakub Hrozek]

On Tue, Jan 19, 2016 at 02:13:14PM +0100, Lukas Slebodnik wrote:
> I would like to appologize for confusion the latest patches
> works fine for local{user,group}
>
> There is only issue with hostmask but we do not pland to add support now.
>
> +1
> tested by: LS (I didn't read patches)

Thank you very much for testing

Pushed upstream..

master:
        a7d2b4f157194c14bc4a40c74f6416b82befa460
        1476d5348fcf387e7481d833becbd993d91f8019
        f58ffb26aeaae0642a149643672fa59ec01a3a36
        8da71a9d5eebe7690b66fde8bfad195d5e3cc629
        8bd44a13de231d025882810c720dd07ca4ee564d
        43bbf5b158ec3152806791ca49ae224ee978de24
        3ff3bb43ae6509905bbf7fa6540c44cdbbd0f738
        cc7f9b639144183eb4f8bd86e5bed077da7d4e35
        ad5a48c4947183fda49308259e3411d17a8b0a13
        d06cc0974e59cd6cf1da45cc8c60d6e822b731c2
        9630a4614ba4d5f68e967d4e108893550a996f30
        a641a13889d617aca6bd998025e9087e822ff7f0
        4ddd5591c50e27dffa55f03fbce0dcc85cd50a8b
        cc7766c8456653ab5d7dedbf432cb1711a905804
        ed8650be18af26b7bf389e1246f7e8cdb363f829
        a2057618f30a3c64bdffb35a2ef3c2ba148c8a03
        0f04241fc90f134af0272eb0999e75fb6749b595
        a6dd4a6c55773e81490dcafd61d4b9782705e9bf
        b407fe0474a674bb42f0f42ab47c7f530a07a367
        cad751beaa12e34e15565bc413442b1e80ac0c29
        e085a79acfcd5331b6f99748e21765579a9a99f2
        85feb8d77a2c832787880944e02104846c4d5376
        68abbe716bed7c8d6790d9bec168ef44469306a1
        e9ae5cd285dcc8fa232e16f9c7a29f18537272f2
        1d3f5fc2802c218916e6d6bc98eeaed79c66bafe
        92ec40e6aa25f75903ffdb166a8ec56b67bfd77d
        d0599eaa9369fd867953e3c58b8d7bb445525ff5 
    sssd-1-13:
        4af65fad63a70de5515080b77bd965646e1e3fc9
        7315eed1adc4e83675b3f72a5c3fa014374bbc6d
        f58ab319363e128f817d90eb7c160e7dc9abee6c
        3d0883f56ed78b9299a3c1e21a7b16e7279ae20c
        f485bbc2c1e28f51b35f546e160a6174e6644d3a
        fe7349304170b827ddef2bdb8f858c828ddb48c7
        cab3b09bf6d9108d8498ca94c19844fa001fb827
        eac510ccc86d1d45b2cc1f0b3f9554b0a9717b78
        1dbb036f0dbe65ceba2f9eae0a1e56848149263e
        6fc3ee299f2d7103aa7357f4a91973883c487888
        2d4bc2fabba94291745112f3c9d4143d893362fb
        43f4ecef75752cc531697a4e215903657c64ca97
        599e8862a0bdd53db5dea0940ca8ae374d167846
        e1b288a9b0c40b299455ca81d0eabe1d73b31ae3
        33d4b29fd45c8f2e138121c472c541a089816d7c
        6494d7a987d895744b3ef8839866e1891df17659
        01db59be8c1175503bec23b480799f9375903884
        b6c32aeed9e02017142a88499955e6a72b103acf
        530e6e0fb086235658bd6387d83e5eddd393ef77
        04e2ea460daa6edc0b6f6ff67d14a1fd3d03e235
        4f833dc1f280f861343b022b703470a5bdddaba6
        216b846cb1acae47b80fd61fc9474b08eabe13b3
        bdfe78351ae09790205deac09027a511d4ee03cb
        c548a507e68cfe1c2ebb98e98d59101d4c4513de
        339dcc48e57d4c38fb4bc5be73cf15cf9dd46908
        f5520fc2c5e8c6a2bc5d0e73900d734e6d862545
        3063486d01c0be2ef64b884a20bfbc7f8cfd7105
        1227cd003e434ba974b6b08280f635047263a450
        69be1acf52839a8b32763397f9531f8fc4f60569