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