[PATCH] Improve krb5 KDC tests, kdc behaviour

[Stefan (metze) Metzmacher]

Hi Andrew,

if Garming already reviewed, I'm fine now.

Thanks!
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...
> 
> Indeed.  I've fixed that up now, much simpler. 
> 
>> This patch also reintroduces direct access to principal->name.name_type.
> 
> Thanks.  
> 
>> Can you also make sure we always check the return value of
>> krb5_make_principal(),
>> there're some places without a check.
> 
> Thanks, I'll tidy that up.
> 
>> As long as it passes the same tests as windows I'm fine, but I haven't
>> looked closing
>> at the test logic...
> 
> The test logic will make your head spin, so don't look at it just after
> lunch :-)
> 
> Andrew Bartlett
> 

-------------- 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/6d47c383/attachment.pgp>