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

Jan Zelený jzeleny at redhat.com
Wed Jun 27 12:46:31 UTC 2012


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
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: This is a digitally signed message part.
URL: <https://lists.fedorahosted.org/pipermail/sssd-devel/attachments/20120627/ef90434e/attachment.sig>


More information about the sssd-devel mailing list