[PATCH] Improve krb5 KDC tests, kdc behaviour

[Stefan (metze) Metzmacher]

Hi Andrew,

do we have cross-realm tests for trusts yet?

metze

>>> Attached is some improvements to our KDC test script, and a fix for our
>>> KDC.  
>>>
>>> I still need to cover the canonicalize case for TGS-REQ, but this needs
>>> further work (I have to rework the code to use krb5_get_creds).
>>>
>>> Garming has reviewed it, but I wanted to see what you thought about it. 
>>
>> I've now covered the TGS-REQ case with canonicalize set, which you made
>> a change to in 51b94ab3fd4d13ee38813eb7d20db11edaa667a8 with:
>>
>>  s4:kdc: canonicalize the principal if HDB_F_FOR_TGS_REQ is given
>>     
>>  Windows seems to always canonicalize the principal in TGS replies.
>>     
>>  metze
>>
>> I can't reproduce this behaviour against Windows 2012R2 with some now
>> quite extensive tests, and so the patches I attach revert this, in
>> part.  
> 
> If you look at the resulting code of my commit:
> 
> 
> -               if (flags & HDB_F_CANON) {
> +               /*
> +                * Windows seems to canonicalize the principal
> +                * in a TGS REP even if the client did not specify
> +                * the canonicalize flag.
> +                */
> +               if (flags & HDB_F_CANON|HDB_F_FOR_TGS_REQ) {
>                         ret = krb5_copy_principal(context, principal,
> &alloc_principal);
>                         if (ret) {
>                                 return ret;
>                         }
> 
>                         /* When requested to do so, ensure that the
>                          * both realm values in the principal are set
>                          * to the upper case, canonical realm */
>                         free(alloc_principal->name.name_string.val[1]);
>                         alloc_principal->name.name_string.val[1] =
> strdup(lpcfg_realm(lp_ctx));
>                         if (!alloc_principal->name.name_string.val[1]) {
>                                 ret = ENOMEM;
>                                 krb5_set_error_message(context, ret,
> "samba_kdc_fetch: strdup() failed!");
>                                 return ret;
>                         }
>                         principal = alloc_principal;
>                 }
> 
> and the resulting code of your patch:
> 
> -               if (flags & (HDB_F_CANON|HDB_F_FOR_TGS_REQ)) {
> +               if (flags & (HDB_F_CANON)) {
> +                       ret = krb5_make_principal(context,
> &entry_ex->entry.principal,
> +                                                 lpcfg_realm(lp_ctx),
> "krbtgt",
> +                                                 lpcfg_realm(lp_ctx),
> NULL);
> +                       if (ret) {
> +                               krb5_clear_error_message(context);
> +                               goto out;
> +                       }
>                         /* When requested to do so, ensure that the
>                          * both realm values in the principal are set
>                          * to the upper case, canonical realm */
> 
> free(entry_ex->entry.principal->name.name_string.val[1]);
> 
> entry_ex->entry.principal->name.name_string.val[1] =
> strdup(lpcfg_realm(lp_ctx));
>                         if
> (!entry_ex->entry.principal->name.name_string.val[1]) {
>                                 ret = ENOMEM;
>                                 krb5_set_error_message(context, ret,
> "samba_kdc_fetch: strdup() failed!");
>                                 return ret;
>                         }
> => can't we remove the above? krb5_make_principal() already
>    sets entry_ex->entry.principal->name.name_string.val[1] to lpcfg_realm()
> 
> +                       krb5_principal_set_type(context,
> entry_ex->entry.principal, KRB5_NT_SRV_INST);
> +               } else {
> +                       ret = krb5_copy_principal(context, principal,
> &entry_ex->entry.principal);
> +                       if (ret) {
> +                               krb5_clear_error_message(context);
> +                               goto out;
> +                       }
> +                       /*
> +                        * this has to be with malloc(), and appears to be
> +                        * required regardless of the canonicalize flag from
> +                        * the client
> +                        */
> +                       krb5_principal_set_realm(context,
> entry_ex->entry.principal, lpcfg_realm(lp_ctx));
>                 }
> 
> You can see that the logic is a bit different, but we'll still
> somehow introduce the real lpcfg_realm() value...
> 
> This patch also reintroduces direct access to principal->name.name_type.
> 
> Can you also make sure we always check the return value of
> krb5_make_principal(),
> there're some places without a check.
> 
> As long as it passes the same tests as windows I'm fine, but I haven't
> looked closing
> at the test logic...
> 
> metze
> 

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 181 bytes
Desc: OpenPGP digital signature
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150203/94e89dc0/attachment.pgp>