[PATCH] Improve krb5 KDC tests, kdc behaviour

[Stefan (metze) Metzmacher]

Hi Andrew,

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