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

Pavel Březina pbrezina at redhat.com
Tue Jun 26 08:08:15 UTC 2012


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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: patches.tgz
Type: application/x-compressed-tar
Size: 46858 bytes
Desc: not available
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120626/a766ee4c/attachment.bin>


More information about the sssd-devel mailing list