Only create the SELinux login file if there are mappings on the server

[Jakub Hrozek]

[PATCH 1/2] Do not try to remove the temp login file if already
renamed

write_selinux_string() would try to unlink the temporary file even after
it was renamed. Failure to unlink the file would not be fatal, but would
produce a confusing error message.

Also don't use "0" for the default fd number, that's reserved for stdin.
Using -1 is safer.

[PATCH 2/2] Only create the SELinux login file if there are mappings
on the server

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

In case there are no rules on the IPA server, we must simply avoid generating
the login file. That would make us fall back to the system-wide default
defined in /etc/selinux/targeted/seusers.

The IPA default must be only used if there *are* rules on the server,
but none matches.

[PATCH] Only create the SELinux login file if there are mappings on the server

[Jakub Hrozek]

On Mon, Aug 06, 2012 at 10:51:39AM +0200, Jakub Hrozek wrote:
> [PATCH 1/2] Do not try to remove the temp login file if already
> renamed
>
> write_selinux_string() would try to unlink the temporary file even after
> it was renamed. Failure to unlink the file would not be fatal, but would
> produce a confusing error message.
>
> Also don't use "0" for the default fd number, that's reserved for stdin.
> Using -1 is safer.
>
> [PATCH 2/2] Only create the SELinux login file if there are mappings
> on the server
>
> https://fedorahosted.org/sssd/ticket/1455
>
> In case there are no rules on the IPA server, we must simply avoid generating
> the login file. That would make us fall back to the system-wide default
> defined in /etc/selinux/targeted/seusers.
>
> The IPA default must be only used if there *are* rules on the server,
> but none matches.

I forgot to include the [PATCH] tag. Sorry if I broke anyone's filters.

[Simo Sorce]

On Mon, 2012-08-06 at 10:51 +0200, Jakub Hrozek wrote:
> [PATCH 1/2] Do not try to remove the temp login file if already
> renamed
>
> write_selinux_string() would try to unlink the temporary file even
> after
> it was renamed. Failure to unlink the file would not be fatal, but
> would
> produce a confusing error message.
>
> Also don't use "0" for the default fd number, that's reserved for
> stdin.
> Using -1 is safer.
>
> [PATCH 2/2] Only create the SELinux login file if there are mappings
> on the server
>
> https://fedorahosted.org/sssd/ticket/1455
>
> In case there are no rules on the IPA server, we must simply avoid
> generating
> the login file. That would make us fall back to the system-wide
> default
> defined in /etc/selinux/targeted/seusers.
>
> The IPA default must be only used if there *are* rules on the server,
> but none matches.

