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

[Sumit Bose]

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.

>      } else {
> +        if (client == NULL) {
> +            *signed_auth_data = NULL;
> +            return 0;
> +        }
>          ks_client_princ = client->princ;
>      }
>  
> @@ -2018,8 +2043,6 @@ krb5_error_code ipadb_sign_authdata(krb5_context context,
>                                    "currently not supported.");
>      }
>  
> -    is_as_req = ((flags & KRB5_KDB_FLAG_CLIENT_REFERRALS_ONLY) != 0);
> -
>      if (is_as_req && with_pac && (flags & KRB5_KDB_FLAG_INCLUDE_PAC)) {
>          /* Be aggressive here: special case for discovering range type
>           * immediately after establishing the trust by IPA framework */
> -- 
> 1.8.3.1
> 


> _______________________________________________
> Freeipa-devel mailing list
> Freeipa-devel at redhat.com
> https://www.redhat.com/mailman/listinfo/freeipa-devel