Dne středa 27 června 2012 13:31:42, Pavel Březina napsal(a):
> On 06/27/2012 11:05 AM, Jan Zelený wrote:
>> Dne úterý 26 června 2012 18:22:15, Pavel Březina napsal(a):
>>> On 06/26/2012 03:45 PM, Jan Zelený wrote:
>>>> Dne úterý 26 června 2012 10:08:15, Pavel Březina napsal(a):
>>>>> On 06/21/2012 05:32 PM, Pavel Březina wrote:
>>>>>> On 3.6.2012 22:17, Pavel Březina wrote:
>>>>>>> On 14.5.2012 22:37, Jakub Hrozek wrote:
>>>>>>>> On Mon, May 14, 2012 at 06:39:30PM +0200, Pavel Březina
wrote:
>>>>>>>>> On 9.5.2012 17:07, Pavel Březina wrote:
>>>>>>>>>> On 05/07/2012 07:13 PM, Pavel Březina wrote:
>>>>>>>>>>> On 04/23/2012 06:34 PM, Pavel Březina wrote:
>>>>>>>>>>>> On 04/23/2012 05:04 PM, Pavel Březina
wrote:
>>>>>>>>>>>>> Hello everybody,
>>>>>>>>>>>>> here is the first set of patches for
the new sudo clothes. [1]
>>>>>>>>>>>>>
>>>>>>>>>>>>> As it does not touch the responder, I
believe it does not
>>>>>>>>>>>>> require
>>>>>>>>>>>>> the
>>>>>>>>>>>>> patches from the other preliminary
sudo thread (already acked).
>>>>>>>>>>>>> However
>>>>>>>>>>>>> it was written atop them.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [2/10]
>>>>>>>>>>>>> This is the main change for the async
processing of the sudo
>>>>>>>>>>>>> rules. It
>>>>>>>>>>>>> make sdap_sudo_refresh_send() more
generic by adding there two
>>>>>>>>>>>>> filters:
>>>>>>>>>>>>> ldap_filter - used for search in the
LDAP
>>>>>>>>>>>>> sysdb_filter - used for a deletion of
the rules from sysdb
>>>>>>>>>>>>>
>>>>>>>>>>>>> This way we can have many different
refresh styles without
>>>>>>>>>>>>> touching
>>>>>>>>>>>>> this fundamental function.
>>>>>>>>>>>>>
>>>>>>>>>>>>> From this patch further the
provider will return an error to
>>>>>>>>>>>>> the
>>>>>>>>>>>>>
>>>>>>>>>>>>> responder making it unusable.
>>>>>>>>>>>>>
>>>>>>>>>>>>> [...]
>>>>>>>>>>>>> The rest of the patches has a
self-describing subject. Don't be
>>>>>>>>>>>>> afraid
>>>>>>>>>>>>> of the count, these patches are very
small :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> The next set of patches is where the
fun begin, so be patient
>>>>>>>>>>>>> :-)
>>>>>>>>>>>>>
>>>>>>>>>>>>> [1]
>>>>>>>>>>>>>
https://fedorahosted.org/sssd/wiki/DesignDocs/SUDOCachingRules
>>>>>>>>>>>>
>>>>>>>>>>>> Ok, I made a mistake already in the 3rd
patch, I'm sending it
>>>>>>>>>>>> again :)
>>>>>>>>>>>
>>>>>>>>>>> Sending new set of patches if you want to
take a look.
>>>>>>>>>>>
>>>>>>>>>>> Full periodical updates should run fine,
there are preparations
>>>>>>>>>>> for
>>>>>>>>>>> the
>>>>>>>>>>> smart update. I'm going to ignore
modifyTimestamp for now and use
>>>>>>>>>>> only
>>>>>>>>>>> entryUSN if available.
>>>>>>>>>>>
>>>>>>>>>>> It also does not filter the rules by host, I
plan to add it as
>>>>>>>>>>> one
>>>>>>>>>>> of
>>>>>>>>>>> the last things.
>>>>>>>>>>
>>>>>>>>>> The provider part is now complete except the
above given
>>>>>>>>>> limitations.
>>>>>>>>>
>>>>>>>>> Ok, the major changes are done in this patch set.
>>>>>>>>>
>>>>>>>>> What left is:
>>>>>>>>> - download rules per host, this only appends a filter
to the search
>>>>>>>>> so
>>>>>>>>> it should not invoke any logic change
>>>>>>>>>
>>>>>>>>> - it ignores modifyTimestamp - simo suggested to use
it in smart
>>>>>>>>> refresh when enabled in configuration and USN not
available, but do
>>>>>>>>> we really want to use it? There is no other part in
sssd that uses
>>>>>>>>> it
>>>>>>>>> (or am I wrong?).
>>>>>>>>
>>>>>>>> Pavel, I won't be able to review the huge patchset in
a whole, so I
>>>>>>>> started with the first couple of patches. Do you know
offhand if
>>>>>>>> there is
>>>>>>>> a particular point where several patches could be applied
and the
>>>>>>>> provider
>>>>>>>> would work so that when first N patches are acked, we can
push them
>>>>>>>> and
>>>>>>>> decrease the amount of rebasing?
>>>>>>>>
>>>>>>>> 0001: sudo ldap provider: move async routines to
sdap_async_sudo.c
>>>>>>>> Ack, this patch just moves code around
>>>>>>>>
>>>>>>>> 0002: sudo ldap provider: give sdap_sudo_refresh_send()
search and
>>>>>>>> purge filters
>>>>>>>> Ack. The sudo provider is not functional after this patch
until
>>>>>>>> patch
>>>>>>>> #8,
>>>>>>>> but I think that's OK. This split is as good as any.
>>>>>>>>
>>>>>>>> 0003: confdb: add entry_cache_sudo_timeout option
>>>>>>>> Nack, the new option is missing from the configAPI
>>>>>>>> Also, I assume the new option is going to be used when
the responder
>>>>>>>> changes land? So far, it's only read, not used.
>>>>>>>
>>>>>>> Your assumption is correct.
>>>>>>> Done. I have also fixed it in subsequent patches. I really
need to
>>>>>>> write it somewhere on my desk :-)
>>>>>>>
>>>>>>>> 0004: sudo ldap provider: add sysdb ctx in
sdap_sudo_refresh_state
>>>>>>>> Ack, but I don't get the point?
>>>>>>>>
>>>>>>>> 0005: sudo ldap provider: add domain info in
sdap_sudo_refresh_state
>>>>>>>> Same as 0004, what's the intent?
>>>>>>>
>>>>>>> Just to shorten access to these contexts.
>>>>>>>
>>>>>>>> 0006: sudo ldap provider: add expiration time to each
rule
>>>>>>>> Ack
>>>>>>>>
>>>>>>>> 0007: sysdb_sudo_set_refresh_time
>>>>>>>> Looks good, but do you anticipate that
>>>>>>>> sysdb_sudo_{get,set}_refresh_time
>>>>>>>> would be called with any other attribute name than
>>>>>>>> SYSDB_SUDO_AT_LAST_FULL_REFRESH?
>>>>>>>
>>>>>>> Originally it should contain SYSDB_SUDO_AT_LAST_SMART_REFRESH
but I
>>>>>>> found out later that it is not necessary.
>>>>>>>
>>>>>>> The intention was to use that value to compute when the first
smart
>>>>>>> refresh should be scheduled after sssd is started. However,
it needs
>>>>>>> to
>>>>>>> know the current highest USN value (which would be a little
tricky).
>>>>>>> Therefore I'm doing full refresh at first and then enable
periodical
>>>>>>> smart updates (as we have discussed on site).
>>>>>>>
>>>>>>>> 0008: sudo ldap provider: provide API for full refresh
>>>>>>>> +
>>>>>>>> + /* free filters with the request */
>>>>>>>> + talloc_steal(subreq, ldap_filter);
>>>>>>>> + talloc_steal(subreq, sysdb_filter);
>>>>>>>> +
>>>>>>>>
>>>>>>>> Do the filters need to outlive the top-level request? It
doesn't
>>>>>>>> look
>>>>>>>> like
>>>>>>>> it's the case, but if so, then I would say it's
more natural to
>>>>>>>> strdup
>>>>>>>> them
>>>>>>>> in the subreq, so that other potential consumers of
>>>>>>>> sdap_sudo_refresh_send()
>>>>>>>> don't need to remember to steal the filters.
>>>>>>>
>>>>>>> Well, you don't really need to steal them. But I
don't have any use
>>>>>>> for
>>>>>>> them outside the subreq so I think it is correct to make
them
>>>>>>> deallocate with subreq.
>>>>>>>
>>>>>>> Those are constant input data, so there is no need to strdup
them.
>>>>>>>
>>>>>>>> _recv should come after _done in the source file,
otherwise the code
>>>>>>>> reads funny.
>>>>>>>
>>>>>>> Ok. There would be unworthy rebasing troubles, thus I have
moved it
>>>>>>> in
>>>>>>> a new patch.
>>>>>>>
>>>>>>>> In sdap_sudo_full_refresh_done(), you read the req and
state
>>>>>>>> variables
>>>>>>>> twice
>>>>>>>> in the patch, but not in the resulting code, so I assume
it was
>>>>>>>> fixed
>>>>>>>> in
>>>>>>>> a follow-up patch. Is it too much trouble to fix? If it
is, don't
>>>>>>>> bother.
>>>>>>>
>>>>>>> This is an error from one of many rebases I have done.
>>>>>>> Fixed.
>>>>>>>
>>>>>>>> I think you meant to quit the request here with an error?
If not,
>>>>>>>> please
>>>>>>>> put a comment saying that you ignore an error
intentionally:
>>>>>>>> + if (ret != EOK) {
>>>>>>>> + DEBUG(SSSDBG_MINOR_FAILURE, ("Unable to save time
of "
>>>>>>>> + "a successful full refresh\n"));
>>>>>>>> + }
>>>>>>>
>>>>>>> I have added the following comment:
>>>>>>> /* this is only a minor error that does not affect the
functionality,
>>>>>>> * therefore there is no need to report it with
tevent_req_error()
>>>>>>> * which would cause problems in the consumers */
>>>>>>>
>>>>>>>> 0009: sudo ldap provider: add support for on demand full
refresh
>>>>>>>> + default:
>>>>>>>> + DEBUG(SSSDBG_CRIT_FAILURE, ("Invalid request type:
%d\n",
>>>>>>>> + sudo_req->type));
>>>>>>>> + }
>>>>>>>> I think you should abort the request with something like
EINVAL
>>>>>>>> here.
>>>>>>>> I know that the code would not continue because of the
following
>>>>>>>> (req
>>>>>>>> != NULL)
>>>>>>>> check but the OOM error would be pretty confusing for the
caller.
>>>>>>>
>>>>>>> Yes, definitely. The same problem was also in
sdap_sudo_reply() where
>>>>>>> ret would be uninitialized in ret != EOK condition.
>>>>>>> Both fixed.
>>>>>>>
>>>>>>>> 0010: provide API for refresh of specific rules
>>>>>>>> Same comment with stealing the filters as with patch #9.
Frankly, I
>>>>>>>> don't
>>>>>>>> really like how you wrapped sdap_sudo_refresh_send().
Wouldn't it be
>>>>>>>> more
>>>>>>>> readable to create both _done and _recv?
>>>>>>>
>>>>>>> I found it necessary after all, because I needed to create
there a
>>>>>>> new
>>>>>>> request. It is converted to it in one of the last patches.
>>>>>>>
>>>>>>>> 0011: sudo ldap provider: add support for on demand
refresh of
>>>>>>>> specific rules
>>>>>>>> Ack
>>>>>>>>
>>>>>>>> I'm going to continue tomorrow. I also haven't
done any testing yet,
>>>>>>>> just
>>>>>>>> reading the code.
>>>>>>>
>>>>>>> I have also fixed typo in man page and added new patch that
handles
>>>>>>> modification of the highest USN value after a specific rules
refresh.
>>>>>>>
>>>>>>> New patches are attached.
>>>>>>>
>>>>>>> Thanks for the review. Keep coming :-)
>>>>>>
>>>>>> Rebased on the current master and patches from "sudo: send
username
>>>>>> and
>>>>>> uid while requesting default options" thread.
>>>>>
>>>>> Added support for modifyTimestamp as a fallback when USN attributes
are
>>>>> not available (squashed to #21).
>>>>>
>>>>> And I have also noticed that 'entry_cache_sudo_timeout' broke
the
>>>>> config
>>>>> test. The fix is squashed to #3.
>>>>
>>>> As a general comment: please make all commit messages more verbose. They
>>>> should completely describe the new functionality and explain motivation
>>>> for
>>>> major changes.
>>>
>>> Ok. I often forget that you can't see in my head. Next time :-)
>>>
>>>> Also please try to merge some patches together. It is very difficult to
>>>> undertand how are patches bound together and some are clearly closely
>>>> bound
>>>> together. I specifically pointed it out in couple places.
>>>>
>>>> Patch #0001: Ack (Jakub)
>>>> Patch #0002: Ack (Jakub)
>>
>> Nack,
>> I don't think it's a good idea to continue operations if strdup() on
>> sysdb_filter failed. You should probably return with an error i such case.
>
> Done.
>
>>>> Patch #0003: Ack
>>>> Patch #0004: Ack (Jakub)
>>>> Patch #0005: Ack (Jakub)
>>>> Patch #0006: Ack (Jakub)
>>>> Patch #0007: Ack
>>>> Patch #0008: Nack,
>>>> I don't like stealing those two variables either. The correct
approach
>>>> would be to strdup them in sdap_sudo_refresh_send() and free both
>>>> originals instead of stealing them. Also it is better (at least for the
>>>> sake of consistency) to use state as the original context instead of the
>>>> req.
>>>
>>> Done and done.
>>
>> Nack,
>> I'm sorry I didn't catch this in previous review but as I already
>> commented in other patches, please don't free req upon an error, rather
>> set an error and return the req. If you have suspicion you have similar
>> code elsewhere, please change it there as well.
>
> Done. And also fixed in other places.
>
>>>> Patch #0009: Ack
>>>> Patch #0010: Nack,
>>>> The comment about stealing filter at the end of
>>>> sdap_sudo_rules_refresh_send() still stands, even after the subsequent
>>>> patch.
>>>
>>> Done.
>>>
>>>> At the end of the patch you have two constants with the same content. If
>>>> you don't anticipate them to have different values, please merge
them
>>>> back into one.
>>>
>>> Done.
>>
>> Ack
>>
>>>> Patch #0011: Ack
>>>> Patch #0012: Nack,
>>>> I don't get why you set sudo_req->rules = NULL while deleting
similar
>>>> statements for uid and groups. I'd prefer if you deleted this
completely
>>>> since the structure is zeroed anyway.
>>>
>>> Because uid and groups are deprecated (I had to left them in the struct
>>> so it can be still compiled, but they are deleted in the clean up
>>> patch). But yes, it can be removed, so - done.
>>
>> Ack
>>
>>>> Patch #0013: Nack,
>>>> I don't like skipping the first argument with
dbus_message_iter_next().
>>>> Better approach would be to use the iterator right away and read the
>>>> type
>>>> with it as well. This will be a solution less susceptible to future
>>>> errors if for some reason another argument would come right after type.
>>>
>>> Done.
>>>
>>>> Nitpick: According to DBus documentation, the type of rule should be
>>>> const
>>>> char *
>>>
>>> Done.
>>
>> Ack
>>
>>>> Patch #0014: Ack
>>>> Patch #0015: Tentative Ack
>>
>> Ack
>>
>>>> Patch #0016: Nack,
>>>> Please don't free req in case of error performing tevent_add_timer()
in
>>>> sdap_sudo_timer_send(), the practice we use is to set an error on that
>>>> request and return it if possible.
>>>
>>> Done.
>>
>> Ack
>>
>>>> Patch #0017: Tentative Ack
>>
>> Ack
>>
>>>> Patch #0018: Nack,
>>>> I'm not sure what is the first part (before the "schedule"
label)
>>>> supposed
>>>> to do exactly but I suspect those ifs are somewhat wrong. Isn't the
>>>> function supposed to not schedule another round if the previous was ok?
>>>> If not then I suppose you don't need them at all.
>>>
>>> If the timer went wrong, we don't care and we will just schedule a new
>>> one. But it is missing a debug message. Added.
>>
>> Ack
>>
>>>> Patch #0019: Nack, please merge with #0018
>>>
>>> Done.
>>>
>>>> Patch #0020: Nack, please merge this patch with #0015
>>>
>>> Done.
>>>
>>>> This is the first logical part of SUDO patch set. I suggest we push it
>>>> after it is finished. It is logically independent on the second part,
>>>> the
>>>> only thing missing would be documentation. Pushing this independently
>>>> will make review a little bit more easy.
>>>>
>>>> Thanks
>>>> Jan
>>>
>>> Thank you for the review!
>>> Corrected patches are attached.
>>>
>>> The tar ball also contains foundations for host based filtering.
>>> It introduces several new options to configure this feature manually
>>> (hostnames, ip addresses, ...) as an addition to auto configuration
>>> which I will hopefully implement tomorrow or the day after. (FYI: That
>>> will be the end of the new implementation.)
>>
>> Thanks
>> Jan
>
> Thank you.
> Pavel.
Ack to patches #0001-#0018. I haven't tested them yet though.
Thanks
Jan
I have found an issue in the timer API. I did not clear timeout after
the request has been completed which cause SIGABRT in talloc function.
Patches are attached.