Minor nitpicks, but I agree with the patches purposes, so auto-ACK when
nitpicks are addressed.
> From 87ec8c6d2bfeed404672ddca114cc8d0e1497346 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sun, 5 Aug 2012 22:03:11 +0200
> Subject: [PATCH 1/2] Do not try to remove the temp login file if
> already
>  renamed
>
> done:
> if (fd > 0) {
> if (fd > -1) {

use fd != -1 here, it makes it clear only -1 is a special value.
> From 90bd88e09e185dbcdaec17b9a8a619a0fdc9589a Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sun, 5 Aug 2012 22:37:09 +0200
> Subject: [PATCH 2/2] Only create the SELinux login file if there are
> mappings
>  on the server
> -static errno_t write_selinux_string(const char *username, char
> *string)
> +static char *selogin_path(TALLOC_CTX *mem_ctx, const char *username)
> +{
> return talloc_asprintf(mem_ctx, "%s/logins/%s",
>
> selinux_policy_root(),
>                       username);
>
> +}

I would use a macro here, a whole function for a oneliner seem
excessive.
> +static errno_t write_selinux_login_file(const char *username, char
> *string)
>  {
>      char *path = NULL;
>      char *tmp_path = NULL;
> @@ -379,8 +385,7 @@ static errno_t write_selinux_string(const char
> *username, char *string)
>          return ENOMEM;
>      }
> path = talloc_asprintf(tmp_ctx, "%s/logins/%s",
>
> selinux_policy_root(),
>                       username);
> path = selogin_path(tmp_ctx, username);
> if (path == NULL) {
>     ret = ENOMEM;
>     goto done;
>
> @@ -448,7 +453,27 @@ done:
>      return ret;
>  }
>
> -static errno_t get_selinux_string(struct pam_auth_req *preq)
> +static errno_t remove_selinux_login_file(const char *username)
> +{
> char *path;
> errno_t ret;
> path = selogin_path(NULL, username);
> if (!path) return ENOMEM;
> errno = 0;
> ret = unlink(path);
> if (ret < 0) {
>    ret = errno;
>    DEBUG(SSSDBG_OP_FAILURE, ("Could not remove login file %s [%
>
> d]: %s\n",
>          path, ret, strerror(ret)));
> }
> talloc_free(path);
> return ret;
>
> +}
>
> +static errno_t process_selinux_mappings(struct pam_auth_req *preq)
>  {
>      struct sysdb_ctx *sysdb;
>      TALLOC_CTX *tmp_ctx;
> @@ -549,13 +574,16 @@ static errno_t get_selinux_string(struct
> pam_auth_req *preq)
>      }
>  if (ret == ENOENT) {
>    DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using
>
> default!"));
>    DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server
>
> \n"));
> } else {
>    /* If no maps match, we'll use the default SELinux user from
>
> the
>       config */
>    file_content = talloc_strdup(tmp_ctx, default_user);
>    if (file_content == NULL) {
>        ret = ENOMEM;
>        goto done;

This error condition will leave the existing selinux users file in
place, shouldn't you remove it in the error path instead of later in the
body of the function ?
>      }
> } else {
>    /* Iterate through the order array and try to find SELinux
>
> users
>           * in fetched maps. The order array contains all SELinux
> users
>           * allowed in the domain in the same order they should appear
> @@ -589,7 +617,9 @@ static errno_t get_selinux_string(struct
> pam_auth_req *preq)
>      }
>  if (file_content) {
>    ret = write_selinux_string(pd->user, file_content);
>    ret = write_selinux_login_file(pd->user, file_content);
> } else {
>    ret = remove_selinux_login_file(pd->user);
> }

See above comment, shoudln;t this be done after the 'done' label ?
> done:
> @@ -796,7 +826,7 @@ static void pam_reply(struct pam_auth_req *preq)
>          pd->pam_status == PAM_SUCCESS) {
>          /* Try to fetch data from sysdb
>           * (auth already passed -> we should have them) */
>    ret = get_selinux_string(preq);
>    ret = process_selinux_mappings(preq);
>    if (ret != EOK) {
>        pd->pam_status = PAM_SYSTEM_ERR;
>    }
>
> -- 
> 1.7.11.2

[Jakub Hrozek]

On Mon, Aug 06, 2012 at 09:31:07AM -0400, Simo Sorce wrote:
> On Mon, 2012-08-06 at 10:51 +0200, Jakub Hrozek wrote:
> > [PATCH 1/2] Do not try to remove the temp login file if already
> > renamed
> >
> > write_selinux_string() would try to unlink the temporary file even
> > after
> > it was renamed. Failure to unlink the file would not be fatal, but
> > would
> > produce a confusing error message.
> >
> > Also don't use "0" for the default fd number, that's reserved for
> > stdin.
> > Using -1 is safer.
> >
> > [PATCH 2/2] Only create the SELinux login file if there are mappings
> > on the server
> >
> > https://fedorahosted.org/sssd/ticket/1455
> >
> > In case there are no rules on the IPA server, we must simply avoid
> > generating
> > the login file. That would make us fall back to the system-wide
> > default
> > defined in /etc/selinux/targeted/seusers.
> >
> > The IPA default must be only used if there *are* rules on the server,
> > but none matches.
>
> Minor nitpicks, but I agree with the patches purposes, so auto-ACK when
> nitpicks are addressed.
> > From 87ec8c6d2bfeed404672ddca114cc8d0e1497346 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sun, 5 Aug 2012 22:03:11 +0200
> > Subject: [PATCH 1/2] Do not try to remove the temp login file if
> > already
> >  renamed
> >
> > done:
> > if (fd > 0) {
> > if (fd > -1) {
>
> use fd != -1 here, it makes it clear only -1 is a special value.

Done.
> > From 90bd88e09e185dbcdaec17b9a8a619a0fdc9589a Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sun, 5 Aug 2012 22:37:09 +0200
> > Subject: [PATCH 2/2] Only create the SELinux login file if there are
> > mappings
> >  on the server
> > -static errno_t write_selinux_string(const char *username, char
> > *string)
> > +static char *selogin_path(TALLOC_CTX *mem_ctx, const char *username)
> > +{
> > return talloc_asprintf(mem_ctx, "%s/logins/%s",
> >
> > selinux_policy_root(),
> >                       username);
> >
> > +}
>
> I would use a macro here, a whole function for a oneliner seem
> excessive.

OK, I assumed the function would be inlined by the compiler but wanted to
keep how the login file path is constructed at one place, so that in case
it changed in the future, there's only one place to change.
> > +static errno_t process_selinux_mappings(struct pam_auth_req *preq)
> >  {
> >      struct sysdb_ctx *sysdb;
> >      TALLOC_CTX *tmp_ctx;
> > @@ -549,13 +574,16 @@ static errno_t get_selinux_string(struct
> > pam_auth_req *preq)
> >      }
> >  if (ret == ENOENT) {

I also moved this ENOENT handler together with handling of other errors
to get rid of one level of indentation. I think the function is more
compact and readable now.
> >    DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using
> >
> > default!"));
> >    DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server
> >
> > \n"));
> > } else {
> >    /* If no maps match, we'll use the default SELinux user from
> >
> > the
> >       config */
> >    file_content = talloc_strdup(tmp_ctx, default_user);
> >    if (file_content == NULL) {
> >        ret = ENOMEM;
> >        goto done;
>
> This error condition will leave the existing selinux users file in
> place, shouldn't you remove it in the error path instead of later in the
> body of the function ?

Probably yes, I didn't do it earlier because removing the file triggers
an allocation, which is likely to fail, too, if you're handling an
ENOMEM situation.
> >      }
> > } else {
> >    /* Iterate through the order array and try to find SELinux
> >
> > users
> >           * in fetched maps. The order array contains all SELinux
> > users
> >           * allowed in the domain in the same order they should appear
> > @@ -589,7 +617,9 @@ static errno_t get_selinux_string(struct
> > pam_auth_req *preq)
> >      }
> >  if (file_content) {
> >    ret = write_selinux_string(pd->user, file_content);
> >    ret = write_selinux_login_file(pd->user, file_content);
> > } else {
> >    ret = remove_selinux_login_file(pd->user);
> > }
>
> See above comment, shoudln;t this be done after the 'done' label ?

Done in 'done' now :-)
> > done:
> > @@ -796,7 +826,7 @@ static void pam_reply(struct pam_auth_req *preq)
> >          pd->pam_status == PAM_SUCCESS) {
> >          /* Try to fetch data from sysdb
> >           * (auth already passed -> we should have them) */
> >    ret = get_selinux_string(preq);
> >    ret = process_selinux_mappings(preq);
> >    if (ret != EOK) {
> >        pd->pam_status = PAM_SYSTEM_ERR;
> >    }

New patches are attached.

[Pavel Březina]

On 08/07/2012 10:01 AM, Jakub Hrozek wrote:
> On Mon, Aug 06, 2012 at 09:31:07AM -0400, Simo Sorce wrote:
> > On Mon, 2012-08-06 at 10:51 +0200, Jakub Hrozek wrote:
> > > [PATCH 1/2] Do not try to remove the temp login file if already
> > > renamed
> > >
> > > write_selinux_string() would try to unlink the temporary file even
> > > after
> > > it was renamed. Failure to unlink the file would not be fatal, but
> > > would
> > > produce a confusing error message.
> > >
> > > Also don't use "0" for the default fd number, that's reserved for
> > > stdin.
> > > Using -1 is safer.
> > >
> > > [PATCH 2/2] Only create the SELinux login file if there are mappings
> > > on the server
> > >
> > > https://fedorahosted.org/sssd/ticket/1455
> > >
> > > In case there are no rules on the IPA server, we must simply avoid
> > > generating
> > > the login file. That would make us fall back to the system-wide
> > > default
> > > defined in /etc/selinux/targeted/seusers.
> > >
> > > The IPA default must be only used if there *are* rules on the server,
> > > but none matches.
> >
> > Minor nitpicks, but I agree with the patches purposes, so auto-ACK when
> > nitpicks are addressed.
> > > From 87ec8c6d2bfeed404672ddca114cc8d0e1497346 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sun, 5 Aug 2012 22:03:11 +0200
> > > Subject: [PATCH 1/2] Do not try to remove the temp login file if
> > > already
> > >   renamed
> > >
> > > done:
> > > if (fd > 0) {
> > > if (fd > -1) {
> >
> > use fd != -1 here, it makes it clear only -1 is a special value.
>
> Done.
> > > From 90bd88e09e185dbcdaec17b9a8a619a0fdc9589a Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sun, 5 Aug 2012 22:37:09 +0200
> > > Subject: [PATCH 2/2] Only create the SELinux login file if there are
> > > mappings
> > >   on the server
> > > -static errno_t write_selinux_string(const char *username, char
> > > *string)
> > > +static char *selogin_path(TALLOC_CTX *mem_ctx, const char *username)
> > > +{
> > > return talloc_asprintf(mem_ctx, "%s/logins/%s",
> > >
> > > selinux_policy_root(),
> > >                       username);
> > >
> > > +}
> >
> > I would use a macro here, a whole function for a oneliner seem
> > excessive.
>
> OK, I assumed the function would be inlined by the compiler but wanted to
> keep how the login file path is constructed at one place, so that in case
> it changed in the future, there's only one place to change.
> > > +static errno_t process_selinux_mappings(struct pam_auth_req *preq)
> > >   {
> > >       struct sysdb_ctx *sysdb;
> > >       TALLOC_CTX *tmp_ctx;
> > > @@ -549,13 +574,16 @@ static errno_t get_selinux_string(struct
> > > pam_auth_req *preq)
> > >       }
> > >   if (ret == ENOENT) {
>
> I also moved this ENOENT handler together with handling of other errors
> to get rid of one level of indentation. I think the function is more
> compact and readable now.
> > >    DEBUG(SSSDBG_TRACE_FUNC, ("No user maps found, using
> > >
> > > default!"));
> > >    DEBUG(SSSDBG_TRACE_FUNC, ("No maps defined on the server
> > >
> > > \n"));
> > > } else {
> > >    /* If no maps match, we'll use the default SELinux user from
> > >
> > > the
> > >       config */
> > >     file_content = talloc_strdup(tmp_ctx, default_user);
> > >     if (file_content == NULL) {
> > >         ret = ENOMEM;
> > >         goto done;
> >
> > This error condition will leave the existing selinux users file in
> > place, shouldn't you remove it in the error path instead of later in the
> > body of the function ?
>
> Probably yes, I didn't do it earlier because removing the file triggers
> an allocation, which is likely to fail, too, if you're handling an
> ENOMEM situation.
> > >       }
> > > } else {
> > >     /* Iterate through the order array and try to find SELinux
> > >
> > > users
> > >            * in fetched maps. The order array contains all SELinux
> > > users
> > >            * allowed in the domain in the same order they should appear
> > > @@ -589,7 +617,9 @@ static errno_t get_selinux_string(struct
> > > pam_auth_req *preq)
> > >       }
> > >   if (file_content) {
> > >    ret = write_selinux_string(pd->user, file_content);
> > >    ret = write_selinux_login_file(pd->user, file_content);
> > > } else {
> > >    ret = remove_selinux_login_file(pd->user);
> > > }
> >
> > See above comment, shoudln;t this be done after the 'done' label ?
>
> Done in 'done' now :-)
> > > done:
> > > @@ -796,7 +826,7 @@ static void pam_reply(struct pam_auth_req *preq)
> > >           pd->pam_status == PAM_SUCCESS) {
> > >           /* Try to fetch data from sysdb
> > >            * (auth already passed -> we should have them) */
> > >    ret = get_selinux_string(preq);
> > >    ret = process_selinux_mappings(preq);
> > >     if (ret != EOK) {
> > >         pd->pam_status = PAM_SYSTEM_ERR;
> > >     }
>
> New patches are attached.

Nack.

User cannot login when the loginfile does not exist because you return 
!EOK here:

if (!file_content) {
         err = remove_selinux_login_file(pd->user);
         /* Don't overwrite original error condition if there was one */
         if (ret == EOK) ret = err;
     }
     talloc_free(tmp_ctx);
     return ret;

[Jakub Hrozek]

On Tue, Aug 14, 2012 at 11:11:01AM +0200, Pavel Březina wrote:
> Nack.
>
> User cannot login when the loginfile does not exist because you
> return !EOK here:
> if (!file_content) {
>     err = remove_selinux_login_file(pd->user);
>     /* Don't overwrite original error condition if there was one */
>     if (ret == EOK) ret = err;
> }
> talloc_free(tmp_ctx);
> return ret;

Thank you. I think I should just ignore ENOENT in
remove_selinux_login_file()

New patches are attached.

[Pavel Březina]

On 08/15/2012 06:27 PM, Jakub Hrozek wrote:
> On Tue, Aug 14, 2012 at 11:11:01AM +0200, Pavel Březina wrote:
> > Nack.
> >
> > User cannot login when the loginfile does not exist because you
> > return !EOK here:
> >  if (!file_content) {
> >      err = remove_selinux_login_file(pd->user);
> >      /* Don't overwrite original error condition if there was one */
> >      if (ret == EOK) ret = err;
> >  }
> >  talloc_free(tmp_ctx);
> >  return ret;
>
> Thank you. I think I should just ignore ENOENT in
> remove_selinux_login_file()
>
> New patches are attached.

Works fine. Ack.

[Jakub Hrozek]

On Thu, Aug 16, 2012 at 11:34:46AM +0200, Pavel Březina wrote:
> On 08/15/2012 06:27 PM, Jakub Hrozek wrote:
> > On Tue, Aug 14, 2012 at 11:11:01AM +0200, Pavel Březina wrote:
> > > Nack.
> > >
> > > User cannot login when the loginfile does not exist because you
> > > return !EOK here:
> > > if (!file_content) {
> > >     err = remove_selinux_login_file(pd->user);
> > >     /* Don't overwrite original error condition if there was one */
> > >     if (ret == EOK) ret = err;
> > > }
> > > talloc_free(tmp_ctx);
> > > return ret;
> >
> > Thank you. I think I should just ignore ENOENT in
> > remove_selinux_login_file()
> >
> > New patches are attached.
>
> Works fine. Ack.

Pushed to master after resolving a trivial conflict (a three-way merge
was all it took)