[PATCH] Back end patches for running SSSD as a non-root user

[Jakub Hrozek]

Hi,

I'll send patches that help SSSD run as a non-root user to this thread.
I'm still chasing some bugs in the krb5_child changes, but the attached
patches are ready for review.

If other developers don't like the idea of a Python-based unit test that
spawns a KDC, I'm equally fine with keeping those patches in my tree --
they served their purpose, after all.

[Michal Židek]

On 10/20/2014 04:13 PM, Jakub Hrozek wrote:
> Hi,
>
> I'll send patches that help SSSD run as a non-root user to this thread.
> I'm still chasing some bugs in the krb5_child changes, but the attached
> patches are ready for review.
>
> If other developers don't like the idea of a Python-based unit test that
> spawns a KDC, I'm equally fine with keeping those patches in my tree --
> they served their purpose, after all.
>
> 0001-BUILD-Install-ldap_child-and-krb5_child-as-setuid-if.patch

Ack.
> 0002-LDAP-Move-sss_krb5_verify_keytab_ex-to-ldap_child.patch

Ack.
> 0003-LDAP-read-the-correct-data-type-from-ldap_child-s-in.patch
>   /* ticket lifetime */
> SAFEALIGN_COPY_INT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
> SAFEALIGN_COPY_UINT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
>  DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %d\n", ibuf->lifetime);

^^^
Should be %u. Otherwise Ack.
> 0004-LDAP-Drop-privileges-after-kinit-in-ldap_child.patch

Ack.
> 0005-TESTS-Fix-krb5_child-test.patch
> 0006-TESTS-Add-a-cwrap-enabled-test-for-krb5_child.patch

I can compile the tests only from your nonroot branch. Even if
I apply the patchsets from:
SSSD: Add the options to specify a UID and GID to run as
Monitor and sbus changes for running SSSD as a non-privileged user

the test compilation fails.

CCLD     usertools-tests
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_getpwnam_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:104: 
undefined reference to `sysdb_search_user_override_by_name'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:126: 
undefined reference to `sysdb_add_overrides_to_object'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_getpwuid_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:209: 
undefined reference to `sysdb_search_user_override_by_uid'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:231: 
undefined reference to `sysdb_add_overrides_to_object'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_enumpwent_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:317: 
undefined reference to `sysdb_add_overrides_to_object'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_getgrnam_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:396: 
undefined reference to `sysdb_search_group_override_by_name'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:428: 
undefined reference to `sysdb_add_overrides_to_object'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:435: 
undefined reference to `sysdb_add_group_member_overrides'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_getgrgid_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:548: 
undefined reference to `sysdb_search_group_override_by_gid'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:580: 
undefined reference to `sysdb_add_overrides_to_object'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:587: 
undefined reference to `sysdb_add_group_member_overrides'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_enumgrent_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:745: 
undefined reference to `sysdb_add_group_member_overrides'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:737: 
undefined reference to `sysdb_add_overrides_to_object'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_initgroups_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:959: 
undefined reference to `sysdb_add_overrides_to_object'
../../../src/db/usertools_tests-sysdb_search.o: In function 
`sysdb_get_user_attr_with_views':
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:1083: 
undefined reference to `sysdb_search_user_override_attrs_by_name'
/home/user/gitrepo/sssd/src/tests/cwrap/../../../src/db/sysdb_search.c:1105: 
undefined reference to `sysdb_add_overrides_to_object'
collect2: error: ld returned 1 exit status
make[3]: *** [usertools-tests] Error 1
make[3]: Leaving directory `/home/user/gitrepo/sssd/src/tests/cwrap'
make[2]: *** [check-am] Error 2
make[2]: Leaving directory `/home/user/gitrepo/sssd/src/tests/cwrap'
make[1]: *** [check-recursive] Error 1
make[1]: Leaving directory `/home/user/gitrepo/sssd'
make: *** [check] Error 2

So there is missing patch in the patchsets on which the test
suite depends.

But even if I run make check from the nonroot branch, It fails
(see attachment).

Michal

[Jakub Hrozek]

