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

Jan Zelený jzeleny at redhat.com
Tue Jun 26 13:45:54 UTC 2012


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.

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)
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.

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.

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.

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.

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.

Nitpick: According to DBus documentation, the type of rule should be const 
char *

Patch #0014: Ack
Patch #0015: Tentative 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.

Patch #0017: Tentative 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.

Patch #0019: Nack, please merge with #0018
Patch #0020: Nack, please merge this patch with #0015

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
-------------- 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/20120626/b700d09c/attachment.sig>


More information about the sssd-devel mailing list