[PATCH] PAM: new option pam_trusted_users

[Pavel Reichl]

Hello,

please see attached patch which adds first of two options proposed in 
https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM

Thanks!

[Pavel Reichl]

Attached patch now contains both options.

Thanks.

On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> Hello,
>
> please see attached patch which adds first of two options proposed in 
> https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
>
> Thanks!
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Pavel Reichl]

Hello,

please see attached patch, which is amended Daniel Gollubs' patch from 
thread

[SSSD] [PATCH] PAM: Add domains= option to pam_sss (https://lists.fedorahosted.org/pipermail/sssd-devel/2014-July/020867.html).

I have just fixed nitpick found by Jakub and rebased it on top of previous patch in this thread.

and I also changed this:

Because I think dom->name would be uninitialized. However, I haven't 
tested this patch.

@@ -836,12 +867,22 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
              ret = ENOENT;
              goto done;
          }

/* skip this domain if not requested */
-        if (!is_domain_requested(pd, dom->name)) {
+        if (!is_domain_requested(pd, pd->domain)) {
        ret = ENOENT;
        goto done;
    }

Thanks!

On 09/27/2014 12:39 PM, Pavel Reichl wrote:
> Attached patch now contains both options.
>
> Thanks.
>
> On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patch which adds first of two options proposed in 
> > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> >
> > Thanks!
> >
> > sssd-devel mailing list
> > [e-mail]
> > https://lists.fedorahosted.org/mailman/listinfo/sssd-devel
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Lukas Slebodnik]

