Re: [SSSD] [PATCH] Process SELinux mapping while offline

[Sumit Bose]

On Tue, Jan 08, 2013 at 02:51:07PM +0100, Jakub Hrozek wrote:
>  On Tue, Jan 08, 2013 at 01:37:50PM +0100, Sumit Bose wrote:
>  > On Tue, Jan 08, 2013 at 12:44:54PM +0100, Sumit Bose wrote:
>  > > On Mon, Jan 07, 2013 at 12:29:14AM +0100, Jakub Hrozek wrote:
>  > > > This patchset fixes the SELinux processing so that it works also offline
>  > > > for cases described in #1626 for example.
>  > > > 
>  > > > The code was architected in an extremely strange way where every request
>  > > > would store a per-request/per-user score attribute in the selinux mapping
>  > > > objects themselves and the responder would just pick the highest score.
>  > > > 
>  > > > At the same time, the scores were only calculated when the mappings were
>  > > > downloaded and only for rules that matched, which pretty much broke
> offline
>  > > > support. I intend to fix the architecture by only making the provider
> only
>  > > > download and store the rules and let the responder pick the context. But
> I'm
>  > > > not sure if I can do that right during the current time constraints. I
> filed
>  > > > #1743 to track that effort and I'll be sending some patches for master
> so
>  > > > far, but I'd like to include this patchset to fix the functionality at
> last.
>  > > > 
>  > > > I also noticed that the rules are downloaded on *every login*, without
>  > > > any timeout in between, so I filed #1744 to either reuse
>  > > > IPA_HBAC_REFRESH or introduce a similar timeout for performance reasons.
>  > > > 
>  > > > [PATCH 1/4] SYSDB: Remove duplicate selinux defines
>  > > > Some constants were defined in both sysdb.h and sysdb_selinux.h. This
>  > > > patch removes the duplicates.
>  > > 
>  > > ACK
>  > 
>  > sorry, ACK was too early, this patch breaks make check. Following patch
>  > fixes it:
>  > 
>  > diff --git a/src/providers/ipa/ipa_opts.h b/src/providers/ipa/ipa_opts.h
>  > index 01c32f4..66ca8ba 100644
>  > --- a/src/providers/ipa/ipa_opts.h
>  > +++ b/src/providers/ipa/ipa_opts.h
>  > @@ -28,6 +28,7 @@
>  >  #include "db/sysdb_sudo.h"
>  >  #include "db/sysdb_autofs.h"
>  >  #include "db/sysdb_services.h"
>  > +#include "db/sysdb_selinux.h"
>  >  
>  >  struct dp_option ipa_basic_opts[] = {
>  >      { "ipa_domain", DP_OPT_STRING, NULL_STRING, NULL_STRING },
>  > 
>  > 
>  > 
>  > bye,
>  > Sumit
>  > 
>  > > 
>  > > > 
>  > > > [PATCH 2/4] SYSDB: Split a function to read all SELinux maps
>  > > > This function will be reused in the DP to read maps from cache when
>  > > > offline.
>  > > 
>  > > ACK
>  > > 
>  > > > 
>  > > > [PATCH 3/4] SELINUX: Process maps even when offline
>  > > > The patch is quite big, but I couldn't split it in a meaningful way,
>  > > > sorry. The patch changes the ipa_get_selinux{send,recv} to only provide
>  > > > data independently on the offline stat and moves all the processing to
>  > > > the ipa_selinux_handler. The scores are calculated
>  > > 
>  > > see comments below
>  > > > 
>  > > > [PATCH 4/4] IPA: Rename IPA_CONFIG_SELINUX_DEFAULT_MAP
>  > > > The option IPA_CONFIG_SELINUX_DEFAULT_MAP was misnamed. It doesn't
>  > > > describe any map, but the default context the user should obtain when
>  > > > nothing else matches.
>  > > 
>  > > ACK
>  > > 
>  > > I'll run some test now. Is there anything I should test explicitly?
>  > > 
>  > > bye,
>  > > Sumit
>  > > 
>  
>  Thank you for the review. A new patchset is attached. 
I was able to reproduce the original issue and it is gone with the
patches.

ACK

bye,
Sumit