On Mon, Oct 20, 2014 at 06:35:13PM +0200, Michal Židek wrote:
> On 10/20/2014 04:13 PM, Jakub Hrozek wrote:
> > Hi,
> >
> > I'll send patches that help SSSD run as a non-root user to this thread.
> > I'm still chasing some bugs in the krb5_child changes, but the attached
> > patches are ready for review.
> >
> > If other developers don't like the idea of a Python-based unit test that
> > spawns a KDC, I'm equally fine with keeping those patches in my tree --
> > they served their purpose, after all.
> >
> > 0001-BUILD-Install-ldap_child-and-krb5_child-as-setuid-if.patch
>
> Ack.
> > 0002-LDAP-Move-sss_krb5_verify_keytab_ex-to-ldap_child.patch
>
> Ack.
> > 0003-LDAP-read-the-correct-data-type-from-ldap_child-s-in.patch
> >  /* ticket lifetime */
> > SAFEALIGN_COPY_INT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
> > SAFEALIGN_COPY_UINT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
> > DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %d\n", ibuf->lifetime);
>                                         ^^^
>
> Should be %u. Otherwise Ack.

Fixed.
> > 0004-LDAP-Drop-privileges-after-kinit-in-ldap_child.patch
>
> Ack.

Thank you for the review.
> > 0005-TESTS-Fix-krb5_child-test.patch
> > 0006-TESTS-Add-a-cwrap-enabled-test-for-krb5_child.patch

I'm withdrawing these two patches for now. The krb5_child needs more
work and should be submitted with the krb5_child changes anyway.

The ldap_child patches are attached.

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Jakub Hrozek]

On Fri, Oct 24, 2014 at 10:52:47PM +0200, Jakub Hrozek wrote:
> On Mon, Oct 20, 2014 at 06:35:13PM +0200, Michal Židek wrote:
> > On 10/20/2014 04:13 PM, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > I'll send patches that help SSSD run as a non-root user to this thread.
> > > I'm still chasing some bugs in the krb5_child changes, but the attached
> > > patches are ready for review.
> > >
> > > If other developers don't like the idea of a Python-based unit test that
> > > spawns a KDC, I'm equally fine with keeping those patches in my tree --
> > > they served their purpose, after all.
> > >
> > > 0001-BUILD-Install-ldap_child-and-krb5_child-as-setuid-if.patch
> >
> > Ack.
> > > 0002-LDAP-Move-sss_krb5_verify_keytab_ex-to-ldap_child.patch
> >
> > Ack.
> > > 0003-LDAP-read-the-correct-data-type-from-ldap_child-s-in.patch
> > >  /* ticket lifetime */
> > > SAFEALIGN_COPY_INT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
> > > SAFEALIGN_COPY_UINT32_CHECK(&ibuf->lifetime, buf + p, size, &p);
> > > DEBUG(SSSDBG_TRACE_LIBS, "lifetime: %d\n", ibuf->lifetime);
> >                                         ^^^
> >
> > Should be %u. Otherwise Ack.
>
> Fixed.
> > > 0004-LDAP-Drop-privileges-after-kinit-in-ldap_child.patch
> >
> > Ack.
>
> Thank you for the review.
> > > 0005-TESTS-Fix-krb5_child-test.patch
> > > 0006-TESTS-Add-a-cwrap-enabled-test-for-krb5_child.patch
>
> I'm withdrawing these two patches for now. The krb5_child needs more
> work and should be submitted with the krb5_child changes anyway.
>
> The ldap_child patches are attached.

Hi,

I'm sorry, but I had one ldap_child change squashed into another
patch...

New patches are attached. Also the subject was changed to something more
appropriate.

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Pavel Reichl]

Just code style nitpicks, feel free to ignore.

