[SSSD] [PATCHES][PRELIMINARY] sudo - new design

Pavel Březina pbrezina at redhat.com
Wed Jun 27 12:57:52 UTC 2012


On 06/27/2012 02:46 PM, Jan Zelený wrote:
> 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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 55363 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120627/a39632f7/attachment.bin>


More information about the sssd-devel mailing list