[PATCH] Move ccache operations to krb5_child, allow the krb5_auth code to run unprivileged

[Jakub Hrozek]

Hi,

attached are patches that apply on top of my previous ldap_child and
selinux_child patches. The complete branch (including tests I'm still
working on) can be inspected here:
    https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot

Simo, Sumit, I added you to CC directly, b/c I suspected you wanted to be
included in larger krb5_child changes..

Below are patch descriptinons of the individual changes.

[PATCH 1/6] BUILD: Install krb5_child as suid if running under non-privileged user
If sssd_be is running unprivileged, then krb5_child must be setuid to be
able to access the keytab and become arbitrary user.

[PATCH 2/6] KRB5: Drop privileges in the child, not the back end
In future patches, sssd_be will be running as a non-privileged user, who
will execute the setuid krb5_child. In this case, the child will start
as root and drop the privileges as soon as possible.

[PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
Add a new module krb5_ccache.c that contains all ccache-related
operations. The only user of this module shall be krb5_child.c as the
other modules will run unprivileged and accessing the keytab requires
either privileges of root or the ccache owner.

This patch only /moves/ code, the changes will come in later patches.

[PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
Otherwise we would have to link krb5_child with pcre and transfer the
regex, which wold be cumbersome. Check for illegal patterns when
expanding the template instead.

[PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
The credential cache operations must be now performed by the krb5_child
completely, because the sssd_be process might be running as the sssd
user who doesn't have access to the ccaches.

src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
until we fix Kerberos ticket renewal as non-root.

Also includes a new error code that indicates that the back end should
remove the old ccache attribute -- the child can't do that if it's
running as the user.

[PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
The code didn't have to handle this case previously as sssd_be was always
running as root and switching to the ccache as the user logging in.

Also handle NULL creds on restore_creds() in case there was no switch.
One less if-condition and fewer indentation levels.

Thank you for the review.

[Jakub Hrozek]

On Tue, Oct 28, 2014 at 03:13:37AM +0100, Jakub Hrozek wrote:
> Hi,
>
> attached are patches that apply on top of my previous ldap_child and
> selinux_child patches. The complete branch (including tests I'm still
> working on) can be inspected here:
>     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot
>
> Simo, Sumit, I added you to CC directly, b/c I suspected you wanted to be
> included in larger krb5_child changes..
>
> Below are patch descriptinons of the individual changes.
>
> [PATCH 1/6] BUILD: Install krb5_child as suid if running under non-privileged user
> If sssd_be is running unprivileged, then krb5_child must be setuid to be
> able to access the keytab and become arbitrary user.
>
> [PATCH 2/6] KRB5: Drop privileges in the child, not the back end
> In future patches, sssd_be will be running as a non-privileged user, who
> will execute the setuid krb5_child. In this case, the child will start
> as root and drop the privileges as soon as possible.
>
> [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
> Add a new module krb5_ccache.c that contains all ccache-related
> operations. The only user of this module shall be krb5_child.c as the
> other modules will run unprivileged and accessing the keytab requires
> either privileges of root or the ccache owner.
>
> This patch only /moves/ code, the changes will come in later patches.
>
> [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> Otherwise we would have to link krb5_child with pcre and transfer the
> regex, which wold be cumbersome. Check for illegal patterns when
> expanding the template instead.
>
> [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> The credential cache operations must be now performed by the krb5_child
> completely, because the sssd_be process might be running as the sssd
> user who doesn't have access to the ccaches.
>
> src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> until we fix Kerberos ticket renewal as non-root.
>
> Also includes a new error code that indicates that the back end should
> remove the old ccache attribute -- the child can't do that if it's
> running as the user.
>
> [PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
> The code didn't have to handle this case previously as sssd_be was always
> running as root and switching to the ccache as the user logging in.
>
> Also handle NULL creds on restore_creds() in case there was no switch.
> One less if-condition and fewer indentation levels.
>
> Thank you for the review.

Hi,

attached patches fix make distcheck issue. There are no changes in the
code itself.

[Jakub Hrozek]

On Fri, Oct 31, 2014 at 06:00:59PM +0100, Jakub Hrozek wrote:
> On Tue, Oct 28, 2014 at 03:13:37AM +0100, Jakub Hrozek wrote:
> > Hi,
> >
> > attached are patches that apply on top of my previous ldap_child and
> > selinux_child patches. The complete branch (including tests I'm still
> > working on) can be inspected here:
> >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot
> >
> > Simo, Sumit, I added you to CC directly, b/c I suspected you wanted to be
> > included in larger krb5_child changes..
> >
> > Below are patch descriptinons of the individual changes.
> >
> > [PATCH 1/6] BUILD: Install krb5_child as suid if running under non-privileged user
> > If sssd_be is running unprivileged, then krb5_child must be setuid to be
> > able to access the keytab and become arbitrary user.
> >
> > [PATCH 2/6] KRB5: Drop privileges in the child, not the back end
> > In future patches, sssd_be will be running as a non-privileged user, who
> > will execute the setuid krb5_child. In this case, the child will start
> > as root and drop the privileges as soon as possible.
> >
> > [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
> > Add a new module krb5_ccache.c that contains all ccache-related
> > operations. The only user of this module shall be krb5_child.c as the
> > other modules will run unprivileged and accessing the keytab requires
> > either privileges of root or the ccache owner.
> >
> > This patch only /moves/ code, the changes will come in later patches.
> >
> > [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> > Otherwise we would have to link krb5_child with pcre and transfer the
> > regex, which wold be cumbersome. Check for illegal patterns when
> > expanding the template instead.
> >
> > [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > The credential cache operations must be now performed by the krb5_child
> > completely, because the sssd_be process might be running as the sssd
> > user who doesn't have access to the ccaches.
> >
> > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > until we fix Kerberos ticket renewal as non-root.
> >
> > Also includes a new error code that indicates that the back end should
> > remove the old ccache attribute -- the child can't do that if it's
> > running as the user.
> >
> > [PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
> > The code didn't have to handle this case previously as sssd_be was always
> > running as root and switching to the ccache as the user logging in.
> >
> > Also handle NULL creds on restore_creds() in case there was no switch.
> > One less if-condition and fewer indentation levels.
> >
> > Thank you for the review.
>
> Hi,
>
> attached patches fix make distcheck issue. There are no changes in the
> code itself.

Hi,

attached are rebased patches. Again, there is no change in the code
itself.

I'm still looking for a reviewer ...

[Jakub Hrozek]

On Wed, Nov 05, 2014 at 06:36:06PM +0100, Jakub Hrozek wrote:
> On Fri, Oct 31, 2014 at 06:00:59PM +0100, Jakub Hrozek wrote:
> > On Tue, Oct 28, 2014 at 03:13:37AM +0100, Jakub Hrozek wrote:
> > > Hi,
> > >
> > > attached are patches that apply on top of my previous ldap_child and
> > > selinux_child patches. The complete branch (including tests I'm still
> > > working on) can be inspected here:
> > >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot
> > >
> > > Simo, Sumit, I added you to CC directly, b/c I suspected you wanted to be
> > > included in larger krb5_child changes..
> > >
> > > Below are patch descriptinons of the individual changes.
> > >
> > > [PATCH 1/6] BUILD: Install krb5_child as suid if running under non-privileged user
> > > If sssd_be is running unprivileged, then krb5_child must be setuid to be
> > > able to access the keytab and become arbitrary user.
> > >
> > > [PATCH 2/6] KRB5: Drop privileges in the child, not the back end
> > > In future patches, sssd_be will be running as a non-privileged user, who
> > > will execute the setuid krb5_child. In this case, the child will start
> > > as root and drop the privileges as soon as possible.
> > >
> > > [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
> > > Add a new module krb5_ccache.c that contains all ccache-related
> > > operations. The only user of this module shall be krb5_child.c as the
> > > other modules will run unprivileged and accessing the keytab requires
> > > either privileges of root or the ccache owner.
> > >
> > > This patch only /moves/ code, the changes will come in later patches.
> > >
> > > [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> > > Otherwise we would have to link krb5_child with pcre and transfer the
> > > regex, which wold be cumbersome. Check for illegal patterns when
> > > expanding the template instead.
> > >
> > > [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > > The credential cache operations must be now performed by the krb5_child
> > > completely, because the sssd_be process might be running as the sssd
> > > user who doesn't have access to the ccaches.
> > >
> > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > until we fix Kerberos ticket renewal as non-root.
> > >
> > > Also includes a new error code that indicates that the back end should
> > > remove the old ccache attribute -- the child can't do that if it's
> > > running as the user.
> > >
> > > [PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
> > > The code didn't have to handle this case previously as sssd_be was always
> > > running as root and switching to the ccache as the user logging in.
> > >
> > > Also handle NULL creds on restore_creds() in case there was no switch.
> > > One less if-condition and fewer indentation levels.
> > >
> > > Thank you for the review.
> >
> > Hi,
> >
> > attached patches fix make distcheck issue. There are no changes in the
> > code itself.
>
> Hi,
>
> attached are rebased patches. Again, there is no change in the code
> itself.
>
> I'm still looking for a reviewer ...

Simo,

in this thread are the patches I pinged you about earlier today on IRC. I'm
sorry I wasn't able for interactive discussion today, but if you have the
time, do you agree with the general direction the patches are taking?

I understand you don't have the time for testing etc. but an
architectural blessing would help as well.

Thanks!

[Simo Sorce]

On Wed, 5 Nov 2014 22:34:50 +0100
Jakub Hrozek  wrote:
> On Wed, Nov 05, 2014 at 06:36:06PM +0100, Jakub Hrozek wrote:
> > On Fri, Oct 31, 2014 at 06:00:59PM +0100, Jakub Hrozek wrote:
> > > On Tue, Oct 28, 2014 at 03:13:37AM +0100, Jakub Hrozek wrote:
> > > > Hi,
> > > >
> > > > attached are patches that apply on top of my previous
> > > > ldap_child and selinux_child patches. The complete branch
> > > > (including tests I'm still working on) can be inspected here:
> > > >     https://fedorapeople.org/cgit/jhrozek/public_git/sssd.git/log/?h=nonroot
> > > >
> > > > Simo, Sumit, I added you to CC directly, b/c I suspected you
> > > > wanted to be included in larger krb5_child changes..
> > > >
> > > > Below are patch descriptinons of the individual changes.
> > > >
> > > > [PATCH 1/6] BUILD: Install krb5_child as suid if running under
> > > > non-privileged user If sssd_be is running unprivileged, then
> > > > krb5_child must be setuid to be able to access the keytab and
> > > > become arbitrary user.
> > > >
> > > > [PATCH 2/6] KRB5: Drop privileges in the child, not the back end
> > > > In future patches, sssd_be will be running as a non-privileged
> > > > user, who will execute the setuid krb5_child. In this case, the
> > > > child will start as root and drop the privileges as soon as
> > > > possible.
> > > >
> > > > [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
> > > > Add a new module krb5_ccache.c that contains all ccache-related
> > > > operations. The only user of this module shall be krb5_child.c
> > > > as the other modules will run unprivileged and accessing the
> > > > keytab requires either privileges of root or the ccache owner.
> > > >
> > > > This patch only /moves/ code, the changes will come in later
> > > > patches.
> > > >
> > > > [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> > > > Otherwise we would have to link krb5_child with pcre and
> > > > transfer the regex, which wold be cumbersome. Check for illegal
> > > > patterns when expanding the template instead.
> > > >
> > > > [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > > > The credential cache operations must be now performed by the
> > > > krb5_child completely, because the sssd_be process might be
> > > > running as the sssd user who doesn't have access to the ccaches.
> > > >
> > > > src/providers/krb5/krb5_ccache.c is still linked against
> > > > libsss_krb5 until we fix Kerberos ticket renewal as non-root.
> > > >
> > > > Also includes a new error code that indicates that the back end
> > > > should remove the old ccache attribute -- the child can't do
> > > > that if it's running as the user.
> > > >
> > > > [PATCH 6/6] KRB5: Do not switch_creds() if already the
> > > > specified user The code didn't have to handle this case
> > > > previously as sssd_be was always running as root and switching
> > > > to the ccache as the user logging in.
> > > >
> > > > Also handle NULL creds on restore_creds() in case there was no
> > > > switch. One less if-condition and fewer indentation levels.
> > > >
> > > > Thank you for the review.
> > >
> > > Hi,
> > >
> > > attached patches fix make distcheck issue. There are no changes
> > > in the code itself.
> >
> > Hi,
> >
> > attached are rebased patches. Again, there is no change in the code
> > itself.
> >
> > I'm still looking for a reviewer ...
>
> Simo,
>
> in this thread are the patches I pinged you about earlier today on
> IRC. I'm sorry I wasn't able for interactive discussion today, but if
> you have the time, do you agree with the general direction the
> patches are taking?
>
> I understand you don't have the time for testing etc. but an
> architectural blessing would help as well.
>
> Thanks!

Comments sent to the list per patch, I think I have no other comments
on the patchset.

Thank you,
Simo.

[Simo Sorce]

Comments inline.
(I'll send multiple emails per patch, where necessary)

On Wed, 5 Nov 2014 18:36:06 +0100
Jakub Hrozek  wrote:
> From c9d484638009371cff3b1213b5640a7827b9e1ba Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Mon, 13 Oct 2014 21:13:38 +0200
> Subject: [PATCH 2/6] KRB5: Drop privileges in the child, not the back
> end
>
> In future patches, sssd_be will be running as a non-privileged user,
> who will execute the setuid krb5_child. In this case, the child will
> start as root and drop the privileges as soon as possible.
>
> However, we need to also remove the privilege drop in sssd_be, because
> if we dropped to the user who is authenticating, we wouldn't be even
> allowed to execute krb5_child. The krb5_child permissions should be
> 4750, owned by root.sssd, to make sure only root and sssd can execute
> the child and if executed by sssd, the child will run as root.

Should we check that the parent process really is running either as
root or the sssd user and fail otherwise ?
> src/providers/krb5/krb5_child.c         | 57
> +++++++++++++++++++++++++--------
> src/providers/krb5/krb5_child_handler.c |  8 ----- 2 files changed,
> 44 insertions(+), 21 deletions(-)
>
> diff --git a/src/providers/krb5/krb5_child.c
> b/src/providers/krb5/krb5_child.c index
> 3234a4e6c740db5e05f7db8eb7f4ea0cc126e7ce..1c657de2d96d9fcea6d041af9cc9217863732b44
> 100644 --- a/src/providers/krb5/krb5_child.c +++
> b/src/providers/krb5/krb5_child.c @@ -1840,11 +1840,52 @@ static int
> k5c_setup_fast(struct krb5_req *kr, bool demand) return EOK;
>  }
>
> -static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> +static errno_t check_use_fast(bool *_fast)
>  {
> krb5_error_code kerr;
> char *use_fast_str;
> bool fast = false;
> use_fast_str = getenv(SSSD_KRB5_USE_FAST);

Although the launching user is somewhat trusted .. should we stop using
environment variables and instead pass info via command line options ?

The reason I ask is that I would like to see all getenv() calls changed
to secure_getenv() calls where possible.
> if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") ==
> {
>    DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
> } else if (strcasecmp(use_fast_str, "try") == 0 ||
>           strcasecmp(use_fast_str, "demand") == 0) {
>    fast = true;
> } else {
>    DEBUG(SSSDBG_CRIT_FAILURE,
>            "Unsupported value [%s] for krb5_use_fast.\n",
>            use_fast_str);
>    return EINVAL;
> }
> *_fast = fast;
> return EOK;
>
> +}
>
> +static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> +{
> krb5_error_code kerr;
> int parse_flags;
> bool use_fast;
> kerr = check_use_fast(&use_fast);
> if (kerr != EOK) {
>    return kerr;
> }
> if (offline || (use_fast == false && kr->validate == false)) {
>    /* If krb5_child was started as setuid, but we don't need to
>     * perform either validation or FAST, just drop privileges to
>     * the SSSD user. The same applies to the offline case

Do I read this wrongly ? Or is this comment misleading ?
As far as I can tell you are dropping privileges to the user trying to
authenticate not the SSSD user? (Did I miss another change where SSSD's
user uid/gid values are set in kr ?)
>     */
>    kerr = become_user(kr->uid, kr->gid);
>    if (kerr != 0) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
>        return kerr;
>    }
> }
> DEBUG(SSSDBG_TRACE_INTERNAL,
>      "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(),
>
> getegid()); 
>      kr->realm = getenv(SSSD_KRB5_REALM);
>      if (kr->realm == NULL) {
> @@ -1931,18 +1972,8 @@ static int k5c_setup(struct krb5_req *kr,
> uint32_t offline) if (!offline) {
>          set_canonicalize_option(kr->options);
>    use_fast_str = getenv(SSSD_KRB5_USE_FAST);
>    if (use_fast_str == NULL || strcasecmp(use_fast_str,
>
> "never") == 0) {
>        DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
>    } else if (strcasecmp(use_fast_str, "try") == 0) {
>    if (use_fast) {
>        kerr = k5c_setup_fast(kr, false);
>    } else if (strcasecmp(use_fast_str, "demand") == 0) {
>        kerr = k5c_setup_fast(kr, true);

This change seem to be breaking k5c_setup_fast, in the sense that you
never pass along the "demand" option and therefore the code will always
fall back even though the configuration wanted to forbid downgrades.

I think this makes me NACK this patch (and I do not see any other
change that addresses this point in the other patches).
>    } else {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "Unsupported value [%s] for krb5_use_fast.\n",
>               use_fast_str);
>        return EINVAL;
>    }
> }
>
> diff --git a/src/providers/krb5/krb5_child_handler.c
> b/src/providers/krb5/krb5_child_handler.c index
> 4ba939deb3e0e282b3ca9b2b21226ea7e64e311b..71c7f9c9f662e16b94afda0c8c0ae24666f0ba15
> 100644 --- a/src/providers/krb5/krb5_child_handler.c +++
> b/src/providers/krb5/krb5_child_handler.c @@ -284,14 +284,6 @@ static
> errno_t fork_child(struct tevent_req *req) pid = fork();
>  if (pid == 0) { /* child */
>    if (state->kr->run_as_user) {
>        ret = become_user(state->kr->uid, state->kr->gid);
>        if (ret != EOK) {
>            DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
>            return ret;
>        }
>    }
>    err = exec_child(state,
>                     pipefd_to_child, pipefd_from_child,
>                     KRB5_CHILD,
>
> state->kr->krb5_ctx->child_debug_fd);

[Jakub Hrozek]

On Thu, Nov 06, 2014 at 09:56:13AM -0500, Simo Sorce wrote:
> Comments inline.
> (I'll send multiple emails per patch, where necessary)

Thanks for the review and sorry about the delay, some other work came
up...
> On Wed, 5 Nov 2014 18:36:06 +0100
> Jakub Hrozek  wrote:
> > From c9d484638009371cff3b1213b5640a7827b9e1ba Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Mon, 13 Oct 2014 21:13:38 +0200
> > Subject: [PATCH 2/6] KRB5: Drop privileges in the child, not the back
> > end
> >
> > In future patches, sssd_be will be running as a non-privileged user,
> > who will execute the setuid krb5_child. In this case, the child will
> > start as root and drop the privileges as soon as possible.
> >
> > However, we need to also remove the privilege drop in sssd_be, because
> > if we dropped to the user who is authenticating, we wouldn't be even
> > allowed to execute krb5_child. The krb5_child permissions should be
> > 4750, owned by root.sssd, to make sure only root and sssd can execute
> > the child and if executed by sssd, the child will run as root.
>
> Should we check that the parent process really is running either as
> root or the sssd user and fail otherwise ?

We could only do that if we limit the user sssd runs as to either the
sssd user or root as well. Currently the 'user' configuration option
blindly accepts anything -- which might not be the best idea, because
realistically, only root or the sssd user would work.

So yeah, I'm fine with doing this change, but I also need to restrict
the user sssd runs at to root or the configured user.
> > src/providers/krb5/krb5_child.c         | 57
> > +++++++++++++++++++++++++--------
> > src/providers/krb5/krb5_child_handler.c |  8 ----- 2 files changed,
> > 44 insertions(+), 21 deletions(-)
> >
> > diff --git a/src/providers/krb5/krb5_child.c
> > b/src/providers/krb5/krb5_child.c index
> > 3234a4e6c740db5e05f7db8eb7f4ea0cc126e7ce..1c657de2d96d9fcea6d041af9cc9217863732b44
> > 100644 --- a/src/providers/krb5/krb5_child.c +++
> > b/src/providers/krb5/krb5_child.c @@ -1840,11 +1840,52 @@ static int
> > k5c_setup_fast(struct krb5_req *kr, bool demand) return EOK;
> >  }
> >
> > -static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> > +static errno_t check_use_fast(bool *_fast)
> >  {
> > krb5_error_code kerr;
> > char *use_fast_str;
> > bool fast = false;
> > use_fast_str = getenv(SSSD_KRB5_USE_FAST);
>
> Although the launching user is somewhat trusted .. should we stop using
> environment variables and instead pass info via command line options ?
>
> The reason I ask is that I would like to see all getenv() calls changed
> to secure_getenv() calls where possible.

Yes, I agree and it's been something that we planned for some time, but
I think it should be a separate patch.

The work is tracked by https://fedorahosted.org/sssd/ticket/697 btw
> > if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") ==
> > {
> >    DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
> > } else if (strcasecmp(use_fast_str, "try") == 0 ||
> >           strcasecmp(use_fast_str, "demand") == 0) {
> >    fast = true;
> > } else {
> >    DEBUG(SSSDBG_CRIT_FAILURE,
> >            "Unsupported value [%s] for krb5_use_fast.\n",
> >            use_fast_str);
> >    return EINVAL;
> > }
> > *_fast = fast;
> > return EOK;
> >
> > +}
> >
> > +static int k5c_setup(struct krb5_req *kr, uint32_t offline)
> > +{
> > krb5_error_code kerr;
> > int parse_flags;
> > bool use_fast;
> > kerr = check_use_fast(&use_fast);
> > if (kerr != EOK) {
> >    return kerr;
> > }
> > if (offline || (use_fast == false && kr->validate == false)) {
> >    /* If krb5_child was started as setuid, but we don't need to
> >     * perform either validation or FAST, just drop privileges to
> >     * the SSSD user. The same applies to the offline case
>
> Do I read this wrongly ? Or is this comment misleading ?
> As far as I can tell you are dropping privileges to the user trying to
> authenticate not the SSSD user? (Did I miss another change where SSSD's
> user uid/gid values are set in kr ?)

The comment is misleading, thanks for catching that. Fixed.
> >     */
> >    kerr = become_user(kr->uid, kr->gid);
> >    if (kerr != 0) {
> >        DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
> >        return kerr;
> >    }
> > }
> > DEBUG(SSSDBG_TRACE_INTERNAL,
> >      "Running as [%"SPRIuid"][%"SPRIgid"].\n", geteuid(),
> >
> > getegid()); 
> >      kr->realm = getenv(SSSD_KRB5_REALM);
> >      if (kr->realm == NULL) {
> > @@ -1931,18 +1972,8 @@ static int k5c_setup(struct krb5_req *kr,
> > uint32_t offline) if (!offline) {
> >          set_canonicalize_option(kr->options);
> >    use_fast_str = getenv(SSSD_KRB5_USE_FAST);
> >    if (use_fast_str == NULL || strcasecmp(use_fast_str,
> >
> > "never") == 0) {
> >        DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
> >    } else if (strcasecmp(use_fast_str, "try") == 0) {
> >    if (use_fast) {
> >        kerr = k5c_setup_fast(kr, false);
> >    } else if (strcasecmp(use_fast_str, "demand") == 0) {
> >        kerr = k5c_setup_fast(kr, true);
>
> This change seem to be breaking k5c_setup_fast, in the sense that you
> never pass along the "demand" option and therefore the code will always
> fall back even though the configuration wanted to forbid downgrades.
>
> I think this makes me NACK this patch (and I do not see any other
> change that addresses this point in the other patches).

Aah, thanks, I think I changed the code as a prototype to make sure FAST
worked at all and then forgot to finish this part..

I'll attach my patches to your other reply.
> >    } else {
> >        DEBUG(SSSDBG_CRIT_FAILURE,
> >              "Unsupported value [%s] for krb5_use_fast.\n",
> >               use_fast_str);
> >        return EINVAL;
> >    }
> > }
> >
> > diff --git a/src/providers/krb5/krb5_child_handler.c
> > b/src/providers/krb5/krb5_child_handler.c index
> > 4ba939deb3e0e282b3ca9b2b21226ea7e64e311b..71c7f9c9f662e16b94afda0c8c0ae24666f0ba15
> > 100644 --- a/src/providers/krb5/krb5_child_handler.c +++
> > b/src/providers/krb5/krb5_child_handler.c @@ -284,14 +284,6 @@ static
> > errno_t fork_child(struct tevent_req *req) pid = fork();
> >  if (pid == 0) { /* child */
> >    if (state->kr->run_as_user) {
> >        ret = become_user(state->kr->uid, state->kr->gid);
> >        if (ret != EOK) {
> >            DEBUG(SSSDBG_CRIT_FAILURE, "become_user failed.\n");
> >            return ret;
> >        }
> >    }
> >    err = exec_child(state,
> >                     pipefd_to_child, pipefd_from_child,
> >                     KRB5_CHILD,
> >
> > state->kr->krb5_ctx->child_debug_fd);

[Simo Sorce]

On Wed, 5 Nov 2014 18:36:06 +0100
Jakub Hrozek  wrote:
> From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:13 +0200
> Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
>
> The credential cache operations must be now performed by the
> krb5_child completely, because the sssd_be process might be running
> as the sssd user who doesn't have access to the ccaches.
>
> src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> until we fix Kerberos ticket renewal as non-root.
>
> Also includes a new error code that indicates that the back end should
> remove the old ccache attribute -- the child can't do that if it's
> running as the user.

I think I see an ordering issue but I am not completely sure
(unfortunately I am looking only at the patches because git am fails to
bring them on a copy of the master tree).

The main reason ccache handling was done in 2 parts is that in some
cases we need to do some preparatory work as root.
For example creating a user directory for DIR: ccache and similar.
I think this patches moves things around and causes all these functions
to be run after krb5_child has already dropped root in
k5c_setup() though. If that's the case as it looks to me I think this
patchset will not work as well as it should.

I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the ccache
once the reply is returned to the backend. But I am afraid deleting a
ccache may also require root privs as the ccache is owned by the user
but the sssd backend is running as the sssd user. Or am I missing
another change that addresses this ?

Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
ERR_CREDS_EXPIRED, why (and why does it need to be different) ?

Simo.

[Jakub Hrozek]

On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> On Wed, 5 Nov 2014 18:36:06 +0100
> Jakub Hrozek  wrote:
> > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> >
> > The credential cache operations must be now performed by the
> > krb5_child completely, because the sssd_be process might be running
> > as the sssd user who doesn't have access to the ccaches.
> >
> > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > until we fix Kerberos ticket renewal as non-root.
> >
> > Also includes a new error code that indicates that the back end should
> > remove the old ccache attribute -- the child can't do that if it's
> > running as the user.
>
> I think I see an ordering issue but I am not completely sure
> (unfortunately I am looking only at the patches because git am fails to
> bring them on a copy of the master tree).

The patches depended on the SELinux changes that were since merged to
master. The attached patchset should apply on master cleanly.
> The main reason ccache handling was done in 2 parts is that in some
> cases we need to do some preparatory work as root.
> For example creating a user directory for DIR: ccache and similar.
> I think this patches moves things around and causes all these functions
> to be run after krb5_child has already dropped root in
> k5c_setup() though. If that's the case as it looks to me I think this
> patchset will not work as well as it should.

You're right, I admit I tested on RHEL and Fedora where we only use
KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
confused by our removal of creating public ccache dir components -- now
I realize we can still create user-owned directories under root-writably
directories and DIR:/run/user/$UID is exactly this case.

This means we need to examine the ccache as root in order to determine
whether to re-create it...with the attached patches the DIR cache seems
to work fine.
> I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the ccache
> once the reply is returned to the backend. But I am afraid deleting a
> ccache may also require root privs as the ccache is owned by the user
> but the sssd backend is running as the sssd user. Or am I missing
> another change that addresses this ?
>
> Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> ERR_CREDS_EXPIRED, why (and why does it need to be different) ?

No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The krb5_child
is able to remove the old ccache from disk or from the kernel keyring,
but in case the child dropped privileges to the user who is logging in,
it cannot delete the ccache attribute from sysdb..so
ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to remove this
attribute istead.

Thank you for the review. New patches are attached (and I just realized
I didn't CC you in my reply to your other mail; sorry)

[Simo Sorce]

On Mon, 10 Nov 2014 17:12:55 +0100
Jakub Hrozek  wrote:
> On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > On Wed, 5 Nov 2014 18:36:06 +0100
> > Jakub Hrozek  wrote:
> > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00
> > > 2001 From: Jakub Hrozek 
> > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to
> > > krb5_child.c
> > >
> > > The credential cache operations must be now performed by the
> > > krb5_child completely, because the sssd_be process might be
> > > running as the sssd user who doesn't have access to the ccaches.
> > >
> > > src/providers/krb5/krb5_ccache.c is still linked against
> > > libsss_krb5 until we fix Kerberos ticket renewal as non-root.
> > >
> > > Also includes a new error code that indicates that the back end
> > > should remove the old ccache attribute -- the child can't do that
> > > if it's running as the user.
> >
> > I think I see an ordering issue but I am not completely sure
> > (unfortunately I am looking only at the patches because git am
> > fails to bring them on a copy of the master tree).
>
> The patches depended on the SELinux changes that were since merged to
> master. The attached patchset should apply on master cleanly.
> > The main reason ccache handling was done in 2 parts is that in some
> > cases we need to do some preparatory work as root.
> > For example creating a user directory for DIR: ccache and similar.
> > I think this patches moves things around and causes all these
> > functions to be run after krb5_child has already dropped root in
> > k5c_setup() though. If that's the case as it looks to me I think
> > this patchset will not work as well as it should.
>
> You're right, I admit I tested on RHEL and Fedora where we only use
> KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
> confused by our removal of creating public ccache dir components --
> now I realize we can still create user-owned directories under
> root-writably directories and DIR:/run/user/$UID is exactly this case.
>
> This means we need to examine the ccache as root in order to determine
> whether to re-create it...with the attached patches the DIR cache
> seems to work fine.
> > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the
> > ccache once the reply is returned to the backend. But I am afraid
> > deleting a ccache may also require root privs as the ccache is
> > owned by the user but the sssd backend is running as the sssd user.
> > Or am I missing another change that addresses this ?
> >
> > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> > ERR_CREDS_EXPIRED, why (and why does it need to be different) ?
>
> No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The
> krb5_child is able to remove the old ccache from disk or from the
> kernel keyring, but in case the child dropped privileges to the user
> who is logging in, it cannot delete the ccache attribute from
> sysdb..so ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to
> remove this attribute istead.

What happen if sssd_be is running unprivileged itself ?
> Thank you for the review. New patches are attached (and I just
> realized I didn't CC you in my reply to your other mail; sorry)

NP: I'll try to catch up on the replies and look at the new patches in
the afternoon.

Simo.

[Jakub Hrozek]

On Mon, Nov 10, 2014 at 11:37:41AM -0500, Simo Sorce wrote:
> On Mon, 10 Nov 2014 17:12:55 +0100
> Jakub Hrozek  wrote:
> > On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > > On Wed, 5 Nov 2014 18:36:06 +0100
> > > Jakub Hrozek  wrote:
> > > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00
> > > > 2001 From: Jakub Hrozek 
> > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to
> > > > krb5_child.c
> > > >
> > > > The credential cache operations must be now performed by the
> > > > krb5_child completely, because the sssd_be process might be
> > > > running as the sssd user who doesn't have access to the ccaches.
> > > >
> > > > src/providers/krb5/krb5_ccache.c is still linked against
> > > > libsss_krb5 until we fix Kerberos ticket renewal as non-root.
> > > >
> > > > Also includes a new error code that indicates that the back end
> > > > should remove the old ccache attribute -- the child can't do that
> > > > if it's running as the user.
> > >
> > > I think I see an ordering issue but I am not completely sure
> > > (unfortunately I am looking only at the patches because git am
> > > fails to bring them on a copy of the master tree).
> >
> > The patches depended on the SELinux changes that were since merged to
> > master. The attached patchset should apply on master cleanly.
> > > The main reason ccache handling was done in 2 parts is that in some
> > > cases we need to do some preparatory work as root.
> > > For example creating a user directory for DIR: ccache and similar.
> > > I think this patches moves things around and causes all these
> > > functions to be run after krb5_child has already dropped root in
> > > k5c_setup() though. If that's the case as it looks to me I think
> > > this patchset will not work as well as it should.
> >
> > You're right, I admit I tested on RHEL and Fedora where we only use
> > KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
> > confused by our removal of creating public ccache dir components --
> > now I realize we can still create user-owned directories under
> > root-writably directories and DIR:/run/user/$UID is exactly this case.
> >
> > This means we need to examine the ccache as root in order to determine
> > whether to re-create it...with the attached patches the DIR cache
> > seems to work fine.
> > > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the
> > > ccache once the reply is returned to the backend. But I am afraid
> > > deleting a ccache may also require root privs as the ccache is
> > > owned by the user but the sssd backend is running as the sssd user.
> > > Or am I missing another change that addresses this ?
> > >
> > > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> > > ERR_CREDS_EXPIRED, why (and why does it need to be different) ?
> >
> > No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The
> > krb5_child is able to remove the old ccache from disk or from the
> > kernel keyring, but in case the child dropped privileges to the user
> > who is logging in, it cannot delete the ccache attribute from
> > sysdb..so ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to
> > remove this attribute istead.
>
> What happen if sssd_be is running unprivileged itself ?

sssd_be is running as the sssd user who has rw access to the cache:
ll /var/lib/sss/db/cache_ipa.example.com.ldb 
-rw-------. 1 sssd sssd 1609728 Nov 10 17:18 /var/lib/sss/db/cache_ipa.example.com.ldb
> > Thank you for the review. New patches are attached (and I just
> > realized I didn't CC you in my reply to your other mail; sorry)
>
> NP: I'll try to catch up on the replies and look at the new patches in
> the afternoon.
>
> Simo.
>
> -- 
> Simo Sorce * Red Hat, Inc * New York

[Simo Sorce]

On Mon, 10 Nov 2014 17:44:48 +0100
Jakub Hrozek  wrote:
> On Mon, Nov 10, 2014 at 11:37:41AM -0500, Simo Sorce wrote:
> > On Mon, 10 Nov 2014 17:12:55 +0100
> > Jakub Hrozek  wrote:
> > > On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > > > On Wed, 5 Nov 2014 18:36:06 +0100
> > > > Jakub Hrozek  wrote:
> > > > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17
> > > > > 00:00:00 2001 From: Jakub Hrozek 
> > > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to
> > > > > krb5_child.c
> > > > >
> > > > > The credential cache operations must be now performed by the
> > > > > krb5_child completely, because the sssd_be process might be
> > > > > running as the sssd user who doesn't have access to the
> > > > > ccaches.
> > > > >
> > > > > src/providers/krb5/krb5_ccache.c is still linked against
> > > > > libsss_krb5 until we fix Kerberos ticket renewal as non-root.
> > > > >
> > > > > Also includes a new error code that indicates that the back
> > > > > end should remove the old ccache attribute -- the child can't
> > > > > do that if it's running as the user.
> > > >
> > > > I think I see an ordering issue but I am not completely sure
> > > > (unfortunately I am looking only at the patches because git am
> > > > fails to bring them on a copy of the master tree).
> > >
> > > The patches depended on the SELinux changes that were since
> > > merged to master. The attached patchset should apply on master
> > > cleanly.
> > > > The main reason ccache handling was done in 2 parts is that in
> > > > some cases we need to do some preparatory work as root.
> > > > For example creating a user directory for DIR: ccache and
> > > > similar. I think this patches moves things around and causes
> > > > all these functions to be run after krb5_child has already
> > > > dropped root in k5c_setup() though. If that's the case as it
> > > > looks to me I think this patchset will not work as well as it
> > > > should.
> > >
> > > You're right, I admit I tested on RHEL and Fedora where we only
> > > use KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was
> > > also confused by our removal of creating public ccache dir
> > > components -- now I realize we can still create user-owned
> > > directories under root-writably directories and
> > > DIR:/run/user/$UID is exactly this case.
> > >
> > > This means we need to examine the ccache as root in order to
> > > determine whether to re-create it...with the attached patches the
> > > DIR cache seems to work fine.
> > > > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the
> > > > ccache once the reply is returned to the backend. But I am
> > > > afraid deleting a ccache may also require root privs as the
> > > > ccache is owned by the user but the sssd backend is running as
> > > > the sssd user. Or am I missing another change that addresses
> > > > this ?
> > > >
> > > > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same
> > > > checks as ERR_CREDS_EXPIRED, why (and why does it need to be
> > > > different) ?
> > >
> > > No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The
> > > krb5_child is able to remove the old ccache from disk or from the
> > > kernel keyring, but in case the child dropped privileges to the
> > > user who is logging in, it cannot delete the ccache attribute from
> > > sysdb..so ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to
> > > remove this attribute istead.
> >
> > What happen if sssd_be is running unprivileged itself ?
>
> sssd_be is running as the sssd user who has rw access to the cache:
> ll /var/lib/sss/db/cache_ipa.example.com.ldb 
> -rw-------. 1 sssd sssd 1609728 Nov 10
> 17:18 /var/lib/sss/db/cache_ipa.example.com.ldb

Wait, isn't this about user's ccaches ?

If the cache is owned by sssd, in what case the krb5_child helper
would not be able to deal with it ?
> > > Thank you for the review. New patches are attached (and I just
> > > realized I didn't CC you in my reply to your other mail; sorry)
> >
> > NP: I'll try to catch up on the replies and look at the new patches
> > in the afternoon.
> >
> > Simo.
> >
> > -- 
> > Simo Sorce * Red Hat, Inc * New York

[Jakub Hrozek]

On Mon, Nov 10, 2014 at 11:50:11AM -0500, Simo Sorce wrote:
> On Mon, 10 Nov 2014 17:44:48 +0100
> Jakub Hrozek  wrote:
> > On Mon, Nov 10, 2014 at 11:37:41AM -0500, Simo Sorce wrote:
> > > On Mon, 10 Nov 2014 17:12:55 +0100
> > > Jakub Hrozek  wrote:
> > > > On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > > > > On Wed, 5 Nov 2014 18:36:06 +0100
> > > > > Jakub Hrozek  wrote:
> > > > > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17
> > > > > > 00:00:00 2001 From: Jakub Hrozek 
> > > > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to
> > > > > > krb5_child.c
> > > > > >
> > > > > > The credential cache operations must be now performed by the
> > > > > > krb5_child completely, because the sssd_be process might be
> > > > > > running as the sssd user who doesn't have access to the
> > > > > > ccaches.
> > > > > >
> > > > > > src/providers/krb5/krb5_ccache.c is still linked against
> > > > > > libsss_krb5 until we fix Kerberos ticket renewal as non-root.
> > > > > >
> > > > > > Also includes a new error code that indicates that the back
> > > > > > end should remove the old ccache attribute -- the child can't
> > > > > > do that if it's running as the user.
> > > > >
> > > > > I think I see an ordering issue but I am not completely sure
> > > > > (unfortunately I am looking only at the patches because git am
> > > > > fails to bring them on a copy of the master tree).
> > > >
> > > > The patches depended on the SELinux changes that were since
> > > > merged to master. The attached patchset should apply on master
> > > > cleanly.
> > > > > The main reason ccache handling was done in 2 parts is that in
> > > > > some cases we need to do some preparatory work as root.
> > > > > For example creating a user directory for DIR: ccache and
> > > > > similar. I think this patches moves things around and causes
> > > > > all these functions to be run after krb5_child has already
> > > > > dropped root in k5c_setup() though. If that's the case as it
> > > > > looks to me I think this patchset will not work as well as it
> > > > > should.
> > > >
> > > > You're right, I admit I tested on RHEL and Fedora where we only
> > > > use KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was
> > > > also confused by our removal of creating public ccache dir
> > > > components -- now I realize we can still create user-owned
> > > > directories under root-writably directories and
> > > > DIR:/run/user/$UID is exactly this case.
> > > >
> > > > This means we need to examine the ccache as root in order to
> > > > determine whether to re-create it...with the attached patches the
> > > > DIR cache seems to work fine.
> > > > > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the
> > > > > ccache once the reply is returned to the backend. But I am
> > > > > afraid deleting a ccache may also require root privs as the
> > > > > ccache is owned by the user but the sssd backend is running as
> > > > > the sssd user. Or am I missing another change that addresses
> > > > > this ?
> > > > >
> > > > > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same
> > > > > checks as ERR_CREDS_EXPIRED, why (and why does it need to be
> > > > > different) ?
> > > >
> > > > No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The
> > > > krb5_child is able to remove the old ccache from disk or from the
> > > > kernel keyring, but in case the child dropped privileges to the
> > > > user who is logging in, it cannot delete the ccache attribute from
> > > > sysdb..so ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to
> > > > remove this attribute istead.
> > >
> > > What happen if sssd_be is running unprivileged itself ?
> >
> > sssd_be is running as the sssd user who has rw access to the cache:
> > ll /var/lib/sss/db/cache_ipa.example.com.ldb 
> > -rw-------. 1 sssd sssd 1609728 Nov 10
> > 17:18 /var/lib/sss/db/cache_ipa.example.com.ldb
>
> Wait, isn't this about user's ccaches ?
>
> If the cache is owned by sssd, in what case the krb5_child helper
> would not be able to deal with it ?

There are two pieces of data. One is the user ccache created by libkrb5,
which is accessible by krb5_child, either running as root or the user
who is logging in. Then there is the "ccacheFile" sysdb attribute which
indicates the ccache the user had used previously. Since sysdb is only
writable by the SSSD user (or root..) we need to remove the ccache
attribute in the sssd_be process in case we switch to a different one.
> > > > Thank you for the review. New patches are attached (and I just
> > > > realized I didn't CC you in my reply to your other mail; sorry)
> > >
> > > NP: I'll try to catch up on the replies and look at the new patches
> > > in the afternoon.
> > >
> > > Simo.
> > >
> > > -- 
> > > Simo Sorce * Red Hat, Inc * New York
>
> -- 
> Simo Sorce * Red Hat, Inc * New York

[Lukas Slebodnik]

On (10/11/14 17:12), Jakub Hrozek wrote:
> On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > On Wed, 5 Nov 2014 18:36:06 +0100
> > Jakub Hrozek  wrote:
> > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > >
> > > The credential cache operations must be now performed by the
> > > krb5_child completely, because the sssd_be process might be running
> > > as the sssd user who doesn't have access to the ccaches.
> > >
> > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > until we fix Kerberos ticket renewal as non-root.
> > >
> > > Also includes a new error code that indicates that the back end should
> > > remove the old ccache attribute -- the child can't do that if it's
> > > running as the user.
> >
> > I think I see an ordering issue but I am not completely sure
> > (unfortunately I am looking only at the patches because git am fails to
> > bring them on a copy of the master tree).
>
> The patches depended on the SELinux changes that were since merged to
> master. The attached patchset should apply on master cleanly.
> > The main reason ccache handling was done in 2 parts is that in some
> > cases we need to do some preparatory work as root.
> > For example creating a user directory for DIR: ccache and similar.
> > I think this patches moves things around and causes all these functions
> > to be run after krb5_child has already dropped root in
> > k5c_setup() though. If that's the case as it looks to me I think this
> > patchset will not work as well as it should.
>
> You're right, I admit I tested on RHEL and Fedora where we only use
> KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
> confused by our removal of creating public ccache dir components -- now
> I realize we can still create user-owned directories under root-writably
> directories and DIR:/run/user/$UID is exactly this case.
>
> This means we need to examine the ccache as root in order to determine
> whether to re-create it...with the attached patches the DIR cache seems
> to work fine.
> > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the ccache
> > once the reply is returned to the backend. But I am afraid deleting a
> > ccache may also require root privs as the ccache is owned by the user
> > but the sssd backend is running as the sssd user. Or am I missing
> > another change that addresses this ?
> >
> > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> > ERR_CREDS_EXPIRED, why (and why does it need to be different) ?
>
> No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The krb5_child
> is able to remove the old ccache from disk or from the kernel keyring,
> but in case the child dropped privileges to the user who is logging in,
> it cannot delete the ccache attribute from sysdb..so
> ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to remove this
> attribute istead.
>
> Thank you for the review. New patches are attached (and I just realized
> I didn't CC you in my reply to your other mail; sorry)
> From 7addb7d573a2ef06f60e0c026f4bd01423d073ea Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Fri, 24 Oct 2014 22:44:17 +0200
> Subject: [PATCH 1/6] BUILD: Install krb5_child as suid if running under
> non-privileged user
>
> If sssd_be is running unprivileged, then krb5_child must be setuid to be
> able to access the keytab and become arbitrary user.

There is a crash in krb_child with refactoring patches.

#0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
222         if (ccname[0] == '/') {
(gdb) p ccname
$1 = 0x0

(gdb) bt full
#0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
        tmp_ctx = 0x0
        filename = <value optimized out>
        ccdirname = <value optimized out>
        end = <value optimized out>
        ret = <value optimized out>
        __FUNCTION__ = "sss_krb5_precreate_ccache"
#1  0x0000000000405adc in k5c_precreate_ccache (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:1993
        ret = <value optimized out>
#2  k5c_setup (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:2042
        kerr = <value optimized out>
        parse_flags = <value optimized out>
        fast_val = K5C_FAST_NEVER
        __FUNCTION__ = "k5c_setup"
#3  0x0000000000408a86 in main (argc=5, argv=<value optimized out>) at src/providers/krb5/krb5_child.c:2246
        kr = 0x785450
        offline = 0
        opt = <value optimized out>
        pc = <value optimized out>
        debug_fd = 18
        ret = <value optimized out>
        long_options = {{longName = 0x0, shortName = 0 '\000', argInfo = 4, arg = 0x617980, val = 0, descrip = 0x4121e9 "Help options:", argDescrip = 0x0}, {
            longName = 0x4121f7 "debug-level", shortName = 100 'd', argInfo = 2, arg = 0x617a40, val = 0, descrip = 0x4121c8 "Debug level", argDescrip = 0x0}, {
            longName = 0x412203 "debug-timestamps", shortName = 0 '\000', argInfo = 2, arg = 0x617960, val = 0, descrip = 0x4121d4 "Add debug timestamps", argDescrip = 0x0}, {
            longName = 0x412214 "debug-microseconds", shortName = 0 '\000', argInfo = 2, arg = 0x617a50, val = 0, descrip = 0x412e18 "Show timestamps with microseconds", 
            argDescrip = 0x0}, {longName = 0x412227 "debug-fd", shortName = 0 '\000', argInfo = 2, arg = 0x7fff7a3bed08, val = 0, 
            descrip = 0x412e40 "An open file descriptor for the debug logs", argDescrip = 0x0}, {longName = 0x412230 "debug-to-stderr", shortName = 0 '\000', argInfo = 1073741824, 
            arg = 0x617a58, val = 0, descrip = 0x412e70 "Send the debug output to stderr directly.", argDescrip = 0x0}, {longName = 0x0, shortName = 0 '\000', argInfo = 0, arg = 0x0, 
            val = 0, descrip = 0x0, argDescrip = 0x0}}
        __FUNCTION__ = "main"

LS

[Lukas Slebodnik]

On (11/11/14 09:09), Lukas Slebodnik wrote:
> On (10/11/14 17:12), Jakub Hrozek wrote:
> > On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > > On Wed, 5 Nov 2014 18:36:06 +0100
> > > Jakub Hrozek  wrote:
> > > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > > >
> > > > The credential cache operations must be now performed by the
> > > > krb5_child completely, because the sssd_be process might be running
> > > > as the sssd user who doesn't have access to the ccaches.
> > > >
> > > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > > until we fix Kerberos ticket renewal as non-root.
> > > >
> > > > Also includes a new error code that indicates that the back end should
> > > > remove the old ccache attribute -- the child can't do that if it's
> > > > running as the user.
> > >
> > > I think I see an ordering issue but I am not completely sure
> > > (unfortunately I am looking only at the patches because git am fails to
> > > bring them on a copy of the master tree).
> >
> > The patches depended on the SELinux changes that were since merged to
> > master. The attached patchset should apply on master cleanly.
> > > The main reason ccache handling was done in 2 parts is that in some
> > > cases we need to do some preparatory work as root.
> > > For example creating a user directory for DIR: ccache and similar.
> > > I think this patches moves things around and causes all these functions
> > > to be run after krb5_child has already dropped root in
> > > k5c_setup() though. If that's the case as it looks to me I think this
> > > patchset will not work as well as it should.
> >
> > You're right, I admit I tested on RHEL and Fedora where we only use
> > KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
> > confused by our removal of creating public ccache dir components -- now
> > I realize we can still create user-owned directories under root-writably
> > directories and DIR:/run/user/$UID is exactly this case.
> >
> > This means we need to examine the ccache as root in order to determine
> > whether to re-create it...with the attached patches the DIR cache seems
> > to work fine.
> > > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the ccache
> > > once the reply is returned to the backend. But I am afraid deleting a
> > > ccache may also require root privs as the ccache is owned by the user
> > > but the sssd backend is running as the sssd user. Or am I missing
> > > another change that addresses this ?
> > >
> > > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> > > ERR_CREDS_EXPIRED, why (and why does it need to be different) ?
> >
> > No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The krb5_child
> > is able to remove the old ccache from disk or from the kernel keyring,
> > but in case the child dropped privileges to the user who is logging in,
> > it cannot delete the ccache attribute from sysdb..so
> > ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to remove this
> > attribute istead.
> >
> > Thank you for the review. New patches are attached (and I just realized
> > I didn't CC you in my reply to your other mail; sorry)
> > From 7addb7d573a2ef06f60e0c026f4bd01423d073ea Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Fri, 24 Oct 2014 22:44:17 +0200
> > Subject: [PATCH 1/6] BUILD: Install krb5_child as suid if running under
> > non-privileged user
> >
> > If sssd_be is running unprivileged, then krb5_child must be setuid to be
> > able to access the keytab and become arbitrary user.
>
> There is a crash in krb_child with refactoring patches.
>
> #0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
> 222         if (ccname[0] == '/') {
> (gdb) p ccname
> $1 = 0x0
>
> (gdb) bt full
> #0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
>        tmp_ctx = 0x0
>        filename = <value optimized out>
>        ccdirname = <value optimized out>
>        end = <value optimized out>
>        ret = <value optimized out>
>        __FUNCTION__ = "sss_krb5_precreate_ccache"
> #1  0x0000000000405adc in k5c_precreate_ccache (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:1993
>        ret = <value optimized out>
> #2  k5c_setup (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:2042
>        kerr = <value optimized out>
>        parse_flags = <value optimized out>
>        fast_val = K5C_FAST_NEVER
>        __FUNCTION__ = "k5c_setup"
> #3  0x0000000000408a86 in main (argc=5, argv=<value optimized out>) at src/providers/krb5/krb5_child.c:2246
>        kr = 0x785450
>        offline = 0
>        opt = <value optimized out>
>        pc = <value optimized out>
>        debug_fd = 18
>        ret = <value optimized out>
>        long_options = {{longName = 0x0, shortName = 0 '\000', argInfo = 4, arg = 0x617980, val = 0, descrip = 0x4121e9 "Help options:", argDescrip = 0x0}, {
>            longName = 0x4121f7 "debug-level", shortName = 100 'd', argInfo = 2, arg = 0x617a40, val = 0, descrip = 0x4121c8 "Debug level", argDescrip = 0x0}, {
>            longName = 0x412203 "debug-timestamps", shortName = 0 '\000', argInfo = 2, arg = 0x617960, val = 0, descrip = 0x4121d4 "Add debug timestamps", argDescrip = 0x0}, {
>            longName = 0x412214 "debug-microseconds", shortName = 0 '\000', argInfo = 2, arg = 0x617a50, val = 0, descrip = 0x412e18 "Show timestamps with microseconds", 
>            argDescrip = 0x0}, {longName = 0x412227 "debug-fd", shortName = 0 '\000', argInfo = 2, arg = 0x7fff7a3bed08, val = 0, 
>            descrip = 0x412e40 "An open file descriptor for the debug logs", argDescrip = 0x0}, {longName = 0x412230 "debug-to-stderr", shortName = 0 '\000', argInfo = 1073741824, 
>            arg = 0x617a58, val = 0, descrip = 0x412e70 "Send the debug output to stderr directly.", argDescrip = 0x0}, {longName = 0x0, shortName = 0 '\000', argInfo = 0, arg = 0x0, 
>            val = 0, descrip = 0x0, argDescrip = 0x0}}
>        __FUNCTION__ = "main"

and here is related part from krb5_child log.

[sss_get_ccache_name_for_principal] (0x4000): Location: [FILE:/tmp/krb5cc_2004_XXXXXX]
[sss_get_ccache_name_for_principal] (0x2000): krb5_cc_cache_match failed: [-1765328243][Can't find client principal [e-mail] in cache collection]
[create_ccache] (0x4000): Initializing ccache of type [FILE]
[switch_creds] (0x0200): Switch user to [2004][2004].
[switch_creds] (0x0200): Already user [2004].
[sss_krb5_check_ccache_princ] (0x2000): Searching for [[e-mail]] in cache of type [FILE]
[switch_creds] (0x0200): Switch user to [2004][2004].
[switch_creds] (0x0200): Already user [2004].
[sss_open_ccache_as_user] (0x0400): ccache (null) is missing or empty
[get_and_save_tgt] (0x0080): Failed to remove old ccache file [(null)], please remove it manually.
[k5c_send_data] (0x0200): Received error code 0
[pack_response_packet] (0x2000): response packet size: [122]
[k5c_send_data] (0x4000): Response sent.
[main] (0x0400): krb5_child completed successfully

[main] (0x0400): krb5_child started.
[unpack_buffer] (0x1000): total buffer size: [66]
[unpack_buffer] (0x0100): cmd [243] uid [2004] gid [2004] validate [false] enterprise principal [false] offline [false] UPN [[e-mail]]
[unpack_buffer] (0x0100): user: [testuser4]
[check_use_fast] (0x0100): Not using FAST.
[k5c_precreate_ccache] (0x4000): Recreating ccache

LS

[Jakub Hrozek]

On Tue, Nov 11, 2014 at 09:39:40AM +0100, Lukas Slebodnik wrote:
> On (11/11/14 09:09), Lukas Slebodnik wrote:
> > On (10/11/14 17:12), Jakub Hrozek wrote:
> > > On Thu, Nov 06, 2014 at 10:21:17AM -0500, Simo Sorce wrote:
> > > > On Wed, 5 Nov 2014 18:36:06 +0100
> > > > Jakub Hrozek  wrote:
> > > > > From 1afae1740eb9bf232c33dba77f643f88d0eeb7a3 Mon Sep 17 00:00:00 2001
> > > > > From: Jakub Hrozek 
> > > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > > > >
> > > > > The credential cache operations must be now performed by the
> > > > > krb5_child completely, because the sssd_be process might be running
> > > > > as the sssd user who doesn't have access to the ccaches.
> > > > >
> > > > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > > > until we fix Kerberos ticket renewal as non-root.
> > > > >
> > > > > Also includes a new error code that indicates that the back end should
> > > > > remove the old ccache attribute -- the child can't do that if it's
> > > > > running as the user.
> > > >
> > > > I think I see an ordering issue but I am not completely sure
> > > > (unfortunately I am looking only at the patches because git am fails to
> > > > bring them on a copy of the master tree).
> > >
> > > The patches depended on the SELinux changes that were since merged to
> > > master. The attached patchset should apply on master cleanly.
> > > > The main reason ccache handling was done in 2 parts is that in some
> > > > cases we need to do some preparatory work as root.
> > > > For example creating a user directory for DIR: ccache and similar.
> > > > I think this patches moves things around and causes all these functions
> > > > to be run after krb5_child has already dropped root in
> > > > k5c_setup() though. If that's the case as it looks to me I think this
> > > > patchset will not work as well as it should.
> > >
> > > You're right, I admit I tested on RHEL and Fedora where we only use
> > > KEYRING: and FILE:/tmp/ where I didn't hit this issue. I was also
> > > confused by our removal of creating public ccache dir components -- now
> > > I realize we can still create user-owned directories under root-writably
> > > directories and DIR:/run/user/$UID is exactly this case.
> > >
> > > This means we need to examine the ccache as root in order to determine
> > > whether to re-create it...with the attached patches the DIR cache seems
> > > to work fine.
> > > > I also see that you added ERR_CREDS_EXPIRED_CCACHE to delete the ccache
> > > > once the reply is returned to the backend. But I am afraid deleting a
> > > > ccache may also require root privs as the ccache is owned by the user
> > > > but the sssd backend is running as the sssd user. Or am I missing
> > > > another change that addresses this ?
> > > >
> > > > Also ERR_CREDS_EXPIRED_CCACHE seem not performing the same checks as
> > > > ERR_CREDS_EXPIRED, why (and why does it need to be different) ?
> > >
> > > No, the ERR_CREDS_EXPIRED_CCACHE semantics is different. The krb5_child
> > > is able to remove the old ccache from disk or from the kernel keyring,
> > > but in case the child dropped privileges to the user who is logging in,
> > > it cannot delete the ccache attribute from sysdb..so
> > > ERR_CREDS_EXPIRED_CCACHE is a way of telling sssd_be to remove this
> > > attribute istead.
> > >
> > > Thank you for the review. New patches are attached (and I just realized
> > > I didn't CC you in my reply to your other mail; sorry)
> > > From 7addb7d573a2ef06f60e0c026f4bd01423d073ea Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Fri, 24 Oct 2014 22:44:17 +0200
> > > Subject: [PATCH 1/6] BUILD: Install krb5_child as suid if running under
> > > non-privileged user
> > >
> > > If sssd_be is running unprivileged, then krb5_child must be setuid to be
> > > able to access the keytab and become arbitrary user.
> >
> > There is a crash in krb_child with refactoring patches.
> >
> > #0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
> > 222         if (ccname[0] == '/') {
> > (gdb) p ccname
> > $1 = 0x0
> >
> > (gdb) bt full
> > #0  sss_krb5_precreate_ccache (ccname=0x0, uid=2003, gid=2003) at src/providers/krb5/krb5_ccache.c:222
> >        tmp_ctx = 0x0
> >        filename = <value optimized out>
> >        ccdirname = <value optimized out>
> >        end = <value optimized out>
> >        ret = <value optimized out>
> >        __FUNCTION__ = "sss_krb5_precreate_ccache"
> > #1  0x0000000000405adc in k5c_precreate_ccache (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:1993
> >        ret = <value optimized out>
> > #2  k5c_setup (kr=0x785450, offline=0) at src/providers/krb5/krb5_child.c:2042
> >        kerr = <value optimized out>
> >        parse_flags = <value optimized out>
> >        fast_val = K5C_FAST_NEVER
> >        __FUNCTION__ = "k5c_setup"
> > #3  0x0000000000408a86 in main (argc=5, argv=<value optimized out>) at src/providers/krb5/krb5_child.c:2246
> >        kr = 0x785450
> >        offline = 0
> >        opt = <value optimized out>
> >        pc = <value optimized out>
> >        debug_fd = 18
> >        ret = <value optimized out>
> >        long_options = {{longName = 0x0, shortName = 0 '\000', argInfo = 4, arg = 0x617980, val = 0, descrip = 0x4121e9 "Help options:", argDescrip = 0x0}, {
> >            longName = 0x4121f7 "debug-level", shortName = 100 'd', argInfo = 2, arg = 0x617a40, val = 0, descrip = 0x4121c8 "Debug level", argDescrip = 0x0}, {
> >            longName = 0x412203 "debug-timestamps", shortName = 0 '\000', argInfo = 2, arg = 0x617960, val = 0, descrip = 0x4121d4 "Add debug timestamps", argDescrip = 0x0}, {
> >            longName = 0x412214 "debug-microseconds", shortName = 0 '\000', argInfo = 2, arg = 0x617a50, val = 0, descrip = 0x412e18 "Show timestamps with microseconds", 
> >            argDescrip = 0x0}, {longName = 0x412227 "debug-fd", shortName = 0 '\000', argInfo = 2, arg = 0x7fff7a3bed08, val = 0, 
> >            descrip = 0x412e40 "An open file descriptor for the debug logs", argDescrip = 0x0}, {longName = 0x412230 "debug-to-stderr", shortName = 0 '\000', argInfo = 1073741824, 
> >            arg = 0x617a58, val = 0, descrip = 0x412e70 "Send the debug output to stderr directly.", argDescrip = 0x0}, {longName = 0x0, shortName = 0 '\000', argInfo = 0, arg = 0x0, 
> >            val = 0, descrip = 0x0, argDescrip = 0x0}}
> >        __FUNCTION__ = "main"
>
> and here is related part from krb5_child log.
>
> [sss_get_ccache_name_for_principal] (0x4000): Location: [FILE:/tmp/krb5cc_2004_XXXXXX]
> [sss_get_ccache_name_for_principal] (0x2000): krb5_cc_cache_match failed: [-1765328243][Can't find client principal [e-mail] in cache collection]
> [create_ccache] (0x4000): Initializing ccache of type [FILE]
> [switch_creds] (0x0200): Switch user to [2004][2004].
> [switch_creds] (0x0200): Already user [2004].
> [sss_krb5_check_ccache_princ] (0x2000): Searching for [[e-mail]] in cache of type [FILE]
> [switch_creds] (0x0200): Switch user to [2004][2004].
> [switch_creds] (0x0200): Already user [2004].
> [sss_open_ccache_as_user] (0x0400): ccache (null) is missing or empty
> [get_and_save_tgt] (0x0080): Failed to remove old ccache file [(null)], please remove it manually.
> [k5c_send_data] (0x0200): Received error code 0
> [pack_response_packet] (0x2000): response packet size: [122]
> [k5c_send_data] (0x4000): Response sent.
> [main] (0x0400): krb5_child completed successfully
>
> [main] (0x0400): krb5_child started.
> [unpack_buffer] (0x1000): total buffer size: [66]
> [unpack_buffer] (0x0100): cmd [243] uid [2004] gid [2004] validate [false] enterprise principal [false] offline [false] UPN [[e-mail]]
> [unpack_buffer] (0x0100): user: [testuser4]
> [check_use_fast] (0x0100): Not using FAST.
> [k5c_precreate_ccache] (0x4000): Recreating ccache

Can you give me access to a host that reproduces this crash? ccname
should never be NULL with the new patches ...

[Jakub Hrozek]

On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> Can you give me access to a host that reproduces this crash? ccname
> should never be NULL with the new patches ...

..except on access_provider=krb5...

Thanks for catching that, new patches are attached.

[Lukas Slebodnik]

On (11/11/14 13:45), Jakub Hrozek wrote:
> On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > Can you give me access to a host that reproduces this crash? ccname
> > should never be NULL with the new patches ...
>
> ..except on access_provider=krb5...
>
> Thanks for catching that, new patches are attached.

There is problem with support of enterprise principals.
authentication for such users failed.
> From 13a1171ad66456d4045159ed1b675099be84c55a Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:13 +0200
> Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
>
> The credential cache operations must be now performed by the krb5_child
> completely, because the sssd_be process might be running as the sssd
> user who doesn't have access to the ccaches.
>
> src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> until we fix Kerberos ticket renewal as non-root.
>
> Also includes a new error code that indicates that the back end should
> remove the old ccache attribute -- the child can't do that if it's
> running as the user.
>
> Makefile.am                             |  14 +-
> src/providers/krb5/krb5_auth.c          | 222 ++++----------------------------
> src/providers/krb5/krb5_child.c         | 209 ++++++++++++++++++++++++++++--
> src/providers/krb5/krb5_child_handler.c |  13 ++
> src/tests/krb5_child-test.c             |   3 +-
> src/util/util_errors.c                  |   1 +
> src/util/util_errors.h                  |   1 +
> 7 files changed, 257 insertions(+), 206 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 41b1843baee41ea8a67eb47e6786286190bbdcb9..c6601468e79244aad0a1bb8d7c87190aad9ef61a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1604,6 +1604,7 @@ krb5_child_test_SOURCES = \
>     src/providers/krb5/krb5_child_handler.c \
>     src/providers/krb5/krb5_common.c \
>     src/util/sss_krb5.c \
> src/util/find_uid.c \
> src/providers/data_provider_fo.c \
> src/providers/data_provider_opts.c \
> src/providers/data_provider_callbacks.c \

This change is not necessary and, because krb5_child test is already linked
with libsss_util, which contains this file.

Moreover, this change caused compilation failures on platforms with disabled
link_all_deplibs.

CCLD     krb5-child-test
/usr/bin/ld: src/util/krb5_child_test-find_uid.o: undefined reference to symbol 'sd_uid_get_sessions@@LIBSYSTEMD_209'
/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

[Jakub Hrozek]

On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> On (11/11/14 13:45), Jakub Hrozek wrote:
> > On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > Can you give me access to a host that reproduces this crash? ccname
> > > should never be NULL with the new patches ...
> >
> > ..except on access_provider=krb5...
> >
> > Thanks for catching that, new patches are attached.
>
> There is problem with support of enterprise principals.
> authentication for such users failed.

Thanks a lot for catching that, I had no trouble logging in as a user
from a child domain, but I could reproduce the issue when I used a
completely different suffix.

I'll work on a fix.

[Jakub Hrozek]

On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > Can you give me access to a host that reproduces this crash? ccname
> > > > should never be NULL with the new patches ...
> > >
> > > ..except on access_provider=krb5...
> > >
> > > Thanks for catching that, new patches are attached.
> >
> > There is problem with support of enterprise principals.
> > authentication for such users failed.
>
> Thanks a lot for catching that, I had no trouble logging in as a user
> from a child domain, but I could reproduce the issue when I used a
> completely different suffix.
>
> I'll work on a fix.

Thanks again for the catch, can you test this additional fix on top of
your patches? (Sorry for sending a separate patch, I want to get a fresh
look at the set tomorrow, some other krb5_ccache.c functions might get
the same treatment)

[Lukas Slebodnik]

On (11/11/14 22:37), Jakub Hrozek wrote:
> On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > Can you give me access to a host that reproduces this crash? ccname
> > > > > should never be NULL with the new patches ...
> > > >
> > > > ..except on access_provider=krb5...
> > > >
> > > > Thanks for catching that, new patches are attached.
> > >
> > > There is problem with support of enterprise principals.
> > > authentication for such users failed.
> >
> > Thanks a lot for catching that, I had no trouble logging in as a user
> > from a child domain, but I could reproduce the issue when I used a
> > completely different suffix.
> >
> > I'll work on a fix.
>
> Thanks again for the catch, can you test this additional fix on top of
> your patches? (Sorry for sending a separate patch, I want to get a fresh
> look at the set tomorrow, some other krb5_ccache.c functions might get
> the same treatment)
> From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Tue, 11 Nov 2014 22:33:28 +0100
> Subject: [PATCH] sss_krb5_check_ccache_princ fix

krb5 + ad tests passed with this patch.

LS

[Jakub Hrozek]

On Wed, Nov 12, 2014 at 01:45:26PM +0100, Lukas Slebodnik wrote:
> On (11/11/14 22:37), Jakub Hrozek wrote:
> > On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > > On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > > Can you give me access to a host that reproduces this crash? ccname
> > > > > > should never be NULL with the new patches ...
> > > > >
> > > > > ..except on access_provider=krb5...
> > > > >
> > > > > Thanks for catching that, new patches are attached.
> > > >
> > > > There is problem with support of enterprise principals.
> > > > authentication for such users failed.
> > >
> > > Thanks a lot for catching that, I had no trouble logging in as a user
> > > from a child domain, but I could reproduce the issue when I used a
> > > completely different suffix.
> > >
> > > I'll work on a fix.
> >
> > Thanks again for the catch, can you test this additional fix on top of
> > your patches? (Sorry for sending a separate patch, I want to get a fresh
> > look at the set tomorrow, some other krb5_ccache.c functions might get
> > the same treatment)
> > From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Tue, 11 Nov 2014 22:33:28 +0100
> > Subject: [PATCH] sss_krb5_check_ccache_princ fix
>
> krb5 + ad tests passed with this patch.
>
> LS

Thanks, the whole patchset is attached again.

[Lukas Slebodnik]

On (12/11/14 15:44), Jakub Hrozek wrote:
> On Wed, Nov 12, 2014 at 01:45:26PM +0100, Lukas Slebodnik wrote:
> > On (11/11/14 22:37), Jakub Hrozek wrote:
> > > On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > > > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > > > On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > > > Can you give me access to a host that reproduces this crash? ccname
> > > > > > > should never be NULL with the new patches ...
> > > > > >
> > > > > > ..except on access_provider=krb5...
> > > > > >
> > > > > > Thanks for catching that, new patches are attached.
> > > > >
> > > > > There is problem with support of enterprise principals.
> > > > > authentication for such users failed.
> > > >
> > > > Thanks a lot for catching that, I had no trouble logging in as a user
> > > > from a child domain, but I could reproduce the issue when I used a
> > > > completely different suffix.
> > > >
> > > > I'll work on a fix.
> > >
> > > Thanks again for the catch, can you test this additional fix on top of
> > > your patches? (Sorry for sending a separate patch, I want to get a fresh
> > > look at the set tomorrow, some other krb5_ccache.c functions might get
> > > the same treatment)
> > > From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Tue, 11 Nov 2014 22:33:28 +0100
> > > Subject: [PATCH] sss_krb5_check_ccache_princ fix
> >
> > krb5 + ad tests passed with this patch.
> >
> > LS
>
> Thanks, the whole patchset is attached again.
> From 7722435d86ab57c91a58028845fe781bc28605e0 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:01 +0200
> Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
>
> Otherwise we would have to link krb5_child with pcre and transfer the
> regex, which wold be cumbersome. Check for illegal patterns when
> expanding the template instead.
>
> src/providers/krb5/krb5_ccache.c | 38 ++------------------
> src/providers/krb5/krb5_ccache.h |  7 +---
> src/providers/krb5/krb5_utils.c  | 36 +++++++++++++++++--
> src/providers/krb5/krb5_utils.h  |  4 +--
> src/tests/krb5_utils-tests.c     | 78 ++++++++++++++++------------------------
> 5 files changed, 69 insertions(+), 94 deletions(-)

Our CI does not like this patch.

src/providers/krb5/krb5_auth.c: In function 'krb5_auth_prepare_ccache_name':
src/providers/krb5/krb5_auth.c:305:74: error: passing argument 4 of 'expand_ccname_template' makes pointer from integer without a cast [-Werror]
             kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
                                                                          ^
In file included from src/providers/krb5/krb5_auth.c:41:0:
src/providers/krb5/krb5_utils.h:45:7: note: expected 'struct pcre *' but argument is of type 'int'
 char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
       ^
src/providers/krb5/krb5_auth.c:305:26: error: too few arguments to function 'expand_ccname_template'
             kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
                          ^
In file included from src/providers/krb5/krb5_auth.c:41:0:
src/providers/krb5/krb5_utils.h:45:7: note: declared here
 char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
       ^
../sssd/src/providers/krb5/krb5_auth.c:313:45: error: passing argument 2 of 'sss_krb5_precreate_ccache' makes integer from pointer without a cast [-Werror]
                                             kr->krb5_ctx->illegal_path_re,
                                             ^
In file included from src/providers/krb5/krb5_auth.h:35:0,
                 from src/providers/krb5/krb5_auth.c:40:
src/providers/krb5/krb5_ccache.h:38:9: note: expected 'uid_t' but argument is of type 'struct pcre *'
 errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
         ^
src/providers/krb5/krb5_auth.c:312:19: error: too many arguments to function 'sss_krb5_precreate_ccache'
             ret = sss_krb5_precreate_ccache(kr->ccname,
                   ^
In file included from src/providers/krb5/krb5_auth.h:35:0,
                 from src/providers/krb5/krb5_auth.c:40:
src/providers/krb5/krb5_ccache.h:38:9: note: declared here
 errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
         ^
cc1: all warnings being treated as errors
Makefile:10727: recipe for target 'src/providers/krb5/libsss_krb5_common_la-krb5_auth.lo' failed
> From 6a854dab8a1affbc9af27b46591a0d8562829afb Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:13 +0200
> Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
>
> The credential cache operations must be now performed by the krb5_child
> completely, because the sssd_be process might be running as the sssd
> user who doesn't have access to the ccaches.
>
> src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> until we fix Kerberos ticket renewal as non-root.
>
> Also includes a new error code that indicates that the back end should
> remove the old ccache attribute -- the child can't do that if it's
> running as the user.
>
> Makefile.am                             |  14 +-
> src/providers/krb5/krb5_auth.c          | 222 ++++----------------------------
> src/providers/krb5/krb5_ccache.c        |  62 ++++-----
> src/providers/krb5/krb5_ccache.h        |   5 +-
> src/providers/krb5/krb5_child.c         | 208 ++++++++++++++++++++++++++++--
> src/providers/krb5/krb5_child_handler.c |  13 ++
> src/tests/krb5_child-test.c             |   3 +-
> src/util/util_errors.c                  |   1 +
> src/util/util_errors.h                  |   1 +
> 9 files changed, 282 insertions(+), 247 deletions(-)
>
> diff --git a/Makefile.am b/Makefile.am
> index 41b1843baee41ea8a67eb47e6786286190bbdcb9..c6601468e79244aad0a1bb8d7c87190aad9ef61a 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -1604,6 +1604,7 @@ krb5_child_test_SOURCES = \
>     src/providers/krb5/krb5_child_handler.c \
>     src/providers/krb5/krb5_common.c \
>     src/util/sss_krb5.c \
> src/util/find_uid.c \
> src/providers/data_provider_fo.c \
> src/providers/data_provider_opts.c \
> src/providers/data_provider_callbacks.c \
>
> @@ -2491,27 +2492,36 @@ libsss_ad_la_LDFLAGS = \

This change is not necessary and, because krb5_child test is already linked
with libsss_util, which contains this file.

Moreover, this change caused compilation failures on platforms with disabled
link_all_deplibs.

CCLD     krb5-child-test
/usr/bin/ld: src/util/krb5_child_test-find_uid.o: undefined reference to symbol
+'sd_uid_get_sessions@@LIBSYSTEMD_209'
/lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
collect2: error: ld returned 1 exit status

LS

[Jakub Hrozek]

On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> On (12/11/14 15:44), Jakub Hrozek wrote:
> > On Wed, Nov 12, 2014 at 01:45:26PM +0100, Lukas Slebodnik wrote:
> > > On (11/11/14 22:37), Jakub Hrozek wrote:
> > > > On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > > > > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > > > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > > > >On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > > >> Can you give me access to a host that reproduces this crash? ccname
> > > > > > >> should never be NULL with the new patches ...
> > > > > > >
> > > > > > >..except on access_provider=krb5...
> > > > > > >
> > > > > > >Thanks for catching that, new patches are attached.
> > > > > >
> > > > > > There is problem with support of enterprise principals.
> > > > > > authentication for such users failed.
> > > > >
> > > > > Thanks a lot for catching that, I had no trouble logging in as a user
> > > > > from a child domain, but I could reproduce the issue when I used a
> > > > > completely different suffix.
> > > > >
> > > > > I'll work on a fix.
> > > >
> > > > Thanks again for the catch, can you test this additional fix on top of
> > > > your patches? (Sorry for sending a separate patch, I want to get a fresh
> > > > look at the set tomorrow, some other krb5_ccache.c functions might get
> > > > the same treatment)
> > > > From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Tue, 11 Nov 2014 22:33:28 +0100
> > > > Subject: [PATCH] sss_krb5_check_ccache_princ fix
> > >
> > > krb5 + ad tests passed with this patch.
> > >
> > > LS
> >
> > Thanks, the whole patchset is attached again.
> > From 7722435d86ab57c91a58028845fe781bc28605e0 Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sat, 18 Oct 2014 22:03:01 +0200
> > Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> >
> > Otherwise we would have to link krb5_child with pcre and transfer the
> > regex, which wold be cumbersome. Check for illegal patterns when
> > expanding the template instead.
> >
> > src/providers/krb5/krb5_ccache.c | 38 ++------------------
> > src/providers/krb5/krb5_ccache.h |  7 +---
> > src/providers/krb5/krb5_utils.c  | 36 +++++++++++++++++--
> > src/providers/krb5/krb5_utils.h  |  4 +--
> > src/tests/krb5_utils-tests.c     | 78 ++++++++++++++++------------------------
> > 5 files changed, 69 insertions(+), 94 deletions(-)
>
> Our CI does not like this patch.
>
> src/providers/krb5/krb5_auth.c: In function 'krb5_auth_prepare_ccache_name':
> src/providers/krb5/krb5_auth.c:305:74: error: passing argument 4 of 'expand_ccname_template' makes pointer from integer without a cast [-Werror]
>              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
>                                                                           ^
> In file included from src/providers/krb5/krb5_auth.c:41:0:
> src/providers/krb5/krb5_utils.h:45:7: note: expected 'struct pcre *' but argument is of type 'int'
>  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
>        ^
> src/providers/krb5/krb5_auth.c:305:26: error: too few arguments to function 'expand_ccname_template'
>              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
>                           ^
> In file included from src/providers/krb5/krb5_auth.c:41:0:
> src/providers/krb5/krb5_utils.h:45:7: note: declared here
>  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
>        ^
> ../sssd/src/providers/krb5/krb5_auth.c:313:45: error: passing argument 2 of 'sss_krb5_precreate_ccache' makes integer from pointer without a cast [-Werror]
>                                              kr->krb5_ctx->illegal_path_re,
>                                              ^
> In file included from src/providers/krb5/krb5_auth.h:35:0,
>                  from src/providers/krb5/krb5_auth.c:40:
> src/providers/krb5/krb5_ccache.h:38:9: note: expected 'uid_t' but argument is of type 'struct pcre *'
>  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
>          ^
> src/providers/krb5/krb5_auth.c:312:19: error: too many arguments to function 'sss_krb5_precreate_ccache'
>              ret = sss_krb5_precreate_ccache(kr->ccname,
>                    ^
> In file included from src/providers/krb5/krb5_auth.h:35:0,
>                  from src/providers/krb5/krb5_auth.c:40:
> src/providers/krb5/krb5_ccache.h:38:9: note: declared here
>  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
>          ^
> cc1: all warnings being treated as errors
> Makefile:10727: recipe for target 'src/providers/krb5/libsss_krb5_common_la-krb5_auth.lo' failed
> > From 6a854dab8a1affbc9af27b46591a0d8562829afb Mon Sep 17 00:00:00 2001
> > From: Jakub Hrozek 
> > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> >
> > The credential cache operations must be now performed by the krb5_child
> > completely, because the sssd_be process might be running as the sssd
> > user who doesn't have access to the ccaches.
> >
> > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > until we fix Kerberos ticket renewal as non-root.
> >
> > Also includes a new error code that indicates that the back end should
> > remove the old ccache attribute -- the child can't do that if it's
> > running as the user.
> >
> > Makefile.am                             |  14 +-
> > src/providers/krb5/krb5_auth.c          | 222 ++++----------------------------
> > src/providers/krb5/krb5_ccache.c        |  62 ++++-----
> > src/providers/krb5/krb5_ccache.h        |   5 +-
> > src/providers/krb5/krb5_child.c         | 208 ++++++++++++++++++++++++++++--
> > src/providers/krb5/krb5_child_handler.c |  13 ++
> > src/tests/krb5_child-test.c             |   3 +-
> > src/util/util_errors.c                  |   1 +
> > src/util/util_errors.h                  |   1 +
> > 9 files changed, 282 insertions(+), 247 deletions(-)
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 41b1843baee41ea8a67eb47e6786286190bbdcb9..c6601468e79244aad0a1bb8d7c87190aad9ef61a 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -1604,6 +1604,7 @@ krb5_child_test_SOURCES = \
> >     src/providers/krb5/krb5_child_handler.c \
> >     src/providers/krb5/krb5_common.c \
> >     src/util/sss_krb5.c \
> > src/util/find_uid.c \
> > src/providers/data_provider_fo.c \
> > src/providers/data_provider_opts.c \
> > src/providers/data_provider_callbacks.c \
> >
> > @@ -2491,27 +2492,36 @@ libsss_ad_la_LDFLAGS = \
>
> This change is not necessary and, because krb5_child test is already linked
> with libsss_util, which contains this file.
>
> Moreover, this change caused compilation failures on platforms with disabled
> link_all_deplibs.
>
> CCLD     krb5-child-test
> /usr/bin/ld: src/util/krb5_child_test-find_uid.o: undefined reference to symbol
> +'sd_uid_get_sessions@@LIBSYSTEMD_209'
> /lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
> collect2: error: ld returned 1 exit status
>
> LS

Thank you, see the attached patches.

[Jakub Hrozek]

On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> > On (12/11/14 15:44), Jakub Hrozek wrote:
> > > On Wed, Nov 12, 2014 at 01:45:26PM +0100, Lukas Slebodnik wrote:
> > > > On (11/11/14 22:37), Jakub Hrozek wrote:
> > > > > On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > > > > > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > > > > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > > > > >On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > > > >> Can you give me access to a host that reproduces this crash? ccname
> > > > > > > >> should never be NULL with the new patches ...
> > > > > > > >
> > > > > > > >..except on access_provider=krb5...
> > > > > > > >
> > > > > > > >Thanks for catching that, new patches are attached.
> > > > > > > 
> > > > > > > There is problem with support of enterprise principals.
> > > > > > > authentication for such users failed.
> > > > > >
> > > > > > Thanks a lot for catching that, I had no trouble logging in as a user
> > > > > > from a child domain, but I could reproduce the issue when I used a
> > > > > > completely different suffix.
> > > > > >
> > > > > > I'll work on a fix.
> > > > >
> > > > > Thanks again for the catch, can you test this additional fix on top of
> > > > > your patches? (Sorry for sending a separate patch, I want to get a fresh
> > > > > look at the set tomorrow, some other krb5_ccache.c functions might get
> > > > > the same treatment)
> > > > > From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> > > > > From: Jakub Hrozek 
> > > > > Date: Tue, 11 Nov 2014 22:33:28 +0100
> > > > > Subject: [PATCH] sss_krb5_check_ccache_princ fix
> > > >
> > > > krb5 + ad tests passed with this patch.
> > > >
> > > > LS
> > >
> > > Thanks, the whole patchset is attached again.
> > > From 7722435d86ab57c91a58028845fe781bc28605e0 Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sat, 18 Oct 2014 22:03:01 +0200
> > > Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> > >
> > > Otherwise we would have to link krb5_child with pcre and transfer the
> > > regex, which wold be cumbersome. Check for illegal patterns when
> > > expanding the template instead.
> > >
> > > src/providers/krb5/krb5_ccache.c | 38 ++------------------
> > > src/providers/krb5/krb5_ccache.h |  7 +---
> > > src/providers/krb5/krb5_utils.c  | 36 +++++++++++++++++--
> > > src/providers/krb5/krb5_utils.h  |  4 +--
> > > src/tests/krb5_utils-tests.c     | 78 ++++++++++++++++------------------------
> > > 5 files changed, 69 insertions(+), 94 deletions(-)
> >
> > Our CI does not like this patch.
> >
> > src/providers/krb5/krb5_auth.c: In function 'krb5_auth_prepare_ccache_name':
> > src/providers/krb5/krb5_auth.c:305:74: error: passing argument 4 of 'expand_ccname_template' makes pointer from integer without a cast [-Werror]
> >              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> >                                                                           ^
> > In file included from src/providers/krb5/krb5_auth.c:41:0:
> > src/providers/krb5/krb5_utils.h:45:7: note: expected 'struct pcre *' but argument is of type 'int'
> >  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
> >        ^
> > src/providers/krb5/krb5_auth.c:305:26: error: too few arguments to function 'expand_ccname_template'
> >              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> >                           ^
> > In file included from src/providers/krb5/krb5_auth.c:41:0:
> > src/providers/krb5/krb5_utils.h:45:7: note: declared here
> >  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
> >        ^
> > ../sssd/src/providers/krb5/krb5_auth.c:313:45: error: passing argument 2 of 'sss_krb5_precreate_ccache' makes integer from pointer without a cast [-Werror]
> >                                              kr->krb5_ctx->illegal_path_re,
> >                                              ^
> > In file included from src/providers/krb5/krb5_auth.h:35:0,
> >                  from src/providers/krb5/krb5_auth.c:40:
> > src/providers/krb5/krb5_ccache.h:38:9: note: expected 'uid_t' but argument is of type 'struct pcre *'
> >  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
> >          ^
> > src/providers/krb5/krb5_auth.c:312:19: error: too many arguments to function 'sss_krb5_precreate_ccache'
> >              ret = sss_krb5_precreate_ccache(kr->ccname,
> >                    ^
> > In file included from src/providers/krb5/krb5_auth.h:35:0,
> >                  from src/providers/krb5/krb5_auth.c:40:
> > src/providers/krb5/krb5_ccache.h:38:9: note: declared here
> >  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
> >          ^
> > cc1: all warnings being treated as errors
> > Makefile:10727: recipe for target 'src/providers/krb5/libsss_krb5_common_la-krb5_auth.lo' failed
> > > From 6a854dab8a1affbc9af27b46591a0d8562829afb Mon Sep 17 00:00:00 2001
> > > From: Jakub Hrozek 
> > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > >
> > > The credential cache operations must be now performed by the krb5_child
> > > completely, because the sssd_be process might be running as the sssd
> > > user who doesn't have access to the ccaches.
> > >
> > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > until we fix Kerberos ticket renewal as non-root.
> > >
> > > Also includes a new error code that indicates that the back end should
> > > remove the old ccache attribute -- the child can't do that if it's
> > > running as the user.
> > >
> > > Makefile.am                             |  14 +-
> > > src/providers/krb5/krb5_auth.c          | 222 ++++----------------------------
> > > src/providers/krb5/krb5_ccache.c        |  62 ++++-----
> > > src/providers/krb5/krb5_ccache.h        |   5 +-
> > > src/providers/krb5/krb5_child.c         | 208 ++++++++++++++++++++++++++++--
> > > src/providers/krb5/krb5_child_handler.c |  13 ++
> > > src/tests/krb5_child-test.c             |   3 +-
> > > src/util/util_errors.c                  |   1 +
> > > src/util/util_errors.h                  |   1 +
> > > 9 files changed, 282 insertions(+), 247 deletions(-)
> > >
> > > diff --git a/Makefile.am b/Makefile.am
> > > index 41b1843baee41ea8a67eb47e6786286190bbdcb9..c6601468e79244aad0a1bb8d7c87190aad9ef61a 100644
> > > --- a/Makefile.am
> > > +++ b/Makefile.am
> > > @@ -1604,6 +1604,7 @@ krb5_child_test_SOURCES = \
> > >     src/providers/krb5/krb5_child_handler.c \
> > >     src/providers/krb5/krb5_common.c \
> > >     src/util/sss_krb5.c \
> > > src/util/find_uid.c \
> > > src/providers/data_provider_fo.c \
> > > src/providers/data_provider_opts.c \
> > > src/providers/data_provider_callbacks.c \
> > >
> > > @@ -2491,27 +2492,36 @@ libsss_ad_la_LDFLAGS = \
> >
> > This change is not necessary and, because krb5_child test is already linked
> > with libsss_util, which contains this file.
> >
> > Moreover, this change caused compilation failures on platforms with disabled
> > link_all_deplibs.
> >
> > CCLD     krb5-child-test
> > /usr/bin/ld: src/util/krb5_child_test-find_uid.o: undefined reference to symbol
> > +'sd_uid_get_sessions@@LIBSYSTEMD_209'
> > /lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
> > collect2: error: ld returned 1 exit status
> >
> > LS
>
> Thank you, see the attached patches.

I forgot to remove the extra find_uid.c from Makefile.am

[Lukas Slebodnik]

On (14/11/14 13:52), Jakub Hrozek wrote:
> On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> > > On (12/11/14 15:44), Jakub Hrozek wrote:
> > > > On Wed, Nov 12, 2014 at 01:45:26PM +0100, Lukas Slebodnik wrote:
> > > > > On (11/11/14 22:37), Jakub Hrozek wrote:
> > > > > > On Tue, Nov 11, 2014 at 09:11:45PM +0100, Jakub Hrozek wrote:
> > > > > > > On Tue, Nov 11, 2014 at 06:23:24PM +0100, Lukas Slebodnik wrote:
> > > > > > > > On (11/11/14 13:45), Jakub Hrozek wrote:
> > > > > > > > >On Tue, Nov 11, 2014 at 11:15:30AM +0100, Jakub Hrozek wrote:
> > > > > > > > >> Can you give me access to a host that reproduces this crash? ccname
> > > > > > > > >> should never be NULL with the new patches ...
> > > > > > > > >
> > > > > > > > >..except on access_provider=krb5...
> > > > > > > > >
> > > > > > > > >Thanks for catching that, new patches are attached.
> > > > > > > > 
> > > > > > > > There is problem with support of enterprise principals.
> > > > > > > > authentication for such users failed.
> > > > > > > 
> > > > > > > Thanks a lot for catching that, I had no trouble logging in as a user
> > > > > > > from a child domain, but I could reproduce the issue when I used a
> > > > > > > completely different suffix.
> > > > > > > 
> > > > > > > I'll work on a fix.
> > > > > >
> > > > > > Thanks again for the catch, can you test this additional fix on top of
> > > > > > your patches? (Sorry for sending a separate patch, I want to get a fresh
> > > > > > look at the set tomorrow, some other krb5_ccache.c functions might get
> > > > > > the same treatment)
> > > > > > From 5d95f998643d875bcea149dde5e7a16aa42063b4 Mon Sep 17 00:00:00 2001
> > > > > > From: Jakub Hrozek 
> > > > > > Date: Tue, 11 Nov 2014 22:33:28 +0100
> > > > > > Subject: [PATCH] sss_krb5_check_ccache_princ fix
> > > > >
> > > > > krb5 + ad tests passed with this patch.
> > > > >
> > > > > LS
> > > >
> > > > Thanks, the whole patchset is attached again.
> > > > From 7722435d86ab57c91a58028845fe781bc28605e0 Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Sat, 18 Oct 2014 22:03:01 +0200
> > > > Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
> > > >
> > > > Otherwise we would have to link krb5_child with pcre and transfer the
> > > > regex, which wold be cumbersome. Check for illegal patterns when
> > > > expanding the template instead.
> > > >
> > > > src/providers/krb5/krb5_ccache.c | 38 ++------------------
> > > > src/providers/krb5/krb5_ccache.h |  7 +---
> > > > src/providers/krb5/krb5_utils.c  | 36 +++++++++++++++++--
> > > > src/providers/krb5/krb5_utils.h  |  4 +--
> > > > src/tests/krb5_utils-tests.c     | 78 ++++++++++++++++------------------------
> > > > 5 files changed, 69 insertions(+), 94 deletions(-)
> > >
> > > Our CI does not like this patch.
> > >
> > > src/providers/krb5/krb5_auth.c: In function 'krb5_auth_prepare_ccache_name':
> > > src/providers/krb5/krb5_auth.c:305:74: error: passing argument 4 of 'expand_ccname_template' makes pointer from integer without a cast [-Werror]
> > >              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> > >                                                                           ^
> > > In file included from src/providers/krb5/krb5_auth.c:41:0:
> > > src/providers/krb5/krb5_utils.h:45:7: note: expected 'struct pcre *' but argument is of type 'int'
> > >  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
> > >        ^
> > > src/providers/krb5/krb5_auth.c:305:26: error: too few arguments to function 'expand_ccname_template'
> > >              kr->ccname = expand_ccname_template(kr, kr, ccname_template, true,
> > >                           ^
> > > In file included from src/providers/krb5/krb5_auth.c:41:0:
> > > src/providers/krb5/krb5_utils.h:45:7: note: declared here
> > >  char *expand_ccname_template(TALLOC_CTX *mem_ctx, struct krb5child_req *kr,
> > >        ^
> > > ../sssd/src/providers/krb5/krb5_auth.c:313:45: error: passing argument 2 of 'sss_krb5_precreate_ccache' makes integer from pointer without a cast [-Werror]
> > >                                              kr->krb5_ctx->illegal_path_re,
> > >                                              ^
> > > In file included from src/providers/krb5/krb5_auth.h:35:0,
> > >                  from src/providers/krb5/krb5_auth.c:40:
> > > src/providers/krb5/krb5_ccache.h:38:9: note: expected 'uid_t' but argument is of type 'struct pcre *'
> > >  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
> > >          ^
> > > src/providers/krb5/krb5_auth.c:312:19: error: too many arguments to function 'sss_krb5_precreate_ccache'
> > >              ret = sss_krb5_precreate_ccache(kr->ccname,
> > >                    ^
> > > In file included from src/providers/krb5/krb5_auth.h:35:0,
> > >                  from src/providers/krb5/krb5_auth.c:40:
> > > src/providers/krb5/krb5_ccache.h:38:9: note: declared here
> > >  errno_t sss_krb5_precreate_ccache(const char *ccname, uid_t uid, gid_t gid);
> > >          ^
> > > cc1: all warnings being treated as errors
> > > Makefile:10727: recipe for target 'src/providers/krb5/libsss_krb5_common_la-krb5_auth.lo' failed
> > > > From 6a854dab8a1affbc9af27b46591a0d8562829afb Mon Sep 17 00:00:00 2001
> > > > From: Jakub Hrozek 
> > > > Date: Sat, 18 Oct 2014 22:03:13 +0200
> > > > Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
> > > >
> > > > The credential cache operations must be now performed by the krb5_child
> > > > completely, because the sssd_be process might be running as the sssd
> > > > user who doesn't have access to the ccaches.
> > > >
> > > > src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> > > > until we fix Kerberos ticket renewal as non-root.
> > > >
> > > > Also includes a new error code that indicates that the back end should
> > > > remove the old ccache attribute -- the child can't do that if it's
> > > > running as the user.
> > > >
> > > > Makefile.am                             |  14 +-
> > > > src/providers/krb5/krb5_auth.c          | 222 ++++----------------------------
> > > > src/providers/krb5/krb5_ccache.c        |  62 ++++-----
> > > > src/providers/krb5/krb5_ccache.h        |   5 +-
> > > > src/providers/krb5/krb5_child.c         | 208 ++++++++++++++++++++++++++++--
> > > > src/providers/krb5/krb5_child_handler.c |  13 ++
> > > > src/tests/krb5_child-test.c             |   3 +-
> > > > src/util/util_errors.c                  |   1 +
> > > > src/util/util_errors.h                  |   1 +
> > > > 9 files changed, 282 insertions(+), 247 deletions(-)
> > > >
> > > > diff --git a/Makefile.am b/Makefile.am
> > > > index 41b1843baee41ea8a67eb47e6786286190bbdcb9..c6601468e79244aad0a1bb8d7c87190aad9ef61a 100644
> > > > --- a/Makefile.am
> > > > +++ b/Makefile.am
> > > > @@ -1604,6 +1604,7 @@ krb5_child_test_SOURCES = \
> > > >     src/providers/krb5/krb5_child_handler.c \
> > > >     src/providers/krb5/krb5_common.c \
> > > >     src/util/sss_krb5.c \
> > > > src/util/find_uid.c \
> > > > src/providers/data_provider_fo.c \
> > > > src/providers/data_provider_opts.c \
> > > > src/providers/data_provider_callbacks.c \
> > > >
> > > > @@ -2491,27 +2492,36 @@ libsss_ad_la_LDFLAGS = \
> > >
> > > This change is not necessary and, because krb5_child test is already linked
> > > with libsss_util, which contains this file.
> > >
> > > Moreover, this change caused compilation failures on platforms with disabled
> > > link_all_deplibs.
> > >
> > > CCLD     krb5-child-test
> > > /usr/bin/ld: src/util/krb5_child_test-find_uid.o: undefined reference to symbol
> > > +'sd_uid_get_sessions@@LIBSYSTEMD_209'
> > > /lib64/libsystemd.so.0: error adding symbols: DSO missing from command line
> > > collect2: error: ld returned 1 exit status
> > >
> > > LS
> >
> > Thank you, see the attached patches.
>
> I forgot to remove the extra find_uid.c from Makefile.am

Tests passed even running sssd as unprivileged user (sssd).
Code looks good to me as well.
http://sssd-ci.duckdns.org/logs/job/3/57/summary.html

ACK

It might be good to have ACK from Sumit or Simo.

LS

[Sumit Bose]

On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:

...
> > Thank you, see the attached patches.
>
> I forgot to remove the extra find_uid.c from Makefile.am

I only have minor comments, see below. I understand that most part of
the patches are refactorings to make sure the related code is run with
the right permissions. (See next mail for some additional suggestions).

The ccache handling always was delicate, e.g. directory creation with
DIR type and determine if FILE with _XXXXXX should be recreated or
reused. I run couple to tests without an issue but I think only
regression testing might give clarity here. So ACK from my side.

bye,
Sumit
> From 242b9273014ade6d507a2f3e0d78e7e2a66f084e Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Mon, 13 Oct 2014 21:13:38 +0200
> Subject: [PATCH 2/6] KRB5: Drop privileges in the child, not the back end
>
> In future patches, sssd_be will be running as a non-privileged user, who
> will execute the setuid krb5_child. In this case, the child will start
> as root and drop the privileges as soon as possible.
>
> However, we need to also remove the privilege drop in sssd_be, because
> if we dropped to the user who is authenticating, we wouldn't be even
> allowed to execute krb5_child. The krb5_child permissions should be
> 4750, owned by root.sssd, to make sure only root and sssd can execute
> the child and if executed by sssd, the child will run as root.
>
> src/providers/krb5/krb5_child.c         | 71 ++++++++++++++++++++++++++-------
>  src/providers/krb5/krb5_child_handler.c |  8 ----
>  2 files changed, 57 insertions(+), 22 deletions(-)

...
>    use_fast_str = getenv(SSSD_KRB5_USE_FAST);
>    if (use_fast_str == NULL || strcasecmp(use_fast_str, "never") == 0) {
>        DEBUG(SSSDBG_CONF_SETTINGS, "Not using FAST.\n");
>    } else if (strcasecmp(use_fast_str, "try") == 0) {
>        kerr = k5c_setup_fast(kr, false);
>    } else if (strcasecmp(use_fast_str, "demand") == 0) {
>        kerr = k5c_setup_fast(kr, true);
>    } else {
>        DEBUG(SSSDBG_CRIT_FAILURE,
>              "Unsupported value [%s] for krb5_use_fast.\n",
>               use_fast_str);
>        return EINVAL;
>    if (fast_val > K5C_FAST_NEVER) {

I think    if (fast_val != K5C_FAST_NEVER) {
would be more readable here
>        kerr = k5c_setup_fast(kr, fast_val == K5C_FAST_DEMAND);
>        if (kerr != EOK) {
>            DEBUG(SSSDBG_OP_FAILURE, "Cannot set up FAST\n");
>            return kerr;
>        }
>    }
> }

...
> From 4fc2b7fd08c2f2ecfff8d559c06d62dd600655f7 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 20:52:43 +0200
> Subject: [PATCH 3/6] KRB5: Move ccache-related functions to krb5_ccache.c
>
> Add a new module krb5_ccache.c that contains all ccache-related
> operations. The only user of this module shall be krb5_child.c as the
> other modules will run unprivileged and accessing the keytab requires

typo:                                                  ^ccache^
> either privileges of root or the ccache owner.
>
> Makefile.am                                        |   4 +
>  src/providers/krb5/krb5_auth.c                     |  16 +-
>  src/providers/krb5/krb5_auth.h                     |   1 +
>  src/providers/krb5/{krb5_utils.c => krb5_ccache.c} | 720 +++++----------------
>  src/providers/krb5/{krb5_utils.h => krb5_ccache.h} |  49 +-
>  src/providers/krb5/krb5_child.c                    |   1 +
>  src/providers/krb5/krb5_common.h                   |   7 -
>  src/providers/krb5/krb5_renew_tgt.c                |   1 +
>  src/providers/krb5/krb5_utils.c                    | 672 +------------------
>  src/providers/krb5/krb5_utils.h                    |  15 -
>  src/tests/krb5_child-test.c                        |   1 +
>  src/tests/krb5_utils-tests.c                       |   1 +
>  12 files changed, 197 insertions(+), 1291 deletions(-)
>  copy src/providers/krb5/{krb5_utils.c => krb5_ccache.c} (57%)
>  copy src/providers/krb5/{krb5_utils.h => krb5_ccache.h} (53%)

...
>        /* Additional syntax from krb5.conf default_ccache_name */
>        case '{':
>            if (strncmp(n , S_EXP_UID, L_EXP_UID) == 0) {
>                action = 'U';
>                n += L_EXP_UID - 1;
>                rerun = true;
>                continue;
>            } else if (strncmp(n , S_EXP_USERID, L_EXP_USERID) == 0) {
>                action = 'U';
>                n += L_EXP_USERID - 1;
>                rerun = true;
>                continue;
>            } else if (strncmp(n , S_EXP_EUID, L_EXP_EUID) == 0) {
>                /* SSSD does not distinguish betwen uid and euid,
>                 * so we treat both the same way */
>                action = 'U';
>                n += L_EXP_EUID - 1;
>                rerun = true;
>                continue;
>            } else if (strncmp(n , S_EXP_USERNAME, L_EXP_USERNAME) == 0) {
>                action = 'u';
>                n += L_EXP_USERNAME - 1;
>                rerun = true;
>                continue;
>            } else {
>                /* ignore any expansion variable we do not understand and
>                 * let libkrb5 hndle it or fail */
>                name = n;
>                n = strchr(name, '}');
>                if (!n) {
>                    DEBUG(SSSDBG_CRIT_FAILURE, 

trailing whitespace                                  ^
>                          "Invalid substitution sequence in cache "
>                          "template. Missing closing '}' in [%s].\n",
>                          template);
>                    goto done;
>                }
>                result = talloc_asprintf_append(result, "%s%%%.*s", p,
>                                                (int)(n - name + 1), name);
>            }
>            break;
>        default:
>            DEBUG(SSSDBG_CRIT_FAILURE,
>                  "format error, unknown template [%%%c].\n", *n);
>            goto done;
>        }
>    }
>    if (result == NULL) {
>        DEBUG(SSSDBG_CRIT_FAILURE, "talloc_asprintf_append failed.\n");
>        goto done;
>    }
>    p = n + 1;
> }

...
> From 9a3732a1622fe4a172e0b753d1b5837691a7bd6d Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:01 +0200
> Subject: [PATCH 4/6] KRB5: Move checking for illegal RE to krb5_utils.c
>
> Otherwise we would have to link krb5_child with pcre and transfer the
> regex, which wold be cumbersome. Check for illegal patterns when

typo:          would
> expanding the template instead.
>
> src/providers/krb5/krb5_auth.c   |  5 +--
>  src/providers/krb5/krb5_ccache.c | 38 ++------------------
>  src/providers/krb5/krb5_ccache.h |  7 +---
>  src/providers/krb5/krb5_utils.c  | 36 +++++++++++++++++--
>  src/providers/krb5/krb5_utils.h  |  4 +--
>  src/tests/krb5_child-test.c      |  2 +-
>  src/tests/krb5_utils-tests.c     | 78 ++++++++++++++++------------------------
>  7 files changed, 73 insertions(+), 97 deletions(-)

...
> From f608986724901a441ec87cdd0e170d9bfb175c21 Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sat, 18 Oct 2014 22:03:13 +0200
> Subject: [PATCH 5/6] KRB5: Move all ccache operations to krb5_child.c
>
> The credential cache operations must be now performed by the krb5_child
> completely, because the sssd_be process might be running as the sssd
> user who doesn't have access to the ccaches.
>
> src/providers/krb5/krb5_ccache.c is still linked against libsss_krb5
> until we fix Kerberos ticket renewal as non-root.
>
> Also includes a new error code that indicates that the back end should
> remove the old ccache attribute -- the child can't do that if it's
> running as the user.
>
> Makefile.am                             |  13 +-
>  src/providers/krb5/krb5_auth.c          | 223 ++++----------------------------
>  src/providers/krb5/krb5_ccache.c        |  62 ++++-----
>  src/providers/krb5/krb5_ccache.h        |   5 +-
>  src/providers/krb5/krb5_child.c         | 208 +++++++++++++++++++++++++++--
>  src/providers/krb5/krb5_child_handler.c |  13 ++
>  src/tests/krb5_child-test.c             |   3 +-
>  src/util/util_errors.c                  |   1 +
>  src/util/util_errors.h                  |   1 +
>  9 files changed, 281 insertions(+), 248 deletions(-)

...
> From 8f0f7aba473f243e369589db46b09e690648e42b Mon Sep 17 00:00:00 2001
> From: Jakub Hrozek 
> Date: Sun, 19 Oct 2014 12:28:13 +0200
> Subject: [PATCH 6/6] KRB5: Do not switch_creds() if already the specified user
>
> The code didn't have to handle this case previously as sssd_be was always
> running as root and switching to the ccache as the user logging in.
>
> Also handle NULL creds on restore_creds() in case there was no switch.
> One less if-condition and fewer indentation levels.
>
> src/tests/cwrap/test_become_user.c |  7 +++++++
>  src/util/become_user.c             | 27 ++++++++++++++++++++-------
>  2 files changed, 27 insertions(+), 7 deletions(-)
>
> diff --git a/src/tests/cwrap/test_become_user.c b/src/tests/cwrap/test_become_user.c
> index 06d3ad425c4928e9e9bff661fbb8f7b4536b8896..7ecea5aac34bb73ca81d94ad481f05b338e65ed0 100644
> --- a/src/tests/cwrap/test_become_user.c
> +++ b/src/tests/cwrap/test_become_user.c
> @@ -76,6 +76,7 @@ void test_switch_user(void **state)
>      struct passwd *sssd;
>      TALLOC_CTX *tmp_ctx;
>      struct sss_creds *saved_creds;
>
> struct sss_creds *saved_creds2 = NULL;
>
> check_leaks_push(global_talloc_context);
> tmp_ctx = talloc_new(global_talloc_context);
>
> @@ -102,6 +103,12 @@ void test_switch_user(void **state)
>      assert_int_equal(saved_creds->uid, 0);
>      assert_int_equal(saved_creds->gid, 0);
> /* Attempt to restore creds again */
> ret = switch_creds(tmp_ctx, sssd->pw_uid, sssd->pw_gid,
>                   0, NULL, &saved_creds2);
> assert_int_equal(ret, EOK);
> assert_null(saved_creds2);
> /* restore root */
>    ret = restore_creds(saved_creds);
>    assert_int_equal(ret, EOK);
>
> diff --git a/src/util/become_user.c b/src/util/become_user.c
> index b5f94f993cd2c23bd3340fc502d36a530aa729fa..8fbc228ad93d16bd076d7920e22fb4b2f83417cb 100644
> --- a/src/util/become_user.c
> +++ b/src/util/become_user.c
> @@ -90,9 +90,14 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
>      struct sss_creds *ssc = NULL;
>      int size;
>      int ret;
>
> uid_t myuid;
>
> uid_t mygid;
>
> DEBUG(SSSDBG_FUNC_DATA, "Switch user to [%d][%d].\n", uid, gid);
>
> myuid = geteuid();
>
> mygid = getegid();
>
> if (saved_creds) {
>        /* save current user credentials */
>        size = getgroups(0, NULL);
>
> @@ -124,8 +129,8 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
>          }
>      /* we care only about effective ids */
>    ssc->uid = geteuid();
>    ssc->gid = getegid();
>    ssc->uid = myuid;
>    ssc->gid = mygid;
>
> }
>
> /* if we are regaining root set euid first so that we have CAP_SETUID back,
>
> @@ -143,6 +148,11 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
>  /* TODO: use prctl to get/set capabilities too ? */

not your patch, but maybe libcap should be added as an alternative to
the comment?
> if (myuid == uid && mygid == gid) {
>    DEBUG(SSSDBG_FUNC_DATA, "Already user [%"SPRIuid"].\n", uid);
>    return EOK;
> }
> /* try to setgroups first should always work if CAP_SETUID is set,
>     * otherwise it will always fail, failure is not critical though as
>     * generally we only really care about uid and at mot primary gid */
>
> @@ -177,11 +187,9 @@ errno_t switch_creds(TALLOC_CTX *mem_ctx,
>
> done:
>      if (ret) {
>    if (ssc) {
>        /* attempt to restore creds first */
>        restore_creds(ssc);
>        talloc_free(ssc);
>    }
>    /* attempt to restore creds first */
>    restore_creds(ssc);
>    talloc_free(ssc);
> } else if (saved_creds) {
>        *saved_creds = ssc;
>    }
>
> @@ -190,6 +198,11 @@ done:
>
> errno_t restore_creds(struct sss_creds *saved_creds)
>  {
> if (saved_creds == NULL) {
>    /* In case save_creds was saved with the UID already dropped */
>    return EOK;
> }
> return switch_creds(saved_creds,
>                        saved_creds->uid,
>                        saved_creds->gid,
>
> -- 
> 1.9.3
> sssd-devel mailing list
> [e-mail]
> https://lists.fedorahosted.org/mailman/listinfo/sssd-devel

[Sumit Bose]

On Mon, Nov 17, 2014 at 06:35:57PM +0100, Sumit Bose wrote:
> On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> > On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
>
> ...
> > > Thank you, see the attached patches.
> >
> > I forgot to remove the extra find_uid.c from Makefile.am
>
> I only have minor comments, see below. I understand that most part of
> the patches are refactorings to make sure the related code is run with
> the right permissions. (See next mail for some additional suggestions).

Currently with a bit more complex/secure setup with e.g. FAST and ticket
validation most of the krb5_child still runs as root.

I still haven't found a way to proper serialize keytab or ccache data
so that it can be send from one process to another, but both keytab and
ccache have a memory type. The three attached patches read the keytab
and for FAST the FAST ccache into a MEMORY type, drop the privileges and
run all other operations as unprivileged users.

For the ldap_child this is straight forward because only the keytab has
to be read. With a bit of refactoring it might be even possible to drop
the privileges earlier.

FAST makes the situation for the krb5_child a bit more complicated
because if there is no valid FAST ccache a whole kinit with the keytab
has to be run and it would be nice if the result can be read by other
krb5_child processes as well as long as the ticket is valid. What would
be possible is to read the keytab as root, switch to the SSSD user to
check the FAST ccache and eventually do a kinit to renew it, switch
back to root and finally drop all privileges and become the user. I'm
not sure about the switching to the SSSD user because we not really drop
the privileges because we can become root again. As an alternative we
can become the user immediately after reading the keytab and create the
FAST ccache only in memory. But this means that we always have to get a
fresh FAST ccache.

Please note that the patches are only a POC, tests are missing and there
might be some memory issues. Nevertheless it would be nice to know if
you think that this idea is worth to follow further.

bye,
Sumit
> The ccache handling always was delicate, e.g. directory creation with
> DIR type and determine if FILE with _XXXXXX should be recreated or
> reused. I run couple to tests without an issue but I think only
> regression testing might give clarity here. So ACK from my side.
>
> bye,
> Sumit

[Jakub Hrozek]

On Mon, Nov 17, 2014 at 08:45:26PM +0100, Sumit Bose wrote:
> On Mon, Nov 17, 2014 at 06:35:57PM +0100, Sumit Bose wrote:
> > On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> > > On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > > > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> >
> > ...
> > > > Thank you, see the attached patches.
> > >
> > > I forgot to remove the extra find_uid.c from Makefile.am
> >
> > I only have minor comments, see below. I understand that most part of
> > the patches are refactorings to make sure the related code is run with
> > the right permissions. (See next mail for some additional suggestions).
>
> Currently with a bit more complex/secure setup with e.g. FAST and ticket
> validation most of the krb5_child still runs as root.
>
> I still haven't found a way to proper serialize keytab or ccache data
> so that it can be send from one process to another, but both keytab and
> ccache have a memory type. The three attached patches read the keytab
> and for FAST the FAST ccache into a MEMORY type, drop the privileges and
> run all other operations as unprivileged users.
>
> For the ldap_child this is straight forward because only the keytab has
> to be read. With a bit of refactoring it might be even possible to drop
> the privileges earlier.
>
> FAST makes the situation for the krb5_child a bit more complicated
> because if there is no valid FAST ccache a whole kinit with the keytab
> has to be run and it would be nice if the result can be read by other
> krb5_child processes as well as long as the ticket is valid. What would
> be possible is to read the keytab as root, switch to the SSSD user to
> check the FAST ccache and eventually do a kinit to renew it, switch
> back to root and finally drop all privileges and become the user. I'm
> not sure about the switching to the SSSD user because we not really drop
> the privileges because we can become root again. As an alternative we
> can become the user immediately after reading the keytab and create the
> FAST ccache only in memory. But this means that we always have to get a
> fresh FAST ccache.
>
> Please note that the patches are only a POC, tests are missing and there
> might be some memory issues. Nevertheless it would be nice to know if
> you think that this idea is worth to follow further.

Absolutely!

You're right that with my patches the child processes still run as root
and this would be an awesome improvement.

About testing -- I'd like to summarize what's done and what's missing in
the design page as the immediate next step, then work on tests. I
submitted some previously, but they were a bit fragile. I guess when
ldap_child and krb5_child tests are in place, we can start merging your
patches..

[Jakub Hrozek]

On Mon, Nov 17, 2014 at 06:35:57PM +0100, Sumit Bose wrote:
> On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> > On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
>
> ...
> > > Thank you, see the attached patches.
> >
> > I forgot to remove the extra find_uid.c from Makefile.am
>
> I only have minor comments, see below. I understand that most part of
> the patches are refactorings to make sure the related code is run with
> the right permissions. (See next mail for some additional suggestions).
>
> The ccache handling always was delicate, e.g. directory creation with
> DIR type and determine if FILE with _XXXXXX should be recreated or
> reused. I run couple to tests without an issue but I think only
> regression testing might give clarity here. So ACK from my side.

Thank you for testing. Lukas was kind enough to run all available tests
our QE has at their disposal related to Kerberos. I hope that raises the
confidence level a bit.

The attached patches fix all the issues you raised previously.

[Sumit Bose]

On Tue, Nov 18, 2014 at 02:56:35PM +0100, Jakub Hrozek wrote:
> On Mon, Nov 17, 2014 at 06:35:57PM +0100, Sumit Bose wrote:
> > On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> > > On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > > > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> >
> > ...
> > > > Thank you, see the attached patches.
> > >
> > > I forgot to remove the extra find_uid.c from Makefile.am
> >
> > I only have minor comments, see below. I understand that most part of
> > the patches are refactorings to make sure the related code is run with
> > the right permissions. (See next mail for some additional suggestions).
> >
> > The ccache handling always was delicate, e.g. directory creation with
> > DIR type and determine if FILE with _XXXXXX should be recreated or
> > reused. I run couple to tests without an issue but I think only
> > regression testing might give clarity here. So ACK from my side.
>
> Thank you for testing. Lukas was kind enough to run all available tests
> our QE has at their disposal related to Kerberos. I hope that raises the
> confidence level a bit.
>
> The attached patches fix all the issues you raised previously.

Thank you, ACK.

bye,
Sumit

[Jakub Hrozek]

On Tue, Nov 18, 2014 at 05:43:40PM +0100, Sumit Bose wrote:
> On Tue, Nov 18, 2014 at 02:56:35PM +0100, Jakub Hrozek wrote:
> > On Mon, Nov 17, 2014 at 06:35:57PM +0100, Sumit Bose wrote:
> > > On Fri, Nov 14, 2014 at 01:52:20PM +0100, Jakub Hrozek wrote:
> > > > On Thu, Nov 13, 2014 at 07:30:41PM +0100, Jakub Hrozek wrote:
> > > > > On Wed, Nov 12, 2014 at 05:08:09PM +0100, Lukas Slebodnik wrote:
> > >
> > > ...
> > > > > Thank you, see the attached patches.
> > > >
> > > > I forgot to remove the extra find_uid.c from Makefile.am
> > >
> > > I only have minor comments, see below. I understand that most part of
> > > the patches are refactorings to make sure the related code is run with
> > > the right permissions. (See next mail for some additional suggestions).
> > >
> > > The ccache handling always was delicate, e.g. directory creation with
> > > DIR type and determine if FILE with _XXXXXX should be recreated or
> > > reused. I run couple to tests without an issue but I think only
> > > regression testing might give clarity here. So ACK from my side.
> >
> > Thank you for testing. Lukas was kind enough to run all available tests
> > our QE has at their disposal related to Kerberos. I hope that raises the
> > confidence level a bit.
> >
> > The attached patches fix all the issues you raised previously.
>
> Thank you, ACK.
>
> bye,
> Sumit

* master:
    35b4b217fa2b91bfc8d58c47024faf41c95fc807
    2745b0156f12df7a7eb93d57716233243658e4d9
    7c5cd2e7711621af9163a41393e88896a91ac33b
    45aeb924ec3ac448bb8d174a5cc061ed98b147c7
    476b78b3f66abc7a0f805154ea1a29f54628224a
    a60f4bb6b321298eb4d1c1c33d1897049a83d357