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.