[PATCH] Improve krb5 KDC tests, kdc behaviour

[Andrew Bartlett]

On Tue, 2015-02-03 at 04:49 +0100, Stefan (metze) Metzmacher wrote:
> 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...

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

-- 
Andrew Bartlett
http://samba.org/~abartlet/
Authentication Developer, Samba Team  http://samba.org
Samba Developer, Catalyst IT          http://catalyst.net.nz/services/samba



-------------- next part --------------
A non-text attachment was scrubbed...
Name: upn-and-tests.patch
Type: text/x-patch
Size: 143839 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150203/d12a7cc3/attachment-0001.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20150203/d12a7cc3/attachment-0001.pgp>