[Freeipa-devel] [PATCH] 0145: trust fix filtering of users from subdomains

[Simo Sorce]

On Tue, 2014-03-04 at 12:10 +0100, Sumit Bose wrote:
> On Tue, Mar 04, 2014 at 11:13:25AM +0200, Alexander Bokovoy wrote:
> > Attached patch should fix https://fedorahosted.org/freeipa/ticket/4207
> > where we didn't filter out users from disabled subdomains aggressively
> > enough.
> > 
> > The code that did not filter exists only in git, not in released
> > versions yet.
> > 
> > Attached also a screenshot that explains the behavior.
> > 
> > The code was tested by me, Sumit, and Scott, and reviewed over last few
> > days by Simo and Sumit.
> 
> This patch worked well in my test, AS and TGS requests for IPA user and
> IPA services went well, as TGS request from trusted users for IPA
> services. According to the krb5kdc logs delegation HTTP->ldap was ok as
> well.
> 
> I was not able to test S4U2Self and S4U2Proxy from the command line with
> kvno, but I think this might be expected since we currently do not
> support enterprise principals in IPA.
> 
> I still have one comment, see inline.
> 
> bye,
> Sumit
> 
> > 
> > -- 
> > / Alexander Bokovoy
> 
> > >From 9eafc57e54311d203254d84e3dab32261f1c9aba Mon Sep 17 00:00:00 2001
> > From: Alexander Bokovoy <abokovoy at redhat.com>
> > Date: Fri, 28 Feb 2014 22:03:29 +0200
> > Subject: [PATCH 9/9] fix filtering of subdomain-based trust users
> > 
> > https://fedorahosted.org/freeipa/ticket/4207
> > ---
> >  daemons/ipa-kdb/ipa_kdb_mspac.c | 41 ++++++++++++++++++++++++++++++++---------
> >  1 file changed, 32 insertions(+), 9 deletions(-)
> > 
> > diff --git a/daemons/ipa-kdb/ipa_kdb_mspac.c b/daemons/ipa-kdb/ipa_kdb_mspac.c
> > index 771b40b..cb1b68e 100644
> > --- a/daemons/ipa-kdb/ipa_kdb_mspac.c
> > +++ b/daemons/ipa-kdb/ipa_kdb_mspac.c
> > @@ -806,6 +806,12 @@ static krb5_error_code ipadb_get_pac(krb5_context kcontext,
> >      krb5_error_code kerr;
> >      enum ndr_err_code ndr_err;
> >  
> > +    /* When no client entry is there, we cannot generate MS-PAC */
> > +    if (!client) {
> > +        *pac = NULL;
> > +        return 0;
> > +    }
> > +
> >      ipactx = ipadb_get_context(kcontext);
> >      if (!ipactx) {
> >          return KRB5_KDB_DBNOTINITED;
> > @@ -1538,6 +1544,12 @@ static krb5_error_code ipadb_add_transited_service(krb5_context context,
> >      uint32_t i;
> >      char *tmpstr;
> >  
> > +    /* When proxy is NULL, authdata flag on the service principal was cleared
> > +     * by an admin. We don't generate MS-PAC in this case */
> > +    if (proxy == NULL) {
> > +        return 0;
> > +    }
> > +
> >      tmpctx = talloc_new(NULL);
> >      if (!tmpctx) {
> >          kerr = ENOMEM;
> > @@ -1735,6 +1747,12 @@ static krb5_error_code ipadb_verify_pac(krb5_context context,
> >      }
> >  
> >      if (flags & KRB5_KDB_FLAG_CONSTRAINED_DELEGATION) {
> > +        if (proxy == NULL) {
> > +            *pac = NULL;
> > +            kerr = 0;
> > +            goto done;
> > +        }
> > +
> >          kerr = ipadb_add_transited_service(context, proxy, server,
> >                                             old_pac, new_pac);
> >          if (kerr) {
> > @@ -1990,20 +2008,27 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
> >      krb5_db_entry *client_entry = NULL;
> >  
> >  
> > -    /* When client is NULL, authdata flag on the service principal was cleared
> > -     * by an admin. We don't generate MS-PAC in this case */
> > -    if (client == NULL) {
> > -        *signed_auth_data = NULL;
> > -        return 0;
> > -    }
> > +    is_as_req = ((flags & KRB5_KDB_FLAG_CLIENT_REFERRALS_ONLY) != 0);
> >  
> >      /* When using s4u2proxy client_princ actually refers to the proxied user
> >       * while client->princ to the proxy service asking for the TGS on behalf
> >       * of the proxied user. So always use client_princ in preference */
> >      if (client_princ != NULL) {
> >          ks_client_princ = client_princ;
> > -        kerr = ipadb_get_principal(context, client_princ, flags, &client_entry);
> > +        if (!is_as_req) {
> > +            kerr = ipadb_get_principal(context, client_princ, flags, &client_entry);
> > +            /* If we didn't find client_princ in our database, it might be:
> > +             * - a principal from another realm, handle it down in ipadb_get/verify_pac()
> > +             */
> > +            if (!kerr) {
> > +                client_entry = NULL;
> > +            }
> > +        }
> 
> If I understand it correctly client_princ should be always set so the
> else block below is rarely executed. For TGS request (!is_as_req)
> another LDAP search is done. I think this is done even in the case
> where an IPA client want's a ticket for an IPA service where the client
> pointer should already have the needed data. I see two identical
> requests in the LDAP logs in this case.
> 
> If there is a easy way to skip the second search for this type of
> request I would vote to include it, otherwise I think it can be kept as
> it is and the performance improvement can be added later.

We should probably compare client_princ and client->princ and skip
ipadb_get_principal() if they are the same.

But I think we can do this as a separate patch.

Simo.

-- 
Simo Sorce * Red Hat, Inc * New York