[PATCH] SELinux: Always use the default if it exists on the server

[Jakub Hrozek]

In order to test these patches properly, you need to be running the
latest patches that are currently on review on freeipa-devel. Ping me
off-line for access to such IPA client.

[PATCH 1/2] DB: Always write the SELinux object to sysdb
This is a fallout of the transaction processing refactoring.

There's no point in checking if the object already exists because we always
wipe the whole sysdb subtree. We were also immediatelly cancelling the
transaction because we'd jump to goto, even though it was with EOK.

[PATCH 2/2] SELinux: Always use the default if it exists on the server

https://fedorahosted.org/sssd/ticket/1513

This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045

During an e-mail discussion, it was decided that

* if the default is set in the IPA config object, the SSSD would use
      that default no matter what
    * if the default is not set (aka empty or missing), the SSSD would just
      use the system default and skip creating the login file altogether

[Pavel Březina]

On 09/13/2012 12:24 PM, Jakub Hrozek wrote:
> In order to test these patches properly, you need to be running the
> latest patches that are currently on review on freeipa-devel. Ping me
> off-line for access to such IPA client.
>
> [PATCH 1/2] DB: Always write the SELinux object to sysdb
> This is a fallout of the transaction processing refactoring.
>
> There's no point in checking if the object already exists because we always
> wipe the whole sysdb subtree. We were also immediatelly cancelling the
> transaction because we'd jump to goto, even though it was with EOK.

Ack.
> [PATCH 2/2] SELinux: Always use the default if it exists on the server
>
> https://fedorahosted.org/sssd/ticket/1513
>
> This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045
>
> During an e-mail discussion, it was decided that
>  * if the default is set in the IPA config object, the SSSD would use
>    that default no matter what
>  * if the default is not set (aka empty or missing), the SSSD would just
>    use the system default and skip creating the login file altogether

Nack.
> @@ -136,11 +136,9 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
>          goto fail;
>      }
> if (default_user != NULL && map_order != NULL) {
>    ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
>    if (ret != EOK) {
>        goto fail;
>    }
> ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> if (ret != EOK) {
>    goto fail;
> }

Are you sure that it is safe to remove the NULL checks? You are testing 
default_user inside sysdb_store_selinux_config(), but not map_order. 
There is a code path where map_order may be NULL.

[Jakub Hrozek]

On Thu, Sep 13, 2012 at 03:24:05PM +0200, Pavel Březina wrote:
> On 09/13/2012 12:24 PM, Jakub Hrozek wrote:
> > In order to test these patches properly, you need to be running the
> > latest patches that are currently on review on freeipa-devel. Ping me
> > off-line for access to such IPA client.
> >
> > [PATCH 1/2] DB: Always write the SELinux object to sysdb
> > This is a fallout of the transaction processing refactoring.
> >
> > There's no point in checking if the object already exists because we always
> > wipe the whole sysdb subtree. We were also immediatelly cancelling the
> > transaction because we'd jump to goto, even though it was with EOK.
>
> Ack.
> > [PATCH 2/2] SELinux: Always use the default if it exists on the server
> >
> > https://fedorahosted.org/sssd/ticket/1513
> >
> > This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045
> >
> > During an e-mail discussion, it was decided that
> > * if the default is set in the IPA config object, the SSSD would use
> >   that default no matter what
> > * if the default is not set (aka empty or missing), the SSSD would just
> >   use the system default and skip creating the login file altogether
>
> Nack.
> > @@ -136,11 +136,9 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
> >         goto fail;
> >     }
> > if (default_user != NULL && map_order != NULL) {
> >    ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> >    if (ret != EOK) {
> >        goto fail;
> >    }
> > ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> > if (ret != EOK) {
> >    goto fail;
> > }
>
> Are you sure that it is safe to remove the NULL checks? You are
> testing default_user inside sysdb_store_selinux_config(), but not
> map_order. There is a code path where map_order may be NULL.

Right, I should have kept the map_order check. I moved it into the save
function itself.

[Pavel Březina]

On 09/13/2012 04:48 PM, Jakub Hrozek wrote:
> On Thu, Sep 13, 2012 at 03:24:05PM +0200, Pavel Březina wrote:
> > On 09/13/2012 12:24 PM, Jakub Hrozek wrote:
> > > In order to test these patches properly, you need to be running the
> > > latest patches that are currently on review on freeipa-devel. Ping me
> > > off-line for access to such IPA client.
> > >
> > > [PATCH 1/2] DB: Always write the SELinux object to sysdb
> > > This is a fallout of the transaction processing refactoring.
> > >
> > > There's no point in checking if the object already exists because we always
> > > wipe the whole sysdb subtree. We were also immediatelly cancelling the
> > > transaction because we'd jump to goto, even though it was with EOK.
> >
> > Ack.
> > > [PATCH 2/2] SELinux: Always use the default if it exists on the server
> > >
> > > https://fedorahosted.org/sssd/ticket/1513
> > >
> > > This is a counterpart of the FreeIPA ticket https://fedorahosted.org/freeipa/ticket/3045
> > >
> > > During an e-mail discussion, it was decided that
> > >  * if the default is set in the IPA config object, the SSSD would use
> > >    that default no matter what
> > >  * if the default is not set (aka empty or missing), the SSSD would just
> > >    use the system default and skip creating the login file altogether
> >
> > Nack.
> > > @@ -136,11 +136,9 @@ static void ipa_selinux_handler_done(struct tevent_req *req)
> > >          goto fail;
> > >      }
> > > if (default_user != NULL && map_order != NULL) {
> > >    ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> > >    if (ret != EOK) {
> > >        goto fail;
> > >    }
> > > ret = sysdb_store_selinux_config(sysdb, default_user, map_order);
> > > if (ret != EOK) {
> > >    goto fail;
> > > }
> >
> > Are you sure that it is safe to remove the NULL checks? You are
> > testing default_user inside sysdb_store_selinux_config(), but not
> > map_order. There is a code path where map_order may be NULL.
>
> Right, I should have kept the map_order check. I moved it into the save
> function itself.

Ack.