On 10/27/2014 04:17 PM, Jakub Hrozek wrote:
> 0002-LDAP-Move-sss_krb5_verify_keytab_ex-to-ldap_child.patch
>
> From 154a897b09136c4499fc024c9110096d34d2b6d5 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek
> Date: Sat, 11 Oct 2014 17:39:21 +0200
> Subject: [PATCH 2/4] LDAP: Move sss_krb5_verify_keytab_ex to ldap_child
>
> The function was called from one place only, so it makes no sense to
> keep it in a shared module. Moreover, the function should only be
> called from code that runs as root.
>
> src/providers/ldap/ldap_child.c | 80 ++++++++++++++++++++++++++++++++++++++++-
>   src/util/sss_krb5.c             | 76 ---------------------------------------
>   src/util/sss_krb5.h             |  3 --
>   3 files changed, 79 insertions(+), 80 deletions(-)
>
> diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
> index e5779b70906d90ab855677f04a154e179f2163c6..faa6f027ba52f55985c63e8055292e616b5bb6bf 100644
> --- a/src/providers/ldap/ldap_child.c
> +++ b/src/providers/ldap/ldap_child.c
> @@ -160,6 +160,84 @@ set_child_debugging(krb5_context ctx)
>       return EOK;
>   }
>
> +static int lc_verify_keytab_ex(const char *principal,
>                           const char *keytab_name,
>                           krb5_context context,
>                           krb5_keytab keytab)
>
> +{
> bool found;
> char *kt_principal;
> krb5_error_code krberr;
> krb5_kt_cursor cursor;
> krb5_keytab_entry entry;
> krberr = krb5_kt_start_seq_get(context, keytab, &cursor);
> if (krberr) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Cannot read keytab [%s].\n", KEYTAB_CLEAN_NAME);
>    sss_log(SSS_LOG_ERR, "Error reading keytab file [%s]: [%d][%s]. "
>                         "Unable to create GSSAPI-encrypted LDAP "
>                         "connection.",
>                         KEYTAB_CLEAN_NAME, krberr,
>                         sss_krb5_get_error_message(context, krberr));
>    return EIO;
> }
> found = false;
> while((krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0){

missing space after while and before {
>    krberr = krb5_unparse_name(context, entry.principal, &kt_principal);
>    if (krberr) {
>        DEBUG(SSSDBG_FATAL_FAILURE,
>              "Could not parse keytab entry\n");
>        sss_log(SSS_LOG_ERR, "Could not parse keytab entry\n");
>        return EIO;
>    }
>    if (strcmp(principal, kt_principal) == 0) {
>        found = true;
>    }
>    free(kt_principal);
>    krberr = sss_krb5_free_keytab_entry_contents(context, &entry);
>    if (krberr) {
>        /* This should never happen. The API docs for this function
>         * specify only success for this function
>         */
>        DEBUG(SSSDBG_CRIT_FAILURE,"Could not free keytab entry contents\n");
>        /* This is non-fatal, so we'll continue here */
>    }
>    if (found) {
>        break;
>    }
> }
> krberr = krb5_kt_end_seq_get(context, keytab, &cursor);
> if (krberr) {
>    DEBUG(SSSDBG_FATAL_FAILURE, "Could not close keytab.\n");
>    sss_log(SSS_LOG_ERR, "Could not close keytab file [%s].",
>                         KEYTAB_CLEAN_NAME);
>    return EIO;
> }
> if (!found) {
>    DEBUG(SSSDBG_FATAL_FAILURE,
>          "Principal [%s] not found in keytab [%s]\n",
>           principal,
>           KEYTAB_CLEAN_NAME);
>    sss_log(SSS_LOG_ERR, "Error processing keytab file [%s]: "
>                         "Principal [%s] was not found. "
>                         "Unable to create GSSAPI-encrypted LDAP connection.",
>                         KEYTAB_CLEAN_NAME, principal);
>    return EFAULT;
> }
> return EOK;
>
> +}

One empty line too many.

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Michal Židek]

