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

Jakub Hrozek jhrozek at redhat.com
Mon May 14 20:37:00 UTC 2012


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.

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?

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?

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.

_recv should come after _done in the source file, otherwise the code
reads funny.

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.

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"));
+    }

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.

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?

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.



More information about the sssd-devel mailing list