On (29/09/14 11:10), Pavel Reichl wrote:
> Hello,
>
> please see attached patch, which is amended Daniel Gollubs' patch from thread
>
> [SSSD] [PATCH] PAM: Add domains= option to pam_sss (https://lists.fedorahosted.org/pipermail/sssd-devel/2014-July/020867.html).
>
> I have just fixed nitpick found by Jakub and rebased it on top of previous patch in this thread.
>
> and I also changed this:
>
> Because I think dom->name would be uninitialized. However, I haven't tested
> this patch.
>
> @@ -836,12 +867,22 @@ static int pam_forwarder(struct cli_ctx *cctx, int pam_cmd)
>             ret = ENOENT;
>             goto done;
>         }
>    /* skip this domain if not requested */
>    if (!is_domain_requested(pd, dom->name)) {
>    if (!is_domain_requested(pd, pd->domain)) {
>  ret = ENOENT;
>  goto done;
> }
>
> Thanks!
>
> On 09/27/2014 12:39 PM, Pavel Reichl wrote:
> > Attached patch now contains both options.
> >
> > Thanks.
> >
> > On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see attached patch which adds first of two options proposed in
> > > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> > >
> > > Thanks!
> From 7ea2724932de7ae651d73b0442c66aa88b4c8f2f Mon Sep 17 00:00:00 2001
> From: Daniel Gollub <dgollub at brocade.com>
> Date: Sat, 27 Sep 2014 12:06:44 +0100
> Subject: [PATCH] PAM: Add domains= option to pam_sss
>
> Design document:
> https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
>
> Fixes:
> https://fedorahosted.org/sssd/ticket/1021
>
> Signed-off-by: Pavel Reichl 
> Reviewed-by: Sven-Thorsten Dietrich <sven at brocade.com>

//snip
> @@ -1284,6 +1298,14 @@ static void eval_argv(pam_handle_t *pamh, int argc, const char **argv,
>             *flags |= FLAGS_USE_FIRST_PASS;
>         } else if (strcmp(*argv, "use_authtok") == 0) {
>             *flags |= FLAGS_USE_AUTHTOK;
>    } else if (strncmp(*argv, OPT_DOMAINS_KEY, strlen(OPT_DOMAINS_KEY)) == 0) {
>        if (*(*argv+strlen(OPT_DOMAINS_KEY)) == '\0') {
>            logger(pamh, LOG_ERR, "Missing argument to option domains.");
>            *domains = '\0';

^^^^^^^^
                  Type of "*domains" is constant pointer to char (const char *)
                  and character '\0' is assigned to pointer, which is
                  interpreted as NULL.

It will be better to assign zero length string ("") or NULL to this output
argument.

LS

BTW: you can resent patches together after fixing nitpicks.
It will be simpler to push them together from one mail.

[Lukas Slebodnik]

On (27/09/14 12:39), Pavel Reichl wrote:
> Attached patch now contains both options.
>
> Thanks.
>
> On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> > Hello,
> >
> > please see attached patch which adds first of two options proposed in
> > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> >
> > Thanks!
> From 78a61eb76d9ccf1d060a621f987dc666b9db6e42 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Thu, 25 Sep 2014 14:52:31 +0100
> Subject: [PATCH] PAM: new options pam_trusted_users & pam_public_domains
>
> pam_public_domains option is a list of numerical UIDs or user names
> that are trusted.
>
> pam_public_domains option is a list of domains accessible even for
> untrusted users.
>
> Based on:
> https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
>
> src/confdb/confdb.h                  |  2 +
> src/config/SSSDConfig/__init__.py.in |  2 +
> src/config/etc/sssd.api.conf         |  2 +
> src/man/sssd.conf.5.xml              | 46 ++++++++++++++++++
> src/responder/pam/pamsrv.c           | 94 ++++++++++++++++++++++++++++++++++++
> src/responder/pam/pamsrv.h           |  7 +++
> src/responder/pam/pamsrv_cmd.c       | 80 ++++++++++++++++++++++++++++--
> src/util/domain_info_utils.c         | 60 +++++++++++++++++++++++
> src/util/util.h                      |  5 ++
> 9 files changed, 295 insertions(+), 3 deletions(-)

//snip
> diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
> index ad091e46e6cde1e12f554dde6470cf138412606e..3ff5fad82240c2c8a7e28c47397a20700b50ec04 100644
> --- a/src/man/sssd.conf.5.xml
> +++ b/src/man/sssd.conf.5.xml
> @@ -836,6 +836,52 @@ fallback_homedir = /home/%u
>                         </para>
>                     </listitem>
>                 </varlistentry>
>            <varlistentry>
>                <term>pam_trusted_users (string)</term>
>                <listitem>
>                    <para>
>                        Specifies the comma-separated list of UID values or
>                        user names that are allowed to access the PAM
>                        responder. User names are resolved to UIDs at
>                        startup.
>                    </para>
>                    <para>
>                        Default: all (All users are allowed to access
>                        the PAM responder)
>                    </para>
>                    <para>
>                        Please note that UID 0 is always allowed to access
>                        the PAM responder even in case it is not in the
>                        pam_trusted_users list.
>                    </para>
>                    <para>
>                        Also please note that if there is a user name in
>                        pam_trusted_users list which fails to be resolved
>                        it will cause that SSSD will not be started.
>                    </para>
>                </listitem>
>            </varlistentry>
>            <varlistentry>
>                <term>pam_public_domains (string)</term>
>                <listitem>
>                    <para>
>                        Specifies the comma-separated list of domain names
>                        that are accessible even to untrusted users.
>                    </para>
>                    <para>
>                        Two special values for pam_public_domains option
>                        are defined:
>                    </para>
>                    <para>
>                        none (Untrusted users are not allowed to access
>                        any domains PAM repsponder.)
>                    </para>

just one special value is described here.
                          The second one "all" is not described.
>                    <para>
>                        Default: none
>                    </para>
>                </listitem>
>            </varlistentry>
>       </variablelist>
>   </refsect2>

//snip
> struct pam_auth_dp_req {
> diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> index 561bd3d9678c73fdfa52592b5a0c502a46e89b5f..ebeaeeb1893b8f4078e3837f304b74dd46df7ec4 100644
> --- a/src/responder/pam/pamsrv_cmd.c
> +++ b/src/responder/pam/pamsrv_cmd.c
> @@ -762,6 +762,45 @@ static int pam_auth_req_destructor(struct pam_auth_req *preq)
>     return 0;
> }
>
> +static bool is_uid_trusted(int32_t uid,

^^^^^^^
                        It is better to have uid as unsigned value ("uint32_t")
>                       size_t trusted_uids_count,
>                       uid_t *trusted_uids)
>
> +{
> size_t i;
> /* root is always trusted */
> if (uid == 0) {
>    return true;
> }
> /* All uids are allowed */
> if (trusted_uids_count == 0) {
>    return true;
> }
> for(i = 0; i < trusted_uids_count; i++) {
>    if (trusted_uids[i] == uid) {
>        return true;
>    }
> }
> return false;
>
> +}

[Pavel Reichl]

On 09/29/2014 05:18 PM, Lukas Slebodnik wrote:
> On (27/09/14 12:39), Pavel Reichl wrote:
> > Attached patch now contains both options.
> >
> > Thanks.
> >
> > On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> > > Hello,
> > >
> > > please see attached patch which adds first of two options proposed in
> > > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> > >
> > > Thanks!
> >
> > From 78a61eb76d9ccf1d060a621f987dc666b9db6e42 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 25 Sep 2014 14:52:31 +0100
> > Subject: [PATCH] PAM: new options pam_trusted_users & pam_public_domains
> >
> > pam_public_domains option is a list of numerical UIDs or user names
> > that are trusted.
> >
> > pam_public_domains option is a list of domains accessible even for
> > untrusted users.
> >
> > Based on:
> > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> >
> > src/confdb/confdb.h                  |  2 +
> > src/config/SSSDConfig/__init__.py.in |  2 +
> > src/config/etc/sssd.api.conf         |  2 +
> > src/man/sssd.conf.5.xml              | 46 ++++++++++++++++++
> > src/responder/pam/pamsrv.c           | 94 ++++++++++++++++++++++++++++++++++++
> > src/responder/pam/pamsrv.h           |  7 +++
> > src/responder/pam/pamsrv_cmd.c       | 80 ++++++++++++++++++++++++++++--
> > src/util/domain_info_utils.c         | 60 +++++++++++++++++++++++
> > src/util/util.h                      |  5 ++
> > 9 files changed, 295 insertions(+), 3 deletions(-)
>
> //snip
> > diff --git a/src/man/sssd.conf.5.xml b/src/man/sssd.conf.5.xml
> > index ad091e46e6cde1e12f554dde6470cf138412606e..3ff5fad82240c2c8a7e28c47397a20700b50ec04 100644
> > --- a/src/man/sssd.conf.5.xml
> > +++ b/src/man/sssd.conf.5.xml
> > @@ -836,6 +836,52 @@ fallback_homedir = /home/%u
> >                          </para>
> >                      </listitem>
> >                  </varlistentry>
> >            <varlistentry>
> >                <term>pam_trusted_users (string)</term>
> >                <listitem>
> >                    <para>
> >                        Specifies the comma-separated list of UID values or
> >                        user names that are allowed to access the PAM
> >                        responder. User names are resolved to UIDs at
> >                        startup.
> >                    </para>
> >                    <para>
> >                        Default: all (All users are allowed to access
> >                        the PAM responder)
> >                    </para>
> >                    <para>
> >                        Please note that UID 0 is always allowed to access
> >                        the PAM responder even in case it is not in the
> >                        pam_trusted_users list.
> >                    </para>
> >                    <para>
> >                        Also please note that if there is a user name in
> >                        pam_trusted_users list which fails to be resolved
> >                        it will cause that SSSD will not be started.
> >                    </para>
> >                </listitem>
> >            </varlistentry>
> >            <varlistentry>
> >                <term>pam_public_domains (string)</term>
> >                <listitem>
> >                    <para>
> >                        Specifies the comma-separated list of domain names
> >                        that are accessible even to untrusted users.
> >                    </para>
> >                    <para>
> >                        Two special values for pam_public_domains option
> >                        are defined:
> >                    </para>
> >                    <para>
> >                        none (Untrusted users are not allowed to access
> >                        any domains PAM repsponder.)
> >                    </para>
>                        just one special value is described here.
>                        The second one "all" is not described.
>
> >                    <para>
> >                        Default: none
> >                    </para>
> >                </listitem>
> >            </varlistentry>
> >        </variablelist>
> >    </refsect2>
>
> //snip
> > struct pam_auth_dp_req {
> > diff --git a/src/responder/pam/pamsrv_cmd.c b/src/responder/pam/pamsrv_cmd.c
> > index 561bd3d9678c73fdfa52592b5a0c502a46e89b5f..ebeaeeb1893b8f4078e3837f304b74dd46df7ec4 100644
> > --- a/src/responder/pam/pamsrv_cmd.c
> > +++ b/src/responder/pam/pamsrv_cmd.c
> > @@ -762,6 +762,45 @@ static int pam_auth_req_destructor(struct pam_auth_req *preq)
> >      return 0;
> > }
> >
> > +static bool is_uid_trusted(int32_t uid,
>                           ^^^^^^^
>                      It is better to have uid as unsigned value ("uint32_t")
>
> >                       size_t trusted_uids_count,
> >                       uid_t *trusted_uids)
> >
> > +{
> > size_t i;
> > /* root is always trusted */
> > if (uid == 0) {
> >    return true;
> > }
> > /* All uids are allowed */
> > if (trusted_uids_count == 0) {
> >    return true;
> > }
> > for(i = 0; i < trusted_uids_count; i++) {
> >    if (trusted_uids[i] == uid) {
> >        return true;
> >    }
> > }
> > return false;
> >
> > +}
>
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

OK, thanks for review. I made changes you requested in both mails.

[Lukas Slebodnik]

On (29/09/14 18:07), Pavel Reichl wrote:
> On 09/29/2014 05:18 PM, Lukas Slebodnik wrote:
> > On (27/09/14 12:39), Pavel Reichl wrote:
> > > Attached patch now contains both options.
> > >
> > > Thanks.
> > >
> > > On 09/25/2014 03:57 PM, Pavel Reichl wrote:
> > > > Hello,
> > > >
> > > > please see attached patch which adds first of two options proposed in
> > > > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> > > >
> > > > Thanks!
>
> OK, thanks for review. I made changes you requested in both mails.
> From 2d80e4913453a2c76ff698765765bf3881ecc4e7 Mon Sep 17 00:00:00 2001
> From: Pavel Reichl 
> Date: Thu, 25 Sep 2014 14:52:31 +0100
> Subject: [PATCH 1/2] PAM: new options pam_trusted_users & pam_public_domains
>
> pam_public_domains option is a list of numerical UIDs or user names
> that are trusted.
>
> pam_public_domains option is a list of domains accessible even for
> untrusted users.
>
> Based on:
> https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM

ACK
> From 841b390c2540f6fb251cc76ca37bc0051b3c31dd Mon Sep 17 00:00:00 2001
> From: Daniel Gollub <dgollub at brocade.com>
> Date: Sat, 27 Sep 2014 12:06:44 +0100
> Subject: [PATCH 2/2] PAM: Add domains= option to pam_sss
>
> Design document:
> https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
>
> Fixes:
> https://fedorahosted.org/sssd/ticket/1021
>
> Signed-off-by: Pavel Reichl 
> Reviewed-by: Sven-Thorsten Dietrich <sven at brocade.com>

ACK

LS

[Jakub Hrozek]

On Mon, Sep 29, 2014 at 06:24:11PM +0200, Lukas Slebodnik wrote:
> > From 2d80e4913453a2c76ff698765765bf3881ecc4e7 Mon Sep 17 00:00:00 2001
> > From: Pavel Reichl 
> > Date: Thu, 25 Sep 2014 14:52:31 +0100
> > Subject: [PATCH 1/2] PAM: new options pam_trusted_users & pam_public_domains
> >
> > pam_public_domains option is a list of numerical UIDs or user names
> > that are trusted.
> >
> > pam_public_domains option is a list of domains accessible even for
> > untrusted users.
> >
> > Based on:
> > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
>
> ACK
> > From 841b390c2540f6fb251cc76ca37bc0051b3c31dd Mon Sep 17 00:00:00 2001
> > From: Daniel Gollub <dgollub at brocade.com>
> > Date: Sat, 27 Sep 2014 12:06:44 +0100
> > Subject: [PATCH 2/2] PAM: Add domains= option to pam_sss
> >
> > Design document:
> > https://fedorahosted.org/sssd/wiki/DesignDocs/RestrictDomainsInPAM
> >
> > Fixes:
> > https://fedorahosted.org/sssd/ticket/1021
> >
> > Signed-off-by: Pavel Reichl 
> > Reviewed-by: Sven-Thorsten Dietrich <sven at brocade.com>
>
> ACK

* master:
    * 830ded27453015080a54d6ba85fd4999ee7e9af1
    * 663fd9bcdcc6b299785ba3434532cd7e6c462bff