On 10/29/2014 11:03 AM, Pavel Reichl wrote:
> Just code style nitpicks, feel free to ignore.
>
> On 10/27/2014 04:17 PM, Jakub Hrozek wrote:
> > 0002-LDAP-Move-sss_krb5_verify_keytab_ex-to-ldap_child.patch
> >
> > From 154a897b09136c4499fc024c9110096d34d2b6d5 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek
> > Date: Sat, 11 Oct 2014 17:39:21 +0200
> > Subject: [PATCH 2/4] LDAP: Move sss_krb5_verify_keytab_ex to ldap_child
> >
> > The function was called from one place only, so it makes no sense to
> > keep it in a shared module. Moreover, the function should only be
> > called from code that runs as root.
> >
> > src/providers/ldap/ldap_child.c | 80 ++++++++++++++++++++++++++++++++++++++++-
> >   src/util/sss_krb5.c             | 76 ---------------------------------------
> >   src/util/sss_krb5.h             |  3 --
> >   3 files changed, 79 insertions(+), 80 deletions(-)
> >
> > diff --git a/src/providers/ldap/ldap_child.c b/src/providers/ldap/ldap_child.c
> > index e5779b70906d90ab855677f04a154e179f2163c6..faa6f027ba52f55985c63e8055292e616b5bb6bf 100644
> > --- a/src/providers/ldap/ldap_child.c
> > +++ b/src/providers/ldap/ldap_child.c
> > @@ -160,6 +160,84 @@ set_child_debugging(krb5_context ctx)
> >       return EOK;
> >   }
> >
> > +static int lc_verify_keytab_ex(const char *principal,
> >                           const char *keytab_name,
> >                           krb5_context context,
> >                           krb5_keytab keytab)
> >
> > +{
> > bool found;
> > char *kt_principal;
> > krb5_error_code krberr;
> > krb5_kt_cursor cursor;
> > krb5_keytab_entry entry;
> > krberr = krb5_kt_start_seq_get(context, keytab, &cursor);
> > if (krberr) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Cannot read keytab [%s].\n", KEYTAB_CLEAN_NAME);
> >    sss_log(SSS_LOG_ERR, "Error reading keytab file [%s]: [%d][%s]. "
> >                         "Unable to create GSSAPI-encrypted LDAP "
> >                         "connection.",
> >                         KEYTAB_CLEAN_NAME, krberr,
> >                         sss_krb5_get_error_message(context, krberr));
> >    return EIO;
> > }
> > found = false;
> > while((krb5_kt_next_entry(context, keytab, &entry, &cursor)) == 0){
>
> missing space after while and before {
> >    krberr = krb5_unparse_name(context, entry.principal, &kt_principal);
> >    if (krberr) {
> >        DEBUG(SSSDBG_FATAL_FAILURE,
> >              "Could not parse keytab entry\n");
> >        sss_log(SSS_LOG_ERR, "Could not parse keytab entry\n");
> >        return EIO;
> >    }
> >    if (strcmp(principal, kt_principal) == 0) {
> >        found = true;
> >    }
> >    free(kt_principal);
> >    krberr = sss_krb5_free_keytab_entry_contents(context, &entry);
> >    if (krberr) {
> >        /* This should never happen. The API docs for this function
> >         * specify only success for this function
> >         */
> >        DEBUG(SSSDBG_CRIT_FAILURE,"Could not free keytab entry contents\n");
> >        /* This is non-fatal, so we'll continue here */
> >    }
> >    if (found) {
> >        break;
> >    }
> > }
> > krberr = krb5_kt_end_seq_get(context, keytab, &cursor);
> > if (krberr) {
> >    DEBUG(SSSDBG_FATAL_FAILURE, "Could not close keytab.\n");
> >    sss_log(SSS_LOG_ERR, "Could not close keytab file [%s].",
> >                         KEYTAB_CLEAN_NAME);
> >    return EIO;
> > }
> > if (!found) {
> >    DEBUG(SSSDBG_FATAL_FAILURE,
> >          "Principal [%s] not found in keytab [%s]\n",
> >           principal,
> >           KEYTAB_CLEAN_NAME);
> >    sss_log(SSS_LOG_ERR, "Error processing keytab file [%s]: "
> >                         "Principal [%s] was not found. "
> >                         "Unable to create GSSAPI-encrypted LDAP connection.",
> >                         KEYTAB_CLEAN_NAME, principal);
> >    return EFAULT;
> > }
> > return EOK;
> >
> > +}
>
> One empty line too many.

These patches look good to me. If the nitpicks that Pavel found
are fixed, I'll ack.

Michal

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Jakub Hrozek]

On Fri, Oct 31, 2014 at 08:15:13PM +0100, Michal Židek wrote:
> These patches look good to me. If the nitpicks that Pavel found
> are fixed, I'll ack.
>
> Michal

OK, see the attached patches. Thank you both for the review.

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Michal Židek]

On 11/05/2014 06:15 PM, Jakub Hrozek wrote:
> On Fri, Oct 31, 2014 at 08:15:13PM +0100, Michal Židek wrote:
> > These patches look good to me. If the nitpicks that Pavel found
> > are fixed, I'll ack.
> >
> > Michal
>
> OK, see the attached patches. Thank you both for the review.

Ack to all.

Michal

[PATCH] Run the ldap_child process as setuid if sssd_be is running as unprivileged user

[Jakub Hrozek]

On Wed, Nov 05, 2014 at 07:38:54PM +0100, Michal Židek wrote:
> On 11/05/2014 06:15 PM, Jakub Hrozek wrote:
> > On Fri, Oct 31, 2014 at 08:15:13PM +0100, Michal Židek wrote:
> > > These patches look good to me. If the nitpicks that Pavel found
> > > are fixed, I'll ack.
> > >
> > > Michal
> >
> > OK, see the attached patches. Thank you both for the review.
>
> Ack to all.
>
> Michal

Thanks a lot for the review!

* master:
    f3a25949de81f80c136bb073e4a8f504b080c20c
    77b13371c87702aee3f858f6b2b73826cf5a01bd
    06f10b2a0ebb26f2460cd445f8040e9205de7500
    936940720b1b0e701a2317abc4c2d05a78338f33