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